fix(accuracysnes): E3.06 records the counts it compares — ares' failure is a wrap - #307
Conversation
…e is a wrap The row asserts timer 2 ticks ~8x timer 0 over one interval, with a band of 8..15 -- and TnOUT is FOUR BITS, so the band ends one tick short of its own wrap. That tightness is structural rather than a choice: timer 0 must tick at least once for the ratio to mean anything, one timer-0 period IS eight timer-2 periods, so timer 2 can never read below 8 and the wrap is only a factor of two above. Nothing recorded the counts, so a failure said only "not 8x" -- which is why ares' failure looked like a rate divergence. With the two slots added: RustySNES reads timer 2 = 10, ares reads 0, and 0 is what 16 reads as. ares' own Timer<128>/Timer<128>/Timer<16> declarations are an exactly correct 8:1 ratio, which makes the wrap the likelier reading. So E3.06 is a WEAK ROW, not a settled divergence: it cannot distinguish "timer 2 wrapped" from "timer 2 is not at 64 kHz". The assertion message now says so and points at the slot, rather than asserting a conclusion the instrument cannot support. The ares host also dumps the whole measurement channel now, not a curated list: it exists to arbitrate disagreements, and which slot matters is decided by whichever row turns out to disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughE3.06 now records timer 0 and timer 2 measurements, accounts for four-bit timer-counter wrap, and reports measurement slots. The ares host dumps nonzero measurement-channel values. Documentation records the differing RustySNES and ares readings. ChangesE3.06 timer measurement diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
Antigravity review (Gemini via Ultra)This PR updates the Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
This PR improves AccuracySNES’s ability to adjudicate emulator divergences by turning E3.06 into an explicitly measured row: it records the raw timer counts it compares, and updates the ares host/harness/docs to surface those measurements for cross-emulator analysis.
Changes:
- Record
E3.06timer-0 and timer-2 tick counts into measurement slots 266/267 before asserting. - Extend the ares host and RustySNES test harness reporting to include the new measurement slots.
- Update generated error-code text and documentation/changelog to reflect the new “measured, not assumed” interpretation.
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 | Adds measurement recording for E3.06 before assertions and updates the failure message to point at the recorded slots. |
| tests/roms/AccuracySNES/ERROR_CODES.md | Updates the generated failure description for E3.06 to note the wrap ambiguity and reference slot 267. |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Regenerated ASM reflecting the new measurement writes for slots 266/267 and updated comments/messages. |
| scripts/accuracysnes/ares_host/README.md | Documents that E3.06 is likely a wrap-related weakness and that the row now records raw counts. |
| scripts/accuracysnes/ares_host/ares_host.cpp | Adds emission of measurement-channel values from WRAM for cross-validation. |
| docs/accuracysnes-coverage.md | Updates generated coverage totals (including rendered-scene coverage count). |
| crates/rustysnes-test-harness/tests/accuracysnes.rs | Adds labels for the new Group E batch measurement slots (266/267) to the harness report. |
| CHANGELOG.md | Adds an Unreleased entry describing the new E3.06 measurements and the ares-host measurement dump. |
| // The measurement channel, which is where a row puts the numbers a one-byte verdict cannot carry. | ||
| // Emitted for every slot rather than a curated list: this host exists to arbitrate disagreements, | ||
| // and which slot matters is decided by whichever row turns out to disagree. | ||
| const u32 MEAS = 0xE200; | ||
| for(u32 i = 0; i < 512; i++) { | ||
| u32 v = wram[MEAS + i * 2] | (wram[MEAS + i * 2 + 1] << 8); | ||
| if(v) printf("meas %u %u\n", i, v); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@scripts/accuracysnes/ares_host/ares_host.cpp`:
- Around line 175-182: Update the measurement loop in the ares host output path
to print a record for every slot, including measurements whose value is zero.
Remove the conditional suppression around the printf in the loop iterating over
MEAS so wrapped timer values remain distinguishable from unwritten slots.
In `@tests/roms/AccuracySNES/gen/src/tests/apu.rs`:
- Around line 8106-8130: Update the APU timer test around the timer 0 and timer
2 assertions to enforce the intended 8:1 relationship under modulo-16 reads,
either by requiring timer 0 to equal 1 before applying the 8–15 timer 2 range or
by comparing timer 2 against timer 0 modulo 16; ensure the test distinguishes
the 64 kHz behavior from the 32 kHz alternative. In
scripts/accuracysnes/ares_host/README.md lines 56-63, remove the claim that
timer 2 cannot read below 8 and document the modulo-16 ambiguity. In
CHANGELOG.md lines 14-25, revise the Unreleased entry to describe the corrected
assertion semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4253512-f4ad-493d-8acf-f5e61632c284
⛔ Files ignored due to path filters (5)
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/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 (5)
CHANGELOG.mdcrates/rustysnes-test-harness/tests/accuracysnes.rsscripts/accuracysnes/ares_host/README.mdscripts/accuracysnes/ares_host/ares_host.cpptests/roms/AccuracySNES/gen/src/tests/apu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (13)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
scripts/accuracysnes/ares_host/ares_host.cppcrates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/tests/apu.rsCHANGELOG.mdscripts/accuracysnes/ares_host/README.md
scripts/accuracysnes/**
⚙️ CodeRabbit configuration file
scripts/accuracysnes/**: The cross-validation harness: the same AccuracySNES image is run on snes9x (through a
libretro host in C) and on Mesen2 (through its test runner and a Lua script), and their
verdicts are compared with the cart's. Its integrity is the whole argument for the
battery, so flag anything that could make a reference appear to agree — a verdict parsed
loosely, a missing-file path that degrades to success, a scene comparison that skips
rather than fails when the golden is absent. A known reference divergence belongs in
SNES9X_KNOWN_FAILURESwith a source citation, never in a widened match.
Files:
scripts/accuracysnes/ares_host/ares_host.cppscripts/accuracysnes/ares_host/README.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/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:
crates/rustysnes-test-harness/tests/accuracysnes.rstests/roms/AccuracySNES/gen/src/tests/apu.rsCHANGELOG.mdscripts/accuracysnes/ares_host/README.md
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/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/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
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.mdscripts/accuracysnes/ares_host/README.md
🧠 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
🪛 Clang (14.0.6)
scripts/accuracysnes/ares_host/ares_host.cpp
[warning] 178-178: variable 'MEAS' is not initialized
(cppcoreguidelines-init-variables)
[warning] 179-179: variable 'i' is not initialized
(cppcoreguidelines-init-variables)
[warning] 180-180: variable 'v' is not initialized
(cppcoreguidelines-init-variables)
[warning] 180-180: variable name 'v' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 181-181: statement should be inside braces
(readability-braces-around-statements)
🔇 Additional comments (4)
tests/roms/AccuracySNES/gen/src/tests/apu.rs (2)
8090-8130: 🎯 Functional CorrectnessVerify that the generated AccuracySNES cartridge was rebuilt.
This file is under
tests/roms/AccuracySNES/gen/. A stale generated cartridge can execute the previous E3.06 program while the source review appears correct. Confirm that the rebuild used the generator source and did not hand-edit generated assembly.As per coding guidelines, rebuild AccuracySNES after every change to
gen/orasm/, and never hand-edit generated assembly.Source: Coding guidelines
8098-8104: 🗄️ Data Integrity & IntegrationNo measurement-slot collision exists for slots 266 and 267. The generated writers claim these slots only in E3.06, and
dossier::check_slotsvalidates all literal and computedrecordslots.> Likely an incorrect or invalid review comment.crates/rustysnes-test-harness/tests/accuracysnes.rs (1)
599-617: LGTM!scripts/accuracysnes/ares_host/README.md (1)
43-43: LGTM!
| // The measurement channel, which is where a row puts the numbers a one-byte verdict cannot carry. | ||
| // Emitted for every slot rather than a curated list: this host exists to arbitrate disagreements, | ||
| // and which slot matters is decided by whichever row turns out to disagree. | ||
| const u32 MEAS = 0xE200; | ||
| for(u32 i = 0; i < 512; i++) { | ||
| u32 v = wram[MEAS + i * 2] | (wram[MEAS + i * 2 + 1] << 8); | ||
| if(v) printf("meas %u %u\n", i, v); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not suppress zero-valued measurements.
When timer 2 wraps from 15 to 0, line 181 drops slot 267. The output then cannot distinguish a valid wrapped zero from an unwritten slot. This prevents the host from establishing the ares result described by the README and changelog.
Print every slot, or emit explicit zero records for all owned slots.
Proposed fix
- if(v) printf("meas %u %u\n", i, v);
+ printf("meas %u %u\n", i, v);As per path instructions, the cross-validation harness must not let omitted measurement output masquerade as a valid result.
📝 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.
| // The measurement channel, which is where a row puts the numbers a one-byte verdict cannot carry. | |
| // Emitted for every slot rather than a curated list: this host exists to arbitrate disagreements, | |
| // and which slot matters is decided by whichever row turns out to disagree. | |
| const u32 MEAS = 0xE200; | |
| for(u32 i = 0; i < 512; i++) { | |
| u32 v = wram[MEAS + i * 2] | (wram[MEAS + i * 2 + 1] << 8); | |
| if(v) printf("meas %u %u\n", i, v); | |
| } | |
| // The measurement channel, which is where a row puts the numbers a one-byte verdict cannot carry. | |
| // Emitted for every slot rather than a curated list: this host exists to arbitrate disagreements, | |
| // and which slot matters is decided by whichever row turns out to disagree. | |
| const u32 MEAS = 0xE200; | |
| for(u32 i = 0; i < 512; i++) { | |
| u32 v = wram[MEAS + i * 2] | (wram[MEAS + i * 2 + 1] << 8); | |
| printf("meas %u %u\n", i, v); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 178-178: variable 'MEAS' is not initialized
(cppcoreguidelines-init-variables)
[warning] 179-179: variable 'i' is not initialized
(cppcoreguidelines-init-variables)
[warning] 180-180: variable 'v' is not initialized
(cppcoreguidelines-init-variables)
[warning] 180-180: variable name 'v' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 181-181: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/accuracysnes/ares_host/ares_host.cpp` around lines 175 - 182, Update
the measurement loop in the ares host output path to print a record for every
slot, including measurements whose value is zero. Remove the conditional
suppression around the printf in the loop iterating over MEAS so wrapped timer
values remain distinguishable from unwritten slots.
Source: Path instructions
| a.c("Timer 0 first: one tick, maybe two. Zero would make the ratio below unmeasurable."); | ||
| a.l("lda f:$7E0101"); | ||
| a.l("and #$00FF"); | ||
| a.assert_a16_range( | ||
| 1, | ||
| 3, | ||
| "timer 0 did not tick once over this interval, or ticked more than three times — either \ | ||
| way the interval is not the one this test needs and the ratio below means nothing", | ||
| ); | ||
| a.c("Timer 2 over the SAME interval: eight times the rate, so eight or more ticks."); | ||
| a.c( | ||
| "Timer 2 over the SAME interval: eight times the rate, so eight or more ticks. The band is", | ||
| ); | ||
| a.c("8..15 and `TnOUT` wraps at 16, which leaves NO headroom above -- and that is structural,"); | ||
| a.c("not a choice: timer 0 must tick at least once for the ratio to mean anything, and one"); | ||
| a.c("timer-0 period IS eight timer-2 periods, so timer 2 can never be below 8 and the wrap is"); | ||
| a.c("only a factor of two away. Measured: RustySNES 10, ares 0 -- and 0 is what 16 reads as."); | ||
| a.l("lda f:$7E0102"); | ||
| a.l("and #$00FF"); | ||
| a.assert_a16_range( | ||
| 8, | ||
| 15, | ||
| "timer 2 did not count roughly eight times what timer 0 did over the same interval, so it \ | ||
| is not running from the 64 kHz stage — a core reading $01 here runs every timer at 8 kHz", | ||
| is not running from the 64 kHz stage — a core reading $01 here runs every timer at 8 kHz. \ | ||
| NOTE: this row cannot distinguish that from timer 2 having WRAPPED past 16, so read slot \ | ||
| 267 before concluding anything; ares fails here with a recorded 0", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the E3.06 assertion and documentation match the modulo-16 timer contract.
The test accepts timer 0 values from 1 through 3 but checks timer 2 against 8 through 15. If timer 0 reads 2, a 32 kHz timer 2 can produce 8 and pass, while a correct 64 kHz timer 2 produces 16 and reads as 0. The current implementation can accept the wrong rate and reject the correct rate while claiming to validate an 8:1 ratio.
tests/roms/AccuracySNES/gen/src/tests/apu.rs#L8106-L8130: compare timer 2 with timer 0 modulo 16, or require timer 0 to equal 1 before applying the 8–15 band.scripts/accuracysnes/ares_host/README.md#L56-L63: remove the claim that timer 2 cannot read below 8 and document the actual ambiguity.CHANGELOG.md#L14-L25: update the Unreleased note to match the corrected assertion semantics.
As per path instructions, an AccuracySNES test must distinguish the named hardware behavior from alternatives.
📍 Affects 3 files
tests/roms/AccuracySNES/gen/src/tests/apu.rs#L8106-L8130(this comment)scripts/accuracysnes/ares_host/README.md#L56-L63CHANGELOG.md#L14-L25
🤖 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 8106 - 8130,
Update the APU timer test around the timer 0 and timer 2 assertions to enforce
the intended 8:1 relationship under modulo-16 reads, either by requiring timer 0
to equal 1 before applying the 8–15 timer 2 range or by comparing timer 2
against timer 0 modulo 16; ensure the test distinguishes the 64 kHz behavior
from the 32 kHz alternative. In scripts/accuracysnes/ares_host/README.md lines
56-63, remove the claim that timer 2 cannot read below 8 and document the
modulo-16 ambiguity. In CHANGELOG.md lines 14-25, revise the Unreleased entry to
describe the corrected assertion semantics.
Source: Path instructions
Continues #305/#306'''s adjudication.
E3.06was one of two genuinely ares-only divergences; it is now measured rather than assumed.The row had no headroom, structurally
E3.06asserts timer 2 ticks ~8x timer 0 over one interval, band8..15.TnOUTis four bits, so the band ends one tick short of its own wrap.That tightness is not a choice. Timer 0 must tick at least once for the ratio to mean anything; one timer-0 period is eight timer-2 periods; so timer 2 can never read below 8, and the wrap sits only a factor of two above.
Nothing recorded the counts, which is why ares''' failure looked like a rate bug
0 is what 16 reads as. And ares''' own declarations —
Timer<128>,Timer<128>,Timer<16>— are an exactly correct 8:1 ratio, which makes the wrap the likelier reading by some margin.So
E3.06is a weak row, not a settled divergenceIt cannot distinguish "timer 2 wrapped" from "timer 2 is not running at 64 kHz". The assertion message now says exactly that and points at the recording slot, instead of asserting a conclusion the instrument cannot support.
I have not widened the band or changed the interval — both would trade one blind spot for another, and the right fix (poll timer 2 and accumulate, so the 4-bit counter stops being the ceiling) is real work that deserves its own change.
Also
The ares host dumps the whole measurement channel rather than a curated list. It exists to arbitrate disagreements, and which slot matters is decided by whichever row turns out to disagree — a curated list would have to be edited every time that changes.
Verification
cargo fmt --check,cargo clippy --workspace --exclude rustysnes-android --all-targets -- -D warnings— clean.cargo test -p rustysnes-test-harness --test accuracysnes --test accuracysnes_scenes --features test-roms— 58 passed, 0 failed.REF_PROJ=$PWD/ref-proj bash scripts/accuracysnes/crossval.sh—snes9x: OK (14 known),Mesen2: OK (2 known), 54 scenes match on both,2 reference(s) agree with the cart.🤖 Generated with Claude Code
Summary
AccuracySNES E3.06now records timer 0 and timer 2 counts used by the ratio check. The dossier assertion identifies timer 2 wrapping as unresolved when ares reports0; a four-bitTnOUTcounter can represent0after wrapping from16.The claim is false if timer 2 does not wrap and ares instead reports
0because timer 2 is not running at 64 kHz. RustySNES reports counts of1and10; ares reports1and0, so the test cannot distinguish these cases.The altered dossier assertion is
E3.06. The test adds measurement slots266and267for timer 0 and timer 2. The coverage denominator does not move because these slots record diagnostic values and do not add dossier assertions.The ares host now reports the complete measurement channel. The timer interval and accepted comparison band remain unchanged.