feat(accuracysnes): pitch scales the sample rate, and $2000 is an octave up (E6.02) by doublegate · Pull Request #185 · doublegate/RustySNES

doublegate · 2026-07-20T23:06:40Z

The first assertion from E6, the S-DSP's pitch and interpolation block, which until now had none
of its eleven rows covered. It is also the first test on this cart that measures a rate, which
needs a different construction from everything before it.

Why three tests for one row

A reading of ENDX reports "finished" or "not finished". That bounds a rate on one side and no
more, so no single reading can establish one. Three readings of the same 384-sample voice bound it
on both sides and then show the doubling:

	pitch	waits	`ENDX`	what it establishes
`E6.02`	`$1000`	6	clear	fewer than 64 samples per wait
`E6.02b`	`$1000`	16	set	at least 24 — so it is playing, and not arbitrarily slowly
`E6.02c`	`$2000`	6	set	at least 64: strictly faster than `$1000`, same wait
`E6.02d`	`$2000`	3	clear	fewer than 128 — the upper bound that makes it a window

Each pitch is read twice, either side of where it finishes. E6.02b matters most for honesty on
the $1000 side — without it a core that plays nothing passes E6.02 by doing nothing — and
E6.02d does the same job on the $2000 side, where "at least 64 samples per wait" is otherwise
satisfied by a core running ten times too fast.

The result is two windows, not a factor: $1000 consumes 24-64 samples per wait, $2000
consumes 64-128. Both contain the documented rates (48 and 96), they do not overlap, and no core
ignoring the pitch register satisfies both. A core scaling by 1.5 rather than 2 does fit, and the
doc comment says so — pinning the factor needs each rate bracketed between adjacent waits, at
roughly a tenth of the elapsed time in margin, which is the timing-marginal construction this group
has been bitten by twice. docs/accuracysnes-plan.md records the trade and that the way in is a
longer sample rather than a narrower wait.

Declared in dossier.rs::SPLITS as one enumerated row across four tests, with that reasoning.

The margin was measured, not assumed

The first draft placed the wait by calculating from DBNZ's cycle count and 32 cycles per output
sample. That was wrong — the voice had already finished — so the finishing points were found by
bisection instead: the $1000 voice finishes between the seventh and eighth wait, the $2000
voice between the fourth and fifth. The ratio of about two is the octave showing up directly in the
bisection.

Six waits is the geometric mean of the two, so each verdict has roughly a third of the elapsed time
in hand on either side. That is the widest margin this construction admits: the two finishing
times differ by a factor of two, so no single reading can sit further than sqrt(2) from both.
Everything timed happens inside the uploaded SPC program, so neither the cart's code size nor the
host's speed can move it — the failure mode that has bitten APU tests here before.

Validation

cargo fmt --check clean, cargo clippy --workspace --all-targets 0 errors 0 warnings.
Harness 10/10, pass rate 100.00% (215 scoring, 11 golden).
Cross-validated: snes9x and Mesen2 agree with the cart on all four, on both the NTSC and PAL
images. No new declared divergences; 50/50 scenes unchanged. Three independent DSP models landing
on the same side of every one of those eight verdicts is what says the margin is real rather than a
property of this emulator's timing.

Coverage moves 232 → 233 of 443 — four tests, but one enumerated dossier row, since E6.02
is a declared split. (An earlier version of this description said 235, which was the test count
mistaken for the assertion count; docs/accuracysnes-coverage.md is generated and is the
authority.)

Adds AccuracySNES dossier assertion E6.02 (implemented via four cart tests E6.02 / E6.02b / E6.02c / E6.02d) to verify that S-DSP pitch scales sample consumption rate in non-overlapping windows: at $1000, the voice finishes within the 24–64 samples per wait bracket; at $2000 (an octave above), it finishes within the 64–128 samples per wait bracket, excluding cores that ignore the pitch register. The documentation notes the boundary wait points were found via bisection and that a single ENDX observation only bounds finish on one side, so each pitch is measured twice.

