fix(icom): preserve finite AX.25 transmit audio over RS-BA1

[jensenpat]

Summary

Fix short AX.25/APRS transmissions over the networked Icom path so an IC-705 does not lose the beginning or end of a packet before it reaches the RF modulator.

The failure had three independent transport causes:

• AetherModem treated the requested CI-V PTT edge as if the radio were already keyed, so sample zero could be released during the IC-705's PTT transition.
• IcomSession drained every queued RS-BA1 frame in one callback, turning the modem's lead buffer into a wire burst instead of a 20 ms cadence.
• the 24-to-48 kHz r8brain resampler was never finalized, leaving about one group delay of real packet audio buffered when the transmitter unkeyed. In the captured failure this preserved leading silence while dropping the AX.25 FCS and postamble.

I'd like to recognize Nigel Fenton G0JKN for his initial work in collecting captures and building a capture harness to isolate this to the resampler. His initial capture PRs will be superceded by this PR.

Changes

Diagnostics

• extend Capture 3m to write correlated RX, generated 24 kHz stereo TX, and Icom post-resample 48 kHz mono float32 WAV files
• share one bounded, validated, atomic WAV writer between production diagnostics and tests
• keep normal transmit logging concise while retaining detailed capture-only evidence

Radio-authoritative PTT and transport pacing

• treat IcomCivBackend::setKeying() as command intent; only decoded CI-V 1C 00 readback publishes the keyed state
• make AetherModem wait up to two seconds for radio-confirmed Icom PTT before releasing audio
• clock exactly one RS-BA1 audio frame per precise 20 ms timer tick
• use the live-validated 300 ms Icom TX buffer and publish the bounded host-plus-radio drain budget for scheduled modem unkey

Finite-stream completion

• add an ordered completion barrier from AudioEngine through RadioModel to the active backend
• drain r8brain's finite 24-to-48 kHz resampler tail while PTT is still confirmed
• pad the last packetizer frame with codec-correct silence and let the normal wire clock send it
• keep operator/manual unkey immediate; the extra hold applies only to scheduled modem completion

Verification

• protobuf-safe macOS build with ONNX Runtime unavailable:
  • cmake --build build --target AetherSDR ax25_libmodem_shim_test icom_audio_test -j22
• headless socket-free tests: 2/2 passed
  • ax25_libmodem_shim_test
  • icom_audio_test
• mutation check: deliberately omitting Resampler::drain() caused the tail test to fail on the missing tail, missing group-delay coverage, and lost AX.25 frame; restoring it returned the suite to 2/2
• python3 tools/check_engine_boundary.py --strict — passed with legacy warnings only
• python3 tools/check_test_registration.py — passed

The Icom backend fixture uses a synthetic UDP peer, so it was not run in the final validation pass under the repository's socket-test escalation rule. No new fake-radio pacing test was added to the default graph.

Live radio proof

On an IC-705, the deployed build transmitted APRS beacons and text messages that a Kenwood TH-D75 decoded successfully. This is over-the-air interoperability proof for the affected 2 m AX.25 path; CI still needs to establish cross-platform build coverage.

Commits

1. feat(modem): capture generated and Icom TX audio stages. Principle VIII.
2. fix(icom): make AX.25 PTT and pacing radio-authoritative. Principle II.
3. fix(icom): flush finite transmit resampler tail. Principle XI.

Generated with OpenAI Codex (Daybreak Blue)

[aethersdr-agent]

CI on 280cba48 — the Linux build job is red; check-macos, check-windows and Static checks are all green. Not infrastructure, and not a compile problem: Build succeeded, and the single failing step is

Test Icom CI-V codec, model table and command scheduler — run 33236434582

That step is the only place in ci.yml that runs the Icom ctest gate, which is why macOS/Windows stay green — those jobs build but don't execute it.

What I believe broke

The gate is pinned to seven targets (ci.yml:384), and one of them, icom_power_derivation_test, asserts the client-commanded unkey clears derived forward power:

// tests/icom_power_derivation_test.cpp:186-194
backend.setKeying(false);
...
check(clientUnkeyReset,
      "client-requested Icom unkey immediately clears derived forward power");

Your diff removes the only code path that satisfies it. Previously setKeying() carried:

if (m_keyed != key) {
    m_keyed = key;
    if (!m_keyed) clearDerivedForwardPower();   // <- this
    ...
}

and at IcomCivBackend.cpp:4620 that block is now replaced by the "DO NOT publish intent as radio state" comment. On 280cba48 clearDerivedForwardPower() has exactly one remaining caller — the decoded 1C 00 branch at line ~2843 — so a client setKeying(false) emits no TX:FWDPWR 0.0, and that check fails. That's the one assertion in the seven-target gate that your change contradicts; I couldn't pull the per-assertion stdout through the API, so treat this as the strongly-indicated cause rather than a quoted failure line.

Two ways to fix it

