-
-
Notifications
You must be signed in to change notification settings - Fork 2
chore(release): cut v2.4.2 "Cairn" — the rung-0 compare surface #438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
2d919f3
chore(release): cut v2.4.2 "Cairn" -- the rung-0 compare surface
doublegate 78216be
docs: address the review -- and one finding is an arithmetic error, n…
doublegate 0e6609d
test(release): gate ordinary status prose, not just the pinned anchors
doublegate File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,247 @@ | ||
| ## v2.4.2 "Cairn" — the rung-0 compare surface | ||
|
|
||
| A cairn is a marker set along a route so that you can tell you are still on it. | ||
| That is what a rolling per-cycle hash checkpoint is, and building the compare | ||
| surface is what this release does. | ||
|
|
||
| RustyNES itself is unchanged. **The emulation core is untouched** — no behaviour | ||
| change to `rustynes-{cpu,ppu,apu,mappers,core}`, no new hot-path API, and | ||
| `crates/rustynes-cosim` stays outside the default build. AccuracyCoin remains | ||
| **141/141 (100.00%, RAM decoder)** and nestest 0-diff. This is scaffolding for | ||
| the v2.4.1 → v2.5.0 "Fabric" line, in which a **new** NES core written in | ||
| SystemVerilog from public hardware documentation is verified against this | ||
| emulator. RustyNES is not being ported to FPGA and cannot be. | ||
|
|
||
| --- | ||
|
|
||
| ### The number that decided the design | ||
|
|
||
| The constraint nobody budgets for in co-simulation is trace **volume**, not | ||
| simulation time. It is now measured rather than projected: | ||
|
|
||
| | 3 frames of AccuracyCoin | 89,343 CPU cycles | | ||
| |---|---| | ||
| | `irq.csv` (full per-cycle capture) | **5,372,427 bytes** | | ||
| | `ckpt.bin` (4096-cycle checkpoints) | **352 bytes** | | ||
| | ratio | **15,263x** | | ||
|
|
||
| Extrapolated, a 4200-frame AccuracyCoin run is roughly 125 M cycles — about | ||
| 7.5 GB of CSV against **roughly 480 KB** of checkpoints. | ||
|
|
||
| > **A figure carried since v2.4.1 is corrected here.** The plan and v2.4.1's | ||
| > notes said **244 KB**, which is 30,518 checkpoints at **8** bytes each — a | ||
| > record holding only the hash. The implemented record is `ENCODED_LEN = 16` | ||
| > bytes, cycle *and* hash, so the real figure is 125,000,000 / 4096 x 16 = | ||
| > **477 KB**, and scaling the measured 352 bytes by 125,000,000 / 89,343 agrees | ||
| > at **481 KB**. The ratio is unaffected; only the absolute extrapolation was | ||
| > wrong, and it was wrong by about 2x. `AGENTS.md`, `docs/mister.md` and | ||
| > `to-dos/ROADMAP.md` are corrected in this release. The published v2.4.1 notes | ||
| > and ADR 0037 are left as they stand — a shipped record is not silently | ||
| > rewritten — so this note is where the correction lives. So both sides chain a 64-bit hash | ||
| over the per-cycle tuple, compare at intervals, and only the divergent window is | ||
| re-run with full capture and waveforms. | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| The hash is **FNV-1a 64 for exactly one reason: a C++ testbench can reimplement | ||
| it without a library.** The top risk at this rung is a format-packing mismatch | ||
| masquerading as an RTL bug, so `encode` fixes a 16-byte little-endian layout with | ||
| an explicit zero pad byte — the C++ side cannot hash uninitialised struct padding | ||
| — and both the layout and the hash are pinned to a hardcoded vector. A reordered | ||
| field fails that test rather than producing a phantom RTL defect. | ||
|
|
||
| ### What is hashed is a decision about hardware, not about convenience | ||
|
|
||
| `CycleRecord` carries **29 fields** and most of them are RustyNES's *model*: | ||
| `dmc_abort_delay_post`, `apu_phase_post`, `dma_cycles_owed`. Gating on those | ||
| would force an independent implementation to transliterate a Rust data structure | ||
| — bad hardware, and on a programme built on never reading a reference | ||
| implementation, an odd form of self-derivation. | ||
|
|
||
| `Observable` is the subset a device-under-test can genuinely produce. | ||
| `from_cycle_record` is the single place the partition is applied, and a test | ||
| perturbs **every** dropped field at once and asserts the hash does not move — | ||
| with its converse, so it cannot pass by dropping everything. | ||
|
|
||
| Two subsets needed their caveats stated rather than buried: | ||
|
|
||
| - **The IRQ line is one wire.** `CycleRecord` attributes each sample to the | ||
| mapper or the APU; hardware has a single wire-OR'd /IRQ pin that cannot. The | ||
| pairs are OR'd before hashing, because hashing them apart would fail a correct | ||
| DUT for disagreeing about something it is not able to observe. | ||
| - **`pc` is DUT-observable, not pin-observable.** It is included because a | ||
| testbench wrapper can expose the register, but a `pc`-only mismatch means | ||
| something weaker than a bus mismatch, and the code says so. | ||
|
|
||
| `a12_events` is excluded for **scope**, not observability — A12 transitions | ||
| genuinely are visible on the cartridge connector — and becomes a gate when the | ||
| PPU rung opens. | ||
|
|
||
| ### The acceptance gate is executable, and it found a defect | ||
|
|
||
| Checkpoints are an **approximation** of "where do these two runs first differ", | ||
| traded for four orders of magnitude of disk. The scheme is worthless if the | ||
| approximation can disagree with the answer, so `first_full_capture_difference` | ||
| computes the answer directly and `localisation_is_consistent` states the contract | ||
| the approximation must honour — as a function, rather than as prose in a plan. | ||
|
|
||
| The contract is narrow on purpose, because a looser reading is satisfiable by a | ||
| broken implementation: | ||
|
|
||
| - Identical streams must report `Identical`. A **false positive** gets a gate | ||
| switched off. | ||
| - A real difference must never report `Identical`. A **false negative** passes a | ||
| wrong DUT. | ||
| - When it reports a divergence, **the named window must contain the difference.** | ||
| A report naming the wrong window sends a full-capture re-run somewhere nothing | ||
| is wrong, spends the debugging budget, and returns "no problem here" — which | ||
| reads as evidence the DUT is fine. | ||
| - `Inconclusive` is acceptable for a real difference and never for identical | ||
| streams. | ||
|
|
||
| A sweep drives **331 cases**: every run length around the interval boundary | ||
| (1, 2, 4095, 4096, 4097, 8192, 8193, 10 000, 12 288), a corruption at every | ||
| position for short runs and a randomised sweep for long ones, and a different | ||
| observable field perturbed each time so it cannot silently be exercising one | ||
| field. Both the gate predicate and the sweep are demonstrated to fail: a | ||
| one-character mutation to `Divergence::contains` reddens two tests. | ||
|
|
||
| **The defect it found: a divergence at cycle zero was reported in a window that | ||
| did not contain it** — at `len = 1`, the degenerate case a hand-written test set | ||
| omits. `Divergence::after_cycle` was a `u64` in which `0` meant both "no prior | ||
| checkpoint" and "cycle zero", so the first window read as `(0, 0]`, which is | ||
| empty. A full-capture re-run of it would have found nothing, and "nothing found" | ||
| reads as evidence the DUT is fine. | ||
|
|
||
| It is now `Option<u64>`, which removes the sentinel collision rather than | ||
| special-casing it. `Divergence::contains` is offered so call sites do not | ||
| reimplement a boundary that is half-open at one end and open-ended at the other, | ||
| and `window_len` returns `Option<u64>`: for the first window the span begins | ||
| wherever the run began, and a checkpoint stream carries no evidence that it began | ||
| at cycle 0 — so the honest answer is "unknown", not an assumed `through_cycle + 1`. | ||
|
|
||
| ### `<stem>.obs.bin` — because the CSV cannot re-derive the checkpoints | ||
|
|
||
| Found by trying to build the rung-0 self-diff on the CSV. `irq.csv` carries | ||
| **23 columns** and neither `pc` nor `put_cycle_post` is among them, so two of the | ||
| nine observable fields are simply absent. An external testbench reading the CSV | ||
| therefore cannot reproduce the checkpoint hashes, and the rung-0 gate — feed | ||
| RustyNES's golden back in as if it were the DUT and get zero divergences — was | ||
| **not implementable as designed**. | ||
|
|
||
| The new golden is repeated 16-byte records in the same wire encoding the hash | ||
| folds, headerless. It is the only artifact the checkpoints can be independently | ||
| re-derived from, and it is also the input a re-run of a located window consumes, | ||
| so it would have been needed regardless. Additive: the CSV is untouched, which | ||
| matters because `scripts/irq_trace_cross_diff.py` and the committed | ||
| `golden/irq_trace/*.csv` both depend on its shape. | ||
|
|
||
| `Observable::decode` is the inverse and **refuses what it does not understand** | ||
| — a non-zero reserved pad byte, an undefined flag bit, an unknown bus-access | ||
| code, a short record — and `observables_from_bytes` rejects a stream whose length | ||
| is not a multiple of 16 before it decodes any record at all. Reading a | ||
| record from a newer producer as though nothing had changed is how a *format* | ||
| divergence gets reported as a *DUT* divergence. | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| Measured across the repository boundary rather than only in unit tests. (The | ||
| count is **89,335**, eight fewer than the 89,343 cycles above: `Nes::from_rom` | ||
| runs the eight-cycle reset before the trace is armed, so the stream covers | ||
| cycles 8..89,342. Nothing is filtered — the checkpoints hash exactly the | ||
| retained sequence.) | ||
| **89,335 records** of AccuracyCoin, re-derived in C++ from `.obs.bin` alone, | ||
| hashing to **byte-identical** checkpoints — and a one-bit corruption at the | ||
| halfway record located to the 4096-cycle window containing it, in the same | ||
| invocation, because a positive control alone is satisfiable by a comparison that | ||
| always agrees. | ||
|
|
||
| ### The `CpuBootTrace` wire layout is pinned on both sides | ||
|
|
||
| The testbench writes this format so `cpu_boot_trace_diff` — already written, | ||
| already tested, already used against a third-party reference emulator — reads a | ||
| device-under-test's output with **no modification at all**. Demonstrated rather | ||
| than asserted: a C++-written 256-record trace loads and reports "All 256 aligned | ||
| records match", and a corrupted register is located at `cyc=307 PC=$C064 A=$64` | ||
| vs `A=$9B`. | ||
|
|
||
| Both sides are anchored to the **same hardcoded bytes** rather than to each | ||
| other, so a drift on either fails its own test instead of the two quietly | ||
| agreeing on something wrong — or disagreeing at co-simulation time, where a | ||
| format difference is indistinguishable from a DUT defect. | ||
|
|
||
| The pinned record uses **`scanline = -1`** deliberately: the pre-render line is | ||
| negative, and a writer that clamped or saturated rather than writing two's | ||
| complement would pass every test that only ever used a positive scanline. | ||
|
|
||
| --- | ||
|
|
||
| ### Ordinary status prose is gated too | ||
|
|
||
| `release_anchor_audit` pins 15 fixed markers and fails closed on a missing one — | ||
| which is why a doubled bold marker on this very cut failed loudly instead of | ||
| silently checking 14 of 15. It cannot cover prose with no marker, and review | ||
| found that **the drift did not stop when the anchors were gated; it moved into | ||
| the prose beside them.** `to-dos/ROADMAP.md` announced "Next up — v2.4.0 <!-- release-state: not-a-claim --> | ||
| Concordance" for a release that shipped inside v2.4.1 and is deliberately never | ||
| tagged — in the very change documenting why it has no tag — and | ||
| `to-dos/README.md` read "In development — v1.8.9", roughly fifteen releases <!-- release-state: not-a-claim --> | ||
| stale, which nothing had flagged at all. | ||
|
|
||
| `release_state_prose_audit.rs` is the pattern counterpart: a status label | ||
| (`Next up`, `In development`, `Planned for`, `Upcoming`) naming a version at or | ||
| below the workspace version is a contradiction needing no judgement to detect. | ||
| Discovery is `git ls-files` intersected with `.markdownlintignore`, so it needs | ||
| no exclusion list of its own. | ||
|
|
||
| A companion rule — "a `latest release` claim must name the workspace version" — | ||
| was **drafted and rejected on measurement**: it fires on `docs/ios.md`, which | ||
| correctly scopes "The current line" to the iOS train. A gate with false positives | ||
| gets switched off, so it is recorded as a measured rejection rather than tuned. | ||
|
|
||
| And **mutation testing changed the design**. The escape hatch was first any line | ||
| containing "historical" — and the first line written against it said "a | ||
| historical snapshot" for unrelated reasons, so a mutation reintroducing the exact | ||
| defect came back NOT CAUGHT, exempted by the prose beside it. The marker is now | ||
| `<!-- release-state: not-a-claim -->`, which cannot be claimed by accident. The rename came from the gate failing on its own documentation: explaining the defect requires quoting it, and a quotation is not a historical snapshot — so the marker says the line is not making a claim, which is true of both. | ||
|
|
||
| --- | ||
|
|
||
| ### Fixed | ||
|
|
||
| **The excluded crate's lockfile was silently gitignored, so CI re-resolved it on | ||
| every run.** `.gitignore` carries a bare `Cargo.lock` — which matches at any | ||
| depth — paired with a `!/Cargo.lock` re-include naming only the workspace root. | ||
| That was written when there was exactly one lockfile. Excluding `rustynes-cosim` | ||
| from the workspace in v2.4.1 gave it its own resolve and its own lockfile, which | ||
| the bare rule then ignored. | ||
|
|
||
| It matters more here than for an ordinary crate: this crate emits the goldens an | ||
| external NES implementation is verified against, and its manifest records the | ||
| *emulator* version rather than the dependency resolve — so a dependency moving | ||
| underneath it would be invisible in exactly the artifact whose job is to | ||
| establish provenance. The lockfile is now committed, and `cosim_manifest_audit.rs` | ||
| asserts it is **tracked** rather than merely present, demonstrated to fail by | ||
| un-tracking it and re-running. | ||
|
|
||
| --- | ||
|
|
||
| ### Verification | ||
|
|
||
| - **AccuracyCoin 141/141 (100.00%, RAM decoder)**; nestest 0-diff. The core is | ||
| untouched, but both were run rather than assumed. | ||
| - `crates/rustynes-cosim` remains excluded from the workspace and absent from | ||
| the default build; `cosim_manifest_audit.rs` asserts both. | ||
| - Every new gate demonstrated to fail by mutation before being trusted. | ||
|
|
||
| ### Compatibility | ||
|
|
||
| Additive and default-off throughout. No save-state, movie, netplay, or | ||
| public-API change **in any shipped or default-build package**; `.rns` and `.rnm` | ||
| formats are unchanged. `rustynes-cosim` does add public Rust and C ABI surface, | ||
| but it is excluded from the workspace and from the default build, so it reaches | ||
| no shipped binary. | ||
|
|
||
| ### A note on v2.4.0 | ||
|
|
||
| There is no `v2.4.0` tag and there will not be one. The workspace version was | ||
| never `2.4.0` on any commit — it went `2.3.9` → `2.4.1` — and v2.4.0 | ||
| "Concordance" shipped **inside v2.4.1**, which its notes and `VERSION-PLAN.md` | ||
| both record. A skipped version number is permitted by SemVer; a tag pointing at a | ||
| tree that calls itself something else is not honest. | ||
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.