feat(ppu): model the long scanline (B2.03) - #295
Closed
doublegate wants to merge 4 commits into
Closed
Conversation
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>
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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>
Antigravity review (Gemini via Ultra)This PR implements dot-model dossier Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
This PR extends RustySNES’s PPU dot/scanline timing model to cover the PAL interlaced long scanline case (AccuracySNES dossier B2.03), where one scanline has 341 dots (1368 master clocks) due to an appended extra 4-clock dot, requiring the PPU’s horizontal counter to reach dot 340 on that line only.
Changes:
- Model the long scanline by making PPU horizontal wrap depend on a new per-line dot count (
Ppu::dots_this_line) and addingPpu::is_long_scanline. - Fix H/V IRQ horizontal matching to bound against the per-line dot count so dot-340 matches are possible on the long line and suppressed elsewhere.
- Add core timing regression tests for PAL interlaced frame-length alternation and update docs/changelog to reflect the newly-modeled behavior (while explicitly noting the still-unmodeled interlaced extra scanline).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/scheduler.md | Updates scheduler/timing documentation to describe B2.03, per-line dot counts, and the IRQ bound change; keeps the extra interlace scanline explicitly listed as unmodeled. |
| docs/ppu.md | Updates PPU timing docs to reflect that both short (B2.02) and long (B2.03) scanlines are now modeled, including the H-wrap implication. |
| crates/rustysnes-ppu/src/lib.rs | Implements per-line dot count (dots_this_line), adds is_long_scanline, updates H wrap, and fixes check_hv_irq to use the per-line dot bound. |
| crates/rustysnes-core/src/bus.rs | Adds PAL interlace frame-length regression tests validating 425,568/425,572 alternation and that progressive PAL is unaffected. |
| CHANGELOG.md | Documents the new long-scanline modeling and the IRQ-bound change, plus notes the still-unmodeled extra interlaced scanline. |
Comment on lines
+1637
to
+1654
| /// The long line is PAL+interlace only. Progressive PAL -- the default -- must not pick it up, | ||
| /// which is what separates it from `B2.02`'s NTSC case. | ||
| #[test] | ||
| fn progressive_pal_does_not_gain_the_long_scanline() { | ||
| let mut bus = Bus::new(Region::Pal); | ||
| let mut frame = bus.ppu.frame_count(); | ||
| let mut last = bus.clock.master; | ||
| let mut lens = std::vec::Vec::new(); | ||
| while lens.len() < 3 { | ||
| 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_eq!(lens, std::vec![425_568, 425_568, 425_568]); | ||
| } |
doublegate
force-pushed
the
feat/short-scanline-b2-02
branch
from
August 1, 2026 02:23
14081d9 to
92bcc2b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second
v1.29.0dot-model residual, stacked on #294. Base retargets tomainautomatically when#294 merges — it is stacked rather than combined because the two changes have genuinely different
shapes and different risk.
The distinction that made these two PRs
B2.02shortB2.03long (this)V=240V=311339 x 4 + 2 x 6 = 1368. Because the long line adds a dot rather than retiming one, it needed a newPpu::dots_this_line, and the H counter reaches dot 340 on that line and on no other.A second, less obvious fix
check_hv_irqbounded the horizontal match withh_target < DOTS_PER_LINE, a constant. It now usesthe same per-line count, so an
HTIMEwhose match lands on dot 340 is a genuine match on the longline and stays suppressed everywhere else. Before this it could never match anywhere — the long
line's own dot was unreachable by an IRQ.
Verification
SETINI $2133bit 0, not by poking the field, so it drives thepath a game drives.
depends on the field's power-on value, and pinning the order would pin an initial condition this
row does not care about.
honest.
dots_this_lineto the constant fails the interlaced test;dropping the interlace term fails the progressive one.
clippy -D warnings,fmt --check,cargo doc -D warnings, and theno_stdthumbv7em-none-eabihfbuild all 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.03is reachable without it becauseV = 311is the last PAL line either way;docs/scheduler.mdrecords it as an open item rather than leaving it implied.🤖 Generated with Claude Code