1. Keep the immediate clear on the unkey edge (my preference). Suppressing the optimistic keyed edge is the whole point of the PR, but zeroing a stale derived wattage on unkey isn't an on-air claim — it's clearing a number that is definitionally invalid once we've asked to stop. Re-adding if (!key) clearDerivedForwardPower(); to setKeying() restores the assertion without reintroducing the premature transmitChanged/m_keyed publish that truncated your AX.25 preamble.
2. Update the test to expect the clear only on the confirmed 1C 00 00 reply — but then a refused or delayed unkey leaves a live-looking forward-power reading on the meter, which is the failure direction that assertion was written to prevent. If you go this way, the PR body should say so explicitly.

One other thing worth knowing before you push

Your changes to tests/icom_backend_test.cpp (the txAudioDrainBudgetMs == 550 pin and the PTT-confirmation rework) never build or run — icom_backend_test sits inside the #[==[ ... ]==] retired-fixture block in tests/tests.cmake and isn't registered. The same goes for the new testTxTailPadding() in icom_audio_test.cpp: icom_audio_test is registered, but it isn't on the PR gate, so it only runs in the weekly sanitizers job. If you want the 550 ms budget and the tail-padding behaviour actually gated on merge, the cheapest route is adding icom_audio_test to ICOM_GATE and bumping the count pin from 7 to 8 — note that pin, it fails the step the moment the enumerated set changes.

The underlying diagnosis here is excellent — the r8brain group-delay tail eating the FCS and postamble, with the drain() boundary put on Resampler rather than open-coded in the backend, is exactly the right shape. It's just this one seam that the existing test still holds a claim on. Thanks for the careful write-up in aetherd-icom-civ-backend-design.md too; it made tracing this quick.

---

_{🤖 aethersdr-agent · cost: $6.3397 · model: claude-opus-5}

[aethersdr-agent]

Reviewed against the PR head at a0595fa2, the be5b5a54 merge base, and the #5011 thread. Everything below is reasoned from source — I have no build and no radio, so no claim here was reproduced at runtime.

The Resampler::drain() idea is genuinely good and the seam is in the right place: the finite-stream boundary belongs on the converter, not open-coded in the backend, and testIcomResamplerTailDrainPreservesVhfPacket is real, mutation-resistant coverage for it. My problems are with the other two commits riding along with it.

1. Issue fit

Partially — and possibly overclaimed. I could not read the PR body (gh auth failed here), so I cannot check what it says it fixes; please treat this section as a question rather than a finding if the body already says what I'm asking for.

The candidate linked issue is #5011. Two things in that thread bear directly on this PR:

• 08-24 ledger row, #5011: "Live re-test with @jensenpat's PTT/CI-V timing fix in place — decode still fails, no improvement → the timing fix is EXCLUDED as the cause of this fault instance." Commit abaee58 is that timing fix. It has already been bench-tested and does not resolve #5011's symptom.
• @nigelfenton, 2026-08-27, #5011: "when the timing fix merges, please close it against the timing defect and not against this issue. #5011 stays open until a burst from AE actually decodes."

So: if this PR carries Fixes #5011, that should come out. The resampler-tail finding is a new mechanism not in that ledger and stands on its own; the PTT-confirmation commit is the already-excluded one.

There is also a direct conflict with the issue's measurements that the PR should reconcile. #5011's 08-20 and 08-24 rows record the post-resample tap decoding 2/2 with valid FCS. This PR's design-doc paragraph says the missing tail was "enough to remove AX.25 FCS plus postamble." Both cannot be describing the same stream. Either the earlier tap was capturing before the truncation point, or the tail loss is not what removes the FCS. Worth a sentence in the body.

2. Scope

File / group	What it changes	Claimed by the title?	Verdict
Resampler.{h,cpp} drain()	Finite-stream tail flush	Yes	In scope
IcomAudio.{h,cpp} padToFrame()	Codec-correct silence to complete the last 20 ms frame	Yes	In scope
IcomCivBackend::finishTxAudio, IRadioBackend::finishTxAudio, AudioEngine::finishModemTxAudio, RadioModel::finishTxAudio, MainWindow_Wiring	Ordered completion barrier	Yes	In scope
docs/MODEM.md, aetherd-icom-civ-backend-design.md	Documents the above	Yes	In scope
IcomCivBackend::setKeying + RadioModel::publishCommandedBackendTransmitEdge	Icom PTT edge is no longer published on command — for every TX path (voice MOX, TUNE, CW, TCI, DAX), not just AX.25	No	Needs maintainer decision — see Blocker 2
IcomSession kTxPumpMs 10→20 + one-frame-per-tick	Changes the RS-BA1 wire cadence for all Icom TX audio	No	Needs maintainer decision — see Blocker 3
IcomSession::Params::txBufferMs 200 → 300	Negotiated radio TX buffer default, all Icom sessions	No	Nit — correct-looking, but disclose it
Ax25AudioCapture.{h,cpp}, icom extension verbs debug.ax25.capture.begin/.end, caps.extensions["icom"]["txAudioDrainBudgetMs"], 64 MiB capture buffer	New diagnostic subsystem + new public extension/capability surface	No	Needs maintainer decision (protocol/capability addition arriving inside a bug fix)
tests/icom_power_derivation_test.cpp	Assertion inverted	No	Blocker 1
tests/icom_backend_test.cpp	56 lines of test rework that never compiles	No	Blocker 4

