Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- **PPU: the field flag's doc contradicted the code, and nothing pinned either.** `Ppu::field` is
`$213F` bit 7 and toggles at the end of **every** frame; its doc comment said "toggles each frame
when interlace is on", which describes neither the code nor the hardware. Only the flag's *use* is
interlace-conditional (picking the odd/even row). The stale reading matters beyond tidiness: it
makes the short/long-scanline gate that keys on the field look unreachable in progressive mode
when it is not, which is a `v1.29.0` prerequisite for `B2.02`/`B2.03`.

Corrected in the code, in `docs/ppu.md`, and pinned by a test that ticks four progressive frames
and requires bit 7 to alternate. Injecting the gate the old doc described (`if self.io.interlace`)
makes it fail with a constant `[0,0,0,0]`, so the test measures the behaviour rather than
restating it.

- **App Store §4.7 self-audit done (`v1.30.0`).** Checked against the shipped tree rather than
against intent. Two of the three risk classes are clean: there is **no ROM acquisition path**
(`load_rom` takes bytes from the caller; no download, fetch or URL anywhere in the mobile crates),
Expand Down
36 changes: 35 additions & 1 deletion crates/rustysnes-ppu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,14 @@ pub struct Ppu {
h: u16,
/// Vertical scanline counter, 0..=(lines_per_frame-1).
v: u16,
/// Interlace field (toggles each frame when interlace is on).
/// The PPU2 field flag — `$213F` bit 7.
///
/// It toggles at the end of **every** frame, interlaced or not ([`Ppu::end_of_scanline`]);
/// only its *use* is conditional. Interlace consumes it to pick the odd/even row
/// (`render.rs`), and it is readable through `$213F` regardless. The doc here previously said
/// "toggles each frame when interlace is on", which describes neither the code nor the
/// hardware, and would make the short/long-scanline gate that keys on it look unreachable in
/// progressive mode when it is not.
field: bool,
/// Currently inside vertical blank.
vblank: bool,
Expand Down Expand Up @@ -1327,6 +1334,33 @@ mod tests {
assert_eq!(p.region, Region::Ntsc);
}

#[test]
fn the_field_flag_toggles_every_frame_even_in_progressive_mode() {
// Pins the `field` doc: the flag is `$213F` bit 7 and toggles unconditionally. Only its
// *use* is interlace-conditional. A model that gated the toggle on `io.interlace` would
// leave bit 7 constant here, and would also make the short/long-scanline gate that keys
// on `field` unreachable in progressive mode -- which is why this is pinned rather than
// left as a comment.
let mut p = Ppu::new();
assert!(!p.io.interlace, "this test is about the progressive case");

let mut bus = NullVideoBus;
let mut seen = [0u8; 4];
for slot in &mut seen {
let before = p.frame_count;
while p.frame_count == before {
p.tick_dot(&mut bus);
}
*slot = p.read_reg(0x213F) & 0x80;
}

assert_eq!(
seen,
[0x80, 0x00, 0x80, 0x00],
"bit 7 must alternate on every frame boundary, not stay put"
);
}

#[test]
fn full_state_round_trips_through_save_state() {
let mut p = Ppu::with_region(Region::Pal);
Expand Down
21 changes: 21 additions & 0 deletions docs/accuracysnes-plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,27 @@ before the H counter wraps and silently returns a plausible small number.
| `B2.07` | gated on `B2.02` | `B2.04` covers the 262-line count, not the clock total, so the frequency is not implied by it. Getting to 60.0988 Hz needs clocks-per-frame, and the frame alternates 357,368/357,364 because of the short scanline — which *is* `B2.02`, a `T-06-A` dot-model residual scheduled for `v1.29.0`. |
| `B2.09` | `v1.29.0` by its own dossier note | the picture window is "not CPU-observable directly; reachable through the framebuffer oracle once the dot-resolution compositor lands". |

#### `B2.02`/`B2.03` — the prerequisite is cleared, the model change is not started

The plan for `v1.29.0` recorded a doubt about whether the short-line gate was even reachable:
`field` is one of its four inputs, and `Ppu::field`'s doc comment said it "toggles each frame when
interlace is on", which would leave it constant in the progressive NTSC case `B2.02` needs.

**Settled against the source: the doc was wrong, the code is right.** `end_of_scanline` toggles
`field` unconditionally, which is what `$213F` bit 7 does on hardware; only the flag's *use* is
interlace-conditional (`render.rs` picks the odd/even row with it). The comment and `docs/ppu.md`
are corrected, and `the_field_flag_toggles_every_frame_even_in_progressive_mode` pins it —
injecting the `if self.io.interlace` gate the old comment described makes that test fail with a
constant `[0,0,0,0]`. So all four gate inputs (`io.interlace`, `field`, `v`, `region`) are live and
the gate is reachable.

What is **not** done is the model change itself. `dot_length` (`rustysnes-core::bus`) is a pure
function of the dot, so it cannot express a per-line variation; the short line (1360 clocks, all 340
dots at 4 — the long dots vanish) and the long line (1368, 341 dots) both need the line context
threaded to it. Expect goldens to move, and note that a previous attempt at the neighbouring H-IRQ
mapping change was reverted for exactly that reason — so the guard test named in the roadmap lands
**first**, before `dot_length` is touched.

#### `A5.18` — why it is parked despite working

`BRK` cannot be measured without `RTI`: a `BRK` that never returns cannot be repeated, so any
Expand Down
5 changes: 5 additions & 0 deletions docs/ppu.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ the rest four**; see `docs/scheduler.md` "Convention (binding)"):
overscan). **HBlank** H=274→H=1.
- **H/V latch:** read **SLHV $2137** latches H/V; **OPHCT $213C / OPVCT $213D** read twice for
the 9-bit values; **STAT78 $213F** read clears the latch + resets the toggle.
- **Field flag ($213F bit 7):** toggles at the end of **every** frame, interlaced or not. Only its
*use* is conditional — interlace consumes it to select the odd/even row. A model that gated the
toggle itself on interlace would leave bit 7 constant in progressive mode, and would make any
short/long-scanline behaviour keyed on the field unreachable there. Pinned by
`the_field_flag_toggles_every_frame_even_in_progressive_mode`.

## CGRAM, VRAM, color math, windows

Expand Down
Loading