The claim would be false if ENDX/finish timing did not place $1000 and $2000 into these respective bracketing windows (including the expected ordering/faster-$2000 behaviour), or if snes9x and Mesen2 disagreed with these windows on either NTSC or PAL images.

Coverage increases to 233 of 443 enumerated assertion rows, with the denominator unchanged (443).

…ave up (E6.02) The first assertion from the S-DSP's pitch block, and the first on this cart to measure a rate. One reading of ENDX cannot: it reports finished or not finished, which bounds a rate on one side only. Three tests play the same 384-sample voice and bound it on both — at $1000 it has not finished after six waits and has after sixteen, and at $2000 six waits are enough. Both finishing points were measured by bisection rather than calculated: $1000 finishes between the seventh and eighth wait, $2000 between the fourth and fifth. Six waits is the geometric mean, so each verdict has about a third of the elapsed time in hand on either side, which is the widest margin this construction admits — the two finishing times differ by a factor of two, so no single reading can sit further than sqrt(2) from both. Cross-validated: snes9x and Mesen2 agree on all three, on both images. That is what says the margin is real rather than a property of one DSP model. 235 of 443 dossier assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

gemini-code-assist · 2026-07-20T23:06:44Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

coderabbitai · 2026-07-20T23:07:08Z

Warning

Review limit reached

@doublegate, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4b4fda97-a2d8-45dc-aacb-b13c127128ef

📥 Commits

Reviewing files that changed from the base of the PR and between ac8897d and 789dff2.

⛔ Files ignored due to path filters (2)

