Repository navigation
fix(ppu): the field flag toggles every frame, not only when interlaced - #293
Conversation
`Ppu::field` is `$213F` bit 7 and is toggled unconditionally at frame end, but its doc comment claimed it toggles "when interlace is on". Only the flag's use is interlace-conditional -- interlace consumes it to select the odd/even row. The stale reading is load-bearing rather than cosmetic: it makes the short/long-scanline gate that keys on the field look unreachable in progressive mode, which is exactly the v1.29.0 prerequisite for B2.02 and B2.03. Correct the comment and docs/ppu.md, and pin the behaviour with a test that ticks four progressive frames and requires bit 7 to alternate. Injecting the gate the old doc described makes it fail with a constant [0,0,0,0]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 13 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 (4)
Comment |
…ains v1.29.0's plan doubted whether B2.02's short-line gate could be reached at all, because `field` is one of its inputs and that field's doc comment said it only toggles when interlace is on. Settled against the source: the code toggles it unconditionally, which is what $213F bit 7 does; the comment was stale. All four gate inputs are live. Record that, and that the model change itself is not started -- and why: `dot_length` is a pure function of the dot and cannot express a per-line variation, so the line context has to be threaded to it, and the guard test lands before it is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR corrects doc comments and Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
A
v1.29.0prerequisite, found where the plan said it would be.The finding
Ppu::fieldis$213Fbit 7. The code toggles it unconditionally at frame end(
end_of_scanline), which is what the hardware does. Its doc comment said:That describes neither. Only the flag's use is interlace-conditional — interlace consumes it in
render.rsto select the odd/even row — and$213Fexposes it regardless.Why it is not cosmetic
The
v1.29.0dot-model work (B2.02short line /B2.03long line) needs a gate that keys on thefield. Reading the comment rather than the code makes that gate look unreachable in progressive
mode, which would have sent the implementation down a wrong path. The plan flagged exactly this
("
fieldtoggles unconditionally at frame end, so the gate is reachable — butfield's doc commentsays otherwise and is stale against the code"); this confirms it against the source and pins it.
Verification
New test
the_field_flag_toggles_every_frame_even_in_progressive_modeassertsio.interlaceisfalse, ticks four frames, and requires
$213Fbit 7 to read[0x80, 0x00, 0x80, 0x00].Injection-checked rather than assumed: replacing the toggle with
if self.io.interlace { ... }—the behaviour the old comment described — fails the test with a constant
[0, 0, 0, 0]. So the testpins the mechanism, not the wording.
docs/ppu.mdgains the rule in the same change, per the chip-doc requirement.Gates:
cargo test -p rustysnes-ppu,clippy -D warnings,fmt --check, and theno_stdthumbv7em-none-eabihfbuild all clean (the test uses a fixed array, notVec, since the crate isno_std).🤖 Generated with Claude Code