I checked the additions are not dead: Ax25AudioCapture.cpp is registered in CORE_SOURCES (CMakeLists.txt:802) and in the shim test target.

Blockers

1. A fail-closed guard was removed and its test assertion was inverted to match. tests/icom_power_derivation_test.cpp:193 — check(clientUnkeyReset, "client-requested Icom unkey immediately clears derived forward power") became check(!sawClientUnkeyReset, "...retains power until radio confirmation"). On current main the only clearDerivedForwardPower() call reachable from a client unkey was inside the setKeying() block this PR deletes (IcomCivBackend.cpp:4620); the remaining caller is the decoded 1C 00 branch. Net effect on an IC-9700: the operator unkeys, and the derived TX:FWDPWR reading stays on the meter until the radio's confirmation arrives — indefinitely if that reply is lost or refused. Zeroing a derived wattage is not an on-air state claim, so it does not need to wait for radio truth; keeping if (!key) clearDerivedForwardPower(); in setKeying() restores the assertion without reintroducing the optimistic m_keyed/transmitChanged publish. (The bot comment on this PR reached the same conclusion and preferred the same option; the diff took the other one.) Inline comment on the test line.

2. Making the Icom PTT edge radio-only is an architecture change to every Icom transmit path, and it is not the AX.25 fix. IcomCivBackend.cpp:4620 and RadioModel::publishCommandedBackendTransmitEdge together mean radioTransmittingChanged for Icom now fires only from the decoded 1C 00 reply, which arrives on the 250 ms fallback poll. Consumers include TCI's key confirmation, the TX-filter-loss latch, D-STAR runtime reconfiguration, the AX.25 dialog, and m_meters.setTransmitting(). I agree the direction is right under Constitution II/III — but per GOVERNANCE.md this is the shape that wants an RFC or at least its own PR and its own bench evidence, especially since #5011's 08-24 re-test says it does not fix the symptom this PR is named after. I'd split abaee58 out.

3. IcomSession::onTxPump now consumes strictly one 20 ms frame per timer tick, with no way to catch up. IcomSession.cpp:737. The deleted comment named the symptom: "Drain every frame that is ready, not just one … pacing them out one per 10 ms tick would fall permanently behind." Qt repeating timers reschedule from the moment they fire and never fire early, so the mean pump period is strictly ≥ 20 ms while production is exactly 20 ms of audio per 20 ms of soundcard clock. Every millisecond of tick lateness is backlog that is never recovered; at 250 ms (kMaxPendingBytes = 24000) TxPacketizer::submit() starts popping the oldest bytes with no logging at the drop site. 1 ms of average lateness reaches that cap in ~5 s of transmission. This is a change to voice TX on every Icom, justified in the diff by "queue depth never turns into a wire burst" without measurement that bursting was harmful — and it is precisely the transport experiment #5011's triage listed as proposed, pending the TUNE-carrier go/no-go that has not been run. If it stays, it needs a bounded catch-up (drain down to N frames rather than exactly one) so lateness cannot accumulate monotonically.

4. Every test written for the PTT change is dead code. tests/icom_backend_test.cpp sits inside the #[==[ … ]==] retired-fixture block at tests/tests.cmake:483-512 — it is not configured, compiled, or registered. ci.yml:380 says so explicitly ("exactly how the retired icom_backend_test disappeared from this gate"). So the txAudioDrainBudgetMs == 550 pin, the reworked PTT-confirmation block, and the "no optimistic true edge escapes" assertion never run anywhere. testTxTailPadding() in icom_audio_test.cpp does build, but icom_audio_test is not in ICOM_GATE (ci.yml:384), so it only runs in the weekly sanitizer job. Adding icom_audio_test to ICOM_GATE (and bumping the count pin from 7 to 8) is cheap; the backend-test coverage needs a live home or it should not be presented as coverage.

5. The AX.25 PTT gate waits for an edge, never checks current state. Ax25HfPacketDecodeDialog.cpp:2341 connects radioTransmittingChanged and does nothing else. RadioModel::publishBackendTransmitEdge early-returns when m_radioTransmitting == tx, so if the radio is already keyed when a packet starts, no edge ever arrives and the transmission aborts after 2 s. Reachable when an operator holds MOX/footswitch while an APRS beacon or KISS frame fires, and on back-to-back queued packets where the previous unkey has not yet been reported (kTxDaxSettleMs is 150 ms; the PTT poll runs at 250 ms). RadioModel::isRadioTransmitting() already exists and maybeStartNextKissTx at :3153 already uses it — a if (m_radio->isRadioTransmitting()) { startTransmitAudioAfterPtt(); return; } after the connect closes it.

Nits (non-blocking)

