test(accuracysnes): three Group E rows — E8.01, E9.02, E5.06 (T-04-E) - #298
Merged
Conversation
PAL, interlace on, field set, V=311 is 1368 master clocks and 341 dots. Its shape is the opposite of the short line's, which is why these are two changes and not one: B2.02 substitutes dot lengths, while this appends a whole extra 4-clock dot and leaves the two 6-clock dots alone (339*4 + 2*6 = 1368). So it moves the H wrap, via a new Ppu::dots_this_line, rather than the Bus's clock table -- and the H counter reaches dot 340 on that line and on no other. The H-IRQ comparator's upper bound follows the same per-line count instead of the constant, so an HTIME landing on dot 340 is a genuine match on the long line and stays suppressed everywhere else. It could previously never match anywhere. PAL interlaced frames alternate 425,568 / 425,572. The test enables interlace through SETINI $2133 bit 0 rather than poking the field, and asserts the SET of frame lengths rather than their order -- which phase carries the extra 4 clocks depends on the field's power-on value, and pinning that would pin an initial condition this row does not care about. Progressive PAL is asserted separately to be unaffected. Both fail under their own injection. 872 workspace tests pass; no golden moved, the long line being in vblank. Separate and still unmodelled: the interlaced frame's extra scanline (263/313 vs 262/312). B2.03 is reachable without it since V=311 is the last PAL line either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both scanline variants are modelled now, so the section that recorded a doubt about reachability is replaced with what was actually built and what remains. The part worth writing down: B2.07 does NOT follow from the model change. The frame-length difference is 4 master clocks -- one dot -- and the hv_begin/hv_end instrument costs a measured 175 dots, so resolving it needs a differential across many frames or a different instrument. That is the same wall A5.18 is parked on, and assuming B2.07 is now easy would waste a session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records the concrete design rather than leaving the next session to restart from the dossier's one-line assertion. E8.01 follows e8_02's shape, which already solves the hard parts: e8_02_sounding_voice builds the voice, e8_02_time_to_envx counts timer-2 ticks to a non-zero ENVX, and T2DIV = 2 gives exactly one tick per output sample. The assertion is a DIFFERENTIAL: write KON twice, the second offset by one sample (32 SPC cycles), and compare the delays. Polled every second sample means they differ by 1; polled every sample means they are equal. So the row asserts "the two differ", which assert_a16_range expresses -- one of the cases the never-hand-write-a-verdict-byte rule exists for. Carries forward both traps e8_02 already paid for: the voice must be fully silent before the second key-on (reuse its arming guard, do not re-derive it), and T2DIV must stay 2 because 1 puts the reading next to TnOUT's wrap and the NTSC/PAL drift gate catches it on PAL alone. Verify by injecting at the named site -- force the KON poll to every sample in the DSP and confirm this row's code fires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deliberately not a PR. The row fails on all three implementations, and committing it to a mergeable branch would break the battery. Authored, registered, and clean through every generator gate -- the 24-column menu-label cap, the dossier map, and the slot-collision gate each caught something real and each was fixed (slots moved to 256-258 off the printed free list). Then the cross-check rejected it on RustySNES (FAIL code 2), snes9x (14 -> 15 failing) and Mesen2 (1 -> 2). Three implementations failing identically is the broken-test signature, and it is correct here. Diagnosis: code 2 is phase A's band guard and the band is wrong. I set 4..13 by reusing E8.02's numbers, but E8.02's 4..11 bounds a DELTA between a sounding-voice baseline and a key-on, while E8.01 phase A reads an ABSOLUTE delay -- key-on latency plus the poll-loop cost. A band for a difference cannot bound a raw reading. The fix is not to widen it. Measure slots 256/257 out of a real run and set the band from what the instrument reports. And because phase A never passed, the difference assertion has never been meaningfully evaluated -- so it is still unverified whether the 16-NOP offset moves the reading at all, and whether 32 SPC cycles really is one output sample here rather than an assumption from round numbers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two real defects, neither of which was the band I was asked to fix. Both phases must start from silence. The first draft keyed phase A off e8_02_sounding_voice, which leaves the voice at full scale, so its poll exited on the first pass (1 tick) while phase B, keyed from silence, measured 8. The 7-tick gap was that asymmetry, not the poll phase -- no band would have made that comparison mean anything. Extracted e8_01_silence and used it before both phases, one guard slot covering both. The offset must be a HALF sample, not a whole one. A full-sample offset measured a difference of ZERO: at T2DIV = 2 one timer tick, one output sample and the shift are all 32 SPC cycles, so shifting by exactly that moves timer, sample clock and KON in lockstep and cancels. Eight NOPs move KON's phase within the sample and measure 8 vs 7 -- the +/-1 a 16 kHz poll predicts. Bands now come from measured values, not from E8.02's delta band. Still not a PR: Mesen2 went 1 -> 3 failing. One is the pre-existing A2.10; the others are unidentified and may be the documented trap where adding a test shifts the DSP poll phase for a downstream row. Identify which before landing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
E8.01 is catalogue index 263 and reads $01 -- plain PASS -- under Mesen2. The rows that fail are Group F input rows (idx279 = F1.03, byte $04 = FAIL code 2): the port-2 limitation the oracle fix documented, materialising. mesen_crossval.lua now sends one setInput call, so PAD2_CONTRACT-dependent rows cannot pass under MesenCE. Nothing to do with the DSP poll phase. Also records the status encoding, because misreading it cost a round: odd = PASS (odd != $01 being "PASS variant n"), even = "FAIL code b/2", $00 = NOTRUN, $FF = SKIP. A dump of non-$01 bytes looks alarming and is mostly passes -- 28 of 29 such entries were PASS variants or a SKIP. Count even bytes only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A direct WRAM probe counts 1 even-byte failure (F1.03, the port-2 row) stably at 1400/1600/2000 frames, while crossval.sh reports "Mesen2: 3 failing test(s)" on the same build. E8.01 reads $01 -- PASS -- in all three probe runs, so it is not among them. But the counts do not match and I have not explained why. Candidates, none checked: the runner's MAX_FRAMES of 4000 puts the cart in a different phase; the runner also scores the PAL image and may fold those into the same line; or it iterates n = rd16(COUNT) over a range the probe's 0..335 loop does not. Recorded rather than rounded off, because a row that passes while the tally it lives in is unexplained is how a later real regression gets absorbed into "the usual 3". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…builds crossval.sh runs ref-proj/Mesen2/.../Mesen.dll under dotnet; every probe in this investigation used ref-proj/MesenCE/.../Mesen, a separate native build. Both directories exist, and the "Mesen2" label in the crossval output means the .dll. So "1 failure" and "3 failures" were never in conflict -- they measured different implementations. A probe meant to predict the gate must use the dotnet binary. With that settled, E8.01 is: pass on RustySNES, pass on snes9x (back to its baseline 14), pass on MesenCE (stable across 1400-4000 frames), and FAIL code 4 on Mesen2 -- the difference assertion. The half-sample offset moves the reading on three implementations and not on Mesen2. That is the favourable three-agree-one-differs shape, and both of this cart's Mesen2-only disagreements now sit in that one runner. But it is not proof: a harness bug upstream of an implementation produces the same signature and did once here. Next step is ares or bsnes as a fourth opinion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Attempted bsnes as the tiebreak for E8.01's Mesen2-only disagreement. The core is built and libretro_crossval.c takes any core as argv[1], but the host exits 255 with "core exposes no usable SYSTEM_RAM (0 bytes, need 131072)": bsnes's libretro core does not publish WRAM through retro_get_memory_data, so the battery's verdicts cannot be read out of it. ares is source-only, not built. Recorded so the next attempt does not repeat this route. A fourth opinion needs ares built, bsnes driven through its own debugger, or a bsnes libretro build exposing RETRO_MEMORY_SYSTEM_RAM. E8.01 stays on this WIP branch: passing on three implementations with the fourth's disagreement characterised, but the discriminator that would settle which side is right has not been run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lures entry
Went to mirror SNES9X_KNOWN_FAILURES with a Mesen2 constant and listed
the set instead of the count. It moves:
before E8.01: 1 failure, reported as A2.10 (idx 11)
after E8.01: 3 -- idx263 E8.01 code 4, idx279 F1.03 code 2,
idx286 code 2
A2.10 is absent from the second list. It failed, then passed, with
nothing touching it. A MESEN2_KNOWN_FAILURES = 3 constant would
therefore encode noise and mask a future real regression inside "the
usual 3".
This is the cart's documented timing-phase trap showing up in the
RUNNER rather than in a row, and it matters beyond E8.01: Mesen2 cannot
be a reliable gate until its verdicts are reproducible run-to-run.
Also recorded: MesenCE is Mesen Community Edition, the maintained fork
of the same emulator, and it passes E8.01 -- so the disagreement is
between two builds of one lineage, not two independent
implementations. That weakens the "three implementations agree" reading
and is a further reason not to bless the row yet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s wrong I reported Mesen2's failing set as moving (A2.10 failing then passing) and concluded the runner could not be trusted as a gate. That was wrong, and the fault was the instrument: my probes read R_STATUS at a fixed frame count (1600, 4000) while the battery finishes at ~479, so they were sampling bytes after the cart had spent 1000+ frames in its results menu with Start held. mesen_crossval.lua reads at DONE == $A5, which is why it was consistent throughout. Read at DONE, Mesen2 is exactly reproducible: two runs, identical set, both completing at frame 479 -- idx263 E8.01 code 4 idx279 F1.03 code 2 idx286 F1.10 code 2 F1.03 and F1.10 are Group F input rows, i.e. the documented PAD2_CONTRACT port-2 limitation of the single-setInput fix. So E8.01 is the ONLY unexplained Mesen2 failure. Any future probe of this cart must read at DONE, not at a frame budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…agile Adds MESEN2_KNOWN_FAILURES=3 and MESEN2_PAL_KNOWN_FAILURES=1 on the same mechanism as the snes9x constant, set only after reading the failing SET at R_DONE == $A5 and reproducing it exactly across runs. crossval.sh now reports "2 reference(s) agree with the cart" instead of DISAGREEMENT. Two of the three NTSC rows are the documented port-2 limitation (F1.03, F1.10): one setInput call, so PAD2_CONTRACT rows cannot pass under this runner. Covered by the in-repo harness and snes9x. The third is E8.01, and enumerating rather than counting is what exposed the real problem: it fails on the NTSC image and PASSES on the PAL image, same Mesen2 build. The S-DSP's 32 kHz sample rate is region-independent, so a genuine KON-poll-rate assertion cannot care which image it runs on. A verdict that flips with the region encodes a timing PHASE, not the poll rate. So E8.01 is not merged, and the reason is not the missing fourth opinion -- the row is not robust. Its agreement on the other three implementations may be alignment luck rather than the mechanism. Fixing it means sweeping several sub-sample offsets and asserting at least one moves the delay, instead of betting on a single chosen offset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sets Worked out the actual fix rather than leaving "sweep more offsets", which does not escape the problem. The timer is the cause: at T2DIV = 2 one tick IS one output sample, so a 32-cycle shift moves the tick boundary and the KON together and cancels exactly (measured diff 0), while a 16-cycle shift dodges that but only crosses a poll boundary for some starting phases. Sweeping offsets inherits both halves -- whole-tick offsets always cancel, the rest stay phase-dependent. Replace the instrument: count poll-loop iterations until ENVX reads non-zero instead of timer ticks. Resolution drops from a whole sample to a few SPC cycles, the counter no longer quantises to the sample clock, and a sub-sample offset shows up directly. This is what rescued F1.09, which was vacuous at line granularity and landed once it counted polls. Also notes that removing the timer frees PORT3 from its guard-sibling duty, so a four-point phase sweep can report without a second upload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements the prescribed fix: e8_01_poll_count replaces the timer with a counted poll loop, removing the sample-clock quantisation that made a whole-sample offset cancel exactly. The instrument is genuinely better -- counts land at 12-14 passes where the timer gave 5-11 ticks, so a sub-sample shift moves the reading by ~2 units instead of 0-or-1. But the region asymmetry SURVIVES. On Mesen2, read at DONE: NTSC gives A=14 B=12 (diff 2), PAL gives A=13 B=13 (diff 0). Same emulator, same row. So the timer was a confound, not the confound. Phase A itself differs by region (14 vs 13), which points at the real problem: something region-dependent moves the cart's own starting phase between the two images, so the row needs its measurement anchored to a known DSP phase before any offset is applied -- not merely a finer counter. Bands are left stale on purpose (both images now fail code 2, the band guard, rather than code 4). Re-banding to make both pass would hide the asymmetry, which is the actual open question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…up-e # Conflicts: # docs/accuracysnes-plan.md
Coverage 347 -> 350 of 443 (297 on-cart + 53 scenes); battery 338 tests at 100%. Both references agree: snes9x 14 known divergences, Mesen2 2 (down from 3), 53/53 scenes on each. E8.01 (KON examined at 16 kHz). The two earlier drafts timed a delay from the KON write to the voice starting. That delay is a sawtooth in the write's phase against the DSP's examination grid, and two readings of a sawtooth cannot recover its period — so the poll-count draft reported a difference of 2 on the NTSC image and 0 on the PAL image of the same build, because the phase is set by where the IPL handshake left the SPC700 and the S-CPU paces that. Neither draft was scored. The replacement measures writes the DSP never sees: KON is a register, not a queue, so a second write before the DSP next looks cancels the first. Eight writes 24 SPC cycles apart span 168 cycles, and a half-open interval of that length contains floor(168/P) or floor(168/P)+1 grid points for EVERY phase — 3-4 voices started at 16 kHz, 6-7 at 32 kHz. Mesen2, which failed the phase-fragile draft, passes this one. E9.02 (noise output is bipolar). E9.01 pins the frozen seed; this scores the transition, because one LFSR step must turn a full-scale negative reading ($81) into a positive one at about half scale. A core emitting the register's 15-bit value directly reads positive throughout — the DC-heavy noise the errata's highpass remark excludes. E5.06 (BRR wraps at 15 bits). The first assertion — "at least one of 64 OUTX readings is negative" — was vacuous, and the injection is what proved it: clamping the store made the row pass HARDER, 64 negatives against the correct decoder's 32. A clamping decoder pins the buffer at +7FFEh and the gaussian interpolator's three-term partial sum overflows from that constant, so the interpolator was supplying the sign. The row now asserts bipolarity, and both saturating variants fail it. E9.01 and E9.02 stand down as SKIP on a menu restart: E9.02 steps the LFSR and FLG bit 7 does not re-seed it (checked against ares, which re-seeds only in DSP::power — a comment claiming otherwise is corrected). The harness's restart-idempotence check moves from first_run_passed - 1 to - 3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (9)
Comment |
Both are real and neither changes a byte of the ROM (checksum $0BE1 either way): the noise-rate literal becomes hex, and `all()` carries an explicit too_many_lines allow rather than being split, because the registration order is load-bearing in three places and a function boundary would hide that. My local clippy run reported clean before the push — it had reused a stale fingerprint for accuracysnes-gen from the `cargo run` that preceded it. Touch the changed sources before trusting a clippy pass on a crate you just ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… counts docs/apu.md gains the two S-DSP properties AccuracySNES now scores on-cart: KON is examined every second sample so a close-following write cancels the one before it, and FLG bit 7 does NOT re-seed the noise LFSR (ares re-seeds only in DSP::power). The second is why E9.01/E9.02 are power-on-only. docs/STATUS.md: 344/443 -> 350/443, 332 -> 338 tests. Its "provably uncoverable" phrasing is softened, because E5.06 and B2.02/B2.03 were all in that bucket and all three have now landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds three new AccuracySNES Group E (SPC700/S-DSP) rows to close out the v1.29.0 batch, along with regenerated catalogs/docs and small harness + cross-validation updates to reflect the new measurements and known divergences.
Changes:
- Add new Group E rows
E5.06(BRR 15-bit wrap),E8.01(KON poll rate), andE9.02(bipolar noise output), including a cold-boot gate and ordering constraints for the noise LFSR. - Update the test harness to report the new batch’s measurement slots and adjust restart-idempotence expectations (
-1→-3). - Update cross-validation script expectations for Mesen2 known divergences, and regenerate/update catalogs + docs + changelog entries.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/gen/src/tests/apu.rs | Implements the three new Group E tests and supporting helper refactors/gates. |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Adds dossier mapping entries for the new rows. |
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Regenerated catalog entries to include the new tests. |
| tests/roms/AccuracySNES/ERROR_CODES.md | Adds documented failure-code tables for E5.06, E8.01, and E9.02. |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Regenerated assembly output including the new test bodies and APU program blobs. |
| crates/rustysnes-test-harness/tests/accuracysnes.rs | Reports new measurement slots and updates restart-idempotence assertion to account for new SKIPs. |
| scripts/accuracysnes/crossval.sh | Adds Mesen2 known-divergence constants and enforces expected counts for NTSC/PAL runs. |
| docs/accuracysnes-plan.md | Updates plan/status narrative to mark these rows as landed and records the design rationale. |
| docs/accuracysnes-coverage.md | Updates coverage counts to reflect the three newly covered assertions. |
| CHANGELOG.md | Records the new rows, coverage delta, and the restart-idempotence behavior change. |
Counts re-derived from docs/accuracysnes-coverage.md, which regenerates with the ROM, rather than from the hand-maintained estimate the row carried. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR adds three on-cart AccuracySNES APU test rows ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the
v1.29.0Group E batch. Coverage 347 → 350 of 443 (297 on-cart + 53 scenes), battery 338 tests at 100%, andMESEN2_KNOWN_FAILUREScomes down from 3 to 2.E8.01— the two earlier drafts were measuring their own timingBoth timed a delay: write
KON, count untilENVXleaves zero. That delay is(P − φ) mod P— a sawtooth in the write's phase against the DSP's examination grid — and two readings of a sawtooth cannot recover its period.φis fixed by where the IPL handshake left the SPC700, which the S-CPU paces, so it differs between the NTSC and PAL images of the same build. The poll-count draft duly reported a difference of 2 on NTSC and 0 on PAL. Neither draft was ever scored.The replacement measures writes the DSP never sees.
KONis a register, not a queue: a second write before the DSP next looks cancels the first, and that voice is never keyed. Eight writes 24 SPC cycles apart span 168 cycles, and a half-open interval of lengthLcontains⌊L/P⌋or⌊L/P⌋ + 1grid points for every phase:KONexaminedRustySNES measures 3 (mask
$25— voices 2, 5, 7). Mesen2, which failed the phase-fragile draft, passes this one.E9.02— the step, not the stateE9.01already pins the frozen seed at$4000, so this scores the transition: one LFSR step turns$81(full-scale negative) into$3F/$1F(positive, halved). A core emitting the register's 15-bit value directly reads positive throughout — the DC-heavy noise the errata's "highpass" remark exists to exclude.E5.06— the first assertion was vacuous, and the injection proved it"At least one of 64
OUTXreadings is negative" looked airtight for a purely positive drive. Injecting the named bug made the row pass harder: clamping the store gave 64 negatives out of 64, against the correct decoder's 32. A clamping decoder pins the buffer at+7FFEh, and the gaussian interpolator's three-term partial sum (the truncationE5.13is about) overflows a signed 16-bit intermediate from that constant — the interpolator, not the decoder, was supplying the sign.The row now asserts bipolarity: a saturating decoder settles, and a settled decoder's readings carry one sign. Both saturating variants — clamp at 15 bits, saturate at 16 after the shift — fail it.
Stated, not buried
E9.02is the only row that moves the noise LFSR, and nothing can put it back:FLGbit 7 is the DSP's soft reset and does not re-seed the shift register (checked against ares, which re-seeds only inDSP::power; a comment invoice_programclaimed otherwise and is corrected). So it is registered last, and both it andE9.01stand down as SKIP on a menu restart — the same gateF1.07uses for its unwritten$4218. The harness's restart-idempotence check moves fromfirst_run_passed - 1to- 3.Verification
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps,cargo test --workspace— all clean.cargo test -p rustysnes-test-harness --test accuracysnes --features test-roms— 56 passed, 0 failed.REF_PROJ=$PWD/ref-proj bash scripts/accuracysnes/crossval.sh— as quoted above.E8.01→ per-sampleKONexamination → FAIL code 1;E9.02→ unipolar noise output → code 1, sign-preserving step → code 3, four-place shift → code 4;E5.06→ 15-bit clamp and 16-bit saturate → code 2. No row moved under another row's injection.🤖 Generated with Claude Code