docs(residuals): the real-PIF re-census refutes my own boot-path concern - #202
Merged
Conversation
Ran the census through both boot paths over all 66 titles, 420 frames each, same two-term rule: === 66 titles: HLE renders 29, real-PIF renders 29 === GAINED under real-PIF (0): LOST under real-PIF (0): real-PIF boot setup failed (0): The boot path makes no difference to the render count and the rendering set is identical. So the "24 silent titles" figure is NOT an artefact of hle_boot, and the warning I added in the previous commit -- that it was "a count under the HLE boot path, not a count of emulation defects" -- was overstated. Retained above as written per the append-only rule: the reasoning was sound, the conclusion was wrong, and the error was generalising from one title to the corpus. What the paths do differ on: titles rendering 29 -> 29 titles issuing zero RDP commands 24 -> 25 zero only under one path -> World Driver Championship DK64 remains boot-path-sensitive but still does not render: under the real PIF it clears its check, programs VI_CTRL = 0x0320E and scans out 625x237 with ~13.5k lit pixels, versus never programming the VI at all under HLE -- but it issues zero RDP commands either way, so it fails the two-term rule on both paths. Lit pixels with no RDP work behind them is the Rayman 2 / Namco Museum signature and needs the look-at test before it counts as progress. World Driver Championship REGRESSES under the real PIF: 176,085 RDP commands to zero. It rendered nothing on either path so the headline count is unaffected, but it is a real defect in the real-PIF path and the one title where HLE is strictly better. Everything else is timing noise -- 0.7-1.0% command-count deltas with identical lit-pixel totals, i.e. the extra IPL1/IPL2 execution shifting sample points. Consequence: the 24 silent titles are genuinely silent under both paths, so each does need its own root-cause hunt. The hoped-for shortcut does not exist. The hle_boot caveat is withdrawn and STATUS's figures stand as written. Gates: fmt, cargo test --workspace, markdownlint -- green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughR-18 records a completed re-census across ChangesR-18 boot-path re-census
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@docs/residuals/R-18.md`:
- Around line 185-188: Qualify the attribution in the “Everything else is timing
noise” statement as an inference rather than an established cause. Retain the
measured command-count and lit-pixel data, but avoid asserting that extra
IPL1/IPL2 execution caused every delta unless the required correlated
differential evidence is documented.
- Around line 190-193: Update the silent-cohort conclusion in the “Consequence
for the plan” section to state that the original 24 common zero-RDP titles
remain unchanged, while World Driver Championship is an additional regression
affecting only real-PIF boot. Preserve the existing conclusion that each silent
title requires its own root-cause investigation and that the hle_boot caveat is
withdrawn.
- Line 149: Update the fenced code block at the indicated location in R-18.md by
adding the text language tag to its opening fence, preserving the block’s
contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67dc2295-5653-451a-b41b-54090444fedb
📒 Files selected for processing (1)
docs/residuals/R-18.md
Adopts three CodeRabbit findings and one from Antigravity. The timing attribution was an inference presented as a finding. What is measured is that the remaining deltas are 0.7-1.0% of the command count with identical lit-pixel totals; that this is caused by extra IPL1/IPL2 execution shifting sample points is the plausible reading, not something these runs isolate. Establishing it needs a differential trace on one title, which has not been run. Marked as inference, with the reason: "it is only timing" is exactly the kind of dismissal that, taken on faith, hides a real per-title divergence inside a small percentage. The cohort arithmetic was ambiguous. Stated explicitly now: the original 24-title silent cohort is UNCHANGED -- all 24 issue zero RDP commands under both paths and none left the cohort -- and the 25th is World Driver Championship, an additional real-PIF-only regression rather than a member of that cohort. WDC is now a tracked work item instead of "wants its own investigation", with the method named. MD040: the result fence is tagged `text`. Rejected: "generalising" -> "generalizing". The repository is consistently en-GB -- 37 "behaviour" to 3 "behavior", 41 "modelled" to 0 "modeled" -- so en-GB is the convention and the suggestion inverts it. Gates: fmt, cargo test --workspace, markdownlint -- green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
Adjudication — review roundCI 10/10. Three CodeRabbit findings adopted, one Antigravity finding adopted, one rejected on evidence. Qualify the timing attribution — adopted, and it was the right catchI wrote "Everything else is timing noise … which is the extra IPL1/IPL2 execution shifting the sample points". That is an inference stated as a finding. What these runs measure is that the deltas are 0.7–1.0 % with identical lit-pixel totals; the causal attribution is the plausible reading of that, not something isolated per title. Rewritten to say so, with the reason recorded: "it is only timing" is exactly the kind of dismissal that, taken on faith, hides a real per-title divergence inside a small percentage. Establishing the cause would need a differential trace on one title, which has not been run — and saying that is more useful than a confident sentence. Clarify the silent-cohort conclusion — adoptedThe arithmetic was genuinely ambiguous: 24 → 25 could read as "the cohort grew". It did not. Now explicit:
|
Antigravity review (Gemini via Ultra)This PR updates Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
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 (3)
docs/residuals/R-18.md (3)
146-154: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd reproducible provenance for the census.
The 66-title/420-frame results are presented as measured, but the record does not identify the corpus revision, exact harness invocation, or retained output/trace used to produce them. Add that provenance so the counts can be independently checked.
🤖 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 146 - 154, Add reproducible provenance to the census section in R-18.md: identify the exact corpus revision, provide the complete harness invocation for both boot paths, and reference the retained output or trace artifact used to generate the 66-title/420-frame counts. Keep the reported results unchanged.Sources: Coding guidelines, Path instructions
156-163: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve superseded material as historical.
Residual records are append-mostly: retain the earlier caveat and tracked re-run action, mark them as
historical/initial investigation state, and append the dated resolution. “Retained above as written” does not provide that historical label, and the new consequence replaces the previous OPEN ACTION.Also applies to: 204-207
🤖 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 156 - 163, Update the residual record around the retained caveat and tracked re-run action, including the corresponding section at lines 204-207, to label both as “historical/initial investigation state.” Append the dated resolution after the superseded material, and replace the previous OPEN ACTION with the new consequence while preserving the earlier text unchanged.Sources: Path instructions, Learnings
185-192: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCall this an observed boot-path regression, not a real defect. Per the discrepancy rule in the coding guidelines, it needs the reference-emulator result and reference-independence note before it can be labelled a defect.
🤖 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 185 - 192, Revise the World Driver Championship entry to classify the result as an observed boot-path regression rather than a real defect. Remove the current defect wording and standalone work-item framing, and add the reference-emulator result plus a reference-independence note required by the discrepancy rule.Source: Coding guidelines
🤖 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/residuals/R-18.md`:
- Around line 193-202: Update the measured command-count comparison in the
residuals discussion to explicitly classify the delta as absolute or
differential, while retaining the existing statement that no differential trace
has been run. Keep the timing attribution clearly labeled as an inference.
---
Outside diff comments:
In `@docs/residuals/R-18.md`:
- Around line 146-154: Add reproducible provenance to the census section in
R-18.md: identify the exact corpus revision, provide the complete harness
invocation for both boot paths, and reference the retained output or trace
artifact used to generate the 66-title/420-frame counts. Keep the reported
results unchanged.
- Around line 156-163: Update the residual record around the retained caveat and
tracked re-run action, including the corresponding section at lines 204-207, to
label both as “historical/initial investigation state.” Append the dated
resolution after the superseded material, and replace the previous OPEN ACTION
with the new consequence while preserving the earlier text unchanged.
- Around line 185-192: Revise the World Driver Championship entry to classify
the result as an observed boot-path regression rather than a real defect. Remove
the current defect wording and standalone work-item framing, and add the
reference-emulator result plus a reference-independence note required by the
discrepancy rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0223f549-e50f-4449-8c7b-d320d7f60989
📒 Files selected for processing (1)
docs/residuals/R-18.md
doublegate
added a commit
that referenced
this pull request
Jul 30, 2026
* chore: write the repo in en-US, and gate it so it stays that way The project standard is en-US. The tree had drifted to en-GB -- and I made that worse: on #202 I rejected a reviewer's generalising -> generalizing suggestion on the measured grounds that the repo was consistently en-GB (37 "behaviour" to 3). The measurement was right and the conclusion was backwards: consistency with a drift is not a reason to keep drifting. 1,505 replacements across 131 files, from 51 stem-level pairs applied case-preservingly, so every inflection of a stem is covered by one entry. Touches prose, doc comments, assertion messages, and private test-function names. NO public item is renamed, so there is no API change -- checked by enumerating `pub fn|struct|enum|const|static|trait|type|mod` names against the stem list and finding none. Also checked in advance: - no CI job or script filters on an affected test name - no excluded directory cites an affected test name, so swept and unswept text cannot desync - no verbatim external quotation is altered (searched for quoted phrases beside a citation marker; the one hit was our own italic emphasis, not a quote) Excluded trees, and why: ref-docs/ immutable research corpus -- corrections land as new dated supplemental files, never in-place rewrites (module 40) n64brew_wiki/ CC BY-SA mirror, quoted verbatim by design ref-proj/ study clones of other emulators third_party/ vendored libdragon; not our prose to edit Two literals are preserved because they are values, not words: `lightgrey` in a shields.io badge URL (a color PARAMETER), and GitHub Actions' own `cancelled` run status where the CHANGELOG quotes it. Stems whose form is correct in en-US too were deliberately left out of the list -- analysis, synthesis, hypothesis, peripheral, exercise, precise, premise, promise, otherwise, likewise, bitwise, controlled, installed, stalled -- since a stem there would corrupt a correct word. Adds scripts/check_en_us.sh and an `en-US spelling` CI job, because a one-off sweep decays: nothing fails when one en-GB form returns, so it survives review and the next has precedent. This repo already demonstrates the pattern -- markdownlint runs in pre-commit only, and an MD040 violation sat unnoticed on main until #203. The gate is mutation-checked in BOTH directions (fails on the unswept tree at 1,355 lines, passes on the swept one at 443 files), strips allowed literals rather than skipping their whole line so a permitted value cannot shield a real hit beside it, and documents a per-line `spell-exempt` escape hatch. Also wired into pre-commit for a faster local loop, with CI as the unskippable gate. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, no_std thumbv7em, markdownlint, check_no_roms, check_en_us -- all green. * fix(en-us): repair 4 malformed words the sweep produced, and gate the class Adjudicates the Antigravity review on #204, which caught one of four corruptions. A before/after word-pair audit over the whole diff caught all four, and it is the check I should have run before committing the sweep: characteris -> characteriz corrupted characteristics -> characteriztics optimis -> optimiz corrupted optimistic -> optimiztic realis -> realiz corrupted realistically -> realiztically centre -> center corrupted centred -> centerd All four are stems matching INSIDE a word already correct in en-US. The gate passed on the corrupted tree, and correctly so -- the outputs are not en-GB either, so an en-GB pattern cannot see them. The same defect was in the gate itself, in both directions: bare -is stems made it report `characteristics`, `optimistic` and `realistically` as violations. Split the stem list in two -- PLAIN_STEMS matched as substrings, ISE_STEMS requiring an en-GB verb/noun ending -- so `optimis+tic` no longer matches while `optimis+ation` still does. Also: - Added a MALFORMED backstop list so these specific non-words cannot return, with a comment saying plainly that it is a backstop for forms already seen and NOT a substitute for auditing a future sweep's word pairs. - Added `cancelled`/`cancelling` and `centring` as stems. Their omission left real en-GB spellings unswept -- `the_sign_of_a_cancelled_zero_...` (renamed) and two prose sites -- while the value references keep the backticked `cancelled` form that names GitHub Actions' literal status. - `centring` and `cancelling` are listed separately because neither CONTAINS its sibling stem, which is how a stem list silently under-matches. The gate is now mutation-checked three ways: it catches an injected en-GB word, catches an injected malformed word, and does NOT flag optimistic / realistically / characteristics / synthesis / analysis. Antigravity also reported scripts/check_en_us.sh as missing from the patch; rejected as incorrect -- `git ls-files` lists it and the `en-US spelling` CI job PASSED on this PR, which is impossible if the script were absent. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, no_std thumbv7em, markdownlint, check_no_roms, check_en_us -- all green.
doublegate
added a commit
that referenced
this pull request
Jul 30, 2026
…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.
doublegate
added a commit
that referenced
this pull request
Jul 30, 2026
* fix(ai): an unprogrammed AI_DACRATE must not stop the DAC (R-16)
`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.
* docs(r16): adjudicate review — ledger the DACRATE-zero conflation
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.
* docs(r16): the real-PIF census closes the boot-path divergence, and retracts 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.
* fix(ai): adjudicate 7 review findings — two of my tests were vacuous
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.
* refactor(ai): derive the power-on rate through recompute_rate, not a 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.
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
1 participant
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.
Ran the census the previous entry demanded. It refutes the concern I raised.
Result
Full corpus, both boot paths, 420 frames each, same two-term rule (≥1,000 RDP commands and ≥1,000 lit pixels):
The boot path makes no difference to the render count, and the rendering set is identical.
So the "24 silent titles" figure is not an artefact of
hle_boot. The warning I added one commit earlier — that it was "a count under the HLE boot path, not a count of emulation defects" — was overstated. It stays in the record as written (append-only), because the reasoning was sound and only the conclusion was wrong: DK64 is boot-path-sensitive, and generalising from one title to the corpus was the error.What the paths actually differ on
VI_CTRL = 0x0320Eand scans out 625×237 with ~13.5k lit pixels — versus never programming the VI at all under HLE. But zero RDP commands either way, so it fails the two-term rule on both paths. Lit pixels with no RDP work behind them is the Rayman 2 / Namco Museum signature; that frame needs the look-at test before it counts as progress.Consequence for the plan
The 24 silent titles are genuinely silent under both boot paths, so each does need its own root-cause hunt. The shortcut I hoped for does not exist, and saying so is more useful than leaving a plausible caveat standing that would have shaped the next 23 investigations.
docs/STATUS.md's figures stand as written; thehle_bootcaveat is withdrawn.Gates
cargo fmt --all --check·cargo test --workspace· markdownlint — green. Docs only.