feat(apu): implement the SMP wait states; E3.09 measures them

CHANGELOG.md

@@ -11,6 +11,53 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **`E3.13` — a write to `$00F0-$00FF` lands in the RAM shadow as well as in the register.** The
+  register block wins every SPC read of those addresses, so the assertion cannot be checked by
+  writing and reading back; it needs a second reader of APU RAM that skips the register decode, and
+  the S-DSP is one. A nine-byte BRR block written **through** the register block at `$00F7` ends
+  exactly at `$00FF`, never reaching the directory at `$0100`, and clears `$F0` (TEST) and `$F1`
+  (CONTROL) — the two writes that would change the wait states or strand `release_to_ipl`. Three of
+  its nine bytes land on `$FD`-`$FF`, which are **read-only**, so for those the shadow is the only
+  thing a write could have affected.
+
+  The row asserts **equality against a control voice** playing a byte-identical copy in ordinary
+  RAM, not "the shadow read back non-zero": what sits under the register block at power-on is
+  undefined and one reference randomises APU RAM, so a non-zero reading could be luck. Nine bytes
+  decoding to the control's exact `OUTX` cannot be. Injecting `address & 0xFFF0 != 0x00F0` into both
+  bus write paths fires code 2 with the guard still passing.
+
+  Two setup faults the guard caught rather than letting the row pass on two zeroes: `KOF` is a level
+  register that a previous program can leave set, and `KON` must be **held** — it is examined once
+  every two output samples (`E8.01`), so a write immediately followed by a clear is cancelled before
+  the DSP ever looks. Coverage `353 → 354 of 443`.
+
+- **`E3.09`, and the SMP wait states it found unimplemented.** `$F0` bits 4-5 and 6-7 select a clock
+  divider for the SMP, nominally `{2, 4, 8, 16}` — but 8 and 16 are glitchy on real silicon and
+  **the CPU consumes 10 and 20 clocks per opcode cycle while the timers still advance by 8 and 16.**
+  ares and bsnes carry the same comment (`sfc/smp/timing.cpp`): *"the timers are not affected by
+  this and advance by their expected values."* Two tables, and the gap between them is the row.
+
+  RustySNES parsed both selectors into `Io::external_wait` / `Io::internal_wait`, saved them,
+  restored them — and **nothing downstream ever read either one**. The eighth instance of the
+  dead-config defect class, and the first found by the cart rather than by grep.
+
+  `rustysnes-apu` now carries `SMP_CYCLE_WAIT = [2,4,10,20]` for the CPU (and hence for the recorded
+  micro-op plan and the S-DSP catch-up, which are real base clocks) against
+  `SMP_TIMER_WAIT = [2,4,8,16]` for the timers alone, with ares' `SMP::wait` address classification:
+  idle cycles, `$00F0-$00FF` and a mapped IPL ROM take the *internal* selector, everything else the
+  external one. **At the reset selector both tables read `SMP_WAIT`, so every program that leaves
+  `$F0` alone — which is every commercial driver — is byte-identical to before.**
+
+  The row reads the gap as a ratio the program can see: a wait selector changes clocks-per-cycle,
+  not an instruction's cycle count, so the same loop is the same number of opcode cycles either way
+  and only the timer-per-cycle rate moves. Over a fixed 48-pass poll loop, timer 0 ticks **4x** as
+  often at selector 2 as at selector 0. Both wrong models were injected and both fail on code 2:
+  no wait states at all reads **1x**, and charging the CPU's glitchy 10 to the timers as well reads
+  **5x**. So the row separates the two ways of having the feature, not merely its absence.
+
+  Coverage `352 → 353 of 443` at the time it landed; battery 100% on-cart. `docs/apu.md` gains
+  the selector model.
+
 - **`E9.09` — the echo write pointer wraps at the 16-bit boundary, over page zero `[ERRATA]`.**
   `ESA` names a page, `EDL` a length, and the address is computed in sixteen bits with nothing
   clamping it at `$FFFF`. A driver that sets `ESA` too high does not get a short buffer or a dropped

crates/rustysnes-apu/src/dsp.rs

