diff --git a/CLAUDE.md b/CLAUDE.md index 201ce9b6..d807c10b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -270,6 +270,9 @@ Each requirement below is done when the linked test passes. Add new links as tes | Winlink header fields are capped, and a realistic multi-recipient message still decodes | `cargo test -p openpulse-b2f --no-default-features -- header_decode_caps header_decode_allows_a_realistic` | | B2F driver survives a hostile peer — line cap, per-operation read deadlines, framing edges | `cargo test -p openpulse-b2f-driver --no-default-features --test cmd_hardening` + `--test timeout_hardening` + `--test data_framing` | | B2F driver reports a refused or fully-rejected ISS transfer instead of silent success | `cargo test -p openpulse-b2f-driver --no-default-features --test iss_failure_paths` | +| The daemon decodes a station **±50 Hz** off frequency (REQ-PHY-03 at its bound), and the acquisition pass recovers one **beyond native reach** (100 Hz, where neither decision arm decodes unaided). Split in #1428: the union's uncancelled arm decodes this fixture at 50 Hz with no acquisition, so a settle guard at 50 Hz measured fixture luck. Sabotage-verified: breaking the settle's estimate fails the 100 Hz test and leaves the 50 Hz one green — which is why the 50 Hz test alone could not guard the pass. Native decode at a given offset is frame-dependent (a different frame failed both arms at 25 Hz); the pass is what REQ-PHY-03 rests on | `cargo test -p openpulse-modem --no-default-features --test daemon_frequency_acquisition` | +| Every hard-decode chain tries **every decision arm a mode offers** and keeps the first its FEC accepts (#1428 union), every plugin's `demodulate_variants` obeys the contract (variant 0 is `demodulate`; declared arms differ at some rung of a fixed 0 … −24 dB ladder; a single-arm plugin's soft path agrees with its hard path under noise), and the second arm is **reached and wins** on a fade but is never credited on a clean channel. The wiring test proves the arm is reached, NOT the gain — the gain (+18/96 via `ota_decode_burst`, zero frames lost) needs a variant-0-only build and is recorded in the ledger | `cargo test -p openpulse-modem --no-default-features --test hard_variant_conformance --test union_second_arm_wiring` + `cargo test -p bpsk-plugin --no-default-features --lib -- variant second_arm symbol_stream_feeds` | +| The GPU BPSK demodulator agrees with the CPU one **where the crossfade cancellation decides the frame** (#1433), with a guard on BOTH sides of the cliff so the sweep cannot drift into a saturated cell. **Manual tier — nothing runs this automatically**: it needs `--features gpu` and a working adapter, the gate is `--no-default-features`, and CI's `gpu` job was removed in #1380. That absence is why #1433 shipped for 71 days | `cargo test -p bpsk-plugin --features gpu --test gpu_cpu_equivalence` (on a host with an adapter) | | CI gates are defined and correct (Linux core/full/pi5 + `macos-build`; the `gpu` job was removed in #1380, subsumed by `gate.sh`'s `--all-features` pass) — **but the `CI` workflow is `disabled_manually` by the maintainer, so they do NOT run on a PR; the gates above are run locally before every merge** | `.github/workflows/ci.yml` `on: pull_request` (definition only; check state with `gh api repos/dc0sk/OpenPulseHF/actions/workflows`) | For any new Phase 1 feature: write the test first, confirm it fails, implement until it passes. Do not mark a task done if its test does not exist. diff --git a/crates/openpulse-core/src/plugin.rs b/crates/openpulse-core/src/plugin.rs index 3cabdf7c..79292fd7 100644 --- a/crates/openpulse-core/src/plugin.rs +++ b/crates/openpulse-core/src/plugin.rs @@ -25,7 +25,7 @@ use crate::error::{ModemError, PluginError}; /// would corroborate settles on pure noise — the exact defect the check exists to prevent, and one /// that produces no error, only a receiver that stops acquiring. A single method makes publishing a /// template without its own measured constants unrepresentable. -pub const PLUGIN_TRAIT_VERSION: &str = "3.0.0"; +pub const PLUGIN_TRAIT_VERSION: &str = "3.1.0"; // ── Plugin metadata ─────────────────────────────────────────────────────────── @@ -309,6 +309,33 @@ pub trait ModulationPlugin: Send + Sync { Ok(llrs) } + /// Every hard-decision wire this mode can produce from ONE acquisition, best-first. + /// + /// The engine tries each in order and keeps the first whose FEC + frame decode succeeds, so a + /// plugin whose demodulator has two defensible decision rules can offer both and let RS, the + /// length prefix and CRC-16 adjudicate — rather than the engine guessing with a predicate. + /// + /// **`variants[0]` MUST equal [`demodulate`](Self::demodulate) byte-for-byte.** The rest of the + /// trait contract hangs off `demodulate`, and the default body below preserves that by + /// construction. `every_plugin_obeys_the_hard_variant_contract` (`openpulse-modem/tests/hard_variant_conformance.rs`) + /// sweeps the registry for it. + /// + /// Return ONE variant unless a second is a genuinely different decode. A duplicate costs a + /// wasted FEC trial per onset in the scanning receive and can be miscounted as a second arm + /// contributing. Note the arms of a given mode may be identical on a clean channel and differ + /// only under noise — that is expected, and it is the noisy case the sweep checks. + /// + /// Motivating case (#1428): BPSK's crossfade-ISI cancellation wins AWGN decisively and loses on + /// `moderate_f1`, measured end-to-end with real RS. Neither arm dominates. Their union was never + /// below the better arm in any measured cell, and above both on the two `moderate_f1` cells. + fn demodulate_variants( + &self, + samples: &[f32], + config: &ModulationConfig, + ) -> Result>, ModemError> { + Ok(vec![self.demodulate(samples, config)?]) + } + /// Frame geometry for `config.mode`, used by the receive engine to size /// its scan step, energy-gate window, and demodulation slices. /// diff --git a/crates/openpulse-modem/src/engine.rs b/crates/openpulse-modem/src/engine.rs index 1a26134f..6b865a50 100644 --- a/crates/openpulse-modem/src/engine.rs +++ b/crates/openpulse-modem/src/engine.rs @@ -776,6 +776,9 @@ pub struct ModemEngine { /// Count of capture blocks the notch processed — a tripwire: an enabled notch that never runs /// on a given path (e.g. a new capture path that skips the InputCapture seam) leaves this at 0. notch_blocks_processed: u64, + /// Accepted frames produced by a non-primary decision arm (#1428). Wiring evidence, not a rescue + /// count — see [`alternate_arm_decodes`](Self::alternate_arm_decodes). + alternate_arm_decodes: u64, notch_freqs_seen: std::collections::BTreeSet, notch_protect_extremes: Option<(f32, f32, f32, f32)>, /// Count of settle anchors condemned by the micro-sweep and handed back to the scan. @@ -1010,6 +1013,7 @@ impl ModemEngine { notch_in_band_interferers: Vec::new(), rx_mode: None, notch_blocks_processed: 0, + alternate_arm_decodes: 0, notch_freqs_seen: std::collections::BTreeSet::new(), notch_protect_extremes: None, settle_condemnations: 0, @@ -4625,8 +4629,11 @@ impl ModemEngine { FecMode::SoftConcatenated | FecMode::Ldpc | FecMode::LdpcHighRate ); - // Soft codecs consume LLRs; hard codecs consume demodulated wire bytes. - let (llrs, raw_wire) = { + // Soft codecs consume LLRs; hard codecs consume demodulated wire bytes — and since #1428 + // the hard family takes EVERY decision arm the mode offers, adjudicated by its own FEC. + // Demodulated here, before `update_afc_estimate` below, so all arms share one centre + // frequency; the decode runs after, via `decode_variants`. + let (llrs, raw_variants) = { let plugin = self .plugins .get(mode) @@ -4641,7 +4648,7 @@ impl ModemEngine { } else { ( None, - Some(self.stage_demodulate_payload(plugin, mode, &samples)?), + Some(self.stage_demodulate_variants(plugin, mode, &samples)?), ) } }; @@ -4652,7 +4659,9 @@ impl ModemEngine { // hard codecs the byte count is what the multiple-of-255 / prefix logic keys off. debug!( "fec demod: mode={mode} fec={fec:?} soft={soft} wire_bytes={} llrs={}", - raw_wire.as_ref().map_or(0, |w| w.bytes.len()), + raw_variants + .as_ref() + .map_or(0, |v| v.first().map_or(0, |w| w.bytes.len())), llrs.as_ref().map_or(0, |l| l.len()) ); @@ -4685,30 +4694,48 @@ impl ModemEngine { // to the FEC family it will be decoded with — hard-decision modes (Rs*/Concatenated) carry // `raw_wire = Some`, soft-decision modes (SoftConcatenated/Ldpc*) carry `llrs = Some`. Each // per-arm `.unwrap()` below is guarded by that producer↔arm pairing, never operator input. - let corrected = match fec { - FecMode::Rs => { - let wire = - self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire.unwrap())?; - WirePayload { - // decode_prefix, not decode: this is the SCANNING receive, so `wire.bytes` is a - // fixed-length window out of the capture buffer — its length is a function of - // the window, not the frame, so `decode` rejected it on the multiple-of-255 - // gate before RS ever ran whenever the capture outlasted the frame - // (audit 2026-07-19). `decode_combined_llrs` and the single-shot - // `receive_with_fec_mode` keep strict `decode` — they know the frame extent. - bytes: self.rs_decode_prefix_free_strengthened(&wire.bytes)?, - } - } - FecMode::RsInterleaved => { - let wire = - self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire.unwrap())?; - WirePayload { - // Prefix trial, not a straight deinterleave: the permutation is derived from the - // buffer length, so the window length must be trimmed to the frame's *before* it - // is unscrambled. Same reason the `Rs` arm above uses `decode_prefix`. - bytes: rs_interleaved_decode_prefix(DEFAULT_INTERLEAVER_DEPTH, &wire.bytes)?, + // The hard family, tried on every decision arm (#1428). The closure takes bytes and not + // `&mut Self`, so a losing arm cannot touch AFC, HARQ retention, the rate controller or the + // SNR record on its way past — that is a property of the signature, not a promise. + if let Some(variants) = raw_variants { + let corrected = self.decode_variants(mode, variants, |bytes| match fec { + // decode_prefix, not decode: this is the SCANNING receive, so `bytes` is a + // fixed-length window out of the capture buffer — its length is a function of + // the window, not the frame, so `decode` rejected it on the multiple-of-255 + // gate before RS ever ran whenever the capture outlasted the frame + // (audit 2026-07-19). `decode_combined_llrs` and the single-shot + // `receive_with_fec_mode` keep strict `decode` — they know the frame extent. + FecMode::Rs => Ok(WirePayload { + bytes: Self::rs_decode_prefix_free_strengthened_pure(bytes)?, + }), + // Prefix trial, not a straight deinterleave: the permutation is derived from the + // buffer length, so the window length must be trimmed to the frame's *before* it + // is unscrambled. Same reason the `Rs` arm above uses `decode_prefix`. + FecMode::RsInterleaved => Ok(WirePayload { + bytes: rs_interleaved_decode_prefix(DEFAULT_INTERLEAVER_DEPTH, bytes)?, + }), + FecMode::Concatenated => { + let conv = ConvCodec::new().decode(bytes)?; + Ok(WirePayload { + bytes: FecCodec::new().decode(&conv)?, + }) } - } + FecMode::RsStrong => Ok(WirePayload { + bytes: FecCodec::strong().decode_prefix(bytes)?, + }), + // ShortRs (byte-exact, no length prefix) and Turbo (fixed QPP block size + // = llrs.len()/3) both need the exact frame length, which the scanning + // receive can't guarantee (trailing-noise samples inflate the count), so + // they stay single-shot. + other => Err(ModemError::Demodulation(format!( + "FEC mode {other:?} is not supported by the timeout receive; \ + use receive_with_fec_mode for a single-shot decode" + ))), + })?; + return self.finish_decoded_frame(mode, corrected, pending_snr); + } + + let corrected = match fec { FecMode::SoftConcatenated => { let llrs = llrs.unwrap(); let rs = soft_concat_decode_llrs(&llrs)?; @@ -4725,25 +4752,7 @@ impl ModemEngine { let info = decode_ldpc_llrs_prefix(&LdpcCodec::high_rate(), &llrs)?; self.route_wire_stage(PipelineStage::DemodulateDecode, WirePayload { bytes: info })? } - FecMode::Concatenated => { - let wire = - self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire.unwrap())?; - let conv = ConvCodec::new().decode(&wire.bytes)?; - WirePayload { - bytes: FecCodec::new().decode(&conv)?, - } - } - FecMode::RsStrong => { - let wire = - self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire.unwrap())?; - WirePayload { - bytes: FecCodec::strong().decode_prefix(&wire.bytes)?, - } - } - // ShortRs (byte-exact, no length prefix) and Turbo (fixed QPP block size - // = llrs.len()/3) both need the exact frame length, which the scanning - // receive can't guarantee (trailing-noise samples inflate the count), so - // they stay single-shot. + // The hard family returned above; anything else reaching here is unsupported. other => { return Err(ModemError::Demodulation(format!( "FEC mode {other:?} is not supported by the timeout receive; \ @@ -4752,6 +4761,20 @@ impl ModemEngine { } }; + self.finish_decoded_frame(mode, corrected, pending_snr) + } + + /// The success tail shared by both `receive_from_samples_with_fec_inner` arms (#1428). + /// + /// Runs ONCE, for whichever decision arm won — frame decode, `HpxStateUpdate`, the + /// success-gated SNR record and `FrameReceived`. Keeping it in one place is what stops the + /// union from emitting two events or recording two SNRs when a later arm rescues a frame. + fn finish_decoded_frame( + &mut self, + mode: &str, + corrected: WirePayload, + pending_snr: Option, + ) -> Result, ModemError> { let frame = self.stage_decode_frame(&corrected)?; let frame = self.route_decoded_stage(PipelineStage::HpxStateUpdate, frame)?; @@ -5189,24 +5212,21 @@ impl ModemEngine { let samples = self.stage_capture_input(Some(mode), device)?; let samples = self.route_audio_stage(PipelineStage::InputCapture, samples)?; - let raw_wire = { - let plugin = self - .plugins - .get(mode) - .ok_or_else(|| ModemError::PluginNotFound(mode.to_string()))?; - self.stage_demodulate_payload(plugin, mode, &samples)? - }; - let raw_wire = self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire)?; + // Both decision arms from one acquisition, adjudicated by RS + the length prefix + CRC-16 + // (#1428). The AFC estimate runs AFTER, so both arms demodulate at the same centre + // frequency — updating first would hand arm B a different `mod_cfg`. + let frame = self.decode_through_arms(mode, &samples, |bytes| { + let corrected = Self::rs_decode_free_strengthened_pure(bytes)?; + Frame::decode(&corrected) + })?; self.update_afc_estimate(mode, &samples.samples); self.emit_afc_update(mode); - let corrected_bytes = self.rs_decode_free_strengthened(&raw_wire.bytes)?; - let corrected_wire = WirePayload { - bytes: corrected_bytes, + let frame = DecodedFrame { + sequence: frame.sequence, + payload: frame.payload, }; - - let frame = self.stage_decode_frame(&corrected_wire)?; let frame = self.route_decoded_stage(PipelineStage::HpxStateUpdate, frame)?; info!("FEC receive: frame seq={}", frame.sequence); @@ -5261,25 +5281,27 @@ impl ModemEngine { let samples = self.stage_capture_input(Some(mode), device)?; let samples = self.route_audio_stage(PipelineStage::InputCapture, samples)?; - let raw_wire = { + let raw_variants = { let plugin = self .plugins .get(mode) .ok_or_else(|| ModemError::PluginNotFound(mode.to_string()))?; - self.stage_demodulate_payload(plugin, mode, &samples)? + self.stage_demodulate_variants(plugin, mode, &samples)? }; - let raw_wire = self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire)?; + + // Every decision arm, adjudicated by this chain's own FEC (#1428). Demodulated above, + // before the AFC update, so all arms share one centre frequency. + let corrected = self.decode_variants(mode, raw_variants, |bytes| { + let deinterleaved = Interleaver::new(interleaver_depth).deinterleave(bytes); + Ok(WirePayload { + bytes: FecCodec::new().decode(&deinterleaved)?, + }) + })?; self.update_afc_estimate(mode, &samples.samples); self.emit_afc_update(mode); - let deinterleaved = Interleaver::new(interleaver_depth).deinterleave(&raw_wire.bytes); - let corrected_bytes = FecCodec::new().decode(&deinterleaved)?; - let corrected_wire = WirePayload { - bytes: corrected_bytes, - }; - - let frame = self.stage_decode_frame(&corrected_wire)?; + let frame = self.stage_decode_frame(&corrected)?; let frame = self.route_decoded_stage(PipelineStage::HpxStateUpdate, frame)?; let _ = self.event_tx.send(EngineEvent::FrameReceived { mode: mode.to_string(), @@ -5340,22 +5362,26 @@ impl ModemEngine { let samples = self.stage_capture_input(Some(mode), device)?; let samples = self.route_audio_stage(PipelineStage::InputCapture, samples)?; - let raw_wire = { + let raw_variants = { let plugin = self .plugins .get(mode) .ok_or_else(|| ModemError::PluginNotFound(mode.to_string()))?; - self.stage_demodulate_payload(plugin, mode, &samples)? + self.stage_demodulate_variants(plugin, mode, &samples)? }; - let raw_wire = self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire)?; + + // Every decision arm, adjudicated by this chain's own FEC (#1428). Demodulated before the + // AFC update below, so every arm demodulates at the same centre frequency. + let corrected_wire = self.decode_variants(mode, raw_variants, |bytes| { + let conv = ConvCodec::new().decode(bytes)?; + Ok(WirePayload { + bytes: FecCodec::new().decode(&conv)?, + }) + })?; self.update_afc_estimate(mode, &samples.samples); self.emit_afc_update(mode); - let conv_decoded = ConvCodec::new().decode(&raw_wire.bytes)?; - let rs_decoded = FecCodec::new().decode(&conv_decoded)?; - let corrected_wire = WirePayload { bytes: rs_decoded }; - let frame = self.stage_decode_frame(&corrected_wire)?; let frame = self.route_decoded_stage(PipelineStage::HpxStateUpdate, frame)?; info!("concatenated FEC receive: frame seq={}", frame.sequence); @@ -5489,20 +5515,26 @@ impl ModemEngine { let samples = self.stage_capture_input(Some(mode), device)?; let samples = self.route_audio_stage(PipelineStage::InputCapture, samples)?; - let raw_wire = { + let raw_variants = { let plugin = self .plugins .get(mode) .ok_or_else(|| ModemError::PluginNotFound(mode.to_string()))?; - self.stage_demodulate_payload(plugin, mode, &samples)? + self.stage_demodulate_variants(plugin, mode, &samples)? }; - let raw_wire = self.route_wire_stage(PipelineStage::DemodulateDecode, raw_wire)?; + + // Every decision arm, adjudicated by this chain's own FEC (#1428). Demodulated before the + // AFC update below, so every arm demodulates at the same centre frequency. + let corrected_wire = self.decode_variants(mode, raw_variants, |bytes| { + Ok(WirePayload { + bytes: FecCodec::strong().decode(bytes)?, + }) + })?; self.update_afc_estimate(mode, &samples.samples); self.emit_afc_update(mode); - let rs_decoded = FecCodec::strong().decode(&raw_wire.bytes)?; - let frame = self.stage_decode_frame(&WirePayload { bytes: rs_decoded })?; + let frame = self.stage_decode_frame(&corrected_wire)?; let frame = self.route_decoded_stage(PipelineStage::HpxStateUpdate, frame)?; let _ = self.event_tx.send(EngineEvent::FrameReceived { mode: mode.to_string(), @@ -6874,23 +6906,25 @@ impl ModemEngine { let samples = self.stage_capture_input(Some(mode), device)?; let samples = self.route_audio_stage(PipelineStage::InputCapture, samples)?; - let wire = { + let raw_variants = { let plugin = self .plugins .get(mode) .ok_or_else(|| ModemError::PluginNotFound(mode.to_string()))?; - self.stage_demodulate_payload(plugin, mode, &samples)? + self.stage_demodulate_variants(plugin, mode, &samples)? }; - let wire = self.route_wire_stage(PipelineStage::DemodulateDecode, wire)?; + + // Every decision arm, adjudicated by this chain's own FEC (#1428). Demodulated before the + // AFC update below, so every arm demodulates at the same centre frequency. + let corrected_wire = self.decode_variants(mode, raw_variants, |bytes| { + Ok(WirePayload { + bytes: ShortFecCodec::with_ecc_len(Self::SHORT_FEC_DATA_ECC_LEN).decode(bytes)?, + }) + })?; self.update_afc_estimate(mode, &samples.samples); self.emit_afc_update(mode); - let corrected_bytes = - ShortFecCodec::with_ecc_len(Self::SHORT_FEC_DATA_ECC_LEN).decode(&wire.bytes)?; - let corrected_wire = WirePayload { - bytes: corrected_bytes, - }; let frame = self.stage_decode_frame(&corrected_wire)?; let frame = self.route_decoded_stage(PipelineStage::HpxStateUpdate, frame)?; let _ = self.event_tx.send(EngineEvent::FrameReceived { @@ -7364,6 +7398,28 @@ impl ModemEngine { } } + /// The SINGLE-arm hard demodulation. Five call sites remain, for four stated reasons (#1428). + /// + /// Every chain that hard-decodes a *FEC-protected frame* now goes through + /// [`decode_variants`](Self::decode_variants) instead, because the union needs an adjudicator + /// — RS plus the length prefix and CRC-16 — to say which arm was right. These four have none, + /// or are not the hard-BPSK family at all: + /// + /// - `receive_from_samples` (the UNCODED path) — for BPSK it never reaches this call: it + /// sign-slices `demodulate_soft` (the uncancelled arm) whenever the plugin advertises soft + /// demod, pinned by `the_uncoded_production_path_takes_the_uncancelled_arm`. So this site serves + /// hard-only modes, all of which offer one arm. Whether uncoded BPSK should take both is #1429, + /// and it is the maintainer's call. + /// - `receive_with_soft_combining` — an `instruments`-only sample-domain Memory-ARQ combiner: a + /// hard chain of the CANCELLED arm plus hard RS. Left single-arm because it ships in no binary + /// and nothing has measured the union on averaged samples; if it ships, it takes + /// `decode_through_arms`. + /// - `receive_window_retransmit_packet` — returns raw wire bytes with no FEC decode, so there + /// is nothing to arbitrate between arms. + /// - the two FSK4-ACK sites — a different plugin, which offers one arm. + /// + /// If you add a hard-decode chain, use `decode_variants`; reaching for this function means + /// asserting one of the four reasons above applies, so say which. fn stage_demodulate_payload( &self, plugin: &dyn openpulse_core::plugin::ModulationPlugin, @@ -7385,6 +7441,115 @@ impl ModemEngine { Ok(WirePayload { bytes: wire_bytes }) } + /// Every hard-decision wire candidate for one captured slice, descrambled (#1428). + /// + /// The variant-aware sibling of [`stage_demodulate_payload`](Self::stage_demodulate_payload), + /// carrying the identical `mod_cfg` construction and the identical `scramble::scramble` + /// un-whitening, so the arms cannot differ from the single-arm path by anything except the + /// plugin's own decision rule. + fn stage_demodulate_variants( + &self, + plugin: &dyn openpulse_core::plugin::ModulationPlugin, + mode: &str, + samples: &AudioSamples, + ) -> Result, ModemError> { + let _stage = PipelineStage::DemodulateDecode; + let mod_cfg = ModulationConfig { + mode: mode.to_string(), + center_frequency: self.center_frequency + self.afc_correction_hz, + afc_correction_hz: self.afc_correction_hz, + ..ModulationConfig::default() + }; + let variants = plugin.demodulate_variants(&samples.samples, &mod_cfg)?; + Ok(variants + .into_iter() + .map(|mut bytes| { + openpulse_core::scramble::scramble(&mut bytes); + WirePayload { bytes } + }) + .collect()) + } + + /// Demodulate `samples` into every arm the mode offers and return the first that DECODES. + /// + /// This is the single hard-decision decode seam (#1428). Before it, `stage_demodulate_payload` + /// had eleven callers, each open-coding demod → route → FEC → frame, and a property wired into + /// one of them was absent from the other ten. That is the same duplicated-open-coding shape that + /// let #1433 sit inside the plugin for 71 days (the GPU path's own copy of the slice lacked the + /// cancellation), one layer up — #1433 itself was never an engine-seam defect. + /// + /// **`decode` takes bytes, not `&mut Self`, on purpose.** A closure that cannot reach the + /// engine cannot move AFC, HARQ retention, the rate controller or the SNR record while a + /// losing arm runs. That makes "a losing arm leaves no trace" a property of the signature + /// rather than a promise in a comment. + /// + /// The winner's tail — `HpxStateUpdate`, `FrameReceived`, the SNR record — stays at the call + /// site and runs once, on the returned bytes. + fn decode_through_arms( + &mut self, + mode: &str, + samples: &AudioSamples, + decode: impl Fn(&[u8]) -> Result, + ) -> Result { + let variants = { + let plugin = self + .plugins + .get(mode) + .ok_or_else(|| ModemError::PluginNotFound(mode.to_string()))?; + self.stage_demodulate_variants(plugin, mode, samples)? + }; + self.decode_variants(mode, variants, decode) + } + + /// The decode half of [`decode_through_arms`](Self::decode_through_arms). + /// + /// Split out because `receive_from_samples_with_fec_inner` demodulates BEFORE + /// `update_afc_estimate` and decodes after it. Folding both halves into one call there would + /// move the demodulation to the far side of the AFC update, so arm B would run at a different + /// `center_frequency` than arm A — the #1428 harness's trap 2, reintroduced in production. + fn decode_variants( + &mut self, + mode: &str, + variants: Vec, + decode: impl Fn(&[u8]) -> Result, + ) -> Result { + let arms = variants.len(); + let mut last_err = None; + for (idx, wire) in variants.into_iter().enumerate() { + let wire = self.route_wire_stage(PipelineStage::DemodulateDecode, wire)?; + match decode(&wire.bytes) { + Ok(out) => { + if idx > 0 { + // Tripwire: stays zero if a later arm is never reached or never wins, which + // is indistinguishable from the union being unwired without a counter. + self.alternate_arm_decodes = self.alternate_arm_decodes.saturating_add(1); + debug!( + "union: arm {idx} of {arms} produced the frame; arm 0 failed at this attempt (mode={mode})" + ); + } + return Ok(out); + } + Err(e) => last_err = Some(e), + } + } + Err(last_err.unwrap_or_else(|| { + ModemError::Demodulation("no demodulation variants produced".into()) + })) + } + + /// Accepted frames produced by a variant other than variant 0, since start-up (#1428). + /// + /// **Wiring evidence, not a rescue count.** Arm 0 keeps first claim on each *attempt*, not on + /// each *frame*: a later arm can win at an onset that arm 0 would have passed, where arm 0 would + /// have won at a later onset anyway. Measured through `ota_decode_burst` on a `moderate_f1` fade: + /// this counter read 26 while only 18 of those frames were ones arm 0 cannot decode at all. So a + /// non-zero value proves a later arm is reached and can win, which is what a tripwire needs; it + /// does not measure the union's gain. That comes from comparing against a variant-0-only build. + #[cfg(feature = "instruments")] + pub fn alternate_arm_decodes(&self) -> u64 { + self.alternate_arm_decodes + } + fn stage_decode_frame(&self, wire: &WirePayload) -> Result { let _stage = PipelineStage::DemodulateDecode; let frame = Frame::decode(&wire.bytes)?; @@ -7406,11 +7571,17 @@ impl ModemEngine { /// use. A t=16 candidate is accepted only if its frame validates; otherwise the strong decode /// is tried. fn rs_decode_free_strengthened(&self, bytes: &[u8]) -> Result, ModemError> { + Self::rs_decode_free_strengthened_pure(bytes) + } + + /// `self`-free form of [`rs_decode_free_strengthened`](Self::rs_decode_free_strengthened). + /// + /// The `&self` receiver only ever reached `stage_decode_frame`, which is `Frame::decode` and + /// pure. Exposing the pure form lets `decode_through_arms`' closure — deliberately given no + /// `&mut Self` (#1428) — run the same arbitration a losing arm must not be able to side-effect. + fn rs_decode_free_strengthened_pure(bytes: &[u8]) -> Result, ModemError> { if let Ok(d) = FecCodec::new().decode(bytes) { - if self - .stage_decode_frame(&WirePayload { bytes: d.clone() }) - .is_ok() - { + if Frame::decode(&d).is_ok() { return Ok(d); } } @@ -7420,12 +7591,14 @@ impl ModemEngine { /// `decode_prefix` variant of [`rs_decode_free_strengthened`](Self::rs_decode_free_strengthened) /// for the scanning receive, whose input length is a function of the capture window rather than /// the frame. - fn rs_decode_prefix_free_strengthened(&self, bytes: &[u8]) -> Result, ModemError> { + /// `self`-free by construction, for `decode_variants`' closure. See + /// [`rs_decode_free_strengthened_pure`](Self::rs_decode_free_strengthened_pure). + /// + /// The `&self` wrapper this replaced lost its last caller when every scanning hard chain moved + /// to `decode_variants` (#1428). + fn rs_decode_prefix_free_strengthened_pure(bytes: &[u8]) -> Result, ModemError> { if let Ok(d) = FecCodec::new().decode_prefix(bytes) { - if self - .stage_decode_frame(&WirePayload { bytes: d.clone() }) - .is_ok() - { + if Frame::decode(&d).is_ok() { return Ok(d); } } diff --git a/crates/openpulse-modem/tests/daemon_frequency_acquisition.rs b/crates/openpulse-modem/tests/daemon_frequency_acquisition.rs index 9e69dd4e..65c35134 100644 --- a/crates/openpulse-modem/tests/daemon_frequency_acquisition.rs +++ b/crates/openpulse-modem/tests/daemon_frequency_acquisition.rs @@ -9,10 +9,23 @@ //! `daemon_vs_cli_on_real_captures::m2_carrier_offset_sweep_cli_vs_daemon`. //! //! **Which arm was broken, measured rather than assumed.** The uncoded arm (`decode_burst`) already -//! tolerated 50 Hz natively — it needs the acquisition pass only past ~200 Hz. The coded arm -//! (`ota_decode_burst`) failed from 50 Hz, which is the requirement bound, so that is where the -//! defect lived. Measured on this file's own fixture, uncoded frame through the uncoded arm: -//! 0 Hz and 50 Hz decode with **0** settles; 200 Hz and 400 Hz decode with 126. Both arms get the +//! tolerated 50 Hz natively; the coded arm (`ota_decode_burst`) failed from 50 Hz, which is the +//! requirement bound, so that is where the defect lived. Measured on this file's own fixture, +//! uncoded frame through the uncoded arm: 0 Hz and 50 Hz decode with **0** settles; 200 Hz and +//! 400 Hz decode with 126. +//! +//! **Corrected 2026-09-23 (#1428): this used to add "it needs the acquisition pass only past +//! ~200 Hz", an interpolation across those four points that is false.** Measured in review (Fable, +//! 2026-09-22/23) across all 64 sub-symbol alignments of this fixture: the uncancelled arm (which the +//! uncoded arm runs) decodes 40/64 at 50 Hz, 0/64 at 62.5, 75, 100 and 200 Hz, and 52/64 at 250 Hz; +//! the cancelled arm decodes 0/64 below 250 Hz and 49/64 there. #1428's own daemon sweep of this +//! fixture (six realisations) needed the acquisition pass on all six at 75–200 Hz and on three at +//! 300 Hz. A review MODEL — not a measurement — has the crossfade term rotating against the symbol's +//! own energy by ~0.4°/Hz, which would make native tolerance non-monotonic in offset. It is also +//! frame-dependent: the same review measured the preamble timing correlation at ~5 % of its peak at +//! 50 Hz, and a different 200 B frame mis-locked and failed both arms at 25 Hz. **Do not read +//! "decodes natively at X Hz" from this fixture as a property of the receiver** — the acquisition +//! pass is what REQ-PHY-03 rests on. Both arms get the //! pass, because both are reachable on a shipping station and the uncoded one is what carries //! station ID, filexfer, handshake, QSY and relay traffic. //! @@ -42,6 +55,19 @@ const SAMPLE_RATE: usize = 8_000; /// requirement names, and the daemon failed at exactly this offset before #1118. const REQUIRED_OFFSET_HZ: f32 = 50.0; +/// An offset where NEITHER hard-decision arm decodes without the acquisition pass, so a decode here +/// is attributable to acquisition and nothing else (#1428). +/// +/// The differential detector's decision variable rotates by 360°·f/250 per symbol at 250 baud: +/// 100 Hz is 144° (cos −0.81), deep in the inverted lobe, so neither arm can decode it unaided. Two +/// nearby values are avoided on purpose — 62.5 Hz is exactly 90°, the detector's own null and a +/// knife-edge; 250 Hz is 360°, where the detector decodes natively again. +/// +/// Measured, from two sources: across all 64 sub-symbol alignments of this fixture both arms decode +/// 0/64 at 100 Hz (review, Fable, 2026-09-22/23); and #1428's daemon sweep spent the full acquisition +/// pass on every realisation at 100 Hz, in both the union build and a variant-0-only build. +const ACQUISITION_OFFSET_HZ: f32 = 100.0; + /// A burst as the daemon would hear it: real recorded idle, a frame shifted by `offset_hz`, more idle. fn burst_at(offset_hz: f32) -> Vec { burst_at_fec(offset_hz, FEC) @@ -114,22 +140,48 @@ fn via_daemon(samples: &[f32]) -> (bool, u64) { (ok, e.afc_settle_attempts()) } -/// The requirement itself, on the surface that was failing it. +/// The requirement itself, at the bound it names, on the surface that was failing it. +/// +/// **This asserts the decode and nothing about HOW.** It used to also require `settles > 0`, as +/// proof the acquisition pass did the work. Since #1428 the union's uncancelled arm decodes THIS +/// fixture at 50 Hz with no acquisition at all (measured: 0 settles, against 198 for the cancelled +/// arm alone over six realisations), so that guard was measuring the fixture's luck rather than the +/// requirement — and per the header, that luck is frame-dependent. The mechanism is gated +/// separately, at an offset where no arm can reach natively: +/// [`the_acquisition_pass_recovers_a_station_beyond_native_reach`]. /// // VERIFIES: REQ-PHY-03 #[test] -fn the_daemon_acquires_a_station_fifty_hz_off_frequency() { - let (ok, settles) = via_daemon(&burst_at(REQUIRED_OFFSET_HZ)); +fn the_daemon_decodes_a_station_fifty_hz_off_frequency() { + let (ok, _settles) = via_daemon(&burst_at(REQUIRED_OFFSET_HZ)); assert!( ok, "the daemon did not decode a frame {REQUIRED_OFFSET_HZ} Hz off frequency — REQ-PHY-03 \ requires tracking station-to-station offsets to ±50 Hz without operator intervention, and \ this is the streaming path a shipping station actually receives on" ); +} + +/// The acquisition pass itself — the mechanism REQ-PHY-03 actually rests on. +/// +/// At [`ACQUISITION_OFFSET_HZ`] neither decision arm decodes without acquisition, so a decode here +/// can only have come from the pass. The settle assertion is the anti-vacuity guard the 50 Hz test +/// had to give up: it is meaningful here and was not there. +/// +// VERIFIES: REQ-PHY-03 +#[test] +fn the_acquisition_pass_recovers_a_station_beyond_native_reach() { + let (ok, settles) = via_daemon(&burst_at(ACQUISITION_OFFSET_HZ)); + assert!( + ok, + "the daemon did not decode a frame {ACQUISITION_OFFSET_HZ} Hz off frequency — at this \ + offset only the acquisition pass can recover it, so the pass is broken" + ); assert!( settles > 0, - "decoded with zero settle attempts, so the acquisition pass is not what recovered it and \ - this gate is not measuring what it claims" + "decoded {ACQUISITION_OFFSET_HZ} Hz off with zero settle attempts, so the acquisition pass \ + is not what recovered it. Either a decision arm now reaches this offset natively (re-measure \ + and move ACQUISITION_OFFSET_HZ), or this gate is no longer measuring what it claims" ); } diff --git a/crates/openpulse-modem/tests/engine_cancellation_ab.rs b/crates/openpulse-modem/tests/engine_cancellation_ab.rs index 1e05fa71..18fefafa 100644 --- a/crates/openpulse-modem/tests/engine_cancellation_ab.rs +++ b/crates/openpulse-modem/tests/engine_cancellation_ab.rs @@ -1,5 +1,16 @@ //! #1428 step 1 — the ENGINE-level soft-vs-hard A/B, with real RS and the scrambler. //! +//! **SINCE #1428's UNION LANDED, THE SECOND COLUMN IS NO LONGER THE CANCELLED ARM.** Its entry, +//! `receive_with_fec_mode(Rs)` → `receive_with_fec`, now decodes BOTH decision arms and keeps the +//! first that RS accepts. So this harness now measures **uncancelled vs union**, not uncancelled vs +//! cancelled, and its "hard"/"H-only" columns are the union's. Expect the union column to be at +//! least the soft column in every cell. A soft-only frame would indicate the soft sign-slice and +//! variant 1 have diverged — nothing pins them bit-identical for BPSK (`hard_variant_conformance`'s +//! I3 skips two-variant plugins) — so treat one as a finding to check, not as noise. +//! The numbers recorded against this harness in the traceability ledger (2026-09-22) predate the +//! union and ARE the cancelled-vs-uncancelled comparison; do not re-run this and compare to them. +//! The labels below are kept as printed so those recorded tables stay readable. +//! //! **What is new here, stated checkably.** #1363 opened with engine-level frame counts, so this is //! not the thread's first decode rate. It is the first since that opening, and the first whose //! apparatus is known to put BOTH arms through the same hard RS — the opening left that open, and @@ -102,6 +113,7 @@ fn engine_soft_vs_hard_paired() { let tx_rms = (tx.iter().map(|s| s * s).sum::() / tx.len() as f32).sqrt(); let sigma09_db = 20.0 * (tx_rms / 0.9).log10(); println!("\nPAIRED-AB-1428 {SEEDS} seeds {MODE} 200 B; tx len {} rms {tx_rms:.4}; sigma0.9 = {sigma09_db:.2} dB", tx.len()); + println!(" (since #1428: \"hard\" = the UNION of both arms, not the cancelled arm alone)"); println!(" cell | soft | hard | diff | both | S-only | H-only | neither | McNemar p"); let cells: Vec = vec![ diff --git a/crates/openpulse-modem/tests/hard_variant_conformance.rs b/crates/openpulse-modem/tests/hard_variant_conformance.rs new file mode 100644 index 00000000..59931b40 --- /dev/null +++ b/crates/openpulse-modem/tests/hard_variant_conformance.rs @@ -0,0 +1,234 @@ +//! Every plugin's `demodulate_variants` obeys the union contract — swept, not listed (#1428). +//! +//! The engine's hard-decode chains try each variant in turn and keep the first whose FEC and frame +//! decode succeed. Three things must hold for that to be sound, and all three are checked for +//! **every mode every registered plugin claims**, because a hand-written list of (plugin, mode) +//! pairs is the inventory-rot shape this repo has paid for repeatedly. +//! +//! - **I1 — variant 0 IS `demodulate`.** The rest of the trait contract hangs off `demodulate` +//! (`soft_demod_conformance` checks the soft arm against it), so variant 0 must keep meaning it. +//! Checked on a clean AND a noisy input, because a plugin could agree on one and not the other. +//! - **I2 — no duplicate variants, under noise.** A duplicate costs a wasted FEC trial on every +//! onset of the scanning receive and can be counted as a second arm contributing when it cannot. +//! **Under noise specifically**: BPSK's two arms are byte-identical on a clean fixture — the +//! crossfade bias flips no decision without noise — so a clean-input version of this assertion +//! fails for a reason that is not a defect. That is the useful form of the fact: *an invariant +//! about two arms differing is only meaningful where they are meant to differ.* +//! - **I3 — a single-variant plugin really has one arm.** For a plugin declaring one variant, +//! `hard_decide(demodulate_soft)` must reproduce `demodulate()` on a NOISY input. This is the +//! measurement, not an assumption: OFDM and SC-FDMA carry separate soft and hard implementations, +//! and nothing previously pinned them sign-equivalent under noise — `soft_demod_conformance`'s +//! implication (B) is noiseless by its own doc. **A failure here is a finding — an undeclared +//! second arm — not a bug in this test.** +//! +//! What this does NOT cover: the GPU variants paths. `Plugin::new()` is the CPU constructor while +//! the daemon builds `with_gpu` by default, and this gate runs `--no-default-features`. That gap is +//! exactly how #1433 survived 71 days, and `plugins/bpsk/tests/gpu_cpu_equivalence.rs` is the +//! only thing covering it. + +use openpulse_core::fec::hard_decide; +use openpulse_core::plugin::{ModulationConfig, ModulationPlugin}; + +/// Modes that cannot be driven at 8 kHz; mirrored from `soft_demod_conformance`'s own list so the +/// two sweeps agree about what is undrivable rather than each maintaining a private opinion. +fn undrivable(mode: &str) -> bool { + let src = include_str!("soft_demod_conformance.rs"); + let list = src + .split("const UNDRIVABLE_AT_8K") + .nth(1) + .and_then(|s| s.split('[').nth(1)) + .and_then(|s| s.split(']').next()) + .unwrap_or(""); + list.split(',') + .map(|s| s.trim().trim_matches('"')) + .any(|m| !m.is_empty() && m == mode) +} + +fn config_for(mode: &str) -> ModulationConfig { + ModulationConfig { + mode: mode.to_string(), + ..ModulationConfig::default() + } +} + +/// Deterministic AWGN at a given total-power SNR. Box-Muller over an LCG. +fn awgn(signal: &[f32], snr_db: f32, seed: u64) -> Vec { + let p: f32 = signal.iter().map(|s| s * s).sum::() / signal.len().max(1) as f32; + let sigma = (p / 10f32.powf(snr_db / 10.0)).sqrt(); + let mut st = seed.wrapping_mul(0x9E37_79B9_7F4A_7C15).wrapping_add(1); + let mut u = || -> f32 { + st = st + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + ((st >> 11) as f32 / (1u64 << 53) as f32).clamp(1e-9, 1.0 - 1e-9) + }; + signal + .iter() + .map(|&s| { + let (a, b) = (u(), u()); + s + sigma * (-2.0 * a.ln()).sqrt() * (std::f32::consts::TAU * b).cos() + }) + .collect() +} + +fn plugins() -> Vec> { + vec![ + Box::new(bpsk_plugin::BpskPlugin::new()), + Box::new(fsk4_plugin::Fsk4Plugin::new()), + Box::new(mfsk16_plugin::Mfsk16Plugin::new()), + Box::new(ofdm_plugin::OfdmPlugin::new()), + Box::new(psk8_plugin::Psk8Plugin::new()), + Box::new(qam64_plugin::Qam64Plugin::new()), + Box::new(qpsk_plugin::QpskPlugin::new()), + Box::new(scfdma_plugin::ScFdmaPlugin::new()), + Box::new(pilot_plugin::PilotPlugin::new()), + ] +} + +/// Per-mode check. Returns the reason on failure so a sabotage test can require one. +fn check_mode(plugin: &dyn ModulationPlugin, mode: &str) -> Result { + let cfg = config_for(mode); + let payload: Vec = (0..96u32) + .map(|i| (i.wrapping_mul(131) >> 2) as u8) + .collect(); + let Ok(tx) = plugin.modulate(&payload, &cfg) else { + return Ok(false); + }; + if tx.is_empty() { + return Ok(false); + } + + // The SNRs at which a declared second arm must prove itself, as a LADDER rather than a point. + // + // A single absolute SNR cannot work for a registry sweep, because each mode's arms diverge at + // its own operating point and processing gain spans ~9 dB across this registry alone. Measured + // while writing this test: at 6 dB total power BPSK250 sits near 21 dB Eb/N0 and its two arms + // are byte-identical; BPSK31, at 256 samples/symbol (~24 dB of gain), is still identical at + // −6 dB. The ladder therefore runs down to where any mode here is unusable, and a mode that + // fails to demodulate at the bottom rungs simply contributes nothing — `demodulate_variants` + // returning `Err` is skipped, not counted as agreement. + const ARM_SNRS: [f32; 5] = [0.0, -6.0, -12.0, -18.0, -24.0]; + + if let Ok(v) = plugin.demodulate_variants(&tx, &cfg) { + if v.len() > 1 { + let mut ever_differed = false; + for snr in ARM_SNRS { + for seed in 0..4u64 { + if let Ok(vs) = plugin.demodulate_variants(&awgn(&tx, snr, seed), &cfg) { + if vs.len() > 1 && vs.windows(2).any(|w| w[0] != w[1]) { + ever_differed = true; + } + } + } + } + if !ever_differed { + return Err(format!( + "{mode}: declares {} arms, but they produce identical bytes at every tested \ + SNR — a duplicate costs an FEC trial per onset and cannot rescue a frame", + v.len() + )); + } + } + } + + for (label, rx) in [("clean", tx.clone()), ("noisy", awgn(&tx, 6.0, 11))] { + let Ok(variants) = plugin.demodulate_variants(&rx, &cfg) else { + continue; + }; + if variants.is_empty() { + return Err(format!("{mode}: demodulate_variants returned no variants")); + } + let Ok(shipped) = plugin.demodulate(&rx, &cfg) else { + continue; + }; + + // I1 + if variants[0] != shipped { + return Err(format!( + "{mode} ({label}): variant 0 is not demodulate()'s bytes — the trait contract \ + hangs off demodulate, so variant 0 must reproduce it" + )); + } + + // I3, noisy only: a plugin claiming ONE arm must not have a second one hiding in its + // soft path. A failure is an undeclared arm, i.e. a finding about that plugin. + if label == "noisy" && variants.len() == 1 { + if let Ok(llrs) = plugin.demodulate_soft(&rx, &cfg) { + let decided = hard_decide(&llrs); + // Compare DECISIONS over the common prefix, not `Vec` equality. The two paths can + // legitimately return different LENGTHS at low SNR — measured, OFDM52-8PSK at 6 dB + // returns 111 B hard against 96 B soft-decided with ZERO differing bits — and a + // bare `!=` reports that as a decision divergence. An earlier draft of this test + // did exactly that and produced four false "undeclared second arm" findings. + let n = decided.len().min(shipped.len()); + let differing: u32 = decided[..n] + .iter() + .zip(&shipped[..n]) + .map(|(a, b)| (a ^ b).count_ones()) + .sum(); + if n >= 32 && differing > 0 { + return Err(format!( + "{mode}: declares ONE hard arm, but hard-deciding demodulate_soft \ + disagrees with demodulate() on {differing} bit(s) of the first {n} \ + bytes under noise — that is an undeclared second arm, and the union \ + could be using it. Investigate the plugin, not this test." + )); + } + } + } + } + Ok(true) +} + +#[test] +fn every_plugin_obeys_the_hard_variant_contract() { + let ps = plugins(); + let (mut checked, mut failures) = (0usize, Vec::new()); + for p in &ps { + for mode in &p.info().supported_modes { + if undrivable(mode) { + continue; + } + match check_mode(p.as_ref(), mode) { + Ok(true) => checked += 1, + Ok(false) => {} + Err(e) => failures.push(e), + } + } + } + assert!( + failures.is_empty(), + "{} mode(s) break the hard-variant contract:\n {}", + failures.len(), + failures.join("\n ") + ); + assert!( + checked >= 40, + "only {checked} modes were actually exercised; the sweep has gone mostly vacuous" + ); + println!( + "hard-variant contract: {checked} modes checked across {} plugins", + ps.len() + ); +} + +/// BPSK must actually DECLARE two arms — otherwise the sweep above passes on a build where the +/// union was never wired, which is the state this whole change exists to leave. +#[test] +fn bpsk_declares_its_second_arm() { + let p = bpsk_plugin::BpskPlugin::new(); + let cfg = config_for("BPSK250"); + let payload: Vec = (0..96u32) + .map(|i| (i.wrapping_mul(131) >> 2) as u8) + .collect(); + let tx = p.modulate(&payload, &cfg).expect("modulate"); + let v = p + .demodulate_variants(&awgn(&tx, 0.0, 3), &cfg) + .expect("variants"); + assert_eq!( + v.len(), + 2, + "BPSK250 must offer the cancelled and uncancelled arms (#1428); it offered {}", + v.len() + ); +} diff --git a/crates/openpulse-modem/tests/union_second_arm_wiring.rs b/crates/openpulse-modem/tests/union_second_arm_wiring.rs new file mode 100644 index 00000000..89a27df8 --- /dev/null +++ b/crates/openpulse-modem/tests/union_second_arm_wiring.rs @@ -0,0 +1,101 @@ +//! BPSK's second decision arm is reached, and wins frames, through the production entry (#1428). +//! +//! **What this asserts, and what it does not.** It asserts WIRING: that the non-primary arm is +//! reached and can produce a frame on a fade, and that it is never credited on a clean channel. +//! It does NOT assert that the union decodes more frames than variant 0 alone — the tripwire it +//! reads counts "a later arm produced this frame at this attempt", which is not "arm 0 could not +//! have decoded it" (measured 26 credits against 18 genuine rescues; see `alternate_arm_decodes`). +//! +//! The GAIN was measured against a variant-0-only build, paired on identical channel realisations: +//! +10/48 through `receive_with_fec_mode` and +18/96 through `ota_decode_burst` on `moderate_f1` +//! @ 8 dB (200 B, plain `Rs`), with zero frames lost in either. That comparison needs a second +//! build, so it lives in the traceability ledger, not in this file. +use openpulse_audio::LoopbackBackend; +use openpulse_channel::{watterson::WattersonChannel, ChannelModel, WattersonConfig}; +use openpulse_core::fec::FecMode; +use openpulse_modem::engine::ModemEngine; + +const MODE: &str = "BPSK250"; + +fn engine() -> (ModemEngine, LoopbackBackend) { + let b = LoopbackBackend::new(); + let mut e = ModemEngine::new(Box::new(b.clone_shared())); + e.register_plugin(Box::new(bpsk_plugin::BpskPlugin::new())) + .expect("register"); + (e, b) +} + +fn payload() -> Vec { + (0..200u32) + .map(|i| (i.wrapping_mul(2654435761) >> 13) as u8) + .collect() +} + +/// A non-primary arm is reached and produces frames on a fade — counted, not assumed. +#[test] +fn a_non_primary_arm_produces_frames_on_a_fade() { + let p = payload(); + let tx = { + let (mut e, b) = engine(); + e.transmit_with_fec_mode(&p, MODE, FecMode::Rs, None) + .expect("tx"); + b.drain_samples() + }; + + let (mut decoded, mut alt_total) = (0u32, 0u64); + let seeds = 48u64; + for seed in 0..seeds { + let mut cfg = WattersonConfig::moderate_f1(Some(seed)); + cfg.snr_db = 8.0; + let rx = WattersonChannel::new(cfg).expect("chan").apply(&tx); + + let (mut e, b) = engine(); + b.push_frame(&rx); + if e.receive_with_fec_mode(MODE, FecMode::Rs, None) + .is_ok_and(|d| d == p) + { + decoded += 1; + } + alt_total += e.alternate_arm_decodes(); + } + + println!( + "moderate_f1 @ 8 dB: {decoded}/{seeds} decoded, {alt_total} produced by a non-primary arm" + ); + + assert!( + decoded > 0, + "nothing decoded at all — the fixture is not exercising the decode path" + ); + assert!( + alt_total > 0, + "the union decoded {decoded}/{seeds} frames but NOT ONE came from a non-primary arm. \ + Either the second arm is never reached, or it never wins — and both are \ + indistinguishable from the union being unwired, which is what this tripwire exists to \ + tell apart." + ); +} + +/// The tripwire stays at zero on a clean channel, where arm 0 wins every attempt — so a non-zero count above is +/// attributable to the fade, not to the counter incrementing on every decode. +#[test] +fn a_clean_channel_needs_no_second_arm() { + let p = payload(); + let (mut e, b) = engine(); + e.transmit_with_fec_mode(&p, MODE, FecMode::Rs, None) + .expect("tx"); + let tx = b.drain_samples(); + + let (mut e, b) = engine(); + b.push_frame(&tx); + let got = e + .receive_with_fec_mode(MODE, FecMode::Rs, None) + .expect("rx"); + assert_eq!(got, p, "the clean-channel control must decode"); + assert_eq!( + e.alternate_arm_decodes(), + 0, + "arm 0 decoded a clean frame, so no later arm should have been credited — a non-zero \ + count here would mean the tripwire fires on ordinary decodes and proves nothing" + ); +} diff --git a/docs/dev/plugin-trait-versioning.md b/docs/dev/plugin-trait-versioning.md index 4fdff5b7..c14491e8 100644 --- a/docs/dev/plugin-trait-versioning.md +++ b/docs/dev/plugin-trait-versioning.md @@ -2,7 +2,7 @@ project: openpulsehf doc: docs/dev/plugin-trait-versioning.md status: living -last_updated: 2026-04-24 +last_updated: 2026-09-23 --- # Plugin Trait Versioning and Compatibility @@ -39,10 +39,22 @@ Plugin trait compatibility is tracked via **semantic versioning** applied to the .. ``` -The current trait version is **`2.0.0`** — bumped from `1.1.0` by #1053, which changed -`ModulationPlugin::preamble_template` to return `PreambleTemplate` (samples bundled with the ρ -constants measured for that waveform) instead of bare `Vec`. See `docs/dev/project/traceability.md` -for the rationale and the one-line migration. +The current trait version is **`3.1.0`**. Recent history, newest first: + +- **`3.1.0`** (#1428, minor) — added `ModulationPlugin::demodulate_variants`, every hard-decision + wire a mode can produce from one acquisition. Additive with a default body returning + `vec![self.demodulate(..)?]`, so every existing plugin compiles unchanged; BPSK overrides it to + offer both crossfade-cancellation arms. +- **`3.0.0`** (PR #1071, commit `f3c58ce3`, 2026-08-04, major) — `PreambleTemplate` gained a required + `for_mode` field binding its ρ constants to the one mode they were measured for. +- **`2.0.0`** (#1053, major) — `preamble_template` returns `PreambleTemplate` (samples bundled with + the ρ constants measured for that waveform) instead of bare `Vec`. + +**Corrected 2026-09-23:** this line said `2.0.0` for seven weeks after the constant had become +`3.0.0` — the bump in `f3c58ce3` updated `plugin.rs` and not this document. The constant is the +canonical source (below); when you change it, change this list in the same commit. + +See `docs/dev/project/traceability.md` for rationale and migrations. ### Trait Version Identification diff --git a/docs/dev/project/traceability.md b/docs/dev/project/traceability.md index f1debbe5..fb8ef557 100644 --- a/docs/dev/project/traceability.md +++ b/docs/dev/project/traceability.md @@ -15,6 +15,85 @@ and the actually-observed results per change. --- +## 2026-09-23 — #1428 the union: both crossfade arms, adjudicated by the FEC + +**Requirement/change.** #1363 / #1428: BPSK's crossfade-ISI cancellation wins AWGN decisively and +loses on `moderate_f1` (harm localised to delayed-dominant dips, 2026-09-22 entry). In #1428 step 1 +(PR #1432 — soft-uncancelled against hard-cancelled, union computed from the discordant pairs) that +was 96/96 against 12/96 at −2 dB AWGN and 38/96 against 49/96 at `moderate_f1` 8 dB, union 52/96. +Neither arm dominates. The union demodulates both from one acquisition and keeps the first that RS, +the 4-byte length prefix and CRC-16 accept — no predicate, where the per-symbol gate it replaces +needed a fitted threshold. + +**Design**, reviewed before the code it covered; the reviews are recorded in +`docs/dev/reviews/review-1428-union.md`: +- `ModulationPlugin::demodulate_variants`, additive with a default body (trait `3.0.0` → `3.1.0`). + BPSK returns cancelled then uncancelled from ONE timing search and ONE `demodulate_iq`; `-RRC` + returns one arm, since it does not crossfade. +- BPSK's override carries its own GPU branch. The trait default would have returned ONE variant on + the GPU daemon while CPU tests saw two — #1433's shape, one method over. +- One hard-decode seam, `decode_through_arms` / `decode_variants`. `stage_demodulate_payload` had + eleven callers; the six FEC-protected chains now go through the seam, and the five call sites that + remain carry four stated reasons at the function. The decode closure takes `&[u8]`, not + `&mut Self`, so a losing arm cannot move AFC, HARQ retention, the rate controller or the SNR record. +- `decode_variants` is split from the demodulation because `receive_from_samples_with_fec_inner` + runs `update_afc_estimate` between them; folding them would demodulate arm 1 at a different centre + frequency than arm 0. + +**Measured**, each paired against a variant-0-only build on identical channels. Every fixture is +BPSK250 + `Rs` on synthetic channels. +- **Gain, and where it was and was not shown.** On 200 B plain-`Rs` frames at `moderate_f1` @ 8 dB: + +10/48 via `receive_with_fec_mode` and +18/96 via `ota_decode_burst`. On 29 B frames (which + `free_rs_strengthening` upgrades to t = 32) the union matched arm 0 in 7 of 8 `moderate_f1` cells + and was +1 in the eighth, and gained +17/384 on a 0.01 Hz fade. The 29 B sweep also set noise from + the unfaded frame's RMS in pure AWGN, where the 200 B runs embedded the frame in recorded idle — + so the two are not one comparison, and which difference removed the `moderate_f1` gain is untested. +- **Frames lost to the union: zero** in every paired run — 96 `ota_decode_burst` seeds, 288 AWGN + pairs, 768 fading pairs. On the single-shot `receive_with_fec_mode` path this is structural rather + than measured: arm 0 is tried first on the same buffer. +- **Cost:** per-burst ratio 1.017, 95 % CI [0.93, 1.10], nine within-round pairs. Arm 1 ran on ~126 + attempts per burst — this fixture's onset-scan geometry, not a property of the union. By structure + the second arm is a few per cent of an attempt; the interval is consistent with that and cannot + resolve it. +- **SNR on frames credited to arm 1 reads low:** paired on the 8 seeds both builds decode, median + −2.97 dB (−0.70 to −6.88), negative on all 8; one frame only the union decodes read −19.3 dB. + Probable cause, unmeasured (the discriminating test is in the follow-up issue): `estimate_snr_db` + rebuilds symbols from the CANCELLED arm's decisions, and a wrong decision leaves the window holding + it largely booked as noise. Not new, but exercised more often. On the ladder, measured: a decoded + frame is never answered with a demotion (`a_decoded_frame_is_never_answered_with_a_demotion`, fed + −20 dB). By code read: the evidence climb does not read the SNR, and the hard arm records no SNR, + so `last_rx_snr_db()` (QSY scan, ADIF) never sees it. It does reach operators, via + `OtaRateDecision`. +- **AFC:** at 50 Hz the uncancelled arm can decode a burst before the settle runs. At offset 0 that + commits no correction; in the onset scan it commits the fine estimate at `afc_step = 0.1`, so the + correction converges ~10 % per burst instead of in one settle (measured 5.0 → 23.5 Hz over six + transmissions). AWGN, −4 to +12 dB, 288 pairs: zero lost; on the first burst the skip occurs only + from +8 dB. Fading, `moderate_f1` and a 0.01 Hz fade chosen because it measurably swings burst to + burst (9/84 consecutive drops > 6 dB, against 0/84 for `moderate_f1`), 768 pairs: zero lost. One + union-specific excursion: an arm-1 win moved a correct 51.0 Hz to 63.4 Hz on a transmission the + variant-0-only build failed to decode. + +**Gates.** +- `daemon_frequency_acquisition` split: 50 Hz asserts the decode only (REQ-PHY-03 at its bound); + 100 Hz — where neither arm decodes unaided — asserts the decode AND that the acquisition pass ran. + Sabotage: destroying the settle's estimate fails the 100 Hz test and leaves the 50 Hz one green. +- `hard_variant_conformance` (new, 67 modes, 9 plugins) and `union_second_arm_wiring` (new; the + second arm is reached and wins on a fade, and is never credited on a clean channel). +- `gpu_cpu_equivalence` now guards the ceiling side of the cliff, not only the floor. +- `alternate_arm_decodes` reworded as wiring evidence: it read 26 where only 18 frames needed arm 1. +- `engine_cancellation_ab` relabelled: its second column is now the union, not the cancelled arm. + +**Corrections recorded in this change.** "Four months" was 71 days — two sites in the tree, plus +the #1433 body and PR #1434's description (the merged commit message gives dates, no duration). The +acquisition test's header claimed acquisition was needed "only past ~200 Hz", which is false. +`plugin-trait-versioning.md` said `2.0.0` for seven weeks after the constant became `3.0.0`. + +**Follow-ups filed:** the SNR-estimator mechanism, and the phase-1 AFC behaviour. + +**Tests → results.** Workspace gate quoted in the PR. + +--- + ## 2026-09-22 — the GPU BPSK demodulator never cancelled the crossfade ISI (#1433) **Requirement/change.** `BpskPlugin::demodulate` dispatches to the GPU when a context exists @@ -63,7 +142,7 @@ GPU; `64qam` has no crossfade canceller. BPSK was the only affected plugin. and the workspace gate runs `--no-default-features`, so **the gate cannot run them**; `gate.sh`'s `--all-features` pass is compile + lint only, and CI's `gpu` job was removed in #1380. The numbers above were run by hand on a host with a working adapter (`the_adapter_is_available_or_this_file_ -proves_nothing` passes here). That absence of an automatic gate is why this survived four months. +proves_nothing` passes here). That absence of an automatic gate is why this survived 71 days. **Tests → results.** `cargo test -p bpsk-plugin --features gpu --test gpu_cpu_equivalence` — 5 passed, 0 failed. The new test **fails before the fix** (0/16 against 11/16, with the other four diff --git a/docs/dev/reviews/review-1428-union.md b/docs/dev/reviews/review-1428-union.md new file mode 100644 index 00000000..86c59ba3 --- /dev/null +++ b/docs/dev/reviews/review-1428-union.md @@ -0,0 +1,100 @@ +--- +project: openpulsehf +doc: docs/dev/reviews/review-1428-union.md +status: resolved +last_updated: 2026-09-23 +--- + +# Adversarial review — #1428's union: the seam, the measurements, and the write-up + +Three reviews by Fable, each before the thing it covered became permanent. Two earlier reviews of +the same line of work already have artifacts: `review-1428-step1-and-step2.md` (the review that +proposed the union in place of a per-symbol gate) and `review-1433-gpu-crossfade.md` (the union +design review that surfaced #1433, a prerequisite). + +## Consumer + +- `crates/openpulse-modem/src/engine.rs` — `decode_through_arms` / `decode_variants`, called by the + six FEC-protected hard-decode chains: `receive_from_samples_with_fec_inner` (daemon, ARDOP, KISS, + OTA, CLI `--listen-ms`) and `receive_with_fec` / `_interleaved` / `_concatenated` / `_strong` / + `receive_with_short_fec_data` (CLI one-shot via `receive_with_fec_mode`, each reachable through + `FecMode::ALL`). +- `plugins/bpsk/src/lib.rs` — `BpskPlugin::demodulate_variants`, CPU and GPU branches. The daemon is + `default = ["gpu"]`, so the GPU branch is the one on air. + +## Prior art + +`stage_demodulate_payload` was the single-arm demod seam, with eleven callers each open-coding the +FEC and frame decode. `combine_and_decode_llrs` already takes a union of decode attempts on the soft +path, and CLAUDE.md records #694's "take the union" finding — the precedent this design follows. + +## Twins + +Six chains now go through the seam; five call sites remain single-arm, each with a stated reason at +`stage_demodulate_payload`. The GPU and CPU variant paths share `variants_from_parts` and +`bytes_from_symbol_stream`, so their framing cannot drift apart the way #1433's did. The uncoded path +(#1429) and `receive_with_soft_combining` (instruments-only) are deliberately not twins of this +change. + +--- + +## Prompt + +**Review A — the seam design, before implementation.** Sent the reachability table, the proposed +`demodulate_variants` trait method, `decode_through_arms` with a closure, the state-discipline rules, +and the scope. Asked hardest about the claim that computing both arms eagerly costs O(symbols), not +O(samples); whether `Vec>` was the right shape; whether a five-call-site helper, four of them +harness-only, is a real seam; the interaction with #1255 and #1123; sequencing against #1361; and +what the honest end-to-end number would be. Closing: *"anything else wrong or unproven — especially +any property of the code I have asserted that is not there."* + +**Review B — the measurements and two hypotheses.** Sent the offset sweep, the cost measurement, the +rescued-frame SNR data with its paired control, and two hypotheses: H1 (the per-symbol carrier +rotation decides which offsets decode natively) and H2 (either decision-directed estimation or onset +misalignment explains the low SNR). Asked for an explanation, what separates H2's two mechanisms, +whether the tripwire should be reworded, and whether the SNR bias should block the union. + +**Review C — the write-up, before commit.** Sent the ledger entry (which is also the PR body), the +two follow-up issues, the #1433 correction, and every code comment making a durable claim. Asked +first about provenance — every sentence stating the reviewer's measurements as the author's, or +stating something nobody measured — then headline order, the scope of "zero frames lost", and the +case for filing the AFC issue. + +## Verdict + +**Review A — two of my premises were false, and the design changed.** My reachability table called +two chains dormant on a grep; `receive_with_fec_mode` dispatches on `FecMode`, and the CLI iterates +`FecMode::ALL`, so every arm is reachable. The trait default would have returned ONE variant on the +GPU daemon. `estimate_snr_db` consumes the cancelled stream against a fitted constant, and the fade +gate's 3 dB tolerance could not see a swap. My own #1432 harness would go vacuous. The design moved +to routing every FEC-protected hard chain through one helper, with a closure that cannot reach +engine state. + +**Review B — H1 had the right mechanism and the wrong criterion; H2 was not either/or.** The +cancellation leaves a residual larger than what it removes under rotation, but which offsets decode +is set by the crossfade term's phase against the symbol's own energy, not by the rotation's cosine. +The misalignment mechanism was ruled out as an account of the −19.3 dB reading. Two premises I had +asserted without knowing: that six idle starts were six alignments (they were one), and that the +timing lock survives 50 Hz (it collapses to ~5 % of its peak). The review also found a behaviour +change the decode columns hid — a 50 Hz station decoded before the settle learns no correction in +one step — which became the AFC sweep and a follow-up issue. + +**Review C — "do not post as written".** Among its findings: +- A false correction: "72 days" (counted to the wrong end date; it is 71). +- A correction that claimed the merged commit message said "four months" (it gives no duration). +- A design doc giving the wrong mechanism for #1433 (it lived in the plugin, not the engine seam). +- A wrong reason for leaving `receive_with_soft_combining` single-arm (it is a hard chain of the + cancelled arm). +- A test name that does not exist. +- "The union beats both arms in every measured cell" (it equals the better arm in most). +- My own AFC issue attributing two wrong corrections to the settle, when neither burst settled. +- Several of the reviewer's own measurements stated in my voice. +- A hypothesis written as a finding. +- An arithmetic error: a 16-symbol window is two bytes, not one. + +"Zero frames lost" was re-derived from the raw logs and held. All findings were checked against +source or logs before being applied, and every one was applied. + +**What this pattern says.** All three reviews found errors in claims about what the code or the +data *is*, not in the code. Review C found four of those errors in text written specifically to +*correct* earlier errors. diff --git a/plugins/bpsk/src/demodulate.rs b/plugins/bpsk/src/demodulate.rs index 14322a5a..1ceb86fd 100644 --- a/plugins/bpsk/src/demodulate.rs +++ b/plugins/bpsk/src/demodulate.rs @@ -65,6 +65,33 @@ fn symbol_stream_with_expected( config: &ModulationConfig, expected: &[f32], ) -> Result<(Vec, Vec), ModemError> { + let (mut iv, mut qv, crossfade) = symbol_stream_parts_with_expected(samples, config, expected)?; + if crossfade { + // The overlapping half-Hann modulator is a crossfade, so the one-slot matched filter recovers + // `r_k = a_k + β·a_{k+1}` (β = 1/3). Left in, that `+β` term adds a constant positive bias to the + // differential dot product `r_k·r_{k-1}` (a_k²=1), eroding the flip-bit margin by several dB. + cancel_crossfade_isi(&mut iv, &mut qv); + } + Ok((iv, qv)) +} + +/// The symbol stream **before** the crossfade cancellation, plus whether this path crossfades at all. +/// +/// Split out of [`symbol_stream_with_expected`] for `demodulate_variants` (#1428), which needs both +/// the cancelled and uncancelled decisions from ONE acquisition — the timing search and +/// `demodulate_iq` are the expensive terms and are shared, so the second arm costs O(symbols). +/// +/// `estimate_snr_db` deliberately keeps consuming the CANCELLED stream through `symbol_stream`: +/// `MATCHED_FILTER_LOSS_DB` was fitted on it, and `symbol_stream_returns_the_cancelled_stream` +/// pins that bit-for-bit, because the fade gate's 3.0 dB tolerance cannot see a ≲1 dB swap. +/// +/// The `-RRC` arm reports `false`: Gardner+LMS with no crossfade, so there is no second arm there +/// and cancelling would inject the neighbour as error. +fn symbol_stream_parts_with_expected( + samples: &[f32], + config: &ModulationConfig, + expected: &[f32], +) -> Result<(Vec, Vec, bool), ModemError> { let baud = parse_baud_rate(&config.mode)?; let fs = config.sample_rate as f32; let fc = config.center_frequency; @@ -95,27 +122,65 @@ fn symbol_stream_with_expected( .into(), )); } - Ok(bpsk_demodulate_rrc( - samples, - n, - baud, - fc, - fs, - alpha, - &config.mode, - )) + let (i, q) = bpsk_demodulate_rrc(samples, n, baud, fc, fs, alpha, &config.mode); + Ok((i, q, false)) } else { let offset = find_timing_offset_with_expected(samples, n, fc, fs, expected); - let (mut iv, mut qv) = demodulate_iq(samples, n, fc, fs, offset); - // The overlapping half-Hann modulator is a crossfade, so the one-slot matched filter recovers - // `r_k = a_k + β·a_{k+1}` (β = 1/3). Left in, that `+β` term adds a constant positive bias to the - // differential dot product `r_k·r_{k-1}` (a_k²=1), eroding the flip-bit margin by several dB. - // Cancel it here (crossfade path only; the -RRC path uses Gardner+LMS and does not crossfade). - cancel_crossfade_isi(&mut iv, &mut qv); - Ok((iv, qv)) + let (iv, qv) = demodulate_iq(samples, n, fc, fs, offset); + Ok((iv, qv, true)) } } +/// Every hard-decision wire this mode can produce from ONE acquisition, best-first (#1428). +/// +/// Variant 0 is byte-identical to [`bpsk_demodulate`] — the trait contract hangs off `demodulate`, +/// so variant 0 must keep meaning it. Variant 1, where it exists, is the same symbols decoded +/// WITHOUT `cancel_crossfade_isi`. +/// +/// **Why two variants rather than a gate.** #1428 step 1 (PR #1432) measured the two arms +/// end-to-end with real RS — its "uncancelled" column is the soft arm sign-sliced — and cancelling +/// won AWGN decisively (96/96 against 12/96 at −2 dB) and lost on `moderate_f1` (38/96 against +/// 49/96 at 8 dB). The union computed from those discordant pairs, 52/96, was never below the better +/// arm in any cell and above both on the two `moderate_f1` cells. It needs no predicate, because RS +/// plus the length prefix and CRC-16 adjudicate which arm was right. +/// +/// **Cost.** The timing search and `demodulate_iq` are O(samples) and are shared; the second arm +/// adds `cancel_crossfade_isi` + `differential_decode` + `bits_to_bytes`, all O(symbols). On +/// BPSK250 that is ~4 120 symbols against ~131 840 samples. +/// +/// Returns ONE variant where there is genuinely only one arm: the `-RRC` path does not crossfade, +/// so a second entry there would be a byte-identical duplicate that costs an RS trial and could be +/// miscounted as an arm-B win. +pub fn bpsk_demodulate_variants( + samples: &[f32], + config: &ModulationConfig, +) -> Result>, ModemError> { + let expected = expected_preamble_symbols(PREAMBLE_SYMS); + let (iv, qv, crossfade) = symbol_stream_parts_with_expected(samples, config, &expected)?; + variants_from_parts(iv, qv, crossfade, expected.len()) +} + +/// Shared by the CPU and GPU arms: cancelled first, uncancelled second when the path crossfades. +/// +/// Both arms go through `bytes_from_symbol_stream`, so the framing cannot drift between them — +/// which is the structural half of #1433's lesson, where the GPU path's own copy of the slice +/// silently lacked the cancellation for 71 days. +fn variants_from_parts( + iv: Vec, + qv: Vec, + crossfade: bool, + preamble_syms: usize, +) -> Result>, ModemError> { + if !crossfade { + return Ok(vec![bytes_from_symbol_stream(&iv, &qv, preamble_syms)?]); + } + let (mut ci, mut cq) = (iv.clone(), qv.clone()); + cancel_crossfade_isi(&mut ci, &mut cq); + let cancelled = bytes_from_symbol_stream(&ci, &cq, preamble_syms)?; + let uncancelled = bytes_from_symbol_stream(&iv, &qv, preamble_syms)?; + Ok(vec![cancelled, uncancelled]) +} + /// Absolute additive SNR (dB) of a received BPSK frame — the rate controller's input. /// /// BPSK had **no** symbol-domain estimator, so the engine fell back to the waveform-blind M2M4 @@ -196,6 +261,20 @@ pub fn bpsk_demodulate_with_expected( // so the SNR is measured on exactly the symbols that get decoded. let (i_syms, q_syms) = symbol_stream_with_expected(samples, config, expected)?; + bytes_from_symbol_stream(&i_syms, &q_syms, preamble_syms) +} + +/// Slice the data span out of a symbol stream and differentially decode it to bytes. +/// +/// Extracted from [`bpsk_demodulate_with_expected`] so the cancelled and uncancelled arms of +/// `demodulate_variants` (#1428) and the GPU path all reach bytes through ONE piece of framing +/// logic. Previously the CPU and GPU paths each open-coded this slice; the GPU copy is how +/// #1433 went 71 days without the cancellation. +fn bytes_from_symbol_stream( + i_syms: &[f32], + q_syms: &[f32], + preamble_syms: usize, +) -> Result, ModemError> { if i_syms.len() <= preamble_syms + TAIL_SYMS { return Err(ModemError::Demodulation( "no data symbols after preamble".into(), @@ -205,25 +284,20 @@ pub fn bpsk_demodulate_with_expected( // Differential phase detection (handles absolute-phase ambiguity). // We take consecutive (I,Q) pairs and compute Re(z[k] * conj(z[k-1])). // Positive → same phase → NRZI "0" (no flip); negative → "1" (flip). - let data_syms_start = preamble_syms; let data_syms_end = i_syms.len() - TAIL_SYMS; - - if data_syms_start >= data_syms_end { + if preamble_syms >= data_syms_end { return Ok(vec![]); } - // Build the full range including the last preamble symbol as the reference - // for the first data bit. - let range_start = preamble_syms - 1; // include prev preamble symbol as reference + // Include the last preamble symbol as the reference for the first data bit. + let range_start = preamble_syms - 1; let iq: Vec<(f32, f32)> = i_syms[range_start..data_syms_end] .iter() .zip(q_syms[range_start..data_syms_end].iter()) .map(|(&i, &q)| (i, q)) .collect(); - let bits = differential_decode(&iq); - let bytes = bits_to_bytes(&bits); - Ok(bytes) + Ok(bits_to_bytes(&differential_decode(&iq))) } // ── AFC frequency-offset estimator ─────────────────────────────────────────── @@ -517,18 +591,22 @@ fn bpsk_demodulate_rrc_gpu( Ok(bits_to_bytes(&bits)) } -/// GPU-accelerated demodulation path. +/// The GPU path's symbol stream **before** cancellation, or `None` when it must fall back to CPU. +/// +/// The GPU counterpart of `symbol_stream_parts_with_expected`. Both GPU consumers — the single-arm +/// `bpsk_demodulate_with_gpu` and the two-arm `bpsk_demodulate_variants_with_gpu` — acquire through +/// this one function, so the arms cannot drift apart again the way the GPU slice drifted from the +/// CPU one in #1433. +/// +/// `None` means "no GPU answer" (the timing search or the IQ kernel declined) and the caller falls +/// back to the CPU path. Errors are real demodulation failures and propagate. #[cfg(feature = "gpu")] -pub fn bpsk_demodulate_with_gpu( +#[allow(clippy::type_complexity)] +fn gpu_symbol_stream_parts( samples: &[f32], config: &ModulationConfig, ctx: &openpulse_gpu::GpuContext, -) -> Result, ModemError> { - // RRC path: downmix on CPU, matched RRC filter on GPU, timing + LMS on CPU. - if matches!(config.pulse_shape, PulseShape::Rrc { .. }) || config.mode.ends_with("-RRC") { - return bpsk_demodulate_rrc_gpu(samples, config, ctx); - } - +) -> Result, Vec)>, ModemError> { let baud = parse_baud_rate(&config.mode)?; let fs = config.sample_rate as f32; let fc = config.center_frequency; @@ -539,30 +617,38 @@ pub fn bpsk_demodulate_with_gpu( } let expected = expected_preamble_symbols(PREAMBLE_SYMS); - let offset = match openpulse_gpu::timing_offset_search_gpu( - ctx, - samples, - n, - PREAMBLE_SYMS, - &expected, - fc, - fs, - ) { - Some(o) => o, - None => return bpsk_demodulate(samples, config), + let Some(offset) = + openpulse_gpu::timing_offset_search_gpu(ctx, samples, n, PREAMBLE_SYMS, &expected, fc, fs) + else { + return Ok(None); }; let effective = &samples[offset.min(samples.len())..]; - let (mut i_syms, mut q_syms) = - match openpulse_gpu::bpsk_iq_demod_gpu(ctx, effective, n, fc, fs, offset) { - Some(iq) => iq, - None => return bpsk_demodulate(samples, config), - }; + Ok(openpulse_gpu::bpsk_iq_demod_gpu( + ctx, effective, n, fc, fs, offset, + )) +} + +/// GPU-accelerated demodulation path. +#[cfg(feature = "gpu")] +pub fn bpsk_demodulate_with_gpu( + samples: &[f32], + config: &ModulationConfig, + ctx: &openpulse_gpu::GpuContext, +) -> Result, ModemError> { + // RRC path: downmix on CPU, matched RRC filter on GPU, timing + LMS on CPU. + if matches!(config.pulse_shape, PulseShape::Rrc { .. }) || config.mode.ends_with("-RRC") { + return bpsk_demodulate_rrc_gpu(samples, config, ctx); + } + + let Some((mut i_syms, mut q_syms)) = gpu_symbol_stream_parts(samples, config, ctx)? else { + return bpsk_demodulate(samples, config); + }; // #1433: the CPU arm cancels the crossfade ISI inside `symbol_stream_with_expected` // (`demodulate.rs`, the `cancel_crossfade_isi` call after `demodulate_iq`); this path landed // 2026-05-04 and #821 added the cancellation 2026-07-13 to that function only, so the GPU arm - // decoded the uncancelled `r_k = a_k + β·a_{k+1}` for four months. Measured on a 200 B frame at + // decoded the uncancelled `r_k = a_k + β·a_{k+1}` for 71 days. Measured on a 200 B frame at // 0 dB total-power SNR: uncancelled 0/16 frames against the CPU arm's 11/16. // // Applied to the WHOLE symbol stream before the preamble/tail slice below, because the @@ -571,26 +657,30 @@ pub fn bpsk_demodulate_with_gpu( // is what holds the two together. cancel_crossfade_isi(&mut i_syms, &mut q_syms); - if i_syms.len() <= PREAMBLE_SYMS + TAIL_SYMS { - return Err(ModemError::Demodulation( - "no data symbols after preamble".into(), - )); - } + bytes_from_symbol_stream(&i_syms, &q_syms, PREAMBLE_SYMS) +} - let data_syms_end = i_syms.len() - TAIL_SYMS; - if PREAMBLE_SYMS >= data_syms_end { - return Ok(vec![]); +/// GPU counterpart of [`bpsk_demodulate_variants`] — one acquisition, both decision arms. +/// +/// The daemon is `default = ["gpu"]` and registers `BpskPlugin::with_gpu` whenever an adapter is +/// present, so a variants implementation that only covered the CPU path would ship the union +/// invisible on the binary that runs on air. That is exactly #1433's shape, one method over, and +/// it is why this function exists rather than a `self.demodulate()` fallback. +#[cfg(feature = "gpu")] +pub fn bpsk_demodulate_variants_with_gpu( + samples: &[f32], + config: &ModulationConfig, + ctx: &openpulse_gpu::GpuContext, +) -> Result>, ModemError> { + // The RRC arm does not crossfade, so it has one arm and the CPU path reports that correctly. + if matches!(config.pulse_shape, PulseShape::Rrc { .. }) || config.mode.ends_with("-RRC") { + return bpsk_demodulate_variants(samples, config); + } + match gpu_symbol_stream_parts(samples, config, ctx)? { + Some((iv, qv)) => variants_from_parts(iv, qv, true, PREAMBLE_SYMS), + // A GPU fallback takes the CPU path, which reports its own arm count. + None => bpsk_demodulate_variants(samples, config), } - - let range_start = PREAMBLE_SYMS - 1; - let iq: Vec<(f32, f32)> = i_syms[range_start..data_syms_end] - .iter() - .zip(q_syms[range_start..data_syms_end].iter()) - .map(|(&i, &q)| (i, q)) - .collect(); - - let bits = differential_decode(&iq); - Ok(bits_to_bytes(&bits)) } // ── RRC baseband demodulation path ─────────────────────────────────────────── @@ -1006,6 +1096,123 @@ pub(crate) fn bits_to_bytes(bits: &[bool]) -> Vec { #[cfg(test)] mod tests { use super::*; + + fn snr_fixture() -> (Vec, ModulationConfig) { + let cfg = ModulationConfig { + mode: "BPSK250".to_string(), + sample_rate: 8000, + center_frequency: 1500.0, + pulse_shape: PulseShape::Hann, + ..Default::default() + }; + let payload: Vec = (0..120u32) + .map(|i| (i.wrapping_mul(97) >> 3) as u8) + .collect(); + let tx = crate::modulate::bpsk_modulate(&payload, &cfg).expect("modulate"); + (tx, cfg) + } + + /// `estimate_snr_db` must keep consuming the CANCELLED stream (#1428). + /// + /// `MATCHED_FILTER_LOSS_DB = 7.1` was fitted on the cancelled stream, and the #1428 split of + /// `symbol_stream_with_expected` into raw parts plus a cancelling wrapper makes it a one-line + /// edit to feed SNR the uncancelled one instead. **Nothing else would catch that**: the + /// cancellation moves the residual by ≲1 dB while `bpsk_snr_tracks_a_fade` tolerates 3.0 dB, so + /// the rate controller's input could shift under a green gate. + /// + /// Asserted bit-for-bit against an independently cancelled copy of the raw parts, and with a + /// control requiring the two streams to actually DIFFER — otherwise a build where + /// `cancel_crossfade_isi` had become a no-op would satisfy the first assertion vacuously. + #[test] + fn symbol_stream_feeds_snr_the_cancelled_stream() { + let (tx, cfg) = snr_fixture(); + let expected = expected_preamble_symbols(PREAMBLE_SYMS); + let (raw_i, raw_q, crossfade) = + symbol_stream_parts_with_expected(&tx, &cfg, &expected).expect("parts"); + assert!( + crossfade, + "BPSK250 is the crossfade path; the fixture is wrong" + ); + + let (mut want_i, mut want_q) = (raw_i.clone(), raw_q.clone()); + cancel_crossfade_isi(&mut want_i, &mut want_q); + + let (got_i, got_q) = symbol_stream(&tx, &cfg).expect("stream"); + assert_eq!( + got_i, want_i, + "symbol_stream (which estimate_snr_db consumes) is no longer the cancelled stream" + ); + assert_eq!(got_q, want_q, "same, on the quadrature arm"); + + // Control: the two streams must genuinely differ, or the assertion above is vacuous. + assert_ne!( + raw_i, want_i, + "cancel_crossfade_isi changed nothing on this fixture, so the pin above proves nothing" + ); + } + + /// Deterministic AWGN at a given total-power SNR. Box-Muller over an LCG. + fn awgn(signal: &[f32], snr_db: f32, seed: u64) -> Vec { + let p: f32 = signal.iter().map(|s| s * s).sum::() / signal.len().max(1) as f32; + let sigma = (p / 10f32.powf(snr_db / 10.0)).sqrt(); + let mut st = seed.wrapping_mul(0x9E37_79B9_7F4A_7C15).wrapping_add(1); + let mut u = || -> f32 { + st = st + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + ((st >> 11) as f32 / (1u64 << 53) as f32).clamp(1e-9, 1.0 - 1e-9) + }; + signal + .iter() + .map(|&s| { + let (a, b) = (u(), u()); + s + sigma * (-2.0 * a.ln()).sqrt() * (std::f32::consts::TAU * b).cos() + }) + .collect() + } + + /// Variant 0 is byte-identical to `demodulate` — the trait contract hangs off `demodulate`. + #[test] + fn variant_zero_is_the_shipped_demodulate() { + let (tx, cfg) = snr_fixture(); + for rx in [tx.clone(), awgn(&tx, 0.0, 7)] { + let variants = bpsk_demodulate_variants(&rx, &cfg).expect("variants"); + let shipped = bpsk_demodulate(&rx, &cfg).expect("demodulate"); + assert_eq!(variants[0], shipped, "variant 0 must BE the shipped decode"); + assert_eq!(variants.len(), 2, "BPSK250 crossfades, so it has two arms"); + } + } + + /// The second arm must be a genuinely different decode — **on a noisy input**. + /// + /// On a CLEAN fixture the two arms are byte-identical: the crossfade bias is small against a + /// noiseless signal and flips no differential decision, so the union would cost an RS trial and + /// gain nothing. The arms diverge only where the union exists to help. A version of this test + /// written on the clean fixture failed for that reason, which is the useful form of the fact: + /// **an invariant about the two arms differing is only meaningful under noise.** + #[test] + fn the_second_arm_differs_from_the_first_under_noise() { + let (tx, cfg) = snr_fixture(); + let clean = bpsk_demodulate_variants(&tx, &cfg).expect("variants"); + assert_eq!( + clean[0], clean[1], + "documenting the boundary: noiseless, the arms agree" + ); + + let mut differing = 0; + for seed in 0..8u64 { + let v = bpsk_demodulate_variants(&awgn(&tx, 0.0, seed), &cfg).expect("variants"); + if v[0] != v[1] { + differing += 1; + } + } + assert!( + differing >= 4, + "only {differing}/8 noisy seeds separated the arms; the second arm is not \ + contributing a distinct decode and the union cannot pay for itself" + ); + } + use crate::modulate::bytes_to_bits; #[test] diff --git a/plugins/bpsk/src/lib.rs b/plugins/bpsk/src/lib.rs index 551cd52c..eb1e7e91 100644 --- a/plugins/bpsk/src/lib.rs +++ b/plugins/bpsk/src/lib.rs @@ -116,6 +116,25 @@ impl ModulationPlugin for BpskPlugin { demodulate::bpsk_demodulate(samples, config) } + /// Both crossfade-cancellation arms from one acquisition (#1428). + /// + /// The GPU branch is not an optimisation here — it is the correctness case. The daemon is + /// `default = ["gpu"]` and registers `with_gpu` whenever an adapter exists, so relying on the + /// trait's default body (which calls `demodulate`, i.e. the GPU path) would return ONE variant + /// on the binary that runs on air while the CPU tests saw two. That is #1433's shape exactly, + /// one method over. + fn demodulate_variants( + &self, + samples: &[f32], + config: &ModulationConfig, + ) -> Result>, ModemError> { + #[cfg(feature = "gpu")] + if let Some(ref ctx) = self.gpu { + return demodulate::bpsk_demodulate_variants_with_gpu(samples, config, ctx); + } + demodulate::bpsk_demodulate_variants(samples, config) + } + fn demodulate_soft( &self, samples: &[f32], diff --git a/plugins/bpsk/tests/gpu_cpu_equivalence.rs b/plugins/bpsk/tests/gpu_cpu_equivalence.rs index bb8ebcc8..a196ac5c 100644 --- a/plugins/bpsk/tests/gpu_cpu_equivalence.rs +++ b/plugins/bpsk/tests/gpu_cpu_equivalence.rs @@ -226,6 +226,7 @@ fn gpu_and_cpu_agree_where_the_cancellation_decides_the_frame() { .map(|i| (i.wrapping_mul(2654435761) >> 13) as u8) .collect(); let tx = bpsk_modulate(&payload, &cfg).expect("modulate"); + let mut unsaturated_cells = 0u32; for snr_db in [0.0f32, 2.0] { let (mut cpu_bad, mut cpu_tot, mut gpu_bad, mut gpu_tot) = (0u32, 0u32, 0u32, 0u32); @@ -269,6 +270,26 @@ fn gpu_and_cpu_agree_where_the_cancellation_decides_the_frame() { crossfade-ISI bias the CPU arm cancels and the GPU arm does not (#1433)", gpu_ber / cpu_ber ); + unsaturated_cells += 1; } } + + // GUARD BOTH SIDES OF THE CLIFF, not just the floor. + // + // `cpu_ok > 0` above rejects a cell BELOW the reference arm's cliff. Nothing rejected a cell + // ABOVE it — and above is the side #1433 lived on: at 2 dB the CPU column is already + // 0.00000 BER and 16/16, so the BER comparison is skipped by `cpu_ber > 0.0` and only the + // decode-count assert bites. If both cells drift above the cliff (a faster machine, a better + // acquisition, a re-tuned fixture) every assertion here passes on a saturated reference and + // this test goes quietly decorative — which is precisely how the sweep it replaced behaved. + // + // So require at least one cell where the reference arm makes REAL ERRORS, and fail loudly + // naming the cause when none does. + assert!( + unsaturated_cells > 0, + "every swept cell had a saturated CPU reference (zero bit errors), so the BER comparison \ + never ran and this test certified agreement where agreement was guaranteed. Lower the \ + cells until the reference arm makes errors again — the cliff moves with the mode's \ + processing gain, not with an absolute SNR." + ); }