HL2: setBandFilter() queues a redundant CONFIG one-shot that can put a stale sample rate on the wire

[ten9876]

Summary

MetisClient::setBandFilter() queues a copy of m_ccConfig into m_oneShot. That one-shot is redundant, and because it is a snapshot, it can put a stale config on the wire.

Found during review of #4503 (not a regression from it — the one-shot is added by that PR, but the effect is bounded and was not worth blocking on).

Why it is redundant

buildNextControlPacket() builds every EP2 frame as:

auto pkt = ep2Packet(m_txSeq++, withMox(m_ccConfig, keyed), withMox(b, keyed));

Bank A is live m_ccConfig on every single frame. The one-shot only ever populates bank B. So the stated purpose — "a band change moves the NCO and the filter in the same gesture, don't wait for the round robin" — is already satisfied by bank A without the push.

Why it can be stale

Bank B is applied after bank A, so when the two disagree the snapshot wins for that frame:

1. setBandFilter(X) pushes snapshot S = ccConfig(rate_old, numRx, X).
2. setSampleRate(rate_new) rebuilds m_ccConfig = ccConfig(rate_new, numRx, X).
3. Next frame: bank A = rate_new, bank B = S = rate_old. The radio ends on rate_old.

Bank A re-asserts the truth on the following frame, so the visible effect is one EP2 frame (~1 ms) of stale DDC rate. Small, but it is a real transient the code does not intend.

Additionally, m_oneShot is not cleared in start() (it is cleared nowhere). Hl2Backend::applyBandFilter() has no m_connected guard and m_metis outlives a disconnect, so a snapshot queued while disconnected can replay on the next session's first frames.

Suggested fix

Delete the m_oneShot.push_back(m_ccConfig) in setBandFilter(). Bank A already does the job, and removing the push eliminates the staleness with it.

If the push is kept for some reason not visible here, then m_oneShot.clear() belongs in start() alongside the other per-session resets (m_txSeq, m_roundRobin, m_haveRxSeq, m_drops, m_linkUp).

[aethersdr-agent]

Thanks for reporting this, @ten9876. To help us track it down, could you share a few more details?

• Radio model and firmware version
• AetherSDR version

If you can attach logs (Help → Support → File an Issue), that would be especially helpful.

— AetherClaude (automated agent for AetherSDR)

[aethersdr-agent]

Thanks for reporting this, @ten9876. To help us track it down, could you share a few more details?

• Radio model and firmware version
• AetherSDR version

If you can attach logs (Help → Support → File an Issue), that would be especially helpful.

— AetherClaude (automated agent for AetherSDR)

[on8st]

@ten9876 — taking the bot's questions first, so the awaiting-response label
has an answer on the record:

• Radio model and firmware version — not applicable. No radio is involved in
  this finding.
• AetherSDR version — not applicable in the sense the field means. This is a
  finding against the source tree rather than against a release. I re-verified it
  against main at 604127e7.
• Logs — there are none to attach. Nothing here is observed at runtime; the
  whole argument is in MetisClient.

I went through the report line by line against 604127e7. Every claim in it
still holds, six weeks and one merge later — #4503 landed as 2e54f17a a few
minutes after you filed, so the shape you describe is what is in main today.

Bank A is live m_ccConfig on every frame.
MetisClient::buildNextControlPacket still ends
ep2Packet(m_txSeq++, withMox(m_ccConfig, keyed), withMox(b, keyed)), and b is
the one-shot front or the round-robin slot. So the one-shot only ever fills
bank B, as reported.

The redundancy argument is a little stronger than the issue puts it. The
comment being removed justifies the push as getting ahead of "the round robin",
but the config register was never in the round robin at all — that rotation is
the receiver NCOs, then m_ccGain, then ccAdcAssign(). There was no rotation
to get ahead of, quite apart from bank A already carrying the value.

The stale frame reproduces. Driving MetisClient::buildNextControlPacket
directly, with no radio and no socket: after setBandFilter() then
setSampleRate(R192k), the built frame carries bank A rate code 2 (192 k) and
bank B rate code 0 (48 k) — two config banks in one frame, disagreeing, the
stale one second. One thing I want to flag rather than assert: which of the two
sub-frames the gateware applies last is not something I verified. That is read
off the HPSDR frame layout, not measured — I have no radio in this. It does not
change the conclusion, because after the fix no frame carries two config banks at
all, so there is nothing to win or lose.

