Skip to content

feat(ppu): model the long scanline (B2.03) - #297

Merged
doublegate merged 3 commits into
mainfrom
b2-03-replay
Aug 1, 2026
Merged

feat(ppu): model the long scanline (B2.03)#297
doublegate merged 3 commits into
mainfrom
b2-03-replay

Conversation

@doublegate

@doublegate doublegate commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Replaces #295, which GitHub auto-closed when #294 merged and deleted its base branch. Same work,
replayed onto main so only the B2.03 commits are present (#294's are already in main as a
squash). Code files applied cleanly; only CHANGELOG.md needed a resolve, verified additive.

The distinction that made this a separate PR from #294

B2.02 short (merged) B2.03 long (this)
when NTSC, progressive, field, V=240 PAL, interlace, field, V=311
clocks / dots 1360 / 340 1368 / 341
mechanism the two 6-clock dots are not long an extra 4-clock dot is appended, long dots unchanged
touches the Bus's clock table the PPU's H wrap

339 x 4 + 2 x 6 = 1368. Because the long line adds a dot rather than retiming one, it needed a
new Ppu::dots_this_line, and the H counter reaches dot 340 on that line and on no other.

A latent bug this surfaced

check_hv_irq bounded the horizontal match with h_target < DOTS_PER_LINE, a constant. It now uses
the per-line count, so an HTIME whose match lands on dot 340 is a genuine match on the long line
and stays suppressed elsewhere. Before this it could never match anywhere — the long line's own dot
was unreachable by an IRQ.

Verification

  • PAL interlaced frames alternate 425,568 / 425,572.
  • Interlace is enabled through SETINI $2133 bit 0, not by poking the field, so the test drives the
    path a game drives.
  • It asserts the set of frame lengths, not their order: which phase carries the extra 4 clocks
    depends on the field's power-on value, and pinning the order would pin an initial condition this
    row does not care about.
  • Progressive PAL asserted separately to be unaffected — that is what keeps the interlace term honest.
  • Both injection-checked. Forcing dots_this_line to the constant fails the interlaced test;
    dropping the interlace term fails the progressive one.
  • Full workspace suite 873 passed, 0 failed. clippy -D warnings, fmt --check clean.

Stated, not silently skipped

The interlaced frame's extra scanline (263/313 rather than 262/312) is still unmodelled and is a
separate concern — B2.03 is reachable without it because V = 311 is the last PAL line either way.
docs/scheduler.md records it as open.

Also carries the plan update: B2.07 (60.0988 Hz) does not follow from this. Its frame-length
difference is 4 master clocks — one dot — against an instrument that costs a measured 175 dots, which
is the same wall A5.18 is parked on.

🤖 Generated with Claude Code

Summary

This change claims that PAL interlaced field 1 at V=311 uses 341 dots and 1,368 master clocks. The H counter now wraps at dot 340 only for this line. HTIME=340 now matches only on this line.

The claim is false if the extra interlaced frame scanline must also be emulated, or if B2.07 behavior is required. Those cases remain unmodeled.

The observable behavior differs for PAL interlaced timing. Frame lengths alternate between 425,568 and 425,572 master clocks. Progressive PAL frames remain 425,568 master clocks. Horizontal IRQ matching uses the active line length.

The AccuracySNES dossier changes assert that B2.02 and B2.03 are implemented. They also record remaining cart-side measurement work for B2.07 and add the E8.01 differential-test design. The coverage denominator does not move.

doublegate and others added 2 commits July 31, 2026 22:48
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>
Copilot AI review requested due to automatic review settings August 1, 2026 02:51
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PPU now models the 341-dot PAL interlaced scanline at V=311. H-counter wrapping and HV-IRQ matching use the active line length. Bus tests verify alternating interlaced PAL frame lengths and stable progressive PAL timing. Documentation records the model and remaining work.

Changes

PAL long-scanline timing

Layer / File(s) Summary
Dynamic PPU line timing
crates/rustysnes-ppu/src/lib.rs
PAL interlaced scanline 311 uses 341 dots. H-counter wrapping and HV-IRQ matching use the current line length.
PAL timing regression coverage
crates/rustysnes-core/src/bus.rs
Tests verify alternating interlaced PAL frame lengths of 425,568 and 425,572 clocks. Progressive PAL frames remain 425,568 clocks.
Timing specification updates
CHANGELOG.md, docs/ppu.md, docs/scheduler.md
Documentation describes the long scanline, H-counter behavior, IRQ matching, frame lengths, and the remaining unmodeled scanline.

