fix(hl2): rate-limit the repeated ADC-overload warning

[on8st]

Two things up front

There is no test, and I'd rather say why than let you find it.
publishTelemetry is private and socket-driven, so the natural fixture is a
fake HL2 on localhost — which is exactly what tests/tests.cmake:697-724 has
commented out inside #[==[ ]==], under a header saying "Retired fake-radio
fixtures … deterministic assertions that survive are extracted into socket-free
tests rather than keeping a localhost peer in the default compile and CTest
graph." Reviving that to cover a log line would reverse a decision you have
already made, in a first contribution, which seemed like the wrong trade.

The socket-free route would be to extract the rate-limit decision into a small
pure helper and test that directly. I didn't do it unasked — it introduces a
type to make a qWarning testable, and that's your call rather than mine. Say
the word and I'll add it.

The severity figure in our own docs is stale, and that's the more interesting
half of this. docs/HERMES.md §15.7 records this warning at ~133/second on
the MW broadcast band, flushing the log ring. I could not reproduce anything
near that, and the reason is that you already fixed it without noticing:
MetisClient coalesces telemetryUpdated to 10 Hz (kTelemetryMinIntervalMs,
credited in-comment to "#4449 review"), with no change-bypass. publishTelemetry
therefore cannot run faster than 10 Hz however hard the comparator chatters, so
the ~133/s ceiling has not been reachable since that landed. Across every
AetherSDR log on this machine the warning peaks at 4/second.

So this PR does not claim to fix ring-flushing. §15.7's bullet is overstated
as it stands; happy to correct that line here or leave it to you.

What this actually fixes

The overload bit is a per-frame sample of a level comparator, not an event. On a
strong band it dithers, so the existing edge gate sees an edge nearly every time
it looks, and one message repeats at the full telemetry cadence for as long as
the band stays strong.

The edge gate is necessary and was never sufficient — §15.7 says so explicitly
("the gate is on the value changing, but the value genuinely chatters"). So it
stays, and a rate limit sits behind it: report the first transition, then at
most once per 10 s, carrying the count of transitions the window swallowed.

The count is reported rather than dropped because the rate is the severity
here: a flag that sets once is a hint, one that sets on every sample for a
minute is a front end being slammed. Suppressing silently would turn a loud
symptom into a quiet one.

Two details that are easy to get wrong and are handled deliberately:

• The window flush lives outside the edge test. Flushing only on the next
  transition would hold a finished burst's tally until the band goes loud again
  — possibly hours later, possibly never. publishTelemetry runs on every
  telemetry update, so the window closes on time whether or not the condition is
  still happening.
• The count is labelled as transitions seen, at the 10 Hz telemetry cadence
  — not comparator edges. Those are sampled far below their true rate, and
  were before this change too. The code comment says so, so nobody later reads
  the number as a hardware event count.

What this does not fix

It lowers a ceiling; it does not solve log volume. One message at up to 10/s on
a strong band is still repetitive, and a genuinely noisy environment will still
produce a summary line every 10 s for as long as it lasts. If you'd prefer a
different window, or the warning demoted to a category so it can be filtered,
both are easy changes — I picked 10 s because it keeps a real overload visible
within a few seconds of it starting.

Testing

Builds clean against Qt 6.8.3 on macOS under the project's own flags, including
-Wall -Wextra -Wpedantic. aethercore compiles and links.

Behaviour is unchanged on every path but the log line: the radio is commanded
identically, and m_adcOverload still tracks the flag exactly as before.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CkisdWQiVxcUG6r6X9W3Fg

[aethersdr-agent]

Issue fit

No linked issue — the PR body carries no fixes/closes #NNNN, and a search turns up no open issue for the log chatter. That is fine here: the defect is recorded in the tree's own backlog as docs/HERMES.md row 6a ("Rate-limit the ADC-overload warning", §15.7, sized XS) and described in §15.7 ("fires ~133 times/second, flushing the log ring… it also buries every other log line, which is how it obstructed the diagnosis in §15.5"). GOVERNANCE.md exempts bug fixes with a clear root cause from the RFC path, and this is one. The diff addresses exactly what 6a asks for: the edge gate stays, a 10 s rate limit sits behind it, and the swallowed count is carried.

I verified the body's two load-bearing factual claims rather than taking them:

• ~133/s on MW, HERMES.md 15.7 — confirmed verbatim at docs/HERMES.md:930 and :1756.
• "MetisClient has since coalesced telemetryUpdated to 10 Hz (#4449)" — confirmed at MetisClient.h:388 (kTelemetryMinIntervalMs = 100) and MetisClient.cpp:885-890. The header is right to call the 133 figure stale rather than repeat it; that is a more honest comment than most.