tests/roms/AccuracySNES/ERROR_CODES.md is excluded by !tests/roms/AccuracySNES/ERROR_CODES.md and included by tests/**
tests/roms/AccuracySNES/asm/tests_group_a.s is excluded by !tests/roms/AccuracySNES/asm/tests_group_a.s and included by tests/**

📒 Files selected for processing (2)

CHANGELOG.md
tests/roms/AccuracySNES/gen/src/tests/apu.rs

Walkthrough

Adds four AccuracySNES E6.02 tests that measure S-DSP pitch-driven sample consumption using ENDX timing, registers them as an intentional dossier split, and updates test-count documentation and the changelog.

Changes

S-DSP pitch measurements

Layer / File(s)	Summary
Pitch timing tests `tests/roms/AccuracySNES/gen/src/tests/apu.rs`	Adds a fixed-ended BRR sample, a shared playback and ENDX assertion harness, and four scored tests covering pitch settings `$1000` and `$2000` with different wait durations.
Dossier mapping and measurement documentation `tests/roms/AccuracySNES/gen/src/dossier.rs`, `docs/accuracysnes-plan.md`, `CHANGELOG.md`	Maps `E6.02`, `E6.02b`, `E6.02c`, and `E6.02d` to the E6.02 assertion, records their intentional split, updates test totals from 223 to 227, and documents the measurement method and emulator results.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AccuracySNES
  participant S-DSP
  participant CartRAM
  AccuracySNES->>S-DSP: play fixed-ended 384-sample BRR voice
  S-DSP->>CartRAM: set voice-0 ENDX on completion
  AccuracySNES->>CartRAM: read ENDX after settle delay
  CartRAM-->>AccuracySNES: return bracket assertion result

🚥 Pre-merge checks | ✅ 10

✅ Passed checks (10 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Changelog Entry	✅ Passed	PASS: The PR changes the AccuracySNES cartridge/tests and also updates CHANGELOG.md in the same diff.
Docs-As-Spec	✅ Passed	PASS: the diff touches only AccuracySNES docs and test ROM generators; no `crates/rustysnes-*` paths changed, so the docs-as-spec rule is not triggered.
Accuracysnes Bookkeeping	✅ Passed	E6.02d is mapped in dossier.rs, the regenerated ROM/catalog/coverage artefacts are committed, and the plan count rises by one to match coverage.
No Panic On Untrusted Input	✅ Passed	No new unwrap/expect/panic was added; the E6 helper builds its BRR sample locally and only passes constants into existing test machinery.
Safety Comment On New Unsafe	✅ Passed	No new `unsafe { ... }` blocks or `unsafe fn`s appear in the PR diff, so there is nothing that needs an adjacent `// SAFETY:` comment.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title uses a valid feat Conventional Commit form and accurately describes the E6.02 pitch-rate assertions.

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

Adds the first AccuracySNES coverage for the S-DSP pitch/sample-counter block (E6) by introducing a three-part, on-cart rate measurement that uses ENDX to bracket completion at pitch $1000 and confirm the expected octave-up behavior at $2000.

Changes:

Add three new APU/DSP tests (E6.02, E6.02b, E6.02c) sharing a common BRR sample + SPC program emitter to measure pitch-driven sample consumption rate via ENDX.
Declare the three tests as one enumerated dossier row (SPLITS/MAP) and regenerate the cart catalog + error-code documentation to include them.
Update AccuracySNES planning/coverage docs and the project changelog to reflect the new test coverage.

Reviewed changes

Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/roms/AccuracySNES/gen/src/tests/apu.rs	Introduces the shared pitch-rate test construction and the three new E6.02* tests.
tests/roms/AccuracySNES/gen/src/dossier.rs	Maps the new tests and declares them as a single enumerated row split across three tests.
tests/roms/AccuracySNES/asm/tests_group_a.s	Regenerated ROM-side assembly to include the new tests and uploaded SPC program blobs.
tests/roms/AccuracySNES/SOURCE_CATALOG.tsv	Regenerated catalog entries and addresses for the newly added tests.
tests/roms/AccuracySNES/ERROR_CODES.md	Regenerated error-code documentation for E6.02/E6.02b/E6.02c.
docs/accuracysnes-plan.md	Updates the headline test counts in the planning doc.
docs/accuracysnes-coverage.md	Updates coverage totals and the “splits across several tests” list to include E6.02.
CHANGELOG.md	Documents the addition of the E6.02 pitch/rate coverage.

coderabbitai

Actionable comments posted: 1

🤖 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/dossier.rs`:
- Around line 282-289: Update the E6.02 dossier description to accurately
describe the differing wait counts: E6.02 waits 6 times and E6.02b waits 16
times. Replace the “twice that wait” wording with language that reflects these
actual counts while preserving the surrounding explanation.

🪄 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: e9dc8741-1f7f-4b7e-96d3-dfd312f9063d

📥 Commits

Reviewing files that changed from the base of the PR and between c1a75e6 and 2df05c4.

⛔ Files ignored due to path filters (6)

docs/accuracysnes-coverage.md is excluded by !docs/accuracysnes-coverage.md and included by docs/**
tests/roms/AccuracySNES/ERROR_CODES.md is excluded by !tests/roms/AccuracySNES/ERROR_CODES.md and included by tests/**
tests/roms/AccuracySNES/SOURCE_CATALOG.tsv is excluded by !**/*.tsv, !tests/roms/AccuracySNES/SOURCE_CATALOG.tsv and included by tests/**
tests/roms/AccuracySNES/asm/tests_group_a.s is excluded by !tests/roms/AccuracySNES/asm/tests_group_a.s and included by tests/**
tests/roms/AccuracySNES/build/accuracysnes-pal.sfc is excluded by !tests/roms/AccuracySNES/build/** and included by tests/**
tests/roms/AccuracySNES/build/accuracysnes.sfc is excluded by !tests/roms/AccuracySNES/build/** and included by tests/**

📒 Files selected for processing (4)

CHANGELOG.md
docs/accuracysnes-plan.md
tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

📜 Review details

⏰ Context from checks skipped due to timeout. (5)

GitHub Check: accuracysnes
GitHub Check: lint
GitHub Check: test-light
GitHub Check: copilot-pull-request-reviewer
GitHub Check: build demo + docs

🧰 Additional context used

📓 Path-based instructions (12)

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.

Treat docs/STATUS.md as the authoritative per-subsystem state and update project documentation alongside relevant implementation changes.

New subsystems must add documentation under docs/.

Files:

docs/accuracysnes-plan.md

**/*.{rs,md,toml}

