Repository navigation
feat(accuracysnes): TCS/TXS set no flags, and ORA [d] is 24-bit (A1.09, A9.03) - #187
Conversation
…9, A9.03) The stack pointer is not data, so moving a value into it does not describe that value. A core routing every transfer through one flag-setting helper gets this wrong in a way nothing crashes on — it changes which branch is taken after a TXS. Both instructions are handed the value the stack pointer already holds, so S is written with what was in it: a native-mode TXS is a full 16-bit write, and a test that put anything else there would be pushing its own PHP into ROM. The surviving flag is planted with BIT #imm, which touches Z alone and so leaves the accumulator the transfer is about to move. TXA moving $8000 is the control, without which a core that sets no transfer flags at all passes. ORA [d] is the addressing mode most likely to be implemented as its 16-bit sibling with the data bank glued on, because for a pointer in bank $00 the two are identical. This image is 128 KiB so they are not: the same pointer with $01 in its third byte reads bank $01's signature where a (d)-style fetch reads bank $00's. A third reading with $0F already in the accumulator is what makes it an assertion about ORA rather than a load — the first two start from zero, where the two instructions are indistinguishable. Cross-validated: snes9x and Mesen2 agree on both, on both images. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (10)**/*.{rs,md,toml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
CHANGELOG.md📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.md⚙️ CodeRabbit configuration file
Files:
**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/roms/AccuracySNES/gen/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,s}📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/roms/AccuracySNES/**📄 CodeRabbit inference engine (Custom checks)
Files:
tests/roms/AccuracySNES/gen/src/**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
WalkthroughTwo AccuracySNES CPU tests cover transfer-instruction flags and 24-bit indirect ChangesCPU accuracy coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/tests/cpu.rs`:
- Around line 2018-2082: The TCS and TXS checks only verify Z preservation, so
seed N before each transfer and assert both N and Z remain unchanged in a1_07.
Update the assertions and related failure messages for both TCS and TXS while
preserving the existing TXA control check; in CHANGELOG.md lines 14-27, either
reflect the expanded N-and-Z coverage or narrow the claim to Z preservation if
the test remains Z-only.
🪄 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: 462d7e69-72c1-4430-8751-77865ec2a046
⛔ Files ignored due to path filters (6)
docs/accuracysnes-coverage.mdis excluded by!docs/accuracysnes-coverage.mdand included bydocs/**tests/roms/AccuracySNES/ERROR_CODES.mdis excluded by!tests/roms/AccuracySNES/ERROR_CODES.mdand included bytests/**tests/roms/AccuracySNES/SOURCE_CATALOG.tsvis excluded by!**/*.tsv,!tests/roms/AccuracySNES/SOURCE_CATALOG.tsvand included bytests/**tests/roms/AccuracySNES/asm/tests_group_a.sis excluded by!tests/roms/AccuracySNES/asm/tests_group_a.sand included bytests/**tests/roms/AccuracySNES/build/accuracysnes-pal.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**tests/roms/AccuracySNES/build/accuracysnes.sfcis excluded by!tests/roms/AccuracySNES/build/**and included bytests/**
📒 Files selected for processing (4)
CHANGELOG.mddocs/accuracysnes-plan.mdtests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/cpu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (12)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use Rust edition 2024 with the pinned toolchain (1.96); satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarning 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 correspondingdocs/<chip>.mdin the same pull request.
**/*.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 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 newunsafe { ... }block orunsafe fnmust 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.rstests/roms/AccuracySNES/gen/src/tests/cpu.rs
tests/roms/AccuracySNES/gen/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rebuild AccuracySNES after changes to
gen/orasm/; run the generator directly without piping throughtail -1, so generator panics remain visible.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/cpu.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:STZhas no long-addressing form,cop #$00must be emitted as.byte $02,$00, and menu labels must not exceed 24 columns.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/cpu.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.rsdocs/accuracysnes-plan.mdCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/cpu.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 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.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rsdocs/accuracysnes-plan.mdCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/cpu.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rsdocs/accuracysnes-plan.mdCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/cpu.rs
tests/roms/AccuracySNES/**
📄 CodeRabbit inference engine (Custom checks)
When adding or removing an AccuracySNES test or scene under
tests/roms/AccuracySNES/gen/src/, updatedossier.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.rstests/roms/AccuracySNES/gen/src/tests/cpu.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::MAPmust not claim an assertion another test already
implements. There is a build gate for this, but flag it in review too.
Files:
tests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/cpu.rs
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.mdas 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
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/accuracysnes-plan.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/accuracysnes-plan.mdCHANGELOG.md
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.
Files:
CHANGELOG.md
🔇 Additional comments (3)
tests/roms/AccuracySNES/gen/src/tests/cpu.rs (1)
79-80: LGTM!Also applies to: 2093-2157
tests/roms/AccuracySNES/gen/src/dossier.rs (1)
167-168: LGTM!docs/accuracysnes-plan.md (1)
16-16: LGTM!
There was a problem hiding this comment.
Pull request overview
Adds two new AccuracySNES CPU (65C816) assertions to expand on-cart coverage for tricky edge cases, and regenerates/updates the associated catalogs, docs, and changelog entries to reflect the new tests.
Changes:
- Add new scored cart tests
A1.07(covers dossier rowA1.09:TCS/TXSset no flags) andA9.04(covers dossier rowA9.03:ORA [d]uses a 24-bit pointer). - Update dossier mapping and regenerate emitted assembly + error-code documentation for the new tests.
- Refresh coverage/plan/docs counts and changelog notes to reflect the added 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/cpu.rs | Adds the two new CPU tests and wires them into the generated test list. |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Maps new cart IDs (A1.07, A9.04) to the intended dossier rows (A1.09, A9.03). |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Regenerated assembly output including new test bodies and updated test tables/counts. |
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Regenerated catalog entries (including insertion of the two new tests). |
| tests/roms/AccuracySNES/ERROR_CODES.md | Regenerated error-code documentation for the new failure codes/messages. |
| docs/accuracysnes-plan.md | Updates total test counts to match the expanded suite. |
| docs/accuracysnes-coverage.md | Updates coverage totals/uncovered lists to reflect new dossier-row coverage. |
| CHANGELOG.md | Documents the newly added AccuracySNES assertions and their motivation. |
…g the wrong byte Checking Z alone was satisfied by a core that wrongly updates N: S is $1FFF, whose bit 15 is clear, so N would read the same before and after either way. N is now planted with SEP #$80 — which writes P directly and touches no register — so both flags sit at the value a flag-setting transfer would have to change. And A9.03's bank-byte diagnostic asserted what the failing value would be rather than what the right one is. What the core actually reads on failure depends on what it used for the bank, so the message now names the expectation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two 65C816 assertions, both from rows that had sat uncovered because the obvious test for each is
the one that does not work.
A1.09—TCSandTXSset no flagsThe stack pointer is not data, so moving a value into it does not describe that value. A core that
routes every transfer through one flag-setting helper gets this wrong, and nothing crashes: it
changes which branch is taken after a
TXS, which produces one wrong branch a frame in code nobodysuspects.
The difficulty is that the obvious test — transfer a value whose flags differ from the current ones
— means writing that value to
S. In native modeTXSis a full 16-bit write, so the testwould then be pushing its own
PHPinto ROM. Both instructions are therefore handed the value thestack pointer already holds (
TSC/TSXfirst), soSis written with what was in it and thequestion is still asked. The flag that has to survive is planted with
BIT #imm, which affectsZalone and so does not disturb the accumulator the transfer is about to move.
TXAmoving$8000is the control. Without it, a core that sets no transfer flags at all passesboth assertions.
A9.03—ORA [d]reaches through a 24-bit pointerDirect-page indirect long is the mode most likely to be implemented as its 16-bit sibling with the
data bank glued on, because for a pointer that happens to live in bank
$00the two are identical.This image is 128 KiB precisely so they are not — each bank carries its own signature at
$xx:8005— so the same pointer with$01in its third byte reads$A1where a(d)-style fetchreads
$A0.Three parts, and the third is load-bearing rather than decorative: parts 1 and 2 both start from
A = $00, whereORAandLDAare indistinguishable, so a reading with$0Falready in theaccumulator is what makes this an assertion about the operation the row names.
Both are new cart IDs (
A1.07,A9.04) mapped to those dossier rows — cartA9.03already existsand is the unenumerated open-bus R-M-W golden, which is exactly the ID confusion
dossier.rsisthere to prevent.
Validation
cargo fmt --checkclean; clippy clean (the only error is the pre-existingndk-sys"Android only"
compile_error!, unrelated).divergences; 50/50 scenes unchanged.
Coverage moves 235 → 237 of 443.
Adds AccuracySNES coverage for dossier assertions A1.09 and A9.03. It claims that
TCS/TXSdo not modify processor flags (specifically preserving N and Z), and thatORA [d]uses the full 24-bit direct-page indirect-long pointer (including its bank byte) and performs an OR merge into the accumulator rather than replacing it.The claim would be false if
TCS/TXSchange N/Z (e.g., derive flags from the transferred value), or ifORA [d]instead ignores the pointer’s bank byte, performs only a 16-bit indirect read, or overwrites/replaces the accumulator instead of OR-ing into it.Coverage increases from 235 to 237 of 443; the denominator (443) is unchanged.