Scope

File	What it changes	Claimed by title/body?	Verdict
src/core/backends/hl2/Hl2Backend.cpp	Splits the edge gate from the emit; adds the windowed flush	Yes	In scope
src/core/backends/hl2/Hl2Backend.h	Three new members + the rationale comment	Yes	In scope

git diff --stat against the merge base is 53 insertions, 0 deletions across 2 files. No deleted guard, no removed comment, no new public/protocol surface, no settings key, no default changed, no CHANGELOG.md entry. QElapsedTimer was already included at Hl2Backend.h:6, so the header addition needs no new include. Everything in the diff is explained by the stated fix.

Sibling check: grep -rn adcOverload src finds only MetisProtocol.{h,cpp} (the bit decode) and this backend. There is no second copy of this warning left unfixed, and no other backend carries an equivalent.

Blockers

None.

Nits (non-blocking)

• The comment at Hl2Backend.cpp:4895-4897 states a reason narrower than the code. See inline — the edges > 1 gate is correct, but a window can expire holding exactly one edge without that edge being the first-ever, and the operator then gets a bare message that may be up to 10 s stale.
• docs/HERMES.md is left asserting the fixed behaviour in the present tense. Backlog row 6a (:930) is unstruck and §15.7 (:1755-1758) still reads "Noticed, not fixed… fires ~133 times/second". The table's own convention for a landed item is ~~12a~~ … **DONE** (:942). The header comment goes out of its way to flag that figure as stale — the doc it cites should say so too.
• qWarning() << "(" << count << ... renders as ( 51 times in 10000 ms) — QDebug's automatic spacing after the paren. Cosmetic only.
• No test. I do not think this is a blocker and I am not asking for one as written: publishTelemetry is private, reachable only through MetisClient::telemetryUpdated, and driving it needs a live UDP peer — there is no deterministic, policy-compliant seam today, and a fake radio peer is exactly what AGENTS.md tells us not to build. If a maintainer does want this pinned, the tree already has the shape for it: Hl2TxLevelPolicy.h is a pure policy header carved out of this same backend, and the (edges, elapsed) → (emit?, count) decision would fit that mould as a socket-free CTest. Worth naming as the option, not worth withholding a merge for.

No socket tests are added, modified, or removed by this PR.

What I tried to break

Findings below are reasoned from the code in /tmp/aetherclaude/pr-5381; I have no build or runtime here, so none of this was reproduced by execution.

• elapsed() on an invalid QElapsedTimer. The edges > 1 branch reads m_adcOverloadClock.elapsed(); on an invalid timer that returns garbage. I tried to reach it: it requires edges ≥ 2 while the clock is still invalid, which requires two increments with no flush between them. The flush block is unconditional on every publishTelemetry call and the increment is at most one per call, so the first-ever edge always flushes in the same call and resets to 0. Unreachable. This is the one genuinely sharp edge in the diff and it is closed.
• "A burst that stops must still report its tally." This depends on publishTelemetry continuing to run after the overload stops. I checked whether the 10 Hz emit is value-gated — it is not: MetisClient.cpp:873-890 sets telemetryChanged = true whenever an EP6 response parses, not when a field changes, so the emit free-runs at 10 Hz for as long as the stream is up. The claim holds. Had it been value-gated, a settled radio would have stranded the tally.
• First-warning latency for a genuinely new overload. The clock is restarted only when a message actually fires, so after a quiet period it is long-expired and the next edge reports immediately — the leading-edge behaviour of the old code is preserved for isolated events. Suppression only applies within 10 s of a report, which is the intended rate limit.
• Counter overflow / drift. int m_adcOverloadEdges accumulates at most one per telemetry sample at 10 Hz over a 10 s window, so ≤ ~100 per window and reset on every flush. No overflow path.
• Dangling else / brace mis-nesting. The new if/else sits outside the edge-test block, and the unbraced if (m_adcOverload) ++m_adcOverloadEdges; has no else attached. Reads correctly.
• Cross-session state. New members are never reset on disconnect, so I checked lifetime: RadioModel::makeBackend (RadioModel.cpp:692) constructs a fresh Hl2Backend per connection, so nothing carries over. (m_adcOverload had the same property before this PR.)
• CI: build, check-macos, check-windows and Static checks all report success on cb823ad. That is a filtered subset and I did not treat it as proof of anything beyond compiling on three platforms.

Recommendation

