fix(pad): give edge detection its own read-miss carry (#271) - #289
Merged
Conversation
Beta-3442 hardware result (zackcage6, video): the HOLD half of #272 is fixed -- "the list now repeats smoothly with no lag or sluggish feel". Discrete taps still misbehave: "scrolling left/right still does jump 2-3 games and hangs on 2-3 games", and crucially "i do not hang or pause during scrolling, my scroll pattern is continuous, ONE BUTTON PRESS AT A TIME, this issue does occur also in settings menu". That rules out most of what was being chased. It is not key-repeat (he is tapping, and the repeat path is confirmed working), and it is not coverflow (it happens in the settings dialog too). Since getKey() tests getKeyOn() FIRST and that branch re-arms delaycnt, a long poll gap can never reach the repeat leg on a fresh press -- so every extra move is a separate getKeyOn EDGE, not a repeat. ROOT CAUSE -- introduced by the #272 carry itself. paddata serves two detectors with opposite failure modes, and they were sharing one bounded carry: * LEVEL (getKeyPressed -> delaycnt): over-carrying INVENTS auto-repeats, so it must stay bounded. PAD_READ_CARRY_MS is load-bearing and is what fixed the hold case. * EDGE (getKeyOn/getKeyOff): under-carrying INVENTS presses. With a ~150ms tap and 33ms frames, the 48ms budget expires mid-press: F0 read OK, LEFT down -> getKeyOn -> move #1 F1 miss, carry (33ms) -> no edge F2 miss, carry (66ms) -> no edge F3 miss, 66 >= 48 -> DROPPED -> global paddata falls to 0, finger still down F4 read OK, LEFT STILL DOWN -> 0 -> 1 -> getKeyOn -> move #2 from ONE press FIX. Split the two views. edgedata/oldedgedata carry a missed poll's bits with NO time bound and feed only getKeyOn/getKeyOff; paddata keeps its bounded carry and feeds everything else. Safe by construction in the direction that matters: an unbounded carry on the EDGE view can only SUPPRESS a second edge, never invent one, because an edge needs a 0 -> 1 transition and carrying a 1 forward cannot produce that. It self-corrects on the first successful read showing the button clear, and it also reduces dropped presses, since a miss no longer advances the edge state. NOT raising PAD_READ_CARRY_MS, which looks like the obvious fix and is wrong: to bridge a whole tap the window must exceed the user's RELEASE gap, at which point the carry still asserts the previous tap when the next arrives, the 0 -> 1 never happens, and presses get eaten. That trades jumps for hangs. Residual, stated rather than hidden: if a genuine release is never sampled at all (the whole release gap falls inside a GUI-thread stall) the next press is suppressed -- an extra move becomes a dropped one. Strictly the better failure, but real, and it does not repair the separate "tap falls entirely between two polls" path. That one needs the frame stalls themselves addressed; the prime suspect is initializePad() running inline on the GUI thread from readPad() (~250-300ms), which is already instrumented as SH on the debug HUD. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (17)
🔇 Additional comments (5)
📝 WalkthroughWalkthrough
ChangesPad edge-state handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #288. Addresses the remaining half of #271.
What Beta-3442 told us
zackcage6, with video:
That last line rules out most of what was being chased. It is not key-repeat (he's tapping, and the repeat path is confirmed working) and not coverflow (it happens in the settings dialog too).
It also rules it out on code grounds, not just statistics:
getKey()testsgetKeyOn()first, and that branch re-armsdelaycnt(pad.c:1019-1024). A long poll gap therefore can never reach the repeat leg on a fresh press. Every extra move is a separategetKeyOnedge.Root cause — introduced by #288's own carry
paddataserves two detectors whose failure modes are opposite, and they were sharing one bounded carry:getKeyPressed→delaycntgetKeyOn/getKeyOffThe 48 ms bound is load-bearing for the level view — it is what makes the hold case correct. But for a ~150 ms tap at 33 ms frames it expires mid-press:
Fix
Split the views.
edgedata/oldedgedatacarry a missed poll's bits with no time bound and feed onlygetKeyOn/getKeyOff;paddatakeeps its bounded carry and feeds everything else.Safe by construction in the direction that matters: an unbounded carry on the edge view can only ever suppress a second edge — it cannot invent one, because an edge requires a
0 → 1transition and carrying a1forward never produces that. It self-corrects on the first successful read showing the button clear, and it reduces dropped presses too, since a miss no longer advances the edge state.Why not just raise
PAD_READ_CARRY_MSBecause it looks obvious and is wrong. To bridge a whole tap the window must exceed the user's release gap — at which point the carry is still asserting the previous tap when the next one arrives, the
0 → 1never happens, and presses get eaten. That trades jumps for hangs.Residual — stated, not hidden
If a genuine release is never sampled at all (the whole release gap falls inside a GUI-thread stall), the following press is suppressed: an extra move becomes a dropped one. Strictly the better failure, but real.
This also does not repair the separate "tap falls entirely between two polls" path. That needs the frame stalls themselves addressed — prime suspect is
initializePad()running inline on the GUI thread fromreadPad()(pad.c:485), ~250-300 ms on the healthy path. Already instrumented: it bumpsSHon the debug HUD, so it can be tested with no new build.Verification
make clean && make opl.elfgreen onps2dev:latest;clang-formatcleanedgedatais BSS-zero exactly likepaddata🤖 Generated with Claude Code
Summary by CodeRabbit