Accuracy plan updates

Layer / File(s) Summary
Scanline accuracy status
docs/accuracysnes-plan.md
The plan marks B2.02 and B2.03 as implemented and records unresolved B2.07 work.
Audio validation design
docs/accuracysnes-plan.md
The Group E plan defines E8.01 validation for key-on delay, silent voices, fixed timer settings, and injection verification.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 10
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses a valid Conventional Commit format and accurately describes the long-scanline modeling change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Changelog Entry ✅ Passed The full diff against main includes a 24-line CHANGELOG.md entry for the user-visible PAL interlaced long-scanline and H-IRQ behavior.
Docs-As-Spec ✅ Passed The PR changes PPU timing and H-IRQ behavior, and edits docs/ppu.md to specify the 341-dot PAL interlaced line; bus.rs changes are tests only.
Accuracysnes Bookkeeping ✅ Passed The complete diff against main changes six files and adds no test or scene under tests/roms/AccuracySNES/gen/src/; the conditional bookkeeping checks do not apply.
No Panic On Untrusted Input ✅ Passed The full main-to-HEAD diff adds no .unwrap(), .expect(), or panic!() calls; the new Rust code uses checked line logic and test-only assertions.
Safety Comment On New Unsafe ✅ Passed The PR range from main adds no unsafe { ... } blocks or unsafe fn declarations, so no new // SAFETY: comment is required.

Comment @coderabbitai help to get the list of available commands.

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>
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Antigravity review (Gemini via Ultra)

Models the PAL interlaced 341-dot long scanline (V = 311, field set, interlace on) in rustysnes-ppu via dynamic H-wrap and H-IRQ comparator bounding, accompanied by frame clock alternation tests and spec documentation updates.

Blocking issues

None found.

Suggestions

  • crates/rustysnes-ppu/src/lib.rs:929: dots_this_line() is called on every dot tick in step() (and again in check_hv_irq()), executing is_long_scanline() which checks self.region, self.io.interlace, self.field, and self.v == 311. Because these conditions are invariant across a scanline, checking self.v == 311 first or caching the line dot count per scanline transition would avoid redundant field/enum matching on the hot dot-stepping path.
  • crates/rustysnes-ppu/src/lib.rs:1100-1128: Coverage for is_long_scanline() and dots_this_line() is currently indirect via Bus frame-clock integration tests. Add a direct unit test inside rustysnes-ppu asserting that dots_this_line() returns 341 on PAL line 311 under interlace + field-set conditions and 340 otherwise to test PPU logic in isolation.

Nitpicks

  • crates/rustysnes-core/src/bus.rs:1615-1632: Use standard vec![...] and Vec::new() instead of fully qualified std::vec![...] and std::vec::Vec::new() in unit tests to remain consistent with surrounding test style.

Automated first-pass review by agy on a self-hosted runner -- not a human review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The HV-IRQ dot-340-on-long-line edge case is a key behavior change but isn’t directly covered by a targeted regression test, making it easier to accidentally break later.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Models the SNES PAL interlaced long scanline timing quirk (dossier B2.03) by allowing a 341-dot scanline at V = 311 on field-set frames, and adjusts the PPU’s horizontal wrap / HV-IRQ comparator bounds to respect the per-line dot count. This fits into RustySNES’s dot/master-clock timing model and correctness goals by bringing PPU timing closer to hardware/reference behavior while keeping the change localized to vblank timing.

Changes:

  • Update Ppu::tick_dot H wrap to use a per-line dot count (dots_this_line()), enabling a true 341-dot line for B2.03.
  • Fix check_hv_irq horizontal bound to use the per-line dot count so h_target == 340 can match on the long line and remains suppressed on normal lines.
  • Add PAL interlace frame-length regression tests and update docs/CHANGELOG to reflect the new modeled behavior and remaining unmodeled interlace scanline count.