Approve with nits. The code is correct as far as I could push it — the one dangerous path (elapsed() on an invalid timer) is provably unreachable, and the two factual claims the design rests on both check out against the tree. The nits are a comment that is narrower than its code, a doc left asserting the symptom this PR removes, and log-string spacing; none of them change behaviour. Concrete next step: strike backlog row 6a and update §15.7 in docs/HERMES.md in this PR, since the header comment already cites that section as the record.

Nice work — the header comment distinguishing the historical figure from the current rate, and being explicit that the count is transitions seen at 10 Hz rather than true comparator edges, is the kind of honesty that makes this reviewable at all.

One more thing: commit signing

main requires verified signatures, and 1 of your commits on this branch is unsigned — routine setup, not a code problem.

Quickest setup (SSH key signing, no GPG needed):

git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub
git config --global commit.gpgsign true

(If you have no SSH key yet: ssh-keygen -t ed25519 first.) Then on GitHub: Settings → SSH and GPG keys → New SSH key → set the key type dropdown to Signing Key → paste the .pub.

Re-sign the commit already on this branch:

git rebase main --exec "git commit --amend --no-edit -n -S"
git push --force-with-lease

For GPG or troubleshooting: https://docs.github.com/authentication/managing-commit-signature-verification

---

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

[aethersdr-agent]

The gate itself is right, but the reason given is narrower than the code's behaviour.

edges > 1 ⟹ the clock is valid — true. But the converse framing ("the first ever transition always reports immediately, so anything counted beyond it was counted against a running window") implies edges == 1 at this point means first-ever, and it does not. A window that expires holding exactly one swallowed edge also lands here: overload at t=1s inside a live window, suppressed, then flushed at t=10s. The operator gets the bare message with no count and no elapsed — indistinguishable from a live first-ever overload, but up to kAdcOverloadWarnIntervalMs stale.

The output is defensible either way (a count of 1 carries no information), so this is a comment fix, not a code fix — but the stated reason should cover the case that actually reaches it. Something closer to: a count of one carries nothing the message does not already say, so the plain form is used whether that one edge is the first ever or the only one a window swallowed.

[aethersdr-agent]

QDebug inserts a space between streamed items, so this renders as … attenuate ( 51 times in 10000 ms) — space after the open paren. Cosmetic; a single QString::arg form would read cleaner if you touch this again.

[aethersdr-agent]

This comment is careful to mark the ~133/s figure as stale — but docs/HERMES.md, the source it cites, still asserts it in the present tense and still lists the fix as outstanding:

• :930 — backlog row | 6a | Rate-limit the ADC-overload warning | §15.7 | …**~133 warnings/second**… | XS |, unstruck
• :1755-1758 — §15.7 "Noticed, not fixed" → "the warning in publishTelemetry — although edge-gated — fires ~133 times/second, flushing the log ring"

The table's convention for a landed item is right above it at :942: ~~12a~~ … **DONE**. Suggest striking 6a and moving/annotating the §15.7 bullet in this PR — non-blocking, but leaving the doc asserting the symptom this PR removes is the kind of drift the header comment is otherwise guarding against.

[on8st]

Thanks for checking the §15.7 quote and the #4449 coalescer against the tree rather than taking them from the body — that was the half of this I was least able to prove to you.

All three nits addressed.

docs/HERMES.md asserting the fixed behaviour in the present tense — ec01ee1. Row 6a is struck using the ~~12a~~ … **DONE** convention already in the table. §15.7 is corrected rather than rewritten: the ~133/s stays as what was measured, followed by the fact that it has not been reachable since #4449 coalesced telemetryUpdated to 10 Hz, and then what remained and is now fixed. I kept the original observation instead of deleting it — a symptom that quietly stops reproducing for a reason nobody recorded is the kind of thing that gets rediscovered as a new bug later.

The comment stating a reason narrower than the code — 0b8d0cc. You were right, and the part that was actually load-bearing was buried under the part that was wrong. It now says two separate things: why reading elapsed() is safe (more than one counted transition implies a valid clock, because the flush is unconditional and the first transition always flushes in the same call), and that a single counted transition does not mean "first overload ever" — it means one transition was seen in this window, which may have arrived at any point since it opened, so a bare message can lag by up to the window. That lag is called out as accepted, with the reason.

( 51 times in 10000 ms) — 0b8d0cc. Composed into a single QStringLiteral with noquote() instead of streaming the paren as its own item.

One thing worth flagging for whoever merges, so it isn't a surprise: the commits on this branch are signed, but with an SSH key that is not yet registered as a Signing Key on the account, so GitHub shows them unverified. Registering the key is in progress. Once it lands I'll re-sign the branch — content-identical, signature only.

[Ozy311]

