feat(icom): count and report TX packetiser drops instead of losing audio silently

[nigelfenton]

Summary

TxPacketizer::submit() discards the OLDEST bytes past its 250 ms cap. For voice that rule is right, and the comment at the drop site argues it well: latency must not grow, and the freshest audio is what matters. For a digital burst it is destructive in a way nothing reported — the oldest bytes of an AX.25 transmission are the preamble and the opening flag, so what survives keys the radio, sounds exactly like packet on a receiver, and syncs on nothing.

Nothing surfaced that. No error, no gap, no counter: a transmission that lost 63% of itself looked identical to one that did not.

This PR makes the drop visible. It does not change the drop rule.

• submit() counts dropped bytes and overflow events; one oversized submit is one event, so a single huge write and a thousand small ones are distinguishable
• IcomSession::Stats carries txDroppedBytes / txDropEvents / txPendingBytes
• onLinkTick reports new drops at WARNING on the edge — running total plus pending/cap ratio — rather than once per dropped byte (an oversized submit sheds thousands of bytes in one loop)
• counting rather than logging keeps IcomAudio.cpp pure protocol: no QObject, no Qt logging, which is also what lets the unit test link it standalone

Test

icom_tx_packetiser_drop_test — standalone, links IcomAudio.cpp + Qt6::Core only. 13 checks:

• under-cap submits drop nothing and record no event
• an oversized submit leaves the queue holding exactly its cap, and droppedBytes equals submitted minus retained — the arithmetic, not the counter's own word for it
• events count submits; bytes accumulate across them
• resetDropCounters() clears both counters
• draining the queue does not disturb the accounting

The test was proven falsifiable before being trusted: with ++dropped commented out it fails 5 of 13 checks and exits non-zero. All 13 pass on this branch (Windows/MSVC, Qt 6.10.3).

Origin and scope

Split out of #5058 per review feedback there — this piece is self-contained and independent of the tap-architecture discussion, so it can land on its own.

Context is #5011: the failure this instrument makes visible — a burst losing its front while still sounding intact — is the same symptom that PTT timing produces by another route (see the code trace in that issue). Distinguishing the two on a live transmission is exactly what this counter is for: if a truncated burst shows txDroppedBytes advancing, the packetiser did it; if the counter stays flat, the front was lost before the radio was keyed.

This is diagnosis, not a fix: the drop rule is unchanged, and a burst that overflows still loses its front.

Not exercised here: the onLinkTick WARNING path against a live RS-BA1 session — the unit test covers the counter arithmetic, and the reporter compiles and links, but no radio was on the bench for this branch. The standalone test is the proof boundary.

🤖 Generated with Claude Code

Evidence: what is observed vs. what is inferred

Raised by @ten9876 in review, and it belongs in the body rather than a comment.

Observed. The counter mechanism itself. icom_tx_packetiser_drop_test drives the overflow directly and asserts the counts; the central claim was mutation-tested (undercounting by half fails the test).

Inferred, not observed. The production path that makes this matter — sendAudio() enqueuing while onTxPump() drains nothing until the codec reports ready — is reasoned from the code. I have no IC-9700 or IC-705 on this bench and have not watched the overflow fire on real hardware.