File summaries
File Description
docs/scheduler.md Updates scheduler documentation to reflect B2.03 being modeled and clarifies the still-unmodeled extra interlace scanline.
docs/ppu.md Updates PPU timing docs to state both short (B2.02) and long (B2.03) scanlines are modeled, with the long line implemented via H-wrap extension.
docs/accuracysnes-plan.md Updates the AccuracySNES plan status/notes to reflect B2.02/B2.03 landed and explains why B2.07 does not trivially follow.
crates/rustysnes-ppu/src/lib.rs Implements is_long_scanline() + dots_this_line(), uses them for H wrap and HV-IRQ horizontal matching bounds.
crates/rustysnes-core/src/bus.rs Adds unit tests asserting PAL interlaced frame-length alternation and progressive PAL invariance.
CHANGELOG.md Adds a release note entry documenting the long scanline model, HV-IRQ bound fix, and remaining interlace scanline-count gap.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment on lines +992 to 996
// Bounded by THIS line's dot count, not the constant: the long line has a dot 340 that
// a normal line does not, so an `HTIME` landing there is a real match on that line and
// a suppressed one everywhere else.
h_target < self.dots_this_line() && self.h == h_target
} else {

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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-core/src/bus.rs`:
- Around line 1604-1635: Update the_pal_interlaced_frame_gains_the_long_scanline
so it explicitly verifies alternation between adjacent frame lengths, rather
than only checking the distinct sorted values in kinds. After collecting the
samples, assert each neighboring pair in lens differs, while retaining the
existing validation that the two expected PAL frame lengths are present.

In `@docs/accuracysnes-plan.md`:
- Around line 654-664: Extend the E8.01 design and verification plan to cover
KOFF as well as KON: add a one-sample-offset differential measurement and
corresponding injection for KOFF, using the existing e8_02 timing/scaffolding
where applicable. Do not mark E8.01 verified until both polling behaviors are
measured; otherwise split the assertion and explicitly leave KOFF unmeasured.

In `@docs/scheduler.md`:
- Around line 109-120: Update the earlier normal-line timing paragraph in
docs/scheduler.md to specify 340 dots indexed 0..=339 and the 1368-clock
composition of 338 × 4 + 2 × 6, matching docs/ppu.md and Ppu::dots_this_line.
Remove the conflicting 341-dot and 336 × 4 + 4 × 5 description while leaving the
interlaced long-line discussion unchanged.
🪄 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: 7d3c1df8-c515-4309-8557-074735bf0b92

📥 Commits

Reviewing files that changed from the base of the PR and between 23fb76a and e09aa2b.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • crates/rustysnes-core/src/bus.rs
  • crates/rustysnes-ppu/src/lib.rs
  • docs/accuracysnes-plan.md
  • docs/ppu.md
  • docs/scheduler.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: accuracysnes
  • GitHub Check: test-light
  • GitHub Check: lint
🧰 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/ppu.md
  • docs/scheduler.md
  • docs/accuracysnes-plan.md
**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Chip-behavior changes must update both the chip implementation and the corresponding docs/<subsystem>.md documentation.

A chip change must update both the chip implementation and its corresponding docs/<chip>.md documentation in the same change.

Files:

  • docs/ppu.md
  • crates/rustysnes-core/src/bus.rs
  • CHANGELOG.md
  • docs/scheduler.md
  • crates/rustysnes-ppu/src/lib.rs
  • docs/accuracysnes-plan.md
**/*

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*: Do not commit or vendor the generated snesdev_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 relevant to-dos/ sprint file.

**/*: Preserve the one-directional crate graph: chip crates must not depend on one another; rustysnes-core ties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keep docs/STATUS.md as the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNES v2.0 or engine-lineage anchors as project releases.

Files:

  • docs/ppu.md
  • crates/rustysnes-core/src/bus.rs
  • CHANGELOG.md
  • docs/scheduler.md
  • crates/rustysnes-ppu/src/lib.rs
  • docs/accuracysnes-plan.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.md
  • docs/scheduler.md
  • 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 generated docs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.

Files:

  • docs/ppu.md
  • docs/scheduler.md
  • 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/ppu.md
  • CHANGELOG.md
  • docs/scheduler.md
  • docs/accuracysnes-plan.md
crates/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Bus owns 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.
Keep unsafe confined to existing allowed areas, namely frontend and FFI code, and document every unsafe block with a // SAFETY: comment.

Files:

  • crates/rustysnes-core/src/bus.rs
  • crates/rustysnes-ppu/src/lib.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-core/src/bus.rs
  • crates/rustysnes-ppu/src/lib.rs
**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Use Rust edition 2024 and the toolchain pinned in rust-toolchain.toml (Rust 1.96).
Run cargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy with cargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc because missing_docs is a workspace lint.
Do not run cargo clippy --all-features; scripting and script-wasm are mutually exclusive. Use explicit per-feature jobs instead.

**/*.rs: Do not introduce .unwrap(), .expect(), or panic!() 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 new unsafe { ... } block or unsafe fn must 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 because unsafe_code is a workspace lint.

**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspace pedantic, nursery, missing_docs, and unsafe_code warnings because CI runs with -D warnings. Document every public item.
Keep unsafe code restricted to the frontend and FFI, and include a // SAFETY: justification for each use.
Keep hot paths allocation-free.
Treat rustysnes_core::Bus as 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-core/src/bus.rs
  • crates/rustysnes-ppu/src/lib.rs
crates/rustysnes-core/**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

The core crate must compile for thumbv7em-none-eabihf with --no-default-features; the chip stack must remain no_std.

Files:

  • crates/rustysnes-core/src/bus.rs
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 matching docs/<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-core/src/bus.rs
  • crates/rustysnes-ppu/src/lib.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-core/src/bus.rs
  • crates/rustysnes-ppu/src/lib.rs
crates/**

⚙️ CodeRabbit configuration file

crates/**: Emulator core. Hot paths are allocation-free; unsafe requires a // SAFETY: comment
naming the invariant. Any change to save-stated fields needs a FORMAT_VERSION bump and a
docs/adr/0006 bump-log entry. Behavior changes must update the matching docs/<chip>.md
in the same change.

Files:

  • crates/rustysnes-core/src/bus.rs
  • crates/rustysnes-ppu/src/lib.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.md when 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
crates/rustysnes-ppu/**/*.{rs,md}

📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)

For the rustysnes-ppu PPU1 (5C77) and PPU2 (5C78) video path, read ../../docs/ppu.md before 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
🧠 Learnings (2)
📚 Learning: 2026-07-21T01:34:22.909Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 189
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T01:34:22.909Z
Learning: When reviewing the AccuracySNES documentation in docs/accuracysnes-*.md (notably docs/accuracysnes-plan.md vs the generated docs/accuracysnes-coverage.md), treat the reported metrics as intentionally non-equivalent: the battery test count and dossier assertion coverage are not interchangeable. Do not infer one count/coverage from the other during review (e.g., one test may contain multiple assertions, and multiple tests may contribute to a single assertion/row such as E6.02).

Applied to files:

  • docs/accuracysnes-plan.md
📚 Learning: 2026-07-21T05:22:58.848Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 197
File: docs/accuracysnes-plan.md:598-600
Timestamp: 2026-07-21T05:22:58.848Z
Learning: In the AccuracySNES documentation under `docs/`, when an assertion exists in the research dossier but cannot be measured/verified by the current cartridge timing test, distinguish the dossier assertion from test measurability: keep the original hardware assertion (and any contribution to the coverage denominator) intact, withdraw/stop using the specific test coverage only if the sources cannot decompose the required CPU-cycle timing into bus vs internal components, and mark the row as not measurable using the `[NOT CART-MEASURABLE ...]` annotation with links to the corresponding plan section (e.g., `docs/accuracysnes-plan.md` §A5.20) and the related roadmap/ticket (e.g., `to-dos/ROADMAP.md` ticket `T-06-A`). Ensure the documentation/coverage reporting treats the row as uncovered rather than removing or redefining the assertion.

Applied to files:

  • docs/accuracysnes-plan.md
🔇 Additional comments (8)
docs/accuracysnes-plan.md (2)

608-634: LGTM!


635-636: 🎯 Functional Correctness

Remove the claim that the extra scanline is unmodeled. The implementation models 312 PAL counter values (V = 0..=311) and adds one dot to the V = 311 line on the selected interlaced field; it does not model a separate V = 312 scanline. Update the text to describe the extra dot on V = 311, not an extra scanline.

			> Likely an incorrect or invalid review comment.
crates/rustysnes-ppu/src/lib.rs (3)

929-929: LGTM!


992-995: LGTM!


1101-1129: LGTM!

crates/rustysnes-core/src/bus.rs (1)

1637-1655: LGTM!

docs/ppu.md (1)

93-97: LGTM!

CHANGELOG.md (1)

119-142: LGTM!

Comment on lines +1604 to +1635
/// Dossier `B2.03`: PAL with interlace on has a 1368-clock, **341**-dot scanline at `V = 311`
/// on field-set frames, so the PAL frame alternates 425,568 / 425,572 master clocks.
///
/// Interlace is enabled through `SETINI $2133` bit 0 rather than by poking the field directly,
/// so the test drives the same path a game would.
#[test]
fn the_pal_interlaced_frame_gains_the_long_scanline() {
let mut bus = Bus::new(Region::Pal);
bus.ppu.write_reg(0x2133, 0x01);
let mut frame = bus.ppu.frame_count();
let mut last = bus.clock.master;
let mut lens = std::vec::Vec::new();
while lens.len() < 4 {
bus.advance_master(MASTER_PER_DOT);
if bus.ppu.frame_count() != frame {
frame = bus.ppu.frame_count();
lens.push(bus.clock.master - last);
last = bus.clock.master;
}
}
// One of the two phases carries the extra 4 clocks; which one depends on the field's
// power-on value, so assert the SET rather than a fixed order -- pinning the order would
// be pinning an initial condition this row does not care about.
let mut kinds = lens.clone();
kinds.sort_unstable();
kinds.dedup();
assert_eq!(
kinds,
std::vec![425_568, 425_572],
"PAL interlaced frames must alternate by the 4 clocks the long scanline adds, got {lens:?}"
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert frame alternation explicitly.

The test sorts and deduplicates the samples. A sequence such as 425_568, 425_568, 425_572, 425_572 would pass. Assert that adjacent frame lengths differ so the test detects a broken field toggle or a long-line predicate that applies on consecutive frames.

Proposed test assertion
         while lens.len() < 4 {
             bus.advance_master(MASTER_PER_DOT);
             if bus.ppu.frame_count() != frame {
                 frame = bus.ppu.frame_count();
                 lens.push(bus.clock.master - last);
                 last = bus.clock.master;
             }
         }
+        assert!(
+            lens.windows(2).all(|window| window[0] != window[1]),
+            "PAL interlaced frame lengths must alternate, got {lens:?}"
+        );
         // One of the two phases carries the extra 4 clocks; which one depends on the field's
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Dossier `B2.03`: PAL with interlace on has a 1368-clock, **341**-dot scanline at `V = 311`
/// on field-set frames, so the PAL frame alternates 425,568 / 425,572 master clocks.
///
/// Interlace is enabled through `SETINI $2133` bit 0 rather than by poking the field directly,
/// so the test drives the same path a game would.
#[test]
fn the_pal_interlaced_frame_gains_the_long_scanline() {
let mut bus = Bus::new(Region::Pal);
bus.ppu.write_reg(0x2133, 0x01);
let mut frame = bus.ppu.frame_count();
let mut last = bus.clock.master;
let mut lens = std::vec::Vec::new();
while lens.len() < 4 {
bus.advance_master(MASTER_PER_DOT);
if bus.ppu.frame_count() != frame {
frame = bus.ppu.frame_count();
lens.push(bus.clock.master - last);
last = bus.clock.master;
}
}
// One of the two phases carries the extra 4 clocks; which one depends on the field's
// power-on value, so assert the SET rather than a fixed order -- pinning the order would
// be pinning an initial condition this row does not care about.
let mut kinds = lens.clone();
kinds.sort_unstable();
kinds.dedup();
assert_eq!(
kinds,
std::vec![425_568, 425_572],
"PAL interlaced frames must alternate by the 4 clocks the long scanline adds, got {lens:?}"
);
}
/// Dossier `B2.03`: PAL with interlace on has a 1368-clock, **341**-dot scanline at `V = 311`
/// on field-set frames, so the PAL frame alternates 425,568 / 425,572 master clocks.
///
/// Interlace is enabled through `SETINI $2133` bit 0 rather than by poking the field directly,
/// so the test drives the same path a game would.
#[test]
fn the_pal_interlaced_frame_gains_the_long_scanline() {
let mut bus = Bus::new(Region::Pal);
bus.ppu.write_reg(0x2133, 0x01);
let mut frame = bus.ppu.frame_count();
let mut last = bus.clock.master;
let mut lens = std::vec::Vec::new();
while lens.len() < 4 {
bus.advance_master(MASTER_PER_DOT);
if bus.ppu.frame_count() != frame {
frame = bus.ppu.frame_count();
lens.push(bus.clock.master - last);
last = bus.clock.master;
}
}
assert!(
lens.windows(2).all(|window| window[0] != window[1]),
"PAL interlaced frame lengths must alternate, got {lens:?}"
);
// One of the two phases carries the extra 4 clocks; which one depends on the field's
// power-on value, so assert the SET rather than a fixed order -- pinning the order would
// be pinning an initial condition this row does not care about.
let mut kinds = lens.clone();
kinds.sort_unstable();
kinds.dedup();
assert_eq!(
kinds,
std::vec![425_568, 425_572],
"PAL interlaced frames must alternate by the 4 clocks the long scanline adds, got {lens:?}"
);
}
🤖 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-core/src/bus.rs` around lines 1604 - 1635, Update
the_pal_interlaced_frame_gains_the_long_scanline so it explicitly verifies
alternation between adjacent frame lengths, rather than only checking the
distinct sorted values in kinds. After collecting the samples, assert each
neighboring pair in lens differs, while retaining the existing validation that
the two expected PAL frame lengths are present.

Comment thread docs/accuracysnes-plan.md
Comment on lines +654 to +664
| `E8.01` | KON/KOFF polled every **second** sample (16 kHz) `[ERRATA]` | pure DSP-register observable; errata-flagged so the reference is explicit |
| `E9.02` | noise output is **highpass-filtered** `[ERRATA]` | DSP-observable via OUTX on a NON voice; errata-flagged |
| `E5.06` | BRR 15-bit wrap: clamp to 16 bits, then `+4000h..+7FFFh → -4000h..-1` | reuses the landed `E5` decoder scaffolding |

**`E8.01` design, worked out so the next session does not restart from the assertion text.** Follow
`e8_02`'s shape (`apu.rs:5298`), 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**, not an absolute: write `KON` twice, the second offset by **one
sample (32 SPC cycles)** from the first, and compare the two measured delays.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Cover KOFF before marking E8.01 verified.

E8.01 asserts behavior for both KON and KOFF, but the proposed differential measures only KON, and the injection changes only the KON polling path.

An implementation that polls KON every second sample and KOFF every sample would pass this test while violating the documented assertion. Add the equivalent differential and injection for KOFF, or split the assertion and mark the KOFF portion as unmeasured.

Also applies to: 681-683

🤖 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 `@docs/accuracysnes-plan.md` around lines 654 - 664, Extend the E8.01 design
and verification plan to cover KOFF as well as KON: add a one-sample-offset
differential measurement and corresponding injection for KOFF, using the
existing e8_02 timing/scaffolding where applicable. Do not mark E8.01 verified
until both polling behaviors are measured; otherwise split the assertion and
explicitly leave KOFF unmeasured.

Source: Path instructions

Comment thread docs/scheduler.md
Comment on lines +109 to +120
**The long line is modelled too** (`B2.03`): PAL, interlace on, field set, `V = 311` — 1368 clocks
and **341** dots. Its shape is the opposite of the short line's and that is why the two landed
separately: the short line substitutes dot *lengths*, while this one **appends a whole extra 4-clock
dot** and leaves the two 6-clock dots alone (`339 x 4 + 2 x 6 = 1368`). So it moves the H wrap
(`Ppu::dots_this_line`) rather than the Bus's clock table, and the H counter reaches 340 on that line
and on no other. The H-IRQ comparator's upper bound follows the same per-line count, so an `HTIME`
landing on dot 340 is a real match there and suppressed everywhere else. PAL interlaced frames
alternate 425,568 / 425,572.

**Still unmodelled, and separate:** the interlaced frame's extra *scanline* (263 / 313 rather than
262 / 312). `Region::lines_per_frame` is documented as the non-interlaced count and nothing varies
it, so `B2.03` above is reachable without it — `V = 311` is the last PAL line either way.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the conflicting normal-line timing specification.

Lines 69-71 still define a normal line as 341 dots with 336 × 4 + 4 × 5. The binding convention, docs/ppu.md, and Ppu::dots_this_line define normal lines as 340 dots (0..=339) with 338 × 4 + 2 × 6. Update the earlier paragraph before merging this change. Otherwise this document contains incompatible H-counter and clock specifications.

Proposed documentation fix
-- **Normal scanline = 1364 master clocks = 341 dots.** The invariant is
-  **1364 = 336×4 + 4×5** ...
+- **Normal scanline = 1364 master clocks = 340 dots.** The invariant is
+  **1364 = 338×4 + 2×6** ...
🤖 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 `@docs/scheduler.md` around lines 109 - 120, Update the earlier normal-line
timing paragraph in docs/scheduler.md to specify 340 dots indexed 0..=339 and
the 1368-clock composition of 338 × 4 + 2 × 6, matching docs/ppu.md and
Ppu::dots_this_line. Remove the conflicting 341-dot and 336 × 4 + 4 × 5
description while leaving the interlaced long-line discussion unchanged.

Source: Path instructions

@doublegate
doublegate merged commit b2057b2 into main Aug 1, 2026
16 checks passed
@doublegate
doublegate deleted the b2-03-replay branch August 1, 2026 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants