feat(pif): report "no device" on empty ports — SM64 renders its title screen (R-18)

[doublegate]

Super Mario 64 renders its title screen. Mario's head, textured cap with the M logo, over the tiled SUPER MARIO 64 background — screenshots/super-mario-64-title.png. 125,278 RDP commands, 138,474/148,125 pixels lit at 625×237, through the full LLE path. Viewed and confirmed, not counted.

The unblocking defect was in the PIF, not the RDP

SM64 was halting in its own assert path — a B -1 self-loop at 0x80246DD8, reached by falling through BNE t1, zero, +4 where t1 is a byte at 0x8032D590 that is never written — immediately after an osCreateThread(..., pri=100) call.

Cause: our PIF answered the 0x00/0xFF info command as a connected controller on all four joybus channels, so osContInit reported four pads on a one-pad console. n64brew_wiki/markdown/PIF-NUS.md §RX byte: special flags is explicit:

Bit 7 | 0x80 | No device — set if the handshake failed because no device appears to be connected to the joybus channel.

We never set it.

A second, pre-existing defect sat next to it

mark_no_device wrote the flag to resp - 1. For a block laid out TX RX tt[tx_len] rr[rx_len] with resp = i + 2 + tx_len, that is the last TX data byte, not the RX byte at i + 1. Since run_channel returns early on tx_len == 0, the two offsets could never coincide — so the flag had never once landed where software reads it, while a doc comment asserted it did.

Mutation-checked — and the second test exists because the first mutation PASSED

mutation	caught?
revert empty-port guard	✅ red
revert mark_no_device offset	❌ passed

The empty-port guard short-circuits before run_channel, so nothing exercised mark_no_device at all. mark_no_device_flags_the_rx_byte_not_the_tx_payload reaches it via a connected channel given an unsupported command, and uses a two-byte TX so resp - 1 and the RX byte are two apart — with a one-byte TX an off-by-one is indistinguishable from a correct write. Both mutations are now caught.

Measured effect

Title	RDP commands	result
Super Mario 64	0 → 125,278	title screen
Banjo-Kazooie	0 → 133,625	real 3D geometry

Banjo-Kazooie is committed as a known-imperfect frame: geometry, textures and depth ordering are right; colours carry a heavy blue/yellow cast (open combiner/texel-format issue). It is evidence of what currently happens, not a correctness target.

Oracle unchanged: n64-systemtest Phase 1 Failed: 0, suite-wide 90.

A measurement trap, recorded

A 240-frame sample reported SM64 at scanout = 0x0 and looked like a geometry failure. It was a transient — one frame with H_VIDEO = 0 mid-reprogram of the VI. Sampling at 60/120/180/300 shows a stable 625×237 throughout. A single-point sample of a value the guest rewrites can land on the rewrite.

Still open

Jet Force Gemini and Banjo-Tooie remain at 0 RDP commands; 1080 Snowboarding still faults in IPL3 on an odd DMEM address. Separate causes, recorded in R-18.

Gates

fmt · clippy -D warnings · cargo test --workspace · rustdoc -D warnings · markdownlint · n64-systemtest — all green. No ROMs committed; only rendered output, which the commercial-roms policy permits.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Added controller-port presence detection to accurately reflect which controller ports are populated.
  • Added new rendered-image evidence for Super Mario 64’s title screen and an early Banjo-Kazooie scene.

• Bug Fixes

  • Corrected “no device” reporting for disconnected controller ports, ensuring the flag is placed in the correct reply byte.
  • Improved commercial game rendering outcomes, including corrected boot sequencing, interrupt handling, and CI4 texture palette mapping.
  • Recorded first verified commercial title-screen render with command and pixel accuracy results.

• Documentation

  • Updated accuracy ledger entries and screenshot documentation with revised methodology, new evidence, and outstanding limitations.

Walkthrough

PIF now tracks connected controller ports and reports absent devices through the correct RX-byte flag. Tests cover the corrected response layout and dispatch behaviour. R-18 records and screenshot documentation now include commercial-title rendering evidence and related accuracy findings.

Changes

PIF controller handling and R-18 evidence

