Repository navigation
Conversation
BunnyFl
left a comment
BunnyFl
left a comment
There was a problem hiding this comment.
The connection-state fix and the per-sample fusion clock look right to me. Two of the new error returns don't match firmware 13.42, though, so I'm requesting changes.
Firmware 13.42's system/common/lib/libScePad.sprx (sha256 57eab7054e10d95ba0028dbd4808f94eb34c7663333a2b7ca9f68dc90cf62c60), read from disassembly, not measured on a console:
0x3090 scePadRead (q1cHNfGycLI): xor ecx, ecx ; jmp 0x1e50
0x1e50
1e9a: cmp dword [0x24018], 0 ; je 2112 ; 2112: mov edx, 0x80920005 (NOT_INITIALIZED)
1eaa: test rsi, rsi ; ... num-0x41 < 0xffffffc0 ; mov edx, 0x80920001 ; jne ; null data or num outside 1..64
... ; handle work only after these
0xf5e0 scePadSetVibrationForce (8BOObG94-tc)
f5fb: cmp dword [0x24018], 0 ; je -> 0x80920005
f604: test rsi, rsi ; je -> 0x80920001
f615: lock ; ... ioctl(fd, 0x80184822, {handle, large, small, ...}) ; unlock
return the ioctl result if negative
scePadReadchecks initialization first. In the module,NOT_INITIALIZEDcomes before the data and count check and before the handle is looked at. This PR puts it afterKnownHandle, the argument checks and the special-handle branch. So beforescePadInit, an unknown handle getsINVALID_HANDLEhere butNOT_INITIALIZEDon the console, and so does a special handle. Movingif (!Pad::IsInitialized()) return PAD_ERROR_NOT_INITIALIZED;to the top ofscePadRead, for every handle kind, matches the module.scePadSetVibrationForcewith a handle that has no pad port. The module never validates the handle itself. After the init and null checks, it passes the handle to the kernelioctl0x80184822and returns whatever that gives. ReturningPAD_ERROR_INVALID_HANDLEthere is a code the module doesn't produce; the kernel's answer isn't known.main'sNotImplementedfor that case was the honest choice, so I'd keep it. The module's order also puts the init check before the null-paramcheck, if you want to align that while you're there.
I didn't trace the scePadReadState body past its first checks (at 0x1a70 it rejects a null data with 0x80920001 first), so I haven't settled where its init check sits. I didn't build this branch; the points above come from the module.
|
Hi MarcZX10, Thanks for the per-port connection fix and the per-sample fusion clock; both read correctly to me, and the two new tests ( I agree with BunnyFl's review on the two error returns, and have one addition on the test side:
|
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Head
Queued motion clock and port 1 connection fixes: unchanged, as accepted. Verified on |
BunnyFl
left a comment
BunnyFl
left a comment
There was a problem hiding this comment.
@MarcZX10 thanks: scePadRead now checks initialisation first (Export.cpp:307), and the vibration-force case is back to NotImplemented. Here is the scePadReadState trace you asked for. Its init check sits elsewhere than at Export.cpp:324 on this head (details below); I'm requesting that one change.
Firmware 13.42 system/common/lib/libScePad.sprx (sha256 57eab7054e10d95ba0028dbd4808f94eb34c7663333a2b7ca9f68dc90cf62c60), read from disassembly, not measured on a console:
3080: 31 d2 xor %edx,%edx ; scePadReadState (YndgXqQVV7c)
3082: e9 e9 e9 ff ff jmp 0x1a70
1a70: 48 85 f6 test %rsi,%rsi ; data
1a73: 74 33 je 0x1aa8 ; 1aa8: mov $0x80920001,%eax ; ret
... ; 1adc-1b12: fill *data with defaults
1afa: 44 89 f1 mov %r14d,%ecx
1b19: e8 32 03 00 00 call 0x1e50 ; (handle, data, 1, 0): scePadRead's reader
1b1e: 85 c0 test %eax,%eax
1b20: 74 0d je 0x1b2f ; 0: continues in scePadReadState
1b22: 0f 88 1b 03 00 00 js 0x1e43 ; negative: returned as is
1e9a: 83 3d 77 21 02 00 00 cmpl $0x0,0x22177(%rip) ; 0x24018: initialised?
1ea1: 0f 84 6b 02 00 00 je 0x2112 ; 2112: mov $0x80920005,%edx (NOT_INITIALIZED)
1eaa: 48 85 f6 test %rsi,%rsi
1ead: 41 8d 57 bf lea -0x41(%r15),%edx
... ; 1eb1-1eba: sete/setb/or combine the two tests
1eb4: 83 fa c0 cmp $0xffffffc0,%edx ; num outside 1..64, or null data
1ebc: ba 01 00 92 80 mov $0x80920001,%edx
1ec1: 0f 85 95 10 00 00 jne 0x2f5c ; 2f5c: return %edx
1eda: e8 11 8d 01 00 call 0x1abf0 ; with 0x24028 (not read); the handle is used from 0x1ee7
So scePadReadState checks, in this order:
- a null
data→INVALID_ARG(0x1a70); - it fills
*datawith defaults; - not initialised →
NOT_INITIALIZED(0x1e9a), for every handle.0x1e50first uses the handle at0x1ee7, after the init and argument checks and a call to0x1abf0that I haven't read.
This head checks the handle first (Export.cpp:322), then data (:323), then initialisation only for handles that are neither remote nor special (:324). Before scePadInit, three cases differ from the module:
- an unknown handle with a valid
data→ the console givesNOT_INITIALIZED, this headINVALID_HANDLE; - an unknown handle with null
data→ the console givesINVALID_ARG, this headINVALID_HANDLE; - a remote or special handle → the console gives
NOT_INITIALIZED, this head fills*dataand returns 0.
Requested change: in scePadReadState, check data == nullptr first, then !Pad::IsInitialized() for every handle kind, then the handle. A test case per bullet above would pin it. Step 2 means the console also writes its defaults into *data before returning NOT_INITIALIZED. Matching that is optional; if you leave it out, the debt entry is the place to say so.
Not blocking, already on main: in scePadRead's shared reader the argument check also comes before the handle (0x1eaa-0x1ec1, then the lookup at 0x1ee7), and it rejects num above 64 as well as below 1 (lea -0x41; cmp $0xffffffc0; I checked the range by modelling those two instructions over -1000..1000: exactly 1..64 pass). This head checks the handle first (Export.cpp:308) and only num <= 0 (:309). Fine to leave for a follow-up.
What I ran. This head merged onto main 23d549a9c7 (merge 5bce0a95a9, clean), Windows 11 Pro, WinLibs GCC, Release:
guest_padandguest_pad_vibration_mode: 2 of 2 pass.- Negative control: with
main'sExport.cpp,guest_padaborts (1 of 2 failed), so the new early-read cases fail onmain. That's the run your Tested section listed as not done. tools/pr_overlap.py --branchat this head: no same exports. #16289 conflicts incore/libs/tests/GuestPad.cpponly. Nine PRs couldn't be compared (#5038, #8260, #12308-#12312, #13486, #14124); none of their GitHub patches changesscePadRead,scePadReadState,scePadSetVibrationForce, the connection check or the fusion clock.
AI-assisted: yes (Claude Code)
|
@MarcZX10 one addition to my review, so you don't need to ask for it: the defaults With On AnyPS5's
These stores ( What I haven't read, so I can't answer it yet:
AI-assisted: yes (Claude Code) |
|
@MarcZX10 here are the two paths my last comment (issuecomment-6690429806) listed as unread. I've now read them; what is still unread is at the end. Same module and sha256, read from disassembly, not measured on a console. Handles in the reader ( There is no separate branch for remote or special handles in this function: every handle goes through the same slot lookup. Whether the handles this PR calls remote or special get a slot depends on how they were opened, which I haven't traced.
Not read: AI-assisted: yes (Claude Code) |
Pad::Read builds each queued entry with the read time as the fusion clock, so only the first queued sample of a read advances the orientation; every later sample sees dt = 0 and its gyro data is dropped from the integration. Use the sample timestamp as the fusion clock for queued entries and keep the last fusion time monotonic. Fresh samples from ReadState and the empty-queue path still use the read time.
The connected flag of scePadGetControllerInformation was true for every handle on a port other than 0, and for port 0 it reflected port 0 regardless of the handle. It now reads the connection state of the port the handle belongs to.
…ePadSetVibrationForce A handle that passed the null check but has no pad port reached a NotImplemented throw. It now returns PAD_ERROR_INVALID_HANDLE, as the other handle-taking exports do.
…State before initialisation scePadOpen does not require scePadInit, so a standard handle can exist before initialisation. scePadRead and scePadReadState then reached Pad::Read and Pad::ReadState, which throw. They now return PAD_ERROR_NOT_INITIALIZED, as the other pad exports do.
… reads Cover the queued motion sample clock, the connection state of a port 1 handle, the vibration force handle error, and scePadRead and scePadReadState before initialisation. The queued motion check was not run against the unfixed code.
…ion force unimplemented without a pad port Firmware 13.42 checks the init state before the handle in scePadRead, and its scePadSetVibrationForce does not validate the handle itself, so the INVALID_HANDLE return is not what the module does. Restore the NotImplemented path.
…ad and drop the vibration handle expectation
…scePadReadState Co-Authored-By: Claude <noreply@anthropic.com>
d8cb5b5 to
a0e86f9
Compare
What
fix(libScePad):scePadGetControllerInformationreads the connection state of the handle's own port (Pad::IsConnected(PortOf(handle))). Before, every handle on port 1 or higher reported connected, and port 0 reported port 0's state regardless of the handle.fix(libScePad):scePadReadandscePadReadStatereturnPAD_ERROR_NOT_INITIALIZEDfor standard pad handles beforescePadInit.scePadReadchecks initialisation first, before the handle and argument checks, as firmware 13.42 does.scePadReadStatechecks it after its handle and argument checks; its order on the console is not traced yet.scePadOpendoes not require initialisation, so such a handle can exist, and the read then threw fromPad::Read/Pad::ReadState.fix(libScePad): queued motion samples are integrated at their own timestamps.Pad::Readused the read time as the fusion clock, so only the first queued sample of a read advanced the orientation.test(libScePad): covers the queued motion clock, the port 1 connection state, and the early reads (including an unknown handle beforescePadInit).Not in this PR: closing a standard pad handle does not make later handle exports return
PAD_ERROR_INVALID_HANDLE.KnownHandlestill accepts any port that was ever opened. Changing this also changes the pinned behaviour inGuestPad.cpp(a closed handle still acceptsscePadSetLightBar), and the console behaviour for closed handles is not measured here. This needs a maintainer decision and is left as a follow-up.Not in this PR:
Pad::Readclearing the button latch. The existingCheckLatchedPressLifetimetest pins that a press drained byscePadReadis not reported again byscePadReadState.Overlap: #6337 changes the timestamps of fresh samples in
ReadStateand the empty-queue path ofRead. This PR changes only the fusion clock of queued entries and does not touch those lines' semantics. #14124 rejects negative user IDs beforePortForUser, which covers the integer overflow item.Tested
run_tests.sharchive build with the host CPU gate patched), final HEADd8cb5b574f:guest_pad: passed (4.75 sec)guest_pad_vibration_mode: passedpython3 tools/check_conventions.py --base upstream/main: clean.git diff --check upstream/main: clean.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)