m_oneShot is cleared nowhere — confirmed. MetisClient::start resets
m_txSeq, m_roundRobin, m_haveRxSeq, m_drops, m_linkUp and (added since
you filed) m_ioBoardTxFreqSent / m_ioBoardTxFreqHz, and not m_oneShot.
MetisClient::stop's std::erase_if removes only unfinished IO-board banks.
Hl2Backend::applyBandFilter guards on !m_metis || m_rx.empty() and not on
m_connected — also confirmed.

One thing that could not have caught this, in case it is useful.
tests/hl2_live_band_filter_probe.cpp carries what its own comment calls the
load-bearing assertion — that the EP6 packet rate does not change across a
filter write, because "a filter bit that leaked into the sample rate field would
halve or double this". That is exactly this failure class, but the probe's step
list only changes frequency and filter, never the sample rate, so the sequence
that produces the stale bank never occurs in it. (I have not run it — it needs
hardware.)

A fix is open as #5511. It is the deletion you suggested, nothing more:
the m_oneShot.push_back(m_ccConfig) removed, the two comments that asserted
the removed mechanism corrected rather than left stale (the block in
setBandFilter and the declaration's doc comment in MetisClient.h), and a
socket-free test built on hl2_tx_gate_test's pattern requiring that no EP2
frame carry two config banks. It fails three assertions before the change and
passes after; full ctest is 364/364 rebased on main at 52c1ced5. It says
Refs #4579 rather than Fixes, for the reason in the next paragraph — and
please close it unmerged without ceremony if you would rather take the
one-liner yourself.

What the deletion does not close, so it is not claimed to. It removes the
snapshot, and with it the band-filter half of the cross-session replay. It does
not make m_oneShot cleared: setRxFrequencyHz, setTxFrequencyHz and
setTxDriveLevel still push with no m_running guard — unlike
setIoBoardTxFrequencyHz, which has one — so a bank queued while disconnected
can still ride the next session's first frames. I did not add m_oneShot.clear()
to start() because it reads as in tension with stop()'s explicit "Preserve
unrelated one-shot setup; only this board's writes are stale", and that looked
like a call for you rather than something to fold into a one-line deletion.

Small process note, offered only as information: the bot's comment is posted
twice on this issue and awaiting-response was applied twice, both within a
second of each other. Same on #4581.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs

[aethersdr-agent]

Handing this to a maintainer — the analysis is settled and there is one open judgement call, not a missing fact.

Environment: N/A by design. Source-tree finding, no radio, no logs. Verified against main at 604127e7; still holds at 52c1ced5.

Confirmed against the tree:

• MetisClient.cpp:545 — m_oneShot.push_back(m_ccConfig) in setBandFilter().
• MetisClient.cpp:697-699 — the one-shot only ever fills bank B; bank A is live m_ccConfig on every EP2 frame, so the push is redundant. The config register is not in the round robin either (rotation is RX NCOs → m_ccGain → ccAdcAssign()), so the comment's "don't wait for the round robin" rationale has no rotation to beat.
• Stale frame reproduces without hardware: setBandFilter() then setSampleRate(R192k) yields one frame with bank A rate code 2 and bank B rate code 0. Which sub-frame the gateware applies last was read off the HPSDR layout, not measured — moot after the fix, since no frame then carries two config banks.
• m_oneShot is cleared nowhere. start() resets m_txSeq, m_roundRobin, m_haveRxSeq, m_drops, m_linkUp, m_ioBoardTxFreqSent/Hz. stop()'s std::erase_if (:364) drops only IO-board banks. Hl2Backend::applyBandFilter guards !m_metis || m_rx.empty(), not m_connected.

Suggested change: delete the one push at MetisClient.cpp:545 and correct the two comments that assert the removed mechanism (the block in setBandFilter, and the declaration doc at MetisClient.h:168). That is exactly what #5511 does, plus a socket-free test asserting no EP2 frame carries two config banks — fails 3 assertions before, passes after, 364/364 ctest.

The one maintainer call: whether m_oneShot.clear() also belongs in start(). The deletion closes the band-filter half of the cross-session replay only; setRxFrequencyHz (:400), setTxFrequencyHz (:620) and setTxDriveLevel (:636) still push with no m_running guard, so a bank queued while disconnected can ride the next session's first frames. It was left out of #5511 because a blanket clear reads as in tension with stop()'s explicit "Preserve unrelated one-shot setup; only this board's writes are stale" — that trade is a policy decision, not a detail to fold into a one-liner.

Not caught by existing tests, for the record: tests/hl2_live_band_filter_probe.cpp asserts the EP6 packet rate is stable across a filter write — the right failure class — but its step list never changes the sample rate, so the stale-bank sequence never occurs. (Needs hardware; not run.)

— AetherClaude (automated agent for AetherSDR)

---

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