From d97353a327f3c3b223e82034de19b6f7c6bc9a47 Mon Sep 17 00:00:00 2001 From: on8st <258096273+on8st@users.noreply.github.com> Date: Tue, 8 Sep 2026 22:18:45 +0200 Subject: [PATCH] fix(hl2): drop setBandFilter()'s stale CONFIG one-shot. Principle VIII. MetisClient::setBandFilter() pushed a COPY of m_ccConfig into m_oneShot. It was redundant and it could put a stale sample rate on the wire. Redundant: buildNextControlPacket() puts LIVE m_ccConfig in bank A of every EP2 frame, so the new relay pattern already reached the radio on the next frame (~2.6 ms at 48 kHz). The comment justified the push as getting "ahead of the rotation", but the config register was never in the round robin -- that rotation is the receiver NCOs, then gain, then the ADC assignment. Harmful: a one-shot only ever fills bank B, the radio applies bank B after bank A, and the queued copy is a SNAPSHOT. setBandFilter() followed by setSampleRate() therefore sent one frame carrying two disagreeing config banks, with the stale one applied last. Observed on the unfixed tree by the new test: bank A rate code 2 (192k), bank B rate code 0 (48k). The deletion also closes the cross-session half of the issue for this path. m_oneShot is cleared nowhere -- not in start() beside m_txSeq, m_roundRobin, m_haveRxSeq, m_drops and m_linkUp -- and Hl2Backend::applyBandFilter() has no m_connected guard while m_metis outlives a disconnect, so a snapshot queued while disconnected would have ridden the next session's first frames. Queuing nothing removes the snapshot rather than the deque. The two comments that asserted the removed mechanism are corrected rather than left stale, per this tree's convention: the block in setBandFilter() and the declaration's doc comment in MetisClient.h. New socket-free test hl2_band_filter_frame_test drives MetisClient's own packet builder -- public for exactly this purpose -- and requires that no EP2 frame carry two config banks. Red on the four assertions above before this change, green after. Refs: #4579 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs --- src/core/backends/hl2/MetisClient.cpp | 19 +++- src/core/backends/hl2/MetisClient.h | 14 ++- tests/hl2_band_filter_frame_test.cpp | 138 ++++++++++++++++++++++++++ tests/tests.cmake | 8 ++ 4 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 tests/hl2_band_filter_frame_test.cpp diff --git a/src/core/backends/hl2/MetisClient.cpp b/src/core/backends/hl2/MetisClient.cpp index d9f0a3464..2716a4b81 100644 --- a/src/core/backends/hl2/MetisClient.cpp +++ b/src/core/backends/hl2/MetisClient.cpp @@ -539,10 +539,21 @@ void MetisClient::setBandFilter(int ocFilterByte) return; // relays already where they belong m_params.ocFilterByte = oc; m_ccConfig = ccConfig(m_params.sampleRate, effectiveNumRx(), oc); - // Ahead of the rotation: a band change moves the NCO and the filter in the - // same gesture, and waiting for the round robin would leave the relays on - // the old band for up to three EP2 frames. - m_oneShot.push_back(m_ccConfig); + // NOTHING IS QUEUED HERE, and that is deliberate rather than an omission. + // + // buildNextControlPacket() puts LIVE m_ccConfig in bank A of EVERY EP2 + // frame, so the new relay pattern is on the wire on the next frame (~2.6 ms + // at 48 kHz) with no help. There is no rotation to get ahead of either: the + // round robin is the receiver NCOs, then gain, then the ADC assignment -- + // the config register was never in it. + // + // Queuing a COPY was also harmful. A one-shot only ever fills bank B, the + // radio applies bank B after bank A, and the copy is a SNAPSHOT -- so a band + // change followed by a sample-rate change sent one frame whose bank A held + // the new DDC rate and whose bank B held the old one, and the radio ended + // that frame on the old one. Bank A re-asserted the truth on the following + // frame, so it was ~1 ms of stale rate: small, real, and intended by + // nothing. (aethersdr/AetherSDR#4579) } void MetisClient::setIoBoardTxFrequencyHz(quint64 hz) diff --git a/src/core/backends/hl2/MetisClient.h b/src/core/backends/hl2/MetisClient.h index 1ca0dcfe3..09e1f3c30 100644 --- a/src/core/backends/hl2/MetisClient.h +++ b/src/core/backends/hl2/MetisClient.h @@ -151,10 +151,16 @@ class MetisClient : public QObject { Q_INVOKABLE void setLnaGainDb(int db); // Select the companion filter board's band filter (MetisProtocol kOc* bits). // - // Latched into the config register and sent on the next round, and ALSO - // pushed as a one-shot so the relays move with the band change rather than - // up to three EP2 frames later. Filter switching that lags the retune is - // the failure mode that matters on transmit. + // Latched into the config register, and that is the whole mechanism: the + // config bank rides bank A of EVERY EP2 frame, so the relays follow within + // one frame (~2.6 ms at 48 kHz). Filter switching that lags the retune is + // the failure mode that matters on transmit, and bank A is what prevents it. + // + // Deliberately NOT also queued as a one-shot. A one-shot fills bank B, which + // the radio applies AFTER bank A, and it would carry a SNAPSHOT -- so a + // copy queued here would overwrite the live config for that frame with + // whatever the register held at the moment the band changed. See the + // definition and aethersdr/AetherSDR#4579. // Takes int, not uint8_t: this crosses threads via // QMetaObject::invokeMethod, which matches Q_ARG against the type name moc // recorded from this declaration, and `std::uint8_t` does not normalize to diff --git a/tests/hl2_band_filter_frame_test.cpp b/tests/hl2_band_filter_frame_test.cpp new file mode 100644 index 000000000..763e822c0 --- /dev/null +++ b/tests/hl2_band_filter_frame_test.cpp @@ -0,0 +1,138 @@ +// A band change must not put a stale sample rate in the later of an EP2 frame's +// two C&C banks. (aethersdr/AetherSDR#4579) +// +// SOCKET-FREE ON PURPOSE. It drives MetisClient's own packet builder, which is +// public for exactly this reason ("Exists so the gate can be tested on the exact +// bytes that would go out"), rather than standing up a fake radio -- the +// fake-radio fixtures for this path are retired, and are kept in tests.cmake +// only as a bracket comment. +// +// WHAT IS UNDER TEST. buildNextControlPacket() puts LIVE m_ccConfig in bank A of +// every frame; a one-shot only ever fills bank B; and the radio applies bank B +// after bank A. So a COPY of m_ccConfig queued as a one-shot is both redundant +// -- bank A already carried the change -- and, because it is a snapshot, able to +// overwrite the live value for one frame when something else rebuilds the config +// register in between. +// +// WHAT IT DOES NOT TEST. That the radio really applies the second sub-frame last +// is a protocol fact read from the HPSDR frame layout, not something measured +// here: no radio is involved and nothing is keyed. The assertions below do not +// depend on it -- they require that no frame carry two config banks at all, so +// there is no second one to win or lose. + +#include "core/backends/hl2/MetisClient.h" +#include "core/backends/hl2/MetisProtocol.h" + +#include + +#include +#include + +using namespace AetherSDR::hl2; + +static int g_failures = 0; +static void check(bool ok, const char* what) +{ + if (!ok) { std::fprintf(stderr, "FAIL: %s\n", what); ++g_failures; } + else { std::fprintf(stderr, "[ OK ] %s\n", what); } +} + +using Ep2 = std::array; + +// C&C sits SYNC(3) into each 512-byte frame. Frame 0 is bank A, frame 1 bank B. +static const std::uint8_t* bank(const Ep2& pkt, int which) +{ + return pkt.data() + (which == 0 ? 8 : 8 + kFrameSize) + 3; +} +// MOX rides C0 bit 0 of every bank, so it is masked off before the address is read. +static bool isConfigBank(const std::uint8_t* cc) +{ + return static_cast(cc[0] & ~kC0MoxBit) == kC0Config; +} +static int rateCodeOf(const std::uint8_t* cc) { return cc[1] & 0x03; } +// Open-collector outputs are C2[7:1] -- the one-bit shift ccConfig() applies. +static std::uint8_t ocByteOf(const std::uint8_t* cc) +{ + return static_cast(cc[2] >> 1); +} + +int main(int argc, char** argv) +{ + QCoreApplication app(argc, argv); + + // ---- 1. bank A alone already carries the band change ---- + // + // This pins the PREMISE of the fix rather than the fix: it passes with the + // one-shot push present too. If it ever fails, deleting the push stops being + // safe, so it is the assertion that has to hold for the rest to be honest. + { + MetisClient c; + const Ep2 before = c.buildNextControlPacket(); + check(isConfigBank(bank(before, 0)), "bank A is the config register on every frame"); + check(ocByteOf(bank(before, 0)) == kOcNone, "no relay engaged before the band change"); + + c.setBandFilter(kOcLpf80); + const Ep2 after = c.buildNextControlPacket(); + check(isConfigBank(bank(after, 0)), "bank A is still the config register after the change"); + check(ocByteOf(bank(after, 0)) == kOcLpf80, + "the new relay pattern reaches the wire on the very next frame, from bank A alone"); + } + + // ---- 2. the defect: a band change then a rate change, no frame in between ---- + { + MetisClient c; // Params::sampleRate defaults to R48k + c.setBandFilter(kOcLpf80); // a snapshot taken here holds 48k + c.setSampleRate(SampleRate::R192k); // the live config register moves to 192k + + const Ep2 pkt = c.buildNextControlPacket(); + const std::uint8_t* a = bank(pkt, 0); + const std::uint8_t* b = bank(pkt, 1); + + check(isConfigBank(a) && rateCodeOf(a) == static_cast(SampleRate::R192k), + "bank A carries the live sample rate"); + check(!isConfigBank(b), + "bank B is not a second config bank, so nothing can overwrite bank A"); + if (isConfigBank(b)) { + std::fprintf(stderr, + " bank A rate code %d, bank B rate code %d" + " -- two config banks in one frame, and the radio ends on the later\n", + rateCodeOf(a), rateCodeOf(b)); + } + } + + // ---- 3. and none appears in the frames that follow ---- + { + MetisClient c; + c.setBandFilter(kOcLpf80); + c.setSampleRate(SampleRate::R192k); + bool sawSecondConfig = false; + for (int i = 0; i < 8; ++i) { + const Ep2 pkt = c.buildNextControlPacket(); + sawSecondConfig = sawSecondConfig || isConfigBank(bank(pkt, 1)); + } + check(!sawSecondConfig, "no frame in the next eight carries a second config bank"); + } + + // ---- 4. the cross-session half ---- + // + // m_oneShot is cleared nowhere -- not in start() alongside m_txSeq, + // m_roundRobin, m_haveRxSeq, m_drops and m_linkUp -- and stop() deliberately + // preserves everything that is not an unfinished IO-board write. So a bank + // queued by a band change while disconnected would ride the next session's + // first frames. Queuing nothing is what closes that here. + { + MetisClient c; + c.setBandFilter(kOcLpf80); + c.stop(); + bool survived = false; + for (int i = 0; i < 4; ++i) { + const Ep2 pkt = c.buildNextControlPacket(); + survived = survived || isConfigBank(bank(pkt, 1)); + } + check(!survived, "nothing a disconnected band change queued survives into the next session"); + } + + if (g_failures == 0) + std::fprintf(stderr, "hl2_band_filter_frame_test: all checks passed\n"); + return g_failures == 0 ? 0 : 1; +} diff --git a/tests/tests.cmake b/tests/tests.cmake index f2ce18bcf..3fae48a7c 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -3402,6 +3402,14 @@ target_include_directories(hl2_tx_gate_test PRIVATE src) target_link_libraries(hl2_tx_gate_test PRIVATE aethercore Qt6::Core Qt6::Network) add_test(NAME hl2_tx_gate_test COMMAND hl2_tx_gate_test) +# HL2 band filter / EP2 frame composition — socket-free, on MetisClient's own +# packet builder. A band change must not leave two disagreeing config banks in +# one frame (#4579). +add_executable(hl2_band_filter_frame_test tests/hl2_band_filter_frame_test.cpp) +target_include_directories(hl2_band_filter_frame_test PRIVATE src) +target_link_libraries(hl2_band_filter_frame_test PRIVATE aethercore Qt6::Core Qt6::Network) +add_test(NAME hl2_band_filter_frame_test COMMAND hl2_band_filter_frame_test) + add_executable(hl2_dbref_test tests/hl2_dbref_test.cpp) target_include_directories(hl2_dbref_test PRIVATE src) add_test(NAME hl2_dbref_test COMMAND hl2_dbref_test)