fix(pad): carry held buttons across transient read misses (#272) - #288
Conversation
Holding a direction in the menus feels "heavy" and drops inputs on real hardware -- zackcage6 has to press repeatedly to move one item (#272) -- and coverflow scrolling sometimes jumps 2-3 entries (the remaining half of #271). ROOT CAUSE. readPads() zeroes the global `paddata` every frame and rebuilds it by OR-ing each pad that read successfully. padRead() returns 0 whenever freepad has nothing fresh, which on console happens routinely: the pads share the SIO2 bus with the memory-card slots and MMCE, so cover-art and config traffic makes reads intermittently come back empty. readPad() skipped the merge on a miss, so the held bits vanished for that frame, and two separate reported bugs fell out: 1. getKeyPressed() read false, so readPads()' loop reset delaycnt to the FULL initial delay. A hold re-armed the initial delay on every miss, so the auto-repeat leg often never fired -- the "sluggish and heavy" report. 2. The next good frame saw the bit set with the previous frame clear, so getKeyOn() fired a fresh EDGE. One physical hold produced extra discrete moves -- the "skips 2 to 3 games" report. This is why it reproduces on hardware and never in an emulator (reads do not fail there), and why it worsens with more games in the list (more art loads = more SIO2 contention). FIX. On a miss, if the pad still reports a ready state, OR its last known paddata into the global. getKeyPressed() then stays true so the repeat timer keeps running, and the bit never disappears so the recovery frame is not an edge. BOUNDED, because this synthesises input and must never invent a hold: - only while the pad reports ready (an unplug drops to DISCONN immediately, and the connect/disconnect handlers clear the carried state); - only while something was actually held; - only for PAD_READ_CARRY_MS (48 ms) of ACCUMULATED MISS TIME. Time, not frames: frames lengthen under the very contention that causes the misses, and some call sites poll readPads() more than once per frame, so a frame counter could mean several seconds. 48 ms sits under the fastest list repeat (100 ms), so a release landing inside a burst cannot manufacture list movement. rcode is set on the carry for the same reason a real read sets it: guiReadPads() treats readPads() == 0 as "user idle" and uses it to release background art/config IO. A carried hold reporting idle would schedule card traffic into exactly the SIO2 stall this branch exists to ride out. Residual, stated rather than hidden: the colour picker uses a 1 ms repeat, so a release inside a burst can still cost a couple of extra steps there. An earlier revision of this fix bounded the carry by frame count (30) and left rcode at 0. Adversarial review found that combination could emit up to 30 spurious repeats on release and told the art cache the user was idle mid-stall; both are fixed above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 participant
Fixes #272, and the remaining scroll-skip half of #271.
Root cause
readPads()zeroes the globalpaddataevery frame and rebuilds it by OR-ing each pad that read successfully.padRead()returns 0 whenever freepad has nothing fresh — which on console happens routinely, because the pads share the SIO2 bus with the memory-card slots and MMCE, so cover-art and config traffic makes reads intermittently come back empty.readPad()skipped the merge on a miss, so the held bits vanished for that frame.Two separately-reported bugs fall out of that one gap:
getKeyPressed()read false, soreadPads()' loop resetdelaycntto the full initial delay. A hold re-armed the initial delay on every miss, so the auto-repeat leg often never fired at all — zackcage6's "sluggish and heavy, I have to press the D-pad several times to move one item".getKeyOn()fired a fresh edge. One physical hold produced extra discrete moves — the "scrolling skips 2 to 3 games" half of [ISSUE]: Coverflow theme: Lag, clipping, and 3D effect dropping during scrolling #271.This explains two things the earlier investigations kept tripping over: why it reproduces on hardware but never in an emulator (reads don't fail there), and why it gets worse with more games in the list (more art loads = more SIO2 contention). zackcage6 reported it at 7 games.
Fix
On a miss, if the pad still reports a ready state, OR its last known
paddatainto the global.getKeyPressed()stays true so the repeat timer keeps running, and the bit never disappears so the recovery frame is not an edge.Bounded three ways, because this synthesises input and must never invent a hold the user isn't performing:
PAD_STATE_DISCONNimmediately, and the connect/disconnect handlers clear the carried state;PAD_READ_CARRY_MS(48 ms) of accumulated miss time.Time, not frames: frames lengthen under the very contention that causes the misses, and some call sites poll
readPads()more than once per frame, so a frame counter could mean anything from 50 ms to several seconds. 48 ms sits under the fastest list repeat (100 ms), so a release landing inside a burst cannot manufacture list movement.rcodeis set on the carry for the same reason a real read sets it:guiReadPads()treatsreadPads() == 0as "user idle" and uses it to release background art/config IO. A carried hold reporting idle would schedule card traffic into exactly the SIO2 stall this branch exists to ride out.Residual, stated rather than hidden: the colour picker uses a 1 ms repeat, so a release inside a burst can still cost a couple of extra steps there. Bounded and cosmetic, unlike the menu-wide input loss it replaces.
On the first revision
An earlier cut of this bounded the carry by frame count (30) and left
rcodeat 0. Adversarial review found that combination could emit up to 30 spurious repeats on release and would tell the art cache the user was idle mid-stall — trading one input bug for another. Both are fixed above; the review is why the bound is now in milliseconds.Verification
make clean && make opl.elfgreen onps2dev:latest;clang-formatclean🤖 Generated with Claude Code