From 419752c59fee9a1d0724eaaf2352715c8d85e4fd Mon Sep 17 00:00:00 2001 From: Randal Warren Date: Mon, 31 Aug 2026 19:31:19 -0700 Subject: [PATCH 1/3] feat(hl2): drive the HL2 IO Board over I2C2 for band-following amp control The Hermes-Lite 2 IO Board (N2ADR/jimahlstrom) switches amplifiers, antenna relays and transverters from the transmit frequency. It is a plain I2C slave at chip 0x1D on the HL2's external bus, and the gateware tells it nothing -- the host is the only party that knows where the operator is tuned. Without these writes the board powers up and nothing follows the band, so an amplifier stays on whatever band the previous session left it. This adds the direct I2C-write path the backend did not have. It is separate from the J16 open-collector filter byte, which is INDIRECT: config bits that the gateware turns into an I2C write. This is a C&C bank naming the bus, the chip and the register itself. Wire format verified against four independent sources that agree: - gateware/rtl/i2c.v: cmd_addr is 6 bits and 6'h3d selects I2C2; the Versa and EEPROM init sequences build the same payload shape, icmd_data_upper = {8'h06, 1'b1, 7'h6a} -- cookie, stop, 7-bit chip - the HL2 wiki Protocol.md C&C table: C0[7]=RQST, C0[6:1]=ADDR, C0[0]=MOX, and one-byte writes only - deskHPSDR, a working client, which sends C0=0x7A C1=0x06 C2=0x80|0x1d - the board's own firmware (n2adr_lib/i2c_slave_handler.c), where REG_TX_FREQ_BYTE0 commits and the bytes are weighted <<0 .. <<32 Design decisions worth review: - WRITE-ONLY, RQST left clear. It is optional for a write, and a reply would reach Hl2Telemetry::apply(), which dispatches on RADDR without consulting the ACK flag. An I2C reply lands in its default: today, but a write that provokes no reply cannot perturb telemetry under a later edit to that switch. - Sent unconditionally, with no "do you have an IO board" setting, on the same reasoning the J16 filter byte is driven blind: an absent chip NACKs its address and the gateware's i2c_master raises missed_ack. A setting defaulted off fails as an amplifier on the wrong band. - Throttled to the board's documented 0.5 s, coalescing rather than dropping. A VFO wheel delivers ~10 tune events a second; discarding them would strand the amplifier on the old band whenever the operator stopped turning mid-cooldown, which is when they are most likely to key. - Sources the TRANSMIT receiver's frequency, not applyBandFilter()'s agree-or-bypass result. The board follows where the operator radiates; receive slices on other bands are irrelevant, and the bypass answer is a relay pattern with no frequency to offer. - Hooked above applyBandFilter()'s oc == m_ocFilterByte early return: the relay pattern is unchanged from 7.100 to 7.200 MHz but the board still needs the new frequency. Also pushed from the linkUp handler, because applyBandFilter() does not run on the connect path -- the connect-time filter byte is primed into MetisClient::Params instead. - MetisClient outlives a connect (built in Hl2Backend's constructor, freed in its destructor), so the dedupe is reset in start(). The board may have been power-cycled between sessions and nothing can be asked what it holds. Verified on live hardware: Hermes-Lite 2, gateware v7.5, board id 0x06, with an N2ADR filter board and an HL2 IO Board fitted. Amplifier PTT and band voltage both confirmed following band changes across 40m/20m/15m/10m, and the throttle observed coalescing a VFO sweep to one push per 500 ms while a band change still fired on the leading edge with no added latency. Unit tests cover the C0/C1/C2 encoding, 7-bit chip-address masking, the big-endian byte split, LSB-last ordering, a round trip through the firmware's own assembly expression, and that all 40 bits reach the wire. Co-Authored-By: Claude Opus 5 --- src/core/backends/hl2/Hl2Backend.cpp | 69 +++++++++++++++++++++ src/core/backends/hl2/Hl2Backend.h | 23 +++++++ src/core/backends/hl2/MetisClient.cpp | 29 +++++++++ src/core/backends/hl2/MetisClient.h | 27 +++++++++ src/core/backends/hl2/MetisProtocol.cpp | 30 ++++++++++ src/core/backends/hl2/MetisProtocol.h | 80 +++++++++++++++++++++++++ tests/hl2_metis_protocol_test.cpp | 49 +++++++++++++++ 7 files changed, 307 insertions(+) diff --git a/src/core/backends/hl2/Hl2Backend.cpp b/src/core/backends/hl2/Hl2Backend.cpp index cdda9284c..fee8530f4 100644 --- a/src/core/backends/hl2/Hl2Backend.cpp +++ b/src/core/backends/hl2/Hl2Backend.cpp @@ -465,6 +465,13 @@ Hl2Backend::Hl2Backend(QObject* parent) : IRadioBackend(parent) pushInitialState(); emitAllSliceState(); defineMeters(); + // Tell the IO board where we came up. applyBandFilter() is NOT called on + // this path — the connect-time filter byte is primed straight into + // MetisClient::Params instead — so without this the board would hold + // whatever the last session left it, and an amplifier would stay on that + // band until the operator's first retune. Placed after pushInitialState() + // so the receiver frequencies it reads are the restored ones. + applyIoBoardFrequency(); // At connect there is one receiver, so this is always "not wide" — but // it is published rather than assumed, so the indicator starts from a // stated value instead of whatever the widget happened to hold. @@ -4954,11 +4961,73 @@ double Hl2Backend::temperatureCelsius(int raw) return (3.26 * (static_cast(raw) / 4096.0) - 0.5) / 0.01; } +void Hl2Backend::applyIoBoardFrequency() +{ + if (!m_metis || m_rx.empty()) + return; + + // The TRANSMIT receiver's frequency — NOT the agree-or-bypass answer the + // filter board gets. The IO board switches amplifiers, antenna relays and + // transverters, all of which must follow where the operator will RADIATE. + // Receive slices parked on other bands are irrelevant to that, and the + // bypass result (kOcNone) is a relay pattern with no frequency to offer. + const Receiver* txRx = rx(m_txDdc); + const double hz = txRx ? txRx->sliceFreqHz : m_rx[0].sliceFreqHz; + if (!(hz > 0.0)) + return; // also rejects NaN, which a comparison to <= 0 would not + + // sliceFreqHz is TRUE-RF and the board's field wants true RF: it compares + // against band edges to pick a relay. The frequency-calibration scaling in + // ncoCommandHz() exists to correct the HL2's own reference and belongs only + // on values going to an NCO register — applying it here would hand the + // board a slightly wrong frequency for no reason. + const auto target = static_cast(hz + 0.5); + + if (!m_ioBoardThrottle) { + m_ioBoardThrottle = new QTimer(this); + m_ioBoardThrottle->setSingleShot(true); + m_ioBoardThrottle->setInterval(kIoBoardThrottleMs); + connect(m_ioBoardThrottle, &QTimer::timeout, this, [this] { + if (m_pendingIoBoardHz == 0) + return; // cooldown expired with nothing waiting + if (!m_connected) { + // Disconnected inside the cooldown. Dropping it matters because + // m_oneShot is NOT cleared by stop(): a bank queued now would + // sit there and go out as the first thing the NEXT session + // sends, briefly pointing an amplifier at the band this one + // ended on. linkUp() pushes the real frequency anyway. + m_pendingIoBoardHz = 0; + return; + } + const quint64 pending = m_pendingIoBoardHz; + m_pendingIoBoardHz = 0; + QMetaObject::invokeMethod(m_metis, "setIoBoardTxFrequencyHz", + Qt::QueuedConnection, Q_ARG(quint64, pending)); + // Re-arm: a tune still in progress must keep coalescing. + m_ioBoardThrottle->start(); + }); + } + + if (m_ioBoardThrottle->isActive()) { + m_pendingIoBoardHz = target; // superseded by any later request + return; + } + + QMetaObject::invokeMethod(m_metis, "setIoBoardTxFrequencyHz", + Qt::QueuedConnection, Q_ARG(quint64, target)); + m_ioBoardThrottle->start(); +} + void Hl2Backend::applyBandFilter(const char* reason) { if (!m_metis || m_rx.empty()) return; + // BEFORE the filter-byte comparison below, deliberately. The relay pattern + // is unchanged across a move from 7.100 to 7.200 MHz and this function + // returns early for it, but the IO board still needs the new frequency. + applyIoBoardFrequency(); + // ONE filter board, N receivers. // // The J16 open-collector byte is a radio-wide register: there is a single diff --git a/src/core/backends/hl2/Hl2Backend.h b/src/core/backends/hl2/Hl2Backend.h index e1446009c..01cda1958 100644 --- a/src/core/backends/hl2/Hl2Backend.h +++ b/src/core/backends/hl2/Hl2Backend.h @@ -143,6 +143,15 @@ class Hl2Backend : public IRadioBackend { // frequency. Idempotent and change-gated, so it is safe to call from every // path that can move the dial. void applyBandFilter(const char* reason); + // Push the transmit frequency to the HL2 IO Board, throttled. + // + // Called from applyBandFilter() so it inherits every trigger that can move + // a band — tune, TX slice, key/unkey, pan, add/close receiver — but from + // ABOVE that function's `oc == m_ocFilterByte` early return, because the + // two have different resolutions. The filter byte is one of seven relays + // and does not change between 7.100 and 7.200 MHz; the IO board wants the + // frequency itself and does. + void applyIoBoardFrequency(); // Per-band memory (RFC #4603 PR 3): apply the remembered LNA + drive for // the band containing freqHz (falling back to the restored defaults), // and record the operator's current values into the maps for the band @@ -588,6 +597,20 @@ class Hl2Backend : public IRadioBackend { QTimer* m_bandwidthThrottle = nullptr; double m_pendingBandwidthHz = 0.0; // 0 = nothing coalesced + // The IO board's README asks for at most one frequency update every 0.5 s, + // and only on change. Leading edge applies IMMEDIATELY, so an operator who + // changes band and keys straight away finds the amplifier already switched; + // anything arriving inside the cooldown is coalesced and the LAST value + // applied when it expires. + // + // Coalesce-and-apply, never drop: a VFO wheel delivers ~10 tune events a + // second, and simply discarding those inside the window would leave the + // amplifier on the old band whenever the operator stopped turning mid- + // cooldown — the one moment they are most likely to key. + static constexpr int kIoBoardThrottleMs = 500; + QTimer* m_ioBoardThrottle = nullptr; + quint64 m_pendingIoBoardHz = 0; // 0 = nothing coalesced + // Has this connect already derived the passband from the mode? (#4484) // // pushInitialState() runs on every linkUp, and MetisClient re-emits linkUp diff --git a/src/core/backends/hl2/MetisClient.cpp b/src/core/backends/hl2/MetisClient.cpp index 4e42672ac..1dfb87ffb 100644 --- a/src/core/backends/hl2/MetisClient.cpp +++ b/src/core/backends/hl2/MetisClient.cpp @@ -228,6 +228,14 @@ bool MetisClient::start(const Params& params) m_haveRxSeq = false; m_drops = 0; m_linkUp = false; + // This object OUTLIVES a connect: Hl2Backend builds it in its constructor + // and deletes it in its destructor, so without this the dedupe would carry + // a frequency across a disconnect and suppress the first push of the next + // session. The IO board may have been power-cycled in between, and nothing + // in the protocol can be asked what it currently holds -- the same reason + // the band filter is re-primed at every connect rather than trusted. + m_ioBoardTxFreqSent = false; + m_ioBoardTxFreqHz = 0; m_socket = new QUdpSocket(this); if (!m_socket->bind(QHostAddress::AnyIPv4, 0)) { @@ -530,6 +538,27 @@ void MetisClient::setBandFilter(int ocFilterByte) m_oneShot.push_back(m_ccConfig); } +void MetisClient::setIoBoardTxFrequencyHz(quint64 hz) +{ + if (m_ioBoardTxFreqSent && hz == m_ioBoardTxFreqHz) + return; // board already holds this frequency + m_ioBoardTxFreqSent = true; + m_ioBoardTxFreqHz = hz; + // INFO, not debug, and for the same reason the band filter is: there is no + // readback. The board never answers -- we deliberately do not set RQST -- + // so this line is the only record of what an amplifier was told to switch + // to, and a support log captured after a mis-keying has to already have it. + qCInfo(lcHl2).nospace() + << "HL2 IO board: TX frequency -> " + << QString::number(static_cast(hz) / 1.0e6, 'f', 6) + << " MHz (I2C2 chip 0x1D, 5 banks)"; + // Order is the batch's, not ours -- see ccIoBoardTxFrequency(). Appending + // in sequence is the whole contract: the deque preserves it, and the last + // bank is the one that makes the board latch. + for (const Cc& bank : ccIoBoardTxFrequency(hz)) + m_oneShot.push_back(bank); +} + void MetisClient::requestPipelineReset() { // DELIBERATELY A NO-OP. Do not re-enable without reading this. diff --git a/src/core/backends/hl2/MetisClient.h b/src/core/backends/hl2/MetisClient.h index 7d1d57007..78f8d2ba8 100644 --- a/src/core/backends/hl2/MetisClient.h +++ b/src/core/backends/hl2/MetisClient.h @@ -160,6 +160,26 @@ class MetisClient : public QObject { // recorded from this declaration, and `std::uint8_t` does not normalize to // the same string as `unsigned char`. Q_INVOKABLE void setBandFilter(int ocFilterByte); + // Push the TRANSMIT frequency to the HL2 IO Board (I2C2 chip 0x1D) so an + // attached amplifier, antenna relay or transverter follows the band. + // + // Five one-shot banks, LSB last, because that register is what commits the + // value on the board — see ccIoBoardTxFrequency(), which owns the ordering. + // They ride m_oneShot rather than the rotation for the same reason the + // filter byte does: an amplifier still switched to the previous band when + // the operator keys is the failure that matters, and the deque both + // preserves order and drains one bank per EP2 frame (~2.6 ms at 48 kHz), + // so all five land in about 13 ms. + // + // SENT UNCONDITIONALLY, with no "do you have an IO board" setting, on the + // same reasoning the J16 filter byte is driven blind: a chip that is not on + // the bus NACKs its address, the gateware's i2c_master raises missed_ack + // and moves on. Costing an absent board nothing is what makes the setting + // unnecessary, and a setting defaulted off is a support burden — the + // symptom of forgetting it is an amplifier on the wrong band. + // + // quint64, not std::uint32_t: the board's field is 40 bits wide. + Q_INVOKABLE void setIoBoardTxFrequencyHz(quint64 hz); [[nodiscard]] std::uint8_t bandFilter() const noexcept { return m_params.ocFilterByte; } // Queue a one-shot filter-pipeline reset (MetisProtocol kC0Sync) to be sent // on the next EP2 frame, ahead of the round robin. @@ -374,6 +394,13 @@ private slots: // radio in that order, and neither should wait up to three frames for the // rotation to come back around. std::deque m_oneShot; // which register pair to send next + // Last transmit frequency handed to the IO board, and whether one ever was. + // A separate flag rather than a 0 sentinel: 0 Hz is not a plausible tuned + // frequency, but "never sent" still has to survive a radio that legitimately + // reports it, and the first push after connect must go out even if the + // backend's throttle happens to compute the same value it had before. + quint64 m_ioBoardTxFreqHz = 0; + bool m_ioBoardTxFreqSent = false; std::uint32_t m_expectedRxSeq = 0; // for EP6 drop detection bool m_haveRxSeq = false; quint64 m_drops = 0; diff --git a/src/core/backends/hl2/MetisProtocol.cpp b/src/core/backends/hl2/MetisProtocol.cpp index 97227fd23..2648a9086 100644 --- a/src/core/backends/hl2/MetisProtocol.cpp +++ b/src/core/backends/hl2/MetisProtocol.cpp @@ -156,6 +156,36 @@ Cc ccTxDrive(int level, bool paEnable) noexcept return {kC0TxDrive, static_cast(level), c2, 0x00, 0x00}; } +Cc ccI2c2Write(std::uint8_t chip, std::uint8_t reg, std::uint8_t data) noexcept +{ + // The chip address is MASKED to 7 bits rather than asserted, because C2 + // bit 7 is the stop flag: an 8-bit I2C address passed by a caller who + // pre-shifted it would otherwise clear the stop bit and leave the bus + // held between transactions. + return {kC0I2c2, + kI2cCookieWrite, + static_cast(kI2cStopAtEnd | (chip & 0x7F)), + reg, + data}; +} + +std::array ccIoBoardTxFrequency(std::uint64_t hz) noexcept +{ + std::array out{}; + for (std::size_t i = 0; i < kIoBoardTxFreqBanks; ++i) { + const auto reg = static_cast(kIoBoardRegTxFreqMsb + i); + // Register 0 carries bits 39:32 and register 4 bits 7:0, so the shift + // counts DOWN as the register number counts up. Writing this as + // (8 * i) would invert the byte order and hand the board a frequency + // in the wrong endianness, which reads as a wildly wrong band rather + // than as a small error. + const unsigned shift = 8u * static_cast(kIoBoardRegTxFreqLsb - reg); + out[i] = ccI2c2Write(kIoBoardI2cAddr, reg, + static_cast((hz >> shift) & 0xFFu)); + } + return out; +} + void ep2WriteTxIq(std::array& pkt, std::span> iq) noexcept { diff --git a/src/core/backends/hl2/MetisProtocol.h b/src/core/backends/hl2/MetisProtocol.h index a6a7a6fdf..86f546e1d 100644 --- a/src/core/backends/hl2/MetisProtocol.h +++ b/src/core/backends/hl2/MetisProtocol.h @@ -327,6 +327,86 @@ Cc ccTxFreq(std::uint32_t hz) noexcept; // Defaulted OFF so that enabling the power amplifier is always something a // caller did on purpose. Cc ccTxDrive(int level, bool paEnable = false) noexcept; + +// ---- Direct I2C writes (companion devices on the external bus) ---- +// +// EASY TO CONFUSE WITH THE FILTER BOARD ABOVE, and the difference matters. The +// J16 open-collector byte is INDIRECT: we set config bits and the gateware +// turns them into an I2C write for us. This is the DIRECT path — a C&C bank +// that names the bus, the chip and the register itself. +// +// The HL2 exposes its two I2C buses as C&C addresses 0x3c (I2C1, internal: +// Versa clock, AD9866) and 0x3d (I2C2, the external companion-board bus). +// Verified against the gateware RTL, whose own init sequences build the +// identical payload shape (gateware/rtl/i2c.v): +// +// icmd_addr = 6'h3c; +// icmd_data_upper = {8'h06, 1'b1, 7'h6a}; // cookie, stop, chip address +// +// DATA layout, from that same RTL (cmd_data[31:16] is the upper half, and +// icmd_reg_val = cmd_data[15:0] is the register/value pair): +// +// C1 = DATA[31:24] cookie: 0x06 to write, 0x07 to read +// C2 = DATA[23] stop at end; DATA[22:16] the 7-bit chip address +// C3 = DATA[15:8] register or control number inside the chip +// C4 = DATA[7:0] the data byte (write only) +// +// The gateware emits {C3, C4} as a two-byte I2C write, which is what an +// ordinary register-then-value slave expects. ONE-BYTE WRITES ONLY — there is +// no burst mode, so an N-byte value costs N C&C banks. +// +// RQST (C0[7]) IS DELIBERATELY LEFT CLEAR. The wiki calls it optional for a +// write, and setting it makes the radio answer with an ACK response — which +// Hl2Telemetry::apply() dispatches on RADDR *without* consulting the ACK flag. +// Today an I2C reply (RADDR 0x3c/0x3d) lands harmlessly in its `default:`, but +// a write that provokes no reply at all cannot perturb the telemetry decoder +// under any future edit to that switch. This path stays write-only. +inline constexpr std::uint8_t kC0I2c1 = 0x78; // addr 0x3c << 1 +inline constexpr std::uint8_t kC0I2c2 = 0x7A; // addr 0x3d << 1 +inline constexpr std::uint8_t kI2cCookieWrite = 0x06; // C1 +inline constexpr std::uint8_t kI2cStopAtEnd = 0x80; // C2 bit 7 + +// ---- Hermes-Lite 2 IO Board (N2ADR), I2C2 chip 0x1D ---- +// +// A Raspberry Pi Pico that switches amplifiers, antenna relays and transverters +// from the TRANSMIT frequency. The gateware tells it nothing: it is a plain +// I2C slave, and the host is the only party that knows where the operator is +// tuned. Without these writes the board powers up and does nothing that +// follows the band. +// +// Registers 0..4 hold the transmit frequency in Hz, MOST significant byte +// first. Writing register 4 (the LSB) COMMITS the value — the firmware +// assembles all five from its own register file at that instant: +// +// case REG_TX_FREQ_BYTE0: +// new_tx_freq = (uint64_t)data +// | (uint64_t)Registers[REG_TX_FREQ_BYTE1] << 8 +// | ... | (uint64_t)Registers[REG_TX_FREQ_BYTE4] << 32; +// new_tx_fcode = hertz2fcode(new_tx_freq); +// +// (HL2IOBoard/n2adr_lib/i2c_slave_handler.c). Two consequences: the LSB must be +// sent LAST, and all five bytes must be sent even though the top one is always +// zero on HF — the board keeps the others in its register file, so an omitted +// byte silently contributes a stale value from the previous commit. +inline constexpr std::uint8_t kIoBoardI2cAddr = 0x1D; +inline constexpr std::uint8_t kIoBoardRegTxFreqMsb = 0; // DATA bits 39:32 +inline constexpr std::uint8_t kIoBoardRegTxFreqLsb = 4; // DATA bits 7:0, COMMITS + +// One single-byte I2C write on the external companion bus (I2C2, addr 0x3d). +// `chip` is the device's 7-bit address; the stop bit is always set, as the +// wiki advises for forward compatibility. +Cc ccI2c2Write(std::uint8_t chip, std::uint8_t reg, std::uint8_t data) noexcept; + +// The five C&C banks that write `hz` into the IO board's transmit-frequency +// registers, ALREADY IN THE ORDER THEY MUST BE SENT: most significant byte +// first, LSB last because that write is what commits the value. +// +// Returned as a batch rather than written one call at a time so the ordering +// constraint lives here, next to the firmware quotation that explains it, +// instead of in a loop at each call site that could be reversed by someone who +// reasonably assumed little-endian. +inline constexpr std::size_t kIoBoardTxFreqBanks = 5; +std::array ccIoBoardTxFrequency(std::uint64_t hz) noexcept; // Set MOX (C0 bit 0) on a C&C bank. Keying is per-FRAME, so this is applied to // whichever bank is being sent rather than to one dedicated register. inline Cc withMox(Cc cc, bool keyed) noexcept diff --git a/tests/hl2_metis_protocol_test.cpp b/tests/hl2_metis_protocol_test.cpp index aa08e880a..8a3da3c70 100644 --- a/tests/hl2_metis_protocol_test.cpp +++ b/tests/hl2_metis_protocol_test.cpp @@ -591,6 +591,55 @@ int main() "reverse above forward clamps to a very high SWR, not negative"); } + // ---- Direct I2C writes on the external bus (I2C2 / addr 0x3d) ---- + { + const Cc w = ccI2c2Write(0x1D, 4, 0xAB); + // C0 is the bus address SHIFTED LEFT ONE, like every other C0 constant: + // 0x3d << 1 == 0x7A. Bit 0 stays clear so withMox() owns keying, and + // bit 7 (RQST) stays clear so the radio sends no reply. + check(w[0] == 0x7A, "I2C2 write C0 is addr 0x3d << 1"); + check((w[0] & 0x01) == 0, "I2C2 write leaves MOX to withMox()"); + check((w[0] & 0x80) == 0, "I2C2 write does NOT set RQST (no reply wanted)"); + check(w[1] == 0x06, "I2C2 write cookie is 0x06"); + check(w[2] == 0x9D, "C2 is stop-bit | 7-bit chip address"); + check(w[3] == 4, "C3 is the register number"); + check(w[4] == 0xAB, "C4 is the data byte"); + + // A caller who passes an already-shifted 8-bit I2C address must not be + // able to clear the stop bit. + check(ccI2c2Write(0x9D, 0, 0)[2] == 0x9D, "chip address masked to 7 bits"); + } + + // ---- IO board transmit-frequency batch ---- + { + // 14.074 MHz = 0x00_00_D6_C0_90. Five bytes, MSB (always 0 on HF) first. + const auto banks = ccIoBoardTxFrequency(14'074'000ull); + check(banks.size() == 5, "five banks, one per frequency register"); + const std::uint8_t wantReg[5] = {0, 1, 2, 3, 4}; + const std::uint8_t wantData[5] = {0x00, 0x00, 0xD6, 0xC0, 0x90}; + for (std::size_t i = 0; i < 5; ++i) { + check(banks[i][3] == wantReg[i], "register order ascends 0..4"); + check(banks[i][4] == wantData[i], "big-endian byte split"); + check(banks[i][0] == 0x7A && banks[i][1] == 0x06 && banks[i][2] == 0x9D, + "every bank addresses the IO board on I2C2"); + } + // The LSB register COMMITS on the board, so it must be sent last. If + // this ever flips, the board latches a frequency built from four new + // bytes and one stale one. + check(banks[4][3] == 4, "LSB register is written LAST (it commits)"); + + // Reassembling the way the Pico firmware does must return the input. + std::uint64_t rebuilt = 0; + for (std::size_t i = 0; i < 5; ++i) + rebuilt = (rebuilt << 8) | banks[i][4]; + check(rebuilt == 14'074'000ull, "round-trips through the firmware's assembly"); + + // Top byte is real: a value above 32 bits must not be truncated. + const auto high = ccIoBoardTxFrequency(0x11'22'33'44'55ull); + check(high[0][4] == 0x11 && high[4][4] == 0x55, + "all 40 bits reach the wire"); + } + if (g_failures == 0) std::fprintf(stderr, "hl2_metis_protocol_test: all checks passed\n"); return g_failures == 0 ? 0 : 1; From 93e730716cf43080bd4d6b7f90b23bb997f839fc Mon Sep 17 00:00:00 2001 From: Randal Warren Date: Tue, 1 Sep 2026 17:05:35 -0700 Subject: [PATCH 2/3] =?UTF-8?q?fix(hl2):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?IO=20board=20scheduling=20guards=20and=20TX=20interlock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four blockers were in the scheduling wrapper, not the encoder, which is where the review said to look. Rather than patch each condition at its call site, the decision now lives in one pure policy the suite can exercise without a radio (Hl2IoBoardPolicy.h) — the defects were conditions, and conditions are only testable if a test can reach them. Blocker 1 — leading edge lacked the !m_connected guard the trailing edge had. Both edges now go through sendIoBoardFrequency(), so the asymmetry cannot return: two call sites that must agree about a hardware safety condition was one call site too many. MetisClient::setIoBoardTxFrequencyHz additionally refuses to queue while !m_running, as defence in depth at the wire itself. Blocker 2 — a band change mid-sweep no longer waits for the cooldown. The throttle exists for VFO sweeps (~10 events/second against a board that asks for two); a band crossing is not that case, because applyBandFilter moves the physical filter relay at once and deferring the amplifier leaves the two disagreeing for up to 500 ms. Frequency tracking is rate-limited; which band the amplifier is on is not. Tracked with m_ioBoardBandKey, keyed off the same bandKeyForHz() table the per-band memory uses. Blocker 3 — resetIoBoardSchedule() drops the armed timer, the coalesced value and the remembered band on linkDown and on connectFailed. Left running, a reconnect inside the residual window stored the connect-time frequency as pending instead of pushing it, delaying the board exactly when linkUp() intends immediacy. Clearing the band key also makes the first push of any session count as a band change, so it takes the leading edge. Maintainer question — never write while keyed: ruled by the operator, whose amplifier this is. Switching a band relay under RF burns the contacts, and this write is what moves it. The window is narrow on this radio but not closed (connect-time pushes and the automation bridge can both reach the scheduler with MOX up), and the cost of being wrong is damaged hardware, so the guard is unconditional rather than reasoned about per caller. Deferring costs nothing: unkey calls applyBandFilter("unkey"), which re-runs the path and recomputes from the TX receiver — fresher than any stashed value. Nit — static_assert ties kIoBoardTxFreqBanks to the Msb..Lsb register span. Raising the bank count alone would underflow the unsigned shift arithmetic and put a garbage byte on a wire that drives a band relay; that edit now fails to compile. Nit not taken — the shared coalesceThrottle() refactor with m_bandwidthThrottle. The two now differ in ways that are load-bearing here (the band-change override and the keyed interlock have no bandwidth analogue), so folding them together would either genericise those away or push HL2 transmit semantics into a display-rate helper. Happy to revisit if you would rather have the shared shape. hl2_io_board_policy_test covers all sixteen condition combinations, with the reviewed regression — disconnected, idle throttle, band changed, i.e. the leading edge — pinned by name. Co-Authored-By: Claude Opus 5 --- src/core/backends/hl2/Hl2Backend.cpp | 86 +++++++++++++++--- src/core/backends/hl2/Hl2Backend.h | 18 ++++ src/core/backends/hl2/Hl2IoBoardPolicy.h | 67 ++++++++++++++ src/core/backends/hl2/MetisClient.cpp | 8 ++ src/core/backends/hl2/MetisProtocol.h | 10 +++ tests/hl2_io_board_policy_test.cpp | 107 +++++++++++++++++++++++ tests/tests.cmake | 7 ++ 7 files changed, 289 insertions(+), 14 deletions(-) create mode 100644 src/core/backends/hl2/Hl2IoBoardPolicy.h create mode 100644 tests/hl2_io_board_policy_test.cpp diff --git a/src/core/backends/hl2/Hl2Backend.cpp b/src/core/backends/hl2/Hl2Backend.cpp index fee8530f4..f2f1b8c90 100644 --- a/src/core/backends/hl2/Hl2Backend.cpp +++ b/src/core/backends/hl2/Hl2Backend.cpp @@ -481,6 +481,7 @@ Hl2Backend::Hl2Backend(QObject* parent) : IRadioBackend(parent) if (m_connected) { m_connected = false; m_linkStatsTimer->stop(); + resetIoBoardSchedule(); emit disconnected(); } }); @@ -496,6 +497,7 @@ Hl2Backend::Hl2Backend(QObject* parent) : IRadioBackend(parent) QMetaObject::invokeMethod(m_metis, "stop", Qt::QueuedConnection); m_connected = false; m_linkStatsTimer->stop(); + resetIoBoardSchedule(); emit connectionError(QStringLiteral("Hermes-Lite 2: %1").arg(reason)); }); @@ -4983,6 +4985,11 @@ void Hl2Backend::applyIoBoardFrequency() // board a slightly wrong frequency for no reason. const auto target = static_cast(hz + 0.5); + // The band, from the same bandKeyForHz() table the per-band memory uses, so + // "which band is this" has exactly one answer in this backend. + const QString targetBand = bandKeyForHz(hz); + const bool bandChanged = (targetBand != m_ioBoardBandKey); + if (!m_ioBoardThrottle) { m_ioBoardThrottle = new QTimer(this); m_ioBoardThrottle->setSingleShot(true); @@ -4990,34 +4997,85 @@ void Hl2Backend::applyIoBoardFrequency() connect(m_ioBoardThrottle, &QTimer::timeout, this, [this] { if (m_pendingIoBoardHz == 0) return; // cooldown expired with nothing waiting - if (!m_connected) { - // Disconnected inside the cooldown. Dropping it matters because - // m_oneShot is NOT cleared by stop(): a bank queued now would - // sit there and go out as the first thing the NEXT session - // sends, briefly pointing an amplifier at the band this one - // ended on. linkUp() pushes the real frequency anyway. - m_pendingIoBoardHz = 0; - return; - } const quint64 pending = m_pendingIoBoardHz; m_pendingIoBoardHz = 0; - QMetaObject::invokeMethod(m_metis, "setIoBoardTxFrequencyHz", - Qt::QueuedConnection, Q_ARG(quint64, pending)); + if (!sendIoBoardFrequency(pending)) + return; // disconnected: nothing to re-arm for // Re-arm: a tune still in progress must keep coalescing. m_ioBoardThrottle->start(); }); } - if (m_ioBoardThrottle->isActive()) { + // The whole decision, in one place, from a policy the suite can exercise + // without a radio — see Hl2IoBoardPolicy.h for why each condition sits + // where it does. m_tuning counts as keyed: TUNE radiates. + switch (ioBoardAction(m_connected, m_keyed || m_tuning, + m_ioBoardThrottle->isActive(), bandChanged)) { + case IoBoardAction::DropDisconnected: + m_pendingIoBoardHz = 0; + return; + case IoBoardAction::DeferKeyed: + // Deliberately not stashed: unkey re-runs this path and recomputes. + return; + case IoBoardAction::Coalesce: m_pendingIoBoardHz = target; // superseded by any later request return; + case IoBoardAction::Send: + break; } - QMetaObject::invokeMethod(m_metis, "setIoBoardTxFrequencyHz", - Qt::QueuedConnection, Q_ARG(quint64, target)); + if (!sendIoBoardFrequency(target)) + return; + m_ioBoardBandKey = targetBand; + // Restarted rather than left running, so the cooldown is measured from the + // push that actually went out — a band change mid-sweep resets the window + // instead of inheriting the remainder of the previous one. m_ioBoardThrottle->start(); } +bool Hl2Backend::sendIoBoardFrequency(quint64 hz) +{ + // THE ONE PLACE either edge of the throttle reaches the wire. + // + // It exists because the guard below was originally written into the + // trailing edge only, and the leading edge — the commoner path — silently + // lacked it. Two call sites that must agree about a hardware safety + // condition is one call site too many, so both now go through here and the + // asymmetry cannot come back. + // + // WHY DISCONNECTED MATTERS: MetisClient::m_oneShot is not cleared by + // start() or stop(), and unlike the filter byte the IO-board frequency is + // not re-primed through Params. A bank queued while down therefore survives + // as the FIRST thing the next session transmits, briefly pointing an + // amplifier at the band this session ended on. linkUp() pushes the real + // frequency immediately afterwards, so dropping it here loses nothing. + if (!m_connected) { + m_pendingIoBoardHz = 0; + return false; + } + QMetaObject::invokeMethod(m_metis, "setIoBoardTxFrequencyHz", + Qt::QueuedConnection, Q_ARG(quint64, hz)); + return true; +} + +void Hl2Backend::resetIoBoardSchedule() +{ + // Called on linkDown. The timer's armed/pending state is about a session: + // left running across a disconnect, a reconnect inside the residual window + // takes the coalescing branch and stores the connect-time frequency as + // PENDING instead of pushing it — delaying the board by up to the cooldown + // at exactly the moment linkUp() intends an immediate push. + // + // The band key is cleared too, so the first push of the next session is + // always treated as a band change and takes the leading edge. Assuming the + // previous session's band still applies is precisely the assumption that + // cannot be made across a disconnect. + if (m_ioBoardThrottle) + m_ioBoardThrottle->stop(); + m_pendingIoBoardHz = 0; + m_ioBoardBandKey.clear(); +} + void Hl2Backend::applyBandFilter(const char* reason) { if (!m_metis || m_rx.empty()) diff --git a/src/core/backends/hl2/Hl2Backend.h b/src/core/backends/hl2/Hl2Backend.h index 01cda1958..bfae01eaf 100644 --- a/src/core/backends/hl2/Hl2Backend.h +++ b/src/core/backends/hl2/Hl2Backend.h @@ -9,6 +9,7 @@ #include #include "core/backends/hl2/Hl2DbReference.h" +#include "core/backends/hl2/Hl2IoBoardPolicy.h" #include "core/backends/hl2/Hl2Receivers.h" #include "core/backends/hl2/MetisProtocol.h" // Hl2Telemetry @@ -152,6 +153,13 @@ class Hl2Backend : public IRadioBackend { // and does not change between 7.100 and 7.200 MHz; the IO board wants the // frequency itself and does. void applyIoBoardFrequency(); + // The single point at which either edge of the IO-board throttle reaches + // the wire, so the disconnected guard cannot be present on one path and + // missing on the other. Returns false when the push was refused. + [[nodiscard]] bool sendIoBoardFrequency(quint64 hz); + // Drop the IO-board schedule on linkDown: armed timer, coalesced value and + // remembered band all describe a session and must not survive one. + void resetIoBoardSchedule(); // Per-band memory (RFC #4603 PR 3): apply the remembered LNA + drive for // the band containing freqHz (falling back to the restored defaults), // and record the operator's current values into the maps for the band @@ -610,6 +618,16 @@ class Hl2Backend : public IRadioBackend { static constexpr int kIoBoardThrottleMs = 500; QTimer* m_ioBoardThrottle = nullptr; quint64 m_pendingIoBoardHz = 0; // 0 = nothing coalesced + // The band the IO board was last told about, as a bandKeyForHz() key. + // Empty means "no session has told it anything", which is also the state + // reset() restores — so the first push after any connect is treated as a + // band change and takes the leading edge rather than being coalesced. + // + // Tracked SEPARATELY from m_currentBandKey (the per-band memory's notion): + // that one follows the operator's tuning for LNA/drive recall and moves on + // paths this does not, and conflating "what the operator is on" with "what + // the amplifier has been told" is how the two silently diverge. + QString m_ioBoardBandKey; // Has this connect already derived the passband from the mode? (#4484) // diff --git a/src/core/backends/hl2/Hl2IoBoardPolicy.h b/src/core/backends/hl2/Hl2IoBoardPolicy.h new file mode 100644 index 000000000..532c3f5aa --- /dev/null +++ b/src/core/backends/hl2/Hl2IoBoardPolicy.h @@ -0,0 +1,67 @@ +#pragma once + +// When an IO-board frequency push may go out, as a pure decision. +// +// WHY THIS IS A POLICY AND NOT AN `if` CHAIN IN THE BACKEND. The encoder that +// builds the five I2C banks is straightforward and was right first time; every +// defect in this feature has been in the SCHEDULING around it — a guard present +// on the trailing edge of a throttle and missing on the leading one, an armed +// timer surviving a disconnect, a band change coalesced as though it were +// ordinary frequency drift. Those are conditions, not arithmetic, and they are +// only testable if they live somewhere a test can reach without a radio. +// +// The thing being scheduled points an amplifier's band relay. That is the +// reason the ordering below is fixed and commented rather than left to whoever +// next edits the call site. + +namespace AetherSDR::hl2 { + +enum class IoBoardAction { + // Push now. The caller sends, records the band, and restarts the cooldown. + Send, + // Same-band movement inside the cooldown: remember it as pending and let + // the timer deliver the latest value when the window expires. + Coalesce, + // Transmitting. Do nothing at all, and do NOT stash the value: unkey runs + // the whole path again and recomputes from the TX receiver, which is + // fresher than anything held here. + DeferKeyed, + // Not connected. Do nothing AND discard any pending value. + DropDisconnected, +}; + +// The decision, in the order the conditions must be tested. +// +// DISCONNECTED FIRST, because the consequence outlives the session: +// MetisClient::m_oneShot is cleared by neither start() nor stop(), and the +// IO-board frequency — unlike the filter byte — is not re-primed through +// Params. A bank queued while the link is down therefore becomes the FIRST +// thing the next connect transmits, pointing an amplifier at the band the +// previous session ended on. +// +// KEYED SECOND, because switching a band relay under RF burns its contacts. +// The operator normally cannot retune mid-transmission on this radio, but +// connect-time pushes and the automation bridge can both reach the scheduler +// while MOX is up, and the cost of being wrong is someone's hardware. +// +// BAND CHANGE BEATS THE THROTTLE, because the rate limit exists for VFO sweeps +// (~10 events/second against a board that asks for two) and a band crossing is +// not that case: applyBandFilter moves the physical filter relay immediately, +// so deferring the amplifier leaves the two disagreeing for up to the cooldown +// — and keying in that window is exactly the hazard. Rate-limit frequency +// tracking; never rate-limit which band the amplifier is on. +[[nodiscard]] constexpr IoBoardAction ioBoardAction(bool connected, + bool keyed, + bool throttleActive, + bool bandChanged) noexcept +{ + if (!connected) + return IoBoardAction::DropDisconnected; + if (keyed) + return IoBoardAction::DeferKeyed; + if (throttleActive && !bandChanged) + return IoBoardAction::Coalesce; + return IoBoardAction::Send; +} + +} // namespace AetherSDR::hl2 diff --git a/src/core/backends/hl2/MetisClient.cpp b/src/core/backends/hl2/MetisClient.cpp index 1dfb87ffb..5b02f3e70 100644 --- a/src/core/backends/hl2/MetisClient.cpp +++ b/src/core/backends/hl2/MetisClient.cpp @@ -540,6 +540,14 @@ void MetisClient::setBandFilter(int ocFilterByte) void MetisClient::setIoBoardTxFrequencyHz(quint64 hz) { + // DEFENCE IN DEPTH against a bank outliving its session. m_oneShot is not + // cleared by start() or stop(), so anything queued while the stream is down + // becomes the first thing the NEXT connect transmits -- pointing an + // amplifier at the band the previous session ended on. Hl2Backend already + // refuses to schedule while disconnected; this is the wire's own refusal, + // so a future caller that misses that guard cannot reintroduce the hazard. + if (!m_running) + return; if (m_ioBoardTxFreqSent && hz == m_ioBoardTxFreqHz) return; // board already holds this frequency m_ioBoardTxFreqSent = true; diff --git a/src/core/backends/hl2/MetisProtocol.h b/src/core/backends/hl2/MetisProtocol.h index 86f546e1d..0bfcafeaa 100644 --- a/src/core/backends/hl2/MetisProtocol.h +++ b/src/core/backends/hl2/MetisProtocol.h @@ -406,6 +406,16 @@ Cc ccI2c2Write(std::uint8_t chip, std::uint8_t reg, std::uint8_t data) noexcept; // instead of in a loop at each call site that could be reversed by someone who // reasonably assumed little-endian. inline constexpr std::size_t kIoBoardTxFreqBanks = 5; +// These three constants are NOT independent: one loop below indexes registers +// with a shift of 8 * (kIoBoardRegTxFreqLsb - reg). Raise the bank count +// without moving the LSB register and the last iteration subtracts past zero in +// unsigned arithmetic — an 8 * 255 shift, undefined, putting a garbage byte on +// a wire that moves an amplifier's band relay. Tie them together so that edit +// fails to compile rather than reaching hardware. +static_assert(kIoBoardTxFreqBanks + == static_cast(kIoBoardRegTxFreqLsb + - kIoBoardRegTxFreqMsb + 1), + "IO board frequency bank count must span Msb..Lsb exactly"); std::array ccIoBoardTxFrequency(std::uint64_t hz) noexcept; // Set MOX (C0 bit 0) on a C&C bank. Keying is per-FRAME, so this is applied to // whichever bank is being sent rather than to one dedicated register. diff --git a/tests/hl2_io_board_policy_test.cpp b/tests/hl2_io_board_policy_test.cpp new file mode 100644 index 000000000..36912b364 --- /dev/null +++ b/tests/hl2_io_board_policy_test.cpp @@ -0,0 +1,107 @@ +// HL2 IO-board push scheduling policy. Pure — no Qt, no aethercore, no radio. +// +// Every defect found in this feature during review was in the scheduling, not +// the encoder: a guard present on one edge of the throttle and absent on the +// other, an armed timer surviving a disconnect, a band change coalesced as +// though it were ordinary frequency drift. These checks pin the conditions so +// those cannot come back silently. + +#include "core/backends/hl2/Hl2IoBoardPolicy.h" + +#include + +using namespace AetherSDR::hl2; + +static int g_failures = 0; +static void check(bool cond, const char* what) +{ + if (!cond) { + std::fprintf(stderr, "FAIL: %s\n", what); + ++g_failures; + } +} + +// Named arguments at the call site: ioBoardAction(connected, keyed, +// throttleActive, bandChanged) is four bools, and a transposed pair would +// otherwise still compile and still look right. +static IoBoardAction act(bool connected, bool keyed, bool throttled, bool bandChanged) +{ + return ioBoardAction(connected, keyed, throttled, bandChanged); +} + +int main() +{ + // ---- disconnected wins over everything ---- + { + // The consequence outlives the session: a bank queued while down is not + // discarded by start()/stop() and becomes the FIRST thing the next + // connect sends, pointing an amplifier at the previous band. + check(act(false, false, false, false) == IoBoardAction::DropDisconnected, + "disconnected and idle drops"); + check(act(false, false, false, true) == IoBoardAction::DropDisconnected, + "a band change while disconnected still drops"); + check(act(false, true, true, true) == IoBoardAction::DropDisconnected, + "disconnected outranks every other condition"); + + // THE REGRESSION. The original code guarded the trailing edge of the + // throttle and not the leading one, so a tune while disconnected with + // an idle timer queued five banks. That is this exact combination. + check(act(false, false, /*throttled=*/false, /*bandChanged=*/true) + == IoBoardAction::DropDisconnected, + "leading edge while disconnected drops (the reviewed regression)"); + } + + // ---- keyed defers, and is never overridden by a band change ---- + { + // Switching a band relay under RF burns its contacts. A band change is + // the one thing that otherwise beats the throttle, so it is the case + // most likely to be let through by a careless edit. + check(act(true, true, false, false) == IoBoardAction::DeferKeyed, + "keyed defers"); + check(act(true, true, false, true) == IoBoardAction::DeferKeyed, + "keyed defers EVEN on a band change — relay under RF"); + check(act(true, true, true, true) == IoBoardAction::DeferKeyed, + "keyed defers regardless of the throttle"); + } + + // ---- the throttle coalesces same-band movement only ---- + { + check(act(true, false, true, false) == IoBoardAction::Coalesce, + "same-band movement inside the cooldown coalesces"); + check(act(true, false, false, false) == IoBoardAction::Send, + "same-band movement with an idle throttle sends"); + } + + // ---- a band change beats the throttle ---- + { + // applyBandFilter moves the physical filter relay immediately. Holding + // the amplifier back for the cooldown leaves the two disagreeing, and + // keying in that window is the hazard. + check(act(true, false, true, true) == IoBoardAction::Send, + "a band change sends on the leading edge even mid-cooldown"); + check(act(true, false, false, true) == IoBoardAction::Send, + "a band change with an idle throttle sends"); + } + + // ---- exhaustive: every combination has exactly one defined outcome ---- + { + int sends = 0, coalesces = 0, defers = 0, drops = 0; + for (int i = 0; i < 16; ++i) { + const bool c = i & 1, k = i & 2, t = i & 4, b = i & 8; + switch (act(c, k, t, b)) { + case IoBoardAction::Send: ++sends; break; + case IoBoardAction::Coalesce: ++coalesces; break; + case IoBoardAction::DeferKeyed: ++defers; break; + case IoBoardAction::DropDisconnected: ++drops; break; + } + } + check(drops == 8, "half of all states are disconnected, and all drop"); + check(defers == 4, "connected+keyed always defers"); + check(coalesces == 1, "only connected, unkeyed, throttled, same-band coalesces"); + check(sends == 3, "the remaining connected+unkeyed states send"); + } + + if (g_failures == 0) + std::fprintf(stderr, "hl2_io_board_policy_test: all checks passed\n"); + return g_failures == 0 ? 0 : 1; +} diff --git a/tests/tests.cmake b/tests/tests.cmake index 4f743661f..cb3a5027f 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -394,6 +394,13 @@ add_executable(hl2_metis_protocol_test target_include_directories(hl2_metis_protocol_test PRIVATE src) add_test(NAME hl2_metis_protocol_test COMMAND hl2_metis_protocol_test) +# HL2 IO-board push scheduling — pure policy, standalone (no Qt, no radio). +add_executable(hl2_io_board_policy_test + tests/hl2_io_board_policy_test.cpp +) +target_include_directories(hl2_io_board_policy_test PRIVATE src) +add_test(NAME hl2_io_board_policy_test COMMAND hl2_io_board_policy_test) + # ANAN P2 protocol — pure wire encode/decode, standalone (no Qt / aethercore). # Direct port of the live-validated anan/spike/phase1a.py spike (aetherd ANAN # P2 Phase 1a), run against a real ANAN-G2 on the bench. From b70823d6823f9c578eb9857c4b98c9a8d90109ad Mon Sep 17 00:00:00 2001 From: jensenpat Date: Sun, 6 Sep 2026 08:39:23 -0700 Subject: [PATCH 3/3] Fix HL2 IO-board scheduling and session cleanup. Principle XI. --- docs/HERMES.md | 17 ++- src/core/backends/hl2/Hl2Backend.cpp | 39 +++---- src/core/backends/hl2/Hl2Backend.h | 6 +- src/core/backends/hl2/Hl2IoBoardPolicy.h | 86 ++++++-------- src/core/backends/hl2/MetisClient.cpp | 17 +-- src/core/backends/hl2/MetisClient.h | 1 + tests/hl2_io_board_policy_test.cpp | 138 ++++++++--------------- tests/hl2_tx_gate_test.cpp | 45 ++++++++ 8 files changed, 167 insertions(+), 182 deletions(-) diff --git a/docs/HERMES.md b/docs/HERMES.md index be4a85ab7..bfb999e63 100644 --- a/docs/HERMES.md +++ b/docs/HERMES.md @@ -1941,8 +1941,8 @@ receiver somewhere it cannot hear. **The HL2 has no switchable filters of its own.** It has seven open-collector outputs at `0x00[23:17]`, which the *gateware* forwards as one byte to I2C -address `0x20`. Nothing in this codebase writes I2C — setting the config bits -IS the whole mechanism (oracle §8). +address `0x20`. For this J16 path, setting the config bits is the whole mechanism +(oracle §8); the separate IO-board path below uses direct I2C2 writes. Two things make this the riskiest change in the area: @@ -1974,6 +1974,19 @@ readback anywhere in the protocol — the gateware writes to I2C and nothing answers — so that log line is the only evidence of what the relays were told to do, and a support log captured after the fact has to already contain it. +The external HL2 IO Board at I2C2 address `0x1D` is separate from J16. +It receives true transmit RF frequency as five single-byte writes to registers +0 through 4, MSB first; register 4 commits the value. Connect and band changes +push immediately, while same-band movement coalesces over 500 ms. An immediate +push supersedes any older pending frequency, and link loss clears the schedule. + +MOX/TUNE do not defer the IO board alone: the existing TX NCO and filter paths +already follow a retune, so withholding only the amplifier leaves it on the +wrong band until unkey. This path does not provide cold relay sequencing or +acknowledged amplifier readiness. Ending transmission before changing bands +requires a separately approved change to the keying behavior. No IO-board +write asserts transmit intent; C0 MOX remains owned by the existing TX gate. + ### 17.3 Verifying something with no readback `tests/hl2_live_band_filter_probe.cpp` (hardware-only, `EXCLUDE_FROM_ALL`) is diff --git a/src/core/backends/hl2/Hl2Backend.cpp b/src/core/backends/hl2/Hl2Backend.cpp index f2f1b8c90..07da306b1 100644 --- a/src/core/backends/hl2/Hl2Backend.cpp +++ b/src/core/backends/hl2/Hl2Backend.cpp @@ -4975,8 +4975,9 @@ void Hl2Backend::applyIoBoardFrequency() // bypass result (kOcNone) is a relay pattern with no frequency to offer. const Receiver* txRx = rx(m_txDdc); const double hz = txRx ? txRx->sliceFreqHz : m_rx[0].sliceFreqHz; - if (!(hz > 0.0)) - return; // also rejects NaN, which a comparison to <= 0 would not + if (!std::isfinite(hz) || hz <= 0.0 || hz > static_cast(0xFF'FF'FF'FF'FFULL)) { + return; // validate before converting to the 40-bit field + } // sliceFreqHz is TRUE-RF and the board's field wants true RF: it compares // against band edges to pick a relay. The frequency-calibration scaling in @@ -4995,10 +4996,10 @@ void Hl2Backend::applyIoBoardFrequency() m_ioBoardThrottle->setSingleShot(true); m_ioBoardThrottle->setInterval(kIoBoardThrottleMs); connect(m_ioBoardThrottle, &QTimer::timeout, this, [this] { - if (m_pendingIoBoardHz == 0) + const quint64 pending = m_ioBoardSchedule.takePending(); + if (pending == 0) { return; // cooldown expired with nothing waiting - const quint64 pending = m_pendingIoBoardHz; - m_pendingIoBoardHz = 0; + } if (!sendIoBoardFrequency(pending)) return; // disconnected: nothing to re-arm for // Re-arm: a tune still in progress must keep coalescing. @@ -5006,19 +5007,13 @@ void Hl2Backend::applyIoBoardFrequency() }); } - // The whole decision, in one place, from a policy the suite can exercise - // without a radio — see Hl2IoBoardPolicy.h for why each condition sits - // where it does. m_tuning counts as keyed: TUNE radiates. - switch (ioBoardAction(m_connected, m_keyed || m_tuning, - m_ioBoardThrottle->isActive(), bandChanged)) { + // Neither MOX nor TUNE defers the amplifier alone: the TX NCO/filter + // already follow the requested band. Immediate sends also discard an older + // coalesced value so the timeout cannot send the board back to that band. + switch (m_ioBoardSchedule.request(m_connected, m_ioBoardThrottle->isActive(), + bandChanged, target)) { case IoBoardAction::DropDisconnected: - m_pendingIoBoardHz = 0; - return; - case IoBoardAction::DeferKeyed: - // Deliberately not stashed: unkey re-runs this path and recomputes. - return; case IoBoardAction::Coalesce: - m_pendingIoBoardHz = target; // superseded by any later request return; case IoBoardAction::Send: break; @@ -5043,14 +5038,10 @@ bool Hl2Backend::sendIoBoardFrequency(quint64 hz) // condition is one call site too many, so both now go through here and the // asymmetry cannot come back. // - // WHY DISCONNECTED MATTERS: MetisClient::m_oneShot is not cleared by - // start() or stop(), and unlike the filter byte the IO-board frequency is - // not re-primed through Params. A bank queued while down therefore survives - // as the FIRST thing the next session transmits, briefly pointing an - // amplifier at the band this session ended on. linkUp() pushes the real - // frequency immediately afterwards, so dropping it here loses nothing. + // Do not let a disconnected tune enqueue work for a future session. + // The MetisClient guard and stop-time purge also enforce this at the wire. if (!m_connected) { - m_pendingIoBoardHz = 0; + m_ioBoardSchedule.reset(); return false; } QMetaObject::invokeMethod(m_metis, "setIoBoardTxFrequencyHz", @@ -5072,7 +5063,7 @@ void Hl2Backend::resetIoBoardSchedule() // cannot be made across a disconnect. if (m_ioBoardThrottle) m_ioBoardThrottle->stop(); - m_pendingIoBoardHz = 0; + m_ioBoardSchedule.reset(); m_ioBoardBandKey.clear(); } diff --git a/src/core/backends/hl2/Hl2Backend.h b/src/core/backends/hl2/Hl2Backend.h index bfae01eaf..a0137da52 100644 --- a/src/core/backends/hl2/Hl2Backend.h +++ b/src/core/backends/hl2/Hl2Backend.h @@ -606,8 +606,8 @@ class Hl2Backend : public IRadioBackend { double m_pendingBandwidthHz = 0.0; // 0 = nothing coalesced // The IO board's README asks for at most one frequency update every 0.5 s, - // and only on change. Leading edge applies IMMEDIATELY, so an operator who - // changes band and keys straight away finds the amplifier already switched; + // and only on change. Leading edge queues immediately; wire delivery and relay settling + // are not acknowledged, so this is not an amplifier-ready interlock; // anything arriving inside the cooldown is coalesced and the LAST value // applied when it expires. // @@ -617,7 +617,7 @@ class Hl2Backend : public IRadioBackend { // cooldown — the one moment they are most likely to key. static constexpr int kIoBoardThrottleMs = 500; QTimer* m_ioBoardThrottle = nullptr; - quint64 m_pendingIoBoardHz = 0; // 0 = nothing coalesced + hl2::IoBoardSchedule m_ioBoardSchedule; // The band the IO board was last told about, as a bandKeyForHz() key. // Empty means "no session has told it anything", which is also the state // reset() restores — so the first push after any connect is treated as a diff --git a/src/core/backends/hl2/Hl2IoBoardPolicy.h b/src/core/backends/hl2/Hl2IoBoardPolicy.h index 532c3f5aa..29c3c94fd 100644 --- a/src/core/backends/hl2/Hl2IoBoardPolicy.h +++ b/src/core/backends/hl2/Hl2IoBoardPolicy.h @@ -1,67 +1,49 @@ #pragma once -// When an IO-board frequency push may go out, as a pure decision. -// -// WHY THIS IS A POLICY AND NOT AN `if` CHAIN IN THE BACKEND. The encoder that -// builds the five I2C banks is straightforward and was right first time; every -// defect in this feature has been in the SCHEDULING around it — a guard present -// on the trailing edge of a throttle and missing on the leading one, an armed -// timer surviving a disconnect, a band change coalesced as though it were -// ordinary frequency drift. Those are conditions, not arithmetic, and they are -// only testable if they live somewhere a test can reach without a radio. -// -// The thing being scheduled points an amplifier's band relay. That is the -// reason the ordering below is fixed and commented rather than left to whoever -// next edits the call site. +#include +#include namespace AetherSDR::hl2 { -enum class IoBoardAction { - // Push now. The caller sends, records the band, and restarts the cooldown. - Send, - // Same-band movement inside the cooldown: remember it as pending and let - // the timer deliver the latest value when the window expires. - Coalesce, - // Transmitting. Do nothing at all, and do NOT stash the value: unkey runs - // the whole path again and recomputes from the TX receiver, which is - // fresher than anything held here. - DeferKeyed, - // Not connected. Do nothing AND discard any pending value. - DropDisconnected, -}; +enum class IoBoardAction { Send, Coalesce, DropDisconnected }; -// The decision, in the order the conditions must be tested. -// -// DISCONNECTED FIRST, because the consequence outlives the session: -// MetisClient::m_oneShot is cleared by neither start() nor stop(), and the -// IO-board frequency — unlike the filter byte — is not re-primed through -// Params. A bank queued while the link is down therefore becomes the FIRST -// thing the next connect transmits, pointing an amplifier at the band the -// previous session ended on. -// -// KEYED SECOND, because switching a band relay under RF burns its contacts. -// The operator normally cannot retune mid-transmission on this radio, but -// connect-time pushes and the automation bridge can both reach the scheduler -// while MOX is up, and the cost of being wrong is someone's hardware. -// -// BAND CHANGE BEATS THE THROTTLE, because the rate limit exists for VFO sweeps -// (~10 events/second against a board that asks for two) and a band crossing is -// not that case: applyBandFilter moves the physical filter relay immediately, -// so deferring the amplifier leaves the two disagreeing for up to the cooldown -// — and keying in that window is exactly the hazard. Rate-limit frequency -// tracking; never rate-limit which band the amplifier is on. +// The radio already moves its TX NCO and filter on a band change, including +// during MOX/TUNE. Deferring only the amplifier until unkey strands it on the +// old band. This schedule follows the radio; it does not sequence RF/relays. [[nodiscard]] constexpr IoBoardAction ioBoardAction(bool connected, - bool keyed, bool throttleActive, bool bandChanged) noexcept { - if (!connected) + if (!connected) { return IoBoardAction::DropDisconnected; - if (keyed) - return IoBoardAction::DeferKeyed; - if (throttleActive && !bandChanged) + } + if (throttleActive && !bandChanged) { return IoBoardAction::Coalesce; + } return IoBoardAction::Send; } -} // namespace AetherSDR::hl2 +// Pending work belongs to the current scheduling window. An immediate band +// change supersedes a same-band value waiting in the previous window. +class IoBoardSchedule { +public: + [[nodiscard]] IoBoardAction request(bool connected, bool throttleActive, + bool bandChanged, std::uint64_t hz) noexcept + { + const IoBoardAction action = ioBoardAction(connected, throttleActive, bandChanged); + m_pendingHz = action == IoBoardAction::Coalesce ? hz : 0; + return action; + } + + [[nodiscard]] std::uint64_t takePending() noexcept + { + return std::exchange(m_pendingHz, 0); + } + + void reset() noexcept { m_pendingHz = 0; } + +private: + std::uint64_t m_pendingHz = 0; +}; + +} // namespace AetherSDR::hl2 diff --git a/src/core/backends/hl2/MetisClient.cpp b/src/core/backends/hl2/MetisClient.cpp index 5b02f3e70..d9f0a3464 100644 --- a/src/core/backends/hl2/MetisClient.cpp +++ b/src/core/backends/hl2/MetisClient.cpp @@ -359,6 +359,13 @@ void MetisClient::stop() m_socket = nullptr; } m_running = false; + // An interrupted five-bank write must not finish in the next session. + // Preserve unrelated one-shot setup; only this board's writes are stale. + std::erase_if(m_oneShot, [](const Cc& bank) { + return bank[0] == kC0I2c2 && bank[1] == kI2cCookieWrite + && bank[2] == (kI2cStopAtEnd | kIoBoardI2cAddr); + }); + m_ioBoardTxFreqSent = false; if (m_linkUp) { m_linkUp = false; emit linkDown(); @@ -540,16 +547,12 @@ void MetisClient::setBandFilter(int ocFilterByte) void MetisClient::setIoBoardTxFrequencyHz(quint64 hz) { - // DEFENCE IN DEPTH against a bank outliving its session. m_oneShot is not - // cleared by start() or stop(), so anything queued while the stream is down - // becomes the first thing the NEXT connect transmits -- pointing an - // amplifier at the band the previous session ended on. Hl2Backend already - // refuses to schedule while disconnected; this is the wire's own refusal, - // so a future caller that misses that guard cannot reintroduce the hazard. + // Refuse disconnected requests even if a caller missed the backend guard. + // stop() also discards any unfinished board write from a running session. if (!m_running) return; if (m_ioBoardTxFreqSent && hz == m_ioBoardTxFreqHz) - return; // board already holds this frequency + return; // already queued in this session m_ioBoardTxFreqSent = true; m_ioBoardTxFreqHz = hz; // INFO, not debug, and for the same reason the band filter is: there is no diff --git a/src/core/backends/hl2/MetisClient.h b/src/core/backends/hl2/MetisClient.h index 78f8d2ba8..1501d68c4 100644 --- a/src/core/backends/hl2/MetisClient.h +++ b/src/core/backends/hl2/MetisClient.h @@ -393,6 +393,7 @@ private slots: // Ordering matters: a frequency change and its pipeline reset must reach the // radio in that order, and neither should wait up to three frames for the // rotation to come back around. + friend struct MetisClientTestAccess; // socket-free transport-state injection std::deque m_oneShot; // which register pair to send next // Last transmit frequency handed to the IO board, and whether one ever was. // A separate flag rather than a 0 sentinel: 0 Hz is not a plausible tuned diff --git a/tests/hl2_io_board_policy_test.cpp b/tests/hl2_io_board_policy_test.cpp index 36912b364..ee84a57d1 100644 --- a/tests/hl2_io_board_policy_test.cpp +++ b/tests/hl2_io_board_policy_test.cpp @@ -1,107 +1,57 @@ -// HL2 IO-board push scheduling policy. Pure — no Qt, no aethercore, no radio. -// -// Every defect found in this feature during review was in the scheduling, not -// the encoder: a guard present on one edge of the throttle and absent on the -// other, an armed timer surviving a disconnect, a band change coalesced as -// though it were ordinary frequency drift. These checks pin the conditions so -// those cannot come back silently. - +// Socket-free scheduling regressions; the backend uses this state on both +// the tune path and the QTimer timeout path. #include "core/backends/hl2/Hl2IoBoardPolicy.h" - #include using namespace AetherSDR::hl2; - -static int g_failures = 0; -static void check(bool cond, const char* what) +static int failures = 0; +static void check(bool ok, const char* message) { - if (!cond) { - std::fprintf(stderr, "FAIL: %s\n", what); - ++g_failures; + if (!ok) { + std::fprintf(stderr, "FAIL: %s\n", message); + ++failures; } } -// Named arguments at the call site: ioBoardAction(connected, keyed, -// throttleActive, bandChanged) is four bools, and a transposed pair would -// otherwise still compile and still look right. -static IoBoardAction act(bool connected, bool keyed, bool throttled, bool bandChanged) -{ - return ioBoardAction(connected, keyed, throttled, bandChanged); -} - int main() { - // ---- disconnected wins over everything ---- - { - // The consequence outlives the session: a bank queued while down is not - // discarded by start()/stop() and becomes the FIRST thing the next - // connect sends, pointing an amplifier at the previous band. - check(act(false, false, false, false) == IoBoardAction::DropDisconnected, - "disconnected and idle drops"); - check(act(false, false, false, true) == IoBoardAction::DropDisconnected, - "a band change while disconnected still drops"); - check(act(false, true, true, true) == IoBoardAction::DropDisconnected, - "disconnected outranks every other condition"); - - // THE REGRESSION. The original code guarded the trailing edge of the - // throttle and not the leading one, so a tune while disconnected with - // an idle timer queued five banks. That is this exact combination. - check(act(false, false, /*throttled=*/false, /*bandChanged=*/true) - == IoBoardAction::DropDisconnected, - "leading edge while disconnected drops (the reviewed regression)"); - } - - // ---- keyed defers, and is never overridden by a band change ---- - { - // Switching a band relay under RF burns its contacts. A band change is - // the one thing that otherwise beats the throttle, so it is the case - // most likely to be let through by a careless edit. - check(act(true, true, false, false) == IoBoardAction::DeferKeyed, - "keyed defers"); - check(act(true, true, false, true) == IoBoardAction::DeferKeyed, - "keyed defers EVEN on a band change — relay under RF"); - check(act(true, true, true, true) == IoBoardAction::DeferKeyed, - "keyed defers regardless of the throttle"); - } - - // ---- the throttle coalesces same-band movement only ---- - { - check(act(true, false, true, false) == IoBoardAction::Coalesce, - "same-band movement inside the cooldown coalesces"); - check(act(true, false, false, false) == IoBoardAction::Send, - "same-band movement with an idle throttle sends"); - } - - // ---- a band change beats the throttle ---- - { - // applyBandFilter moves the physical filter relay immediately. Holding - // the amplifier back for the cooldown leaves the two disagreeing, and - // keying in that window is the hazard. - check(act(true, false, true, true) == IoBoardAction::Send, - "a band change sends on the leading edge even mid-cooldown"); - check(act(true, false, false, true) == IoBoardAction::Send, - "a band change with an idle throttle sends"); - } - - // ---- exhaustive: every combination has exactly one defined outcome ---- - { - int sends = 0, coalesces = 0, defers = 0, drops = 0; - for (int i = 0; i < 16; ++i) { - const bool c = i & 1, k = i & 2, t = i & 4, b = i & 8; - switch (act(c, k, t, b)) { - case IoBoardAction::Send: ++sends; break; - case IoBoardAction::Coalesce: ++coalesces; break; - case IoBoardAction::DeferKeyed: ++defers; break; - case IoBoardAction::DropDisconnected: ++drops; break; - } + IoBoardSchedule schedule; + check(schedule.request(true, false, true, 7'100'000) == IoBoardAction::Send, + "connect sends immediately"); + check(schedule.request(true, true, false, 7'101'000) == IoBoardAction::Coalesce, + "same-band sweep coalesces"); + check(schedule.request(true, true, true, 14'225'000) == IoBoardAction::Send, + "40m to 20m beats the cooldown, including during MOX/TUNE"); + check(schedule.takePending() == 0, + "timeout after band change must not restore the pending 40m value"); + + (void)schedule.request(true, true, false, 14'226'000); + (void)schedule.request(true, true, false, 14'227'000); + check(schedule.takePending() == 14'227'000, "timeout sends latest same-band value"); + check(schedule.takePending() == 0, "timeout consumes pending work once"); + + (void)schedule.request(true, true, false, 14'228'000); + check(schedule.request(false, false, true, 7'100'000) == IoBoardAction::DropDisconnected, + "disconnected leading edge refuses to send"); + check(schedule.takePending() == 0, "disconnected request discards pending work"); + (void)schedule.request(true, true, false, 14'229'000); + schedule.reset(); + check(schedule.takePending() == 0, "link loss cancels pending work"); + check(schedule.request(true, false, true, 7'100'000) == IoBoardAction::Send, + "reconnect immediately pushes the current frequency"); + + for (int bits = 0; bits < 8; ++bits) { + const bool connected = bits & 1; + const bool throttled = bits & 2; + const bool bandChanged = bits & 4; + const IoBoardAction result = ioBoardAction(connected, throttled, bandChanged); + if (!connected) { + check(result == IoBoardAction::DropDisconnected, "all disconnected states drop"); + } else if (bandChanged || !throttled) { + check(result == IoBoardAction::Send, "connected band changes and idle sends proceed"); + } else { + check(result == IoBoardAction::Coalesce, "only same-band cooldown coalesces"); } - check(drops == 8, "half of all states are disconnected, and all drop"); - check(defers == 4, "connected+keyed always defers"); - check(coalesces == 1, "only connected, unkeyed, throttled, same-band coalesces"); - check(sends == 3, "the remaining connected+unkeyed states send"); } - - if (g_failures == 0) - std::fprintf(stderr, "hl2_io_board_policy_test: all checks passed\n"); - return g_failures == 0 ? 0 : 1; + return failures == 0 ? 0 : 1; } diff --git a/tests/hl2_tx_gate_test.cpp b/tests/hl2_tx_gate_test.cpp index 688c79d33..dc5178034 100644 --- a/tests/hl2_tx_gate_test.cpp +++ b/tests/hl2_tx_gate_test.cpp @@ -16,6 +16,14 @@ #include #include +namespace AetherSDR::hl2 { +struct MetisClientTestAccess { + // No start(), bind(), peer or datagrams: inject streaming state and inspect + // packets using the same builder as the transport. + static void setStreaming(MetisClient& client) { client.m_running = true; } +}; +} + using namespace AetherSDR::hl2; static int g_failures = 0; @@ -55,6 +63,43 @@ int main(int argc, char** argv) QCoreApplication app(argc, argv); MetisClient client; + { + MetisClient board; + const auto isBoardWrite = [](const auto& packet) { + return (packet[8 + kFrameSize + 3] & ~kC0MoxBit) == kC0I2c2; + }; + board.setIoBoardTxFrequencyHz(7'100'000); + check(!isBoardWrite(board.buildNextControlPacket()), + "disconnected IO-board request queues nothing"); + MetisClientTestAccess::setStreaming(board); + board.setIoBoardTxFrequencyHz(7'100'000); + check(isBoardWrite(board.buildNextControlPacket()), "connected IO-board push reaches packet builder"); + board.stop(); // interrupt after only the MSB: four stale banks remain + for (int i = 0; i < 8; ++i) { + check(!isBoardWrite(board.buildNextControlPacket()), + "stop discards every unfinished IO-board bank"); + } + MetisClientTestAccess::setStreaming(board); + board.setIoBoardTxFrequencyHz(7'100'000); + for (int reg = 0; reg < 5; ++reg) { + const auto packet = board.buildNextControlPacket(); + check(isBoardWrite(packet), "same frequency is resent after session reset"); + check(packet[8 + kFrameSize + 6] == reg, "session restarts at MSB and commits LSB last"); + check(!anyFrameKeyed(packet), "IO-board writes never key an unkeyed transmitter"); + } + board.enableTransmit(true); + board.setMox(true); + board.setIoBoardTxFrequencyHz(14'225'000); + bool sawBoard = false; + for (int i = 0; i < 12; ++i) { + const auto packet = board.buildNextControlPacket(); + sawBoard = sawBoard || isBoardWrite(packet); + check(anyFrameKeyed(packet), "IO-board update preserves explicit key state"); + } + check(sawBoard, "IO-board band update is not withheld while keyed"); + board.setMox(false); + } + check(!client.transmitEnabled(), "transmit is DISABLED by default"); check(!client.isKeyed(), "not keyed by default");