Repository navigation
feat(accuracysnes): emulation mode uses its own vector table (A6.02) - #189
Conversation
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>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 7 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 selected for processing (1)
WalkthroughIn emulation mode, ChangesEmulation COP vector routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CPU
participant EmulationVectorTable
participant cop_trampoline_e
participant V_COP_VEC_E
participant COPHandler
CPU->>EmulationVectorTable: Execute COP with E=1
EmulationVectorTable->>cop_trampoline_e: Dispatch through $FFF4
cop_trampoline_e->>V_COP_VEC_E: Load emulation handler address
V_COP_VEC_E->>COPHandler: Jump to installed handler or irq_stub
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds an AccuracySNES CPU regression test for dossier A6.02 by making emulation-mode COP ($FFF4) distinguishable from native-mode COP ($FFE4) via a dedicated emulation trampoline + RAM vector pointer, and introducing a scored cart test (A6.10) that fails if the core fetches the wrong vector table.
Changes:
- Add a dedicated emulation-mode
COPtrampoline (cop_trampoline_e) plus a new RAM pointer (V_COP_VEC_E) and wire$FFF4to it. - Add scored test A6.10 (mapped to dossier A6.02) that installs distinct emulation/native handlers and asserts emulation
COPvectors through$FFF4. - Regenerate AccuracySNES catalogs/docs (source catalog, error codes, coverage/plan counts) and update the changelog entry.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/roms/AccuracySNES/SOURCE_CATALOG.tsv | Adds the new A6.10 test entry to the generated catalog. |
| tests/roms/AccuracySNES/gen/src/tests/cpu.rs | Implements generator-side definition of the new A6.10 CPU test. |
| tests/roms/AccuracySNES/gen/src/dossier.rs | Maps cart test A6.10 to dossier assertion A6.02. |
| tests/roms/AccuracySNES/ERROR_CODES.md | Documents the new A6.10 failure code meaning. |
| tests/roms/AccuracySNES/asm/tests_group_a.s | Generated assembly for the new A6.10 test, plus updated test tables/counts. |
| tests/roms/AccuracySNES/asm/runtime.s | Initializes the new emulation COP pointer and adds cop_trampoline_e. |
| tests/roms/AccuracySNES/asm/runtime.inc | Defines the new direct-page pointer V_COP_VEC_E. |
| tests/roms/AccuracySNES/asm/header.s | Wires the emulation COP vector ($FFF4) to cop_trampoline_e. |
| docs/accuracysnes-plan.md | Updates the documented test count. |
| docs/accuracysnes-coverage.md | Updates coverage totals to reflect the newly covered dossier row. |
| CHANGELOG.md | Adds release notes describing the new emulation COP vector coverage. |
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>
![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 `@docs/accuracysnes-plan.md`:
- Line 16: Update the Tests count in accuracysnes-plan.md from 234 to 240,
matching the covered assertion total reported by accuracysnes-coverage.md while
preserving the existing breakdown text unless it must be adjusted for
consistency.
🪄 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: 094ec41e-6c10-4e4a-a9e5-e6bd60fc7819
⛔ 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 (7)
CHANGELOG.mddocs/accuracysnes-plan.mdtests/roms/AccuracySNES/asm/header.stests/roms/AccuracySNES/asm/runtime.inctests/roms/AccuracySNES/asm/runtime.stests/roms/AccuracySNES/gen/src/dossier.rstests/roms/AccuracySNES/gen/src/tests/cpu.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (15)
tests/roms/AccuracySNES/asm/**/*.s
📄 CodeRabbit inference engine (AGENTS.md)
tests/roms/AccuracySNES/asm/**/*.s: Never hand-edit generatedasm/tests_group_a.sorasm/scenes.s; regenerate them with the AccuracySNES generator.
Use assertion helpers for every AccuracySNES verdict, including non-equality conditions; never hand-write asta V_TEST_RESULTverdict byte.
Ensure every scene starts with a rebuilt canvas and a rerun ofinit_registers; verify that each scene visibly renders the state it claims to arrange.
Every SPC700 test program must callrelease_to_ipl, remap the IPL ROM before jumping to$FFC0, and bound every APU handshake wait; tests that receive no answer must report SKIP.
Files:
tests/roms/AccuracySNES/asm/header.stests/roms/AccuracySNES/asm/runtime.s
**/*.{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/asm/header.stests/roms/AccuracySNES/asm/runtime.stests/roms/AccuracySNES/gen/src/tests/cpu.rstests/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 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/asm/header.sdocs/accuracysnes-plan.mdtests/roms/AccuracySNES/asm/runtime.inctests/roms/AccuracySNES/asm/runtime.sCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/cpu.rstests/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/, 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/asm/header.stests/roms/AccuracySNES/asm/runtime.inctests/roms/AccuracySNES/asm/runtime.stests/roms/AccuracySNES/gen/src/tests/cpu.rstests/roms/AccuracySNES/gen/src/dossier.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
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Never commit commercial ROMs; only commit derived screenshots or hashes.
Files:
docs/accuracysnes-plan.mdCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/cpu.rstests/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>.mddocumentation.
Files:
docs/accuracysnes-plan.mdCHANGELOG.mdtests/roms/AccuracySNES/gen/src/tests/cpu.rstests/roms/AccuracySNES/gen/src/dossier.rs
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
tests/roms/AccuracySNES/asm/runtime.inc
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/asm/runtime.inc: Shared symbol definitions. Every address here is claimed by something; a new variable that
overlaps an existing block, or a block whose stated width is smaller than what the code
stores into it, is the failure mode. Check the arithmetic ofVAR_BASE + $nnoffsets
against their declared sizes and against the neighbouring definitions.
Files:
tests/roms/AccuracySNES/asm/runtime.inc
tests/roms/AccuracySNES/asm/runtime.s
⚙️ CodeRabbit configuration file
tests/roms/AccuracySNES/asm/runtime.s: Hand-written 65816. The battery runs under forced blank throughout; anything that lifts it
must put it back. Helpers are width-neutral (php/plp) by convention — flag one that is
not, because ca65's.a8/.a16state is file-global and a mismatch emits silently wrong
instruction lengths.
Files:
tests/roms/AccuracySNES/asm/runtime.s
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 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/tests/cpu.rstests/roms/AccuracySNES/gen/src/dossier.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/tests/cpu.rstests/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 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/cpu.rstests/roms/AccuracySNES/gen/src/dossier.rs
🔇 Additional comments (6)
tests/roms/AccuracySNES/asm/runtime.inc (1)
166-174: LGTM!tests/roms/AccuracySNES/asm/runtime.s (1)
81-82: LGTM!Also applies to: 1537-1550
tests/roms/AccuracySNES/asm/header.s (1)
14-14: LGTM!Also applies to: 55-60
tests/roms/AccuracySNES/gen/src/tests/cpu.rs (1)
83-83: LGTM!Also applies to: 2303-2376
tests/roms/AccuracySNES/gen/src/dossier.rs (1)
169-169: LGTM!CHANGELOG.md (1)
14-28: LGTM!
A6.02— emulation mode has its own vector table.COPgoes through$FFF4, sixteen bytes fromthe native
$FFE4. A core that keeps one set of vectors, or that picks the table from somethingother than the E flag, lands in the wrong handler; nothing about that is visible in ordinary code,
since a game's
COPhandler 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 test is only possible because the runtime changed: the
emulation
COPvector now has its own trampoline and its own RAM pointer, initialised toirq_stubat reset so a
COPin a test that installed no handler returns harmlessly.Both handlers are installed and live, so taking the wrong table is a wrong answer rather than a
hang — the failure code distinguishes "took
$FFE4" ($B0) from "reached neither" ($00).BRKis deliberately not splitIn emulation,
$FFFEis shared betweenIRQandBRK— the hardware conflates them — so a RAMpointer behind it could not mean "the BRK handler" unambiguously, and splitting it would invent a
distinction the machine does not have.
That argument got made concrete the hard way: the first version split
$FFFEtoo, andA6.06andA6.09immediately failed. Both install throughV_BRK_VECand both run in emulation, so they weresuddenly vectoring past their own handlers. Reverting the
BRKhalf fixed them and is also theright model.
The other thing this cost, worth recording
The first draft reported neither handler. The handlers run in emulation mode, but ca65 still had
the test body's
.a16in force where they were emitted, solda #$E0assembled as three bytes, theCPU took two, and the third (
$00) executed as aBRK. The handlers now carry explicit.a8/.i8and the body restores
.a16/.i16— this is the file-global assembler-state trap the repository'sown review instructions warn about, and it produces a silently wrong instruction stream rather than
an error.
Validation
cargo fmt --checkclean; clippy clean apart from the pre-existingndk-sysAndroid-onlycompile_error!.A6.06andA6.09re-confirmedpassing after the
BRKrevert.divergences; 50/50 scenes unchanged.
Coverage moves 239 → 240 of 443.
In emulation mode,
COPnow vectors through$FFF4, independently of the native-mode$FFE4vector;BRK/IRQremain shared at$FFFE. The AccuracySNES harness adds dossier assertion A6.02 via cart test A6.10, increasing coverage from 239/443 to 240/443.The claim would be false if an emulation-mode
COPreached the native handler, neither handler, or incorrectly split the sharedBRK/IRQbehavior.