That distinction is load-bearing: it is the difference between "a silent front-of-queue discard is now visible" (demonstrated) and "this silently corrupts AX.25 bursts in the field" (argued from the code, and consistent with #5011, but not witnessed here).

[aethersdr-agent]

Issue fit

Partially. There is no Fixes #NNNN — the commit message names #5011 as context and says this is split out of #5058, which reads correctly: #5011 explicitly asks for nothing to be fixed in AetherSDR and ranks "surface it at runtime" as option 2. This PR is that option, narrowed to one counter. Judged against its own stated intent — make a silent front-of-queue discard visible — the mechanism is sound and the unit test is honest. One defect stops it doing that reliably across a reconnect (Blocker 1).

One claim from #5011 worth putting back on the record, because this PR's comments lean on the opposite: the issue's own elimination table says "packetiser drops — the qCWarning that fires on any drop never fired across ~12 keyings." Whatever tree that warning lived in (there is none on main), the author's own measurement is that this overflow did not occur in the sessions that motivated it. That does not make the instrument unwarranted — it makes the in-code assertion that an oversized submit is "reachable in production, not just in theory" unevidenced. See the nit on IcomAudio.cpp:167; I did find a plausible reachable path, but it isn't the one the comment names.

Scope

File / group	What it changes	Claimed by title/body	Verdict
IcomAudio.{h,cpp}	m_droppedBytes / m_dropEvents, accessors, resetDropCounters()	yes — "count"	In scope
IcomSession.{h,cpp}	three new Stats fields, populated in stats()	yes — carries the count to the owner	In scope
IcomCivBackend.{h,cpp}	edge-detected qCWarning in onLinkTick, m_lastTxDroppedBytes	yes — "report"	In scope (see Blocker 1)
tests/icom_tx_packetiser_drop_test.cpp, tests/tests.cmake	new standalone test target	yes	In scope; the target is redundant — see Nits

No unrelated files, no deleted guards, no CHANGELOG.md entry, no settings/persistence/capability surface, no UI or default changed, no new protocol verb. Stats is an internal struct, not third-party-visible. Nothing here is a personal-preference change: it adds a log line at an existing failure, it does not alter any behavior an operator can see. Everything in the diff is explained by the stated intent.

Blockers

1. m_lastTxDroppedBytes outlives the counter it tracks, so the warning goes silent after a reconnect. (inline: IcomCivBackend.cpp:4854)

TxPacketizer's counters are per-TxPacketizer, and m_tx is reassigned on every IcomSession::start() (IcomSession.cpp:66) — on top of which the whole IcomSession is a fresh make_unique on each connect (IcomCivBackend.cpp:606). m_lastTxDroppedBytes is a backend member and is reset nowhere: grep -rn m_lastTxDroppedBytes src/ returns only the declaration, the comparison, and the assignment.

Failure scenario, reasoned from the code (not reproduced — I have no build here): session A overflows and accumulates, say, 36 000 dropped bytes; m_lastTxDroppedBytes ends at 36 000. Operator reconnects. Session B's counter starts at 0. Session B then loses the preamble of an AX.25 burst — 10 000 bytes. 10000 > 36000 is false, so no warning is emitted at all, and none will be until session B exceeds session A's lifetime total. That is precisely the "no error, no gap, a transmission the operator can hear on a second receiver" failure the commit message is written to end.

The sibling in the same function already gets this right: m_schedulerTimeoutsReported is reset at IcomCivBackend.cpp:676 and :2781. Adding m_lastTxDroppedBytes = 0; next to the one at :676 matches the established pattern and closes it. (resetDropCounters() is added but never called from anywhere in src/ — resetting the backend-side tracker rather than the session-side counter is the fix that fits the existing shape.)

Nits (non-blocking)

• tests/tests.cmake:422 — Qt6::Core is not needed. IcomAudio.cpp includes only <algorithm>/<cmath> plus IcomAudio.h, and IcomProtocol.h includes no Qt header either. The pre-existing icom_audio_test (tests.cmake:439) compiles the same IcomAudio.cpp and links no Qt at all. The link line also mildly contradicts the target's own comment ("no QObject and no Qt logging").
• The whole target is arguably redundant. icom_audio_test already exists to drive this exact class — IcomAudio.h:17 says so in as many words. Five check() blocks appended there would need no new target, no new CMake stanza, and no second compile of IcomAudio.cpp. Maintainer's call; not worth a re-push on its own.
• Neither test gates a merge. ci.yml:385 filters ^icom_(civ|civ_scheduler|meters|backend)_test$, which excludes both icom_audio_test and the new target. So "CI green" on this PR does not mean these assertions ran. If the counter is worth pinning, adding it to that regex is the cheap way to keep it pinned.
• IcomAudio.cpp:167 cites a function that doesn't exist. There is no onTxPaceTick anywhere in src/; the drain is IcomSession::onTxPump (IcomSession.cpp:728), and it doesn't "ship a larger chunk when a tick lands late" — it loops takeFrame() until empty, which is the opposite shape. The reachable overflow path I did find is different and stronger: sendAudio() enqueues whenever m_params.enableTx (IcomSession.cpp:766), while onTxPump() returns early unless m_audio && m_audio->isReady(). Producer with no drain ⇒ 250 ms of pending and then sustained front-drops. That's worth having in the comment instead.

What I tried to break

• The arithmetic. Cap 24 000; a 2.5× submit is 30 000 samples ⇒ 60 000 bytes; while (size > cap) leaves exactly 24 000 and sheds 36 000 = submitted - cap. The test's assertion is against that arithmetic, not against the counter's own word, exactly as it claims. Survived.
• The falsifiability claim. The commit says commenting out ++dropped fails 5 of 13 checks. I counted 13 check() calls, and traced the mutation: because ++m_dropEvents is nested under if (dropped > 0), both counters go dead, failing 2 in block 2, 2 in block 3, and 1 in block 4 = 5. The claim holds. A test that only asserted droppedBytes > 0 would have caught 3; this one is not self-confirming.
• Termination and lifetime in the test. while (!p.takeFrame().empty()) terminates — takeFrame returns {} below kAudioFrameBytes. submit(tone(n)) builds a span<const float> from a prvalue vector; legal (is_const_v<element_type> satisfies the range ctor) and the temporary outlives the call. No dangling.
• The standalone link. kAudioFrameBytes / kAudioSplitLarge are inline constexpr in IcomProtocol.h (:463, :472), so compiling IcomAudio.cpp without IcomProtocol.cpp links. The new target is not a build break.
• Unsigned underflow in the log line. s.txDroppedBytes - m_lastTxDroppedBytes is guarded by the > test, so a backwards-moving counter prints nothing rather than ~2^64. That guard is also what makes Blocker 1 silent instead of loud — correct arithmetic, wrong outcome.
• Log volume. kLinkTickMs = 1000 (IcomCivBackend.cpp:70), so a sustained overflow warns once per second, not per drop or per 10 ms pump. Reasonable.
• CodeGuard. All eight CG-PATH-001 hits are in src/core/MidiSettings.cpp and tests/midi_settings_test.cpp — neither file is in this diff. Nothing to fold in.
• CI. All four checks pass on ed1e248. Per the filter at ci.yml:385, that green does not cover the new test; treat it as "the tree builds on three platforms", which it does.

Everything above is reasoned from the head checkout at /tmp/aetherclaude/pr-5162; I am headless and ran no build, no ctest, and no radio.

Recommendation

Request changes — for Blocker 1 only, which is a one-line fix at IcomCivBackend.cpp:676 alongside the reset that is already there. The rest is a well-shaped, well-tested diagnostic: correct seam (count in the Qt-free protocol file, report in the owner), edge-detected, honest test. Worth fixing before merge rather than after, because the defect is in the reporting path itself — it fails the same way it is designed to prevent, and would look exactly like "no drops occurred."

---

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

[aethersdr-agent]

Blocker — this goes silent after a reconnect.

m_lastTxDroppedBytes is a backend member that is never reset, but the counter it tracks is per-TxPacketizer: m_tx is reassigned on every IcomSession::start() (IcomSession.cpp:66), and the whole session is a fresh make_unique on each connect (IcomCivBackend.cpp:606). grep -rn m_lastTxDroppedBytes src/ finds only the declaration, this comparison, and the assignment below.

So: session A drops 36 000 bytes and leaves m_lastTxDroppedBytes = 36000. Reconnect. Session B loses the preamble of an AX.25 burst — 10 000 bytes. 10000 > 36000 is false ⇒ no warning, and none until B exceeds A's lifetime total. That is exactly the silent failure this PR exists to end.

The sibling tracker directly above gets this right: m_schedulerTimeoutsReported is reset at IcomCivBackend.cpp:676 and :2781. Adding one line beside the first of those matches the pattern:

    m_schedulerTimeoutsReported = 0;
    m_lastTxDroppedBytes = 0;   // the session's counter restarts too

(Reasoned from the checkout, not reproduced — I have no build here.)

[aethersdr-agent]

Same issue as the inline on IcomCivBackend.cpp:4854: this member's initialiser is the only place it is ever set to 0, but the counter it mirrors restarts with each TxPacketizer / IcomSession. It needs a reset in the teardown block that already resets m_schedulerTimeoutsReported.

[aethersdr-agent]

Nit — the mechanism named here doesn't exist. There is no onTxPaceTick anywhere in src/; the drain is IcomSession::onTxPump (IcomSession.cpp:728), and it doesn't "ship a larger chunk whenever a tick lands late" — it loops takeFrame() until empty, which is the opposite shape and is what normally keeps m_pending under one frame.

The reachable overflow path I did find is stronger and worth having here instead: sendAudio() enqueues whenever m_params.enableTx (IcomSession.cpp:766), while onTxPump() returns early unless m_audio && m_audio->isReady(). Producer with no drain ⇒ 250 ms of backlog and then sustained front-drops.

Related: #5011's own elimination table records that a drop warning "never fired across ~12 keyings", so "reachable in production, not just in theory" is currently an assertion against the author's own measurement. Citing the isReady() gate would make it an argument.

[aethersdr-agent]

Nit — Qt6::Core isn't needed. IcomAudio.cpp includes only <algorithm> and <cmath> beyond its own header, and IcomProtocol.h pulls in no Qt either. The pre-existing icom_audio_test at line 439 compiles the same IcomAudio.cpp and links nothing. The line also sits oddly against this target's own comment three lines up ("no QObject and no Qt logging").

Suggested change

	target_link_libraries(icom_tx_packetiser_drop_test PRIVATE Qt6::Core)
	add_test(NAME icom_tx_packetiser_drop_test COMMAND icom_tx_packetiser_drop_test)

Broader (maintainer's call, not worth a re-push alone): icom_audio_test already exists to drive this class — IcomAudio.h:17 says so — so these five blocks could live there with no new target at all. Either way, note that ci.yml:385 filters ^icom_(civ|civ_scheduler|meters|backend)_test$, so neither this test nor icom_audio_test gates a merge today.

[aethersdr-agent]

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

• [MEDIUM] CG-PATH-001 — Potential path traversal in src/core/MidiSettings.cpp /tmp/aetherclaude/pr-5162/src/core/MidiSettings.cpp:623
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/core/MidiSettings.cpp /tmp/aetherclaude/pr-5162/src/core/MidiSettings.cpp:775
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/midi_settings_test.cpp /tmp/aetherclaude/pr-5162/tests/midi_settings_test.cpp:697
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/midi_settings_test.cpp /tmp/aetherclaude/pr-5162/tests/midi_settings_test.cpp:699
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/midi_settings_test.cpp /tmp/aetherclaude/pr-5162/tests/midi_settings_test.cpp:722
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/midi_settings_test.cpp /tmp/aetherclaude/pr-5162/tests/midi_settings_test.cpp:723
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/midi_settings_test.cpp /tmp/aetherclaude/pr-5162/tests/midi_settings_test.cpp:724
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/midi_settings_test.cpp /tmp/aetherclaude/pr-5162/tests/midi_settings_test.cpp:726

_{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: $7.1716 · model: claude-opus-5}

[nigelfenton]

All four review items addressed in a544f2ba:

• Blocker 1 fixed — m_lastTxDroppedBytes = 0 in the same teardown block as the m_schedulerTimeoutsReported reset, with a comment saying why it must die with the session. Your failure scenario was exactly right, and it fails the way the counter exists to prevent — good catch.
• onTxPaceTick comment replaced with your verified path: sendAudio() enqueues whenever TX is enabled while onTxPump() drains nothing until the codec reports ready. Checked against IcomSession.cpp before writing it down — producer with no drain is real and stronger than what I had.
• Qt6::Core dropped from the test target (matching icom_audio_test's no-Qt link).
• Gate regex extended to include the new test — a green gate that never ran the assertion is a trap this repo has already paid for once.

On the redundant-target nit: kept as its own target for now since it documents the standalone-linkability contract in its stanza, but zero attachment — fold it into icom_audio_test on a maintainer's word and I'll do it.

Full gate set run locally (civ, civ_scheduler, meters, backend, new test): green. icom_backend_test flaked once under bench load on a confirmation-timing assertion this change can't reach; clean on rerun.

[ten9876]

Issue fit

No linked issue, and none is needed — GOVERNANCE.md exempts bug fixes with a clear root cause, and the root cause here is stated precisely: submit() drops the OLDEST bytes past a 250 ms cap, which is correct for voice and destructive for a digital burst, because the oldest bytes of an AX.25 transmission are the preamble and the opening flag. A frame that keys the radio, sounds like packet on a second receiver, and syncs on nothing is about the worst failure shape available, and nothing surfaced it.

The discipline of making the drop visible without changing the drop rule is the right call and the body says so plainly.

Verified empirically on macOS/arm64: clean build of the full app target; icom_tx_packetiser_drop_test (0.38 s) and icom_backend_test (81 s) both pass. I also mutation-tested the central claim — undercounting by half:

m_droppedBytes += dropped / 2;
→ FAIL: droppedBytes equals submitted minus retained

so the arithmetic assertion is real and not the counter marking its own homework, exactly as the body claims.

No blockers. Two nits.

Things I checked that turned out fine

• Threading. New non-atomic std::size_t counters read from onLinkTick() looked like a cross-thread race, so I traced it: submit() is reached only via IcomSession::sendAudio() from IcomCivBackend, there is no moveToThread anywhere in the Icom backend, and onLinkTick() is a QTimer on the same object. The counters live on exactly the same call path m_pending already did, so they add no exposure the deque did not already have. No finding.
• The edge tracker. m_lastTxDroppedBytes = s.txDroppedBytes; sits outside the if, so a single overflow warns once rather than every tick forever.
• a544f2ba. Resetting m_lastTxDroppedBytes on disconnect is the subtle half of this, and the comment nails why: with a fresh TxPacketizer per connect, a stale tracker keeps a new session's drops silent until they exceed the previous session's lifetime total — the warning failing in precisely the way it exists to prevent. That is the kind of thing that normally ships broken and is discovered a year later.

The decision to keep IcomAudio.cpp free of Qt logging so the unit test can link it standalone is also the right trade, and it is what made the mutation test above cheap for me to run.

Scope

Everything is explained by the stated defect. No CHANGELOG.md entry — correct. 92315135 ("ci: drop the test-gate edit from this PR") narrows rather than widens, which brings me to the first nit.

Nits

• The new test is on no CI gate. (inline: tests/tests.cmake:417) Confirmed: grep -c icom_tx_packetiser_drop .github/workflows/ci.yml → 0. 92315135 removed that edit deliberately, and if the reason was conflict avoidance it is a well-founded one — I reviewed two other open PRs today (#5244 and #5240) that both edit the same ctest -R regex line on the Icom gate and will conflict with each other. Worth saying so in the body, and worth a follow-up once those settle: this is a 0.38 s pure-arithmetic test with no Qt GUI, no event loop and no hardware, which is the exact profile the comments around those gates describe as worth failing a merge on.
• resetDropCounters() has no production caller. (inline: IcomAudio.h:101) Its only two uses are in the test. That is consistent with the design — the packetiser is fresh per connect, so nothing in production needs it — but it is public surface that exists for the test, and a future reader will reasonably assume something calls it. Either a one-line comment saying "test-only; production gets a fresh packetiser per session", or drop it and let the test construct a second TxPacketizer.

Verified vs. read

Built and ran: the full app target, both test targets, and the undercount mutation above (reverted). Read: the threading trace, the edge-tracker placement, and the disconnect reset. Not verified: no IC-9700 or IC-705 here, so the production overflow path you describe — sendAudio() enqueuing while onTxPump() drains nothing until the codec reports ready — is reasoned from the code rather than observed. That path is the strongest part of the argument and it would be worth one line in the body saying whether you have seen it fire on hardware or inferred it, since it is what makes this a real-world bug rather than a theoretical one.

[ten9876]

Nit — registered with ctest, but on no CI filter.

$ grep -c 'icom_tx_packetiser_drop' .github/workflows/ci.yml
0

92315135 removed that edit on purpose, so this is a question rather than an objection: was it conflict avoidance? If so it was a good instinct. I reviewed two other open PRs today — #5244 and #5240 — that both add a target to the same ctest -R regex on the Icom gate (ci.yml:385), and they will conflict with each other on merge. A third PR joining that queue would have made it three-way.

Once those land, this test is close to ideal gate material: 0.38 s measured, links only IcomAudio.cpp + Qt6::Core, no Qt GUI, no event loop, no hardware, no display. And what it guards is invisible from every other angle — the whole premise of the PR is that a 63% audio loss looked identical to a clean transmission, so a silent regression here restores exactly the condition you set out to remove.

Worth a line in the body noting the gate was deliberately deferred and why, so it does not read as an oversight to whoever merges.

[ten9876]

Nit — this has no production caller.

Both uses are in icom_tx_packetiser_drop_test.cpp:101-102. Nothing in src/ calls it.

That is consistent with your own design note — IcomCivBackend::disconnectRadio()'s comment says the packetiser's counter "dies with the session (fresh TxPacketizer per connect)", so production never needs a reset. The awkwardness is that a reader arriving at this header sees a public mutator and reasonably assumes some caller depends on it, which affects how freely they'd change it.

Two clean options:

Suggested change

	void resetDropCounters() noexcept { m_droppedBytes = 0; m_dropEvents = 0; }
	// Test-only: production gets a fresh TxPacketizer per session, so nothing
	// in src/ resets these — see IcomCivBackend::disconnectRadio().
	void resetDropCounters() noexcept { m_droppedBytes = 0; m_dropEvents = 0; }

or drop it entirely and have the test construct a second TxPacketizer for the "counters start at zero" case, which also proves the per-session freshness the production comment relies on rather than asserting it.

[nigelfenton]

Thanks — and the things you checked that turned out fine are as useful to me as the nits, particularly tracing the threading rather than assuming a race from the new non-atomic counters.

resetDropCounters() now says test-only in place (032dc21f), with the reason: IcomSession builds a fresh TxPacketizer per connect, so production never needs it. Kept rather than dropped — the two-sequence assertion reads better with the reset than with a second packetiser.

The CI gate stays off in this PR, deliberately. 92315135 dropped that edit because #5244 and #5240 both edit the same ctest -R regex on the Icom gate and will conflict with each other; adding a third would compound it. You are right that a 0.38 s pure-arithmetic test with no Qt, no event loop and no hardware is exactly the profile those gate comments describe as worth failing a merge on — worth adding once those settle, as a separate change, since build-time CI gates are maintainer-merge territory here.

On your last point, which is the one that deserves a straight answer: the production overflow path is inferred from the code, not observed on hardware. I have no IC-9700 or IC-705 on this bench. sendAudio() enqueuing while onTxPump() drains nothing until the codec reports ready is read, not seen firing. You are right that it is what makes this a real-world bug rather than a theoretical one, and the body should have said so plainly rather than leaving it implied — I have updated it.

What is observed is the counter mechanism itself: the unit test drives the overflow directly, and I mutation-tested the central claim the same way you did.

[nigelfenton]

Bump for review — this one is small, standalone, and is the blocker for shrinking #5058.

What it is: TxPacketizer::submit() drops the oldest bytes when a burst exceeds its 250 ms cap. That is the right policy for voice, where the oldest audio is the least interesting. It is destructive for a digital burst, where the oldest bytes are the preamble and the opening flag — so an AX.25 frame loses exactly the part a TNC needs to acquire sync, and the failure was completely silent: no error, no log line, no gap in the audio.

This counts the drops and warns on the edge with a running total. Behaviour is otherwise unchanged — nothing new is dropped or kept, the loss is just no longer invisible.

Why it is separate: split out of #5058 at @aethersdr-agent's suggestion, with its own falsifiability-proven test (icom_tx_packetiser_drop_test). #5058 keeps the instrumentation and the architecture question; this is the one piece with a behaviour change that stands on its own.

Verification: Windows/MSVC clean; the test observes a real 40960-byte drop event rather than asserting on an exit code. Same limits as its parent — Windows only, IC-9700 over RS-BA1, one radio.

No rush if the queue is deep — flagging it mainly because #5058 cannot shed these commits until this lands.

73, Nigel G0JKN

[nigelfenton]

Bump for review — this one is small, standalone, and has already been through a clean review round.

221 additions across 8 files, one deletion. @ten9876 reviewed it on macOS/arm64 and found no blockers, having mutation-tested the central claim rather than taking it (m_droppedBytes += dropped / 2 → the arithmetic assertion fails, so the counter is not marking its own homework). The earlier agent review's Blocker 1 and all three nits are fixed on the head:

Finding	Fixed in	Where it landed
Blocker 1 — m_lastTxDroppedBytes outlived the per-session counter, so drops stayed silent after a reconnect until they exceeded the old session's lifetime total	a544f2ba	IcomCivBackend.cpp:703, beside m_schedulerTimeoutsReported — the sibling pattern the review pointed at
Nit — Qt6::Core not needed on the test target	92315135	gone from tests/tests.cmake:417-422; links no Qt at all now
Nit — comment cited onTxPaceTick, which does not exist	032dc21f	replaced with the reachable path the reviewer actually found: sendAudio() enqueues whenever TX is enabled while onTxPump() drains nothing until the codec reports ready — producer with no drain
Nit — resetDropCounters() never called from src/	032dc21f	marked test-only in a doc comment

Why this one matters beyond itself

It is the blocker for shrinking #5058. Commit 9d755d8f there is this same work; if this lands I rebase that branch and drop the commit, which takes a real bite out of a 1,438-line diff that has sat eleven days without a human review. That is the main reason I keep bumping this one — not urgency on its own.

⚠️ One thing a reviewer or merger should know: do NOT rebase this branch

It is 30 commits behind main. A rebase resurrects a .github/workflows/ci.yml edit that commit 92315135 exists to remove — an earlier version of this branch added tx_packetiser_drop to the Icom test gate, and main has since hardened that same block with a count-pin. Replaying the intermediate commit conflicts there and the naive resolution silently discards upstream's newer gate in favour of my old edit.

A merge is clean — I ran it locally: zero conflicts, and ci.yml in the merged tree is byte-identical to main. The net diff of this PR does not touch ci.yml at all, which is the correct end state. I have deliberately not pushed a merge commit; say the word if you would rather the branch be brought current and I will merge (not rebase) and push.

State

• Four CI checks green; MERGEABLE.
• Built and tested locally today with main merged in (3096621e), Windows/MSVC: clean build, and icom_tx_packetiser_drop_test, icom_audio_test, icom_civ_test, icom_civ_scheduler_test, icom_meters_test all 5/5 pass.
• Full local suite 312/320. The 8 failures are an identical set to what I get on two unrelated branches of mine today, and two of them are the same pair @skerker independently reports as pre-existing on feat(audio): compile the PortAudio CW sidetone sink on Windows (#5200) #5201. None touch this branch's files.

Not verified

No IC-9700 on the bench for this push — the counter is pinned by the unit test and by @ten9876's mutation run, not re-measured against a live radio. Worth restating the reviewer's own caveat, which I agree with: #5011's elimination table records that the drop warning never fired across ~12 keyings, so this instrument is not evidenced to fire in the sessions that motivated it. It makes a silent discard visible if it happens; it is not a claim that it did.

Also note ci.yml:385 filters the Icom gate, so neither this test nor icom_audio_test runs on a PR — "CI green" here does not mean these assertions executed. I am not touching that gate in this PR (it is build-time CI and a maintainer's call); flagging it so nobody reads the green checks as covering the new counter.

@jeremymturner @jensenpat @ten9876 — whenever there is a moment.

[nigelfenton]

Brought current with main (3096621e) — 029c8605.

Merged, not rebased, and that was deliberate. As flagged in the bump above: replaying this branch conflicts in .github/workflows/ci.yml, because an earlier version of a544f2ba added tx_packetiser_drop to the Icom test gate and 92315135 exists to remove that edit again. main has since hardened the same block with a count-pin, so the naive rebase resolution silently discards upstream's newer gate in favour of my withdrawn one. The reason is recorded in the merge commit message so the next person doesn't rediscover it.

Verified after the merge:

• .github/workflows/ci.yml is byte-identical to origin/main — git diff origin/main -- .github/workflows/ci.yml is empty. Upstream's count-pinned gate is intact and my old edit is not resurrected.
• The net PR diff is unchanged: 8 files, 221 insertions, 1 deletion, and still does not touch ci.yml. The merge contributed nothing to the diff under review.
• Blocker 1's fix survived — m_lastTxDroppedBytes = 0 is still in the session teardown beside m_schedulerTimeoutsReported (now IcomCivBackend.cpp:804; main shifted the file).

Built and tested on Windows/MSVC at 029c8605: clean build, and icom_tx_packetiser_drop_test, icom_audio_test, icom_civ_test, icom_civ_scheduler_test, icom_meters_test all 5/5 pass.

Still no radio on the bench for this push, so the caveats in the bump stand unchanged: the counter is pinned by the unit test and @ten9876's mutation run, not re-measured against a live IC-9700 — and ci.yml:385 filters the Icom gate, so neither this test nor icom_audio_test runs on a PR.

[nigelfenton]

CI is green on 029c8605 — and here is precisely what that does and does not prove

All four checks pass on the merge commit (verified head_sha = 029c8605, not a stale SHA): Static checks 18s, build 6m25s, check-macos 6m30s, check-windows 6m54s. No new compiler warnings in the three files this PR touches.

What it proves: the merge preserved upstream's gate

The Linux job ran $ICOM_GATE and the count-pin passed at exactly 7:

ICOM_GATE: ^(icom_civ_test|icom_civ_scheduler_test|icom_meters_test|icom_family_test|
             rf_gain_presentation_test|icom_power_derivation_test|phone_cw_mic_gain_authority_test)$
7/7 Test #319: phone_cw_mic_gain_authority_test ... Passed

That is the direct evidence for the merge-not-rebase decision above. A rebase would have replayed my withdrawn 5-name version of this gate over main's hardened one, and the count-pin would have failed the step. The guard main added is doing its job.

⚠️ What it does NOT prove: that this PR's assertions ran

icom_tx_packetiser_drop_test is built and linked in CI, then never executed. From the Linux log:

[537/2972] Linking CXX executable icom_tx_packetiser_drop_test

...and that is the last mention. On Windows it does not appear at all — 0 occurrences in the whole job log.

I checked this mechanically rather than by eye: I enumerated all 17 ctest --test-dir build -R invocations in ci.yml and ran each pattern through ctest -N against this build tree. Neither icom_tx_packetiser_drop_test nor the pre-existing icom_audio_test is matched by any of them.

So the four green badges mean this PR compiles and links on three platforms and breaks nothing that is gated. They do not mean the counter arithmetic was checked. Flagging it because a green check is easy to read as coverage, and here it is not.

Where the actual evidence for the counter comes from:

• icom_tx_packetiser_drop_test 5/5 locally on Windows/MSVC at 029c8605, alongside icom_audio_test, icom_civ_test, icom_civ_scheduler_test, icom_meters_test.
• @ten9876's independent macOS/arm64 run, including the mutation (m_droppedBytes += dropped / 2 → the arithmetic assertion fails), which is what shows the test is not self-confirming.

That is two platforms and one adversarial mutation — reasonable, but it is human-run evidence, not CI-enforced. If the counter regresses later, nothing on a PR will catch it.

Not proposing a fix here

Adding either test to that regex is a one-line change and I am deliberately not making it: ci.yml is build-time CI, which is privileged and a maintainer's call, and this PR's net diff must keep it byte-identical to main (it does — git diff origin/main -- .github/workflows/ci.yml is empty). If a maintainer wants the counter pinned in CI, the cheap way is adding icom_tx_packetiser_drop_test to ICOM_GATE and bumping the count-pin from 7 to 8 — but that is your change to make, not mine to slip into a PR about drop counters.

@jeremymturner @jensenpat @ten9876

[jensenpat]

Superceded by #5311