fix(ai): an unprogrammed AI_DACRATE must not stop the DAC (R-16) - #205
Conversation
`AI_STATUS.FULL` could latch and never clear, so a game polling it for a free
audio DMA slot spun forever.
Found by root-causing World Driver Championship, the one title the boot-path
census newly implicated: 176,085 RDP commands under `hle_boot` and ZERO under
`real_pif_boot`. Measured rather than guessed -- the dominant retiring
instruction under real-PIF is
0x8007a8a0 LUI t6, 0xA450 ; the AI register block
0x8007a8a4 LW a0, 0xC(t6) ; AI_STATUS <- dominant
0x8007a8b0 AND t7, a0, at ; test bit 31 (FULL)
0x8007a8b4 BEQ t7, zero, +3 ; exits only when FULL is clear
and counting FULL *transitions* rather than sampling the bit -- the sticky-
register lesson from this ledger row's own Cause.ExcCode mistake -- settles it:
over 300 frames the flag transitions 48 times under HLE and exactly ONCE under
real-PIF, latching at ~frame 30 and never clearing again.
Mechanism: `recompute_rate` mapped an unprogrammed `AI_DACRATE` to
`sample_rate = 0`, `period_ticks()` to 0, and `tick()` then returned BEFORE
`emit_sample` -- which is the only place a drained transfer is retired. A
stopped DAC therefore makes the two-deep FIFO unable to advance, and
`FULL` (`dma_count > 1`) becomes permanent. WDC queues two buffers and polls
FULL before programming the DAC, so it sat exactly in that window.
The fix invents nothing. Hardware has no stopped-DAC state to model -- the DAC
counter runs off the video clock from power-on whatever `AI_DACRATE` holds --
and ares (ISC, vendorable, so readable per ref-proj/README.md) makes the same
choice structurally: `AI::power()` sets `dac.frequency = 44100` and `AI::main()`
calls `sample()` unconditionally, so its equivalent retirement block runs from
power-on. `DEFAULT_DAC_HZ = 44_100` is taken from there and is labeled a
MODELING DEFAULT, not a measured value: `AI_DACRATE`'s reset value is not
documented in anything this project mirrors. Nothing observable should depend on
the exact number, because every title programs the register before it plays
anything.
The old zero-gate was avoiding a real failure -- `dac_rate == 0` computing
`video_clock / 1` ~= 48 MHz and flooding the sink -- and a default rate avoids
both that and the latch.
`Audio::new()` had to change too, and the reason is worth recording: it
initialized `sample_rate: 0` literally, and `recompute_rate` only runs on a
DACRATE or region write, so the first version of this fix left a machine that
never programmed the AI with a stopped DAC anyway. The defect survived its own
fix until the constructor matched.
Measured effect: WDC under `real_pif_boot` goes 0 -> 120,015 RDP commands and
from `scanout 0x0` to a real 625x237 frame; FULL transitions 1 -> 46. Full
workspace suite 810 pass, so the Phase-4 golden PCM stream is unaffected.
Pinned by `full_clears_even_when_dacrate_was_never_programmed`, mutation-checked
(restoring either half of the fix turns it red). One pre-existing test was
CORRECTED rather than deleted: `set_region_before_dacrate_keeps_rate_zero`
asserted the rate was exactly 0, which over-specified its own stated purpose --
its comment says it exists to prevent a ~48 MHz rate, and that protection is
kept as an order-of-magnitude bound in the renamed
`set_region_before_dacrate_does_not_fabricate_the_video_clock_rate`.
Still open and NOT claimed fixed: WDC now renders but ends its run in a `B -1`
self-loop at 0x8000_28C0, a different halt reached after 120k commands. This
closes the AI livelock, not that title.
n64-systemtest impact: none -- the suite has no AI coverage, so the count stays
at 90. Validated by ares' structure, the unit test, and WDC's unblocking.
Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings,
no_std thumbv7em, markdownlint, check_no_roms, check_en_us -- all green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe audio model uses a 44.1 kHz fallback when ChangesAI audio DAC fallback
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AudioNew
participant AudioRate
participant FIFO
participant AIStatus
AudioNew->>AudioRate: Initialise unprogrammed DAC rate
AudioRate->>FIFO: Apply DEFAULT_DAC_HZ
FIFO->>AIStatus: Retire queued transfer
AIStatus-->>FIFO: Clear FULL
Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
Adopts the Antigravity review on #205. No blocking issues; three items. 1. `dac_rate == 0` conflates "never programmed" with "software wrote 0". CORRECT, and confirmed against the oracle: ares keeps the two apart — `AI::power()` sets 44100 while its AI_DACRATE write honors a literal zero (`dac.frequency = max(1, system.videoFrequency() / (io.dacRate + 1))`, ares/n64/ai/io.cpp:61), so a written 0 asks for a ~48 MHz DAC and ares gives it. NOT fixed here, deliberately. Telling the two apart needs a `dac_rate_programmed` flag, which adds a field to a serialized struct and so changes the save-state layout (ADR 0005) — an announced-in-advance change, not one to make in passing while fixing an unrelated livelock. It is also unobservable: no title requests a ~48 MHz DAC. Recorded in R-16 and in a code comment, with the exact ares line fidelity would require, rather than left as an undocumented conflation. 2. Raw register indices and bitmasks in the new test. REJECTED with reason: the crate has no symbolic register constants and every existing test in the module uses raw indices (`ai.write_reg(1, 0x40)`), so the new test matches the surrounding convention. Introducing named constants is worth doing, but as one change across the whole module rather than in two lines of a bug-fix PR, where it would read as unrelated churn. 3. Assert the exact constant rather than only a bound. ADOPTED, kept alongside the bound, because the two catch different things: the equality pins the WIRING (that `recompute_rate` reads the named constant instead of an inlined literal that could drift), while the bound pins the PROPERTY and is what catches the failure class the test exists for — a rate derived from the video clock rather than an audio clock. Also records the corpus census result, which is negative and easy to omit: the 66-title census is UNCHANGED after the fix at 29 render / 24 silent, so no other silent title shared this latch. That census boots through `hle_boot`, where WDC already rendered, so it cannot show the improvement it was launched to measure — foreseeable before the run, and stated rather than dropped. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint, check_no_roms, check_en_us — all green.
Adjudication of the Antigravity review1. You're right, and ares proves it. Its reset state and its written-zero state are genuinely different: // ai/ai.cpp -- reset
auto AI::power(bool reset) -> void { dac.frequency = 44100; ... }
// ai/io.cpp:61 -- an AI_DACRATE write, honoring a literal zero
dac.frequency = max(1, system.videoFrequency() / (io.dacRate + 1));So a written Not fixed here, deliberately, and for a reason the diff doesn't show. Telling the two apart needs a So it is now ledgered in R-16 and stated in a code comment, with the exact ares line fidelity would require, rather than left as an undocumented conflation. That is the trade this project's ledger exists to make explicit — a known simplification recorded beats a quiet one, and beats a save-state break taken unilaterally. 2. Raw register indices and bitmasks in the new test. REJECTED, with reason. The crate has no symbolic register constants, and every existing test in the module uses raw indices — e.g. Introducing named offsets is a genuine improvement and worth doing — but as one change across the whole module, not as two lines inside a bug-fix PR where it would read as unrelated churn and obscure the actual fix. Noted as a follow-up rather than dismissed. 3. Assert the exact constant, not only a bound. ADOPTED — kept alongside the bound. Both now, because they catch different failures: assert_eq!(ai.sample_rate(), Audio::DEFAULT_DAC_HZ, ...); // pins the WIRING
assert!(ai.sample_rate() > 0 && ai.sample_rate() < 100_000, ...); // pins the PROPERTYThe equality pins that Corpus census result — negative, and reported rather than droppedI said in the PR body I'd post this. The 66-title census re-run after the fix is unchanged: 29 render, 24 issue zero RDP commands — exactly the pre-fix figures. No other silent title was blocked on this latch. Worth being blunt about a mistake in how I framed that measurement: the committed census boots through A |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-audio/src/lib.rs`:
- Around line 215-229: Update docs/audio.md to document the AI reset model
represented by DEFAULT_DAC_HZ: use 44,100 Hz while AI_DACRATE is unprogrammed,
allowing the transfer FIFO to retire despite the DAC having no stopped state.
Clearly scope this as a modelling fallback for draining silence before software
programs AI_DACRATE, not a measured or observable hardware reset value.
- Around line 448-451: Update the sample-rate calculation in the register-write
handling around the video_clock and dac_rate fields so a programmed AI_DACRATE
value of literal zero computes video_clock / (0 + 1), rather than selecting
DEFAULT_DAC_HZ. Track the distinction between an unprogrammed register and an
explicitly written zero using existing state or an internal sentinel, and
preserve or explicitly migrate serialized save-state data if the state
representation changes.
- Around line 429-447: Replace the hardware-reset assertion in the DAC fallback
comments near the audio retirement logic with explicitly labelled modelling
rationale, identifying ares as reference-model evidence and primary hardware
reset semantics as undocumented. Update docs/accuracy-ledger.md at lines 413-413
to preserve the evidence trail while distinguishing ares behavior from hardware
evidence, and update CHANGELOG.md at lines 23-26 to describe the fallback as a
modelled default rather than established hardware behavior.
- Around line 243-248: Synchronize the unset-rate documentation and test naming
with the initialized DEFAULT_DAC_HZ behavior. Update the references around
sample_rate, period_ticks, tick, and idle_tick_emits_nothing_before_dacrate so
they no longer describe an unset DAC as zero or stopped, and accurately document
the first-tick anchoring rule. Preserve the existing implementation behavior.
- Around line 716-724: Update the test around the initial ai.tick call so the
scheduler is primed before measuring emitted samples, ensuring the first tick’s
anchor behavior cannot make the assertion vacuous. Then assert that drain()
produces a non-zero count within the expected default-rate range, while
retaining the upper bound that guards against 48 MHz-rate flooding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c8503ad-5985-4df9-82fe-6cb4b93da9b6
📒 Files selected for processing (3)
CHANGELOG.mdcrates/rustyn64-audio/src/lib.rsdocs/accuracy-ledger.md
…etracts a claim The 66-title census under `real_pif_boot` came back **29 render / 24 zero-command**, against 29/25 before the fix. `hle_boot` is also 29/24, so the two paths now agree EXACTLY and no title is boot-path sensitive in that partition any more. The sole regression the #202 census found is closed. RETRACTION, and it matters more than the census: #205 and R-16 said WDC gains "a real 625x237 frame". That was wrong. The scratch probe printed `scanout_scaled`'s DIMENSIONS and never counted content, and I read dimensions as a picture -- R-18's lit-pixel lesson run in reverse, inferring a frame from geometry instead of from what is in it. Measured properly: WDC issues 175,815 RDP commands (from 0, near-parity with 176,085 under HLE) and lights ZERO pixels at every 60-frame sample. The VI is genuinely programmed -- the single-shot 625x237 establishes that, so it is a real black frame, not a blanked VI -- and R-18 already recorded the same shape under HLE: "45 commands and exactly one distinct value (0x0001) -- it clears and draws nothing." So the correct claim is narrow: the AI livelock is removed and WDC's real-PIF behavior now MATCHES its HLE behavior, submitting a full command stream and rasterizing black on both. It does not render, and this fix never made it render. Why the zero-command count moves while the render count does not: WDC clears the 1,000-command floor and fails the 1,000-lit-pixel floor, which is the two-term rule working as designed. The 24-title silent cohort is UNCHANGED and still needs per-title root-causing. What closed here is the 25th. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint, check_no_roms, check_en_us -- all green.
The
|
| render | zero-command | |
|---|---|---|
hle_boot (before & after) |
29 | 24 |
real_pif_boot before this fix |
29 | 25 (+ WDC) |
real_pif_boot after this fix |
29 | 24 |
The two boot paths now agree exactly. World Driver Championship was the only title real_pif_boot made strictly worse — the finding #202's census surfaced — and it is out of the silent cohort. No title is boot-path sensitive in the render/silent partition any more.
Retraction: WDC does not gain "a real 625×237 frame"
The PR body says it does. That is wrong.
World Driver Championship rdp=175815 lit=0
175,815 RDP commands (from 0 — near-exact parity with its 176,085 under HLE) and zero lit pixels at every 60-frame sample.
The error: my scratch probe printed scanout_scaled's dimensions and never counted content, and I read the dimensions as a picture. That is R-18's lit-pixel lesson run in reverse — that row spent weeks unlearning "many lit pixels means it renders", and here I inferred a frame from geometry instead of from what was in it.
The VI is genuinely programmed, so this is a real black frame rather than a blanked VI — the single-shot 625×237 establishes that much. And R-18 already recorded the same shape for this title under HLE: "World Driver Championship 45 commands and exactly one distinct value (0x0001) — it clears and draws nothing."
So the correct claim, stated narrowly
The AI livelock is removed, and WDC's real_pif_boot behavior now matches its hle_boot behavior: it submits a full command stream and rasterizes black on both paths. It does not render, and this fix never made it render.
Why the zero-command count moves while the render count does not: WDC clears the 1,000-command floor and fails the 1,000-lit-pixel floor. That is the two-term rule working exactly as designed — the rule exists so "submits commands" and "produces a picture" cannot be confused, and here it stopped them being confused.
The 24-title silent cohort is unchanged and still needs per-title root-causing. What closed here is the 25th.
Both corrections are in docs/accuracy-ledger.md §R-16, docs/residuals/R-18.md (whose index row for WDC now reads CLOSED, with the reason), and the CHANGELOG — the retraction recorded beside the original claim rather than replacing it, per the append-only rule.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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: Use one explicit n64-systemtest statement consistently: in
docs/accuracy-ledger.md at lines 413-413, replace ambiguous “impact: none”
wording with “not measured; n64-systemtest has no AI coverage”; in CHANGELOG.md
at lines 28-32, add the same measured/not-measured statement and do not claim a
before/after failing-assertion count.
In `@docs/residuals/R-18.md`:
- Line 29: Mark the earlier contradictory WDC residual investigation text near
the document index as historical or initial investigation state, while
preserving the later corrected status and append-only content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c33d4f33-f4bd-4312-919c-2bc3e259bdc3
📒 Files selected for processing (3)
CHANGELOG.mddocs/accuracy-ledger.mddocs/residuals/R-18.md
CodeRabbit posted seven inline findings on #205; five adopted, two rejected with reasons. The two that mattered were real defects in my own tests. VACUITY (the sharpest finding, adopted). My new `set_region_before_dacrate_...` asserted `emitted < 5_000` after a SINGLE `tick` -- but the first tick returns at the `next_sample_tick == 0` anchor branch, so `emitted` was ALWAYS 0 and the bound passed with the emission path completely broken. Fixed by priming the clock and making the bound two-sided: only the pair is evidence, since an upper bound alone accepts a DAC that emits nothing, which is precisely the state that caused this livelock. Chasing that found the SAME defect in a PRE-EXISTING test, which the review did not flag: `idle_tick_emits_nothing_before_dacrate` also called `tick` once, so it too passed at the anchor branch regardless of the rate -- it passed identically before and after a change to the exact behavior it claimed to pin. Renamed to `the_first_tick_only_anchors_the_sample_clock`, re-aimed at the rule it actually exercises, and given a second half asserting that the NEXT tick does emit, without which it would pass against a DAC that never emits at all. All three audio tests are now mutation-checked together: reverting either half of the fix turns all three red. STALE CONTRACT (adopted). `sample_rate`, `period_ticks` and `tick` still documented an unset rate as zero/stopped, which the fix made false. Synchronized -- and the `period == 0` early return is now labeled as a divide-by-zero guard rather than a modeled DAC state, which is what it actually is. docs/audio.md (adopted; a rule I broke). "A chip change touches the chip code AND its docs/<chip>.md in the same commit." It did not. Added a section covering the reset model, why it is a CORRECTNESS requirement and not an audio nicety, the WDC evidence, and the known DACRATE=0 simplification. PROVENANCE (adopted). The comment asserted "hardware has no stopped-DAC state" as fact while admitting two paragraphs later that reset semantics are undocumented. Now split explicitly into ESTABLISHED (ares runs its DAC from power-on), INFERRED (a divider has no "off" encoding, so hardware likely matches), and NOT ESTABLISHED (what AI_DACRATE holds at reset). n64-systemtest wording (adopted): "impact: none" reads as a measurement that came back clean. It is "not measured, and it cannot be" -- the suite has zero AI coverage. Stated that way in both the ledger and the CHANGELOG. R-18 historical marker (adopted): a header now states that everything under `## Status` is append-only investigation history containing retracted claims, so a paragraph read in isolation is not mistaken for current status. REJECTED, with reasons: - `DEFAULT_DAC_HZ` -> `pub(crate)` "in case a diagnostic tool needs it": nothing outside the crate reads it. Widening visibility on speculation is the inert-API hazard docs/engineering-lessons.md §3.2 describes. - Repurposing spare bits in existing serialized state to smuggle in `dac_rate_programmed` without changing the layout: worse than the honest alternative. It makes the save-state format misdescribe itself, and the simplification is already ledgered with the ares line fidelity would require. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, no_std thumbv7em, markdownlint, check_no_roms, check_en_us -- all green.
Adjudication — 7 CodeRabbit findings + 3 Antigravity itemsFive adopted, two rejected with reasons. Two were real defects in my own tests, and one of those led to a third the review didn't flag. Adopted
ai.tick(0, &mut bus); // prime
ai.tick(MASTER_HZ / 60, &mut bus); // one frame
assert!((500..5_000).contains(&emitted));Only the pair is evidence: the upper bound rejects a video-clock rate, the lower bound rejects silence. Chasing it found the same defect in a pre-existing test, which this review did not flag. All three audio tests are now mutation-checked together:
Rejected, with reasons
Antigravity: repurpose spare bits in existing serialized state to avoid the layout change. Rejected, and this one I'd push back on firmly: smuggling a new field into unused bits makes the save-state format misdescribe itself. An honest layout change announced in advance is strictly better than a hidden one that no future reader can discover from the struct. Antigravity: make Antigravity nitpick: the CHANGELOG entry is verbose. Partially adopted — trimmed, with the detail pointing at R-16 and |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/residuals/R-18.md (1)
34-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse MD060’s padded table separator.
Line 35 must use
| --- | --- |;|---|---|violates the repository’s configured padded table style.Proposed fix
-|---|---| +| --- | --- |Based on learnings, the pinned hook does not enforce configured MD060 padding, so this requires manual review.
🤖 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/residuals/R-18.md` around lines 34 - 37, Update the Markdown table separator in the residuals document to use padded cells, changing the separator associated with the “Item” and “State” columns to the repository-required `| --- | --- |` style while leaving the table content unchanged.Sources: Path instructions, Learnings
🤖 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.
Outside diff comments:
In `@docs/residuals/R-18.md`:
- Around line 34-37: Update the Markdown table separator in the residuals
document to use padded cells, changing the separator associated with the “Item”
and “State” columns to the repository-required `| --- | --- |` style while
leaving the table content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06ff1410-4008-4bae-9d2d-8332f218265f
📒 Files selected for processing (5)
CHANGELOG.mdcrates/rustyn64-audio/src/lib.rsdocs/accuracy-ledger.mddocs/audio.mddocs/residuals/R-18.md
…literal Adopts all three items from the second Antigravity review on #205. 1. `new()` duplicated `recompute_rate`'s `dac_rate == 0` decision as a literal, kept correct only by a comment saying "must match `recompute_rate`" -- exactly the comment-enforced invariant this project distrusts, and the drift would be silent: a constructor left at 0 puts the machine back in the stopped-DAC state R-16's livelock needs, and no test of a *programmed* DAC would notice. `new()` now calls `recompute_rate()` (both are `const fn`, so this stays const). This measurably improves the tests: a SINGLE-POINT mutation of `recompute_rate` alone now fails four tests, where before the same defect needed both halves reverted before anything went red. 2. Added `TODO(T-AUDIO-01)` so the deferred AI_DACRATE=0 separation is discoverable from the code, not only from ledger R-16 -- it needs a save-state layout bump (ADR 0005), which is why it is deferred. 3. Added `debug_assert!(period > 0)` beside the `period == 0` guard. The comment claimed the state is unreachable on a constructed machine; the assert makes that claim CHECKABLE rather than merely stated, and a future change that reintroduces a zero rate is precisely the R-16 defect -- which hid the first time behind exactly this silent `return`. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint, check_en_us -- all green.
Adjudication — second Antigravity review: all three adopted1. You're right, and the duplicate was worse than redundant: it was kept correct only by a comment reading "must match
The concrete gain is in mutation sensitivity: One place to get wrong instead of two, and the suite now catches a single-point defect. 2. Add an explicit TODO for the deferred save-state bump. Adopted. 3. That closes every finding from both bots across three review rounds: 7 CodeRabbit + 6 Antigravity, 11 adopted, 3 rejected with reasons ( |
Antigravity review (Gemini via Ultra)This PR changes the default unprogrammed Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
1 participant
Motivation
AI_STATUS.FULLcould latch and never clear, so a game polling it for a free audio DMA slot spun forever.Found by root-causing World Driver Championship — the one title the boot-path census in #202 newly implicated: 176,085 RDP commands under
hle_boot, zero underreal_pif_boot.Measured, not guessed
The dominant retiring instruction under real-PIF (DC/WB latch, one-cycle granularity):
Then — counting transitions rather than sampling the bit, which is the sticky-register lesson this same ledger row learned the hard way on
Cause.ExcCode:hle_bootreal_pif_bootOne instantaneous
FULL=1sample would have proved nothing; both boots show it set at an arbitrary instant.Mechanism
recompute_ratemapped an unprogrammedAI_DACRATEtosample_rate = 0→period_ticks() == 0→tick()returned beforeemit_sample. Andemit_sampleis the only place a drained transfer is retired:So a stopped DAC makes the two-deep FIFO unable to advance, and
FULL(dma_count > 1) becomes permanent. WDC queues two buffers and pollsFULLbefore programming the DAC, landing exactly in that window.The fix invents nothing — and the source matters
Hardware has no stopped-DAC state to model: the DAC counter runs off the video clock from power-on whatever
AI_DACRATEholds. ares (ISC — vendorable, therefore readable underref-proj/README.md, unlike Angrylion) makes the same choice structurally:Its
sample()contains the byte-for-byte equivalent retirement block and runs from power-on unconditionally.So an unprogrammed
AI_DACRATEnow falls back toDEFAULT_DAC_HZ = 44_100, taken from ares and labeled a modeling default, not a measured value — the register's reset value is not documented in anything this project mirrors, so no rate can be derived for that window. Nothing observable should depend on the exact number, since every title programs the register before it plays anything; the constant's rustdoc says so, and says that if a future output does depend on it, that dependency is the bug.The old zero-gate was avoiding a real failure —
dac_rate == 0computingvideo_clock / 1≈ 48 MHz and flooding the sink. A default rate avoids both that and the latch.Audio::new()had to change too, and the reason is worth recording: it initializedsample_rate: 0literally, andrecompute_rateonly runs on a DACRATE or region write — so the first version of this fix left a machine that never programmed the AI with a stopped DAC anyway. The defect survived its own fix until the constructor matched. Caught by the new test failing.Measured effect
real_pif_boot)0x0FULLtransitions / 300 framesThe Phase-4 golden PCM stream is unaffected — which was the main regression risk, since this changes when the DAC emits.
Tests
full_clears_even_when_dacrate_was_never_programmed— queues two buffers with noAI_DACRATEwrite and assertsFULLclears. Mutation-checked: restoring either half of the fix (therecompute_ratebranch or the constructor) turns it red.set_region_before_dacrate_keeps_rate_zeroasserted the rate was exactly0— which over-specified its own stated purpose. Its comment says it exists to stop a ~48 MHz rate being fabricated, and that protection is preserved as an order-of-magnitude bound (0 < rate < 100_000) plus a sink-flood bound, in the renamedset_region_before_dacrate_does_not_fabricate_the_video_clock_rate. The old exact assertion is what encoded the bug as a requirement.Explicitly NOT claimed
WDC now renders but ends its run in a
B -1self-loop at0x8000_28C0— a different halt, reached after 120k commands. This closes the AI livelock, not that title.n64-systemtest impact: none. The suite has zero AI coverage, so the count stays at 90. This fix is validated by ares' structure, the mutation-checked unit test, and WDC's measured unblocking — not by the oracle. Ledger R-16 says so in those words.
A corpus-wide census is running to measure whether any other silent title was blocked on the same latch; the result will be posted here and folded into R-16 before merge.
Gates run locally
cargo fmt --all --check·cargo clippy --workspace --all-targets -- -D warnings·cargo test --workspace(810) ·RUSTDOCFLAGS="-D warnings" cargo doc·no_std thumbv7em·markdownlint·check_no_roms.sh·check_en_us.sh— all green.🤖 Generated with Claude Code