-
-
Notifications
You must be signed in to change notification settings - Fork 118
fix(hl2): preserve per-band gain across startup and overrides #5402
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
922e9d5
fix(hl2): rate-limit the repeated ADC-overload warning
on8st 8e20af6
docs(hermes): mark 6a done and correct 15.7's stale severity
on8st 776a70d
fix(hl2): correct the flush comment, and stop QDebug spacing the paren
on8st 1b9b647
fix(hl2): a pinned connect gain must not consume the band's stored LNA
on8st b1f0576
fix(hl2): the snapshot path must preserve the entry too (#5402 review)
on8st e3d28da
fix(hl2): start() not restart() on the overload clock, behind a polic…
on8st 038a4b0
test(hl2): cover the real snapshot writer. Principle XI.
on8st 971cd05
Merge remote-tracking branch 'origin/main' into aether/pr5402-hl2-gain
jensenpat 7be1edd
fix(hl2): preserve per-band gain on startup. Principle V.
jensenpat dbbf86d
Merge current main into HL2 overload fix. Principle X.
Ozy311 079e169
Merge released memory-tab fix into overload branch. Principle X.
Ozy311 9722108
Merge main into HL2 gain restore. Principle X.
Ozy311 875f415
Merge current main for release validation. Principle X.
Ozy311 54c737e
Integrate reviewed HL2 overload fix. Principle X.
Ozy311 a71edf6
Merge decided overload squash into release branch. Principle X.
Ozy311 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| #pragma once | ||
|
|
||
| // Per-band LNA memory: which value a session comes up on, and which value the | ||
| // band memory records when the operator leaves that band. | ||
| // | ||
| // These are two separate questions and the backend previously answered only the | ||
| // first. The second is where the defect lives: a connect that pins the LNA via | ||
| // the namespaced lnaGainDb param diverges the live value from the start band's | ||
| // stored entry, and the FIRST band change then writes the live value back over | ||
| // that entry (Hl2Backend::rememberCurrentBandState). The operator's calibration | ||
| // for that band is gone, replaced by a number that was only ever meant to hold | ||
| // for one session. | ||
| // | ||
| // Symptom, and why it is worth a header: the loss is silent and it is delayed. | ||
| // Nothing is wrong at connect — the pinned value is what was asked for. The | ||
| // stored entry dies later, on an unrelated action, and the next session comes | ||
| // up on the pinned value as though the operator had chosen it. By the time a | ||
| // band sounds wrong there is nothing left on disk that says what it used to be. | ||
| // | ||
| // They live in a header, evaluated by Hl2Backend rather than copied into it, so | ||
| // the suite exercises the SAME expressions the backend runs — the reasoning | ||
| // Hl2TxLevelPolicy.h states, and the same reason it applies here: a test | ||
| // against a re-typed copy of this decision would agree with itself while the | ||
| // backend kept the bug. | ||
| // | ||
| // See Hl2Backend::connectRadio and ::applyPerBandStateFor for the surrounding | ||
| // ordering; this header is the decision only. | ||
|
|
||
| namespace AetherSDR::hl2 { | ||
|
|
||
| // A clamp local to this header so the decision is testable without pulling in | ||
| // the backend's translation unit. Mirrors qBound's argument order. | ||
| constexpr int clampDb(int minDb, int v, int maxDb) | ||
| { | ||
| return v < minDb ? minDb : (v > maxDb ? maxDb : v); | ||
| } | ||
|
|
||
| // What a session comes up on for the start band. | ||
| struct ConnectLna { | ||
| int liveDb = 0; | ||
| // TRUE when liveDb came from the connect param while the start band ALSO | ||
| // had a stored entry — i.e. the live value is a session pin that the | ||
| // operator never chose for this band. Purely informational to the caller; | ||
| // it is bandMemoryWriteback below that decides what it costs. | ||
| bool sessionPin = false; | ||
| }; | ||
|
|
||
| inline ConnectLna connectLna(bool haveRestoredState, | ||
| bool hasStoredEntry, int storedDb, | ||
| bool paramPresent, int paramDb, | ||
| int defaultDb, int minDb, int maxDb) | ||
| { | ||
| ConnectLna out; | ||
| // The explicit param still wins the LIVE value. That precedence is | ||
| // deliberate and documented at the call site: an automation or test caller | ||
| // pins the gain outright, and a stored entry must not silently ignore what | ||
| // the caller asked for. This header does not reverse it. | ||
| if (paramPresent) { | ||
|
Ozy311 marked this conversation as resolved.
|
||
| out.liveDb = paramDb; | ||
| out.sessionPin = haveRestoredState && hasStoredEntry && paramDb != storedDb; | ||
| return out; | ||
| } | ||
| if (haveRestoredState) { | ||
| out.liveDb = clampDb(minDb, hasStoredEntry ? storedDb : defaultDb, maxDb); | ||
| return out; | ||
| } | ||
| out.liveDb = defaultDb; | ||
| return out; | ||
| } | ||
|
|
||
| // What rememberCurrentBandState() should record for the band being left. | ||
| // | ||
| // Normally the live value: leaving a band records what the operator set while | ||
| // they were on it, which is the whole point of the memory. | ||
| // | ||
| // The exception is a session pin. That value came from the connect param, not | ||
| // from the operator acting on this band, and the band already had an entry of | ||
| // its own — so recording it would overwrite a calibration with a number nobody | ||
| // chose for this band. The stored entry is kept instead. | ||
| // | ||
| // Note what this deliberately does NOT do: it does not make the pin invisible. | ||
| // The live gain stays pinned, the radio runs at the requested value, and every | ||
| // pan is told about it. Only the persistence is refused, because persistence is | ||
| // the part that outlives the session that asked for it. | ||
| inline int bandMemoryWriteback(int liveDb, bool sessionPin, | ||
| bool hasStoredEntry, int storedDb) | ||
| { | ||
| if (sessionPin && hasStoredEntry) { | ||
| return storedDb; | ||
| } | ||
| return liveDb; | ||
| } | ||
|
|
||
| } // namespace AetherSDR::hl2 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| // Per-band LNA memory across a connect that pins the gain. | ||
| // | ||
| // The defect these cover is a SILENT, DELAYED loss of operator calibration. | ||
| // A connect carrying the namespaced lnaGainDb param sets the live gain to the | ||
| // pinned value while the start band's stored entry says something else; the | ||
| // first band change then calls rememberCurrentBandState(), which writes the | ||
| // live value back over that entry. Nothing is wrong at connect, nothing warns, | ||
| // and the band that used to be calibrated comes up on the pinned value in every | ||
| // later session as though the operator had chosen it. | ||
| // | ||
| // NO FIELD OBSERVATION IS CLAIMED FOR THIS MECHANISM. An earlier version of | ||
| // this comment cited a bench run in which 40 m went from -6 dB to -12 dB. That | ||
| // loss is real but it is NOT this defect: neither launch supplied a | ||
| // lnaGainDb connect param, so no session pin existed and this path never fired. | ||
| // The cause was a separate global RF-gain replay. Inference presented as | ||
| // observation, corrected in the PR body and left corrected here. (#5402 review.) | ||
| // | ||
| // The defect below is established by reading the path and by these assertions. | ||
| // | ||
| // Hl2Backend evaluates these same functions rather than its own copy, so what | ||
| // passes here is what the radio runs | ||
| // (core/backends/hl2/Hl2BandMemoryPolicy.h). | ||
|
|
||
| #include "core/backends/hl2/Hl2BandMemoryPolicy.h" | ||
|
|
||
| #include <cstdio> | ||
|
|
||
| using AetherSDR::hl2::bandMemoryWriteback; | ||
| using AetherSDR::hl2::connectLna; | ||
|
|
||
| namespace { | ||
|
|
||
| int g_failures = 0; | ||
|
|
||
| void check(bool ok, const char* what) | ||
| { | ||
| std::printf("%s %s\n", ok ? "[ OK ]" : "[FAIL]", what); | ||
| if (!ok) { | ||
| ++g_failures; | ||
| } | ||
| } | ||
|
|
||
| // This station's clamp, from Hl2Backend's kLnaGainMinDb/kLnaGainMaxDb. | ||
| constexpr int kMin = -12; | ||
| constexpr int kMax = 48; | ||
| constexpr int kDefault = 20; | ||
|
|
||
| } // namespace | ||
|
|
||
| int main() | ||
| { | ||
| // ---- A connect with no param takes the band's stored entry ------------- | ||
| // | ||
| // Already true before this header existed. Kept because it is the | ||
| // precondition for everything below: if a plain connect did NOT restore the | ||
| // stored entry, the writeback case would be unreachable and the defect | ||
| // would be somewhere else entirely. | ||
| { | ||
| const auto seed = connectLna(/*haveRestoredState=*/true, | ||
| /*hasStoredEntry=*/true, /*storedDb=*/-12, | ||
| /*paramPresent=*/false, /*paramDb=*/0, | ||
| kDefault, kMin, kMax); | ||
| check(seed.liveDb == -12, | ||
| "a plain connect comes up on the start band's stored entry"); | ||
| check(!seed.sessionPin, | ||
| "and nothing about that value is a session pin"); | ||
| } | ||
|
|
||
| // ---- A connect WITH the param pins the live value ---------------------- | ||
| // | ||
| // The param still wins, deliberately: it is how an automation or test | ||
| // caller pins the gain, and this fix does not reverse that precedence. | ||
| // What it does is mark the divergence, because the divergence is what the | ||
| // band memory must not swallow. | ||
| { | ||
| const auto seed = connectLna(/*haveRestoredState=*/true, | ||
| /*hasStoredEntry=*/true, /*storedDb=*/-12, | ||
| /*paramPresent=*/true, /*paramDb=*/20, | ||
| kDefault, kMin, kMax); | ||
| check(seed.liveDb == 20, | ||
| "an explicit lnaGainDb param still wins the live value"); | ||
| check(seed.sessionPin, | ||
| "and is marked a session pin, because the band stored -12"); | ||
| } | ||
|
|
||
| // ---- THE DEFECT: the first band change must not consume the entry ------ | ||
| { | ||
| const auto seed = connectLna(true, true, -12, true, 20, kDefault, kMin, kMax); | ||
| const int written = bandMemoryWriteback(seed.liveDb, seed.sessionPin, | ||
| /*hasStoredEntry=*/true, | ||
| /*storedDb=*/-12); | ||
| check(written == -12, | ||
| "leaving the start band after a pinned connect KEEPS the stored -12"); | ||
| } | ||
|
|
||
| // ---- A pin that agrees with the entry is not a pin --------------------- | ||
| // | ||
| // Pinning the value the band already had costs nothing and must not be | ||
| // treated as a divergence — otherwise the flag is set on ordinary | ||
| // automation connects and stops meaning anything. | ||
| { | ||
| const auto seed = connectLna(true, true, -12, true, -12, kDefault, kMin, kMax); | ||
| check(!seed.sessionPin, | ||
| "a param equal to the stored entry is not a session pin"); | ||
| check(bandMemoryWriteback(seed.liveDb, seed.sessionPin, true, -12) == -12, | ||
| "and records the same -12 either way"); | ||
| } | ||
|
|
||
| // ---- An operator value on a band with no entry is still recorded ------- | ||
| // | ||
| // The fix must not turn the memory off. A band the operator has never | ||
| // calibrated has nothing to protect, so the live value is what gets stored | ||
| // — including when it arrived as a connect param. | ||
| { | ||
| const auto seed = connectLna(true, /*hasStoredEntry=*/false, 0, | ||
| /*paramPresent=*/true, /*paramDb=*/6, | ||
| kDefault, kMin, kMax); | ||
| check(!seed.sessionPin, | ||
| "a param on an uncalibrated band is not a pin — nothing to lose"); | ||
| check(bandMemoryWriteback(seed.liveDb, seed.sessionPin, false, 0) == 6, | ||
| "and leaving that band records it, so the memory still works"); | ||
| } | ||
|
|
||
| // ---- Ordinary operation is untouched ---------------------------------- | ||
| { | ||
| // No pin at all: the operator moved the slider to +30 on a band that | ||
| // remembered -12. That is real intent and must overwrite. | ||
| check(bandMemoryWriteback(/*liveDb=*/30, /*sessionPin=*/false, | ||
| /*hasStoredEntry=*/true, /*storedDb=*/-12) == 30, | ||
| "without a pin, the live value overwrites the entry as before"); | ||
| } | ||
|
|
||
| // ---- A stored entry outside the clamp is bounded, not honoured --------- | ||
| { | ||
| const auto seed = connectLna(true, true, /*storedDb=*/900, | ||
| false, 0, kDefault, kMin, kMax); | ||
| check(seed.liveDb == kMax, | ||
| "a stored entry above the range clamps to the ceiling"); | ||
| } | ||
|
|
||
|
|
||
| // ---- THE SNAPSHOT PATH, which the write-back protection alone missed ---- | ||
| // | ||
| // Hl2Backend::currentOperatingState() builds the persisted band map, and it | ||
| // runs on a DEBOUNCED store that any unrelated action schedules -- a | ||
| // same-band tune, a mode change, a filter change. So it reaches the map long | ||
| // before the first band change, and protecting only rememberCurrentBandState() | ||
| // left the pin free to be persisted through it. (#5402 review, Ozy311.) | ||
| // | ||
| // Both call sites ask THIS function, so these cases cover the production | ||
|
Ozy311 marked this conversation as resolved.
Outdated
|
||
| // snapshot decision rather than a re-typed copy of it. | ||
| { | ||
| // The reviewer's exact scenario: 20 m stored at -12, connect pins 20, | ||
| // then a same-band tune triggers a capture. The capture must record -12. | ||
| const auto seed = connectLna(/*haveRestoredState=*/true, | ||
| /*hasStoredEntry=*/true, /*storedDb=*/-12, | ||
| /*paramPresent=*/true, /*paramDb=*/20, | ||
| kDefault, kMin, kMax); | ||
| check(seed.liveDb == 20 && seed.sessionPin, | ||
| "snapshot: the pin is live at 20 and marked"); | ||
| check(bandMemoryWriteback(seed.liveDb, seed.sessionPin, | ||
| /*hasStoredEntry=*/true, /*storedDb=*/-12) == -12, | ||
| "snapshot: a capture during a pinned session records the stored -12"); | ||
| } | ||
| { | ||
| // Without a pin the snapshot must still record the live value, or a | ||
| // capture would freeze the band memory against genuine operator changes. | ||
| check(bandMemoryWriteback(/*liveDb=*/30, /*sessionPin=*/false, | ||
| /*hasStoredEntry=*/true, /*storedDb=*/-12) == 30, | ||
| "snapshot: without a pin the capture records the live value"); | ||
| } | ||
| { | ||
| // A pinned session on a band with NO stored entry has nothing to | ||
| // protect, so the capture records the live value and the memory still | ||
| // learns the band. | ||
| check(bandMemoryWriteback(/*liveDb=*/6, /*sessionPin=*/false, | ||
| /*hasStoredEntry=*/false, /*storedDb=*/0) == 6, | ||
| "snapshot: an uncalibrated band still records through a capture"); | ||
| } | ||
|
|
||
| if (g_failures == 0) { | ||
| std::printf("\nALL PASS\n"); | ||
| return 0; | ||
| } | ||
| std::printf("\nFAILURES PRESENT\n"); | ||
| return 1; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.