Layer / File(s)	Summary
PIF controller state and dispatch crates/rustyn64-cart/src/pif.rs	Adds controller connectivity state and accessors, routes disconnected channels to no-device responses, and writes RX_NO_DEVICE using the explicit RX-byte offset.
Controller response tests crates/rustyn64-cart/src/pif.rs	Verifies absent ports flag the RX byte without altering the final TX byte, connectivity affects dispatch, and populated ports respond without the flag.
R-18 evidence records docs/accuracy-ledger.md, screenshots/README.md, CHANGELOG.md	Records commercial-title rendering results, corrected accuracy findings, screenshot evidence, and the PIF fix in the unreleased changelog.

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

Sequence Diagram(s)

sequenceDiagram
  participant PifExecute
  participant ControllerPort
  participant ResponseBuffer
  PifExecute->>ControllerPort: inspect connected state
  ControllerPort->>ResponseBuffer: set RX_NO_DEVICE on RX byte
  ResponseBuffer-->>PifExecute: return channel response

Loading

🚥 Pre-merge checks | ✅ 7 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	It matches the main change, but it exceeds the 72-character Conventional Commits limit, so it fails the title rule.	Shorten it to type(scope): subject within 72 characters, e.g. feat(pif): report no device on empty ports.
Docs-As-Spec Sync	⚠️ Warning	FAIL: rustyn64-cart/src/pif.rs changed observable Joybus behaviour, but docs/cart.md did not change; HEAD only touches pif.rs and docs/accuracy-ledger.md, with no no-docs-needed rationale.	Add or update docs/cart.md for the new controller-port mask and RX 'no device' semantics, or explain in the PR body why cart docs are intentionally unchanged.
Measured, Never Tuned	⚠️ Warning	FAIL: Pif::new() hard-codes [true, false, false, false] with no manual/wiki citation or ledger provenance; R-18 records the SM64 outcome, not that default mask.	Either cite the one-pad default to a manual/wiki page or record its measurement in docs/accuracy-ledger.md; otherwise remove the hard-coded hardware assumption from Pif::new().

