fix(repeater): the receive window could not contain a frame (#1297) - #1313
Merged
Conversation
`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<dyn AudioInputStream>` 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0188ATCj6DZ9aRVQ2vSirua6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1297.
What was wrong
relay_one_frame_atcalledengine_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. The cross-band repeater could not receive on real audio however long it ran.This is the #1118 shape on a shipping surface, and
capture_burst's own comment already recorded the mechanism: a fresh cpal stream needs tens of ms to start delivering, so reopening per tick never warms up.Why a green suite never saw it
LoopbackBackend::readdrains the whole buffer, so the buffer IS the frame and onereceivecall gets all of it. Every existing test in the crate feeds it that way.The new gate defeats exactly that: it delivers one frame's audio one chunk per read via
push_frame. Sabotage-verified — reverting the RX toengine_rx.receive(...)fails it with "no frame relayed within 16 ticks" while the rest of the suite stays green.A shared helper, not a second copy
server.rs's rx ticker already does held-stream +accumulate_captureby hand. Open-coding it here 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).So it is
openpulse_modem::capture_ticker::CaptureTicker. Adoption by the other three is #1310, deliberately not in this PR: refactoring a working receive path inside a fix for a broken one trades risk for tidiness.Two things the implementation taught me that the design did not
The stream cannot live on the struct.
Box<dyn AudioInputStream>is notSend(a cpalStreamis not, on most hosts) and the daemon moves the repeater into a thread, so a field madeCrossBandRepeaterunspawnable. Caught by the workspace build, not by design. It is a loop local, which is whatserver.rsdoes for the same reason.Twelve existing tests needed a
relay_untilhelper, because one call is now one capture tick rather than one receive attempt — the burst flushes on the first empty read after the frame. That is how the daemon's loop experiences it, so the tests are more faithful, but it is a real contract change on a public method.Also fixed: a second defect #1300 introduced
That PR's
Err => Ok(None)arm swallowedModemError::Audiofromopen_inputalongside the demodulation errors it was written for. A repeater whose RX device could not be opened — no default input, or ALSAEBUSYbecause the daemon already holds that card — was indistinguishable from a quiet band, forever, at DEBUG. The ticker reports the first fault at WARN and retries.What this does NOT deliver
It does not make the repeater work on a station, and the ledger says so:
cfg.audio.deviceexists and is never passed), only the TX device is a genuine config gap, and fixing the first creates a capture collision with the daemon's ownrx_ticker. Blocked on a maintainer ruling.receive_from_samplesreturnsframe.payloadand the tx engine re-frames with its own sequence counter.decode_burstdecodes withFecMode::None, so FEC-coded traffic is not relayable at all — underhpx_hfthat is everything surviving a fade.The honest claim for the record is "cannot relay on cpal at HEAD by construction, and no on-air run is recorded" — not "has never relayed", which was inference presented as fact in my original filing and has been corrected on the issue.
GATE: PASS e7c04953dbf946a227f2cf87cceef3e9f0bdaab0 clean— 326 suites, 2503 passed, 0 failed.Consumer
daemon/src/lib.rs:2631EnableRepeaterspawnsrun_full_duplex, the sole production consumer; it owns theCaptureTickeras a loop local.Prior art
server.rs:886-1030is the working open-coded version of this pattern, added because reopening per tick "never warms up on real hardware" — its own words, describing the repeater's behaviour precisely.Twins
Checked rather than assumed, and the answer produced #1310:
openpulse-ardopandopenpulse-kisscallreceivein free-running loops and have the same structural defect;server.rshas the correct pattern open-coded.CaptureTicker's module doc names all three so the next reader does not re-derive it.Implements: REQ-DEV-01
🤖 Generated with Claude Code
https://claude.ai/code/session_0188ATCj6DZ9aRVQ2vSirua6