From e7c04953dbf946a227f2cf87cceef3e9f0bdaab0 Mon Sep 17 00:00:00 2001 From: Simon Keimer Date: Tue, 8 Sep 2026 17:04:10 +0200 Subject: [PATCH] fix(repeater): the receive window could not contain a frame (#1297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `relay_one_frame_at` called `engine_rx.receive(...)`, which opens an input stream, reads once and drops it. On a callback backend each attempt therefore saw a fresh buffer covering one poll interval — tens of milliseconds against a seconds-long frame — so the cross-band repeater could not receive on real audio however long it ran. `LoopbackBackend` hides this completely: its read drains the whole buffer, so the buffer IS the frame and `receive` works. That is why a green suite never saw it, and it is what the new gate had to defeat: the frame is delivered one chunk per read via `push_frame`, and reverting to `receive()` fails it. Extracted as `CaptureTicker` in `openpulse-modem` rather than open-coded here. `server.rs`'s rx ticker already does this by hand, and ARDOP and KISS both call `receive` in free-running loops with the same defect — the assumption that their TCP-driven shape makes a per-call window correct is false. Their adoption is #1310, kept out of this PR: refactoring a working receive path inside a fix for a broken one trades risk for tidiness. The stream is a loop local, not a struct field: `Box` is not `Send` and the daemon moves the repeater into a thread, so a field would make `CrossBandRepeater` unspawnable. Also closes a second defect in the arm #1300 added: its `Err => Ok(None)` swallowed `ModemError::Audio` from `open_input`, so a repeater whose RX device could not be opened was indistinguishable from a quiet band, forever, at DEBUG. This does not make the repeater work on a station. #1308 — no config field anywhere can name a second sound card — is still open, and the relay is payload-for-payload with `FecMode::None`, so FEC-coded traffic is not relayable at all. Implements: REQ-DEV-01 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0188ATCj6DZ9aRVQ2vSirua6 --- CLAUDE.md | 1 + crates/openpulse-modem/src/capture_ticker.rs | 143 ++++++++++++++++++ crates/openpulse-modem/src/lib.rs | 1 + crates/openpulse-repeater/src/lib.rs | 67 ++++++-- .../tests/half_duplex_ptt.rs | 17 ++- .../tests/repeater_integration.rs | 111 +++++++++++--- docs/dev/project/requirements.yaml | 3 + docs/dev/project/traceability.md | 55 +++++++ 8 files changed, 363 insertions(+), 35 deletions(-) create mode 100644 crates/openpulse-modem/src/capture_ticker.rs diff --git a/CLAUDE.md b/CLAUDE.md index 041c3b90..f5b7065e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -239,6 +239,7 @@ Each requirement below is done when the linked test passes. Add new links as tes | `[radio.rig_b]` cannot alias the rigctld the main rig already uses — the daemon refuses to start **when the repeater is enabled at startup** (otherwise it warns and builds no repeater). Two controllers over one transmitter key and release each other, and #1263's refusal rule reaches only *within* one `SharedPtt`. Not exotic: both `RigConfig::default()` and `RadioConfig::default()` carry `127.0.0.1:4532`, so an empty `[radio.rig_b]` header IS the collision — and the shared endpoint is rigctld itself, so `cat_backend = "rigctld"` alone collides even with a non-rigctld `ptt_backend`. Scoped to string equality: it catches the shipped defaults, not `localhost` vs `127.0.0.1` | `cargo test -p openpulse-daemon --no-default-features --lib repeater_rig_b_tests` | | A **cap-flushed** burst is not evidence about the rate ladder (#1255) — the cap exceeds the longest candidate frame, so hitting it means the carrier was still up and the slab is not one transmission. A failed decode of one must not key an ACK or move `recommended_level`. The decode itself still runs: when the squelch sits below the band floor EVERY burst is a cap flush (#1254's regime), so skipping it would make the daemon deaf on a hot band — pinned by a control that decodes a frame at the head of a capped slab. Runs ~70 s, dominated by one `ota_decode_burst` over the candidate rungs | `cargo test -p openpulse-modem --no-default-features --test cap_flush_is_not_ladder_evidence` | | A repeater that is not running is not reported as running (#1298) — enabling with nothing to run FAILS with a reason instead of emitting `RepeaterChanged { enabled: true }`, and a thread that exited is reaped so the next command sees the truth rather than "already enabled" forever. The thread OWNS the `CrossBandRepeater`, so its exit means the repeater is gone | `cargo test -p openpulse-daemon --no-default-features --lib command_apply_tests` | +| The cross-band repeater can receive a frame that arrives across SEVERAL reads (#1297) — it holds one capture stream and accumulates under DCD gating instead of opening a stream, reading once and dropping it, which on a callback backend gave it a window of one poll interval against a seconds-long frame. `LoopbackBackend` hides this entirely (its read drains the whole buffer), so the gate delivers the frame one chunk per read via `push_frame`; sabotage-verified against the old `receive()` path | `cargo test -p openpulse-repeater --no-default-features --test repeater_integration a_frame_split_across_several_reads_is_still_relayed` | | `openpulse-kiss`'s `SharedPtt` has a **watchdog thread**, so its deadline is enforced — the crate built one and called `spawn_watchdog` nowhere, leaving `force_release_if_expired` with no caller in the crate. The guard covers an early return and an unwind; it cannot reach a transmit that BLOCKS, which is the case the watchdog exists for. Driven through the real constructor, since the defect was the wiring | `cargo test -p openpulse-kiss --no-default-features --test ptt_keys_every_transmit` | | `openpulse-mesh` has no route to a sound card — it beacons and relays automatically with no PTT controller, no carrier sense and no station-ID timer, and its beacon carries no callsign field, so the capability was REMOVED rather than guarded (a fourth hand-rolled keying path on a crate with no §97.221 mapping, no control point and no on-air record). Each check is validated against a planted input | `cargo test -p openpulse-mesh --no-default-features --test no_real_audio` | | A CONACK cannot select a signing mode the CONREQ never offered (F-1147-05 — v1 checked local policy only) | `cargo test -p openpulse-core --no-default-features --test handshake_integration conack_rejected_when_mode_not_offered` | diff --git a/crates/openpulse-modem/src/capture_ticker.rs b/crates/openpulse-modem/src/capture_ticker.rs new file mode 100644 index 00000000..bb19300d --- /dev/null +++ b/crates/openpulse-modem/src/capture_ticker.rs @@ -0,0 +1,143 @@ +//! One held-open capture stream, read a tick at a time and folded into the engine's burst +//! accumulator (#1297). +//! +//! **Why this exists rather than `ModemEngine::receive`.** `receive` opens an input stream, reads +//! once, and drops the stream — so on a callback backend each call sees a fresh, nearly-empty buffer +//! covering one poll interval. A frame is seconds long; a poll is tens of milliseconds. Any caller +//! that listens *continuously* therefore cannot receive at all on real audio, however long it runs, +//! and `capture_burst`'s own comment records the reason: a fresh cpal stream needs tens of ms to +//! start delivering, so reopening per tick never warms up. +//! +//! `LoopbackBackend` hides this completely — its `read` drains the whole buffer, so the buffer IS +//! the frame and `receive` works. That is why the defect survived a green suite. +//! +//! The daemon's `rx_ticker` already does the right thing by hand. This is that pattern, owned once: +//! open lazily, read, accumulate, and on a fault drop the stream so the next tick reopens it — +//! reporting the first failure at WARN, because a station that cannot hear is off the air and +//! `receive`'s error path made that indistinguishable from a quiet band. +//! +//! **Twin copies that should adopt this.** `server.rs`'s `rx_ticker` (`:886-1030`) is the original, +//! open-coded, and still carries its own `block_in_place` wrapper, discovery tee and logging — it is +//! left alone here because refactoring a working receive path inside a PR that fixes a broken one +//! trades risk for tidiness. `openpulse-ardop` (`bridge.rs:430,437,557,564`) and `openpulse-kiss` +//! (`bridge.rs:237,256`) call `receive` in free-running loops and have the SAME structural defect +//! this fixes — the assumption that their TCP-driven shape makes a per-call window correct is false; +//! both poll continuously. Tracked as a follow-up rather than fixed blind. + +use openpulse_core::audio::AudioInputStream; +use openpulse_core::error::ModemError; + +use crate::engine::ModemEngine; +use crate::pipeline::AudioSamples; + +/// What one tick produced. +pub struct Tick { + /// A burst, when the accumulator flushed one this tick. + pub burst: Option, + /// The raw samples this tick read, before the RX front end — for callers that tee the audio + /// (the daemon's JS8 discovery dwell). Empty when the read produced nothing. + pub raw: Vec, +} + +/// A capture stream held across ticks, feeding [`ModemEngine::accumulate_capture`]. +pub struct CaptureTicker { + stream: Option>, + device: Option, + /// Whether the last read or open failed, so the recurring case logs at DEBUG and the first + /// failure and the recovery each log at WARN. + failed: bool, +} + +impl CaptureTicker { + /// Capture from `device`, or the engine's default when `None`. + pub fn new(device: Option) -> Self { + Self { + stream: None, + device, + failed: false, + } + } + + /// Whether the stream is currently faulted; the next tick will try to reopen. + pub fn is_faulted(&self) -> bool { + self.failed + } + + /// Read one tick and fold it into `engine`'s burst accumulator. + /// + /// Never returns the open/read error: a capture fault is reported and retried, not propagated, + /// because a caller that treats it as fatal stops listening for good. Decode-side errors are the + /// caller's business and reach it through the returned burst. + pub fn tick(&mut self, engine: &mut ModemEngine, mode: &str) -> Tick { + if self.stream.is_none() { + match engine.open_capture_stream(self.device.as_deref()) { + Ok(s) => { + if self.failed { + self.failed = false; + tracing::warn!("audio capture recovered"); + } + self.stream = Some(s); + } + Err(e) => { + self.note_fault(&e, "cannot open the capture device"); + return Tick { + burst: None, + raw: Vec::new(), + }; + } + } + } + + let read = match self.stream.as_mut() { + Some(s) => s.read(), + None => { + return Tick { + burst: None, + raw: Vec::new(), + } + } + }; + + match read { + Ok(samples) => { + if self.failed { + self.failed = false; + tracing::warn!("audio capture recovered"); + } + let raw = samples.clone(); + let burst = engine + .accumulate_capture(Some(mode), samples) + .unwrap_or_else(|e| { + // The front end failed on this block; not a capture fault, and not worth + // dropping the stream for. + tracing::debug!(error = %e, "capture accumulate failed for one block"); + None + }); + Tick { burst, raw } + } + Err(e) => { + // Drop the stream so the next tick reopens it. + self.stream = None; + self.note_fault( + &ModemError::Audio(e.to_string()), + "audio capture read failed", + ); + Tick { + burst: None, + raw: Vec::new(), + } + } + } + } + + fn note_fault(&mut self, e: &ModemError, what: &str) { + if self.failed { + tracing::debug!(error = %e, "{what}; still failing, will retry"); + } else { + self.failed = true; + // WARN, not DEBUG: this is the state in which a station hears nothing at all, and it + // must not look like a quiet band. + tracing::warn!(error = %e, "{what}; retrying on the next tick"); + } + } +} diff --git a/crates/openpulse-modem/src/lib.rs b/crates/openpulse-modem/src/lib.rs index f1040781..db9b8743 100644 --- a/crates/openpulse-modem/src/lib.rs +++ b/crates/openpulse-modem/src/lib.rs @@ -5,6 +5,7 @@ pub mod benchmark; pub mod capture_replay; +pub mod capture_ticker; pub mod channel_sim; pub mod diagnostics; pub mod engine; diff --git a/crates/openpulse-repeater/src/lib.rs b/crates/openpulse-repeater/src/lib.rs index 08f8ad4b..1e1dc5b0 100644 --- a/crates/openpulse-repeater/src/lib.rs +++ b/crates/openpulse-repeater/src/lib.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use std::time::Instant; use openpulse_core::station_id::StationIdTimer; +use openpulse_modem::capture_ticker::CaptureTicker; use openpulse_modem::ModemEngine; use openpulse_radio::{PttController, PttKeyGuard, SharedPtt, DEFAULT_PTT_MAX}; use thiserror::Error; @@ -85,25 +86,42 @@ impl CrossBandRepeater { /// /// Returns the number of bytes relayed, or `None` if no frame was available. /// FEC is not applied on the relay path (raw mode). - pub fn relay_one_frame(&mut self) -> Result, RepeaterError> { + pub fn relay_one_frame( + &mut self, + rx: &mut CaptureTicker, + ) -> Result, RepeaterError> { let now_ms = self.start.elapsed().as_millis() as u64; - self.relay_one_frame_at(now_ms) + self.relay_one_frame_at(rx, now_ms) } /// [`relay_one_frame`] with an explicit monotonic clock (for deterministic ID-timing tests). - pub fn relay_one_frame_at(&mut self, now_ms: u64) -> Result, RepeaterError> { + pub fn relay_one_frame_at( + &mut self, + rx: &mut CaptureTicker, + now_ms: u64, + ) -> Result, RepeaterError> { if !self.config.enabled { return Ok(None); } - let bytes = match self.engine_rx.receive(&self.config.mode.clone(), None) { + // Hold ONE capture stream across attempts and accumulate under DCD gating, the way the + // daemon's rx ticker does (#1297). A capture fault is reported and retried inside the + // ticker rather than surfacing here: treating it as fatal would stop the repeater + // listening for good, and treating it as silence — which the previous `Err => Ok(None)` + // arm did — made an unopenable RX device indistinguishable from a quiet band, at DEBUG. + let Some(burst) = rx.tick(&mut self.engine_rx, &self.config.mode).burst else { + return Ok(None); + }; + + let bytes = match self + .engine_rx + .decode_burst(&self.config.mode.clone(), &burst) + { Ok(b) => b, Err(e) => { - // A capture that does not demodulate is indistinguishable from an idle channel — it - // is the ordinary quiet case, not a fault. Propagating it ended the whole session on - // the first window with no frame in it (#1297); only PTT and transmit faults stop a - // repeater. - tracing::debug!(error = %e, "cross-band relay: no frame in this capture window"); + // A burst that does not decode is the ordinary case on a live band: noise that + // opened the squelch, or a frame this repeater's mode cannot read. Not a fault. + tracing::debug!(error = %e, "cross-band relay: burst did not decode"); return Ok(None); } }; @@ -205,12 +223,17 @@ impl CrossBandRepeater { if !self.config.enabled { return Ok(0); } + // The capture stream is owned HERE, not on the struct: `Box` is not + // `Send` (a cpal `Stream` is not, on most hosts) and the daemon moves the repeater into a + // thread, so a stream field would make `CrossBandRepeater` unspawnable. The daemon's own rx + // ticker keeps its stream as a loop local for the same reason. + let mut rx = CaptureTicker::new(None); let mut count = 0u64; let result = loop { if stop.load(Ordering::Relaxed) { break Ok(count); } - match self.relay_one_frame() { + match self.relay_one_frame(&mut rx) { Ok(Some(_)) => count += 1, // Since #1297 an idle window is `Ok(None)` rather than a session-ending error, so // this arm is now reached continuously instead of never. Without a pause the loop @@ -270,6 +293,22 @@ mod full_duplex_silence_tests { e } + /// Tick until a frame is relayed, the way the daemon's loop does. + /// + /// Since #1297 one `relay_one_frame_at` is one capture TICK, not one receive attempt: the burst + /// accumulator flushes when the carrier drops, which on `LoopbackBackend` is the first empty + /// read after the frame. So relaying takes at least two calls. + fn relay_until(rp: &mut CrossBandRepeater, now_ms: u64) -> usize { + let mut rx = CaptureTicker::new(None); + for _ in 0..16 { + match rp.relay_one_frame_at(&mut rx, now_ms).expect("relay") { + Some(n) => return n, + None => continue, + } + } + panic!("no frame relayed within 16 ticks"); + } + /// The half of "the deadline measures silence" that no integration test can reach. /// /// `acquire_key`'s re-key branch fires only after the watchdog has taken a held key, and the @@ -306,7 +345,7 @@ mod full_duplex_silence_tests { }; feed(); - rp.relay_one_frame_at(0).expect("relay").expect("Some"); + relay_until(&mut rp, 0); assert_eq!( *spy.edges.lock().expect("lock"), vec!["assert"], @@ -323,7 +362,7 @@ mod full_duplex_silence_tests { ); feed(); - rp.relay_one_frame_at(1_000).expect("relay").expect("Some"); + relay_until(&mut rp, 1_000); assert_eq!( *spy.edges.lock().expect("lock"), vec!["assert", "release", "assert"], @@ -358,9 +397,7 @@ mod full_duplex_silence_tests { src.register_plugin(Box::new(BpskPlugin::new())) .expect("register src"); src.transmit(b"fd frame", "BPSK250", None).expect("tx"); - rp.relay_one_frame_at(i * 100) - .expect("relay") - .expect("Some"); + relay_until(&mut rp, i * 100); std::thread::sleep(Duration::from_millis(100)); } diff --git a/crates/openpulse-repeater/tests/half_duplex_ptt.rs b/crates/openpulse-repeater/tests/half_duplex_ptt.rs index 02cb29ae..8568d069 100644 --- a/crates/openpulse-repeater/tests/half_duplex_ptt.rs +++ b/crates/openpulse-repeater/tests/half_duplex_ptt.rs @@ -160,9 +160,20 @@ fn a_failed_relay_transmit_does_not_leave_the_transmitter_keyed() { src.transmit(b"relay frame", "BPSK250", None).expect("tx"); let mut rp = CrossBandRepeater::new(Box::new(spy.clone()), engine_rx, engine_tx, config); - let err = rp - .relay_one_frame() - .expect_err("the tx engine has no plugin, so the relay must fail"); + // Since #1297 one call is one capture TICK: the burst flushes on the first empty read after the + // frame, so the transmit — and its failure — happen on a later tick than the one that read it. + let mut rx = openpulse_modem::capture_ticker::CaptureTicker::new(None); + let mut err = None; + for _ in 0..16 { + match rp.relay_one_frame(&mut rx) { + Ok(_) => continue, + Err(e) => { + err = Some(e); + break; + } + } + } + let err = err.expect("the tx engine has no plugin, so the relay must fail within 16 ticks"); assert!( !spy.is_asserted(), diff --git a/crates/openpulse-repeater/tests/repeater_integration.rs b/crates/openpulse-repeater/tests/repeater_integration.rs index 8dbc3f0a..e1994584 100644 --- a/crates/openpulse-repeater/tests/repeater_integration.rs +++ b/crates/openpulse-repeater/tests/repeater_integration.rs @@ -10,6 +10,19 @@ use openpulse_modem::ModemEngine; use openpulse_radio::NoOpPtt; use openpulse_repeater::{CrossBandRepeater, RepeaterConfig}; +/// Tick until a frame is relayed. Since #1297 one call is one capture TICK: the burst accumulator +/// flushes when the carrier drops, which on `LoopbackBackend` is the first empty read after the +/// frame, so relaying takes at least two calls — exactly as the daemon's loop experiences it. +fn relay_until(rp: &mut CrossBandRepeater, now_ms: u64) -> usize { + let mut rx = openpulse_modem::capture_ticker::CaptureTicker::new(None); + for _ in 0..16 { + if let Some(n) = rp.relay_one_frame_at(&mut rx, now_ms).expect("relay") { + return n; + } + } + panic!("no frame relayed within 16 ticks"); +} + fn make_engine_with_plugin() -> (ModemEngine, LoopbackBackend) { let lb = LoopbackBackend::new(); let mut engine = ModemEngine::new(Box::new(lb.clone_shared())); @@ -68,7 +81,8 @@ fn relay_disabled_returns_none() { }; let mut repeater = CrossBandRepeater::new(Box::new(NoOpPtt::new()), engine_rx, engine_tx, config); - let result = repeater.relay_one_frame().expect("no error"); + let mut rx = openpulse_modem::capture_ticker::CaptureTicker::new(None); + let result = repeater.relay_one_frame(&mut rx).expect("no error"); assert_eq!(result, None); } @@ -104,7 +118,7 @@ fn relay_loopback_cross_band() { ..Default::default() }; let mut repeater = CrossBandRepeater::new(Box::new(rig_b), engine_rx, engine_tx, config); - let n = repeater.relay_one_frame().expect("relay").expect("Some"); + let n = relay_until(&mut repeater, 0); assert_eq!(n, payload.len()); // Verify PTT was asserted then released. @@ -195,10 +209,7 @@ fn transmitting_rig_is_station_identified_when_the_interval_elapses() { // First relay at t=0: one keying pair for the relayed frame, no ID yet. feed_frame(&lb_rx); - repeater - .relay_one_frame_at(0) - .expect("relay") - .expect("Some"); + relay_until(&mut repeater, 0); assert_eq!( *log.lock().unwrap(), vec!["assert", "release"], @@ -213,10 +224,7 @@ fn transmitting_rig_is_station_identified_when_the_interval_elapses() { // Second relay at t = 601 s: the interval has elapsed, so the ID goes out under the SAME key. log.lock().unwrap().clear(); feed_frame(&lb_rx); - repeater - .relay_one_frame_at(601_000) - .expect("relay") - .expect("Some"); + relay_until(&mut repeater, 601_000); assert_eq!( *log.lock().unwrap(), vec!["assert", "release"], @@ -261,7 +269,8 @@ fn relay_empty_buffer_returns_none() { // a repeater that keyed up and relayed garbage on an empty buffer. let mut repeater = CrossBandRepeater::new(Box::new(NoOpPtt::new()), engine_rx, engine_tx, config); - match repeater.relay_one_frame() { + let mut rx = openpulse_modem::capture_ticker::CaptureTicker::new(None); + match repeater.relay_one_frame(&mut rx) { Ok(None) => {} Ok(Some(n)) => panic!("relayed {n} bytes from an empty receive buffer"), Err(_) => {} // receive() surfacing an error on an empty buffer is acceptable @@ -333,10 +342,7 @@ fn full_duplex_holds_one_key_across_frames_and_releases_it_at_session_end() { for t in [0u64, 1_000] { feed(&lb_rx); - repeater - .relay_one_frame_at(t) - .expect("relay") - .expect("Some"); + relay_until(&mut repeater, t); } assert_eq!( *ptt_log.lock().unwrap(), @@ -413,11 +419,82 @@ fn full_duplex_relay_one_frame_keys_rather_than_transmitting_into_an_unkeyed_rig }; let mut repeater = CrossBandRepeater::new(Box::new(rig_b), engine_rx, engine_tx, config); - let result = repeater.relay_one_frame().expect("relay"); - assert!(result.is_some(), "expected a frame to relay"); + let n = relay_until(&mut repeater, 0); + assert!(n > 0, "expected a frame to relay"); assert_eq!( *ptt_log.lock().unwrap(), vec!["T 1"], "a full-duplex relay must key before transmitting, and hold rather than release" ); } + +/// THE #1297 GATE: a frame delivered across SEVERAL reads is still relayed. +/// +/// This is the defect's whole shape. `engine_rx.receive(...)` opened an input stream, read once and +/// dropped it, so on a callback backend each attempt saw a fresh buffer covering one poll interval — +/// tens of milliseconds against a seconds-long frame. The repeater could not receive on real audio +/// however long it ran. +/// +/// **Why the existing tests could not see it.** `LoopbackBackend::read` drains the whole buffer, so +/// the buffer IS the frame and one `receive` call got all of it. Every other test in this file feeds +/// the frame that way. `push_frame` pops ONE queued frame per read, which is the chunked delivery a +/// real capture device does — and the property that discriminates the fix from the defect. +/// +/// It does NOT reproduce cpal in one respect worth naming: the flush here is triggered by an empty +/// read, whereas a live stream returns noise and the flush comes from the DCD energy dropping below +/// the adaptive squelch. So this proves accumulation across reads, not flush-on-DCD-drop. +#[test] +fn a_frame_split_across_several_reads_is_still_relayed() { + let (engine_rx, lb_rx) = make_engine_with_plugin(); + let (engine_tx, lb_tx) = make_engine_with_plugin(); + + let frame = { + let lb = LoopbackBackend::new(); + let mut src = ModemEngine::new(Box::new(lb.clone_shared())); + src.register_plugin(Box::new(BpskPlugin::new())) + .expect("register"); + src.transmit(b"split across reads", "BPSK250", None) + .expect("tx"); + lb.drain_samples() + }; + // One read per chunk. 12 chunks of a ~9.5 k-sample frame is ~790 samples each — the order of a + // real tick's read, and far short of a frame however many arrive. + let chunk = frame.len() / 12 + 1; + let chunks = frame.chunks(chunk).count(); + assert!(chunks >= 8, "fixture must span several reads, got {chunks}"); + for c in frame.chunks(chunk) { + lb_rx.push_frame(c); + } + + let config = RepeaterConfig { + enabled: true, + mode: "BPSK250".into(), + tx_hang_ms: 0, + full_duplex: false, + ..Default::default() + }; + let mut repeater = + CrossBandRepeater::new(Box::new(NoOpPtt::new()), engine_rx, engine_tx, config); + + let n = relay_until(&mut repeater, 0); + assert!( + n > 0, + "no frame relayed from audio delivered across {chunks} reads — the receive path cannot \ + accumulate, so on a callback backend it can never see a whole frame" + ); + + // And what reached rig_b is the payload, not noise. + let out = { + let lb = LoopbackBackend::new(); + let mut rx = ModemEngine::new(Box::new(lb.clone_shared())); + rx.register_plugin(Box::new(BpskPlugin::new())) + .expect("register"); + lb.fill_samples(&lb_tx.drain_samples()); + rx.receive("BPSK250", None).expect("decode relayed frame") + }; + assert_eq!( + out.as_slice(), + b"split across reads", + "the relayed bytes are not the frame that arrived" + ); +} diff --git a/docs/dev/project/requirements.yaml b/docs/dev/project/requirements.yaml index e78f631c..c3701148 100644 --- a/docs/dev/project/requirements.yaml +++ b/docs/dev/project/requirements.yaml @@ -621,6 +621,8 @@ capabilities: tests: - crates/openpulse-core/tests/relay_integration.rs - crates/openpulse-repeater/tests/repeater_integration.rs + - crates/openpulse-repeater/tests/half_duplex_ptt.rs + - crates/openpulse-repeater/tests/repeater_integration.rs traceability: baseline CAP-48: code: @@ -1031,6 +1033,7 @@ capabilities: code: - crates/openpulse-core/src/audio.rs - crates/openpulse-daemon/src/monitor.rs + - crates/openpulse-modem/src/capture_ticker.rs name: Device resolution and multi-mode receive satisfies: - REQ-DEV-01 diff --git a/docs/dev/project/traceability.md b/docs/dev/project/traceability.md index 2ccc474f..af8f759c 100644 --- a/docs/dev/project/traceability.md +++ b/docs/dev/project/traceability.md @@ -9,6 +9,61 @@ and the actually-observed results per change. --- +## 2026-09-08 — The repeater's receive window could not contain a frame (#1297) + +- **Requirement/change:** `relay_one_frame_at` called `engine_rx.receive(...)`, which opens an input + stream, reads once and drops it. On a callback backend each attempt therefore saw a fresh buffer + covering one poll interval — tens of ms against a seconds-long frame — so the cross-band repeater + could not receive on real audio however long it ran. The #1118 shape, on a shipping surface. + +- **Design decision (reviewed by Fable before implementing; it changed the shape and corrected two + of my claims).** + 1. **A shared helper, not a second open-coded copy.** `server.rs`'s rx ticker already does this by + hand. My proposal would have made the repeater the second copy with ARDOP and KISS to follow — + and my assumption that their TCP-driven shape made a per-call window correct is **false**: both + poll continuously (`ardop/src/bridge.rs:430`, `kiss/src/bridge.rs:237`). Extracted as + `openpulse_modem::capture_ticker::CaptureTicker`; adoption by the other three is #1310, kept + out of this PR because refactoring a working receive path inside a fix for a broken one trades + risk for tidiness. + 2. **The stream cannot live on the struct.** `Box` is not `Send` (a cpal + `Stream` is not, on most hosts) and the daemon moves the repeater into a thread — so a field + would make `CrossBandRepeater` unspawnable. Caught by the workspace build, not by design: the + ticker is a loop local, which is what `server.rs` does for the same reason. `relay_one_frame` + now takes `&mut CaptureTicker`. + 3. **My "has never relayed a frame on real audio" was inference presented as fact.** Provable: + cannot relay on cpal at HEAD by construction, and no on-air run is recorded. Not provable: that + no earlier version ever did. The issue was amended. + 4. **A second defect in the same arm, which #1300 put there.** Its `Err => Ok(None)` swallowed + `ModemError::Audio` from `open_input` alongside the demodulation errors it was written for, so + a repeater whose RX device could not be opened — no default input, or ALSA `EBUSY` because the + daemon holds that card — was indistinguishable from a quiet band, forever, at DEBUG. The ticker + reports the first fault at WARN and retries. + +- **Implementation:** `crates/openpulse-modem/src/capture_ticker.rs` (new; claimed by CAP-73) and + `crates/openpulse-repeater/src/lib.rs` — `receive` → tick + `accumulate_capture` + `decode_burst`. + +- **Tests:** `repeater_integration::a_frame_split_across_several_reads_is_still_relayed` delivers one + frame's audio **one chunk per read** via `LoopbackBackend::push_frame`. That is the whole point: + `LoopbackBackend::read` drains the entire buffer, so every other test in the crate hands `receive` + the whole frame at once — which is exactly why a green suite never saw this. Twelve existing tests + needed a `relay_until` helper, because one call is now one capture TICK rather than one receive + attempt (the burst flushes on the first empty read after the frame), which is how the daemon's loop + experiences it. + +- **Test results:** 15 passed across the crate. **Sabotage-verified**: reverting the RX to + `engine_rx.receive(...)` fails the new gate with "no frame relayed within 16 ticks" while the rest + of the suite stays green — so the fixture discriminates and the old suite genuinely could not. + Full workspace gate below. Note the first local clippy/trace pass FAILED on two things this + produced — the `Send` break above, and `capture_ticker.rs` as a `NEW-ORPHAN` claimed by no + capability — both fixed before commit. + +- **What this does NOT deliver.** The repeater still cannot be pointed at a second sound card: + **#1308** — both its engines use the OS default input and no config field anywhere could name + another. So this is provable in-process and unverifiable on a station until that lands. Two further + limits found in review and recorded on the issue: the relay is payload-for-payload rather than + wire-for-wire, and `decode_burst` decodes with `FecMode::None`, so **FEC-coded traffic is not + relayable at all** — under `hpx_hf` that is everything that survives a fade. + ## 2026-09-08 — A repeater that is not running was reported as running (#1298) - **Requirement/change:** the `EnableRepeater` thread `take()`s the `CrossBandRepeater` out of the