fix(hl2): an operator write ends the pin and records the band even when the value has not moved

[on8st]

Summary

Follow-up to #5402, which merged with one disclosed nit unfixed: in
Hl2Backend::setPanRfGain the equality early return

const int clamped = qBound(kLnaGainMinDb, gainDb, kLnaGainMaxDb);
if (clamped == m_lnaGainDb)
    return;

sits above both m_lnaSessionPin = false; and the m_lnaDbByBand.insert(...)
that follows it. So an operator who sets the gain to exactly the value already
live neither ends a session pin nor records their band choice — and the one
value guaranteed to be already live is the one a connect param pinned.

Concretely: 20 m stored at −12, connect with lnaGainDb=20, the operator still
on 20 m deliberately sets 20. The write is a no-op, the pin survives, and
currentOperatingState() keeps persisting −12 for a band the operator has just
told the radio they want at +20.

This is the follow-up I committed to in
#5402 (comment).
Nothing else in the merged fix changes.

The window is narrow and worth saying plainly. It is the start band only:
after the first band change applyPerBandStateFor clears the pin, and without a
pin bandMemoryWriteback returns the live value anyway. Reachability is also
narrow — lnaGainDb is a connect param the shipped UI does not populate, so a
pin needs an automation or embedder connect. And the failure direction is
conservative: an old calibration survives rather than dying. That is exactly why
@Ozy311 filed it as a non-blocking nit and why it should not have held #5402's
merge. It is still wrong, and it was uncovered as well as unfixed.

What changed, and why this shape

The early return was protecting one thing that is genuinely redundant on an
unchanged value: applyLnaGainDb(), which writes the AD9866 LNA register through
Metis, moves m_dbRef and echoes panRfGainChanged to every pan. That stays
guarded — an unchanged value is still not re-sent to the radio and still does not
re-echo.

Everything below it was never redundant. The pin clear and the band-map write
are consequences of the operator having chosen this value for this band, and an
operator who deliberately confirms the pinned value has chosen it just as much as
one who moved the slider. So the guard now wraps only the register write, and the
persistence half runs unconditionally.

notifyOperatingStateChanged() is guarded on moved || endedPin || recordedBand
so that a write which moves nothing, ends no pin and changes no stored entry does
not schedule a debounced store for a no-op. Any of the three actually changing
notifies exactly as before.

Constitution principle honored

Principle VIII — Evidence Over Assertion. The new assertions were run red
against the unmodified merge base before the production change and green after;
the exact commands and output are below. Not Principle XI: XI is the
squash-merge CI re-run and the maintainer's reproduction, which is not mine to
claim.

Test plan

Both directions were actually executed, in this worktree, in this order.

1. Test added first, built and run against unmodified origin/main
(10a566e3) — RED:

$ ninja -j 6 hl2_gain_restore_test      # test file only, production untouched
$ ./hl2_gain_restore_test
...
[ OK ] connect seeding creates a usable pan identity
[ OK ] same-value case starts pinned at +20 with 20m still stored as -12
[FAIL] an operator write of the pinned value itself records the band
[FAIL] the confirmed value survives a band round trip instead of reverting to -12
...
exit status 1 — 26 checks pass, 2 fail

2. Production change applied, incremental rebuild, same binary re-run — GREEN:

$ ninja -j 6 hl2_gain_restore_test      # rebuilds Hl2Backend.cpp.o and relinks
$ ./hl2_gain_restore_test
...
[ OK ] connect seeding creates a usable pan identity
[ OK ] same-value case starts pinned at +20 with 20m still stored as -12
[ OK ] an operator write of the pinned value itself records the band
[ OK ] the confirmed value survives a band round trip instead of reverting to -12
...
exit status 0 — 28 checks pass, 0 fail

The mutation is the one @Ozy311's #5402 review taught: the new assertion must
fail on the merged code, not merely agree with a re-typed copy of the rule.
That is also why the case is in tests/hl2_gain_restore_test.cpp, which drives
the real Hl2Backend, rather than in the policy-header test.