📄 CodeRabbit inference engine (AGENTS.md)

Never commit commercial ROMs; only commit derived screenshots or hashes.

Files:

docs/accuracysnes-plan.md
CHANGELOG.md
tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

**/*

📄 CodeRabbit inference engine (AGENTS.md)

Preserve the architecture contracts: the Bus owns mutable state, timing is master-clock-driven, crate dependencies are one-directional, and determinism is mandatory.

**/*: Do not commit or vendor the generated snesdev_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 relevant to-dos/ sprint file.

Files:

docs/accuracysnes-plan.md
CHANGELOG.md
tests/roms/AccuracySNES/gen/src/dossier.rs
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>.md documentation.

Files:

docs/accuracysnes-plan.md
CHANGELOG.md
tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

docs/**

⚙️ CodeRabbit configuration file

docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generated docs/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
CHANGELOG.md

CHANGELOG.md

📄 CodeRabbit inference engine (CONTRIBUTING.md)

User-visible changes must be recorded under the [Unreleased] section.

Files:

CHANGELOG.md

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Use Rust edition 2024 with the pinned toolchain (1.96); satisfy workspace pedantic, nursery, missing_docs, and unsafe_code warning levels because CI uses -D warnings. Document every public item.
Keep hot paths allocation-free.
Preserve determinism: identical seed, ROM, and input must produce bit-identical audio and video; additive features must remain default-off so shipped, native, no_std, and WASM builds remain byte-identical.
When implementing hardware behavior, pin the failing test ROM first and treat committed test ROMs as the specification.
When changing a chip, update the corresponding docs/<chip>.md in the same pull request.

**/*.rs: Use Rust edition 2024 and the toolchain pinned in rust-toolchain.toml (Rust 1.96).
Run cargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy with cargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc because missing_docs is a workspace lint.
Do not run cargo clippy --all-features; scripting and script-wasm are mutually exclusive. Use explicit per-feature jobs instead.

**/*.rs: Do not introduce .unwrap(), .expect(), or panic!() on untrusted external input such as ROM or save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths, except in #[cfg(test)] code. Use typed errors at those boundaries; locally constructed values or immediately checked invariants are acceptable and must be identified.
Every new unsafe { ... } block or unsafe fn must have an adjacent // SAFETY: comment stating the relied-upon invariant and its guarantor. Unsafe code outside the frontend and FFI shims requires additional justification.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

tests/roms/AccuracySNES/gen/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rebuild AccuracySNES after changes to gen/ or asm/; run the generator directly without piping through tail -1, so generator panics remain visible.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

**/*.{rs,s}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{rs,s}: Do not rely on identical failures across three emulators as proof that a test is correct; investigate possible shared harness defects before treating the result as an emulator bug.
Do not use unsupported assembly constructs: STZ has no long-addressing form, cop #$00 must be emitted as .byte $02,$00, and menu labels must not exceed 24 columns.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

