fix(vrc7): carry the live OPLL in the save state so rewind resumes the music - #398
Conversation
…e music Closes the most actionable open row in `docs/accuracy-ledger.md`, open since v2.2.3 and latent since the ADR 0006 VRC7 audio landing. THE DEFECT `Vrc7::save_state` wrote the *shadow* OPLL register bytes — `addr_latch`, `data_latch`, `silenced`, `regs[0..64]` — and never the live synthesizer: not `self.opll`, not `opll_clock_counter`, not `last_opll_sample`. `load_state` restored those shadow bytes and never replayed them into the chip either. So after a rewind, a netplay rollback, or a TAS/save-state restore, the FM voice resumed from whatever envelope and phase state it happened to be holding — an arbitrary point in an unrelated note. Banking, IRQ, mirroring and PRG-RAM had always round-tripped correctly; this was audio-only, and only on mapper 85 with `mapper-audio` on. It was not serializable at the time it was found: `rustynes_apu::Opll` exposed no serialization surface at all, and its envelope generators, phase accumulators and LFO are private. That is what made this "a change of its own" rather than a release-cut drive-by, and it is what this commit does. THE SNAPSHOT SURFACE `Opll::snapshot` / `Opll::restore`, versioned by `OPLL_SNAPSHOT_VERSION` with a fixed `OPLL_SNAPSHOT_LEN`. Carried: the 64-byte register shadow, the current register address, the test flag, the key-status mask, the EG counter, both LFO phases and the AM output, the per-channel patch numbers, the user patch pair (writeable through `$00-$07`), all 18 operator slots in full — phase accumulators, envelope state machines, feedback history, the derived TLL/RKS and rate fields, and the pending-update mask — plus the per-channel outputs and the mix. NOT carried, because they are constants of construction and serializing them would be writing a copy of the binary into the save file: `waves` (the 1024-entry sine / half-sine tables), `tll_rks` (~128 KiB of TLL + RKS tables), and `patch_set[2..]` (the chip's patch ROM, selected by `chip_type`). `chip_type` itself is written as a tag ONLY, never assigned from — its job is to reject a cross-chip restore, since a YM2413 blob loaded into a VRC7 is structurally valid in every field and would silently reinterpret all 18 slot patches against the wrong instrument set. The reader is bounds-checked on every field and decodes the entire blob into locals BEFORE touching `self`, so a truncated or hand-edited save leaves the synthesizer on its previous state rather than half-overwritten — this parses untrusted input, and the caller reports the error and keeps running. Enum tags (`EgState`, `ChipType`) are explicit `to_tag`/`from_tag` mappings rather than `as u8`, so reordering a variant cannot silently reinterpret existing states. Its error type is deliberately NOT `ApuSnapshotError`: the blob rides in the *mapper* section of whichever board carries the chip, which is versioned independently of `APU_SNAPSHOT_VERSION`. Sharing the type would assert a coupling between two schemas that must be free to move apart. THE MAPPER SECTION VRC7 goes to section **v2**, appending `opll_clock_counter` (u16), `last_opll_sample` (i16) and the OPLL blob after the VRAM. Additive: `load_state` still accepts v1, and a v1 blob leaves the synthesizer untouched — which is precisely the pre-fix behaviour, so an old save is no worse than it always was rather than newly silent. A build without `mapper-audio` has no synthesizer to describe, so it writes v1 and validates-then-ignores a v2 tail. That keeps the property this crate's feature documentation and ADR 0004 promise in both directions — an audio build's save loads in a no-audio build, and a no-audio build's save loads everywhere — which is why `VRC7_SECTION_VERSION` is build-dependent rather than a flat 2. THE ALTERNATIVE, AND WHY IT LOST Replaying `audio.regs` through `Opll::write_reg` on load needs no new format and is the repair a reader thinks of first. It restarts every keyed-on channel's envelope at attack, so every rewind frame produces an audible transient. The ledger recorded that there was no oracle to adjudicate which was worse; carrying the state verbatim removes the question, because it reproduces the sound that was actually playing. TESTS `vrc7_save_state_carries_the_live_opll_so_audio_resumes_identically` keys a note on channel 0 with a real melodic patch, advances 20,000 CPU cycles so the envelope is well past attack and the phase accumulators hold values no reset could coincidentally match, saves, then compares 4,000 mixed samples from the source against 4,000 from a FRESH mapper restored from the blob — equal sample for sample. Mutation-checked: making the tail carry a *reset* synthesizer reproduces the pre-fix failure on the first divergent sample. Plus, at the `Opll` level: a 2,000-sample stream round-trip into a fresh chip; snapshot -> restore -> snapshot byte idempotence (which catches a field written but not read back, a case the stream test can miss); cross-chip rejection; truncation rejection asserting the target is left UNMUTATED; unknown-version rejection; and a corrupt envelope-state tag. At the mapper level: a v1 back-compat load and a truncated-v2-tail rejection. `Opll` is now registered in `snapshot_schema_audit.rs`. That audit — the standing field-vs-schema check that found the v2.2.3 PPU and APU gaps mechanically — knew only about the three chips inside the console, so it could not see this surface at all. A save-state surface no audit can see is exactly how a gap this size survives four releases, so the surface is registered in the same change that creates it. Registering it immediately caught two of my own exclusion entries as false admissions (`chip_type` and `patch_set` ARE written), which is the audit working. Out of scope, checked and left alone: `nsf_expansion.rs` holds an `Opll` too and also carries no phase, but that is a written decision with a stated rationale (an NSF driver re-establishes channel state on the next play call), not an undocumented gap. VERIFICATION Nothing on the synthesis path moved, so emulation output is unchanged — but this touches `rustynes-apu` and `rustynes-mappers`, so the contract was verified rather than assumed: AccuracyCoin **141/141 (100.00%)** via the authoritative RAM decoder, nestest 0-diff. Workspace clippy, `mapper-audio`-off clippy, rustdoc with warnings denied, the `no_std` cross-build, and 124 workspace test binaries all green.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a determinism/save-state gap for the VRC7 (mapper 85) FM audio path by snapshotting/restoring the live OPLL synthesizer state, so rewind/rollback/TAS restore resumes VRC7 music sample-identically instead of restarting from an arbitrary envelope/phase.
Changes:
- Add
Opll::snapshot/Opll::restorewith a fixed-size, versioned blob and robust validation to safely parse untrusted save-state bytes. - Extend the VRC7 mapper save-state section to v2 (appending OPLL runtime state + timing counters) and add regression tests for sample-identical resume plus back-compat.
- Register OPLL in the snapshot schema audit and update the accuracy ledger + changelog entry to reflect the remediation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/accuracy-ledger.md | Marks the VRC7 OPLL save-state continuity item as remediated and documents the chosen approach + tests. |
| crates/rustynes-test-harness/tests/snapshot_schema_audit.rs | Adds OPLL to the schema audit so future fields must be classified (derived/config/gap). |
| crates/rustynes-mappers/src/m085_vrc7.rs | Adds v2 tail to mapper 85 save state to carry live OPLL state; adds targeted tests. |
| crates/rustynes-apu/src/opll.rs | Implements the OPLL snapshot/restore schema and extensive validation + unit tests. |
| crates/rustynes-apu/src/lib.rs | Re-exports OPLL snapshot constants and error type for downstream use. |
| CHANGELOG.md | Documents the user-visible fix in the Unreleased “Fixed” section. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review found a real defect in the previous commit, and the reason no test could
have caught it is the more useful half.
THE DEFECT
`load_state` gated its accept set on `VRC7_SECTION_VERSION`:
if version != 1 && version != VRC7_SECTION_VERSION { reject }
That constant is the version this build WRITES, and it is deliberately **1** on
a `mapper-audio`-off build (no synthesizer to describe). So on that build the
condition collapsed to `version != 1` and a v2 blob was rejected outright with
`UnsupportedVersion(2)` — the precise opposite of the validate-then-ignore
portability ADR 0004 asks for, and the opposite of what the constant's own doc
comment asserted two lines above it.
What a build can WRITE and what it must ACCEPT are different sets, and only the
first varies by feature. Deriving one from the other reads as tidy and silently
couples them. The check now compares against the literals 1 and 2, with the
reason recorded at both the check and the constant.
WHY NOTHING CAUGHT IT
The default build takes the other branch and was correct throughout, so every
gate stayed green. CI **linted** the `--no-default-features` shape
(`cargo clippy -p rustynes-mappers --no-default-features`) and never **ran** it.
A configuration that is compiled but never executed is not covered, and this is
what that costs.
`cargo test -p rustynes-mappers --no-default-features` is now a CI step, Linux
only and scoped to the one crate whose behaviour is feature-gated this way.
One extra compile of a mid-sized crate, so it runs on every PR rather than
waiting for a full run.
TESTS
`vrc7_load_state_accepts_a_v2_blob_on_every_build` asserts the property directly.
On a `mapper-audio` build `save_state` already emits v2; on a no-audio build it
emits v1, so the test synthesizes the v2 shape — correct, because that path
validates the tail's LENGTH on every build and reads its CONTENTS only where
there is a synthesizer.
Mutation-checked in BOTH configurations, which is the part that documents the
hazard: restoring the old condition turns the test red under
`--no-default-features` with `UnsupportedVersion(2)`, and leaves it green on the
default build. A single-configuration mutation check would have reported the bug
as absent.
Also from review: `save_state`'s `Vec::with_capacity` unconditionally reserved
`VRC7_V2_TAIL_LEN`, over-allocating ~1.3 KiB per save on a build that never
writes that tail. Now `cfg`-gated.
Declined: aligning `OpllR::u32`'s `copy_from_slice` with `OpllR::patch`'s
`try_into` destructuring. The difference is deliberate — `patch` destructures
positionally so its field order is visibly the same list `OpllW::patch` writes,
which is the property that keeps the two halves from drifting; a scalar read has
no such list to mirror.
|
Antigravity review addressed — the blocking finding was real. Fixed in b080ece. Blocking — cross-build compatibility. Confirmed and fixed exactly as The part worth keeping is why no gate caught it: the default build takes the Suggestion — unconditional Nitpick — |
….6 cut Merging `main` after the release SILENTLY DROPPED this entry, and reported no conflict while doing it. Worth writing down, because "mergeable: MERGEABLE" is what GitHub said right up to the moment the text disappeared. The release moved everything out of `[Unreleased]` into `[2.3.6]`, deleting the `### Fixed` heading this entry was anchored under. Git saw one side delete a region and the other side add a line inside it, resolved in favour of the deletion, and produced a clean tree. No marker, no warning -- the entry was simply gone from the merged CHANGELOG. Caught by inspecting `git merge-tree --write-tree` output before merging rather than trusting the mergeability flag; all three post-release fix branches were affected the same way. The fix is mechanical (re-anchor under a fresh `### Fixed` in the now-empty `[Unreleased]`), but the failure mode is not: a CHANGELOG entry is exactly the kind of content whose absence nothing downstream detects.
…3.6] My own error, and the diagnosis that produced it is worth recording. Merging `main` after the v2.3.6 cut did not DROP this entry, as I concluded -- it MOVED it. The release relocated the whole `[Unreleased]` block into `[2.3.6]`, and git carried this branch's addition along with the block it was written inside. I checked for the entry only under `[Unreleased]`, saw nothing, called it dropped, and re-added it there. Two copies: one correctly under `[Unreleased]`, one wrongly inside a released section describing work that release does not contain. The check was too narrow, not wrong in kind: `git merge-tree --write-tree` was the right instrument and it did show the real merged content. I searched four lines of it instead of the whole file. Both review bots caught the duplicate. Removing the `[2.3.6]` copy; the `[Unreleased]` one stays, which is where a fix that ships in the NEXT release belongs.
Two defects from a second review pass, both in code this PR introduced, and both
of a kind the tests as written could not see.
A HAND-EDITED SAVE STATE COULD CRASH THE EMULATOR
`commit_slot_update` indexes the TLL table as `[block_fnum][tl][kl]` with
dimensions `[128][64][4]`, and `Opll::restore` handed it raw deserialized bytes.
A `tl` of 255 computes an index of 524,539 into a 32,768-entry table:
index out of bounds: the len is 32768 but the index is 524539
`blk_fnum` reaches the same tables through `>> 5` (128 rows) and `>> 8` (16), so
an unmasked u16 indexes far past both. A save state is a file on disk --
untrusted input, and module 60 is explicit that every field read from untrusted
bytes is bounds-checked before use.
Every register field is now masked to its hardware width AT THE PARSE BOUNDARY:
the 13 patch parameters to their documented widths, `blk_fnum` to 0x0FFF (a
legal value is `(blk3 << 9) | fnum9`), `fnum` to 9 bits, `blk` to 3, the
single-bit flags to 1, and `number` into slot range. This is parse-don't-
validate rather than a repair: these are register fields of fixed bit width, so
a wider value does not describe a chip state that exists.
The test is careful about one thing worth stating, because the obvious version
of it is useless. An all-`0xFF` blob is REJECTED by `EgState::from_tag` before a
single numeric field is read -- so the naive hostile input passes by accident and
reports the emulator safe. The test therefore repairs the envelope-state tags and
leaves everything else hostile: the interesting input is the one that satisfies
every explicit check and is still nonsense. Mutation-checked -- removing the `tl`
mask alone reproduces the panic.
`load_state` WAS NOT ATOMIC
The v2 tail introduced a failure that can occur AFTER the core fields are
assigned, which the v1 layout could not: v1 validated its whole length and
version up front, so once it began writing it could not fail. A truncated or
corrupt v2 tail returned `Err` with `prg_0`, `chr`, the IRQ state and 2 KiB of
VRAM already overwritten -- a mapper in neither its old state nor its new one,
while the caller reports the load as failed and keeps running.
`Opll::restore` was already atomic internally, and that is exactly what made this
easy to miss: the guarantee existed one level down and was silently discarded one
level up. `load_state` now validates the tail length and parses the synthesizer
into a staged clone BEFORE the first write, then commits infallibly. The
no-audio build performs the same length validation, so the same blob is accepted
or refused identically on every build.
The truncation test asserted only on the return value, which is why review found
this and the test did not. It now gives the target different state from the
source and asserts it is byte-identical after the rejected load. Mutation-checked
-- moving the length guard back after the writes turns it red.
ALSO TAKEN
`tail[0..2].try_into()` over manual `[tail[0], tail[1]]` indexing, guaranteed by
the preceding length check.
DECLINED
Bounds-checking `patch_number`, `wave_table_idx` and `s.number` as suggested:
traced, and none of them index anything. `wave_table_idx` selects through a
`match idx { 0 => .., _ => .. }`; `patch_number` is only ever compared to zero;
`s.number` is stored and compared, never used as a subscript. The reachable path
was elsewhere, through the patch parameters and `blk_fnum` -- which is why it was
worth tracing each one instead of masking on the strength of the report.
VERIFIED
AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff. Workspace
clippy, `mapper-audio`-off clippy AND tests, rustdoc warnings-denied, the no_std
cross-build, 124 workspace test binaries.
|
Antigravity review addressed — both blocking findings were real, and both were Blocking — What makes this one worth naming: Blocking — unvalidated indices from a save state. Real, and reachable — but The reachable path is Every register field is now masked to its hardware width at the parse boundary. Suggestion — Suggestion — advance the slice instead of a Nitpick — Nitpick — |
# Conflicts: # CHANGELOG.md
…underflow-proof Two review findings. A LEAKED diff3 CONFLICT MARKER `CHANGELOG.md` carried a stray `||||||| 3f99a3d`. My resolver checked for `<<<<<<<`, `=======` and `>>>>>>>` and reported zero markers left -- but this repo sets `merge.conflictStyle = diff3`, which adds a fourth marker naming the merge BASE. I verified the wrong alphabet and believed the answer. Content was unaffected (the base section was empty, so only the marker line survived), and all three post-release branches had it. THE NO-AUDIO TAIL-LENGTH CHECK COULD UNDERFLOW data.len() - core_expected < VRC7_V2_TAIL_LEN cannot underflow today: the guard at the top of `load_state` already proved `data.len() >= core_expected`. But the proof lives in a different statement thirty lines away, and the failure mode if it ever moves is not a panic -- the subtraction wraps to a huge value, the comparison goes false, and a TRUNCATED BLOB IS SILENTLY ACCEPTED. That is the worst available outcome for a save-state length check, and it is one refactor away. Rewritten as `data.len() < core_expected + VRC7_V2_TAIL_LEN`, which is correct independent of anything above it. Not worth spreading a correctness argument across two distant statements to save an addition. Verified in both feature configurations: `mapper-audio` on (20 VRC7 tests) and off (16), plus no-audio clippy.
|
Antigravity review addressed. Both findings taken. Blocking — leaked Suggestion — Nitpick — |
…found
Review pushed back that my "every register field is masked at the parse
boundary" claim did not match the code. It was right, and the pushback was worth
far more than the specific fields it named.
THE FIXED PAYLOAD WAS HIDING BUGS
The previous test used one all-`0xFF` payload. Replacing it with a deterministic
pseudo-random sweep (64 rounds, xorshift, no dependency) found THREE more panics
immediately -- and one of them the fixed payload actively CONCEALED:
* `eg_shift` is used as a shift amount, `1u32 << eg_shift`, which panics at 32
and above. Legal values are `13 - eg_rate_h`, so 0..=13. The all-`0xFF` blob
never reached it, because with every byte 0xFF `update_requests` is also
all-ones -- so `commit_slot_update` RECOMPUTED `eg_shift` before
`calc_envelope` could use the restored one. A blob that is maximally hostile
in one dimension can be harmless in another.
* `output[0] + output[1]`, the operator feedback pair, summed as two arbitrary
`i32`s. The field is `i32` for headroom in that sum; the values are only ever
`i32::from(out)` with `out: i16`. Clamped to i16 on restore.
* `eg_rate_l` INDEXES `EG_STEP_TABLES`, outer dimension 4. Legal values are
`rks & 3`.
That last one is the one to remember. Review named `eg_rate_l` explicitly. I
replied that I had traced it and it reached no subscript. My "trace" was a grep
pipeline that emitted nothing because the pipeline itself was broken, and I read
the empty output as evidence of absence. This is the second time tonight I
verified with a broken instrument and believed the answer -- the first being a
conflict-marker check that did not know about diff3.
WHAT CHANGED
`eg_shift` clamped to `EG_SHIFT_MAX`, `output` clamped to i16, and the flag
fields masked to their real widths: `eg_rate_l & 3`, `eg_rate_h & 0x0F`,
`rks & 0x0F`, `type_flags & 3`, `key_flag`/`sus_flag`/`test_flag & 1`.
The comment that overclaimed now states the scope precisely -- what is masked,
and for each field that is NOT, the specific mechanism that makes it safe
(matched-on, clamped at use, recomputed before use, or self-masking index). It
also points at the sweep, because that is what keeps the claim honest rather than
a second round of hand-tracing.
Soaked at 4,000 rounds in both debug (overflow checks on) and release before
settling at 64 in the committed test.
VERIFIED
AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff. Workspace
clippy, no-audio clippy and tests, the no_std cross-build, VRC7 20/20.
|
You were right, and pushing back was worth far more than the specific fields "The PR claims every register field is masked … however these are unmasked."
The first one the fixed payload actively concealed: with every byte The third is the one I owe you a correction on. You named Now:
AccuracyCoin 141/141, nestest 0-diff. |
# Conflicts: # CHANGELOG.md
Review asked whether `adr` and `patch_number` are bounded, noting `adr` could index `self.reg` (length 64) on a subsequent data-port write. Answering that by reading the code is what I did twice tonight, and both times the reading was wrong -- once because a grep pipeline was broken and I took its empty output as proof. So the sweep answers it instead. The randomized sweep only called `calc()`. Synthesis alone never reaches `write_reg`, so any restored field consumed on a subsequent PORT write would sail straight past it -- which is precisely the hole review pointed at. Each round now also drives 64 randomized `write_reg` / `read_reg` pairs against the restored chip, interleaved with `calc()`. Clean, including a 3,000-round soak in debug with overflow checks on. That is a stronger statement than the trace review asked for: not "I looked and `adr` is never an index", but "randomized values in `adr` survive being used as one, on a restored chip, through the port". Covering both paths is also cheaper than adjudicating which fields reach a subscript -- and it keeps working when someone later adds an index that does.
|
Answered with the fuzzer rather than a trace, because tracing is the instrument You are right that the sweep had a hole: it only called Each round now also drives 64 randomized That is a stronger claim than the one you asked for: not "I looked, and On |
Third review round flagging `patch_number`. It is still not a subscript -- it appears only as `patch_number[ch] == 0` comparisons and `patch_number[ch] = num as i32` assignments, and `set_patch_internal` bounds-checks its own `num` against `patch_set.len()` before indexing. The randomized sweep exercises it with arbitrary values, through both synthesis and the register port, and finds nothing. Clamped anyway, and the reason is not appeasement. Every other field in this parse is constrained by its own WIDTH: the value that comes out cannot describe a chip state that does not exist. `patch_number` was the one exception, safe only because nothing currently indexes it -- an invariant that lives in other functions and holds until someone adds an index. The legal domain is 0..=15 (the `$3x` high nibble is four bits), so clamping costs nothing and makes the field's safety self-evident from the parse rather than from a survey of its consumers. Three reviewers reading the same code and reaching the same worry is decent evidence the invariant was too subtle to be load-bearing, even if each individual report was wrong about the mechanism. No fidelity cost, and the existing tests prove it: `snapshot -> restore -> snapshot` byte idempotence and the 2,000-sample stream comparison both still pass, which they could not if a legitimate value were being clamped.
|
Clamped — though I want to be accurate about why, since this is the third round It is still not a subscript: But the argument for clamping it does not depend on that. Every other field in Three reviewers reading the same code and landing on the same worry is decent No fidelity cost, and the existing tests prove that rather than assert it: |
# Conflicts: # CHANGELOG.md
Antigravity review (Gemini via Ultra)This PR appends the live OPLL synthesizer state to the VRC7 mapper save-state section (bumping it to v2) so that audio continuity is maintained across rewinds, while fully preserving backward and cross-feature compatibility. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
Summary
Closes the most actionable open row in
docs/accuracy-ledger.md— open sincev2.2.3, latent since the ADR 0006 VRC7 audio landing.
Rewind a VRC7 game and the music came back wrong.
Vrc7::save_statewrotethe shadow OPLL register bytes and never the live synthesizer (
opll,opll_clock_counter,last_opll_sample), andload_statenever replayed themeither — so after a rewind, netplay rollback, or TAS/save-state restore the FM
voice resumed from an arbitrary point in an unrelated note. Banking, IRQ,
mirroring and PRG-RAM always round-tripped correctly; this was audio-only, and
only on mapper 85.
It was not serializable when it was found:
rustynes_apu::Opllexposed noserialization surface at all. That is what made it "a change of its own" rather
than a release-cut drive-by.
What lands
Opll::snapshot/Opll::restore(OPLL_SNAPSHOT_VERSION1, fixedOPLL_SNAPSHOT_LEN). Carried: the register shadow, the EG and LFO counters, theper-channel patch selection, the user patch pair, all 18 operator slots in full
(phase accumulators, envelope state machines, feedback history), and the
per-channel outputs.
Not carried, because they are constants of construction:
waves,tll_rks, andpatch_set[2..](the chip's patch ROM).chip_typeis written as a tag only— a YM2413 blob restored into a VRC7 is structurally valid in every field and
would silently reinterpret all 18 slot patches against the wrong instrument set,
so it is rejected instead.
The reader decodes the whole blob into locals before touching
self, so atruncated or hand-edited save leaves the synthesizer on its previous state rather
than half-overwritten. This parses untrusted input — a save state is a file on
disk. Enum tags are explicit
to_tag/from_tagmaps, notas u8, so reorderinga variant cannot reinterpret existing states.
VRC7 mapper section v2, appending the blob after the VRAM. Additive: a v1
blob still loads and leaves the synthesizer where the old build left it, so an
old save is no worse than it was rather than newly silent. A build without
mapper-audiohas no synthesizer to describe, so it writes v1 andvalidates-then-ignores a v2 tail — preserving the cross-feature portability
ADR 0004 promises, which is why the version byte is build-dependent.
The alternative, and why it lost
Replaying
audio.regsthroughOpll::write_regon load needs no new format andis the repair a reader thinks of first. It restarts every keyed-on channel's
envelope at attack, so every rewind frame produces an audible transient. The
ledger recorded that no oracle could adjudicate which was worse; carrying the
state verbatim removes the question, because it reproduces the sound that was
actually playing.
Tests
vrc7_save_state_carries_the_live_opll_so_audio_resumes_identicallykeys a note,advances 20,000 CPU cycles so the envelope is well past attack, saves, then
compares 4,000 mixed samples from the source against 4,000 from a fresh
mapper restored from the blob — equal sample for sample.
Mutation-checked: making the tail carry a reset synthesizer reproduces the
pre-fix failure on the first divergent sample.
Plus, at the
Oplllevel — a 2,000-sample stream round-trip into a fresh chip;snapshot → restore → snapshot byte idempotence (catches a field written but
not read back, which the stream test can miss); cross-chip rejection; truncation
rejection asserting the target is left unmutated; unknown-version rejection; a
corrupt envelope-state tag. At the mapper level — v1 back-compat and a truncated
v2 tail.
Opllis now registered insnapshot_schema_audit.rs. That audit found thev2.2.3 PPU and APU gaps mechanically, but it only knew about the three chips
inside the console, so it could not see this surface at all. A save-state surface
no audit can see is exactly how a gap this size survives four releases.
Registering it immediately caught two of my own exclusion entries as false
admissions (
chip_typeandpatch_setare written) — the audit working.Out of scope, checked and left alone:
nsf_expansion.rsalso holds anOpllandalso carries no phase, but that is a written decision with a stated rationale
(an NSF driver re-establishes channel state on the next play call), not an
undocumented gap.
Verification
Nothing on the synthesis path moved, so emulation output is unchanged — but this
touches
rustynes-apuandrustynes-mappers, so the contract was verified,not assumed:
cargo clippy --workspace --all-targets -- -D warningscargo clippy -p rustynes-mappers --no-default-features(audio off)RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depsno_stdcross-build (thumbv7em-none-eabihf)