feat(accuracysnes): B2.07/B2.08 — the frame rate, measured against the APU - #317
Conversation
…e APU "60.0988 Hz" is a claim about wall-clock time and a cart has no wall clock. The one independent timebase it can reach is the APU's, whose 24.576 MHz crystal runs at the same rate in both regions: timer 0 at T0DIV = 1 steps T0OUT every 256 SMP base clocks -- 8,010 ticks a second on either console -- so counting ticks over a known number of frames measures the frame rate in units of that crystal. This only became a real measurement once the APU stopped scaling with the video clock (the region-independence fix). An APU that speeds up exactly when the frames get shorter cannot measure how long a frame is; the row would have read the same number on both images. Over 48 frames: ~6,398 NTSC against ~7,689 PAL, 1,291 apart, keyed on the MEASURED frame height rather than the region bit. The band is +/-2% and the doc says plainly that this pins the rate to about a part in fifty -- NOT to the precision the quoted figures have. What it excludes is a wrong scanline count or a region-scaled APU clock, both percent-level errors and one of which this emulator had. The cart cannot sit in upload_and_run's wait loop while it counts frames, so the row drives the program itself through a new upload_only: the SPC announces it is accumulating, the cart counts vblanks, then writes a stop byte. Every wait on both sides is bounded, so an APU that never answers reports SKIP rather than hanging the battery. Adding it also made Mesen2's PAL verdict on F1.10 flip between runs of a single build -- the third host/image combination to show that row is decided by the cart's execution phase. mesen_crossval.lua now skips one index supplied through ACCURACYSNES_SKIP_INDEX, which crossval.sh resolves BY ID from SOURCE_CATALOG.tsv; the ares gate shares the same resolved index instead of computing its own. Three consecutive full cross-validation runs are stable. Coverage 355 -> 356 of 443 (302 on-cart + 54 scenes), battery 343 tests at 100% on-cart, three references agree on both images. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe AccuracySNES suite adds B2.07/B2.08 APU timing measurements over 48 frames. Cross-validation now resolves and excludes the unstable F1.10 catalogue row for both Mesen2 and ares. ChangesAccuracySNES APU timing coverage
F1.10 cross-validation exclusion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccuracySNESCart
participant SPC700
participant APUTest
AccuracySNESCart->>SPC700: upload timer program
APUTest->>AccuracySNESCart: count 48 vblank intervals
AccuracySNESCart->>SPC700: send stop command
SPC700-->>APUTest: return timer ticks
APUTest->>APUTest: validate NTSC or PAL range
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new AccuracySNES on-cart timing check that measures the console frame rate against the APU’s region-independent crystal (via SPC700 timer ticks), and stabilizes cross-validation gating by excluding a phase-fragile controller-port row via an ID-resolved catalog index.
Changes:
- Add
B2.07/B2.08“Frame rate vs APU clock” test, including a new CPU↔APU handshake path (upload_only) and measurement-slot reporting. - Update AccuracySNES metadata/docs (catalog, dossier map, error codes, coverage/plan/status) to account for the added row and coverage increments.
- Make Mesen2 + ares cross-validation consistently exclude
F1.10by passing a shared, ID-resolvedACCURACYSNES_SKIP_INDEX.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Registers the new B2.07 row and shifts subsequent catalog indices. |
| tests/roms/AccuracySNES/gen/src/tests/apu.rs | Implements the new APU-timed frame-rate test and adds upload_only/tagged timeout label support. |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Maps dossier ID B2.07 to assertion rows B2.07 + B2.08. |
| tests/roms/AccuracySNES/ERROR_CODES.md | Documents new failure codes for B2.07. |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Generated ROM test bodies updated to include test_b2_07 and new SPC payload bytes. |
| scripts/accuracysnes/mesen_crossval.lua | Adds env-driven skip index to exclude a phase-fragile row from the failing count. |
| scripts/accuracysnes/crossval.sh | Resolves unstable row index by ID from SOURCE_CATALOG.tsv and exports it for all hosts. |
| docs/STATUS.md | Updates reported AccuracySNES test/coverage counts (355→356; 342→343; on-cart 301→302). |
| docs/accuracysnes-plan.md | Updates headline test/coverage counts to match the new row. |
| docs/accuracysnes-coverage.md | Updates B2 coverage and overall on-cart/total coverage counts. |
| crates/rustysnes-test-harness/tests/accuracysnes.rs | Extends the measurement-slot report list to include the new B2.07 slots. |
| CHANGELOG.md | Documents the new B2.07/B2.08 measurement row and the expanded crossval exclusion for F1.10. |
| /// The cart cannot sit in `upload_and_run`'s wait loop while it counts frames, so this uses | ||
| /// [`upload_only`] and drives the program itself: the SPC announces it is accumulating, the cart | ||
| /// counts vblanks, then writes a stop byte. **Every wait on both sides is bounded** — an APU that | ||
| /// never answers reports SKIP rather than hanging the battery, and the SPC's own poll loop exits on | ||
| /// a counter as well as on the stop byte, so a cart that dies mid-count cannot strand it. |
| @@ -596,7 +596,7 @@ const A5_08_SLOTS: [(u8, &str); 7] = [ | |||
| ]; | |||
|
|
|||
| /// The measurement slots the `v1.29.0` Group E batch records. | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@CHANGELOG.md`:
- Around line 72-75: Correct the changelog statement that claims every wait is
bounded, or update the SPC poll loop emitted by b2_07 in the APU test
implementation so it has a bounded timeout rather than waiting indefinitely for
the port 3 stop byte. Ensure the changelog accurately reflects the shipped
behavior and the row reports SKIP when the SPC does not respond.
In `@crates/rustysnes-test-harness/tests/accuracysnes.rs`:
- Around line 624-628: Update the accuracy assertions for slot 276 in the test
around the “B2.07 measured frame height” entry so the measured value is accepted
only when it equals 261 (NTSC) or 311 (PAL). Preserve the existing slot 257
validation and ensure any other value, including an unwritten zero, fails the
test.
In `@scripts/accuracysnes/crossval.sh`:
- Around line 303-304: Update the UNSTABLE_INDEX lookup in the cross-validation
flow to require exactly one matching UNSTABLE_ROW entry, then validate that the
catalogue index is a decimal integer within 0 <= index < test_count. Reject
invalid, ambiguous, one-based, or values such as “293x” before exporting
ACCURACYSNES_SKIP_INDEX or applying either host’s filtering.
In `@tests/roms/AccuracySNES/gen/src/tests/apu.rs`:
- Around line 8923-8937: Bound the SPC polling loop around the visible poll
label and T0OUT accumulation by adding the iteration counter described in the
nearby documentation, while preserving exit on STOP and allowing fall-through to
publish and release_to_ipl. In CHANGELOG.md lines 72-75, retain the statement
that every wait on both sides is bounded after this code change.
- Around line 8821-8832: Align the measurement start with a vblank boundary
instead of beginning accumulation at the arbitrary `@b2go` phase. In the
`@b2go/`@b2frame flow, call wait_vblank_far once before writing the start byte on
port 3, and update the SPC-side startup logic to drain $FD and begin
accumulation only after receiving that byte. Preserve the intended B2_FRAMES
frame interval and ±2% assertion bounds.
- Around line 180-215: Refactor upload_and_run_tagged to reuse upload_only for
the shared program emission, register setup, upload, and APUIO0 clearing instead
of duplicating that upload half. Keep only the wait-for-done loop and its
surrounding behavior in upload_and_run_tagged, while preserving upload_only’s
existing semantics.
🪄 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: 4acdc5a6-965e-4cb7-bc47-b692a15d0f4c
⛔ Files ignored due to path filters (6)
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-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 (8)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes.rsdocs/STATUS.mddocs/accuracysnes-plan.mdscripts/accuracysnes/crossval.shscripts/accuracysnes/mesen_crossval.luatests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: lint
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: review
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (16)
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/STATUS.mddocs/accuracysnes-plan.md
**/*.{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:
docs/STATUS.mddocs/accuracysnes-plan.mdcrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/dossier.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/apu.rs
**/*
📄 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:
docs/STATUS.mdscripts/accuracysnes/mesen_crossval.luascripts/accuracysnes/crossval.shdocs/accuracysnes-plan.mdcrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/dossier.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/apu.rs
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/STATUS.mddocs/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/STATUS.mddocs/accuracysnes-plan.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:
docs/STATUS.mddocs/accuracysnes-plan.mdCHANGELOG.md
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/mesen_crossval.luascripts/accuracysnes/crossval.sh
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-test-harness/tests/accuracysnes.rs
**/*.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:
crates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-test-harness/tests/accuracysnes.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:
crates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-test-harness/tests/accuracysnes.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/apu.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/apu.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/apu.rs
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
🧠 Learnings (5)
📚 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-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/apu.rs
📚 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/apu.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/apu.rs
🪛 LanguageTool
docs/STATUS.md
[style] ~244-~244: Try using a descriptive adverb here.
Context: ...cs/adr/0013), kept as separate columns on purpose (docs/accuracysnes-coverage.md`, regen...
(ON_PURPOSE_DELIBERATELY)
🔇 Additional comments (10)
scripts/accuracysnes/crossval.sh (2)
293-301: LGTM!
305-309: LGTM!Also applies to: 369-371
scripts/accuracysnes/mesen_crossval.lua (1)
24-28: LGTM!Also applies to: 70-79, 97-97
tests/roms/AccuracySNES/gen/src/tests/apu.rs (2)
28-28: LGTM!Also applies to: 152-152, 398-411, 8947-8965
8805-8819: 🎯 Functional CorrectnessRetain the current
STARTEDhandshake.apu_uploadwrites zero toAPUIO1at the final transfer close beforeupload_onlyreturns, so an earlier program or the IPL cannot leave$3Cthere. The wait accepts$3Conly after this program writes it.> Likely an incorrect or invalid review comment.CHANGELOG.md (1)
14-26: LGTM!Also applies to: 56-71, 76-76
crates/rustysnes-test-harness/tests/accuracysnes.rs (1)
599-599: LGTM!docs/STATUS.md (1)
26-27: LGTM!Also applies to: 244-244
docs/accuracysnes-plan.md (1)
16-17: LGTM!tests/roms/AccuracySNES/gen/src/dossier.rs (1)
275-275: 🗄️ Data Integrity & IntegrationKeep the coverage totals unchanged.
B2.07andB2.08form one enumerated dossier row, so the generated report correctly increases on-cart coverage from 301 to 302.> Likely an incorrect or invalid review comment.
| The cart cannot sit in `upload_and_run`'s wait loop while it counts frames, so the row drives the | ||
| program itself through a new `upload_only`: the SPC announces it is accumulating, the cart counts | ||
| vblanks, then writes a stop byte. Every wait on both sides is bounded, so an APU that never answers | ||
| reports SKIP rather than hanging the battery. Coverage `355 → 356 of 443`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
"Every wait on both sides is bounded" is not true of the shipped SPC program.
The cart side is bounded. The SPC side is not: the emitted poll loop in b2_07 exits only on the stop byte from port 3. See the finding on tests/roms/AccuracySNES/gen/src/tests/apu.rs. Correct the code or this sentence; the changelog is the user-visible statement of what the row guarantees.
🤖 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 `@CHANGELOG.md` around lines 72 - 75, Correct the changelog statement that
claims every wait is bounded, or update the SPC poll loop emitted by b2_07 in
the APU test implementation so it has a bounded timeout rather than waiting
indefinitely for the port 3 stop byte. Ensure the changelog accurately reflects
the shipped behavior and the row reports SKIP when the SPC does not respond.
| ( | ||
| 275, | ||
| "B2.07 APU ticks over 48 frames (~6398 NTSC, ~7689 PAL)", | ||
| ), | ||
| (276, "B2.07 measured frame height (261 NTSC, 311 PAL)"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Slot 276 has exactly two legal values; assert them.
The measured frame height is 261 or 311. Nothing else is a valid reading. The existing crude check on slot 257 exists because an unwritten slot reads zero and prints as a plausible-looking row. Slot 276 admits the same failure and is cheaper to bound than slot 257.
Proposed additional check
let voices = report.meas[257];
assert!(
(1..=8).contains(&voices),
"E8.01 recorded {voices} started voices, which is not a count of eight voices — the slot \
was never written, or the mask reduction overflowed"
);
+
+ let height = report.meas[276];
+ assert!(
+ height == 261 || height == 311,
+ "B2.07 recorded a frame height of {height}, which is neither NTSC's 261 nor PAL's 311 — \
+ the slot was never written, or the measurement is wrong"
+ );
}🤖 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 `@crates/rustysnes-test-harness/tests/accuracysnes.rs` around lines 624 - 628,
Update the accuracy assertions for slot 276 in the test around the “B2.07
measured frame height” entry so the measured value is accepted only when it
equals 261 (NTSC) or 311 (PAL). Preserve the existing slot 257 validation and
ensure any other value, including an unwritten zero, fails the test.
| UNSTABLE_INDEX=$(awk -F'\t' -v id="$UNSTABLE_ROW" '$2 == id { print $1; exit }' \ | ||
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
catalog=tests/roms/AccuracySNES/SOURCE_CATALOG.tsv
count=$(awk -F'\t' '$2 == "F1.10" { n++ } END { print n + 0 }' "$catalog")
[[ "$count" -eq 1 ]]
index=$(awk -F'\t' '$2 == "F1.10" { print $1; exit }' "$catalog")
[[ "$index" =~ ^[0-9]+$ ]]
first_index=$(awk -F'\t' '$1 ~ /^[0-9]+$/ { print $1; exit }' "$catalog")
[[ "$first_index" == "0" ]]
printf 'F1.10 index: %s\n' "$index"
rg -n \
'ACCURACYSNES_SKIP_INDEX|SOURCE_CATALOG.tsv|STATUS \+|skip.*\+ 0|for i = 0' \
scripts/accuracysnesRepository: doublegate/RustySNES
Length of output: 2489
🏁 Script executed:
set -euo pipefail
catalog=tests/roms/AccuracySNES/SOURCE_CATALOG.tsv
printf '%s\n' '--- catalog header and F1.10 row ---'
awk -F'\t' '
NR <= 5 || $2 == "F1.10" { printf "%d:%s\n", NR, $0 }
' "$catalog"
printf '%s\n' '--- catalog index diagnostics ---'
python3 - "$catalog" <<'PY'
import csv, sys
path = sys.argv[1]
with open(path, newline="") as f:
rows = list(csv.reader(f, delimiter="\t"))
print("rows:", len(rows))
for i, row in enumerate(rows, 1):
if len(row) < 2:
print("short_row", i, len(row), row)
indices = [row[0] for row in rows if len(row) >= 1]
numeric = [(i, int(v)) for i, v in enumerate(indices, 1) if v.isdigit()]
print("numeric_rows:", len(numeric))
print("first_numeric:", numeric[:3])
print("last_numeric:", numeric[-3:])
from collections import Counter
counts = Counter(v for _, v in numeric)
print("duplicate_numeric_indices:", sorted((v, n) for v, n in counts.items() if n > 1)[:20])
if numeric:
values = [v for _, v in numeric]
expected = list(range(min(values), max(values) + 1))
print("min_index:", min(values), "max_index:", max(values))
print("missing_indices:", sorted(set(expected) - set(values))[:20])
print("index_count:", len(values), "range_count:", len(expected))
PY
printf '%s\n' '--- crossval.sh mapping and filters ---'
sed -n '170,220p;245,325p;350,390p' scripts/accuracysnes/crossval.sh
printf '%s\n' '--- Mesen skip logic ---'
cat -n scripts/accuracysnes/mesen_crossval.lua | sed -n '1,110p'
printf '%s\n' '--- Ares/status filter implementation ---'
cat -n scripts/accuracysnes/crossval.sh | sed -n '325,385p'
rg -n -C 4 'UNSTABLE_INDEX|ACCURACYSNES_SKIP_INDEX|skip|status' scripts/accuracysnes/ares_host scripts/accuracysnes/crossval.shRepository: doublegate/RustySNES
Length of output: 30245
🏁 Script executed:
set -euo pipefail
catalog=tests/roms/AccuracySNES/SOURCE_CATALOG.tsv
printf '%s\n' '--- catalog header and F1.10 row ---'
awk -F'\t' '
NR <= 5 || $2 == "F1.10" { printf "%d:%s\n", NR, $0 }
' "$catalog"
printf '%s\n' '--- catalog index diagnostics ---'
python3 - "$catalog" <<'PY'
import csv, sys
from collections import Counter
with open(sys.argv[1], newline="") as f:
rows = list(csv.reader(f, delimiter="\t"))
print("rows:", len(rows))
for line, row in enumerate(rows, 1):
if len(row) < 2:
print("short_row", line, len(row), row)
numeric = [(line, int(row[0])) for line, row in enumerate(rows, 1)
if row and row[0].isdigit()]
print("numeric_rows:", len(numeric))
print("first_numeric:", numeric[:3])
print("last_numeric:", numeric[-3:])
counts = Counter(value for _, value in numeric)
print("duplicate_numeric_indices:",
sorted((value, count) for value, count in counts.items() if count > 1)[:20])
if numeric:
values = [value for _, value in numeric]
expected = set(range(min(values), max(values) + 1))
print("min_index:", min(values), "max_index:", max(values))
print("missing_indices:", sorted(expected - set(values))[:20])
print("index_count:", len(values), "range_count:", len(expected))
PY
printf '%s\n' '--- crossval.sh mapping and filters ---'
sed -n '170,220p;245,325p;350,390p' scripts/accuracysnes/crossval.sh
printf '%s\n' '--- Mesen skip logic ---'
cat -n scripts/accuracysnes/mesen_crossval.lua | sed -n '1,110p'
printf '%s\n' '--- Ares/status filter implementation ---'
cat -n scripts/accuracysnes/crossval.sh | sed -n '325,385p'
rg -n -C 4 'UNSTABLE_INDEX|ACCURACYSNES_SKIP_INDEX|skip|status' \
scripts/accuracysnes/ares_host scripts/accuracysnes/crossval.shRepository: doublegate/RustySNES
Length of output: 30245
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- catalog generation and index consumers ---'
rg -n -C 5 \
'SOURCE_CATALOG|catalog|enumerate|index|status' \
--glob '*.rs' --glob '*.py' --glob '*.sh' --glob '*.lua' --glob '*.cpp' \
. | sed -n '1,260p'
printf '%s\n' '--- exact lookup and host-index contracts ---'
rg -n -C 8 \
'UNSTABLE_ROW|UNSTABLE_INDEX|ACCURACYSNES_SKIP_INDEX|for i = 0|status %|STATUS \+|SOURCE_CATALOG.tsv' \
scripts tests | sed -n '1,260p'
printf '%s\n' '--- malformed lookup behavior model ---'
python3 - <<'PY'
import re
def lua_tonumber(value):
# Lua tonumber accepts ordinary decimal forms; malformed strings become nil.
try:
value = value.strip()
if not value or not re.fullmatch(r'[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?', value):
return None
return float(value)
except ValueError:
return None
def awk_number(value):
# Model the numeric prefix used by awk's string-to-number conversion.
m = re.match(r'^[ \t]*[+-]?(?:\d+(?:\.\d*)?|\.\d+)', value)
return float(m.group(0)) if m else 0.0
for value in ["293", "294", "293x", "x293", "", "-1", "+293"]:
lua = lua_tonumber(value)
mesen = -1 if lua is None else lua
ares = awk_number(value)
print(f"{value!r}: Mesen={mesen:g}, ares={ares:g}")
PYRepository: doublegate/RustySNES
Length of output: 45951
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- catalog generation and index consumers ---'
rg -n -C 5 \
'SOURCE_CATALOG|catalog|enumerate|index|status' \
--glob '*.rs' --glob '*.py' --glob '*.sh' --glob '*.lua' --glob '*.cpp' \
. | sed -n '1,260p'
printf '%s\n' '--- exact lookup and host-index contracts ---'
rg -n -C 8 \
'UNSTABLE_ROW|UNSTABLE_INDEX|ACCURACYSNES_SKIP_INDEX|for i = 0|status %|STATUS \+|SOURCE_CATALOG.tsv' \
scripts tests | sed -n '1,260p'
printf '%s\n' '--- malformed lookup behavior model ---'
python3 - <<'PY'
import re
def lua_tonumber(value):
value = value.strip()
if not re.fullmatch(r'[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?', value):
return None
return float(value)
def awk_number(value):
match = re.match(r'^[ \t]*[+-]?(?:\d+(?:\.\d*)?|\.\d+)', value)
return float(match.group(0)) if match else 0.0
for value in ["293", "294", "293x", "x293", "", "-1", "+293"]:
lua = lua_tonumber(value)
mesen = -1 if lua is None else lua
ares = awk_number(value)
print(f"{value!r}: Mesen={mesen:g}, ares={ares:g}")
PYRepository: doublegate/RustySNES
Length of output: 48424
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- catalog emitter ---'
cat -n tests/roms/AccuracySNES/gen/src/emit.rs | sed -n '288,345p'
printf '%s\n' '--- test ordering and duplicate-ID checks ---'
rg -n -C 6 \
'tests\.iter\(\)\.enumerate|enumerate\(\)|assert.*id|duplicate|sort_by|sort_unstable' \
tests/roms/AccuracySNES/gen/src
printf '%s\n' '--- exact ares awk behavior for malformed skip values ---'
for skip in 293 294 293x x293 -1 +293; do
result=$(
printf 'status 0 01\nstatus 293 02\nstatus 294 01\n' |
awk -v skip="$skip" '
$1 == "status" && $2 + 0 != skip + 0 &&
$3 != "ff" && $3 != "00" &&
$3 ~ /[02468aceACE]$/ { c++ }
END { print c + 0 }
'
)
printf 'skip=%q ares_count=%s\n' "$skip" "$result"
done
printf '%s\n' '--- runtime guard presence ---'
python3 - <<'PY'
from pathlib import Path
text = Path("scripts/accuracysnes/crossval.sh").read_text()
start = text.index("UNSTABLE_INDEX=")
end = text.index("ARES_HOST=", start)
block = text[start:end]
print(block)
print("has_numeric_guard:", "[[ \"$UNSTABLE_INDEX\" =~" in block)
print("has_duplicate_guard:", "count" in block and "== 1" in block)
print("has_range_guard:", "343" in block or "COUNT" in block)
PYRepository: doublegate/RustySNES
Length of output: 1929
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- catalog emitter ---'
cat -n tests/roms/AccuracySNES/gen/src/emit.rs | sed -n '288,345p'
printf '%s\n' '--- test ordering and duplicate-ID checks ---'
rg -n -C 6 \
'tests\.iter\(\)\.enumerate|enumerate\(\)|assert.*id|duplicate|sort_by|sort_unstable' \
tests/roms/AccuracySNES/gen/src
printf '%s\n' '--- exact ares awk behavior for malformed skip values ---'
for skip in 293 294 293x x293 -1 +293; do
result=$(
printf 'status 0 01\nstatus 293 02\nstatus 294 01\n' |
awk -v skip="$skip" '
$1 == "status" && $2 + 0 != skip + 0 &&
$3 != "ff" && $3 != "00" &&
$3 ~ /[02468aceACE]$/ { c++ }
END { print c + 0 }
'
)
printf 'skip=%q ares_count=%s\n' "$skip" "$result"
done
printf '%s\n' '--- runtime guard presence ---'
python3 - <<'PY'
from pathlib import Path
text = Path("scripts/accuracysnes/crossval.sh").read_text()
start = text.index("UNSTABLE_INDEX=")
end = text.index("ARES_HOST=", start)
block = text[start:end]
print(block)
print("has_numeric_guard:", "[[ \"$UNSTABLE_INDEX\" =~" in block)
print("has_duplicate_guard:", "count" in block and "== 1" in block)
print("has_range_guard:", "343" in block or "COUNT" in block)
PYRepository: doublegate/RustySNES
Length of output: 1929
Reject invalid catalogue indices before exporting ACCURACYSNES_SKIP_INDEX.
The current catalogue is zero-based, contiguous, and unique, but line 303 accepts any non-empty first match. A one-based index skips the wrong status in both hosts. A value such as 293x makes Mesen skip nothing while ares treats it as 293. Require exactly one UNSTABLE_ROW match, a decimal index, and 0 <= index < test_count before filtering either host.
🤖 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 `@scripts/accuracysnes/crossval.sh` around lines 303 - 304, Update the
UNSTABLE_INDEX lookup in the cross-validation flow to require exactly one
matching UNSTABLE_ROW entry, then validate that the catalogue index is a decimal
integer within 0 <= index < test_count. Reject invalid, ambiguous, one-based, or
values such as “293x” before exporting ACCURACYSNES_SKIP_INDEX or applying
either host’s filtering.
Source: Path instructions
| /// Emit only the UPLOAD half of [`upload_and_run_tagged`]: the transfer and the port-0 clear, | ||
| /// stopping short of the wait-for-done loop. | ||
| /// | ||
| /// Split out for the one row that has to do something on the CPU side **while** the SPC program | ||
| /// runs — `B2.07`/`B2.08` count frames against the APU's timer, so the cart cannot be sitting in a | ||
| /// busy-wait. Everything else should keep calling [`upload_and_run`], which is this plus the wait. | ||
| fn upload_only(a: &mut Asm, prog: &Spc) { | ||
| let label = format!("apu_prog_{}", next_prog_id()); | ||
| a.d(&format!("{label}:")); | ||
| for line in prog.as_ca65(" ").lines() { | ||
| a.d(line); | ||
| } | ||
| a.l("rep #$30"); | ||
| a.l("phk"); | ||
| a.l("plb"); | ||
| a.c("Point apu_upload at this test's own program image, which lives in another bank."); | ||
| a.l(&format!("lda #.loword({label})")); | ||
| a.l("sta f:V_APU_SRC"); | ||
| a.l("sep #$20"); | ||
| a.l(&format!("lda #^{label}")); | ||
| a.l("sta f:V_APU_BANK"); | ||
| a.l("rep #$30"); | ||
| a.l(&format!("lda #{}", prog.bytes().len())); | ||
| a.l("sta f:V_APU_LEN"); | ||
| a.l("lda #$0200"); | ||
| a.l("sta f:V_APU_DEST ; APU RAM $0200: clear of the zero page and the stack"); | ||
| a.l("lda #$0200"); | ||
| a.l("sta f:V_APU_ENTRY"); | ||
| a.l("jsl apu_upload_far"); | ||
| a.c("Clear the CPU-side port 0 before the program can look at it. The previous test left the"); | ||
| a.c("release byte there, and a program whose release loop sees it immediately jumps back to"); | ||
| a.c("the IPL before the cart has read a thing — which reads as a wrong answer, not a race."); | ||
| a.l("sep #$20"); | ||
| a.l("lda #$00"); | ||
| a.l("sta APUIO0"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
upload_only duplicates upload_and_run_tagged's upload half verbatim.
Lines 186-215 and lines 221-248 are identical, comments included. The two will drift. Express the wait-carrying variant in terms of the upload-only one.
Proposed deduplication
fn upload_and_run_tagged(a: &mut Asm, prog: &Spc, tag: &str) {
- // The image goes in the out-of-bank data segment, not inline in the test body: these are
- // several hundred bytes each and bank $00 is finite. `apu_upload` takes a 24-bit pointer
- // anyway, so nothing about the upload cares where it lives.
- let label = format!("apu_prog_{}", next_prog_id());
- a.d(&format!("{label}:"));
- for line in prog.as_ca65(" ").lines() {
- a.d(line);
- }
- a.l("rep #$30");
- ...
- a.l("sta APUIO0");
+ upload_only(a, prog);
a.c("Wait for the program's done marker, but not forever: an APU that never boots would");🤖 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/gen/src/tests/apu.rs` around lines 180 - 215,
Refactor upload_and_run_tagged to reuse upload_only for the shared program
emission, register setup, upload, and APUIO0 clearing instead of duplicating
that upload half. Keep only the wait-for-done loop and its surrounding behavior
in upload_and_run_tagged, while preserving upload_only’s existing semantics.
| a.c( | ||
| "Count frames. wait_vblank_far returns at vblank start, so this is exactly N frame periods", | ||
| ); | ||
| a.c("of CPU time -- the interval the APU's tick count is being measured against."); | ||
| a.l("rep #$30"); | ||
| a.l(&format!("ldx #{frames}")); | ||
| a.label("b2frame"); | ||
| a.l("phx"); | ||
| a.l("jsl wait_vblank_far"); | ||
| a.l("plx"); | ||
| a.l("dex"); | ||
| a.l("bne @b2frame"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The counted interval is 47 to 48 frames, not 48, and the NTSC lower bound sits inside that jitter.
wait_vblank_far returns at the next vblank start. The loop runs it B2_FRAMES times, so the elapsed CPU time is the partial frame remaining when the loop is entered plus 47 whole frames. The APU, however, started accumulating at STARTED, which the cart observes at an arbitrary point inside a frame.
Let f be the fraction of a frame still remaining when @b2go is reached. The measured interval is 48 - f frames. On NTSC the tick total is therefore in (6265, 6398], not ~6398. The asserted lower bound is 6270. A handshake that completes in the last 4% of a frame produces a reading below 6270 and the row fails with nothing wrong in the core. The PAL side has the same shape: (7529, 7689] against a 7535 bound.
The phase of that handshake depends on upload duration and on how much code runs ahead of this test, which is exactly the sensitivity that made F1.10 unstable in this same PR.
Widening the band is the wrong fix; it weakens the ±2% claim the doc comment is built on. Align to a frame boundary first, then start the APU accumulating:
- call
wait_vblank_faronce after@b2go, - then write a start byte on port 3,
- and have the SPC drain
$FDand begin only on that byte.
The residual error is then one SPC poll-loop period instead of one frame.
🤖 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/gen/src/tests/apu.rs` around lines 8821 - 8832, Align
the measurement start with a vblank boundary instead of beginning accumulation
at the arbitrary `@b2go` phase. In the `@b2go/`@b2frame flow, call wait_vblank_far
once before writing the start byte on port 3, and update the SPC-side startup
logic to drain $FD and begin accumulation only after receiving that byte.
Preserve the intended B2_FRAMES frame interval and ±2% assertion bounds.
Source: Path instructions
| // Accumulate T0OUT into a 16-bit sum until the cart says stop. MOV never touches carry, so the | ||
| // high byte's `adc #$00` picks up exactly the carry the low byte's add produced. | ||
| let poll = prog.here(); | ||
| prog.mov_a_dp(0xFD) // T0OUT, read-and-clear | ||
| .mov_dp_a(0x12) | ||
| .mov_a_dp(0x10) | ||
| .clrc() | ||
| .adc_a_dp(0x12) | ||
| .mov_dp_a(0x10) | ||
| .mov_a_dp(0x11) | ||
| .adc_a_imm(0x00) | ||
| .mov_dp_a(0x11) | ||
| .mov_a_dp(PORT3) | ||
| .cmp_a_imm(STOP); | ||
| prog.bne_back(poll); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The SPC poll loop is unbounded, and two documents state that it is bounded. The emitted program exits its accumulate loop only on the stop byte from port 3. There is no iteration counter. A cart that reaches @b2timeout after the upload leaves the SPC outside the IPL and un-released, which breaks every later APU upload.
tests/roms/AccuracySNES/gen/src/tests/apu.rs#L8923-L8937: add the iteration counter the doc comment at lines 8896-8902 describes, so the program falls through to publish andrelease_to_iplon its own; alternatively writeRELEASEon the@b2timeoutpath and correct the doc comment.CHANGELOG.md#L72-L75: after the code change, keep the "every wait on both sides is bounded" sentence; if the code is not changed, restrict the sentence to the cart side.
📍 Affects 2 files
tests/roms/AccuracySNES/gen/src/tests/apu.rs#L8923-L8937(this comment)CHANGELOG.md#L72-L75
🤖 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/gen/src/tests/apu.rs` around lines 8923 - 8937, Bound
the SPC polling loop around the visible poll label and T0OUT accumulation by
adding the iteration counter described in the nearby documentation, while
preserving exit on STOP and allowing fall-through to publish and release_to_ipl.
In CHANGELOG.md lines 72-75, retain the statement that every wait on both sides
is bounded after this code change.
Source: Path instructions
…caught it `[`B2_FRAMES`]` is a function-local constant, so rustdoc cannot resolve it -- a plain code span instead. Local `cargo doc --workspace` passed because CI runs it with `--exclude rustysnes-android`, and my earlier local run predated this row; running the gate's exact command reproduces it. ROM byte-identical, checksum still $5B04. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)Adds AccuracySNES test Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/roms/AccuracySNES/gen/src/tests/apu.rs (2)
8792-8858: 🩺 Stability & Availability | 🟠 MajorRelease the APU before reporting SKIP.
b2_07_driveuploads an SPC program before its bounded waits complete. If a timeout reaches the tagged SKIP handler while the SPC remains outside the IPL, the next APU test cannot upload reliably. Ensure every timeout path invokesrelease_to_iplor sends the release command before reporting SKIP.This repeats the previous review finding; verify the shared SKIP handler and every timeout branch.
#!/usr/bin/env bash set -euo pipefail file='tests/roms/AccuracySNES/gen/src/tests/apu.rs' sed -n '8792,9010p' "$file" rg -n -C 5 'apu_timeout_arm_tagged|release_to_ipl|RELEASE|SKIP|timeout|b2_07_drive' "$file"🤖 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/gen/src/tests/apu.rs` around lines 8792 - 8858, Update b2_07_drive and the shared tagged SKIP timeout flow so every timeout path releases the APU to the IPL before reporting SKIP. Ensure both `@b2timeout` branches invoke release_to_ipl or send the RELEASE command, and verify apu_timeout_arm_tagged and related timeout handlers do not bypass that release.
8792-8858: 🎯 Functional Correctness | 🟠 MajorStart APU accumulation on a frame boundary.
The changed flow waits for the start marker and then counts
B2_FRAMESvblanks. If the SPC starts accumulating before the first counted vblank, the interval is 47–48 frames instead of a fixed 48-frame interval. The NTSC and PAL lower bounds can reject a correct implementation when the start occurs late in a frame. Wait for one vblank before sending the start byte, then start the SPC timer on that byte.As per path instructions, timing-marginal measurements must use a settled or provably stationary value.
#!/usr/bin/env bash set -euo pipefail file='tests/roms/AccuracySNES/gen/src/tests/apu.rs' sed -n '8792,9010p' "$file" rg -n -C 4 'B2_FRAMES|wait_vblank_far|APUIO3|START|STOP|T0OUT' "$file"Also applies to: 8877-8877
🤖 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/gen/src/tests/apu.rs` around lines 8792 - 8858, Align the APU accumulation start with a frame boundary in b2_07_drive. After the readiness handshake, wait for one vblank, then send the start command/byte so the SPC begins timing at that boundary; count B2_FRAMES vblanks only after that start. Preserve the existing bounded timeout behavior and stop/result collection flow.Source: Path instructions
🤖 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.
Duplicate comments:
In `@tests/roms/AccuracySNES/gen/src/tests/apu.rs`:
- Around line 8792-8858: Update b2_07_drive and the shared tagged SKIP timeout
flow so every timeout path releases the APU to the IPL before reporting SKIP.
Ensure both `@b2timeout` branches invoke release_to_ipl or send the RELEASE
command, and verify apu_timeout_arm_tagged and related timeout handlers do not
bypass that release.
- Around line 8792-8858: Align the APU accumulation start with a frame boundary
in b2_07_drive. After the readiness handshake, wait for one vblank, then send
the start command/byte so the SPC begins timing at that boundary; count
B2_FRAMES vblanks only after that start. Preserve the existing bounded timeout
behavior and stop/result collection flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d4788ed3-48e1-4e07-a349-b1077fea204d
📒 Files selected for processing (1)
tests/roms/AccuracySNES/gen/src/tests/apu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: review
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/tests/apu.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/tests/apu.rs
**/*
📄 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:
tests/roms/AccuracySNES/gen/src/tests/apu.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/tests/apu.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/tests/apu.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/tests/apu.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/tests/apu.rs
🧠 Learnings (3)
📚 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/tests/apu.rs
📚 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/apu.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/apu.rs
🔇 Additional comments (3)
tests/roms/AccuracySNES/gen/src/tests/apu.rs (3)
180-216: 📐 Maintainability & Code QualityRemove the duplicated APU upload sequence.
upload_and_run_taggedstill emits the upload andAPUIO0-clear sequence at Line 221-248. Verify thatupload_onlydoes not duplicate this logic. If both bodies contain it, makeupload_and_run_taggedcallupload_onlyand retain only its wait, result-copy, and release logic.This repeats the previous review finding; verify the current function bodies before merging.
#!/usr/bin/env bash set -euo pipefail sed -n '180,280p' tests/roms/AccuracySNES/gen/src/tests/apu.rs
28-28: LGTM!Also applies to: 152-152, 399-405
8860-9009: 🗄️ Data Integrity & IntegrationKeep the measurement-slot and
$213Flogic unchanged. The 35-entry sweep ends at slots 231 and 232, so slots 275 and 276 are unclaimed.measure_frame_heightreads$213Fonly to reset flip-flops before$2137;$4201is initialized and restored.> Likely an incorrect or invalid review comment.
"60.0988 Hz" is a claim about wall-clock time, and a cart has no wall clock. The one independent timebase it can reach is the APU's: the SPC700 runs from its own 24.576 MHz crystal at the same rate in both regions, so counting APU timer ticks over a known number of frames measures the frame rate in units of that crystal.
Timer 0 at
T0DIV = 1stepsT0OUTevery 256 SMP base clocks — 8,010 ticks a second on either console.1,291 apart, keyed on the measured frame height rather than the region bit — whose position
B2.10had to settle, and which a frame-rate test must not depend on.It only became a real measurement last PR
The SPC divisor used to be pinned to the NTSC master clock, which made the APU scale with the video clock. An APU that speeds up exactly when the frames get shorter cannot measure how long a frame is — this row would have read the same number on both images. #316 is what made it meaningful.
What it pins, stated honestly
The band is ±2%, which pins the rate to about a part in fifty. That is not the precision the quoted figures have, and the doc comment says so: distinguishing 60.0988 from a flat 60.0 is one part in 600 and needs a count ten times longer than the battery should spend. What this row excludes is a core whose frame rate is wrong by a scanline count or whose APU is region-scaled — both percent-level errors, and one of which this emulator actually had.
The handshake
The cart cannot sit in
upload_and_run's wait loop while it counts frames, so the row drives the program itself through a newupload_only: the SPC announces it is accumulating, the cart counts vblanks, then writes a stop byte on port 3. Every wait on both sides is bounded — an APU that never answers reports SKIP rather than hanging the battery.And it flipped
F1.10again, on a third host/imageAdding it made Mesen2's PAL verdict on
F1.10flip between runs of a single build: two runs of the same image gaveF1.03alone, thenF1.03+F1.10. That row samples$4212right at the vblank edge, so any test added ahead of Group F moves the phase that decides it.mesen_crossval.luanow skips one index supplied throughACCURACYSNES_SKIP_INDEX, whichcrossval.shresolves by ID fromSOURCE_CATALOG.tsv— the index moves whenever a test is added ahead of it, which is exactly the circumstance that exposed the problem. The row is still logged, just not counted, and the ares gate now shares the same resolved index instead of computing its own.Three consecutive full cross-validation runs are stable, which is the check that matters for a gate that was flaking.
Verification
cargo fmt --check, workspace clippy at-D warnings, and the 56-test AccuracySNES harness suite pass.snes9x: OK (15 known),Mesen2: OK (1 known),ares: OK (3 known),snes9x PAL: OK (15 known),Mesen2 PAL: OK (1 known), 54/54 scenes on both scene hosts,3 reference(s) agree with the cart.Coverage 355 → 356 of 443 (302 on-cart + 54 scenes); battery 343 tests, 100% on-cart.
🤖 Generated with Claude Code
Summary
Adds AccuracySNES dossier assertions
B2.07andB2.08. The tests measure APU timer ticks over 48 frames and use measured frame height to distinguish NTSC and PAL timing.The claim is false if the tests use wall-clock time or the region bit, misclassify frame timing, or report success when the APU does not respond. Bounded waits and
SKIPreporting prevent an unresponsive APU from producing a verdict.Cross-validation excludes
F1.10by catalogue ID on both Mesen2 and ares. The row remains logged. The stability claim is false if the three full runs disagree or if the excluded row affects verdict counts.AccuracySNES coverage increases from 355/443 to 356/443. The battery increases from 342 to 343 tests. On-cart coverage increases from 301 to 302. Rendered-scene coverage remains 54.