test(accuracysnes): C7.05 — Range Over trips at OAM.INDEX * 2 - #285
Conversation
A fixed bracket would be vacuous. An H-IRQ is serviced ~22-27 dots after its HTIME, so a bracket tight enough to pin dot 65 sits inside the latency's own uncertainty, and one loose enough to be safe passes for any set dot across most of the line -- the F1.09 shape. What pins H = OAM.INDEX * 2 is that the set dot must MOVE with the index. Both phases sample the same dot and change only which OAM entry is the 33rd in-range sprite: index 32 (set dot 65, reads set) against index 72 (set dot 145, reads clear). Phase B also re-reads $213E in the same frame's vblank, because "clear at dot 105" is otherwise exactly what a core that never sets the flag would report; the flags clear at frame start, not on read, so the later read sees the same frame. Three injections, each failing its own code: - a fixed set dot -> code 2 - never setting the flag -> code 1 - correct for a low index, never for high -> code 3, the guard phase A cannot cover New runtime machinery, which no v1.28.0 row was supposed to need. irq_trampoline is a bank-local jmp (V_IRQ_VEC), so only bank-$00 groups could install an IRQ handler at all; Groups C-G are relocated out of bank $00 and none ever had one. irq_far_shim (bank $00) plus the 24-bit V_IRQ_VEC_FAR at $0058 fixes that for every relocated group: jml pushes nothing, so a far handler still ends in a plain rti. Opt-in, and the far vector defaults to irq_stub, so default behaviour is unchanged. snes9x fails the row and is recorded as an expected divergence (SNES9X_KNOWN_FAILURES 12 -> 13): it reads set in both phases, so its Range Over is scanline-granular rather than per-sprite. RustySNES's position is anchored to MesenCE on the LINE by the eval-line probe; the DOT is documentation-anchored only, because the Mesen2 headless runner times out in this environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 3 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 ignored due to path filters (3)
📒 Files selected for processing (3)
WalkthroughThis PR adds AccuracySNES C7.05 coverage for index-dependent sprite Range Over timing. It adds far IRQ dispatch for relocated tests, registers the new assertion, and records snes9x’s known divergence. ChangesAccuracySNES C7.05
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant c7_05
participant setup_over_flag_sprites
participant arm_hv_irq_and_run
participant irq_far_shim
participant PPU213E
c7_05->>setup_over_flag_sprites: Configure sprite layout
c7_05->>arm_hv_irq_and_run: Schedule H/V IRQ and render frame
arm_hv_irq_and_run->>irq_far_shim: Dispatch IRQ handler
irq_far_shim->>PPU213E: Sample $213E
PPU213E-->>c7_05: Return Range Over state
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/accuracysnes-plan.md`:
- Around line 499-506: Update the C7.05 guidance around the moving-index probe
to remove or revise the later Lines 545-549 instruction that describes its rows
as a fixed bracket. Explicitly state that C7.05 samples the same dot while
moving the OAM index, and separately identify whether C7.06 uses a bracket.
- Around line 505-519: Align the C7.05 validation documentation and harness
records with the assertions actually executed: in
docs/accuracysnes-plan.md:505-519, add validation samples around dots 65 and 145
or narrow the coverage claim; in CHANGELOG.md:14-24, describe index-dependent
movement without claiming exact timing; and in
scripts/accuracysnes/crossval.sh:150-158, update the harness record to match the
cartridge assertions. Preserve the distinction between index 32 being set by
approximately dot 105 and index 72 being clear there but set by vblank.
In `@tests/roms/AccuracySNES/asm/runtime.inc`:
- Around line 317-337: Relocate V_IRQ_VEC_FAR from $0058-$005A to an unclaimed
three-byte low-RAM range that does not overlap V_BANKPROBE, V_BANKPROBE_RET, or
V_STR_PTR, and update every reference, reset initialization, and irq_far_shim
access accordingly. Correct the surrounding memory-map comment to document the
complete allocation through the bankprobe and string-pointer blocks and identify
the actual next free byte.
In `@tests/roms/AccuracySNES/gen/src/tests/ppu.rs`:
- Around line 2484-2506: Update the IRQ setup around setup_over_flag_sprites to
anchor execution at vblank before enabling and arming the display IRQ, then
retain the wait needed to cross line 100 while armed. Replace the $FF poison
with a value that cannot equal either expected phase result, and validate the
raw handler state (or a separate run marker) before masked comparisons so a
handler that never runs is distinguishable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 865fab98-f6bc-447d-8485-2c834d5b87f4
⛔ Files ignored due to path filters (8)
docs/accuracysnes-coverage.mdis excluded by!docs/accuracysnes-coverage.mdand included bydocs/**tests/roms/AccuracySNES/ERROR_CODES.mdis excluded by!tests/roms/AccuracySNES/ERROR_CODES.mdand included bytests/**tests/roms/AccuracySNES/SOURCE_CATALOG.tsvis excluded by!**/*.tsv,!tests/roms/AccuracySNES/SOURCE_CATALOG.tsvand included bytests/**tests/roms/AccuracySNES/asm/tests_group_a.sis excluded by!tests/roms/AccuracySNES/asm/tests_group_a.sand included bytests/**tests/roms/AccuracySNES/build/accuracysnes-exhirom.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/accuracysnes-hirom.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/accuracysnes-pal.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/accuracysnes.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**
📒 Files selected for processing (7)
CHANGELOG.mddocs/accuracysnes-plan.mdscripts/accuracysnes/crossval.shtests/roms/AccuracySNES/asm/runtime.inctests/roms/AccuracySNES/asm/runtime.stests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (17)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
scripts/accuracysnes/crossval.shtests/roms/AccuracySNES/asm/runtime.inctests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/asm/runtime.sCHANGELOG.mddocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/tests/ppu.rs
scripts/accuracysnes/**
⚙️ CodeRabbit configuration file
scripts/accuracysnes/**: The cross-validation harness: the same AccuracySNES image is run on snes9x (through a
libretro host in C) and on Mesen2 (through its test runner and a Lua script), and their
verdicts are compared with the cart's. Its integrity is the whole argument for the
battery, so flag anything that could make a reference appear to agree — a verdict parsed
loosely, a missing-file path that degrades to success, a scene comparison that skips
rather than fails when the golden is absent. A known reference divergence belongs in
SNES9X_KNOWN_FAILURESwith a source citation, never in a widened match.
Files:
scripts/accuracysnes/crossval.sh
tests/roms/AccuracySNES/asm/runtime.inc
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/asm/runtime.inc: Shared symbol definitions. Every address here is claimed by something; a new variable that
overlaps an existing block, or a block whose stated width is smaller than what the code
stores into it, is the failure mode. Check the arithmetic ofVAR_BASE + $nnoffsets
against their declared sizes and against the neighbouring definitions.
Files:
tests/roms/AccuracySNES/asm/runtime.inc
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI runs with-D warnings. Document every public item.
Keepunsafecode restricted to the frontend and FFI, and include a// SAFETY:justification for each use.
Keep hot paths allocation-free.
Treatrustysnes_core::Busas the owner of mutable emulator state; the CPU borrows&mut Bus.
Use the master clock at 21477270 Hz as the timing master; advance the scheduler in lockstep and run other chips on their divisors.
Maintain determinism: seed, ROM, and input must produce bit-identical audio/video; frontend rate control must not alter emulation results.
When implementing hardware behavior, pin and run the failing test ROM first; treat test ROMs as the specification.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rsCHANGELOG.mddocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/tests/ppu.rs
tests/roms/AccuracySNES/gen/src/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, when a test or scene is added or removed under
tests/roms/AccuracySNES/gen/src/, verify itsdossier.rs::MAPentry, all required regenerated artifacts, and matching count changes indocs/accuracysnes-plan.md. Artifact presence must be judged from the path-filter exclusion list, not from unreadable contents.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Additive features must be default-off so shipped/native,no_std, and wasm builds remain byte-identical.
Never use or configure--all-features; validate opt-in feature combinations individually as required by the project recipe.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
tests/roms/AccuracySNES/gen/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rebuild AccuracySNES after any change to
gen/orasm/; never hand-edit generatedasm/tests_group_a.sorasm/scenes.s.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
tests/roms/AccuracySNES/gen/src/**
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/gen/src/**: This generates a hardware-accuracy test cartridge. Judge each test by whether it can
distinguish the behavior it names from the alternatives, not by whether it passes.Flag, specifically:
- Vacuity. An assertion whose expected value is also what a broken or absent
implementation produces (zero, "unchanged", "not $FF") needs a paired control assertion
that would fail on that implementation. Say which alternative goes uncaught.- Overstated doc comments. The prose above a test is a claim about what it validates.
If it names behaviors the emitted program does not exercise, or asserts a rationale that
is not true of the code, that is a defect even though the test passes.- Shared state. OAM, CGRAM, VRAM and the S-DSP registers are not reset between tests. A
test that does not establish its own starting conditions may be measuring the previous
one; look for an earlier test that leaves the relevant state dirty.- Timing-marginal reads. Reading a register a few cycles after disturbing it, or
asserting on a value that is still moving, produces a verdict that flips when unrelated
code shifts. Prefer a settle, a disarm, or a provably stationary value.- Scanline geometry. Line 0 is a blanking line; the V counter's low byte aliases on a
312-line PAL frame; the visible height is 224 or 239 depending on overscan. Constants
derived from any of these deserve a second look.- Duplicate coverage.
dossier.rs::MAPmust not claim an assertion another test already
implements. There is a build gate for this, but flag it in review too.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
tests/roms/AccuracySNES/asm/**/*.s
📄 CodeRabbit inference engine (AGENTS.md)
tests/roms/AccuracySNES/asm/**/*.s: Do not hand-editasm/tests_group_a.sorasm/scenes.s; regenerate them withcargo run -p accuracysnes-gen.
Use assertion helpers for every verdict, including non-equality conditions; never hand-write a verdict byte such assta V_TEST_RESULT.
Do not let a guard subsume the assertion it protects; mask guards so the assertion remains independently capable of failing, and validate named bug injections.
Inject faults at the site named by the test, not at another site that merely changes the observed result.
When a result depends on timing phase, record it and score nothing; otherwise widen comparisons by one sampling interval when appropriate. Use the NTSC/PAL drift gate as a tripwire.
Wait for results to become valid before measuring them; preserve required waits such as the vblank/automatic joypad-read delay.
Use the measurement channel without slot collisions;dossier::check_slotsmust pass, and widen the channel when it is full.
Keep Groups A and B in bank$00; do not relocate them, because their tests depend on program-bank placement and access speed.
Do not place a segment across a bank boundary; use_farwrappers for relocated groups and.loword(...)for address immediates when a group leaves bank$00.
Assembly helpers must be width-neutral (php/plp); emit correct.a8/.a16directives after everysep/rep.
Every APU test program must callrelease_to_ipl, which re-maps the IPL ROM before returning control.
Bound every APU handshake wait; if the APU does not answer, report SKIP rather than hanging the battery.
Files:
tests/roms/AccuracySNES/asm/runtime.s
tests/roms/AccuracySNES/**/*.s
📄 CodeRabbit inference engine (AGENTS.md)
Respect the Group F input contract: runners must hold both controllers for the entire run using
PAD_CONTRACT = $9050andPAD2_CONTRACT = $60A0; runF1.07before arming auto-read.
Files:
tests/roms/AccuracySNES/asm/runtime.s
tests/roms/AccuracySNES/asm/runtime.s
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/asm/runtime.s: Hand-written 65816. The battery runs under forced blank throughout; anything that lifts it
must put it back. Helpers are width-neutral (php/plp) by convention — flag one that is
not, because ca65's.a8/.a16state is file-global and a mismatch emits silently wrong
instruction lengths.
Files:
tests/roms/AccuracySNES/asm/runtime.s
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
CHANGELOG.mddocs/accuracysnes-plan.md
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/accuracysnes-plan.md
docs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/accuracysnes-plan.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/accuracysnes-plan.md
🧠 Learnings (6)
📚 Learning: 2026-07-22T12:38:06.947Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 206
File: tests/roms/AccuracySNES/asm/runtime.inc:0-0
Timestamp: 2026-07-22T12:38:06.947Z
Learning: In `tests/roms/**/asm/runtime.inc`, when adding “low-RAM” runtime variables, verify that the new variable’s VAR_BASE-relative range/map does not overlap any existing variable declarations (including VAR_BASE + offsets ranges), regardless of declaration order. Do not rely on the measurement-slot collision gate as it may not cover the full variable map (e.g., `V_CURSOR_PREV` owns `VAR_BASE + $2A` through `$2B` even though its declaration is out of numeric order). Reviewers should check overlaps against all declarations, not just adjacent/ordered ones or gate-covered slots.
Applied to files:
tests/roms/AccuracySNES/asm/runtime.inc
📚 Learning: 2026-07-21T02:10:49.581Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 190
File: tests/roms/AccuracySNES/gen/src/tests/ppu.rs:0-0
Timestamp: 2026-07-21T02:10:49.581Z
Learning: For any AccuracySNES tests that perform a runtime measurement investigation using writes to $2137 and $4201, do not reuse measurement/slot indices that may already be owned by another test. Before using a slot, verify it is unused (e.g., via an on-cart probe/readback that confirms the slot contains no prior test result). Then independently record $213F immediately before and immediately after each $2137/$4201 operation, so the test can attribute changes to its own operation and avoid cross-test interference.
Applied to files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/ppu.rs
📚 Learning: 2026-07-21T01:34:22.909Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 189
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T01:34:22.909Z
Learning: When reviewing the AccuracySNES documentation in docs/accuracysnes-*.md (notably docs/accuracysnes-plan.md vs the generated docs/accuracysnes-coverage.md), treat the reported metrics as intentionally non-equivalent: the battery test count and dossier assertion coverage are not interchangeable. Do not infer one count/coverage from the other during review (e.g., one test may contain multiple assertions, and multiple tests may contribute to a single assertion/row such as E6.02).
Applied to files:
docs/accuracysnes-plan.md
📚 Learning: 2026-07-21T05:22:58.848Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 197
File: docs/accuracysnes-plan.md:598-600
Timestamp: 2026-07-21T05:22:58.848Z
Learning: In the AccuracySNES documentation under `docs/`, when an assertion exists in the research dossier but cannot be measured/verified by the current cartridge timing test, distinguish the dossier assertion from test measurability: keep the original hardware assertion (and any contribution to the coverage denominator) intact, withdraw/stop using the specific test coverage only if the sources cannot decompose the required CPU-cycle timing into bus vs internal components, and mark the row as not measurable using the `[NOT CART-MEASURABLE ...]` annotation with links to the corresponding plan section (e.g., `docs/accuracysnes-plan.md` §A5.20) and the related roadmap/ticket (e.g., `to-dos/ROADMAP.md` ticket `T-06-A`). Ensure the documentation/coverage reporting treats the row as uncovered rather than removing or redefining the assertion.
Applied to files:
docs/accuracysnes-plan.md
📚 Learning: 2026-07-21T06:21:34.629Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 198
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T06:21:34.629Z
Learning: When reviewing slot allocation for SNES/ROM measurement channels in generator-driven Rust code, account for slots assigned via generation-time computed writers (e.g., slots derived from formulas like `slot_base = 8 + index * 2`) rather than only literal `record(...)` calls. Trace the computed writer’s full emitted slot range and verify there are no collisions with other opcodes/channels that may use different slot ranges after earlier conflict resolution.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/ppu.rs
📚 Learning: 2026-07-22T06:12:00.703Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 201
File: tests/roms/AccuracySNES/gen/src/tests/apu.rs:1508-1538
Timestamp: 2026-07-22T06:12:00.703Z
Learning: When working on APU/voice timing in these AccuracySNES test sources, treat NTSC vs PAL differences as a first-class constraint. In particular, do not change `Voice::settle` (or any settling/code-size/timing logic it affects) unless you cross-validate against both regions (NTSC and PAL), because timing/polling phase shifts can cause regressions that may appear as PAL-only test failures. For ROM-specific expectations like `E7.13`, keep the intentionally chosen ENVX range (e.g., `0x68..=0x7C`) unless you re-validate that the absorbed post-KON timing variation still matches across both regions.
Applied to files:
tests/roms/AccuracySNES/gen/src/tests/ppu.rs
🔇 Additional comments (12)
docs/accuracysnes-plan.md (2)
471-472: LGTM!
521-531: LGTM!CHANGELOG.md (1)
26-35: LGTM!scripts/accuracysnes/crossval.sh (1)
159-159: LGTM!tests/roms/AccuracySNES/asm/runtime.inc (1)
451-451: LGTM!tests/roms/AccuracySNES/asm/runtime.s (2)
108-120: LGTM!
3814-3829: LGTM!tests/roms/AccuracySNES/gen/src/tests/ppu.rs (4)
73-73: LGTM!
2361-2433: LGTM!
2439-2443: 🎯 Functional CorrectnessNo width-directive change is needed.
Asm::lautomatically emits.a8aftersep #$20and.i16afterrep #$10.> Likely an incorrect or invalid review comment.
2354-2360: 🗄️ Data Integrity & IntegrationKeep the same-frame Range Over guard. Range Over clears at the end of VBlank, and
$213Ereads do not clear it in RustySNES, Snes9x, or Mesen2.tests/roms/AccuracySNES/gen/src/dossier.rs (1)
214-214: LGTM!
| #### `C7.05` — a fixed bracket is vacuous; move the index instead | ||
|
|
||
| An H-IRQ is serviced ~22-27 dots after its `HTIME`, so a bracket tight enough to pin dot 65 sits | ||
| inside the latency's own uncertainty, while one loose enough to be safe passes for any set dot across | ||
| most of the line. That is the `F1.09` shape. | ||
|
|
||
| What pins `H = OAM.INDEX * 2` is that the set dot must **move with the index**. Both phases sample | ||
| the *same* dot and change only which OAM entry is the 33rd in-range sprite: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the conflicting fixed-bracket guidance.
This section states that a fixed H-IRQ bracket is vacuous for C7.05. Later, Lines 545-549 still instruct the reader to write these rows as a bracket. Clarify that C7.05 uses the moving-index probe and state separately whether C7.06 uses a bracket.
As per path instructions, docs are the specification, so conflicting sampling instructions must be resolved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/accuracysnes-plan.md` around lines 499 - 506, Update the C7.05 guidance
around the moving-index probe to remove or revise the later Lines 545-549
instruction that describes its rows as a fixed bracket. Explicitly state that
C7.05 samples the same dot while moving the OAM index, and separately identify
whether C7.06 uses a bracket.
Source: Path instructions
| What pins `H = OAM.INDEX * 2` is that the set dot must **move with the index**. Both phases sample | ||
| the *same* dot and change only which OAM entry is the 33rd in-range sprite: | ||
|
|
||
| | phase | in-range sprites | 33rd is index | set dot `2i + 1` | read at ~dot 105 | | ||
| |---|---|---:|---:|---| | ||
| | A | `0..39` | 32 | 65 | **set** | | ||
| | B | `40..79` | 72 | 145 | **clear** | | ||
|
|
||
| Phase B additionally re-reads `$213E` in the vblank of the *same* frame and requires it set by then — | ||
| without that, "clear at dot 105" is also what a core that never sets the flag would report. The flags | ||
| clear at frame start, not on read (`Ppu::advance`), so the later read sees the same frame. | ||
|
|
||
| Three injections, each failing its own code: a fixed set dot fails **code 2**, never setting the flag | ||
| fails **code 1**, and setting correctly for a low index but never for a high one — which phase A | ||
| cannot catch — fails **code 3**, the guard. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Align all validation records with the assertions that C7.05 actually executes.
The supplied test does not measure exact set dots. It proves only that index 32 is set by approximately dot 105, while index 72 is clear at dot 105 and set by vblank. An implementation such as set_dot = index + 50 passes these assertions but violates H = 2 * index.
docs/accuracysnes-plan.md#L505-L519: add samples around dots 65 and 145, or narrow the coverage claim.CHANGELOG.md#L14-L24: describe index-dependent movement unless exact timing is tested.scripts/accuracysnes/crossval.sh#L150-L158: align the harness record with the actual cartridge assertions.
As per path instructions, AccuracySNES tests must distinguish the named behavior from alternatives, and documentation must not claim broader coverage than the corresponding assertion.
📍 Affects 3 files
docs/accuracysnes-plan.md#L505-L519(this comment)CHANGELOG.md#L14-L24scripts/accuracysnes/crossval.sh#L150-L158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/accuracysnes-plan.md` around lines 505 - 519, Align the C7.05 validation
documentation and harness records with the assertions actually executed: in
docs/accuracysnes-plan.md:505-519, add validation samples around dots 65 and 145
or narrow the coverage claim; in CHANGELOG.md:14-24, describe index-dependent
movement without claiming exact timing; and in
scripts/accuracysnes/crossval.sh:150-158, update the harness record to match the
cartridge assertions. Preserve the distinction between index 32 being set by
approximately dot 105 and index 72 being clear there but set by vblank.
Source: Path instructions
| ; $005B is the next free byte: the low-RAM map is $0040 PAD_X/V_DISPATCH_TMP, $0042 V_BRK_VEC, | ||
| ; $0044 V_COP_VEC, $0046 V_H0, $0048 V_H1, $004A V_IRQ_VEC, $004C V_DISPATCH, $0050-$0052 | ||
| ; V_APU_PTR (24-bit, owns three bytes), $0054 V_COP_VEC_E, $0080 PAD_A. | ||
| ; V_APU_PTR (24-bit, owns three bytes), $0054 V_COP_VEC_E, $0056 V_NMI_VEC, | ||
| ; $0058-$005A V_IRQ_VEC_FAR (24-bit, owns three bytes), $0080 PAD_A. | ||
| V_NMI_VEC = $0056 | ||
|
|
||
| ; A 24-bit IRQ handler pointer, for tests that do NOT live in bank $00. | ||
| ; | ||
| ; `irq_trampoline` is a bank-LOCAL `jmp (V_IRQ_VEC)`, so the pointer it follows can only name a | ||
| ; handler in bank $00 — which means Groups C-G, whose bodies are relocated out of bank $00, could | ||
| ; not install an IRQ handler at all. A test outside bank $00 therefore points `V_IRQ_VEC` at | ||
| ; `irq_far_shim` (which IS in bank $00, so the existing trampoline reaches it) and puts its own | ||
| ; 24-bit handler address here; the shim does `jml [V_IRQ_VEC_FAR]`. | ||
| ; | ||
| ; `jml` pushes nothing, so the interrupt's own return frame is untouched and the far handler still | ||
| ; ends with a plain `rti`. Owns three bytes, $0058-$005A; $005B is the next free byte. Initialised | ||
| ; at reset to `irq_stub` in bank $00, for the same reason every other vector here is: a test that | ||
| ; enables the shim without filling this in returns harmlessly instead of jumping into whatever RAM | ||
| ; happened to hold. | ||
| V_IRQ_VEC_FAR = $0058 | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
V_IRQ_VEC_FAR at $0058 overlaps V_BANKPROBE and V_BANKPROBE_RET.
Line 336 allocates $0058-$005A to V_IRQ_VEC_FAR. Line 351 already allocates $0058 to V_BANKPROBE, and line 352 allocates $005A-$005C to V_BANKPROBE_RET (a 24-bit pointer, as its own comment states). The new pointer therefore covers V_BANKPROBE completely and the low byte of V_BANKPROBE_RET.
Both directions corrupt state:
resetwritesirq_stuband bank$00into$0058-$005Aon every boot, so it clobbersV_BANKPROBEand the low byte ofV_BANKPROBE_RETbefore any test runs.A4.04/A4.05writeV_BANKPROBE_RETand the bankprobe stubs writeV_BANKPROBE, so those tests destroy an installed far IRQ vector. A laterirq_far_shimdispatch then doesjmlthrough a partially overwritten pointer.
The new map comment also omits the bankprobe block and V_STR_PTR ($005C), which is why the claim "$005B is the next free byte" is wrong: $005B-$005E are owned by V_BANKPROBE_RET and V_STR_PTR.
Relocate the far vector to an unclaimed triple and record the full map.
Proposed fix: move the far vector clear of the bankprobe block
-; $005B is the next free byte: the low-RAM map is $0040 PAD_X/V_DISPATCH_TMP, $0042 V_BRK_VEC,
+; $0063 is the next free byte: the low-RAM map is $0040 PAD_X/V_DISPATCH_TMP, $0042 V_BRK_VEC,
; $0044 V_COP_VEC, $0046 V_H0, $0048 V_H1, $004A V_IRQ_VEC, $004C V_DISPATCH, $0050-$0052
; V_APU_PTR (24-bit, owns three bytes), $0054 V_COP_VEC_E, $0056 V_NMI_VEC,
-; $0058-$005A V_IRQ_VEC_FAR (24-bit, owns three bytes), $0080 PAD_A.
+; $0058 V_BANKPROBE, $005A-$005C V_BANKPROBE_RET (24-bit), $005C-$005E V_STR_PTR (24-bit),
+; $0060-$0062 V_IRQ_VEC_FAR (24-bit, owns three bytes), $0080 PAD_A.
@@
-; ends with a plain `rti`. Owns three bytes, $0058-$005A; $005B is the next free byte. Initialised
+; ends with a plain `rti`. Owns three bytes, $0060-$0062; $0063 is the next free byte. $0058-$005E
+; is NOT available: V_BANKPROBE, V_BANKPROBE_RET and V_STR_PTR own it. Initialised
; at reset to `irq_stub` in bank $00, for the same reason every other vector here is: a test that
; enables the shim without filling this in returns harmlessly instead of jumping into whatever RAM
; happened to hold.
-V_IRQ_VEC_FAR = $0058
+V_IRQ_VEC_FAR = $0060📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ; $005B is the next free byte: the low-RAM map is $0040 PAD_X/V_DISPATCH_TMP, $0042 V_BRK_VEC, | |
| ; $0044 V_COP_VEC, $0046 V_H0, $0048 V_H1, $004A V_IRQ_VEC, $004C V_DISPATCH, $0050-$0052 | |
| ; V_APU_PTR (24-bit, owns three bytes), $0054 V_COP_VEC_E, $0080 PAD_A. | |
| ; V_APU_PTR (24-bit, owns three bytes), $0054 V_COP_VEC_E, $0056 V_NMI_VEC, | |
| ; $0058-$005A V_IRQ_VEC_FAR (24-bit, owns three bytes), $0080 PAD_A. | |
| V_NMI_VEC = $0056 | |
| ; A 24-bit IRQ handler pointer, for tests that do NOT live in bank $00. | |
| ; | |
| ; `irq_trampoline` is a bank-LOCAL `jmp (V_IRQ_VEC)`, so the pointer it follows can only name a | |
| ; handler in bank $00 — which means Groups C-G, whose bodies are relocated out of bank $00, could | |
| ; not install an IRQ handler at all. A test outside bank $00 therefore points `V_IRQ_VEC` at | |
| ; `irq_far_shim` (which IS in bank $00, so the existing trampoline reaches it) and puts its own | |
| ; 24-bit handler address here; the shim does `jml [V_IRQ_VEC_FAR]`. | |
| ; | |
| ; `jml` pushes nothing, so the interrupt's own return frame is untouched and the far handler still | |
| ; ends with a plain `rti`. Owns three bytes, $0058-$005A; $005B is the next free byte. Initialised | |
| ; at reset to `irq_stub` in bank $00, for the same reason every other vector here is: a test that | |
| ; enables the shim without filling this in returns harmlessly instead of jumping into whatever RAM | |
| ; happened to hold. | |
| V_IRQ_VEC_FAR = $0058 | |
| ; $0063 is the next free byte: the low-RAM map is $0040 PAD_X/V_DISPATCH_TMP, $0042 V_BRK_VEC, | |
| ; $0044 V_COP_VEC, $0046 V_H0, $0048 V_H1, $004A V_IRQ_VEC, $004C V_DISPATCH, $0050-$0052 | |
| ; V_APU_PTR (24-bit, owns three bytes), $0054 V_COP_VEC_E, $0056 V_NMI_VEC, | |
| ; $0058 V_BANKPROBE, $005A-$005C V_BANKPROBE_RET (24-bit), $005C-$005E V_STR_PTR (24-bit), | |
| ; $0060-$0062 V_IRQ_VEC_FAR (24-bit, owns three bytes), $0080 PAD_A. | |
| V_NMI_VEC = $0056 | |
| ; A 24-bit IRQ handler pointer, for tests that do NOT live in bank $00. | |
| ; | |
| ; `irq_trampoline` is a bank-LOCAL `jmp (V_IRQ_VEC)`, so the pointer it follows can only name a | |
| ; handler in bank $00 — which means Groups C-G, whose bodies are relocated out of bank $00, could | |
| ; not install an IRQ handler at all. A test outside bank $00 therefore points `V_IRQ_VEC` at | |
| ; `irq_far_shim` (which IS in bank $00, so the existing trampoline reaches it) and puts its own | |
| ; 24-bit handler address here; the shim does `jml [V_IRQ_VEC_FAR]`. | |
| ; | |
| ; `jml` pushes nothing, so the interrupt's own return frame is untouched and the far handler still | |
| ; ends with a plain `rti`. Owns three bytes, $0060-$0062; $0063 is the next free byte. $0058-$005E | |
| ; is NOT available: V_BANKPROBE, V_BANKPROBE_RET and V_STR_PTR own it. Initialised | |
| ; at reset to `irq_stub` in bank $00, for the same reason every other vector here is: a test that | |
| ; enables the shim without filling this in returns harmlessly instead of jumping into whatever RAM | |
| ; happened to hold. | |
| V_IRQ_VEC_FAR = $0060 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/roms/AccuracySNES/asm/runtime.inc` around lines 317 - 337, Relocate
V_IRQ_VEC_FAR from $0058-$005A to an unclaimed three-byte low-RAM range that
does not overlap V_BANKPROBE, V_BANKPROBE_RET, or V_STR_PTR, and update every
reference, reset initialization, and irq_far_shim access accordingly. Correct
the surrounding memory-map comment to document the complete allocation through
the bankprobe and string-pointer blocks and identify the actual next free byte.
Sources: Path instructions, Learnings
…ple dot Two review findings, both real, and the first exposed the second. The handler read $213E and $4211 without saving A, while firing inside wait_vblank_far's own poll loop. It now brackets the body with php/pha and pla/plp. That fix broke the row -- which is how the second problem surfaced. The HTIME was chosen from a ~22-27 dot IRQ latency estimate borrowed from a different measurement, putting the sample at ~170: PAST phase B's set dot of 145. It had been passing only because the handler was three instructions shorter and landed just under it. Adding the three instructions pushed it over. Measured the sample dot properly, by temporarily latching $213C inside the handler: the real IRQ-to-sample latency is ~93 dots, because the interrupt is only taken at an instruction boundary in the poll loop and the trampoline, far shim and handler prologue all precede the read. HTIME 78 -> 12 puts the sample at 95-99, ~30 dots clear of both 65 and 145. All three injections re-verified at the corrected position and still fail their own codes (2, 1, 3). Also from review: arm_hv_irq_and_run's unused `tag` parameter is gone, and setup_over_flag_sprites no longer emits an unsatisfiable cpx #$0000 / bcc when the window starts at index 0. Declined: the claim that irq_far_shim's `jml [V_IRQ_VEC_FAR]` is direct-page indirect and depends on D. The 65816 has no JMP [dp] mode; the shim assembles to DC 58 00, absolute indirect long, which reads the pointer from bank $00 address $0058 with no D involvement. Confirmed by disassembling the built ROM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review adjudicationTwo blocking findings. One applied — and it exposed a worse problem underneath. One not reproduced. Applied: the handler clobbered
|
Antigravity review (Gemini via Ultra)Adds the Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
1 participant
Second row of
v1.28.0.C7on-cart coverage 6 → 7 of 16.A fixed bracket would have been vacuous
#284 proposed sampling
$213Eearly on the line expecting clear and late expecting set. Authoring itshowed that does not work: an H-IRQ is serviced ~22-27 dots after its
HTIME, so a bracket tightenough to pin dot 65 sits inside the latency's own uncertainty, while one loose enough to be safe
passes for any set dot across most of the line. That is the
F1.09shape — a sampler too coarse forits subject agrees with whatever the subject does.
What actually pins
H = OAM.INDEX * 2is that the set dot must move with the index. Both phasessample the same dot and change only which OAM entry is the 33rd in-range sprite:
2i + 10..3940..79A core that sets Range Over at a fixed dot, at line start, or on the count alone answers identically
in both phases. The ~40 dots of margin either side also absorbs the IRQ latency entirely, so the row
does not depend on knowing it.
Phase B additionally re-reads
$213Ein the same frame's vblank: "clear at dot 105" is otherwiseexactly what a core that never sets the flag reports, and phase A cannot cover for it since the two
phases use different sprite layouts. The flags clear at frame start, not on read (
Ppu::advance), sothe later read sees the same frame's result.
Verified by injection — three of them, each hitting its own code
The third is the one that matters: it is the failure mode the vblank re-read exists for, and it
passes both other assertions.
This needed new runtime machinery
Which no
v1.28.0row was supposed to.irq_trampolineis a bank-localjmp (V_IRQ_VEC), sothat pointer can only name a bank-
$00handler — meaning Groups C-G, whose bodies are relocated outof bank
$00, could not install an IRQ handler at all. Nothing outsidecpu.rs/bus.rsever had.irq_far_shim(in bank$00, so the existing trampoline reaches it) plus a 24-bitV_IRQ_VEC_FARat
$0058fixes that for every relocated group.jmlpushes nothing, so the interrupt's returnframe is untouched and a far handler still ends in a plain
rti. It is opt-in and the far vectoris initialised to
irq_stub, so default behaviour and every existing test are unchanged — thebattery is green at 334 tests.
Cross-validation
snes9x fails this row, and it is recorded as an expected divergence (
SNES9X_KNOWN_FAILURES12 → 13, with the reasoning in the script): it reads set in both phases, so its Range Over does
not track the index — a scanline-granularity flag rather than a per-sprite one, which is precisely
the modelling difference the row exists to detect.
Stated plainly, because only two hosts are available: RustySNES's position is anchored to MesenCE on
the line by
scripts/probes/eval-line-213e, but the dot is documentation-anchored only(fullsnes / SNESdev Wiki: evaluation walks OAM two cycles per sprite). The Mesen2 headless runner
times out in this environment, which predates this work, so it could not arbitrate.
cargo run -p accuracysnes-genreproduces the committed ROM; battery 100% on-cart; fmt andclippy --workspace -D warningsclean.🤖 Generated with Claude Code
Adds AccuracySNES dossier assertion
C7.05. It verifies that Range Over timing tracks the 33rd in-range sprite and that the flag persists during same-frame vblank. Sprite index 32 must set the flag by dot 65; index 72 must leave it clear until after dot 105.The claim is false if Range Over does not depend on
OAM.INDEX * 2, or if the flag does not persist during vblank. Coverage increases from 6/16 to 7/16.The test also adds opt-in far IRQ support for relocated groups through
irq_far_shimandV_IRQ_VEC_FAR. SNES9x is recorded as an expected failure because its Range Over timing does not track the sprite index.