No issue is linked. Against the PR's stated intent, the change is directionally correct: it counts ADC-overload assertions and emits at most one aggregate warning per ten-second window. This is a clear-root-cause bug fix and does not need an RFC, but the current implementation is not ready to merge.

Scope

File/group	Claimed by title/body?	Verdict
src/core/backends/hl2/Hl2Backend.cpp	Yes	Core rate-limit behavior; in scope
src/core/backends/hl2/Hl2Backend.h	Yes	State and interval for that behavior; in scope
docs/HERMES.md	Yes	Roadmap status and corrected 10 Hz telemetry description; in scope

Everything in the diff is explained by the PR. I confirmed the 100 ms telemetry coalescing in MetisClient, so the documentation's corrected 10 Hz figure holds.

Blockers

1. The first warning restarts an invalid QElapsedTimer (inline at Hl2Backend.cpp:4919). Qt defines that call as undefined behavior. The first overload is exactly the path where the clock is invalid, so the operation intended to arm the limiter is not reliable. Use start(); no restart return value is needed.
2. The new timing/state policy has no deterministic regression (inline at Hl2Backend.cpp:4891). A socket-free pure-policy seam is practical and already has a nearby project precedent in Hl2TxLevelPolicy.h; the repo review standard therefore treats this missing coverage as blocking.

Nits (non-blocking)

• m_adcOverloadEdges and several comments say "transitions", but the counter increments only on rising/asserted overload edges (inline at Hl2Backend.cpp:4872). Naming it for overload assertions/rises would make the aggregate message unambiguous.
• The current force-pushed head has no GitHub status checks yet. All three commits are GitHub-verified, but CI still needs to run after the blockers are fixed.

What I tried to break

• Configured and built the exact PR head's aethercore target successfully on macOS; the existing socket-free hl2_tx_level_policy_test also passed.
• Ran tools/check_engine_boundary.py --strict: 0 blocking findings (known baseline warnings only), and git diff --check passed.
• Checked the PR head against current origin/main with git merge-tree; it merges cleanly even though main has touched Hl2Backend.cpp since this branch's merge base.
• Walked first assertion, repeated chatter, quiet-window flush, and post-quiet assertion paths. The invalid-first-restart defect is code-derived; the remaining state transitions hold under that walkthrough.
• I did not drive the app: demo mode cannot produce HL2 telemetry, real station hardware is out of review scope, and a synthetic HL2 firmware peer is prohibited by the test policy.

Recommendation

Request changes. Replace the invalid restart() with start(), extract the rate-limit decision into a socket-free policy seam, and add a registered mutation-checked CTest for first assertion, chatter suppression/aggregation, quiet flush without a new edge, and an assertion after a quiet interval. The rest of the diff is scoped, builds, and survives the current-main merge check.

[Ozy311]

Blocker: m_adcOverloadClock is default-constructed invalid, and the first overload reaches this line through !m_adcOverloadClock.isValid(). Qt explicitly documents restart() on an invalid QElapsedTimer as undefined behavior: https://doc.qt.io/qt-6/qelapsedtimer.html#restart. That makes the call intended to arm the limiter platform-dependent. Since the return value is unused, start() is the safe reset for both invalid and valid clocks.

Suggested change

	m_adcOverloadClock.restart();
	m_adcOverloadClock.start();

[Ozy311]

Blocker: this introduces a deterministic timing/state policy without regression coverage. It does not require a fake Metis peer: extract the (overload sample, elapsed time) -> warning decision/count state machine into a pure socket-free helper (the existing Hl2TxLevelPolicy.h is a close precedent) and register a CTest in tests/tests.cmake. Cover first assertion, repeated chatter inside 10 s, tally flush on a later unchanged telemetry sample, and an assertion after a quiet interval; mutation-check the rate gate and the placement of the flush outside the edge test.

[Ozy311]

Nit (non-blocking): this counts only false-to-true overload assertions, not both transitions. Consider m_adcOverloadRises / m_adcOverloadAssertions and matching prose so the eventual N times summary has one precise meaning.

[on8st]

Both blockers addressed in 7ba6b45.

Blocker 1 — restart() on the never-started timer: you are right, and it was on the worst path — the first assertion is exactly where the clock is invalid, so the arming that mattered most was the undefined one. Replaced with start(); the discarded return value is gone with it.