tests/roms/AccuracySNES/**

📄 CodeRabbit inference engine (Custom checks)

When adding or removing an AccuracySNES test or scene under tests/roms/AccuracySNES/gen/src/, update dossier.rs::MAP, regenerate all required assembly, catalog, coverage, ROM, and scene artefacts, and adjust the hand-maintained plan counts by the same amount as the generated coverage table.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
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 behaviour 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 behaviours 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::MAP must 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.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

🔇 Additional comments (5)

tests/roms/AccuracySNES/gen/src/tests/apu.rs (2)

98-100: LGTM!

3381-3510: Solid three-test bracket; state reset and vacuity concerns are properly addressed.

voice_program explicitly rewrites every DSP register it touches (including ENDX itself before KON) before each test runs, and e6_02/e6_02b/e6_02c together correctly bracket the "never finishes" and "1:1 vs octave" failure modes rather than any one test standing alone. No correctness issues found in this segment.

tests/roms/AccuracySNES/gen/src/dossier.rs (1)

167-169: LGTM!

docs/accuracysnes-plan.md (1)

16-16: LGTM!

CHANGELOG.md (1)

14-26: LGTM!

The SPLITS entry said the second test waits "twice" the first. It waits six and sixteen, which is not twice — an overstated doc comment of exactly the kind the review instructions ask for, even though nothing about the test changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

🤖 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/dossier.rs`:
- Around line 284-289: Revise the documentation string in the dossier
description to remove the claim that the E6.02c result proves an exact octave
rate. Describe $2000 as the expected doubled-rate outcome, or add a paired
boundary assertion that distinguishes 2× from intermediate rates; ensure the
text only claims behavior exercised by the emitted test.

🪄 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: da8f5bfb-67b8-4a37-813b-b53552648343

📥 Commits

Reviewing files that changed from the base of the PR and between 2df05c4 and 8e3e43d.

⛔ Files ignored due to path filters (1)

docs/accuracysnes-coverage.md is excluded by !docs/accuracysnes-coverage.md and included by docs/**

📒 Files selected for processing (1)

tests/roms/AccuracySNES/gen/src/dossier.rs

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

GitHub Check: test-light
GitHub Check: lint
GitHub Check: accuracysnes
GitHub Check: build demo + docs

🧰 Additional context used

📓 Path-based instructions (8)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Use Rust edition 2024 with the pinned toolchain (1.96); satisfy workspace pedantic, nursery, missing_docs, and unsafe_code warning levels because CI uses -D warnings. Document every public item.
Keep hot paths allocation-free.
Preserve determinism: identical seed, ROM, and input must produce bit-identical audio and video; additive features must remain default-off so shipped, native, no_std, and WASM builds remain byte-identical.
When implementing hardware behavior, pin the failing test ROM first and treat committed test ROMs as the specification.
When changing a chip, update the corresponding docs/<chip>.md in the same pull request.

**/*.rs: Use Rust edition 2024 and the toolchain pinned in rust-toolchain.toml (Rust 1.96).
Run cargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy with cargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc because missing_docs is a workspace lint.
Do not run cargo clippy --all-features; scripting and script-wasm are mutually exclusive. Use explicit per-feature jobs instead.

**/*.rs: Do not introduce .unwrap(), .expect(), or panic!() on untrusted external input such as ROM or save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths, except in #[cfg(test)] code. Use typed errors at those boundaries; locally constructed values or immediately checked invariants are acceptable and must be identified.
Every new unsafe { ... } block or unsafe fn must have an adjacent // SAFETY: comment stating the relied-upon invariant and its guarantor. Unsafe code outside the frontend and FFI shims requires additional justification.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs

tests/roms/AccuracySNES/gen/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rebuild AccuracySNES after changes to gen/ or asm/; run the generator directly without piping through tail -1, so generator panics remain visible.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs

**/*.{rs,s}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{rs,s}: Do not rely on identical failures across three emulators as proof that a test is correct; investigate possible shared harness defects before treating the result as an emulator bug.
Do not use unsupported assembly constructs: STZ has no long-addressing form, cop #$00 must be emitted as .byte $02,$00, and menu labels must not exceed 24 columns.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs

**/*.{rs,md,toml}

📄 CodeRabbit inference engine (AGENTS.md)

Never commit commercial ROMs; only commit derived screenshots or hashes.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs

**/*

📄 CodeRabbit inference engine (AGENTS.md)

Preserve the architecture contracts: the Bus owns mutable state, timing is master-clock-driven, crate dependencies are one-directional, and determinism is mandatory.