• Ax25HfPacketDecodeDialog.cpp:2542 — unkeyDelayMs = m_txTailMs + drainBudgetMs is 200 + 550 = 750 ms of extra keyed carrier after every packet. txAudioDrainBudgetMs is the packetizer's worst-case 250 ms plus the radio's 300 ms buffer, but finishTxAudio() has just called padToFrame(), so the real queue is usually a single 20 ms frame. On a shared 1200-baud APRS channel that is ~half a second of avoidable dead air per transmission. Deriving the host half from TxPacketizer::pendingBytes() at drain time would make the wait match reality.
• IcomCivBackend::finishTxAudio() early-returns on !m_txResampler, so when the producer's rate already equals m_audioRateHz the packetizer is never padded and the last partial frame is discarded by flushTxAudio() on unkey — the exact loss the padding exists to prevent. Unreachable today (AudioEngine::DEFAULT_SAMPLE_RATE is 24 kHz) but it is a latent asymmetry; the drain and the pad want separate guards.
• The drain test does not prove the causal claim. testIcomResamplerTailDrainPreservesVhfPacket shows the drained stream decodes and that the undrained stream is shorter, but never decodes the undrained buffer. One extra processMonoFloat over the pre-drain samples asserting frames.isEmpty() (or !fcsOk) would pin "the tail loss removes the FCS", which is the claim the design doc makes.
• finishAx25PostResampleCapture() and appendAx25PostResampleCapture() are declared inside private slots: in IcomCivBackend.h; they are ordinary members. Also, finishAx25PostResampleCapture() runs a blocking QSaveFile write of up to 64 MiB from the destructor and from disconnectRadio().
• IcomSession::Params::txBufferMs 200 → 300 is a user-affecting default change for all Icom sessions. It is well justified in the comment (kappanhang parity, #4799) — it just is not mentioned in the title.

Other tools' findings

• CG-PATH-001 × 5 in src/gui/MainWindow.cpp (8807, 8919, 9038-9040) — refuted. This PR touches exactly one line of MainWindow.cpp (:1696, wireModemAudioCompletion()). Line 8807 is an NR2 FFT-wisdom QLabel. Untouched code, not this PR's.
• CG-PATH-001 in tests/ax25_libmodem_shim_test.cpp:870 — refuted, and it is pointing at the opposite of a defect. That line is report("capture path rejects unsafe identifier", ax25AudioCapturePath(..., QStringLiteral("../escape"), 1).isEmpty()). validCaptureId() in Ax25AudioCapture.cpp anchors on ^[0-9]{8}-[0-9]{6}Z$, the filename is fully templated, and the directory is SettingsPaths::configDir() — no caller-supplied path component survives. Good hardening; credit where due.

What I tried to break and could not

• The RFC #4983 stale-poll guard. I expected removing the optimistic edge to reopen the captured FT8 teardown. It does not: m_pendingPttIntent / m_pendingPttUntilMs and the one-directional guarding test at IcomCivBackend.cpp:2780 are intact, and the unkey direction still publishes radio truth immediately. The rewritten test's "a refused/delayed unkey cannot make the UI claim the radio is RX" is a fair restatement of clause (b) — even though it never runs (Blocker 4).
• The ordering claim behind finishModemTxAudio. Verified that txFinalMonitorPcmReady (AudioEngine.cpp:9146, host-modulation branch) and modemTxAudioFinished are both emitted from the AudioEngine thread (MainWindow.cpp:1232-1239) and both cross to the GUI thread queued, so FIFO ordering does hold and the barrier is real. Not a paper guarantee.
• The 550 arithmetic. kMaxPendingBytes = 24000, kRadioAudioRateHz = 48000, sizeof(qint16) = 2 → 250 ms, + kDefaultTxBufferMs = 300 = 550. The constexpr matches the pin.
• Generation/reentrancy in the dialog. Every deferred lambda (kTxDaxSettleMs, kIcomPttConfirmTimeoutMs, kTxLeadMs, the unkey timer) captures and re-checks m_txGeneration; finishTransmit disconnects m_txPttConfirmConnection and clears both new flags; disconnecting a connection from inside its own slot is legal. The abort-mid-transmission path holds. What I could not break here I did find at Blocker 5 — the already-keyed case, which is a missing precondition rather than a lifetime bug.
• TxPacketizer::padToFrame() overflow eviction. Walked empty queue, sub-frame remainder, exact-multiple, and near-cap inputs; the loop cannot underflow and 0x00 / 0x80 / 0xFF are the right silences for s16 / u8 PCM / µ-law.
• Whether the new files are dead. They are not — Ax25AudioCapture.cpp is in CORE_SOURCES and in the shim test target, and finishTxAudio() has a real caller chain from the dialog through AudioEngine and RadioModel.
• CI. All four checks are green on a0595fa2 — but that is not evidence for this PR's core change, because the test that covers it is not built (Blocker 4) and the one gated test that contradicted it was edited to agree (Blocker 1).

Recommendation

Request changes, with a maintainer decision needed on the split.

Blockers 1 and 4 are the ones I'd want closed before anything merges: a fail-closed guard came out and the automated evidence against it was inverted rather than the code fixed, while the tests written to defend the new behaviour never execute. Blocker 5 is a straightforward missing precondition. Concretely, I'd suggest landing a0595fa (the resampler drain + padding + completion barrier) as its own PR — it is well-shaped, it is the part with live coverage, and it does not touch keying semantics — and taking abaee58 (radio-authoritative PTT) and the IcomSession cadence change to a separate PR with an RFC, since #5011's 08-24 bench data already excludes the timing fix as the cause of the symptom and the cadence change is still a proposed experiment there. e9d2064 (the capture stages) is useful diagnostics but is new public extension surface; that is a maintainer call either way.

Thanks for the careful design-doc write-up — tracing the drain reasoning through aetherd-icom-civ-backend-design.md made this review much faster than it would otherwise have been.

---

_{🤖 aethersdr-agent · cost: $12.4192 · model: claude-opus-5}

[aethersdr-agent]

Blocker. This assertion was inverted rather than the code fixed, and the behaviour it used to protect is a fail-closed one.

On main the only clearDerivedForwardPower() reachable from a client unkey lived in the setKeying() block this PR deletes (IcomCivBackend.cpp:4620); the sole remaining caller is the decoded 1C 00 branch. So after this change the operator unkeys an IC-9700 and the derived TX:FWDPWR reading stays on the meter until the radio's confirmation arrives — indefinitely if that reply is lost, refused, or overridden at the front panel. That is the direction Constitution VI asks to fail closed, and the original assertion's wording named it.

Zeroing a derived wattage is not an on-air state claim, so it does not have to wait for radio truth. Restoring if (!key) clearDerivedForwardPower(); in setKeying() satisfies this check without reintroducing the optimistic m_keyed/transmitChanged publish that the PR is removing on purpose.

[aethersdr-agent]

Blocker — this file never builds. icom_backend_test sits inside the #[==[ … ]==] retired-fixture block at tests/tests.cmake:483-512: not configured, not compiled, not registered. ci.yml:380 says so in as many words ("exactly how the retired icom_backend_test disappeared from this gate").

So this 550 pin, the reworked PTT-confirmation block below, and the "no optimistic true edge escapes" assertion are all dead text. The PR's central behavioural change currently has zero executing coverage, and the one gated test that did contradict it (icom_power_derivation_test) was edited to agree.

testTxTailPadding() in icom_audio_test.cpp does compile, but icom_audio_test is not in ICOM_GATE (ci.yml:384) — it only runs in the weekly sanitizer job. Adding it to ICOM_GATE and bumping the count pin from 7 to 8 is the cheap half of this.

[aethersdr-agent]

Two things this deletion takes with it.

1. The derived-power clear. clearDerivedForwardPower() was called here on the unkey edge and now is not — see the inverted assertion in tests/icom_power_derivation_test.cpp:193. Suggest re-adding just that line; it is not an on-air claim and does not conflict with the intent-vs-state separation you are drawing.

2. Scope. Combined with RadioModel::publishCommandedBackendTransmitEdge, this makes radioTransmittingChanged fire for Icom only from the decoded 1C 00 reply, i.e. on the 250 ms fallback poll — for every transmit path, not just AX.25: TCI's key confirmation, m_meters.setTransmitting(), the TX-filter-loss latch, D-STAR runtime reconfiguration, the MOX/TUNE indicators.

I think the direction is right under Constitution II/III. But #5011's 08-24 ledger row records this exact fix bench-tested live with "decode still fails, no improvement → the timing fix is EXCLUDED as the cause of this fault instance", and @nigelfenton asked on 2026-08-27 that it be closed against the timing defect rather than #5011. Given that, and GOVERNANCE.md on architectural changes, I'd split abaee58 into its own PR with an RFC rather than land it behind the resampler fix.

[aethersdr-agent]

Blocker (risk) — this can only fall behind; it can never catch up.

The comment you replaced named the symptom directly: "Drain every frame that is ready, not just one … pacing them out one per 10 ms tick would fall permanently behind."

Qt repeating timers reschedule from the moment they fire and never fire early, so the mean pump period is strictly ≥ 20 ms, while production is exactly 20 ms of audio per 20 ms of soundcard clock. Every millisecond of tick lateness becomes backlog that nothing ever recovers. At kMaxPendingBytes = 24000 (250 ms) TxPacketizer::submit() starts popping the oldest bytes, with no logging at the drop site — 1 ms of average lateness reaches that cap in about 5 s of continuous transmission. Qt::PreciseTimer bounds jitter, not the sign of the error.

This affects voice TX on every Icom, not just AX.25, and the justification in the comment ("queue depth never turns into a wire burst") is asserted rather than measured — it is the transport experiment #5011's triage listed as proposed, pending a TUNE-carrier go/no-go that the ledger does not show as run.

If it stays, please make the catch-up bounded rather than absent — drain down to a small target depth (say ≤ 2 frames) instead of exactly one per tick — so lateness cannot accumulate monotonically.

[aethersdr-agent]

Blocker — this waits for an edge that may never come.

RadioModel::publishBackendTransmitEdge early-returns when m_radioTransmitting == tx, so if the radio is already keyed when this transmission starts, radioTransmittingChanged(true) never fires and the packet aborts after kIcomPttConfirmTimeoutMs.

Reachable at least two ways: an operator holding MOX or a footswitch while an APRS beacon / KISS frame fires, and back-to-back queued packets where the previous unkey has not yet been reported (kTxDaxSettleMs is 150 ms, the PTT fallback poll runs at 250 ms).

RadioModel::isRadioTransmitting() already exists, and maybeStartNextKissTx() at :3153 already uses it. Checking current state right after the connect — if (m_radio->isRadioTransmitting()) { startTransmitAudioAfterPtt(); return; } — closes it without losing the confirmation semantics.

[aethersdr-agent]

Nit, but it costs real air time. This is 200 + 550 = 750 ms of keyed carrier after the last sample of every packet.

txAudioDrainBudgetMs is the packetizer's worst-case 250 ms plus the radio's 300 ms buffer — but finishTxAudio() has just called padToFrame(), so in the normal case the host queue holds a single 20 ms frame, not 250 ms of backlog. On a shared 1200-baud APRS channel this is roughly half a second of avoidable dead air per transmission that a peer cannot talk over.

Deriving the host half from TxPacketizer::pendingBytes() at drain time (and keeping the radio's 300 ms as the fixed part) would make the wait match what is actually queued. The capability field is still the right place to publish the radio-side number.

[aethersdr-agent]

Cisco CodeGuard — static analysis of this PR (6 finding(s))

• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5311/src/gui/MainWindow.cpp:8807
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5311/src/gui/MainWindow.cpp:8919
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5311/src/gui/MainWindow.cpp:9038
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5311/src/gui/MainWindow.cpp:9039
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5311/src/gui/MainWindow.cpp:9040
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/ax25_libmodem_shim_test.cpp /tmp/aetherclaude/pr-5311/tests/ax25_libmodem_shim_test.cpp:870

_{Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them.}

---

_{🤖 aethersdr-agent · cost: $13.7734 · model: claude-opus-5}

[ten9876]

Issue fit

#5011's symptom — short AX.25 packets losing head or tail over RS-BA1 — is diagnosed into three independent transport causes (premature sample-zero release on the command edge, burst draining of the lead buffer, the never-finalized resampler holding a group delay of tail), each fixed at the right layer, with a mutation check on the resampler drain and over-the-air interop proof (TH-D75 decode). The credit to G0JKN's capture work and the explicit adherence to the socket-test escalation rule are both to the PR's credit. The architecture (command-intent vs radio-confirmed PTT per Principle II, an ordered completion barrier, codec-correct padding) is right.

Four blockers. Each was verified in the primary sources rather than adopted from analysis passes — and three of the four are regressions to the paths this PR did not set out to change: ordinary Icom voice, TUNE, and TCI digital keying. The AX.25 fix itself is sound; the collateral is not.

Preflight disclosure (recorded per step 0)

This PR modifies tests/icom_backend_test.cpp (+26/−30) — the retired fake-radio UDP-socket test, still bracket-commented out in tests.cmake at the PR head. It was not built or run in this review, and per canon those edits are not counted as coverage (they compile in no default build and run in no CI job). The PR body itself is honest about this and runs the socket-free pair instead. The other three touched tests own no sockets (verified at head).

Scope

All 29 files map to the three stated causes plus diagnostics: capture plumbing (Ax25AudioCapture, dialog, backend taps), PTT/pacing (IcomCivBackend/IcomSession/RadioModel), completion barrier (AudioEngine→IRadioBackend finishTxAudio() — a new but minimal public backend surface with a no-op default), resampler drain, and the two socket-free tests. tests.cmake gains only build wiring. Nothing unexplained.

Blockers

1. Removing the optimistic keyed publication regresses every non-AX.25 Icom TX path (inline at IcomCivBackend.cpp:4620). On main, setKeying set m_keyed, updated meters, and emitted transmitChanged, with a comment explaining exactly why that was load-bearing; the PR deletes all three and moves m_keyed to the decoded 1C 00 readback only — but every consumer of m_keyed outside the AX.25 dialog was left assuming the old timing. Four verified consequences:

• Head-clip on every voice/DAX/digital over. submitTxAudio hard-drops on !m_keyed, and only the AX.25 dialog was taught to wait — MOX speech and WSJT-X/TCI/DAX streams start delivering immediately, so the first ~60–250 ms (one CI-V round trip, up to the fallback poll) of every transmission is silently discarded. If the confirming readback is ever lost, the radio sits keyed with a permanently silent transmitter and no log at the drop site.
• TUNE goes silent inside the keyed window. setTune(true) primes one 20 ms tone frame; onTuneAudioTick then gates on m_keyed — the file's own comment states the requirement this breaks: "Raise the tone BEFORE keying, so no part of the keyed window is silent — a tuner sampling that edge can otherwise read infinite SWR."
• A refused unkey is no longer republished. On main, unkey optimistically cleared m_keyed, so a radio still reporting keyed was a change and got published — the deleted test row said it in words: "a radio reporting KEYED after an unkey request is published immediately, NOT suppressed." Now m_keyed stays true through the unkey command, the contradictory readback compares equal, nothing is emitted — and since RadioModel::setTransmit(false) already dropped the UI to RX optimistically, the operator sees RX while the radio transmits, with no path that ever corrects it.
• TCI key-confirm aborts on back-to-back FT8 periods. publishCommandedBackendTransmitEdge suppresses the command edge for the icom family, so TCI's 1250 ms confirmation now needs a change edge from the poll; keying the next period while the previous unkey readback is in flight yields no edge, and abortTciPtt sends trx:false mid-transmission.

The Principle II direction is right for the model; the regression is applying it to the audio/tune gates and edge publications without touching their consumers. A m_pendingPttIntent || m_keyed gate for audio/tune, plus re-publishing a readback that contradicts commanded intent (what the deleted test pinned), keeps the radio-authoritative model and un-breaks the other paths.

2. The TX pump loses its catch-up path — and it clocks all Icom TX audio, including voice (inline at IcomSession.cpp). The change from "drain every ready frame at 10 ms" to "exactly one frame per 20 ms tick" deletes this comment:

"Drain every frame that is ready, not just one: a host audio callback can deliver several frames' worth in one block, and pacing them out one per 10 ms tick would fall permanently behind."

Nothing replaces what that comment guarded. Producer and consumer now run at equal rate with zero recovery capacity: every late or coalesced tick adds backlog that can never be worked off (the queue can only grow), until TxPacketizer's 250 ms cap starts discarding the oldest bytes mid-stream. I verified the blast radius: takesTxAudioOverSeam=true routes microphone/voice TX through this same packetizer and pump, so a long Icom voice over under GUI load accumulates latency and then drops audio mid-transmission — a regression the short APRS bench bursts could not expose. Qt::PreciseTimer reduces jitter; it cannot eliminate coalescing, and the asymmetry (can fall behind, can never catch up) is structural. The wire-cadence goal and recovery are compatible: drain min(ceil(elapsedMs/20), pending) frames per tick, or drain while backlog exceeds a two-frame cushion — the RS-BA1 stream still never sees a burst beyond the recovery bound, and steady state is identical to this PR.

3. An already-keyed radio aborts the transmission (inline at the dialog). The Icom path arms a change-gated radioTransmittingChanged confirm and has no already-transmitting pre-check (the !txModel.isTransmitting() check lives only in the non-Icom branch). If the keyed readback is already true when PTT is requested — the unkey status of a just-finished tune cycle or previous packet not yet landed — no change edge ever fires, the confirm lambda never runs, and the 2 s timeout calls finishTransmit(true, "Icom PTT was not confirmed…") on a radio that is on the air. The repo already has the exact tool: RadioModel::radioTransmitConfirmed, whose doc comment says it carries "unchanged answers hidden by the change-gated radioTransmittingChanged signal", with TciServer as the consuming precedent for this very Icom question.

4. The completion path can truncate or corrupt the very frame it completes (inline at finishTxAudio). if (!m_txResampler) return; skips padTxAudioToFrame() entirely, so a finite stream whose source is already at the negotiated 48 kHz loses its final partial frame at unkey flush — this PR's own bug class, surviving in the no-resampler variant. And padToFrame's overflow eviction pops the oldest pending frames to make room for trailing silence, so with the queue near its cap (which blocker 2 makes likely) the padding punches a 20 ms hole into unsent audio of the same packet — an FCS failure by another route. Run the padding regardless of the resampler, and refuse (rather than evict) when padding would overflow.

Nits (non-blocking; the sharpest inline, rest here)

• The drain budget travels as caps.extensions["icom"]["txAudioDrainBudgetMs"] — an untyped string key produced in one file and consumed in another behind a family == "icom" gate; a typo on either side silently degrades to a 0 ms drain, i.e. the truncation bug this PR fixes, with no diagnostic. A plain int field on RadioCapabilities (default 0) deletes the key, the nested map, and the family gate.
• The budget itself is the compile-time worst case (250+300 ms) applied to every packet, though pendingBytes() is exposed and known at drain time — ~550 ms of dead air per packet on a half-duplex channel, and it additively overrides the operator's TX-Tail knob's observable meaning.
• family == QLatin1String("icom") now gates behavior in three layers (dialog ×2, RadioModel) — the property being tested is "PTT readback is radio-confirmed", which wants to be a RadioCapabilities flag before a second backend gains readback.
• The PTT confirm barrier re-implements IcomTciUnkeySettle's generation/confirm/expire machinery inline (untestable except through the whole dialog), with a 2000 ms timeout beside TciServer's field-tested 1250 ms and no cross-reference.
• writeAx25Float32Wav is the repo's sixth hand-rolled WAV emitter (RADEEngine, ClientPuduMonitor, QsoRecorder, RemoteAsrBackend…) — this PR would have been the moment for one WavWriter; relatedly the capture-id format string and its validating regex are two unlinked literals, and captures land in the settings dir (unbounded float32 WAVs beside AetherSDR.db, invisible to SupportBundle) rather than a diagnostics dir.
• The dialog writes the multi-MB generated-TX WAV synchronously on the GUI thread immediately before keying, and discards the backend extension's arm result — so "capture armed" is printed even when arming failed.
• The capture accumulator is the third bounded-PCM-append with its own cap policy (seconds vs 64 MiB vs wrap), so the correlated stages this feature exists for can truncate at unrelated points.

What was verified vs read

• Verified by me: the pump's shared blast radius (mic → submitTxAudio → same packetizer), the deleted catch-up comment, the missing already-keyed pre-check and the radioTransmitConfirmed doc text, the retired test's bracket-comment state at the PR head, socket-freedom of the other three tests, and that tests.cmake registers nothing new beyond shim-test build wiring.
• From the automated pass, verified before adoption: the extension-key fragility, worst-case-budget cost, WAV/capture duplication set; several of its candidates were dropped as refuted.
• Not run: the retired socket fixture (per step 0, recorded above); no bridge session (the demo cannot exercise RS-BA1 pacing); the live-radio interop claim rests on the author's IC-705/TH-D75 evidence, which is the right kind of proof for this path and is credited as such.
• IcomSession default changes (txBufferMs 200→300, pump 10→20 ms) and the new cross-layer signal routing are the two categories AGENTS.md's Autonomous Agent Boundaries reserves for explicit maintainer sign-off ("Default values", "Architecture — … changing signal routing"); the body describes them but does not flag them for design review — worth a sentence.
• Neither new test is in a PR ctest gate (no ci.yml change) — their static_assert-free runtime checks execute in no CI job; worth a gate line when the blockers are fixed.

[ten9876]

Blocker 1 — this comment's rule is right for the model, but its consumers were not updated, and main's deleted comment named the failure that comes back. What was removed here set m_keyed, m_meters.setTransmitting(), and emitted transmitChanged, with this justification:

"Setting m_keyed silently here and leaving the announcement to the poll does not work now that the poll only speaks on change… The model then read mox=false through an entire live transmission."

Four verified consequences of removing it without touching the consumers — voice/DAX head-clip on every over (submitTxAudio drops on !m_keyed and only the AX.25 dialog waits), TUNE silent inside the keyed window (see the comment at setTune: "a tuner sampling that edge can otherwise read infinite SWR"), a refused unkey never republished (the deleted test row: "published immediately, NOT suppressed") while RadioModel::setTransmit(false) already showed RX — operator transmitting with an RX indicator — and TCI's 1250 ms confirm aborting back-to-back FT8 periods for want of a change edge.

Keeping Principle II for the published model while gating audio/tune on m_pendingPttIntent || m_keyed, and re-publishing any readback that contradicts commanded intent, preserves this fix's goal and un-breaks the rest.

[ten9876]

Blocker 2 — the deleted comment guarded exactly this. From main:

"Drain every frame that is ready, not just one: a host audio callback can deliver several frames' worth in one block, and pacing them out one per 10 ms tick would fall permanently behind."

One frame per 20 ms tick makes producer and consumer equal-rate with zero recovery: a Qt timer only ever fires late, so backlog is monotonic until TxPacketizer's 250 ms cap drops the oldest bytes mid-stream — and this pump clocks all Icom TX audio (mic → submitTxAudio via takesTxAudioOverSeam), not just the modem. A long SSB over on a loaded desktop ratchets to the cap and then sheds speech.

Wire cadence and recovery are compatible:

Suggested change

	constexpr int kTxPumpMs = 20;
	// The radio consumes one 20 ms frame at a time. Keep this timer as the wire
	// clock, but drain by elapsed time: a late or coalesced tick may send the
	// frames it owes (bounded), so backlog cannot ratchet — while steady state
	// still never bursts the RS-BA1 stream.
	constexpr int kTxPumpMs = 20;

(with onTxPump sending min(elapsed/20ms, pending) frames, or draining while backlog exceeds a two-frame cushion).

[ten9876]

Blocker 3 — edge-only confirm with no current-state check. publishBackendTransmitEdge early-returns when the value hasn't changed, so if m_radioTransmitting is already true at requestPttOn — a previous packet's unkey readback still in flight (back-to-back KISS frames re-arm within singleShot(0)), or hardware PTT held — no signal ever fires and the 2 s timeout aborts a transmission the radio is actually making, after 2 s of dead carrier.

The repo has the purpose-built tool: RadioModel::radioTransmitConfirmed, whose doc comment says it carries "unchanged answers hidden by the change-gated radioTransmittingChanged signal" — TciServer already consumes it for this exact Icom question. Connect to that (or pre-check isRadioTransmitting() before arming the wait).

[ten9876]

Blocker 4 — the early return skips the padding too. A finite stream whose source is already at the negotiated rate (no resampler ever constructed) leaves its final partial frame un-padded; takeFrame() won't emit it and the unkey flushTxAudio() discards it — the tail-truncation class this PR fixes, surviving in the no-resampler variant. The padding is unconditionally correct:

Suggested change

	if (!m_txResampler) {
	if (!m_txResampler) {
	if (m_session && m_connected && m_keyed && !m_tuning) {
	const std::size_t paddedBytes = m_session->padTxAudioToFrame();
	qCInfo(lcIcomTx) << "Icom finite TX audio (native rate): silence padding"
	<< paddedBytes << "bytes";
	}
	return;
	}

Related, same function's downstream: padToFrame evicts the oldest pending frames to make room for trailing silence — near the cap, that punches a 20 ms hole into unsent audio of the same packet. Refusing the pad on overflow beats corrupting the frame it exists to complete.