feat(apu): implement the SMP wait states; E3.09 measures them - #313
Conversation
… them
$F0 bits 4-5 and 6-7 select a clock divider for the SMP, nominally
{2, 4, 8, 16} -- but 8 and 16 are glitchy on real silicon and the CPU
consumes 10 and 20 clocks per opcode cycle while the timers still
advance by 8 and 16. ares and bsnes carry the same comment
(sfc/smp/timing.cpp): "the timers are not affected by this and advance
by their expected values." Two tables, and the gap between them is what
the new row measures.
RustySNES parsed both selectors into Io::external_wait /
Io::internal_wait, saved them, restored them -- and nothing downstream
ever read either one. The eighth instance of the dead-config defect
class, and the first the cart found rather than grep.
rustysnes-apu now carries SMP_CYCLE_WAIT = [2,4,10,20] for the CPU (and
so for the recorded micro-op plan and the S-DSP catch-up, which are real
base clocks) against SMP_TIMER_WAIT = [2,4,8,16] for the timers alone,
with ares' SMP::wait address classification: idle cycles, $00F0-$00FF
and a mapped IPL ROM take the internal selector, everything else the
external one. At the reset selector both tables read SMP_WAIT, so every
program that leaves $F0 alone -- which is every commercial driver -- is
byte-identical to before.
E3.09 reads the gap as a ratio the program can see: a selector changes
clocks-per-cycle, not an instruction's cycle count, so the same loop is
the same number of opcode cycles either way and only the timer-per-cycle
rate moves. Over a fixed 48-pass poll loop timer 0 ticks 4x as often at
selector 2 as at selector 0. Both wrong models were injected and both
fail on code 2: no wait states reads 1x, charging the CPU's glitchy 10
to the timers as well reads 5x. The row separates the two ways of having
the feature, not merely its absence.
Mesen2 and ares both pass it; snes9x fails it off the same missing
`case 0xf0` that already costs it E3.08 and E3.10, so
SNES9X_KNOWN_FAILURES goes 14 -> 15 with that citation.
Coverage 352 -> 353 of 443 (299 on-cart + 54 scenes), battery 340 tests
at 100% on-cart, three references agree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 24 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 (6)
📒 Files selected for processing (7)
WalkthroughThe APU now applies selector- and address-dependent CPU and timer wait-state timing. Bus, recording, save-state validation, documentation, and AccuracySNES coverage were updated. The new E3.09 test verifies selector 2 produces approximately four times the timer ticks. ChangesSMP wait-state timing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AccuracySNESTest
participant RustySNESAPU
participant Timer0
AccuracySNESTest->>RustySNESAPU: Set normal $F0 timing
RustySNESAPU->>Timer0: Accumulate timer ticks
AccuracySNESTest->>RustySNESAPU: Set selector 2
RustySNESAPU->>Timer0: Accumulate timer ticks
AccuracySNESTest->>AccuracySNESTest: Assert approximately 4x ticks
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Implements the SPC700/SMP $F0 wait selectors in rustysnes-apu, including the documented “glitch” where opcode-cycle cost (CPU) diverges from timer advancement (timers), and adds an AccuracySNES row (E3.09) that measures the expected 4x ratio to distinguish correct behavior from common incorrect models.
Changes:
- Add separate wait tables for CPU vs timers and apply ares-style internal/external address classification (incl. IPL-mapped region handling).
- Add AccuracySNES
E3.09test program + documentation/error-code/catalog updates and regenerate emitted artifacts. - Update docs/cross-validation metadata and project status/changelog coverage counts.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/rustysnes-apu/src/lib.rs | Implements split CPU/timer wait-state model, address classification, recording/stepping changes, and adds unit tests |
| tests/roms/AccuracySNES/gen/src/tests/apu.rs | Adds E3.09 generator-side test definition and helper program logic |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Adds dossier mapping entry for E3.09 |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Regenerated ROM assembly including E3.09 test entry + program bytes |
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Regenerated catalog entry for E3.09 |
| tests/roms/AccuracySNES/ERROR_CODES.md | Regenerated error-code documentation for E3.09 |
| scripts/accuracysnes/crossval.sh | Documents snes9x divergence and increments known-failures count |
| docs/apu.md | Documents $F0 selector model and the two-table rationale |
| docs/STATUS.md | Updates AccuracySNES coverage and battery counts |
| docs/accuracysnes-plan.md | Updates counts and notes the newly found defect/row |
| docs/accuracysnes-coverage.md | Updates coverage totals and E3 subgroup coverage list |
| CHANGELOG.md | Adds release notes entry for E3.09 and SMP wait-state implementation |
| const fn wait_index(&self, address: Option<u16>) -> usize { | ||
| let internal = match address { | ||
| None => true, | ||
| Some(a) => a & 0xFFF0 == 0x00F0 || (a >= 0xFFC0 && self.iplrom_enable), | ||
| }; | ||
| (if internal { | ||
| self.internal_wait | ||
| } else { | ||
| self.external_wait | ||
| }) as usize | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/rustysnes-apu/src/lib.rs`:
- Around line 292-314: Update Io::load_state to validate external_wait and
internal_wait immediately after reading them, rejecting any value above 3 with
the established typed SaveStateError::Invalid pattern used for Apu::load_state
instead of allowing invalid values to reach wait_index and wait_clocks.
In `@docs/accuracysnes-plan.md`:
- Around line 16-17: Update the SNES9X recorded divergence count on Line 20 of
the accuracy plan from 14 to 15, matching SNES9X_KNOWN_FAILURES in crossval.sh
and keeping the documentation metrics synchronized.
In `@docs/apu.md`:
- Around line 362-363: Update the public timing descriptions to make reset
compatibility conditional on $F0 remaining unchanged: in docs/apu.md lines
362-363 and CHANGELOG.md lines 28-29, replace the universal claim about every
commercial driver with wording that applies only to programs that leave $F0
unchanged.
🪄 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: bfe11eeb-a03d-47f1-897f-f5c0ccf01545
⛔ 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-apu/src/lib.rsdocs/STATUS.mddocs/accuracysnes-plan.mddocs/apu.mdscripts/accuracysnes/crossval.shtests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (18)
**/*
📄 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.shdocs/apu.mdtests/roms/AccuracySNES/gen/src/dossier.rsdocs/accuracysnes-plan.mddocs/STATUS.mdtests/roms/AccuracySNES/gen/src/tests/apu.rsCHANGELOG.mdcrates/rustysnes-apu/src/lib.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
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/apu.mddocs/accuracysnes-plan.mddocs/STATUS.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/apu.mdtests/roms/AccuracySNES/gen/src/dossier.rsdocs/accuracysnes-plan.mddocs/STATUS.mdtests/roms/AccuracySNES/gen/src/tests/apu.rsCHANGELOG.mdcrates/rustysnes-apu/src/lib.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/apu.mddocs/accuracysnes-plan.mddocs/STATUS.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/apu.mddocs/accuracysnes-plan.mddocs/STATUS.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/apu.mddocs/accuracysnes-plan.mddocs/STATUS.mdCHANGELOG.md
**/*.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/apu.rscrates/rustysnes-apu/src/lib.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
**/*.{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/apu.rscrates/rustysnes-apu/src/lib.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
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-apu/src/lib.rs
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Files:
crates/rustysnes-apu/src/lib.rs
crates/rustysnes-apu/**/*.rs
📄 CodeRabbit inference engine (crates/rustysnes-apu/CLAUDE.md)
Before changing the SPC700 core, DSP mixing, or APU/main-CPU synchronization, read the specification at
../../docs/apu.md.
Files:
crates/rustysnes-apu/src/lib.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-apu/src/lib.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-apu/src/lib.rs
🧠 Learnings (5)
📚 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-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/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)
docs/apu.md (1)
342-361: LGTM!Also applies to: 364-367
CHANGELOG.md (1)
14-27: LGTM!Also applies to: 31-40
scripts/accuracysnes/crossval.sh (1)
95-103: LGTM!Also applies to: 178-178
docs/STATUS.md (1)
26-27: LGTM!Also applies to: 244-244
docs/accuracysnes-plan.md (1)
18-19: LGTM!Also applies to: 21-22
crates/rustysnes-apu/src/lib.rs (2)
676-681: 🗄️ Data Integrity & IntegrationConfirm whether this save-state validation widening needs a FORMAT_VERSION bump.
This change widens the accepted range for a saved plan step's
base_clocksfrom the old restriction (SMP_WAITor its half) to0 < base_clocks <= MAX_PLAN_BASE_CLOCKS. No new field is added and the byte layout is unchanged, but the referenced ADR ties save-state validation changes in this exact PR to a version-bump requirement.Confirm whether
docs/adr/0006andFORMAT_VERSIONneed an entry for this change, or whether the team's position is that widening validation for an existing field (without a layout change) is exempt.As per path instructions for
crates/**: "Any change to save-stated fields needs aFORMAT_VERSIONbump and adocs/adr/0006bump-log entry," and the referenced ADR notes: "Because this PR changes APU/SMP timing state and save-state validation, preserve deterministic save/load behavior."Also applies to: 703-706
Source: Path instructions
49-81: LGTM!Also applies to: 770-782, 878-893, 902-904, 938-946, 1064-1071, 1086-1089, 1102-1102, 1150-1195
tests/roms/AccuracySNES/gen/src/tests/apu.rs (2)
150-150: LGTM!Also applies to: 8449-8523, 8533-8577, 8579-8601
8524-8532: 🎯 Functional CorrectnessNo slot collision exists for slots 268 or 269.
> Likely an incorrect or invalid review comment.tests/roms/AccuracySNES/gen/src/dossier.rs (1)
273-273: LGTM!
| const fn wait_index(&self, address: Option<u16>) -> usize { | ||
| let internal = match address { | ||
| None => true, | ||
| Some(a) => a & 0xFFF0 == 0x00F0 || (a >= 0xFFC0 && self.iplrom_enable), | ||
| }; | ||
| (if internal { | ||
| self.internal_wait | ||
| } else { | ||
| self.external_wait | ||
| }) as usize | ||
| } | ||
|
|
||
| /// The `(cpu, timer)` base-clock pair one access costs, halved for the split `$F4-$F7` reads. | ||
| /// | ||
| /// Returned as a pair rather than resolved separately at each call site because the whole point | ||
| /// of the glitch is that the two numbers differ; computing them apart invites one of them to be | ||
| /// derived from the wrong table. | ||
| const fn wait_clocks(&self, halve: bool, address: Option<u16>) -> (u32, u32) { | ||
| let idx = self.wait_index(address); | ||
| let shift = if halve { 1 } else { 0 }; | ||
| (SMP_CYCLE_WAIT[idx] >> shift, SMP_TIMER_WAIT[idx] >> shift) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Out-of-range $F0 wait selector from a save state panics on indexing.
wait_index casts self.internal_wait/self.external_wait directly to usize and wait_clocks indexes SMP_CYCLE_WAIT/SMP_TIMER_WAIT (4-element arrays) with that index. The live write path always masks these fields with & 0x03, so normal execution never produces an out-of-range value. Io::load_state does not: it assigns self.external_wait = s.read_u8()?; and self.internal_wait = s.read_u8()?; directly, with no bounds check.
A save-state file with either byte set above 3 makes wait_index return an index of up to 255. The first subsequent APU access calls wait_clocks, which indexes SMP_CYCLE_WAIT[idx]/SMP_TIMER_WAIT[idx] and panics.
Before this PR these fields were parsed but never consulted, so the missing validation in Io::load_state was harmless. This PR's wait_index/wait_clocks are the first code to read them as array indices, turning that gap into a crash on untrusted save-state input.
Add the bounds check in Io::load_state, consistent with how Apu::load_state already rejects an out-of-range base_clocks (lines 703-706) with a typed error instead of a panic.
The coding guideline states: "Do not introduce .unwrap(), .expect(), or panic!() on untrusted external input—such as ROM/save-state bytes... Use typed errors at those boundaries."
🛡️ Proposed fix in `Io::load_state` (outside the selected range, around line 334)
self.external_wait = s.read_u8()?;
self.internal_wait = s.read_u8()?;
if self.external_wait > 3 || self.internal_wait > 3 {
return Err(SaveStateError::Invalid(alloc::format!(
"APU wait selector external={} internal={} exceeds the 2-bit field range (0..=3)",
self.external_wait, self.internal_wait
)));
}🤖 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-apu/src/lib.rs` around lines 292 - 314, Update
Io::load_state to validate external_wait and internal_wait immediately after
reading them, rejecting any value above 3 with the established typed
SaveStateError::Invalid pattern used for Apu::load_state instead of allowing
invalid values to reach wait_index and wait_clocks.
Source: Coding guidelines
| | Tests | **340** (scoring + golden vectors + region SKIP per image) — *tests, not assertions; see the note below the table* | | ||
| | Assertion coverage | **353 of 443** dossier assertions — **299 on-cart** + **54 rendered scenes**, kept as separate columns (`docs/accuracysnes-coverage.md`) | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Synchronize the SNES9X divergence count.
The changed metrics row is paired with Line 20, which still says SNES9X has 14 recorded divergences. scripts/accuracysnes/crossval.sh now sets SNES9X_KNOWN_FAILURES to 15 after E3.09.
Change Line 20 to 15.
As per path instructions, docs are the spec and cross-file claims must remain synchronized.
🤖 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 16 - 17, Update the SNES9X recorded
divergence count on Line 20 of the accuracy plan from 14 to 15, matching
SNES9X_KNOWN_FAILURES in crossval.sh and keeping the documentation metrics
synchronized.
Source: Path instructions
| At the reset selector both tables read `SMP_WAIT`, so this is byte-identical to the previous | ||
| single-number model for every program that leaves `$F0` alone — which is every commercial driver. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep reset compatibility conditional in both public descriptions.
A nonzero wait selector in $F0 changes timing. The implementation guarantees byte-identical behavior only when $F0 remains at reset. Both entries add the unsupported claim that every commercial driver leaves $F0 unchanged.
docs/apu.md#L362-L363: replace the universal claim with “for programs that leave$F0unchanged.”CHANGELOG.md#L28-L29: apply the same conditional wording.
Proposed wording
-for every program that leaves `$F0` alone — which is every commercial driver.
+for programs that leave `$F0` unchanged.As per path instructions, docs are the spec, so public timing descriptions must match the conditional behavior.
📝 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.
| At the reset selector both tables read `SMP_WAIT`, so this is byte-identical to the previous | |
| single-number model for every program that leaves `$F0` alone — which is every commercial driver. | |
| At the reset selector both tables read `SMP_WAIT`, so this is byte-identical to the previous | |
| single-number model for programs that leave `$F0` unchanged. |
📍 Affects 2 files
docs/apu.md#L362-L363(this comment)CHANGELOG.md#L28-L29
🤖 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/apu.md` around lines 362 - 363, Update the public timing descriptions to
make reset compatibility conditional on $F0 remaining unchanged: in docs/apu.md
lines 362-363 and CHANGELOG.md lines 28-29, replace the universal claim about
every commercial driver with wording that applies only to programs that leave
$F0 unchanged.
Source: Path instructions
…them The $00F0-$00FF register block wins every SPC read of those addresses, so "a write also lands in RAM" cannot be checked by writing and reading back. It needs a second reader of APU RAM that skips the register decode, and the S-DSP is one: it fetches BRR sample data straight out of ARAM. A nine-byte block written THROUGH the register block at $00F7 ends exactly at $00FF, never reaching the directory at $0100, and clears the two addresses that would break the run -- $F0 (TEST, the wait states and the RAM-write enable) and $F1 (CONTROL, which would unmap the IPL ROM and strand release_to_ipl). Three of its nine bytes land on $FD-$FF, which are read-only, so for those the shadow is the only thing a write could have affected. The assertion is EQUALITY against a control voice playing a byte-identical copy in ordinary RAM, not "the shadow read back non-zero". What sits under the register block at power-on is undefined and one reference randomises APU RAM, so a non-zero reading could be luck; nine bytes decoding to the control's exact OUTX cannot be. The control doubles as the guard. Injecting `address & 0xFFF0 != 0x00F0` into both bus write paths fires code 2 with the guard passing. Two setup faults the guard caught rather than letting the row pass on two zeroes: KOF is a level register a previous program can leave set, and KON must be HELD -- it is examined once every two output samples (E8.01), so a write immediately followed by a clear is cancelled before the DSP ever looks. Coverage 353 -> 354 of 443 (300 on-cart + 54 scenes), battery 341 tests at 100% on-cart, three references agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by a dead-config sweep of the APU while writing E3.13: the field is written at $4C and serialized, and nothing downstream reads it. `keylatch` is what actually keys a voice on -- misc29/misc30 clear it against `_keyon` once per sample, which is what makes a second $4C write cancel a pending key-on (E8.01). Not an accuracy bug: the behaviour is correct because it comes from the latch. But a reader who mistook this field for the authoritative flag would reintroduce the "keys on for as long as the bit is set" bug the latch exists to prevent -- and that is exactly what silenced both voices on E3.13's first run, from the cart side. Kept rather than removed: it is part of the serialized layout, so deleting it is a save-state format change and not worth one here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR implements the SPC700/SMP hardware wait states controlled by register Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
The gap between two tables
$F0bits 4-5 (external) and 6-7 (internal) select a clock divider for the SMP, nominally{2, 4, 8, 16}. Dividers of 8 and 16 are glitchy on real silicon: the CPU ends up consuming 10 and 20 clocks per opcode cycle while the timers still advance by 8 and 16. ares and bsnes carry the same comment (sfc/smp/timing.cpp) — "sometimes the SMP will run far slower than expected, other times … the SMP will deadlock until the system is reset. The timers are not affected by this and advance by their expected values."What the cart found
RustySNES parsed both selectors into
Io::external_wait/Io::internal_wait, saved them, restored them — and nothing downstream ever read either one. Eighth instance of the dead-config defect class in this repo, and the first found by the test cartridge rather than by grep.rustysnes-apunow carries:SMP_CYCLE_WAIT = [2,4,10,20]SmpBus::cycles, the recorded micro-op plan, the S-DSP catch-up — all real base clocksSMP_TIMER_WAIT = [2,4,8,16]with ares'
SMP::waitaddress classification: idle cycles,$00F0-$00FF, and the IPL ROM while$F1bit 7 keeps it mapped take the internal selector; everything else external.$F4-$F7reads keep their split halved steps.At the reset selector both tables read
SMP_WAIT, so every program that leaves$F0alone — which is every commercial driver — is byte-identical to before. A unit test pins that explicitly.E3.09reads the gap as a ratioA wait selector changes clocks-per-opcode-cycle, not an instruction's cycle count, so the same loop is the same number of opcode cycles in both phases and only the timer-per-cycle rate moves. Over a fixed 48-pass poll loop with
T0DIV = 1:So the row separates the two ways of having the feature, not merely its absence. Both wrong models were injected and both fail on code 2.
Three
$F0bits are not the subject and must not move: the timer halt (0), the RAM write enable (1) and the global timer enable (3). Phase B is$AA, not a bare$A0— clearing bit 1 would drop everyMOV dp,Ain the poll loop and the row would read as a timing finding when it was a store that never happened. Both selectors are set together because the loop mixes external accesses (fetches) with internal ones ($FD).Cross-validation
Mesen2 and ares both pass it. snes9x fails it off the same missing
case 0xf0that already costs itE3.08andE3.10— three rows from one absent switch case — soSNES9X_KNOWN_FAILURESgoes 14 → 15 with that citation. 3-vs-1, snes9x the documented outlier.Verification
cargo fmt --check, workspace clippy at-D warnings,RUSTDOCFLAGS="-D warnings" cargo doc, theno_stdthumbv7em build,cargo test --workspace(68 suites), and the 56-test AccuracySNES harness suite all pass.docs/apu.mdgains the selector model in the same change.Coverage 352 → 353 of 443 (299 on-cart + 54 scenes); battery 340 tests, 100% on-cart.
🤖 Generated with Claude Code
Implements
$F0-selected SMP wait states. CPU and DSP timing uses[2, 4, 10, 20]; timers use[2, 4, 8, 16]. Address classification selects internal or external waits, and write timing occurs before side effects. The observable change is selector-dependent CPU and timer progression. The claim is false if$F0selectors do not change timing as specified, or if reset behavior changes when$F0remains unchanged.Adds AccuracySNES dossier assertion
E3.09. Coverage increases from 352/443 to 353/443 assertions. The test detects the defect when selector 2 fails to produce approximately four times the timer ticks of normal$F0timing.Updates APU documentation, cross-validation results, and the SNES9X known-failure list.