**/*: Do not commit or vendor the generated snesdev_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 relevant to-dos/ sprint file.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs

**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Chip-behavior changes must update both the chip implementation and the corresponding docs/<subsystem>.md documentation.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs

tests/roms/AccuracySNES/**

📄 CodeRabbit inference engine (Custom checks)

When adding or removing an AccuracySNES test or scene under tests/roms/AccuracySNES/gen/src/, update dossier.rs::MAP, regenerate all required assembly, catalog, coverage, ROM, and scene artefacts, and adjust the hand-maintained plan counts by the same amount as the generated coverage table.

Files:

tests/roms/AccuracySNES/gen/src/dossier.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 behaviour 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 behaviours 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::MAP must 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.rs

🔇 Additional comments (1)

tests/roms/AccuracySNES/gen/src/dossier.rs (1)

167-169: LGTM!

E6.02c established that $2000 consumes at least 64 samples per wait. A core running it ten times too fast satisfies that as well as one running it right, so that was half a measurement. Three waits are not enough for a 384-sample voice to finish at anything up to 128 samples per wait, which closes the window. The result is two windows rather than a factor: $1000 at 24-64 samples per wait and $2000 at 64-128. Both contain the documented rates, they do not overlap, and no core ignoring the pitch register satisfies both. The docs no longer say the pair proves an octave — a core scaling by 1.5 fits both windows, and excluding it means bracketing between adjacent waits at a tenth of the margin, which is the timing-marginal construction this group has been bitten by twice. The trade is written up in docs/accuracysnes-plan.md. Cross-validated: snes9x and Mesen2 agree on all four, on both images. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 25-26: Update the changelog sentence describing the waits to
remove the geometric-mean placement and one-third elapsed-time claims. Describe
the waits as bisection-derived, deliberately non-adjacent bounds, preserving the
stated $1000 and $2000 wait counts.

In `@tests/roms/AccuracySNES/gen/src/tests/apu.rs`:
- Around line 3539-3540: Update the exact-boundary diagnostic for the 384-sample
voice pitch-$2000 test to say “at least 128” instead of “more than 128”, and
update the corresponding wording in ERROR_CODES.md to keep the documented error
message aligned.

🪄 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: 51e3ea8e-4db7-4902-a489-0c78399f587c

📥 Commits

Reviewing files that changed from the base of the PR and between 8e3e43d and ac8897d.

⛔ Files ignored due to path filters (6)

docs/accuracysnes-coverage.md is excluded by !docs/accuracysnes-coverage.md and included by docs/**
tests/roms/AccuracySNES/ERROR_CODES.md is excluded by !tests/roms/AccuracySNES/ERROR_CODES.md and included by tests/**
tests/roms/AccuracySNES/SOURCE_CATALOG.tsv is excluded by !**/*.tsv, !tests/roms/AccuracySNES/SOURCE_CATALOG.tsv and included by tests/**
tests/roms/AccuracySNES/asm/tests_group_a.s is excluded by !tests/roms/AccuracySNES/asm/tests_group_a.s and included by tests/**
tests/roms/AccuracySNES/build/accuracysnes-pal.sfc is excluded by !tests/roms/AccuracySNES/build/** and included by tests/**
tests/roms/AccuracySNES/build/accuracysnes.sfc is excluded by !tests/roms/AccuracySNES/build/** and included by tests/**

📒 Files selected for processing (4)

CHANGELOG.md
docs/accuracysnes-plan.md
tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

GitHub Check: lint
GitHub Check: test-light
GitHub Check: accuracysnes
GitHub Check: build demo + docs

🧰 Additional context used

📓 Path-based instructions (12)

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.

Treat docs/STATUS.md as the authoritative per-subsystem state and update project documentation alongside relevant implementation changes.

New subsystems must add documentation under docs/.

Files:

docs/accuracysnes-plan.md

**/*.{rs,md,toml}

📄 CodeRabbit inference engine (AGENTS.md)

Never commit commercial ROMs; only commit derived screenshots or hashes.

Files:

docs/accuracysnes-plan.md
CHANGELOG.md
tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

**/*

📄 CodeRabbit inference engine (AGENTS.md)

Preserve the architecture contracts: the Bus owns mutable state, timing is master-clock-driven, crate dependencies are one-directional, and determinism is mandatory.

**/*: Do not commit or vendor the generated snesdev_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 relevant to-dos/ sprint file.

