diff --git a/docs/dev/project/traceability.md b/docs/dev/project/traceability.md index 04259fb9..85d2dc4c 100644 --- a/docs/dev/project/traceability.md +++ b/docs/dev/project/traceability.md @@ -15,6 +15,62 @@ and the actually-observed results per change. --- +## 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 +(`plugins/bpsk/src/lib.rs:112`), and `bpsk_demodulate_with_gpu`'s non-RRC branch went GPU timing +search → `bpsk_iq_demod_gpu` → slice → `differential_decode` with **no `cancel_crossfade_isi`**. The +CPU arm cancels at `demodulate.rs:114`. The GPU path landed 2026-05-04 (`664122b9`); #821 added the +cancellation 2026-07-13 to `symbol_stream_with_expected` only. + +**Blast radius: the shipped daemon.** `crates/openpulse-daemon/Cargo.toml:28` is `default = ["gpu"]` +and `server.rs:142` registers `BpskPlugin::with_gpu(ctx)` whenever an adapter is present. So on a +GPU daemon the coded BPSK receive took the uncancelled arm — and `demodulate_soft` has no GPU path +and already skips cancellation by design (#832), so **both** arms were uncancelled there. + +**Design decision.** Cancel on the whole symbol stream immediately after `bpsk_iq_demod_gpu`, before +the preamble/tail slice — mirroring the CPU ordering, because the cancellation is a backward +substitution and running it on a slice changes the boundary symbol. The RRC branch is untouched: it +returns before this point and RRC does not crossfade. + +**Measured, 200 B BPSK250, 16 seeds, total-power SNR:** + +| SNR | CPU BER | GPU BER before | ratio | CPU ok | GPU ok before | GPU ok after | +|---|---|---|---|---|---|---| +| 0 dB | 0.00023 | 0.00328 | 14.0× | 11/16 | **0/16** | 11/16 | +| 2 dB | 0.00000 | 0.00047 | — | 16/16 | **9/16** | 16/16 | +| 4 dB | 0.00000 | 0.00000 | — | 16/16 | 16/16 | 16/16 | + +After the fix the two arms agree bit-for-bit (BER 0.00023 both at 0 dB). + +**Why nothing caught it, and the fixture that replaces it.** `gpu_and_cpu_agree_under_noise` swept +4–20 dB on a 27-byte payload. BPSK250 at 8 kHz is 32 samples/symbol (~15 dB processing gain), so its +lowest cell sits near 19 dB Eb/N0 — and **4 dB is measured above as the first SNR at which the +difference vanishes** (16/16 both ways). The fixture's easiest cell was exactly the boundary. The new +`gpu_and_cpu_agree_where_the_cancellation_decides_the_frame` runs 0 and 2 dB, asserts the mechanism +(BER ratio ≤ 2×) *and* the outcome (decode counts), and guards against going vacuous by requiring the +cell to be one where the CPU both decodes and errs. + +**A correction to #1080's record.** `gpu_and_cpu_agree_under_a_carrier_offset` printed "GPU decoded 6 +of 30 frames the CPU did not… the two searches still disagree off-frequency. See #1080." After this +fix it prints 0. That divergence was this defect, not the timing search. + +**Twins swept, with reasons rather than absence.** `psk8_demodulate_gpu` returns `None` for non-RRC +modes (`demodulate.rs:377`) and RRC does not crossfade; `qpsk`'s `demodulate` never dispatches to the +GPU; `64qam` has no crossfade canceller. BPSK was the only affected plugin. + +**Evidence tier — stated because it is lower than usual.** These tests are `#![cfg(feature = "gpu")]` +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. + +**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 +passing), which is the discriminating pair. Workspace gate: see the PR. + +--- + ## 2026-09-22 — the uncoded BPSK path misses #821's own bar by 1.7×; #1429 **Change.** A characterisation test and a corrected comment; **no behaviour change**, deliberately. diff --git a/docs/dev/reviews/review-1433-gpu-crossfade.md b/docs/dev/reviews/review-1433-gpu-crossfade.md new file mode 100644 index 00000000..6df59dde --- /dev/null +++ b/docs/dev/reviews/review-1433-gpu-crossfade.md @@ -0,0 +1,91 @@ +--- +project: openpulsehf +doc: docs/dev/reviews/review-1433-gpu-crossfade.md +status: resolved +last_updated: 2026-09-22 +--- + +# Review provenance — the GPU crossfade-cancellation defect (#1433) + +This defect was **not found by reading the GPU code**. It was found by an adversarial review of an +unrelated design — the #1428 union — which asked what would have to be true for that design to work +on the shipped binary. The answer was that it would not, and why. + +Recording the provenance because the finding's shape is more reusable than the fix. + +## Prompt + +Not a review of this defect — there was no prompt about the GPU path, which is the point. The +defect surfaced inside a review of the **#1428 union design**, whose prompt asked where to seam the +union and with what scope, and closed with: *"Anything else wrong or unproven in the framing above — +in particular any place I have asserted a property of the code that is not actually there."* + +That last clause is what produced it. The framing asserted that the coded receive path takes the +cancelled arm; the reviewer checked the assertion against the shipped build configuration rather +than the source alone, and found it false on any host with a GPU adapter. + +## Verdict + +Confirmed, and independently re-derived before any action was taken (table above). The union design +as proposed would have been **degenerate on the binary that runs on air**, because both of its arms +are uncancelled there — and the more consequential half is unrelated to the union: an 84-frame swing +at −2 dB AWGN, live since 2026-07-13. + +Two of the review's other findings were also confirmed and changed the union's design: the proposed +seam (`receive_from_samples_with_fec_inner`) is not on the path the #1428 A/B measured, and there is +no single hard-decode seam to place it at — `stage_demodulate_payload` has eleven callers across +nine distinct frame-decode chains. Those are recorded against #1428, not here. + +## Consumer + +- `plugins/bpsk/src/demodulate.rs` `bpsk_demodulate_with_gpu` — the site. +- `crates/openpulse-daemon/src/server.rs:142` — registers `BpskPlugin::with_gpu(ctx)` when an + adapter exists; `Cargo.toml:28` is `default = ["gpu"]`. This is what makes it a shipped defect + rather than a latent one. +- #1428's union and #1429's arm question both assume the coded path is the *cancelled* arm. On a GPU + daemon it was not, so both issues' premises were wrong there. + +## Prior art + +`gh issue list --state all --search "crossfade GPU"` returned nothing; the control search on +`crossfade` alone returned #1363, #1429, #1361, #1428, #923 — so the filter works and there was no +existing issue. #1080's GPU-divergence sweep found three defects (I-only timing search, psk8's +reversed LLR bit order, modulator drift) and not this one. + +## Twins + +Swept, with a reason for each rather than an absence: `psk8_demodulate_gpu` returns `None` for +non-RRC modes (`plugins/psk8/src/demodulate.rs:377`) and RRC does not crossfade; `qpsk`'s +`demodulate` never dispatches to the GPU; `64qam` has no crossfade canceller at all. BPSK was the +only affected plugin. + +## What the review contributed, and what I verified before acting + +The review asserted the defect and the daemon's exposure. Per the standing rule that a subagent's +structural claims are not taken at face value, every step was re-derived against the source before +anything was filed or changed: + +| claim | how it was checked | +|---|---| +| GPU branch does not cancel | read `demodulate.rs:522-585`; control — the CPU path at `:114` does | +| daemon defaults to GPU | `Cargo.toml:28`, `server.rs:126/142` | +| `demodulate` dispatches to it | `lib.rs:112-116` | +| the existing gate cannot see it | ran it: 4–20 dB, 27 B, "byte-level disagreements 0" | +| it actually costs frames | measured: 0 dB → CPU 11/16, GPU **0/16** | + +The last row is the one that mattered. The review argued from processing gain that the fixture was +too easy; the measurement showed **4 dB is the first SNR at which the difference vanishes**, i.e. +the existing fixture's easiest cell sits exactly on the boundary. That is a sharper statement than +the argument, and it is what the new test's cells are chosen from. + +## The lesson worth keeping + +An equivalence test between an accelerated path and its reference must run in a cell where **the +property under test decides the outcome**. Comparing decode success where both arms succeed +certifies nothing — and "both arms succeed" is the comfortable place a fixture drifts to, because it +is also where the test is fast and never flakes. The probe: *at what input does this test's +assertion start to depend on the thing it is named after?* If the answer is "outside the swept +range", the test is decorative. + +This is the artificially-easy-fixture archetype with an accelerator twist: the reference arm hid the +defect, because the test only ever asked whether the two agreed, never whether either was right. diff --git a/plugins/bpsk/src/demodulate.rs b/plugins/bpsk/src/demodulate.rs index 95bac1ae..14322a5a 100644 --- a/plugins/bpsk/src/demodulate.rs +++ b/plugins/bpsk/src/demodulate.rs @@ -553,11 +553,23 @@ pub fn bpsk_demodulate_with_gpu( }; let effective = &samples[offset.min(samples.len())..]; - let (i_syms, q_syms) = match openpulse_gpu::bpsk_iq_demod_gpu(ctx, effective, n, fc, fs, offset) - { - Some(iq) => iq, - None => return bpsk_demodulate(samples, config), - }; + 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), + }; + + // #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 + // 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 + // cancellation is a backward substitution — running it on a slice changes the boundary symbol. + // That is the CPU ordering, and `gpu_and_cpu_agree_where_the_cancellation_decides_the_frame` + // 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( diff --git a/plugins/bpsk/tests/gpu_cpu_equivalence.rs b/plugins/bpsk/tests/gpu_cpu_equivalence.rs index 02083ee9..bb8ebcc8 100644 --- a/plugins/bpsk/tests/gpu_cpu_equivalence.rs +++ b/plugins/bpsk/tests/gpu_cpu_equivalence.rs @@ -184,3 +184,91 @@ fn gpu_and_cpu_agree_under_a_carrier_offset() { ); } } + +/// Bit errors of `got` against `want`, over `want`'s length. `None` if the demod produced too little. +fn bit_errors(got: Option>, want: &[u8]) -> Option<(u32, u32)> { + let g = got?; + if g.len() < want.len() { + return None; + } + let bad = g[..want.len()] + .iter() + .zip(want) + .map(|(a, b)| (a ^ b).count_ones()) + .sum(); + Some((bad, (want.len() * 8) as u32)) +} + +/// The two arms must agree **where the crossfade cancellation actually decides the frame** (#1433). +/// +/// `gpu_and_cpu_agree_under_noise` sweeps 4…20 dB *total-power* SNR on a 27-byte payload. BPSK250 at +/// 8 kHz is 32 samples/symbol (~15 dB of processing gain), so its lowest cell sits near 19 dB Eb/N0 — +/// both arms decode every seed there and a BER difference cannot show up. Measured on this host, 4 dB +/// is the *first* SNR at which the difference vanishes: 16/16 both ways. One and two dB lower it is +/// the whole frame. +/// +/// Pre-fix measurement (GPU not cancelling), 200 B, 16 seeds: +/// +/// | total-power SNR | CPU BER | GPU BER | ratio | CPU ok | GPU ok | +/// |---|---|---|---|---|---| +/// | 0 dB | 0.00023 | 0.00328 | 14.0 | 11/16 | **0/16** | +/// | 2 dB | 0.00000 | 0.00047 | — | 16/16 | **9/16** | +/// | 4 dB | 0.00000 | 0.00000 | — | 16/16 | 16/16 | +/// +/// So this asserts the mechanism (the BER ratio) *and* the outcome (decode counts), and carries its +/// own guards against going vacuous: the cell is required to be one where the CPU both decodes some +/// frames and makes some errors, or the ratio would be meaningless and the counts uninformative. +#[test] +fn gpu_and_cpu_agree_where_the_cancellation_decides_the_frame() { + let Some(c) = ctx() else { return }; + let cfg = config("BPSK250", 1500.0); + let payload: Vec = (0..200u32) + .map(|i| (i.wrapping_mul(2654435761) >> 13) as u8) + .collect(); + let tx = bpsk_modulate(&payload, &cfg).expect("modulate"); + + 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); + let (mut cpu_ok, mut gpu_ok) = (0u32, 0u32); + let seeds = 16u64; + for seed in 0..seeds { + let rx = add_awgn(&tx, snr_db, seed * 7919 + 13); + let cpu = bpsk_demodulate(&rx, &cfg).ok(); + let gpu = bpsk_demodulate_with_gpu(&rx, &cfg, &c).ok(); + if let Some((b, t)) = bit_errors(cpu.clone(), &payload) { + cpu_bad += b; + cpu_tot += t; + } + if let Some((b, t)) = bit_errors(gpu.clone(), &payload) { + gpu_bad += b; + gpu_tot += t; + } + cpu_ok += u32::from(cpu.is_some_and(|b| b.starts_with(&payload))); + gpu_ok += u32::from(gpu.is_some_and(|b| b.starts_with(&payload))); + } + let cpu_ber = f64::from(cpu_bad) / f64::from(cpu_tot.max(1)); + let gpu_ber = f64::from(gpu_bad) / f64::from(gpu_tot.max(1)); + println!( + " {snr_db:4.1} dB: cpu BER {cpu_ber:.5} ok {cpu_ok}/{seeds} | gpu BER {gpu_ber:.5} ok {gpu_ok}/{seeds}" + ); + + assert!( + cpu_ok > 0, + "{snr_db} dB is below the CPU arm's own cliff (cpu_ok = 0) — the cell cannot \ + discriminate between the arms, so this assertion would be vacuous" + ); + assert!( + gpu_ok + 2 >= cpu_ok, + "{snr_db} dB: the GPU arm decoded {gpu_ok}/{seeds} against the CPU arm's \ + {cpu_ok}/{seeds}. The GPU demodulator is skipping cancel_crossfade_isi (#1433)" + ); + if cpu_ber > 0.0 { + assert!( + gpu_ber <= cpu_ber * 2.0, + "{snr_db} dB: GPU BER {gpu_ber:.5} is {:.1}x the CPU's {cpu_ber:.5}. That is the \ + crossfade-ISI bias the CPU arm cancels and the GPU arm does not (#1433)", + gpu_ber / cpu_ber + ); + } + } +}