• Local build passes — for the hl2_gain_restore_test target and its
  aethercore dependency (ninja hl2_gain_restore_test, exit 0). A full
  cmake --build build was NOT run here and the rest of the suite was
  NOT run here; CI covers both.
• Behavior verified on a real radio — not by me, not yet. See below.
• Existing tests pass — the other 15 checks in hl2_gain_restore_test pass
  alongside the 3 new ones. Other test binaries were not built or run here.
• Reproduction steps documented — above.

What still wants hardware, and who has it

Separately from this diff: the half of #5402 that closes #5400 — the GUI startup
replay — has landed on socket-free tests and a demo backend. @Ozy311 wrote that
their fresh execution was limited to the standalone policy test and that they did
not rerun the expanded suite or the GUI startup path; @jensenpat wrote that the
bridge session was app-startup evidence, not HL2 hardware evidence, with no
physical radio connected. I filed #5400 from a real Hermes-Lite 2 and still have
it, so the operator here will confirm the merged behaviour on hardware across a
genuine restart and report it on #5402 whether or not it agrees with the tests.
That confirmation is not a precondition for this diff, which is a pure
persistence-ordering fix with a test that fails without it.

Checklist

• Commits are signed (SSH)
• No new flat-key AppSettings calls — no settings keys added
• Code is clean-room
• All meter UI uses MeterSmoother — no UI touched
• Documentation updated if user-visible behavior changed — none needed; this
  restores the behaviour the existing comment at the call site already
  describes ("This is also what ends a session pin")
• Security-sensitive changes reference a GHSA — n/a

Deliberately not widened

• connectLna()'s unclamped explicit-parameter branch (fix(hl2): preserve per-band gain across startup and overrides #5402 bot nit 4) is left
  alone — it was documented rather than deferred, and Hl2BandMemoryPolicy.h
  says so in its own comment.
• MainWindow::restoreBandState is untouched. It calls
  m_radioModel.setPanRfGain(snap.rfGain) without going through
  restoreLegacyRfGain, so wiring it up would reintroduce HL2: the restored global RF gain is written into per-band LNA memory, destroying the stored entry for whatever band is current #5400 exactly — but
  it has no callers anywhere in src or tests (only its declaration in
  MainWindow.h:969 and its definition in MainWindow.cpp:10614). Dead, not
  broken.
• No re-opening of the merged design, and no neighbouring refactor.

🤖 Generated with Claude Code

https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs

[aethersdr-agent]

1. Issue fit

Yes — for the thing it says it fixes. There is no Fixes #NNNN; the stated target is the nit you recorded yourself in #5402 (comment): "an operator who sets the gain to exactly the pinned value neither ends the pin nor records their choice… I will send a follow-up PR that makes an operator write end the pin and record the band whether or not the value moved, with that case added to the same test and failing before it passes. Nothing else in the merged fix changes."

I checked that promise item by item and it holds:

• The pin end (Hl2Backend.cpp:3064-3065) and the band record (:3067-3073) now sit below the equality check instead of behind it.
• The register write, which genuinely is redundant, still doesn't happen (:3055).
• The added test block would fail on the unfixed code, and I traced why rather than trusting the label: on main, clamped == m_lnaGainDb returns at the top, so 20m stays at -12 and bandGain(...) == 20 fails on the first of the three new checks. It is not an implementation echo.
• "Nothing else changes" survives too, which surprised me and is worth stating: the new if (moved || endedPin || recordedBand) gate at :3077 looks like added behaviour but is exactly the old one. Under main, a value-that-did-not-move returned before notifyOperatingStateChanged(); under this diff the same case (no pin, stored entry already equal) still doesn't notify. The gate preserves the old no-op path rather than adding a new one.

Bug fixes with a clear root cause do not need an RFC (GOVERNANCE.md), and this is a two-line-of-logic follow-up to a merged review nit.

2. Scope

File	What it changes	Claimed by the title?	Verdict
src/core/backends/hl2/Hl2Backend.cpp	setPanRfGain: split the equality check so only the register write is skipped; gate the notify on the three things that can actually change	Yes	In scope
tests/hl2_gain_restore_test.cpp	One added block: pinned session, operator writes the pinned value, band round trip	Yes	In scope

