From 446d7b899d0bffd810376cc19b3b6472b1397ad1 Mon Sep 17 00:00:00 2001 From: Simon Keimer Date: Tue, 22 Sep 2026 20:12:14 +0200 Subject: [PATCH 1/2] fix(bpsk): the GPU demodulator never cancelled the crossfade ISI (#1433) `BpskPlugin::demodulate` dispatches to the GPU when a context exists (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. THIS SHIPS. The daemon is `default = ["gpu"]` and registers `BpskPlugin::with_gpu(ctx)` whenever an adapter is present, so the coded BPSK receive took the uncancelled arm -- and `demodulate_soft` has no GPU path and already skips cancellation by design (#832), so on a GPU daemon BOTH arms were uncancelled. Measured, 200 B BPSK250, 16 seeds, total-power SNR: SNR CPU BER GPU BER before ratio CPU ok GPU before GPU after 0 dB 0.00023 0.00328 14.0x 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 Cancellation runs on the whole symbol stream before the preamble/tail slice, mirroring the CPU ordering: it is a backward substitution, so running it on a slice changes the boundary symbol. The RRC branch is untouched -- it returns earlier and does not crossfade. WHY NOTHING CAUGHT IT. `gpu_and_cpu_agree_under_noise` swept 4-20 dB on a 27-byte payload; BPSK250 at 8 kHz has ~15 dB of processing gain, and 4 dB is measured above as the FIRST SNR at which the difference vanishes. The fixture's easiest cell was exactly the boundary. The new test runs 0 and 2 dB, asserts the mechanism (BER ratio <= 2x) and the outcome (decode counts), and refuses to run in a cell where the CPU cannot decode or makes no errors, so it cannot go vacuous. Corrects #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." It now prints 0. That divergence was this defect, not the timing search. Twins swept with reasons: `psk8_demodulate_gpu` returns None for non-RRC modes and RRC does not crossfade; qpsk's `demodulate` never dispatches to the GPU; 64qam has no crossfade canceller. 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. Run by hand on a host with a working adapter: 5 passed, 0 failed; the new test fails before the fix (0/16 vs 11/16) with the other four passing. Closes #1433 Refactors: CAP-12 Verification-objective: a GPU-accelerated arm must agree with its CPU twin in a cell where the property under test decides the frame, not only where both arms succeed Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0188ATCj6DZ9aRVQ2vSirua6 --- docs/dev/project/traceability.md | 56 +++++++++++++++ plugins/bpsk/src/demodulate.rs | 22 ++++-- plugins/bpsk/tests/gpu_cpu_equivalence.rs | 88 +++++++++++++++++++++++ 3 files changed, 161 insertions(+), 5 deletions(-) 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/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 + ); + } + } +} From 88a5649abfcabc3126cadb1ffe4f22dae16935a5 Mon Sep 17 00:00:00 2001 From: Simon Keimer Date: Tue, 22 Sep 2026 20:12:53 +0200 Subject: [PATCH 2/2] docs: record the review provenance of the #1433 GPU defect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The defect was found by adversarial review of the #1428 union design, not by reading the GPU code — the review asked what would have to be true for that design to work on the shipped binary. Records the finding's provenance, the independent verification of each structural claim before acting on it, and the transferable lesson: an equivalence test between an accelerated path and its reference must run in a cell where the property under test decides the outcome. Measured, 4 dB is the first SNR at which the two arms stop differing, and the existing fixture's easiest cell sat exactly there. Refactors: CAP-12 Verification-objective: a GPU-accelerated arm must agree with its CPU twin in a cell where the property under test decides the frame, not only where both arms succeed Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0188ATCj6DZ9aRVQ2vSirua6 --- docs/dev/reviews/review-1433-gpu-crossfade.md | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 docs/dev/reviews/review-1433-gpu-crossfade.md 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.