Files:

docs/accuracysnes-plan.md
CHANGELOG.md
tests/roms/AccuracySNES/gen/src/dossier.rs
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>.md documentation.

Files:

docs/accuracysnes-plan.md
CHANGELOG.md
tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

docs/**

⚙️ CodeRabbit configuration file

docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generated docs/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
CHANGELOG.md

CHANGELOG.md

📄 CodeRabbit inference engine (CONTRIBUTING.md)

User-visible changes must be recorded under the [Unreleased] section.

Files:

CHANGELOG.md

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Use Rust edition 2024 with the pinned toolchain (1.96); satisfy workspace pedantic, nursery, missing_docs, and unsafe_code warning levels because CI uses -D warnings. Document every public item.
Keep hot paths allocation-free.
Preserve determinism: identical seed, ROM, and input must produce bit-identical audio and video; additive features must remain default-off so shipped, native, no_std, and WASM builds remain byte-identical.
When implementing hardware behavior, pin the failing test ROM first and treat committed test ROMs as the specification.
When changing a chip, update the corresponding docs/<chip>.md in the same pull request.

**/*.rs: Use Rust edition 2024 and the toolchain pinned in rust-toolchain.toml (Rust 1.96).
Run cargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy with cargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc because missing_docs is a workspace lint.
Do not run cargo clippy --all-features; scripting and script-wasm are mutually exclusive. Use explicit per-feature jobs instead.

**/*.rs: Do not introduce .unwrap(), .expect(), or panic!() on untrusted external input such as ROM or save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths, except in #[cfg(test)] code. Use typed errors at those boundaries; locally constructed values or immediately checked invariants are acceptable and must be identified.
Every new unsafe { ... } block or unsafe fn must have an adjacent // SAFETY: comment stating the relied-upon invariant and its guarantor. Unsafe code outside the frontend and FFI shims requires additional justification.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

tests/roms/AccuracySNES/gen/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rebuild AccuracySNES after changes to gen/ or asm/; run the generator directly without piping through tail -1, so generator panics remain visible.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

**/*.{rs,s}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{rs,s}: Do not rely on identical failures across three emulators as proof that a test is correct; investigate possible shared harness defects before treating the result as an emulator bug.
Do not use unsupported assembly constructs: STZ has no long-addressing form, cop #$00 must be emitted as .byte $02,$00, and menu labels must not exceed 24 columns.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