Everything in the diff is explained by the stated fix. No CHANGELOG.md entry (correct). No new public surface, no settings key, no protocol verb, no capability change. The only - lines are the two-line early return being restructured, and the guard it removed is re-expressed at :3055 rather than deleted. Two files, one commit, signed.

3. Blockers

None.

4. Nits (non-blocking)

• The sibling per-band memory in this same file guards the opposite way, for a bench-found reason. Hl2Backend::setTxPower (:3959-3960) computes operatorChange = !m_applyingBandMemory && clamped != m_rfPowerPercent, and its comment says why in as many words: "value-identical echoes (RadioModel's connect-time power push re-asserting what we just seeded) must neither claim the band nor set the baseline. Without the change-gate, the connect push at the model default bootstrapped defaultPercent=100 and rewrote the start band's stored drive on every reconnect (PR #4619 bench + review)." This PR asserts the exact opposite rule for gain — a value-identical write is an operator choice. I could not find a live gain equivalent of that power push (see below), so this is not a defect today; it is the two per-band memories in one file now disagreeing about what "operator" means, with #4619 as evidence that the disagreement has cost before. Worth a sentence in the code saying the gain path has no m_applyingBandMemory-style escape hatch, so whoever later adds a connect-time gain re-assert sees the trap. Inline at :3054.
• A knob spun into the rail is now "the operator choosing this value". MainWindow_Controllers.cpp:1590 (WheelRfGain) computes next = std::clamp(current + steps * step, low, high) and calls setPanRfGainFor unconditionally — its keyboard sibling at MainWindow_Shortcuts.cpp:752 guards with if (next == current) return;. So on a pinned start band, one extra detent past the top now ends the pin and overwrites the stored band gain with the pinned value. The operator asked for more, not for this — which is the one reading the comment at :3063 doesn't cover. Not in this diff, and adding the missing rail guard at the wheel call site is the cleaner fix, but the PR makes it observable.
• A pin-ending same-value write logs nothing. The qCInfo moved inside if (moved), so the case this PR exists for — band memory going -12 → 20, pin gone — leaves no line in a support bundle. Given how much of #5400/#5402 was reconstructed from what the app said about itself, that seems worth one line. Suggestion inline at :3055-3058.

5. What I tried to break

Findings below are reasoned from the head checkout at /tmp/aetherclaude/pr-5466; I have no build and ran nothing.

• Echo feedback loop. The obvious kill: applyLnaGainDb emits panRfGainChanged to every pan; if the GUI slider round-tripped that back, a pinned connect would immediately self-write the pinned value, end its own pin and destroy the band memory before the operator touched anything. It does not — SpectrumOverlayMenu::setRfGain wraps the slider in a QSignalBlocker and syncs m_lastEmittedRfGain, and setRfGainRange blocks too. The loop is closed on both counts.
• Every call site that could push a value-identical gain. All six reachable ones: the overlay slider (MainWindow_Wiring.cpp:5335, operator gesture), the keyboard step (guarded, above), the wheel/MIDI step (unguarded — the nit), the bridge (AutomationServer.cpp:10410, explicit automation intent), RadioCertification.cpp:505/508 (moves to a target then back to startGain, both real moves), and restoreLegacyRfGain in MainWindow_Session.cpp:1679 — which for HL2 has clientOwnsRfGain == true and does not write at all, asserted by writes == 0 in this very test file. So the #4619-shaped hazard has no live carrier today.
• MainWindow::restoreBandState (MainWindow.cpp:10647) pushes snap.rfGain unconditionally and would be a textbook non-operator write. Grepped src/ and tests/: no callers. Dead, as you and @jensenpat both already reported on #5402.
• Notify suppression losing a real change. Walked the persisted snapshot: currentOperatingState() writes the current band through bandMemoryWriteback(m_lnaGainDb, m_lnaSessionPin, hasStored, stored). In the only suppressed case — not moved, no pin, stored already equal — every input to that function is unchanged, so nothing is lost. The empty-m_currentBandKey case also persists no gain. I could not construct a state where the gate drops a store that mattered.
• recordedBand vs. the writeback policy. The new code compares against the raw m_lnaDbByBand entry, not the policy result, so I looked for a case where the raw map is equal but the persisted value would change: that needs a live pin, and endedPin covers exactly that. No gap found.
• Two things I could not check: whether the test passes (no build), and whether it ever runs in CI. It does not — I read every ctest -R filter in ci.yml and hl2_gain_restore_test is in none of them, so the five green checks on 2623afe7 compiled this test at most and never executed it. That is the frozen-gate design, not a fault of this PR, but the green badge is not evidence for the assertions here.

