-
-
Notifications
You must be signed in to change notification settings - Fork 114
fix(hl2): rate-limit the repeated ADC-overload warning #5381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
922e9d5
8e20af6
776a70d
e3d28da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4869,7 +4869,55 @@ void Hl2Backend::publishTelemetry(const Hl2Telemetry& t) | |||||
| if (t.adcOverload && *t.adcOverload != m_adcOverload) { | ||||||
| m_adcOverload = *t.adcOverload; | ||||||
| if (m_adcOverload) | ||||||
| ++m_adcOverloadEdges; | ||||||
| } | ||||||
| // Rate-limited, not merely edge-gated. The edge gate above is necessary and | ||||||
| // was never sufficient: the comparator genuinely chatters on a strong band, | ||||||
| // so nearly every telemetry sample is an edge and one message repeats at the | ||||||
| // full telemetry cadence (see the members' comment in the header for the | ||||||
| // rate, and for why the historical figure there is not repeated as a | ||||||
| // current one). | ||||||
| // | ||||||
| // Deliberately OUTSIDE the edge test, and this is the whole reason the two | ||||||
| // are separate: a burst that stops must still report its tally. Flushing | ||||||
| // only on the next edge would hold the count until the band goes loud | ||||||
| // again, which could be hours away or never. publishTelemetry runs on every | ||||||
| // telemetry update, so the window closes on time whether or not the | ||||||
| // condition is still happening. | ||||||
| // | ||||||
| // 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. | ||||||
| if (m_adcOverloadEdges > 0 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocker: this introduces a deterministic timing/state policy without regression coverage. It does not require a fake Metis peer: extract the |
||||||
| && (!m_adcOverloadClock.isValid() | ||||||
| || m_adcOverloadClock.hasExpired(kAdcOverloadWarnIntervalMs))) { | ||||||
| // Why reading elapsed() here is safe: more than one transition implies | ||||||
| // the clock is valid. This flush is unconditional on every telemetry | ||||||
| // update and the counter rises by at most one per update, so the first | ||||||
| // transition after an invalid clock always flushes in the same call and | ||||||
| // resets the count. The count can only exceed one against a running | ||||||
| // window. | ||||||
| // | ||||||
| // What the single-transition branch does NOT mean. It is not "this is | ||||||
| // the first overload ever" — it is "exactly one transition was seen in | ||||||
| // this window". That lone transition may have arrived at any point since | ||||||
| // the window opened, so a bare message can lag the event by up to | ||||||
| // kAdcOverloadWarnIntervalMs. Accepted deliberately: it is the cost of | ||||||
| // the rate limit, one transition is a hint rather than an emergency, and | ||||||
| // an isolated overload after a quiet period still reports immediately | ||||||
| // because the clock is long expired by then. | ||||||
| if (m_adcOverloadEdges > 1) | ||||||
| // noquote + one composed string: streaming "(" as its own item makes | ||||||
| // QDebug insert a space after it and print "( 51 times in 10000 ms)". | ||||||
| qWarning().noquote() | ||||||
| << "Hl2Backend: ADC OVERLOAD — reduce LNA gain or attenuate" | ||||||
| << QStringLiteral("(%1 times in %2 ms)") | ||||||
| .arg(m_adcOverloadEdges) | ||||||
| .arg(m_adcOverloadClock.elapsed()); | ||||||
| else | ||||||
| qWarning() << "Hl2Backend: ADC OVERLOAD — reduce LNA gain or attenuate"; | ||||||
| m_adcOverloadClock.restart(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocker:
Suggested change
|
||||||
| m_adcOverloadEdges = 0; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -645,6 +645,27 @@ class Hl2Backend : public IRadioBackend { | |
| quint64 m_linkRxPacketsAtLastTick = 0; | ||
| static constexpr int kLinkStatsIntervalMs = 1000; | ||
| bool m_adcOverload = false; | ||
| // The overload bit is a per-frame sample of a level comparator, not an | ||
| // event: on a strong band it dithers, so the edge gate in publishTelemetry | ||
| // sees an edge nearly every time it looks. docs/HERMES.md 15.7 recorded | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is careful to mark the ~133/s figure as stale — but
The table's convention for a landed item is right above it at |
||
| // ~133 warnings/second on the MW broadcast band, flushing the log ring. | ||
| // | ||
| // That figure is stale and deliberately not repeated as a present-tense | ||
| // claim: MetisClient has since coalesced telemetryUpdated to 10 Hz (#4449), | ||
| // which caps this at ~10/s however hard the comparator chatters. What | ||
| // remains is one message repeating ten times a second for as long as the | ||
| // band stays strong — no longer ring-flushing, still enough to bury the | ||
| // lines around it over a session. | ||
| // | ||
| // So the edge gate stays and a rate limit sits behind it: warn on the first | ||
| // transition, then at most once per window, carrying the count of | ||
| // transitions the window swallowed. Note what that count is and is not — it | ||
| // counts the transitions SEEN, at the 10 Hz telemetry cadence, not | ||
| // comparator edges, which are sampled far below their true rate and always | ||
| // were. | ||
| QElapsedTimer m_adcOverloadClock; | ||
| int m_adcOverloadEdges = 0; | ||
| static constexpr qint64 kAdcOverloadWarnIntervalMs = 10000; | ||
| bool m_keyed = false; | ||
| bool m_tuning = false; | ||
| bool m_cwAutoKeyed = false; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit (non-blocking): this counts only false-to-true overload assertions, not both transitions. Consider
m_adcOverloadRises/m_adcOverloadAssertionsand matching prose so the eventualN timessummary has one precise meaning.