tests/roms/AccuracySNES/**

📄 CodeRabbit inference engine (Custom checks)

When adding or removing an AccuracySNES test or scene under tests/roms/AccuracySNES/gen/src/, update dossier.rs::MAP, regenerate all required assembly, catalog, coverage, ROM, and scene artefacts, and adjust the hand-maintained plan counts by the same amount as the generated coverage table.

Files:

tests/roms/AccuracySNES/gen/src/dossier.rs
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 behaviour 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 behaviours 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::MAP must 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.rs
tests/roms/AccuracySNES/gen/src/tests/apu.rs

🔇 Additional comments (5)

tests/roms/AccuracySNES/gen/src/tests/apu.rs (2)

101-101: LGTM!

3445-3463: 🎯 Functional Correctness

No issue: pitch_ramp_sample() is 24 BRR blocks long with END on the final block, and pitch_rate_test() masks voice 0’s ENDX bit before comparing it to want_end.

tests/roms/AccuracySNES/gen/src/dossier.rs (1)

170-170: LGTM!

Also applies to: 283-291

docs/accuracysnes-plan.md (1)

16-16: LGTM!

Also applies to: 334-353

CHANGELOG.md (1)

20-23: LGTM!

Two wording defects, both about the arithmetic of a threshold. A core consuming exactly 128 samples per wait finishes a 384-sample voice on the third wait, so E6.02d's failure means 'at least 128', not 'more than'. The other three messages now name the bound each verdict establishes rather than paraphrasing it as a comparison with 1:1. And the waits are no longer geometric-mean placements — E6.02d made the set 6/16 and 3/6 — so the docs say what is actually true: bisection found the boundaries, and each wait was then put a wait or more clear of the nearest one. The windows are wider than the bisection knows them to be, deliberately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ent counts Two readers in a fortnight have read the coverage figure off the test count — me, on #185, and the review bot on #189. Section 2 already said so; the table at the top, which is where the numbers actually are, did not. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…189) * feat(accuracysnes): emulation mode uses its own vector table (A6.02) COP goes through $FFF4, sixteen bytes from the native $FFE4, and a core that keeps one set of vectors — or picks the table from something other than the E flag — lands in the wrong handler. Nothing about that is visible in ordinary code, since a game's COP handler is usually the same routine either way. The cart could not see it either: both vectors pointed at the same trampoline, so a core taking the native table in emulation mode ran the same handler and passed. The emulation COP vector now has its own trampoline and its own RAM pointer. Both handlers are installed and live, so taking the wrong table is a wrong answer rather than a hang and the failure names the vector used. BRK deliberately does not get the same treatment. In emulation, $FFFE is shared between IRQ and BRK — the hardware conflates them — so a pointer behind it could not mean 'the BRK handler' unambiguously, and splitting it would invent a distinction the machine does not have. Splitting it also broke A6.06 and A6.09, which install through V_BRK_VEC and run in emulation, which is how the argument got made concrete. Cross-validated: snes9x and Mesen2 agree, on both images. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(accuracysnes): V_COP_VEC_E overlapped the APU pointer's bank byte V_APU_PTR at $0050 is a 24-bit pointer used with lda [V_APU_PTR],y, so it owns $0050-$0052 — and the new emulation COP vector was put at $0052, on top of its bank byte. Whichever ran second corrupted the other. Moved to $0054, with the reason recorded next to it so the next free-slot search does not repeat it. Also: the native handler's comment claimed it could not return from an emulation-mode interrupt. It is entered in emulation mode too — the mode is decided by the COP, not by which vector was used — so its RTI returns just as cleanly, and the marker it leaves is the whole of its job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(accuracysnes): say outright that tests and assertions are different counts Two readers in a fortnight have read the coverage figure off the test count — me, on #185, and the review bot on #189. Section 2 already said so; the table at the top, which is where the numbers actually are, did not. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings July 20, 2026 23:06

Copilot started reviewing on behalf of doublegate July 20, 2026 23:07 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread docs/accuracysnes-coverage.md

coderabbitai Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread tests/roms/AccuracySNES/gen/src/dossier.rs

coderabbitai Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread tests/roms/AccuracySNES/gen/src/dossier.rs Outdated

coderabbitai Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread tests/roms/AccuracySNES/gen/src/tests/apu.rs Outdated

doublegate merged commit e004022 into main Jul 20, 2026
13 of 14 checks passed

doublegate deleted the feat/accuracysnes-e6-pitch branch July 20, 2026 23:45

doublegate mentioned this pull request Jul 21, 2026

feat(accuracysnes): emulation mode uses its own vector table (A6.02) #189

Merged

Navigation Menu

Repository navigation

feat(accuracysnes): pitch scales the sample rate, and $2000 is an octave up (E6.02) - #185

feat(accuracysnes): pitch scales the sample rate, and $2000 is an octave up (E6.02)#185
doublegate merged 4 commits into
mainfrom
feat/accuracysnes-e6-pitch

doublegate commented Jul 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

gemini-code-assist Bot commented Jul 20, 2026

Uh oh!

coderabbitai Bot commented Jul 20, 2026 •

edited

Loading

Review limit reached

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

doublegate commented Jul 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why three tests for one row

The margin was measured, not assumed

Validation

Uh oh!

gemini-code-assist Bot commented Jul 20, 2026

Uh oh!

coderabbitai Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

doublegate commented Jul 20, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 20, 2026 •

edited

Loading