Repository navigation
fix(ppu): derive the H-IRQ dot from the clock, not a constant (T-06-A) - #300
Merged
Merged
Conversation
HIRQ_TRIGGER_DELAY = 4 was a dot-domain rounding of ares' `hcounter(10) == (HTIME+1)<<2`, exact only while every dot is four clocks -- which stopped being true when dots 323 and 327 became six. The match is now computed where it happens: clock 4*HTIME + 14, mapped to the first dot boundary at or after it. Below the long dots the two agree EXACTLY, because 4*HTIME + 14 is never a multiple of 4 and the next boundary is HTIME + 4. They diverge only for HTIME 321..=337, where the six-clock dots have displaced every later boundary: the old constant fired up to a whole dot late, and at HTIME 336 suppressed an IRQ that does fire. HTIME 337 lands on dot 340's boundary, which exists only on the long line, so it is honoured there and suppressed elsewhere. The plan records this change as attempted and reverted once because it moved hdmaen_latch_test_2's golden. It does not this time: no framebuffer golden moved, the undisbeliever suite passes unchanged, and cross-validation is byte-identical. B4.16 is a weaker guard than its own doc claimed. Measured either side of the change both its readings are unchanged, including the HTIME = 330 one whose trigger dot moved 334 -> 333: the CPU takes an IRQ at an instruction boundary, so the handler-entry dot quantises to the spin loop's instruction length and a one-dot shift is absorbed. It can say "nothing regressed"; it cannot say "the change took effect". The new unit test does that, sweeping every HTIME and asserting equality with the old constant below the long dots and strict inequality above them. LONG_DOTS and the per-dot clock count move to rustysnes-ppu, which owns the dot model and now needs the layout twice; the scheduler delegates rather than keeping a second copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 29 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. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Comment |
…ions The roadmap predicted the clock-domain conversion "would shift IRQ timing by ~2 clocks" and re-bless framebuffer goldens. It shifts nothing below the long dots and moved no golden; only HTIME 321..=337 changes. The plan predicted the change was "the likeliest thing under B1.05's residual". It is not, and B4.16 shows why directly: its HTIME = 330 trigger dot moved 334 -> 333 and its recorded reading did not move, because the handler-entry dot quantises to the spin loop's instruction length. B1.05 stays blocked on the same missing clock-domain instrument. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR refactors H-IRQ comparator timing in Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
Moves the HV-timer horizontal IRQ (H-IRQ) trigger from a dot-based constant to a clock-derived compare, so the model remains correct in the presence of 6-clock “long dots”, while keeping behavior unchanged below the long-dot region.
Changes:
- Replace
HIRQ_TRIGGER_DELAYwith clock-domainhirq_match_clockand derivedhirq_trigger_dot, and update the HV-IRQ comparator logic accordingly. - Centralize the long-dot layout in
rustysnes-ppu(LONG_DOTS+dot_clocks) and haverustysnes-coredelegate to it. - Update docs and changelog, and add targeted unit coverage asserting unchanged behavior below long dots and changed behavior above them.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/scheduler.md | Updates scheduler/HV-IRQ documentation to describe the clock-domain compare and its interaction with long dots. |
| docs/ppu.md | Updates PPU timing documentation to reflect clock-domain HV-IRQ behavior and long-line dot-count bounding. |
| crates/rustysnes-ppu/src/lib.rs | Implements clock-domain H-IRQ mapping, exports long-dot timing helpers, updates comparator logic, and adds unit tests. |
| crates/rustysnes-core/src/bus.rs | Removes duplicated long-dot table and uses rustysnes_ppu::dot_clocks for dot duration. |
| CHANGELOG.md | Documents the behavioral/model change and its safety/verification notes under Unreleased. |
Comment on lines
+162
to
+164
| // Walked rather than closed-form: the layout is two irregular dots in a 340-dot line, and a | ||
| // closed form would have to encode their positions a second time. `DOTS_PER_LINE + 1` covers | ||
| // the long line's extra dot; the loop is const-evaluable and runs at most 341 steps. |
| for htime in 321..=336u16 { | ||
| assert!( | ||
| hirq_trigger_dot(htime, false) < htime + 4, | ||
| "HTIME {htime} is past the long dots and must now fire EARLIER than the old constant said, not at the same dot" |
doublegate
added a commit
that referenced
this pull request
Aug 3, 2026
…ed against Three findings from the second review round, all real. CodeRabbit caught two errors in my CHANGELOG prose. 14.34 -> 7.03 ms is a 51% reduction, not 47% -- the 47% was Criterion's own change-against-saved-baseline line, which is a different comparison than the sentence was making, and I quoted it as though it were the same one. And 7.03 ms is 3% ABOVE the 6.83 ms measured before #300, so "back to the pre-regression baseline" overstated it; the remaining gap is the other commits that landed in the same window. Both corrected, with the correction stated rather than silently applied. Antigravity caught that fuzz/run.sh line 175 still invoked `cargo +nightly fuzz run` -- the exact `+toolchain` form the five-line comment I had just added a hundred lines above argues is unsafe. Fixing the rustc call and leaving the cargo call is the same shape as re-implementing a gate instead of reusing the one already settled: the reasoning was written down and then not applied. Now `rustup run nightly cargo fuzz run`. The error string that still cited `rustc +nightly -vV` is updated too. Campaign re-verified through the new invocation: rom_header clean at cov: 760 ft: 982. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Aug 3, 2026
#333) * fix(ppu,fuzz): the frame-time regression was real; the fuzz campaign never ran Two red CI gates, two genuine findings -- and neither was a flaky gate. FRAME TIME. The gate has been red since 2026-08-01 and it was right: headless frame production had halved. check_hv_irq runs once per dot, ~89,000 times a frame, and #300 had it walk the scanline from dot 0 on every call to find the comparator's dot -- up to 341 steps, ~30 million iterations a frame. Measured: 6.83 ms/frame before that commit, 13.31 ms after, against a 16.64 ms NTSC deadline. `git bisect run` over the 22-commit window named it exactly. The walk now starts from a lower bound rather than dot 0. Every dot is at least 4 clocks, so the answer cannot be below ceil((target - 4) / 4), and from there it converges in at most two steps. The `- 4` is load-bearing: a bound of ceil(target / 4) OVERSHOOTS for targets landing just past dot 323, where the two 6-clock dots make the prefix exceed 4 * dot. The target is also computed inside the irq_enable_h branch instead of above it, since the V-only arm never reads it, so a ROM using no H-IRQ pays nothing. 14.34 ms -> 7.03 ms, a 47% improvement, back to the pre-regression baseline. Safety is an exhaustive test against the ORIGINAL function verbatim for every HTIME on both line lengths -- comparing the change with what it replaced rather than with a belief about what it replaced. Battery 56/56, framebuffer goldens unmoved, 68 workspace suites green. FUZZING. The infrastructure had never actually run a campaign: the job is skipped on every push and PR and runs only on the weekly cron, so 2026-08-03 was its first real execution -- and all 14 targets reported a FINDING within about a second each, with fuzz/artifacts/ empty. run.sh's own header already names that failure mode for a different cause: "A campaign that reports 14 findings and has actually found none is worse than one that reports nothing." Here the cause is that cargo fuzz defaults --target to the triple the cargo-fuzz BINARY was built for. CI installs it through taiki-e/install-action, which ships a static musl build, so on a gnu runner every target failed with `sanitizer is incompatible with statically linked libc` and `can't find crate for core` -- and run.sh counts a non-zero exit as a finding, because a build failure and a crash look alike. It never reproduced locally because a cargo-installed cargo-fuzz is a gnu build whose default is already right. run.sh now passes --target explicitly from `rustc +nightly -vV`. Verified with a real campaign: rom_header clean at cov: 759 ft: 978, where before it exited in under a second having built nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ppu,fuzz): adjudicate the review — clamp before the cast, drop `rustc +nightly` Two reviewers (Copilot and Antigravity) independently flagged the same cast, and Antigravity's description is the accurate one: the truncation does not produce a wrong ANSWER, it degrades back to a full 341-step walk. Verified over the whole u16 domain before changing anything -- 98,308 HTIME values truncate and ZERO disagree with the reference, because a real match exists only for htime <= 337, where nothing truncates, and truncation only ever lowers the start. Fixed anyway, and the reason is sharper than "sloppy cast": irq_h is restored from a save state by read_u16() with no masking, and a save_state fuzz target reaches it. A malformed state therefore sends the walk back to scanning all 340 dots, per dot, 89,000 times a frame -- the exact pathology this function was rewritten to remove, reachable from untrusted input. The bound is now computed in u32 and clamped before the cast. The exhaustive test now covers the full u16 domain rather than stopping at 1023. That prefix was the problem: above every HTIME hardware can produce and below every one that overflows the arithmetic, so it could not have caught this. "No ROM can set that" is not a bound this function gets to assume when the value arrives from a save state. Also `rustup run nightly rustc -vV` rather than `rustc +nightly -vV`. The `+toolchain` form is parsed by rustup's shim, not by rustc, so it fails wherever rustc on PATH is a real binary -- a distro toolchain, or a container without the wrapper. It works on this machine only because rustc there IS the shim, which is precisely the kind of environment-dependence that hid the musl bug. run.sh already requires rustup, so `rustup run` costs nothing. Declined: the CHANGELOG:112 nitpick. That line is inside this PR's own fuzz entry; the A6.15 text it matched is [Unreleased] content accumulated from #331. Campaign re-verified: rom_header clean at cov: 760 ft: 979. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs,fix(fuzz): correct my own arithmetic; stop using the form I argued against Three findings from the second review round, all real. CodeRabbit caught two errors in my CHANGELOG prose. 14.34 -> 7.03 ms is a 51% reduction, not 47% -- the 47% was Criterion's own change-against-saved-baseline line, which is a different comparison than the sentence was making, and I quoted it as though it were the same one. And 7.03 ms is 3% ABOVE the 6.83 ms measured before #300, so "back to the pre-regression baseline" overstated it; the remaining gap is the other commits that landed in the same window. Both corrected, with the correction stated rather than silently applied. Antigravity caught that fuzz/run.sh line 175 still invoked `cargo +nightly fuzz run` -- the exact `+toolchain` form the five-line comment I had just added a hundred lines above argues is unsafe. Fixing the rustc call and leaving the cargo call is the same shape as re-implementing a gate instead of reusing the one already settled: the reasoning was written down and then not applied. Now `rustup run nightly cargo fuzz run`. The error string that still cited `rustc +nightly -vV` is updated too. Campaign re-verified through the new invocation: rom_header clean at cov: 760 ft: 982. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
The last of the
v1.29.0dot-model residuals, and the one the plan records as "attempted and reverted because it moveshdmaen_latch_test_2's golden". It does not, this time.What was wrong
HIRQ_TRIGGER_DELAY = 4was a dot-domain rounding of ares'hcounter(10) == (HTIME+1)<<2(sfc/cpu/irq.cpp,sfc/cpu/io.cpp) — exact only while every dot is four clocks, which stopped being true when dots 323 and 327 became six. The match is now computed where it actually happens: clock4·HTIME + 14(hirq_match_clock), mapped to the first dot boundary at or after it (hirq_trigger_dot). The compare is in clocks; the six-clock dots do not move it, they move which dot contains it.Why nothing moved
4·HTIME + 14is never a multiple of 4, so below the long dots the next boundary isHTIME + 4— identical to the old constant. The two diverge only forHTIME321..=337:HTIME≤ 320HTIME + 4HTIME321..=335HTIME + 4HTIME336HTIME337That is why no framebuffer golden moves. Every H-IRQ a game arms in the visible window is below the long dots.
B4.16is a weaker guard than its own doc claimedThe plan says the guard comes first, and
B4.16already existed and is blessed. Measured either side of this change, both of its readings are unchanged — including theHTIME = 330one, whose trigger dot moved 334 → 333. The CPU takes an IRQ at an instruction boundary, so the handler-entry dot quantises to the spin loop's instruction length and a one-dot shift is absorbed.So
B4.16can say "nothing regressed", which is what a guard is for, but it cannot say "the change took effect".the_h_irq_dot_is_unchanged_below_the_long_dots_and_moves_above_themdoes that: it sweeps everyHTIME, asserting equality with the old constant below the long dots and strict inequality above them. The finding is recorded at bothhirq_trigger_dotand indocs/scheduler.mdso a future reader does not take a staticB4.16as evidence the model did not move.Also
LONG_DOTSand the per-dot clock count move torustysnes-ppu, which owns the dot model and now needs the layout in two places.rustysnes-core's scheduler delegates to it rather than keeping a second copy — two statements of "which dots are six clocks" is exactly the kind of fact that drifts.Verification
cargo fmt --check,cargo clippy --workspace --exclude rustysnes-android --all-targets -- -D warnings— clean.cargo test -p rustysnes-ppu -p rustysnes-core— 143 passed, 0 failed.cargo test -p rustysnes-test-harness --test undisbeliever_golden --features test-roms— passes unchanged. This is the suite holdinghdmaen_latch_test/hdmaen_latch_test_2.REF_PROJ=$PWD/ref-proj bash scripts/accuracysnes/crossval.sh—snes9x: OK (14 known),Mesen2: OK (2 known), 53 scenes match on both,2 reference(s) agree with the cart. Byte-identical to before the change.B4.16was measured by temporarily restoring the old constant, not inferred.🤖 Generated with Claude Code