✅ Passed checks (7 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is clearly about the PIF no-device fix and SM64 render result, so it is related to the changeset.
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.
Oracle Number Is Stated	✅ Passed	PASS: the PR states the n64-systemtest oracle effect explicitly ('Phase 1 Failed: 0, suite-wide 90'), matching the STATUS ledger.
Changelog Entry For User-Visible Changes	✅ Passed	PASS: CHANGELOG.md has an [Unreleased] section with Added/Fixed entries for the SM64 render and PIF joybus fix, matching the rule.
Unsafe Stays Out Of The Chip Crates	✅ Passed	No unsafe constructs were found; every chip/core lib.rs still has #![forbid(unsafe_code)], and the frontend has no unsafe blocks.

---

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

[doublegate]

Adjudication — Antigravity review

No blocking issues. All three suggestions adopted.

1. No API to configure connected controllers — adopted

Correct, and it would have been a real limitation: connected was private with no way in, so the PIF permanently reported single-player and a 2-4 player frontend had no route to attach pads. Added set_controllers_connected([bool; 4]) and controllers_connected(), documented that channel 4 is the cartridge/EEPROM bus and not a controller port.

2. Bypassing the mark_no_device helper — adopted, and it improved the test

Right on the DRY/bounds-checking grounds, and it turned out to matter more than that. Delegating means the two no-device paths now share the write, so reverting the offset turns BOTH tests red instead of one:

before refactor:  offset mutation -> 1 failed
after  refactor:  offset mutation -> 2 failed

Worth noting because the reason the second test exists is that the first mutation originally passed — the empty-port guard short-circuited before run_channel, so nothing exercised mark_no_device at all. Delegating removes that split entirely.

3. Magic subtraction in the test offsets — adopted

resp - 1 - 1 was opaque and, worse, fragile in the direction that matters: it encodes the answer the test is checking. The offsets now derive from the documented layout —

const SKIPS: usize = 0;
const TX: [u8; 2] = [0x99, 0x5A];
let rx_byte = SKIPS + 1;                    // TX RX tt[..] rr[..]
let last_tx = SKIPS + 2 + TX.len() - 1;
assert_eq!(resp, SKIPS + 2 + TX.len(), "frame layout as documented");

— with the layout itself asserted, so the derivation cannot drift silently against frame().

(Clippy items_after_statements required hoisting those consts to the top of each test; fixed in the same commit.)

Gates re-run: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings — green, and both mutations re-checked after the refactor.

[coderabbitai]

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/rustyn64-cart/src/pif.rs`:
- Around line 521-528: Update the documentation around Pif::mark_no_device to
remove the claim that the connected unsupported-command path is its only route.
Describe it specifically as the connected-controller unsupported-command route,
while acknowledging that the missing-EEPROM path also reaches mark_no_device.
- Around line 116-133: Add a controller dispatch test covering
set_controllers_connected: enable port 1, send its info handshake through the
PIF dispatch path, and assert the expected controller ID plus an RX byte without
error flags. Ensure the test fails when the configured presence mask is ignored,
while preserving the default controller behavior for existing tests.

In `@docs/accuracy-ledger.md`:
- Line 413: Update the later historical passage about `tlut_en` so it no longer
says the field remains undecoded; preserve the surrounding historical wording
and clarify that `tlut_en` is decoded while retaining the documented limitation
that the committed vector does not independently validate its gating behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ad312a15-8e34-4520-a890-ed73e378b068

📥 Commits

Reviewing files that changed from the base of the PR and between bc91aae and 465494c.

⛔ Files ignored due to path filters (2)

• screenshots/banjo-kazooie-first-3d-scene.png is excluded by !**/*.png
• screenshots/super-mario-64-title.png is excluded by !**/*.png

📒 Files selected for processing (4)

• CHANGELOG.md
• crates/rustyn64-cart/src/pif.rs
• docs/accuracy-ledger.md
• screenshots/README.md

[github-actions]

Antigravity review (Gemini via Ultra)

This PR configures joybus channels 2–4 as disconnected by default in the PIF and fixes mark_no_device to target the RX byte (i + 1) rather than the last TX data byte (resp - 1), preventing empty controller ports from falsely reporting connected devices to osContInit.

Blocking issues

None found.

Suggestions

• pif.rs: Save-state binary compatibility (ADR 0005): Adding connected: [bool; 4] alters the struct memory footprint and serialization layout of Pif. If Pif is serialized directly into save states, verify whether adding this field breaks backward compatibility with existing save states or requires migration logic under ADR 0005.

Nitpicks

• pif.rs: Field connected: [bool; 4] is ambiguous next to pak_present: [bool; 4]; consider renaming to ports_connected or controllers_connected for consistency with its public accessor methods.

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

[coderabbitai]

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)

crates/rustyn64-cart/src/pif.rs (1)

292-310: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the cart/PIF spec and oracle note
crates/rustyn64-cart/src/pif.rs changes behaviour, but this commit only edits docs/accuracy-ledger.md for R-18. docs/cart.md is still untouched, and there is no PIF-specific n64-systemtest impact note here (or an explicit “not measured” entry) for this change.

🤖 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/rustyn64-cart/src/pif.rs` around lines 292 - 310, Update the
documentation alongside the PIF behavior change in `pif.rs`: document the
empty-controller-channel handling and channel 4 exception in `docs/cart.md`, and
add a PIF-specific `n64-systemtest` impact note in the relevant accuracy
documentation, explicitly marking it as not measured if no result is available.

Sources: Coding guidelines, Path instructions

🤖 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/accuracy-ledger.md`:
- Line 413: Remove the unreleased “v0.6.0” version reference from the R-18 entry
in the accuracy ledger. Replace it with an unversioned description of shipping
based on the committable gates and honest “boots and executes” capstone,
preserving the surrounding historical context.

---

Outside diff comments:
In `@crates/rustyn64-cart/src/pif.rs`:
- Around line 292-310: Update the documentation alongside the PIF behavior
change in `pif.rs`: document the empty-controller-channel handling and channel 4
exception in `docs/cart.md`, and add a PIF-specific `n64-systemtest` impact note
in the relevant accuracy documentation, explicitly marking it as not measured if
no result is available.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6173be44-3899-4d1a-9b66-44abfc524652

📥 Commits

Reviewing files that changed from the base of the PR and between 465494c and 925c22e.

📒 Files selected for processing (2)

• crates/rustyn64-cart/src/pif.rs
• docs/accuracy-ledger.md