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)