feat(accuracysnes): add the full 256-opcode SPC700 cycle sweep (E2.10) - #330
Conversation
Coverage 359 -> 360 of 443. The cart measures how long every opcode the SPC700 can execute in a straight line actually takes, and compares it ON-CART against the cycle count fullsnes documents for it. The host supplies no expected values; it reads back three bytes -- opcodes measured, opcodes disagreeing, and the first one that did. How one opcode is timed. T2OUT steps once every 16 opcode cycles, far too coarse for a single instruction, so the sweep never times one. It times a block of six copies, sixteen times over, against the same block built from NOP. Everything around the copies -- a fixed four-byte prologue, a fixed five-byte epilogue, the CALL, the register reset, the poll -- is identical in every arm and cancels in the difference, leaving six ticks per cycle against a quantisation of +/-1 on each side. Branches are measured TAKEN. A relative branch with a displacement of zero lands on the following instruction, which is where a not-taken branch would have gone anyway, so a block of six runs straight through whichever way each one goes and the arm can arrange the taken path. The prologue that arranges it is two two-cycle immediates whatever flag it has to set, so choosing one costs nothing that could leak into a difference. The table is built from fullsnes's own rules and the build fails if they do not tile the map. The operand KIND is recorded at construction rather than derived from the opcode byte -- the first draft derived it from the low nibble, which is nearly right and hides at least four traps, one of which gave MOV [aa+X],A a pointer read from uninitialised memory, which is zero, which is the driver's own variables. Twenty-five opcodes are excluded by name and with reasons: the absolute jumps and calls, the vectored calls whose vectors are in the IPL ROM, the returns, and SLEEP/STOP, for which fullsnes itself gives the cycle count as `?`. The count of what WAS measured is asserted on-cart at 231, so a sweep covering a different set fails rather than quietly reporting no disagreements. This also lands E2.10i, the smaller instrument the sweep grew out of: a NOP/XCN/MUL YA differential on a different shape (eight copies, thirty-two iterations) that reports its three raw tick totals rather than a verdict. It stays as a golden because those totals cross-validate directly, where the sweep's answer is a pass/fail count. Verified by injection twice. One extra idle cycle in XCN -> exactly one disagreement, first-bad $9F, the opcode broken. One idle cycle removed from the taken relative branch -> exactly nine, first-bad $10: the eight conditional branches plus BRA, which is also the proof that the taken-path prologues really do take the branch in all eight conditions. Two things had to be measured rather than reasoned about. The results page was first at $0900 and the image reached $0948, so the sweep overwrote its own driver as it recorded; a build-time assertion now rejects that layout. And the shared bounded APU wait is a fraction of a frame, right for every other Group E program and far too short for this one, which runs about 42 frames -- the row reported SKIP while the SPC was working correctly and had already reached the right answer in RAM. Battery 310/310. Three references agree: snes9x, Mesen2 and ares all report 231 measured and zero disagreements, with their known-failure counts unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe PR adds a rule-based SPC700 opcode table, assembler support, extended APU waits, timer measurements, and the AccuracySNES E2.10 sweep. The sweep measures 231 straight-line opcodes and reports results through SNES ports. ChangesSPC700 cycle sweep
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant E210Test
participant APUProgram
participant Timer2
participant SNESPorts
E210Test->>APUProgram: Build opcode tables and execution blocks
APUProgram->>Timer2: Execute opcode blocks and count ticks
Timer2-->>APUProgram: Return timer totals
APUProgram->>SNESPorts: Publish disagreement and count results
SNESPorts-->>E210Test: Return measured results
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 an on-cart AccuracySNES Group E test (E2.10) that sweeps all straight-line SPC700 opcodes and verifies their measured timings against fullsnes-documented cycle counts, increasing overall coverage to 360 / 443.
Changes:
- Introduces the
E2.10256-opcode SPC700 cycle sweep test driver (RAM-built blocks + on-cart comparison). - Adds
E2.10ias a golden “instrument validation” row and updates dossier/coverage/error-code documentation accordingly. - Extends the generator’s SPC DSL (
Spc) with forward-branch/jump patching to support large generated control flow.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Registers E2.10/E2.10i in the catalog and updates slot mapping. |
| tests/roms/AccuracySNES/gen/src/tests/mod.rs | Wires the new APU sweep test module into the generator test list. |
| tests/roms/AccuracySNES/gen/src/tests/apu.rs | Adds E2.10i, adds a long bounded APU wait helper, and exposes APU upload/timeout helpers for reuse. |
| tests/roms/AccuracySNES/gen/src/tests/apu_sweep.rs | Implements E2.10 (build opcode tables, assemble per-opcode blocks in RAM, measure/compare, report summary). |
| tests/roms/AccuracySNES/gen/src/spc.rs | Extends the SPC700 program emitter with forward-branch and long-range jump patching utilities used by the sweep. |
| tests/roms/AccuracySNES/gen/src/spc_opcodes.rs | Defines the full SPC700 opcode table (length/cycles/operands/measurability) derived from fullsnes rules, with table-completeness tests. |
| tests/roms/AccuracySNES/gen/src/main.rs | Adds the new spc_opcodes module to the generator build. |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Adds dossier mapping for E2.10 and explains why E2.10i is unenumerated/golden. |
| tests/roms/AccuracySNES/ERROR_CODES.md | Documents E2.10 failure codes and the golden nature of E2.10i. |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Generated assembly updates to include the new tests and embedded SPC payloads. |
| docs/accuracysnes-coverage.md | Updates coverage totals and lists E2.10i under non-enumerated tests. |
| crates/rustysnes-test-harness/tests/accuracysnes.rs | Updates harness measurement-slot labeling to include E2.10/E2.10i slots. |
| CHANGELOG.md | Notes the addition of E2.10 and the resulting coverage increase. |
Suppressed comments (1)
tests/roms/AccuracySNES/gen/src/tests/apu.rs:9229
- This doc comment is internally inconsistent (it says the 8-bit sum “cannot overflow” and then explains it does), and it references the old doubled expected differences (96/224) even though the instrument section above establishes the correct expected differences as 48/112 ticks. This can mislead future edits of the instrument sizing/validation logic.
/// The sum is 8-bit and the arms are sized so it cannot overflow: the slowest arm here is ~11 ticks
/// an iteration over 32 iterations, about 350 — which does NOT fit in eight bits, so the low byte is
/// what reaches the port and the caller compares differences rather than absolutes. Differences of
/// the low byte are still exact while the arms stay within 256 ticks of each other, which the
/// expected 96 and 224 both do.
| fn e2_10_instrument() -> Test { | ||
| /// Copies of the opcode per iteration. Bounded by `T2OUT`'s four bits — see the doc comment. | ||
| const REPEATS: usize = 8; | ||
| /// Iterations. `REPEATS * ITERATIONS / 8` is the ticks-per-cycle-of-difference resolution. |
| #[derive(Debug)] | ||
| #[must_use = "a forward branch that is never patched branches to itself"] | ||
| pub struct Fwd(usize); |
| /// The measurement slots the `v1.29.0` Group E batch records. | ||
| const GROUP_E_BATCH_SLOTS: [(u16, &str); 19] = [ | ||
| const GROUP_E_BATCH_SLOTS: [(u16, &str); 25] = [ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@tests/roms/AccuracySNES/gen/src/spc_opcodes.rs`:
- Around line 183-187: Update the SINGLES documentation comment to list the
tuple fields in their actual order, including operands between measure and name.
In `@tests/roms/AccuracySNES/gen/src/spc.rs`:
- Around line 38-40: Update the #[must_use] message on the Fwd type to describe
the actual unpatched behavior: branch_fwd leaves a zero displacement that falls
through to the following instruction and can silently skip the guarded body.
Keep the Fwd definition and attribute unchanged apart from correcting this
diagnostic text.
- Around line 299-302: Remove the duplicate public inc_x_reg method from the
opcode builder, keeping the existing inc_x implementation as the sole emitter
for opcode $3D. Update all inc_x_reg call sites in apu_sweep.rs to invoke inc_x
instead.
- Around line 249-280: Correct the documentation on `Spc::jmp_fwd` so it no
longer describes the nonexistent `base` parameter, and place that upload-address
explanation on `patch_jmp_fwd` or `jmp_back`. Add `# Panics` sections to both
`patch_jmp_fwd` and `jmp_back` documenting that they panic when the program
length or target offset cannot convert to `u16`, matching their existing
`expect` calls.
In `@tests/roms/AccuracySNES/gen/src/tests/apu_sweep.rs`:
- Around line 203-209: Update the operand documentation table in the module docs
to use `$0E00` for both the absolute operand and bit address, matching
`ram::WINDOW` used by `operand_bytes`. Preserve the rationale that this window
is separate from the image, block, and stack; do not change the code or refer to
`$0C00`, which is `ram::RESULTS`.
- Around line 459-466: Add a unit test covering the arithmetic used by the APU
sweep expected-value calculation: verify every documented cycle count in table
keeps (cycles minus two) multiplied by TICKS_PER_CYCLE below 256, so MUL YA’s
high byte remains zero before the driver reads A. Use the existing Measure and
table imports and make the test fail when future timing constants or entries
violate this invariant.
- Around line 548-556: Update the AccuracySNES coverage count in
docs/accuracysnes-plan.md from 359 of 443 to 360 of 443 to account for the
mapped E2.10 assertion in the test flow, while leaving E2.10i and its
UNENUMERATED justification unchanged.
In `@tests/roms/AccuracySNES/gen/src/tests/apu.rs`:
- Around line 9165-9166: Correct the remaining halved tick-rate documentation in
e2_10_instrument: at tests/roms/AccuracySNES/gen/src/tests/apu.rs lines
9165-9166, update the REPEATS * ITERATIONS divisor from 8 to 16; at lines
9140-9140, change “16 ticks = 128 cycles” to “16 ticks = 256 cycles”; and at
lines 9225-9229, change expected differences “96 and 224” to “48 and 112” and
remove the contradictory claim that their sum cannot overflow.
In `@tests/roms/AccuracySNES/gen/src/tests/mod.rs`:
- Line 31: Add a concise comment immediately before the apu_sweep::e2_10() push
documenting that it must remain after apu::all() because it perturbs timer 2 for
E3.06, while apu::all() must remain last within its own APU sequence due to the
noise-LFSR constraint. Keep the existing ordering 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: 649e436e-cee8-4dfa-8c9a-1fd622fe0ea4
⛔ 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 (9)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/tests/mod.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
- GitHub Check: review
🧰 Additional context used
📓 Path-based instructions (13)
**/*.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/mod.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/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/mod.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/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/mod.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rsCHANGELOG.mdtests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/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/mod.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.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/tests/mod.rscrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.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/tests/mod.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.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/tests/mod.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
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
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
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
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
CHANGELOG.md
tests/roms/AccuracySNES/gen/src/spc.rs
📄 CodeRabbit inference engine (AGENTS.md)
Verify every new SPC700 opcode encoding against
crates/rustysnes-apu/src/spc700_exec.rs; emit only encodings exercised by committed tests.
Files:
tests/roms/AccuracySNES/gen/src/spc.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/mod.rstests/roms/AccuracySNES/gen/src/main.rstests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.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/mod.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/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/mod.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
🔇 Additional comments (13)
tests/roms/AccuracySNES/gen/src/dossier.rs (2)
103-104: LGTM!
521-530: LGTM!CHANGELOG.md (1)
12-62: LGTM!tests/roms/AccuracySNES/gen/src/main.rs (1)
21-21: LGTM!tests/roms/AccuracySNES/gen/src/spc_opcodes.rs (2)
690-739: LGTM!
527-534: 🗄️ Data Integrity & IntegrationNo change required. The executor matches all finite excluded costs.
SLEEPandSTOPhave no finite documented cost; their seven-cycle executor window is not an instruction duration.tests/roms/AccuracySNES/gen/src/spc.rs (2)
198-237: LGTM!Also applies to: 309-327
239-247: 🎯 Functional CorrectnessNo change required. All fifteen encodings match
spc700_exec.rs, and committedapu_sweepcode exercises each one.tests/roms/AccuracySNES/gen/src/tests/apu.rs (3)
9230-9255: LGTM!
154-158: 🎯 Functional CorrectnessRemove this finding.
e9_02does not read or enable timer 2. It controls the noise clock through DSP$6C, ande2_10_instrumentdisables timer 2 before release.> Likely an incorrect or invalid review comment.
240-241: 🎯 Functional CorrectnessNo collision is present.
upload_and_run_longis used only byE2.10;$7E01F8is initialized before use, and other users are in separate test bodies.E4.03uses tagged labels, so@waitand@rando not collide. The flag handling is correct.> Likely an incorrect or invalid review comment.tests/roms/AccuracySNES/gen/src/tests/apu_sweep.rs (1)
353-431: LGTM!crates/rustysnes-test-harness/tests/accuracysnes.rs (1)
629-634: 🗄️ Data Integrity & IntegrationNo slot collision exists for slots 277–282. The computed sweep writer emits slots 8–75 and 231–232. E2.10 uses slots 277–282.
> Likely an incorrect or invalid review comment.
| /// `INC X` — `$3D`. | ||
| pub fn inc_x_reg(&mut self) -> &mut Self { | ||
| self.push(&[0x3D]) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
inc_x_reg duplicates the existing inc_x.
Line 366 of this same file already defines pub fn inc_x(&mut self) -> &mut Self with the identical body self.push(&[0x3D]) and the identical doc comment /// INC X—$3D.. This adds a second public name for it.
Two emitters for one opcode is the failure the file guards against elsewhere — release_to_ipl's comment says "an opcode spelled out twice is an opcode that can disagree with itself". Delete inc_x_reg and call inc_x from apu_sweep.rs.
Proposed fix
- /// `INC X` — `$3D`.
- pub fn inc_x_reg(&mut self) -> &mut Self {
- self.push(&[0x3D])
- }
-
/// `INC Y` — `$FC`.
pub fn inc_y(&mut self) -> &mut Self {
self.push(&[0xFC])
}Then in tests/roms/AccuracySNES/gen/src/tests/apu_sweep.rs, replace each inc_x_reg() call with inc_x().
🤖 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/spc.rs` around lines 299 - 302, Remove the
duplicate public inc_x_reg method from the opcode builder, keeping the existing
inc_x implementation as the sole emitter for opcode $3D. Update all inc_x_reg
call sites in apu_sweep.rs to invoke inc_x instead.
| a.finish( | ||
| "E2.10", | ||
| 'E', | ||
| "256-opcode cycle sweep", | ||
| Provenance::Documented("fullsnes, SNES APU SPC700 CPU instruction set"), | ||
| Kind::Scored, | ||
| None, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the dossier entries, the plan counts, and the rebuilt artifacts.
set -euo pipefail
echo "--- MAP entries for E2.10, E2.10i and E1.14 ---"
fd -t f 'dossier.rs' tests/roms/AccuracySNES -x rg -n -C 4 'E2\.10|E1\.14' {}
echo "--- UNENUMERATED, which the E2.10i doc says names it ---"
fd -t f 'dossier.rs' tests/roms/AccuracySNES -x rg -n -C 6 'UNENUMERATED' {}
echo "--- plan counts ---"
rg -n -C 3 'E2\.10' docs/accuracysnes-plan.md || echo " no E2.10 in the plan"
rg -nP '\b(23[0-9]|2[0-9]{2})\s+(tests|rows|assertions)' docs/accuracysnes-plan.md || true
echo "--- were the generated artifacts rebuilt in this PR? ---"
git diff --name-only origin/HEAD... 2>/dev/null | rg 'AccuracySNES' || git diff --name-only HEAD~1 | rg 'AccuracySNES' || true
echo "--- CHANGELOG [Unreleased] ---"
rg -n -A 30 '\[Unreleased\]' CHANGELOG.md | rg -n 'E2\.10|AccuracySNES|sweep' || echo " no matching entry"Repository: doublegate/RustySNES
Length of output: 7498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- dossier mappings and duplicate-claim rules ---"
sed -n '80,112p' tests/roms/AccuracySNES/gen/src/dossier.rs
sed -n '519,535p' tests/roms/AccuracySNES/gen/src/dossier.rs
sed -n '630,690p' tests/roms/AccuracySNES/gen/src/dossier.rs
echo "--- plan section and Unreleased entry ---"
sed -n '828,845p' docs/accuracysnes-plan.md
sed -n '1,28p' CHANGELOG.md
echo "--- registrations and relevant test definitions ---"
rg -n -C 8 '"E2\.10i"|"E2\.10"|"E1\.14"|NOP|XCN|MUL YA' \
tests/roms/AccuracySNES/gen/src/tests/apu.rs \
tests/roms/AccuracySNES/gen/src/tests/apu_sweep.rs
echo "--- AccuracySNES change summary ---"
git diff --stat HEAD~1 -- tests/roms/AccuracySNES docs/accuracysnes-plan.md CHANGELOG.md
git diff --name-status HEAD~1 -- tests/roms/AccuracySNES docs/accuracysnes-plan.md CHANGELOG.md
echo "--- generated artifact presence ---"
for f in \
tests/roms/AccuracySNES/asm/tests_group_a.s \
tests/roms/AccuracySNES/build/accuracysnes.sfc \
tests/roms/AccuracySNES/build/accuracysnes-pal.sfc \
tests/roms/AccuracySNES/asm/scenes.s \
tests/roms/AccuracySNES/build; do
if [ -e "$f" ]; then printf 'present: %s\n' "$f"; else printf 'absent: %s\n' "$f"; fi
doneRepository: doublegate/RustySNES
Length of output: 39794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("docs/accuracysnes-plan.md")
for i, line in enumerate(p.read_text().splitlines(), 1):
if "E2.10" in line or "443" in line or "360" in line:
print(f"{i}:{line}")
PY
echo "--- current diff status for required files ---"
for f in \
tests/roms/AccuracySNES/gen/src/dossier.rs \
tests/roms/AccuracySNES/asm/tests_group_a.s \
tests/roms/AccuracySNES/asm/scenes.s \
tests/roms/AccuracySNES/build/accuracysnes.sfc \
tests/roms/AccuracySNES/build/accuracysnes-pal.sfc \
docs/accuracysnes-plan.md; do
if git diff --quiet HEAD~1 -- "$f"; then
printf 'unchanged: %s\n' "$f"
else
printf 'changed: %s\n' "$f"
fi
done
echo "--- exact MAP/UNENUMERATED entries ---"
python3 - <<'PY'
from pathlib import Path
s = Path("tests/roms/AccuracySNES/gen/src/dossier.rs").read_text()
for needle in ['("E2.10i"', '("E2.10"', '("E1.14"', '"E2.10i",']:
print(needle, s.count(needle))
PYRepository: doublegate/RustySNES
Length of output: 1496
Update the AccuracySNES coverage count.
docs/accuracysnes-plan.md still reports 359 of 443, but the new mapped assertion E2.10 raises coverage to 360 of 443. E2.10i is correctly mapped to &[] and justified in UNENUMERATED; it does not add another dossier assertion.
🤖 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_sweep.rs` around lines 548 - 556,
Update the AccuracySNES coverage count in docs/accuracysnes-plan.md from 359 of
443 to 360 of 443 to account for the mapped E2.10 assertion in the test flow,
while leaving E2.10i and its UNENUMERATED justification unchanged.
Sources: Coding guidelines, Path instructions
| /// Iterations. `REPEATS * ITERATIONS / 8` is the ticks-per-cycle-of-difference resolution. | ||
| const ITERATIONS: u8 = 32; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Three surviving instances of the halved tick rate in e2_10_instrument's documentation.
Lines 9123-9127 record that the first draft derived 8 opcode cycles per T2OUT tick and that the real figure is 16. The correction reached the main narrative and the validator table. It did not reach three dependent statements, each of which still carries arithmetic built on 8.
tests/roms/AccuracySNES/gen/src/tests/apu.rs#L9165-L9166: change the divisor inREPEATS * ITERATIONS / 8to 16, so the constant's doc gives 16 ticks per cycle of difference and agrees with line 9135.tests/roms/AccuracySNES/gen/src/tests/apu.rs#L9140-L9140: change "16 ticks = 128 cycles" to "16 ticks = 256 cycles".T2OUTis four bits and line 9120 of this same file states 16 opcode cycles per tick, so the ceiling is 256 cycles.apu_sweep.rsline 78-79 already uses 256 for the same ceiling.tests/roms/AccuracySNES/gen/src/tests/apu.rs#L9225-L9229: change the expected differences from "96 and 224" to "48 and 112", which line 9126 identifies as the measured values and lines 9200 and 9205 report. Also drop the opening claim that the sum "cannot overflow", which the rest of the same sentence contradicts by giving the total as about 350.
📍 Affects 1 file
tests/roms/AccuracySNES/gen/src/tests/apu.rs#L9165-L9166(this comment)tests/roms/AccuracySNES/gen/src/tests/apu.rs#L9140-L9140tests/roms/AccuracySNES/gen/src/tests/apu.rs#L9225-L9229
🤖 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 9165 - 9166,
Correct the remaining halved tick-rate documentation in e2_10_instrument: at
tests/roms/AccuracySNES/gen/src/tests/apu.rs lines 9165-9166, update the REPEATS
* ITERATIONS divisor from 8 to 16; at lines 9140-9140, change “16 ticks = 128
cycles” to “16 ticks = 256 cycles”; and at lines 9225-9229, change expected
differences “96 and 224” to “48 and 112” and remove the contradictory claim that
their sum cannot overflow.
| v.extend(bus::all()); | ||
| v.extend(dma::all()); | ||
| v.extend(apu::all()); | ||
| v.push(apu_sweep::e2_10()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
e2_10() runs after every APU row, including the one documented as needing to be last.
apu::all() ends with e9_02(), and the comment above it at line 159-161 of apu.rs states that nothing may be appended after it. Line 31 pushes e2_10() onto the vector after apu::all() has been extended, so e2_10 does run after e9_02.
The stated reason for e9_02 being last is the noise LFSR, which e2_10 does not read, so the ordering is harmless in fact. It is not harmless in intent: the constraint apu.rs documents is now violated from a different file, and the next row appended here inherits the violation without seeing the comment that forbids it.
The placement is also load-bearing in the other direction. e2_10 drives timer 2 for roughly 42 frames, and apu.rs lines 154-157 record that exactly this perturbed E3.06's timer-ratio reading. E3.06 runs inside apu::all(), so it is safe here — by module ordering, not by any stated rule.
Record the constraint at this call site.
Proposed fix
v.extend(apu::all());
+ // After every APU row on purpose. E2.10 drives timer 2 for some 42 frames, which moves
+ // E3.06's timer-ratio reading if it runs first — the same hazard E2.10i's placement inside
+ // `apu::all()` records. It runs after `e9_02` as well, which `apu.rs` documents as needing to
+ // be last; that is safe only because this row never reads the noise LFSR.
v.push(apu_sweep::e2_10());📝 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.
| v.push(apu_sweep::e2_10()); | |
| v.extend(apu::all()); | |
| // After every APU row on purpose. E2.10 drives timer 2 for some 42 frames, which moves | |
| // E3.06's timer-ratio reading if it runs first — the same hazard E2.10i's placement inside | |
| // `apu::all()` records. It runs after `e9_02` as well, which `apu.rs` documents as needing to | |
| // be last; that is safe only because this row never reads the noise LFSR. | |
| v.push(apu_sweep::e2_10()); |
🤖 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/mod.rs` at line 31, Add a concise
comment immediately before the apu_sweep::e2_10() push documenting that it must
remain after apu::all() because it perturbs timer 2 for E3.06, while apu::all()
must remain last within its own APU sequence due to the noise-LFSR constraint.
Keep the existing ordering unchanged.
All eight were real; the ROM image is byte-identical, so the battery and cross-validation results are unchanged. - inc_x_reg duplicated the existing inc_x. Two emitters for one opcode is the exact failure spc.rs's module doc guards against; removed and the callers moved to inc_x. - The operand doc table named $0C00 for the absolute operand and the bit address. WINDOW moved to $0E00 when the results/block overlap was fixed, and $0C00 is now the RESULTS page -- so the table pointed at the one address the rules exist to keep operands away from. It now links ram::WINDOW rather than restating a literal that can drift again. - Nothing pinned the MUL YA product below 256. The driver reads only A, the low byte; correct today only because DIV YA,X at 12 cycles scales to 60, and TICKS_PER_CYCLE is derived from two constants a change to the block shape would move. Crossing it would report an arithmetic artefact as a timing fault, which looks exactly like a real one. Now a unit test. - E2.10i's ITERATIONS doc said `REPEATS * ITERATIONS / 8` where T2OUT steps every 16 opcode cycles -- the same halving error the row's own doc comment already records having made once. - The Fwd must_use message said an unpatched branch "branches to itself". The placeholder displacement is zero, so it falls THROUGH -- the more dangerous and more silent of the two outcomes, which is the reason the message exists. - jmp_fwd's doc described a `base` parameter it does not take; that explanation belongs on patch_jmp_fwd, which does. Both it and jmp_back now carry the `# Panics` section their expect() calls imply. - The SINGLES tuple doc omitted the operands field it gained. A reader checking entries against fullsnes counts fields against that line. - The Group E slot list named v1.29.0 in a doc comment; it is version-agnostic now, having been wrong the first time the list grew. Also: docs/accuracysnes-plan.md still read 359/302. It is hand-maintained where the coverage report is generated, which is exactly the drift CLAUDE.md warns about; now 360/303. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doublegate
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/roms/AccuracySNES/gen/src/spc.rs (1)
260-287: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject an absolute jump target that exceeds APU RAM.
When
base + offsetexceedsu16::MAX,patch_jmp_fwdandjmp_backdo not reject the target. For example,base = 0x0200andoffset = 0xFE00overflow. Debug builds panic. Builds without overflow checks can encode a wrapped jump target.Use
checked_addafter converting the offset. Document that the complete absolute target must fit inu16. Apply the same invariant todata_first.Proposed fix
- let target = base + u16::try_from(self.bytes.len()).expect("a program is smaller than RAM"); + let offset = u16::try_from(self.bytes.len()).expect("a program is smaller than RAM"); + let target = base + .checked_add(offset) + .expect("APU program target exceeds address space"); - let addr = base + u16::try_from(target).expect("a program is smaller than RAM"); + let offset = u16::try_from(target).expect("a program is smaller than RAM"); + let addr = base + .checked_add(offset) + .expect("APU program target exceeds address space");#!/bin/bash set -euo pipefail sed -n '170,292p' tests/roms/AccuracySNES/gen/src/spc.rs rg -n -C 3 'overflow-checks|^\[profile\.' --glob 'Cargo.toml' .As per coding guidelines, public Rust items must document every panic condition.
🤖 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/spc.rs` around lines 260 - 287, Update Spc::patch_jmp_fwd, Spc::jmp_back, and data_first to convert the offset and use checked_add with the supplied base, rejecting any absolute target that exceeds u16::MAX instead of wrapping. Preserve the existing panic behavior and update each public item's documentation to state that the complete absolute target must fit in u16, including every resulting panic condition.Source: Coding guidelines
🤖 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/spc.rs`:
- Around line 260-287: Update Spc::patch_jmp_fwd, Spc::jmp_back, and data_first
to convert the offset and use checked_add with the supplied base, rejecting any
absolute target that exceeds u16::MAX instead of wrapping. Preserve the existing
panic behavior and update each public item's documentation to state that the
complete absolute target must fit in u16, including every resulting panic
condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b61233ba-05b3-4bf6-a11d-27c2d9ce1c9b
📒 Files selected for processing (6)
crates/rustysnes-test-harness/tests/accuracysnes.rsdocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: build demo + docs
- GitHub Check: review
🧰 Additional context used
📓 Path-based instructions (15)
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/accuracysnes-plan.md
**/*.{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/accuracysnes-plan.mdcrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.rstests/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/accuracysnes-plan.mdcrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.rstests/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/accuracysnes-plan.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/accuracysnes-plan.md
**/*.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/accuracysnes-plan.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-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/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.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/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.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/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.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/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.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/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
tests/roms/AccuracySNES/gen/src/spc.rs
📄 CodeRabbit inference engine (AGENTS.md)
Verify every new SPC700 opcode encoding against
crates/rustysnes-apu/src/spc700_exec.rs; emit only encodings exercised by committed tests.
Files:
tests/roms/AccuracySNES/gen/src/spc.rs
🧠 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/spc_opcodes.rstests/roms/AccuracySNES/gen/src/tests/apu_sweep.rstests/roms/AccuracySNES/gen/src/spc.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_sweep.rstests/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_sweep.rstests/roms/AccuracySNES/gen/src/tests/apu.rs
🔇 Additional comments (7)
tests/roms/AccuracySNES/gen/src/spc_opcodes.rs (1)
185-188: LGTM!tests/roms/AccuracySNES/gen/src/spc.rs (1)
39-40: LGTM!tests/roms/AccuracySNES/gen/src/tests/apu.rs (1)
9165-9167: LGTM!tests/roms/AccuracySNES/gen/src/tests/apu_sweep.rs (1)
207-208: LGTM!Also applies to: 362-362, 386-389, 402-402, 631-654
crates/rustysnes-test-harness/tests/accuracysnes.rs (2)
659-659: LGTM!
598-601: 🗄️ Data Integrity & IntegrationNo change needed. E2.10i writes slots 277–279, E2.10 writes slots 280–282, and the dynamic writer uses only slots 197–200.
> Likely an incorrect or invalid review comment.docs/accuracysnes-plan.md (1)
17-17: LGTM!
…mod.rs Two ordering constraints apply to this row and neither is visible from mod.rs, which is where the registration first went: - it must not run BEFORE E3.06. It drives timer 2 for some forty frames, harder than anything else on the cart, and apu.rs already records that the far smaller E2.10i instrument moved E3.06's reading when it was registered earlier. - it must not run AFTER e9_02, which apu.rs marks as necessarily last: every other program leaves FLG's noise rate at zero, e9_02 steps the LFSR, and nothing can put it back. Appending after apu::all() satisfied neither by intent -- only by luck, since e2_10 happens not to read the noise LFSR. It now sits next to e2_10_instrument, inside the window both constraints define, with the reasoning at the call site where the next row appended will see it. Battery 310/310 and three references still agree after the reorder; the test order changes the image, so this was re-cross-validated rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)Adds the E2.10 on-cart 256-opcode SPC700 cycle sweep test to AccuracySNES, along with its opcode metadata rules generator, SPC700 assembly driver, and updated test tracking docs. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/roms/AccuracySNES/gen/src/tests/apu.rs (1)
9230-9264: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject 8-bit timer aliasing.
e2_10_measureaccumulates into one byte, andE2.10irecords only the low byte. Although the documented totals 94, 142, and 206 fit, an implementation that adds 256 ticks to one arm produces the same record. Accumulate or publish the high byte, or assert that it remains zero before recording.🤖 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 9230 - 9264, Update e2_10_measure so E2.10i cannot accept measurements aliased by 8-bit overflow: either accumulate and publish the high byte alongside the low byte, or validate that the high byte remains zero before writing the result to port. Preserve the existing per-iteration T2OUT accumulation and loop behavior while ensuring documented totals are recorded unambiguously.
🤖 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.
Outside diff comments:
In `@tests/roms/AccuracySNES/gen/src/tests/apu.rs`:
- Around line 9230-9264: Update e2_10_measure so E2.10i cannot accept
measurements aliased by 8-bit overflow: either accumulate and publish the high
byte alongside the low byte, or validate that the high byte remains zero before
writing the result to port. Preserve the existing per-iteration T2OUT
accumulation and loop behavior while ensuring documented totals are recorded
unambiguously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e1ab939-9428-42ed-92e4-7eb06e2087e1
⛔ Files ignored due to path filters (5)
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 (2)
tests/roms/AccuracySNES/gen/src/tests/apu.rstests/roms/AccuracySNES/gen/src/tests/mod.rs
💤 Files with no reviewable changes (1)
- tests/roms/AccuracySNES/gen/src/tests/mod.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: lint
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: build demo + docs
- GitHub Check: review
🧰 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 (4)
tests/roms/AccuracySNES/gen/src/tests/apu.rs (4)
228-283: 🗄️ Data Integrity & IntegrationThe timeout path already bypasses result copying.
upload_and_run_longcopiesAPUIO1–APUIO3only at@ran. A timeout jumps to@timeout, which recordsSKIPand exits before therecordcalls.> Likely an incorrect or invalid review comment.
182-182: 📐 Maintainability & Code QualityNo rustdoc change is required. The cited public functions already have adjacent
///documentation.upload_onlyis private.> Likely an incorrect or invalid review comment.
9115-9228: 🗄️ Data Integrity & IntegrationCheck all computed result slots for collisions with
E2.10islots 277–279
159-164: 🗄️ Data Integrity & IntegrationNo registration update is required.
E2.10was already present indossier.rs, the plan, and the generated artifacts before this change. This commit only changes test ordering and regenerates affected outputs.> Likely an incorrect or invalid review comment.
Coverage 359 → 360 of 443.
What the row claims
For every opcode the SPC700 can execute in a straight line, the cart measures how long it actually takes and compares that on-cart against the cycle count fullsnes documents for it. The host supplies no expected values — it reads back three bytes: opcodes measured, opcodes disagreeing, and the first one that did.
How one opcode is timed
T2OUTsteps once every 16 opcode cycles, far too coarse for a single instruction, so the sweep never times one. It times a block of six copies, sixteen times over, against the same block built fromNOP:Everything around the copies cancels in the difference, leaving 6 ticks per cycle against a quantisation of ±1 on each side. The band is ±3 — half a cycle.
Branches are measured taken
A relative branch with a displacement of zero lands on the following instruction, which is where a not-taken branch would have gone anyway. So a block of six runs straight through whichever way each one goes, and the arm can arrange the taken path — the more interesting cost. The prologue that arranges it is two two-cycle immediates whatever flag it needs, so choosing one costs nothing that could leak into a difference.
The table is checked, not trusted
fullsnes documents the opcode map as rules, and
spc_opcodes.rsfollows them. A slot filled twice or left empty is a build failure, not a shipped hole.The operand kind is recorded at construction rather than derived from the opcode byte afterwards. The first draft derived it from the low nibble — nearly right, and hiding at least four traps. One of them gave
MOV [aa+X],Aa pointer read from uninitialised memory, which is zero, which is the driver's own variables.What it does not cover
Twenty-five opcodes end the straight line rather than continuing it, each named with its reason: the absolute jumps and calls (one encoding cannot make six copies at six addresses each fall into the next), the vectored calls (
TCALL/PCALL/BRK— their vectors are in the IPL ROM every Group E program keeps mapped), the returns (they pop an address the block never pushed), andSLEEP/STOP, for which fullsnes itself gives the cycle count as?.The count of what was measured is asserted on-cart at 231, so a sweep covering a different set fails rather than quietly reporting no disagreements. That liveness assertion is deliberately checked before anything is concluded from the failure count — a driver that fell over after one opcode would otherwise report
0and read as a pass.Verified by injection, twice
XCN$9F— the opcode broken, and nothing else$10— the eight conditional branches plusBRAThe second is also the proof that the taken-path prologues really do take the branch in all eight conditions; if any arm had been measuring the not-taken path it would have been unaffected.
Two things that had to be measured rather than reasoned about
$0900and the uploaded image reached$0948, so the sweep overwrote its own driver as it recorded. A build-time assertion now rejects that layout.upload_and_run_longis the bounded-but-longer wait.Also lands
E2.10i, the smaller NOP/XCN/MUL YA instrument the sweep grew out of, on a different shape (eight copies, thirty-two iterations). It stays as a golden because its three raw tick totals cross-validate directly, where the sweep's answer is a pass/fail count.Verification
cargo run -p accuracysnes-gen— 345 tests; bank$065,416 bytes freecargo test --workspace— 68 suites, all okcargo fmt --all --check,cargo clippy --workspace --exclude rustysnes-android --all-targets -- -D warnings,RUSTDOCFLAGS="-D warnings" cargo doc— cleancrossval.sh— battery 310/310, 55 scenes match / 0 unblessed / 0 mismatched, 3 references agree. snes9x, Mesen2 and ares each report 231 measured and zero disagreements; known-failure counts unchanged at 15 / 1 / 3.Four independent SPC700 implementations agree with the documented cycle table for every straight-line opcode.
🤖 Generated with Claude Code
Adds AccuracySNES dossier assertion
E2.10for a full SPC700 cycle sweep. The sweep measures 231 straight-line opcodes and compares timer results with fullsnes cycle data.E2.10iremains unenumerated as a three-opcode golden test. Coverage increases to 360 of 443 assertions, with on-cart coverage increasing to 303; the denominator remains 443.The claim is false if any measured opcode has an incorrect cycle count, fewer than 231 opcodes are measured, or the opcode table contains a missing or duplicate entry. An injected timing fault must increase the disagreement count and identify the faulty opcode.
The change adds generator support for safe SPC700 instruction emission, branch patching, aligned measurement tables, long APU waits, and on-cart result reporting. Validation includes workspace checks and cross-emulator agreement across Snes9x, Mesen2, and ares.