@@ -78,6 +78,14 @@ struct Voice {
     adsr1: u8,
     gain: u8,
     envx: u8,
+    /// The raw `$4C` bit as last written — a **mirror, not the control**.
+    ///
+    /// `KON` is write-triggered and non-persistent (`E8.04`), so what the voice actually keys on is
+    /// [`Voice::keylatch`]: `misc29`/`misc30` clear it against `_keyon` once per sample, which is
+    /// what makes a second `$4C` write cancel a pending key-on (`E8.01`). Nothing downstream reads
+    /// this field, and a reader who mistakes it for the authoritative flag would reintroduce
+    /// exactly the "keys on for as long as the bit is set" bug the latch exists to prevent. Kept
+    /// because it is part of the serialized layout; removing it is a save-state format change.
     keyon: bool,
     keyoff: bool,
     modulate: bool,

crates/rustysnes-apu/src/lib.rs

@@ -46,14 +46,39 @@ use dsp::{ARAM_SIZE, Dsp};
 use rustysnes_savestate::{SaveReader, SaveStateError, SaveWriter};
 use spc700::{Spc700, Spc700Bus};
 
-/// SMP base clocks one bus access consumes — ares `cycleWaitStates[0]` (the reset wait state).
+/// SMP base clocks one bus access consumes at the **reset** wait state — `SMP_CYCLE_WAIT[0]`.
 ///
 /// The SMP base clock is `apuFrequency / 12` (ares `SMP::create(apuFrequency()/12, …)`); a normal
-/// access is 2 of those base ticks, giving the ~1.024 MHz effective opcode-cycle rate. The
-/// per-region external/internal wait-state divider (the glitchy `{2,4,10,20}` table) collapses to
-/// this reset default — no committed program reprograms `$F0`'s wait selectors.
+/// access is 2 of those base ticks, giving the ~1.024 MHz effective opcode-cycle rate. Kept as a
+/// named constant because it is the value every unit test and every shipped program actually sees:
+/// `$F0`'s wait selectors reset to 0 and no commercial driver reprograms them.
 const SMP_WAIT: u32 = 2;
 
+/// SMP base clocks the **CPU** consumes per opcode cycle, indexed by a `$F0` wait selector.
+///
+/// The selector is nominally a clock divider of `{2, 4, 8, 16}`, but dividers of 8 and 16 are
+/// glitchy on real silicon and the CPU ends up consuming **10 and 20**. ares and bsnes document it
+/// identically (`sfc/smp/timing.cpp`): *"sometimes the SMP will run far slower than expected, other
+/// times … the SMP will deadlock until the system is reset. The timers are not affected by this and
+/// advance by their expected values."*
+///
+/// That last sentence is why there are two tables and not one — see [`SMP_TIMER_WAIT`]. The gap
+/// between them is directly measurable from a cart, and `E3.09` measures it.
+const SMP_CYCLE_WAIT: [u32; 4] = [2, 4, 10, 20];
+
+/// SMP base clocks the **timers** advance per opcode cycle, indexed by a `$F0` wait selector.
+///
+/// The un-glitched divider, ares `timerWaitStates`. At selectors 2 and 3 the timers advance 8 and
+/// 16 where the CPU pays 10 and 20, so over a fixed instruction sequence the timers accumulate
+/// `8/10` of the elapsed real time — which reads from a cart as the timers running **faster
+/// relative to the program** than at selector 0, by exactly `8/2 = 4`. See [`SMP_CYCLE_WAIT`].
+const SMP_TIMER_WAIT: [u32; 4] = [2, 4, 8, 16];
+
+/// The largest `base_clocks` a recorded micro-op can legitimately carry: the slowest wait selector,
+/// un-halved. A save-state claiming more never came from real execution, and a value `plan_sub`
+/// could never reach would wedge `advance_smp_cycle`.
+const MAX_PLAN_BASE_CLOCKS: u32 = SMP_CYCLE_WAIT[3];
+
 /// SMP base clocks per S-DSP **micro-tick**. The S-DSP runs its 32-step voice sequence one
 /// [`Dsp::tick`] at a time; 32 ticks = one 32 kHz stereo sample. `apuFrequency = 32040 × 768`, the
 /// SMP/DSP base is `apuFrequency / 12`, so one sample is `(32040 × 768 / 12) / 32040 = 64` base
@@ -258,6 +283,35 @@ impl Default for Io {
 }
 
 impl Io {
+    /// Which `$F0` wait selector governs an access — ares `SMP::wait`'s address classification.
+    ///
+    /// Idle cycles (no address), the `$00F0-$00FF` register block, and the IPL ROM **while it is
+    /// mapped** are *internal*; everything else is *external*. The IPL clause reads oddly until you
+    /// note it is conditional on the mapping: once `$F1` bit 7 is clear those addresses are ordinary
+    /// ARAM and take the external selector like any other.
+    const fn wait_index(&self, address: Option<u16>) -> usize {
+        let internal = match address {
+            None => true,
+            Some(a) => a & 0xFFF0 == 0x00F0 || (a >= 0xFFC0 && self.iplrom_enable),
+        };
+        (if internal {
+            self.internal_wait
+        } else {
+            self.external_wait
+        }) as usize
+    }
+
+    /// The `(cpu, timer)` base-clock pair one access costs, halved for the split `$F4-$F7` reads.
+    ///
+    /// Returned as a pair rather than resolved separately at each call site because the whole point
+    /// of the glitch is that the two numbers differ; computing them apart invites one of them to be
+    /// derived from the wrong table.
+    const fn wait_clocks(&self, halve: bool, address: Option<u16>) -> (u32, u32) {
+        let idx = self.wait_index(address);
+        let shift = if halve { 1 } else { 0 };
+        (SMP_CYCLE_WAIT[idx] >> shift, SMP_TIMER_WAIT[idx] >> shift)
+    }
+
     fn save_state(&self, s: &mut SaveWriter) {
         s.write_bool(self.timers_disable);
         s.write_bool(self.ram_writable);
@@ -619,8 +673,8 @@ impl Apu {
     /// `MAX_SAVED_PLAN_LEN` (mirroring the GSU's `pending_clocks` validation in
     /// `rustysnes-cart` — an in-flight instruction has at most a handful of micro-ops, so a
     /// larger claimed length could never have come from real execution); a step's `base_clocks`
-    /// is neither `1` nor `2` (`record`/`record_next_instruction` only ever push `SMP_WAIT` or
-    /// `SMP_WAIT >> 1`; an out-of-range value risks `plan_sub` never reaching it, wedging
+    /// is zero or above `MAX_PLAN_BASE_CLOCKS` (`record` only ever pushes a `SMP_CYCLE_WAIT`
+    /// entry, optionally halved; an out-of-range value risks `plan_sub` never reaching it, wedging
     /// `advance_smp_cycle`, or an unbounded drain); `plan_pos` exceeds the restored plan's
     /// length; or `plan_sub` is inconsistent with `plan_pos` (nonzero while `plan_pos` is already
     /// past the end of the plan, or `>=` the step at `plan_pos`'s own `base_clocks` — either
@@ -646,9 +700,10 @@ impl Apu {
         self.plan.clear();
         for _ in 0..plan_len {
             let base_clocks = s.read_u32()?;
-            if base_clocks == 0 || base_clocks > SMP_WAIT {
+            if base_clocks == 0 || base_clocks > MAX_PLAN_BASE_CLOCKS {
                 return Err(SaveStateError::Invalid(alloc::format!(
-                    "APU instruction plan step base_clocks {base_clocks} is not 1 or {SMP_WAIT}"
+                    "APU instruction plan step base_clocks {base_clocks} exceeds the largest a \
+                     wait selector can produce ({MAX_PLAN_BASE_CLOCKS})"
                 )));
             }
             let port_write = if s.read_bool()? {
@@ -712,15 +767,19 @@ struct SmpBus<'a> {
 }
 
 impl SmpBus<'_> {
-    /// Advance the SMP base clock + the timers by `clocks` base ticks (ares `SMP::step` +
-    /// `stepTimers`). One normal bus access is [`SMP_WAIT`] (= ares `cycleWaitStates[0]`) ticks.
-    fn step(&mut self, clocks: u32) {
-        self.cycles += clocks;
+    /// Advance the SMP base clock + the timers for one access — ares `SMP::wait`.
+    ///
+    /// The CPU and the timers are advanced by **different** amounts whenever a `$F0` wait selector
+    /// is 2 or 3: the CPU pays the glitchy `{10, 20}` while the timers advance the expected
+    /// `{8, 16}`. At the reset selector both are [`SMP_WAIT`], which is why this reduces to the
+    /// previous single-number behaviour for every program that leaves `$F0` alone.
+    fn wait(&mut self, halve: bool, address: Option<u16>) {
+        let (cpu_clocks, timer_clocks) = self.io.wait_clocks(halve, address);
+        self.cycles += cpu_clocks;
         let te = self.io.timers_enable;
         let td = self.io.timers_disable;
-        // Timers advance on the same SMP base timebase as the CPU (ares `timerWaitStates[0]` = 2).
         for t in self.timers.iter_mut() {
-            t.step(clocks as u16, te, td);
+            t.step(timer_clocks as u16, te, td);
         }
     }
 
@@ -816,22 +875,22 @@ impl Spc700Bus for SmpBus<'_> {
         // steps around the data fetch (`wait(1)` twice). At the reset wait state the total is the
         // same SMP_WAIT base clocks as any other access, but the split is preserved for fidelity.
         if address & 0xFFFC == 0x00F4 {
-            self.step(SMP_WAIT >> 1);
+            self.wait(true, Some(address));
             let v = self
                 .read_io(address)
                 .unwrap_or_else(|| self.read_ram(address));
-            self.step(SMP_WAIT >> 1);
+            self.wait(true, Some(address));
             return v;
         }
-        self.step(SMP_WAIT);
+        self.wait(false, Some(address));
         if let Some(io) = self.read_io(address) {
             return io;
         }
         self.read_ram(address)
     }
 
     fn write(&mut self, address: u16, data: u8) {
-        self.step(SMP_WAIT);
+        self.wait(false, Some(address));
         // Writes to $FFC0-$FFFF always reach ARAM even with the IPL ROM mapped in.
         if self.io.ram_writable && !self.io.ram_disable {
             self.aram[address as usize] = data;
@@ -840,7 +899,9 @@ impl Spc700Bus for SmpBus<'_> {
     }
 
     fn idle(&mut self) {
-        self.step(SMP_WAIT);
+        // ares `SMP::idle` is `wait(0)`: NOT halved, and with no address, so an idle cycle takes
+        // the *internal* selector. At the reset selector that is [`SMP_WAIT`], unchanged.
+        self.wait(false, None);
     }
 }
 
@@ -874,11 +935,15 @@ impl RecordingSmpBus<'_> {
     /// register (`$F3`) mid-execution the **cycle-correct** value: the DSP has advanced exactly the
     /// ticks up to that base clock and no further. blargg's `spc_dsp6` / `spc_mem_access_times` use
     /// the DSP as a sub-cycle reference, so this granularity is required for them to resolve.
-    fn record(&mut self, clocks: u32) {
+    fn record(&mut self, halve: bool, address: Option<u16>) {
+        // The CPU's cost and the timers' are two different numbers whenever a `$F0` wait selector
+        // is 2 or 3 — see `SMP_CYCLE_WAIT`. The plan, the DSP catch-up and `consumed` all follow
+        // the CPU's, because those are real base clocks; only the timers take the other table.
+        let (clocks, timer_clocks) = self.io.wait_clocks(halve, address);
         let te = self.io.timers_enable;
         let td = self.io.timers_disable;
         for t in self.timers.iter_mut() {
-            t.step(clocks as u16, te, td);
+            t.step(timer_clocks as u16, te, td);
         }
         *self.consumed += clocks;
         *self.dsp_counter += clocks;
@@ -996,14 +1061,14 @@ impl Spc700Bus for RecordingSmpBus<'_> {
     fn read(&mut self, address: u16) -> u8 {
         if address & 0xFFFC == 0x00F4 {
             // $F4-$F7 read: ares splits the wait into two halved steps around the fetch.
-            self.record(SMP_WAIT >> 1);
+            self.record(true, Some(address));
             let v = self
                 .read_io(address)
                 .unwrap_or_else(|| self.read_ram(address));
-            self.record(SMP_WAIT >> 1);
+            self.record(true, Some(address));
             return v;
         }
-        self.record(SMP_WAIT);
+        self.record(false, Some(address));
         if let Some(io) = self.read_io(address) {
             return io;
         }
@@ -1018,7 +1083,10 @@ impl Spc700Bus for RecordingSmpBus<'_> {
         // already-happened — the one-access phase the blargg `spc_timer` / `spc_smp` /
         // `spc_mem_access_times` suites pin. (Matches [`SmpBus::write`], which already steps first;
         // the recording bus previously stored first, shifting the timer phase by one access.)
-        self.record(SMP_WAIT);
+        //
+        // Stepping first also means a write that CHANGES a wait selector pays the OLD one, which is
+        // ares' ordering too — `wait()` runs before `writeIO`.
+        self.record(false, Some(address));
         // Writes to $FFC0-$FFFF always reach ARAM even with the IPL ROM mapped in.
         if self.io.ram_writable && !self.io.ram_disable {
             self.aram[address as usize] = data;
@@ -1031,7 +1099,7 @@ impl Spc700Bus for RecordingSmpBus<'_> {
     }
 
     fn idle(&mut self) {
-        self.record(SMP_WAIT);
+        self.record(false, None); // ares `SMP::idle` = `wait(0)`: internal selector, not halved
     }
 }
 
@@ -1079,6 +1147,52 @@ mod tests {
         apu.tick(&mut bus);
     }
 
+    /// The reset wait selector must reproduce the old single-number behaviour exactly, or every
+    /// timing golden in the tree moves for a feature no shipped program uses.
+    #[test]
+    fn the_reset_wait_selector_costs_what_it_always_did() {
+        let io = Io::default();
+        assert_eq!(io.wait_clocks(false, Some(0x0200)), (SMP_WAIT, SMP_WAIT));
+        assert_eq!(io.wait_clocks(false, None), (SMP_WAIT, SMP_WAIT));
+        assert_eq!(io.wait_clocks(true, Some(0x00F4)), (1, 1));
+    }
+
+    /// The whole point of `E3.09`: at selectors 2 and 3 the CPU pays 10/20 while the timers advance
+    /// 8/16. A core using one table for both is what this pins against.
+    #[test]
+    fn the_glitchy_wait_selectors_charge_the_cpu_more_than_the_timers() {
+        let mut io = Io::default();
+        for (selector, cpu, timer) in [(1u8, 4, 4), (2, 10, 8), (3, 20, 16)] {
+            io.external_wait = selector;
+            io.internal_wait = selector;
+            assert_eq!(
+                io.wait_clocks(false, Some(0x0200)),
+                (cpu, timer),
+                "selector {selector}"
+            );
+        }
+    }
+
+    /// The address classification, which decides *which* selector an access takes.
+    #[test]
+    fn the_register_block_and_a_mapped_ipl_take_the_internal_selector() {
+        let mut io = Io {
+            external_wait: 1, // 4 clocks
+            internal_wait: 3, // 20 / 16
+            ..Io::default()
+        };
+        assert_eq!(io.wait_clocks(false, Some(0x0200)), (4, 4), "plain ARAM");
+        assert_eq!(io.wait_clocks(false, Some(0x00F3)), (20, 16), "$F3 is IO");
+        assert_eq!(io.wait_clocks(false, None), (20, 16), "an idle cycle");
+        assert_eq!(io.wait_clocks(false, Some(0xFFC0)), (20, 16), "IPL mapped");
+        io.iplrom_enable = false;
+        assert_eq!(
+            io.wait_clocks(false, Some(0xFFC0)),
+            (4, 4),
+            "unmapped, $FFC0 is ordinary ARAM and takes the external selector"
+        );
+    }
+
     #[test]
     fn power_on_state_matches_hardware() {
         let cpu = Spc700::new();

crates/rustysnes-test-harness/tests/accuracysnes.rs

@@ -596,7 +596,7 @@ const A5_08_SLOTS: [(u8, &str); 7] = [
 ];
 
 /// The measurement slots the `v1.29.0` Group E batch records.
-const GROUP_E_BATCH_SLOTS: [(u16, &str); 12] = [
+const GROUP_E_BATCH_SLOTS: [(u16, &str); 14] = [
     (256, "E8.01 KON sweep key-on mask   (bit 7 = voice 0)"),
     (
         257,
@@ -616,6 +616,8 @@ const GROUP_E_BATCH_SLOTS: [(u16, &str); 12] = [
         "E9.09 the same byte one wrap later, $0001 (equal, or the pointer did not wrap)",
     ),
     (265, "E9.09 its low byte at $0000 (E9.12 masks bit 0)"),
+    (270, "E3.13 control OUTX, a copy in ordinary RAM"),
+    (271, "E3.13 OUTX decoded from under $00F0-$00FF"),
     (266, "E3.06 timer 0 ticks over the interval"),
     (
         267,