Blocker 2 — the rate-limit decision now lives in a socket-free seam, Hl2OverloadPolicy.h, evaluated by the backend rather than copied, with a registered ctest of thirteen assertions: the four you named (first assertion immediate against an invalid clock; chatter suppressed inside the window; the window flushing with no new assertion so a stopped burst still reports its tally; an isolated overload after a quiet interval) plus silence when nothing was seen on both an expired window and an invalid clock, and both sides of the expiry boundary. Mutation-checked: four mutations (first-assertion case removed; aggregate threshold off by one; expiry boundary made strict; nothing-seen guard deleted), each caught by three or four assertions; restored, 13/13.

Nit: m_adcOverloadEdges → m_adcOverloadAssertions, and the "transitions" comments say "assertions" — the counter only increments on a rising edge, and the old name invited exactly the off-by-one the second mutation now checks.

Checkers: check_engine_boundary.py --strict 0 blocking (86 pre-existing tracked warnings), check_test_registration.py --strict OK, git diff --check clean.

[on8st]

Head is now e3d28da0 on 776a70d1: the previous push had carried an unrelated commit (a guard test that no command addresses register 0x17) into this PR beneath the fix; it is removed as out of scope and kept on its own branch. The PR is now exactly the rate-limit change, its policy seam and its test. Re-verified on the new head: 13/13, checkers clean.

[Ozy311]

Follow-up to my review of 776a70d17514bcb7d9b1f059185b4dc6e3691603, now at e3d28da04abfbaa120a340af8f4abad7119c2f28. No linked issue; reviewed against the stated overload-log rate-limit intent. The invalid-timer blocker is fixed with start(), and the production decision now has a registered, socket-free regression test. I found no remaining blocking code defect.

Scope

File/group	Claimed?	Verdict
Hl2Backend.{cpp,h}	Yes	Rate limiter, safe clock initialization, assertion naming; in scope
Hl2OverloadPolicy.h, hl2_overload_policy_test.cpp, tests/tests.cmake	Author follow-up	Production decision seam and registered coverage; in scope
docs/HERMES.md	Yes	Corrected historical telemetry-rate explanation; in scope

The unrelated register-0x17 test commit mentioned in the response is absent. Current-main additions survive a clean merge-tree calculation.

Blockers

None identified in this follow-up. This is a COMMENT, not a merge approval.

Nits / evidence limits (non-blocking)

• Please refresh the PR body: it still leads with “There is no test” and offers to add the helper that this head already contains.
• The new test proves the decision for an accumulated count and elapsed time. It does not invoke publishTelemetry, so the quiet-burst caller placement, edge counting/reset, and actual QElapsedTimer operation remain code-inspected, not integration-tested. I checked that the production decision is outside the edge gate and start() is used; I am not treating the test's case 4 as empirical proof of that wiring. A future stateful seam test should distinguish these layers.
• Current head has no status checks in GitHub's rollup. The four commits are GitHub-verified, but required CI and the human Tier-3 @aethersdr/reviewers path remain outstanding; my local checks do not replace them.

What I tried to break

• Built the exact head's hl2_overload_policy_test and Hl2Backend.cpp object on macOS arm64, AppleClang 21 / Qt 6.11.1. Focused CTest passed (1/1); direct execution passed all 13 assertions.
• Independently broke four production-policy conditions: invalid-clock bypass, aggregate threshold, expiry comparison, and zero-count silence. All four mutants failed (2, 2, 3, and 2 assertions respectively); restored source passed. No mutation remains in tracked files.
• Inspected first-warning, suppressed chatter, unchanged-telemetry flush, long-quiet interval, and invalid-clock aggregate reachability. Metis telemetry continues on parsed frames, not only on value changes. No wire commands, TX policy, or other-radio paths change.
• check_engine_boundary.py --strict: 0 blocking findings (86 tracked warnings). check_test_registration.py --strict and git diff --check pass. Merge-tree against current main succeeds.
• Test-boundary preflight: the added test owns no socket or peer. No app or live-radio session was run. The demo cannot exercise HL2 telemetry, and deliberate live ADC overload would not improve on controlled timing inputs; silence on a dummy load would be inconclusive.

Recommendation

Code concerns from the prior review are addressed; continue through normal CI and human review after updating the stale body. Do not interpret this as approval to merge.

Snapshot: trusted base dafe441b194bb570fa44836c415a3ebae03280f8; merge base 0f78183cfe34c78b8b349310124b46b73e9396e1; governance manifest PASS, constitution mirrors identical. Repository PR-review skill SHA-256 dc9bd117248079081ef9f5708e2ea3e9789b5c9f71c5efcf560054186fe309a4; constitution d585c0383b782a8ade22a341134ab16b63f897446b1dc2fe1fc990590312d438. No separate model-invocable code-review skill was available; adversarial pass performed directly. Head, existing reviews and sole Ozy311 assignment rechecked before posting.