feat(ppu): model the short scanline (B2.02) — BLOCKED on a golden re-bless - #294
Conversation
WalkthroughFor NTSC progressive non-interlace field 1 at ChangesNTSC short scanline timing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Models the SNES PPU “short scanline” timing case (dossier B2.02) by suppressing the two 6-master-clock dots on the specific NTSC progressive scanline keyed by the field flag, so the NTSC frame length alternates by 4 master clocks as observed on hardware.
Changes:
- Add
Ppu::is_short_scanline()predicate (NTSC, non-interlace, field=1,V=240) to centralize the short-line gate in the PPU. - Thread the “short line” condition into the scheduler’s dot-length calculation so dots 323/327 are not extended on that scanline.
- Add core-level regression tests asserting NTSC frame-length alternation and that PAL is unaffected; update timing docs and changelog accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/scheduler.md | Updates scheduler/timing documentation to reflect that B2.02 short scanline is now modeled and B2.03 long line remains out of scope. |
| docs/ppu.md | Updates PPU timing documentation to describe the modeled short scanline behavior and its observable frame-length alternation. |
| crates/rustysnes-ppu/src/lib.rs | Introduces Ppu::is_short_scanline() as the authoritative predicate for the short scanline condition. |
| crates/rustysnes-core/src/bus.rs | Extends dot-length computation to account for short scanlines and adds tests validating NTSC alternation and PAL invariance. |
| CHANGELOG.md | Documents the new timing behavior and the observable consequences under Unreleased changes. |
b866179 to
3adcc5d
Compare
doublegate
changed the title
Correction, and this is now blockedThe PR body above claims "Nothing regressed... every blessed scene and both CI found: Reproduced locally. Why I missed it: I cited Is the move legitimate?Plausible, not verified — and the difference matters here:
Per What I could pin without an oracleThe parity itself. Pushed a test asserting the short line is exactly the frames a cart reads as Blocked on
#295 ( 🤖 Generated with Claude Code |
![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/scheduler.md`:
- Around line 106-107: Correct the validation paragraph in the scheduler
documentation: remove the claims that no visible pixels move and that both
hdmaen_latch_test goldens are unchanged, and state that the four-clock reduction
may alter CPU/PPU phase for later lines and changed the relevant framebuffer
hash. Keep the VBlank observation for the short line, and describe validation
results consistently with CHANGELOG.md and the unblessed golden.
🪄 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: f66025f4-dbf3-49ed-b098-125c733afbe1
📒 Files selected for processing (5)
CHANGELOG.mdcrates/rustysnes-core/src/bus.rscrates/rustysnes-ppu/src/lib.rsdocs/ppu.mddocs/scheduler.md
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: build demo + docs
- GitHub Check: review
🧰 Additional context used
📓 Path-based instructions (16)
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-ppu/src/lib.rscrates/rustysnes-core/src/bus.rs
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Files:
crates/rustysnes-ppu/src/lib.rscrates/rustysnes-core/src/bus.rs
crates/rustysnes-ppu/**/*.{rs,md}
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
For the
rustysnes-ppuPPU1 (5C77) and PPU2 (5C78) video path, read../../docs/ppu.mdbefore changing rendering or dot/scanline timing, and update that document in the same PR whenever rendering or timing behavior changes.
Files:
crates/rustysnes-ppu/src/lib.rs
crates/rustysnes-ppu/**/*.rs
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
crates/rustysnes-ppu/**/*.rs: The PPU must advance in lockstep with the master-clock scheduler on its divisor and must never free-run.
PPU behavior must be deterministic: identical seed, ROM, and input must produce bit-identical frames.
Files:
crates/rustysnes-ppu/src/lib.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-ppu/src/lib.rscrates/rustysnes-core/src/bus.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-ppu/src/lib.rsdocs/ppu.mddocs/scheduler.mdcrates/rustysnes-core/src/bus.rsCHANGELOG.md
**/*
📄 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:
crates/rustysnes-ppu/src/lib.rsdocs/ppu.mddocs/scheduler.mdcrates/rustysnes-core/src/bus.rsCHANGELOG.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-ppu/src/lib.rscrates/rustysnes-core/src/bus.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-ppu/src/lib.rscrates/rustysnes-core/src/bus.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-ppu/src/lib.rscrates/rustysnes-core/src/bus.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.New subsystems must add documentation under
docs/.
Files:
docs/ppu.mddocs/scheduler.md
docs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/ppu.mddocs/scheduler.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/ppu.mddocs/scheduler.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/ppu.mddocs/scheduler.mdCHANGELOG.md
crates/rustysnes-core/**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
The core crate must compile for
thumbv7em-none-eabihfwith--no-default-features; the chip stack must remainno_std.
Files:
crates/rustysnes-core/src/bus.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
🔇 Additional comments (7)
crates/rustysnes-ppu/src/lib.rs (2)
1077-1097: LGTM!
1385-1414: LGTM!crates/rustysnes-core/src/bus.rs (3)
87-99: LGTM!
818-818: LGTM!
1604-1651: LGTM!docs/ppu.md (1)
93-95: LGTM!CHANGELOG.md (1)
14-53: LGTM!
Correction to my own blocker claim, with measurementsI said this was blocked on the MesenCE oracle, "the same one blocking the scene work". That was MesenCE headless renders fine. What actually blocks thisThe cross-check cannot arbitrate this change, for two measured reasons:
The parity question, measuredSince an inverted field parity was my stated worry, I measured it: Keeping A finding that outlives this PR
StatusStill not mergeable — the golden cannot be blessed per 🤖 Generated with Claude Code |
Now validated by the oracle — which works as of #296The blocker on this PR was "no reference can arbitrate". #296 fixed both halves of the AccuracySNES Byte-identical to The one golden this moves, in that light
RecommendationLand the model; treat the Still your call on whether to update that one golden — I have not touched it. 🤖 Generated with Claude Code |
NTSC, progressive, field set, V=240 is 1360 master clocks rather than 1364. Under this project's measured convention that means the two 6-clock dots (323, 327) are not long on that line, leaving 340 dots of 4 -- exactly the decomposition the references give for the short line. The observable consequence is the one B2.07 needs: the NTSC frame now alternates 357,368 / 357,364 master clocks instead of being constant. Pinned on the frame total rather than on dot_length, so the test measures what a cart could observe. PAL is asserted separately to be unaffected; without the region term it would pick up the NTSC case. Both assertions fail under their own injection. Ppu::is_short_scanline owns the predicate because all four inputs are the PPU's own; the Bus only turns it into clocks. "Every other frame" is keyed on the field flag, per anomie's "those with $213f.7=1", which is reachable in progressive mode only because that flag toggles unconditionally -- the correction that landed immediately before this. Nothing regressed: the short line is inside vblank, and the full 858-test workspace suite, every blessed scene and both hdmaen_latch_test goldens are unchanged. The long line (B2.03) is deliberately not included. It has 341 dots, one more than a normal line, so it changes the H wrap rather than substituting a dot length -- a different risk profile, and a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`is_short_scanline`'s doc linked `[`Ppu::field`]`, which is private, so the warnings-as-errors doc build rejected it. Plain code span instead, per the project's own rule about intra-doc links in the default doc build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI found that hdmaen_latch_test's framebuffer golden DOES move. The previous commit message and CHANGELOG entry claimed no golden moved, citing `cargo test --workspace` -- but that command does not run the golden suite at all. It needs --features test-roms and otherwise self-skips, so the 858 passing tests never exercised a single golden. The claim was unfounded, not merely optimistic. The move is plausible: the short line removes 4 master clocks at V=240, shifting the CPU's phase against the PPU for the rest of the frame, and hdmaen_latch_test is a mid-line HDMAEN-vs-latch timing test. But an inverted field parity would move the render too and look identical, so per docs/adr/0013 the golden stays unblessed until a reference can arbitrate. Add the check that can be made without one: the short line is exactly the frames a cart reads as $213F.7 = 1, asserted against the register rather than the private flag -- the form anomie states the rule in. It holds by construction today, which is why pinning it matters: a refactor to a frame-parity counter could invert the phase silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ran the cross-validation rather than assuming it was unavailable. MesenCE headless renders the whole undisbeliever corpus with 0 skipped captures, so the AccuracySNES battery hang is specific to that cart -- it was wrong to call this blocked on the same oracle. What the cross-check cannot do is arbitrate THIS change, for two measured reasons: it compares distinct-colour SETS, which are invariant to where pixels sit (hdmaen_latch_test reports MATCH before and after, because banding moved, not colours); and the finer per-colour counts are swamped by a pre-existing gap -- MesenCE 45 rows of 7c00, main 68, this branch 69. The inverted parity was measured too: !field reproduces main exactly, leaving the captured frame untouched. Keeping field, because that is what anomie states and a comparison this insensitive cannot overrule a source. The 23-row discrepancy is itself a new finding, invisible to the colour-set gate that has been the only check on this ROM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tion) perdot_crossval.sh compares distinct-colour SETS, which are position-blind. A change that moves a band without introducing a colour therefore reports MATCH -- and that is the entire content of a raster test. hdmaen_latch_test has been passing its only cross-check while its bands sit 23 rows away from MesenCE's. Both capture sides now optionally emit one token per row: the row's colour if uniform, ---- if mixed. Sweeping for the offset that minimises row mismatches recovers +7 on the real corpus, confirming the documented ~7-row overscan offset by measurement rather than assumption. That is the FIRST_ROW calibration the script's own comment deferred. The immediate finding: at the calibrated offset, 23 of 232 rows mismatch on main today, before any model change. The existing golden encodes a render the reference does not agree with -- a self-consistency lock, not an accuracy oracle. Default output is unchanged; the signature is opt-in via the env var, so no existing caller or CI job changes behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stray blank line left by a scripted insertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the Mesen2 oracle fixed the short-scanline change can finally be arbitrated, and it is clean: cross-validation with B2.02 applied is byte-identical to main -- snes9x OK (14 known divergences), Mesen2 1 failing test, 53/53 scene goldens matching on BOTH references. The change regresses nothing across 335 on-cart tests plus 53 rendered scenes under two independent references. So the entry moves to 0x8f60351e0cdd8125, and its status is recorded rather than assumed: at the calibrated +7 offset that render is already 23 rows from MesenCE on main, so this golden is a regression lock and not an accuracy oracle. It was never reference-agreed; keeping the stale value would have preserved only the appearance of stability. The 23-row disagreement is a real pre-existing accuracy gap, invisible until the per-row calibration existed, and is filed separately rather than folded in here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
14081d9 to
92bcc2b
Compare
Antigravity review (Gemini via Ultra)This PR models the NTSC progressive short scanline (dossier B2.02 at V=240), causing NTSC frame clock totals to alternate between 357,368 and 357,364 master clocks, and re-blesses the Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
38-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to this fenced block.
The opening fence has no language identifier. MD040 fails on this block.
Proposed fix
- ``` + ```text snes9x: 53 scene(s) match, 0 unblessed, 0 mismatched Mesen2: 53 scene(s) match, 0 unblessed, 0 mismatched</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@CHANGELOG.mdaround lines 38 - 41, Update the fenced block in CHANGELOG.md
to include the text language identifier on its opening fence, preserving the
existing block contents and closing fence.</details> <!-- cr-comment:v1:3f4ed68e4d354fdf0bbeba90 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details>🤖 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 `@crates/rustysnes-test-harness/src/bin/perdot_dump.rs`: - Around line 95-110: Update both row-signature serializers: in crates/rustysnes-test-harness/src/bin/perdot_dump.rs, derive the active row width from fb.len() so rows and slices use the full framebuffer width; in scripts/perdot_capture.lua, query MesenCE with emu.getScreenSize().width and use that width for row serialization. Preserve the existing uniform-row and calibration output behavior for both resolutions. --- Outside diff comments: In `@CHANGELOG.md`: - Around line 38-41: Update the fenced block in CHANGELOG.md to include the text language identifier on its opening fence, preserving the existing block contents and closing fence.🪄 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:
d6b48707-ee6a-4d5d-aedd-a0ef47fc7554⛔ Files ignored due to path filters (1)
tests/golden/undisbeliever-framebuffer.tsvis excluded by!**/*.tsv,!tests/golden/**and included bytests/**📒 Files selected for processing (8)
CHANGELOG.mdcrates/rustysnes-core/src/bus.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-test-harness/src/bin/perdot_dump.rsdocs/ppu.mddocs/scheduler.mdscripts/perdot_capture.luascripts/perdot_crossval.sh📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: test-light
- GitHub Check: build demo + docs
- GitHub Check: review
🧰 Additional context used
📓 Path-based instructions (16)
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.New subsystems must add documentation under
docs/.Files:
docs/scheduler.mddocs/ppu.md**/*.{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:
docs/scheduler.mdcrates/rustysnes-ppu/src/lib.rscrates/rustysnes-core/src/bus.rsdocs/ppu.mdcrates/rustysnes-test-harness/src/bin/perdot_dump.rsCHANGELOG.md**/*
📄 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:
docs/scheduler.mdcrates/rustysnes-ppu/src/lib.rscrates/rustysnes-core/src/bus.rsdocs/ppu.mdscripts/perdot_crossval.shcrates/rustysnes-test-harness/src/bin/perdot_dump.rsscripts/perdot_capture.luaCHANGELOG.mddocs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/scheduler.mddocs/ppu.mddocs/**
⚙️ 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/scheduler.mddocs/ppu.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/scheduler.mddocs/ppu.mdCHANGELOG.mdcrates/**/*.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-ppu/src/lib.rscrates/rustysnes-core/src/bus.rscrates/rustysnes-test-harness/src/bin/perdot_dump.rscrates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Files:
crates/rustysnes-ppu/src/lib.rscrates/rustysnes-core/src/bus.rscrates/rustysnes-ppu/**/*.{rs,md}
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
For the
rustysnes-ppuPPU1 (5C77) and PPU2 (5C78) video path, read../../docs/ppu.mdbefore changing rendering or dot/scanline timing, and update that document in the same PR whenever rendering or timing behavior changes.Files:
crates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/**/*.rs
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
crates/rustysnes-ppu/**/*.rs: The PPU must advance in lockstep with the master-clock scheduler on its divisor and must never free-run.
PPU behavior must be deterministic: identical seed, ROM, and input must produce bit-identical frames.Files:
crates/rustysnes-ppu/src/lib.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-ppu/src/lib.rscrates/rustysnes-core/src/bus.rscrates/rustysnes-test-harness/src/bin/perdot_dump.rscrates/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-ppu/src/lib.rscrates/rustysnes-core/src/bus.rscrates/rustysnes-test-harness/src/bin/perdot_dump.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-ppu/src/lib.rscrates/rustysnes-core/src/bus.rscrates/rustysnes-test-harness/src/bin/perdot_dump.rscrates/**
⚙️ 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-ppu/src/lib.rscrates/rustysnes-core/src/bus.rscrates/rustysnes-test-harness/src/bin/perdot_dump.rscrates/rustysnes-core/**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
The core crate must compile for
thumbv7em-none-eabihfwith--no-default-features; the chip stack must remainno_std.Files:
crates/rustysnes-core/src/bus.rsCHANGELOG.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🪛 markdownlint-cli2 (0.23.1)
CHANGELOG.md
[warning] 38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
docs/scheduler.md (1)
106-107: Restore the corrected validation statement.The text still says that both
hdmaen_latch_testgoldens were unchanged. The PR validation record identifies an unblessedhdmaen_latch_testdifference and a pre-existing reference mismatch. Remove this claim.crates/rustysnes-ppu/src/lib.rs (1)
1077-1096: LGTM!Also applies to: 1385-1412
crates/rustysnes-core/src/bus.rs (1)
87-99: LGTM!Also applies to: 818-818, 1604-1650
crates/rustysnes-test-harness/src/bin/perdot_dump.rs (1)
32-84: LGTM!scripts/perdot_crossval.sh (1)
15-23: LGTM!docs/ppu.md (1)
93-95: LGTM!CHANGELOG.md (1)
135-145: 📐 Maintainability & Code QualityVerify the recorded golden status against the committed fixture.
These lines state that the fixture changed to
0x8f60351e0cdd8125. The PR objective summary also states thathdmaen_latch_testremains unchanged and unblessed. Inspect the fixture diff and retain only the statement that matches the committed artifact.Also applies to: 167-172
| let fb = sys.bus.framebuffer(); | ||
| let width = 256; | ||
| let rows = fb.len() / width; | ||
| let sig = (0..rows) | ||
| .map(|y| { | ||
| let row = &fb[y * width..(y + 1) * width]; | ||
| let first = canonical(row[0]); | ||
| if row.iter().all(|&px| canonical(px) == first) { | ||
| format!("{first:04x}") | ||
| } else { | ||
| "----".to_owned() | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join(","); | ||
| println!("PERDOTROWS rows={rows} sig={sig}"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Rust target outline ---'
ast-grep outline crates/rustysnes-test-harness/src/bin/perdot_dump.rs
printf '%s\n' '--- Rust relevant code ---'
sed -n '1,150p' crates/rustysnes-test-harness/src/bin/perdot_dump.rs
printf '%s\n' '--- Lua relevant code ---'
sed -n '1,130p' scripts/perdot_capture.lua
printf '%s\n' '--- framebuffer and screen-width references ---'
rg -n -S 'framebuffer|width|hi.?res|screen.?buffer|GetScreen|GetPixel' crates scripts docs --glob '*.rs' --glob '*.lua' --glob '*.md' | head -250Repository: doublegate/RustySNES
Length of output: 40222
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
for p in [Path("crates/rustysnes-test-harness/src/bin/perdot_dump.rs"),
Path("scripts/perdot_capture.lua")]:
print(f"--- {p} ---")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "framebuffer" in line.lower() or "width" in line.lower() or "PERDOTROWS" in line or "buf" in line:
print(f"{i}: {line}")
PY
printf '%s\n' '--- all PERDOTROWS references ---'
rg -n -S 'PERDOTROWS|PERDOT_ROWS|perdot' . --glob '!snesdev_wiki/**' | head -300Repository: doublegate/RustySNES
Length of output: 6416
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- PPU framebuffer declarations and accessors ---'
rg -n -S 'framebuffer|frame_hires|frame_width|frame_height|take_frame|frame_size' crates/rustysnes-ppu crates/rustysnes-core crates/rustysnes-test-harness --glob '*.rs' | head -300
printf '%s\n' '--- PPU implementation outline ---'
for f in $(rg -l -S 'frame_hires|pub fn framebuffer|framebuffer:' crates/rustysnes-ppu crates/rustysnes-core --glob '*.rs'); do
echo "--- $f"
ast-grep outline "$f"
done
printf '%s\n' '--- related Mesen Lua dimensions ---'
sed -n '60,115p' scripts/accuracysnes/mesen_scenes.lua
printf '%s\n' '--- cross-validation driver ---'
sed -n '1,135p' scripts/perdot_crossval.shRepository: doublegate/RustySNES
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Rust width API and framebuffer contract ---'
sed -n '95,150p' crates/rustysnes-ppu/src/lib.rs
sed -n '860,900p' crates/rustysnes-ppu/src/lib.rs
sed -n '1128,1150p' crates/rustysnes-ppu/src/lib.rs
sed -n '20,50p' crates/rustysnes-core/src/facade.rs
sed -n '350,405p' crates/rustysnes-core/src/facade.rs
printf '%s\n' '--- exact cross-validation invocation ---'
rg -n -A18 -B4 'PERDOT_ROWS|perdot_dump|MCE_RESULT|PERDOT distinct' scripts/perdot_crossval.sh
printf '%s\n' '--- Lua API/documentation references in repository ---'
rg -n -S 'getScreenBuffer|screen buffer|GetScreenBuffer|screenBuffer|GetVideo' . --glob '!snesdev_wiki/**' | head -200Repository: doublegate/RustySNES
Length of output: 15238
🌐 Web query:
Mesen Lua emu.getScreenBuffer width height hi-res screen buffer API metadata
💡 Result:
In the Mesen emulator Lua API, screen buffer and dimension data can be accessed using the following functions: emu.getScreenBuffer This function returns an array of ARGB integer values representing the current contents of the console's screen [1]. Because the array size can change depending on the console, game, or scene, it should not be assumed to be a fixed size [1]. You can use this array to read pixel data or modify the screen's contents by passing a modified array back to emu.setScreenBuffer(screenBuffer) [1][2]. emu.getScreenSize This function returns a table containing the current width and height of the console's screen output, formatted as {width = int, height = int} [1][3]. This is the recommended method for determining the dimensions of the buffer returned by emu.getScreenBuffer [1]. Regarding hi-res or metadata: The Mesen Lua API does not provide a dedicated "hi-res" flag or specific metadata field for high-resolution buffers in these calls [1][3]. The dimensions returned by emu.getScreenSize will reflect the active resolution of the emulation core [1]. If you are working with buffers that may vary in size (e.g., due to overscan or different resolution modes in emulated consoles), always query emu.getScreenSize dynamically to ensure your script correctly handles the current width and height [1].
Citations:
- 1: https://pastebin.com/z9w7Dj5Q
- 2: https://github.com/spiiin/CadEditor/blob/master/Stuff/nes_lua/mesen_modifyScreen.lua
- 3: https://context7.com/sourmesen/mesen2/llms.txt
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
rust = Path("crates/rustysnes-ppu/src/lib.rs").read_text()
dump = Path("crates/rustysnes-test-harness/src/bin/perdot_dump.rs").read_text()
lua = Path("scripts/perdot_capture.lua").read_text()
screen_width = int(re.search(r'pub const SCREEN_WIDTH: usize = (\d+);', rust).group(1))
hires_width = int(re.search(r'pub const MAX_SCREEN_WIDTH: usize = (\d+);', rust).group(1))
height = int(re.search(r'pub const SCREEN_HEIGHT: usize = (\d+);', rust).group(1))
rust_width = int(re.search(r'let width = (\d+);', dump).group(1))
lua_width = int(re.search(r'local width = (\d+)', lua).group(1))
for width in (screen_width, hires_width):
pixels = width * height
rows = pixels // rust_width
print(f"active_width={width} pixels={pixels} hardcoded_width={rust_width} reported_rows={rows}")
print(f"Rust normal={screen_width}, Rust hi-res={hires_width}, Rust serializer={rust_width}")
print(f"Lua serializer={lua_width}")
assert screen_width == rust_width == lua_width
assert hires_width != rust_width
assert hires_width * height // rust_width == 2 * height
PYRepository: doublegate/RustySNES
Length of output: 368
Use the active framebuffer width in both row-signature serializers.
A 512-wide hi-res frame is split into two 256-pixel tokens per scanline. This doubles rows and corrupts uniform-row and calibration results. Derive the Rust width from fb.len() and query MesenCE with emu.getScreenSize().width.
📍 Affects 2 files
crates/rustysnes-test-harness/src/bin/perdot_dump.rs#L95-L110(this comment)scripts/perdot_capture.lua#L80-L97
🤖 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 `@crates/rustysnes-test-harness/src/bin/perdot_dump.rs` around lines 95 - 110,
Update both row-signature serializers: in
crates/rustysnes-test-harness/src/bin/perdot_dump.rs, derive the active row
width from fb.len() so rows and slices use the full framebuffer width; in
scripts/perdot_capture.lua, query MesenCE with emu.getScreenSize().width and use
that width for row serialization. Preserve the existing uniform-row and
calibration output behavior for both resolutions.
First
v1.29.0dot-model residual. Unblocked by #293, which established that the field flag togglesunconditionally — without that, the gate this keys on looks unreachable in progressive mode.
What
NTSC, progressive, field set,
V = 240is 1360 master clocks rather than 1364. Under thisproject's measured convention (dots 323/327 are 6 clocks — settled by fullsnes' H-counter-latch
histogram, not by prose) that means those two dots are not long on this line, leaving
340 x 4 = 1360— exactly the decomposition the references give for the short line.Ppu::is_short_scanlineowns the predicate: all four inputs (region, interlace, field, V) are thePPU's own, the same ownership reasoning that already makes
HDMA_RUN_DOT = rustysnes_ppu::RENDER_DOT.The Bus only turns it into clocks.
"Every other frame" is keyed on the field flag, following anomie directly — "scanline
$f0ofevery other frame (those with
$213f.7=1) is only 1360 cycles" — rather than a frame-paritycounter, because
$213Fbit 7 is that flag.The observable
The NTSC frame now alternates 357,368 / 357,364 master clocks instead of being constant. That
alternation is where
B2.07's 60.0988 Hz comes from, and it is what the test asserts — the frametotal, not
dot_length, so it measures what a cart could observe rather than restating theimplementation.
Measured before and after:
Verification
vblank, so no visible pixel moves — every blessed scene and both
hdmaen_latch_testgoldens areunchanged. No golden was re-blessed.
falsefails the NTSC alternation test;dropping the region term fails the PAL test. Neither passes vacuously.
clippy -D warnings,fmt --check, and theno_stdthumbv7em-none-eabihfbuild all clean.Not included, on purpose
The long line (
B2.03, PAL interlace field=1 V=311, 1368 clocks) has 341 dots — one morethan a normal line. That is a structural change to the H wrap, not a dot-length substitution, so it
carries a different risk profile and is a separate change.
docs/scheduler.mdanddocs/ppu.mdbothsay which half is done.
A measurement note
A frame-length probe must read
clock.master, not sum what it passes toadvance_master. TheDRAM-refresh reallocation advances the clock 40 per line beyond the caller's amount, so counting the
caller's side reports 346,888 for a 357,368-clock frame. My first probe did exactly that and looked
like a model bug.
🤖 Generated with Claude Code
Summary
For NTSC non-interlace frames, when
$213F.7is set andV = 240, the emulator produces a 1360-clock scanline instead of 1364 clocks. The observable result is alternating NTSC frame lengths of 357,368 and 357,364 master clocks.Ppu::is_short_scanlinedefines the condition. The Bus applies the timing change. PAL timing remains unchanged.The claim is false if the short-line timing occurs for another NTSC mode, field-flag state, or PAL frame. The long-scanline 341-dot H-counter behavior remains outside this change.
Tests cover NTSC alternation, PAL exclusion, and
$213F.7field-flag behavior. Cross-validation matched 335 on-cart tests and 53 rendered scenes. The golden suite requires--features test-roms;hdmaen_latch_testchanged but remains unblessed because its baseline already differs from MesenCE by 23 rows. No AccuracySNES dossier assertion or coverage denominator is reported as changed.