Socket-test disclosure: the PR modifies tests/hl2_gain_restore_test.cpp, an already-registered CTest target (tests.cmake:4145-4148, plus AETHER_SETTINGS_CONSUMERS). The added block adds no network behaviour — it reuses the existing GainSession, which the file documents as socket-free (boardMaxRx skips unicast discovery, no event loop is pumped, TEST-NET-1 192.0.2.1 is never reached). No fake peer, no new binding. Not run by CI, per above.

6. Recommendation

Approve with nits. The change does what it says, does only that, and the new test is a real negative control rather than a restatement of the code — I checked that it fails on main by tracing the early return, not by trusting the comment. The three nits are all about the blast radius of the new rule rather than the rule itself: the drive setter next door guards the same situation the opposite way for a reason that cost a bench session (#4619), and the wheel call site has no rail guard. Concrete next step: add the one-line "no m_applyingBandMemory equivalent here" note at :3054 and the pin-ending log line, and open a separate issue for the WheelRfGain rail guard rather than growing this PR.

---

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

[jensenpat]

Approve with nits — reviewed 2623afe.

Issue fit: this implements the explicitly disclosed same-value follow-up to #5402, within the accepted #5400 HL2 band-memory work. No unrelated changes, new protocol surface, defaults, architecture, or CHANGELOG entry.

File	Scope and conclusion
src/core/backends/hl2/Hl2Backend.cpp	Keeps redundant hardware/echo work guarded while ending the session pin and recording an explicit gain choice; notification is emitted only when live gain, pin, or stored entry changes. In scope.
tests/hl2_gain_restore_test.cpp	Exercises the production setter and snapshot with live +20/stored -12, then a band round trip. The old equality return leaves -12 and contradicts both new result assertions. In scope.

No blocking findings. Invalid pan rejection and clamping remain. Band restoration calls applyLnaGainDb directly, so it does not enter the operator setter. HL2 legacy startup replay is suppressed by restoreLegacyRfGain; slider status updates use QSignalBlocker. RadioModel dispatch keeps this change within HL2: Flex, Icom, ANAN, RTL and Sim implementations are untouched. The unused restoreBandState definition has no callers.

Disposition of the two existing bot threads: both are non-blocking and may be resolved without code changes. The internal-restore separation already exists in applyLnaGainDb/applyPerBandStateFor and the HL2 startup exclusion; an additional warning comment is optional. A same-value pin-ending log would improve diagnosis but is not required for persistence correctness. The wheel-at-rail observation is an explicit operator gesture reaching the setter and does not establish an unintended startup write.

Verification: complete two-file diff and relevant call paths inspected; git diff --check passed. Merge simulation against refreshed origin/main 7fe857a is clean. GitHub verifies the contributor commit signature. CI run 34062453346 succeeded on synthetic merge 11e6269 (2623afe into 10a566e), including Linux/macOS compilation of hl2_gain_restore_test; its frozen PR selections do not execute this regression. Static Checks run 34062453340 succeeded. The registered regression belongs to the full-suite main lane.

Behavioral evidence reused: the author's documented production-before/after run (two failing assertions before, 28 passing checks after), inspected against the actual test and implementation. I did not independently rerun that binary or its mutation. The helper skips discovery with boardMaxRx, does not pump the main event loop, and cancels completion before Metis start; no new socket peer or binding is introduced. Local builds/tests not run; no app launches or live-hardware validation. Hardware confirmation of the broader startup behavior remains separate.

Cost: no configure/build calls, local tests, app launches or agents; reused CI logs and reported regression evidence. Token/dollar measurements unavailable.