From a6164dd7b80fa2bbd0e86badb47fa37558a94757 Mon Sep 17 00:00:00 2001 From: nigelfenton Date: Sat, 29 Aug 2026 16:08:53 -0400 Subject: [PATCH 1/2] fix(icom): a refused tune must not read as a successful one FA is the radio's NG. IcomCivScheduler::observe() retires FB and FA identically -- both merely release the command slot and carry no state -- so nothing in the backend ever consumed a refusal. isNg() existed in CivCodec.h with no caller in the backend at all. A refused frequency write left the optimistic value standing in the model, and the operator looking at a number the radio had rejected. Reachable in ordinary use on an IC-9700. It has three bands and two receivers, so a receiver cannot be tuned to a band the other one already holds; the radio answers cmd 05 with FA and stays put. Measured on live hardware 2026-08-29 over RS-BA1: six cross-band sets to 70 cm, six FAs, and the display followed all six while rigctl reported RPRT 0 for every one. Evidence in #4840. Correct on every model, not just that one: FA on a frequency write means the write did not take, whatever the reason. The correction follows the pattern the file already uses twice -- the out-of-band gate in setSliceFrequency() and the refused mode in setSliceMode(). Both re-assert radio truth one event-loop turn later, because SliceModel has already accepted and announced the operator's request by then, so a direct emit is applied and immediately announced away. Same ordering contract here. Deliberately narrow: only a frequency write is corrected, because that is the case with hardware evidence and a known-good restoration value (m_frequencyHz, which is radio-authoritative). Other refused writes are a separate question and are left alone rather than guessed at. Test lives in icom_incident_telemetry_test, which is socket-free and actually builds -- icom_backend_test and its fake-radio fixtures are inside a retired #[==[ ]==] block that tests.cmake deliberately does not configure, so a test added there would never have run. Break-tested rather than only run green: with the guard disabled, "a refused tune TELLS the operator" and "republishes the radio's real VFO" both FAIL and the suite exits 1. Restored, 10/10 pass. icom_civ_test, icom_civ_scheduler_test and icom_family_test all still pass, and AetherSDR builds and links on MSVC. Co-Authored-By: Claude Opus 5 --- src/core/backends/icom/IcomCivBackend.cpp | 43 ++++++++++++ tests/icom_incident_telemetry_test.cpp | 86 +++++++++++++++++++++++ 2 files changed, 129 insertions(+) diff --git a/src/core/backends/icom/IcomCivBackend.cpp b/src/core/backends/icom/IcomCivBackend.cpp index 787a27171..a857a7151 100644 --- a/src/core/backends/icom/IcomCivBackend.cpp +++ b/src/core/backends/icom/IcomCivBackend.cpp @@ -1858,6 +1858,49 @@ void IcomCivBackend::onCivFrame(const CivFrame& frame, return; } + // A REFUSED TUNE MUST NOT READ AS A SUCCESSFUL ONE. + // + // FA is the radio's NG. Until now nothing consumed it: observe() treats + // FB and FA identically (both merely retire the transaction and carry no + // state), so a refused write left the optimistic frequency standing in the + // model and the operator looking at a number the radio never entered. + // + // The IC-9700 makes this reachable in ordinary use. It has three bands and + // two receivers, so a receiver cannot be tuned to a band the other one + // already holds; the radio answers cmd 05 with FA and stays put. Measured + // on hardware 2026-08-29 — six cross-band sets, six FAs, and the display + // followed all six. See #4840. + // + // Correct on every model, not just that one: FA on a frequency write means + // the write did not take, whatever the reason. + // + // Deliberately narrow. Only a frequency write is corrected here, because + // that is the case with hardware evidence and a known-good restoration + // value (m_frequencyHz, which is radio-authoritative). Other refused + // writes are a separate question and are left alone rather than guessed at. + if (frame.isNg() && m_civScheduler.stats().lastCompletedKey == "frequency" + && m_frequencyHz != 0) { + // Re-assert the radio's real VFO one event-loop turn later, exactly as + // the out-of-band gate in setSliceFrequency() and the refused mode in + // setSliceMode() already do: SliceModel has accepted and announced the + // operator's request by now, so a direct emit would be overwritten by + // that announcement and the indicator would keep lying. + const double actualMhz = static_cast(m_frequencyHz) / 1.0e6; + qCWarning(lcIcomLink) + << "radio refused the frequency write (CI-V FA); restoring" + << actualMhz << "MHz"; + QTimer::singleShot(0, this, [this, actualMhz] { + SliceDelta delta; + delta.frequency = actualMhz; + emit sliceChanged(sliceId(), delta); + }); + emit configurationWarning( + tr("The radio refused the tune. It is still on %1 MHz — on this " + "model a receiver cannot move to a band the other receiver " + "already holds.") + .arg(actualMhz, 0, 'f', 6)); + } + noteControlSeen(frame.cmd, frame.sub, frame.hasSub); switch (frame.cmd) { diff --git a/tests/icom_incident_telemetry_test.cpp b/tests/icom_incident_telemetry_test.cpp index b4f4b67eb..4c1544eb9 100644 --- a/tests/icom_incident_telemetry_test.cpp +++ b/tests/icom_incident_telemetry_test.cpp @@ -7,10 +7,13 @@ #include "core/backends/icom/IcomCivBackend.h" #include +#include #include +#include #include #include +#include #include using namespace AetherSDR; @@ -42,6 +45,34 @@ struct IcomCivBackendTestAccess { return backend.m_lastIncident; } + // A backend that has a radio-authoritative frequency and one frequency + // write outstanding on the wire — the state a refused tune arrives into. + static void prepareOutstandingFrequencyWrite(IcomCivBackend& backend, + const IcomModel& model, + std::uint64_t generation, + std::uint64_t heldHz) + { + backend.m_model = &model; + backend.m_connected = true; + backend.m_sessionGeneration = generation; + backend.m_frequencyHz = heldHz; + IcomCivScheduler::Request request; + request.frame = cmdSetFrequency(model.civAddress, heldHz + 1'000'000); + request.key = "frequency"; + request.expectsReply = true; + request.acceptsGenericReply = true; + backend.m_civScheduler.enqueue(request, backend.nowMs()); + // Take it off the queue so it is genuinely in flight: observe() only + // retires a transaction that was actually dispatched, and the whole + // point is that the FA below completes THIS request. + (void)backend.m_civScheduler.takeNext(backend.nowMs()); + } + + static std::string lastCompletedKey(const IcomCivBackend& backend) + { + return backend.m_civScheduler.stats().lastCompletedKey; + } + static void prepareAcceptedPttRead(IcomCivBackend& backend, const IcomModel& model, std::uint64_t sessionGeneration) @@ -141,5 +172,60 @@ int main(int argc, char** argv) check(confirmations.size() == 1 && !confirmations.front(), "only an accepted CI-V PTT-off readback publishes confirmation"); + // ---- A REFUSED TUNE IS NOT A SUCCESSFUL ONE -------------------------- + // + // FA is the radio's NG. observe() retires FB and FA identically — both + // merely release the slot and carry no state — so before this, nothing in + // the backend consumed a refusal and the optimistic frequency stood. + // isNg() existed in CivCodec.h with no caller in the backend at all. + // + // Reachable in ordinary use on an IC-9700: three bands, two receivers, so + // a receiver cannot take a band the other one already holds. The radio + // answers cmd 05 with FA and does not move. Measured on hardware + // 2026-08-29 — six cross-band sets, six FAs, display followed all six + // (#4840). + { + constexpr std::uint64_t kHeldHz = 145'030'000; + IcomCivBackend refusedBackend; + std::vector published; + QObject::connect(&refusedBackend, &IRadioBackend::sliceChanged, &app, + [&published](int, const SliceDelta& delta) { + if (delta.frequency) + published.push_back(*delta.frequency); + }); + QStringList warnings; + QObject::connect(&refusedBackend, &IRadioBackend::configurationWarning, + &app, [&warnings](const QString& w) { warnings << w; }); + + IcomCivBackendTestAccess::prepareOutstandingFrequencyWrite( + refusedBackend, *ic705, kGeneration, kHeldHz); + + CivFrame refused; + refused.to = kControllerAddress; + refused.from = ic705->civAddress; + refused.cmd = kCivNg; + IcomCivBackendTestAccess::deliver(refusedBackend, refused, kGeneration); + + // The correction is deferred one event-loop turn, for the same reason + // setSliceFrequency()'s out-of-band gate defers it: SliceModel has + // already announced the operator's request, so a direct emit would be + // announced away and the indicator would keep lying. + QCoreApplication::processEvents(); + + check(IcomCivBackendTestAccess::lastCompletedKey(refusedBackend) + == "frequency", + "the FA retires the outstanding frequency write"); + check(!warnings.isEmpty() + && warnings.constLast().contains(QLatin1String("refused")), + "a refused tune TELLS the operator the radio said no"); + // The load-bearing assertion: a backend that ignores FA publishes + // nothing here, and the display keeps the frequency the radio rejected. + check(published.size() == 1 + && std::llround(published.front() * 1.0e6) + == static_cast(kHeldHz), + "a refused tune republishes the radio's real VFO, not the " + "frequency the radio rejected"); + } + return failures == 0 ? 0 : 1; } From 7a42040b15cceb15ff5e19c9e03f6211b09f39b3 Mon Sep 17 00:00:00 2001 From: nigelfenton Date: Tue, 1 Sep 2026 22:45:33 -0400 Subject: [PATCH 2/2] fix(icom): gate the FA correction on the observation, not a stale key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch from @ten9876: the FA handler tested only `stats().lastCompletedKey == "frequency"`, which is the wrong signal for "a frequency write was just refused". `observe()` sets that key ONLY when a frame matches the in-flight transaction. An unmatched FA returns Observation::Unmatched and leaves the key at its previous value — and since frequency writes are the most common transaction, the key is usually "frequency" from the last real tune. So a stray or duplicate NG, or an NG for a transaction that had already expired, fired the block with no frequency write refused at all: a false "the radio refused the tune" toast plus a redundant re-assert. That is the lying-indicator failure this PR exists to remove, inverted. The correct signal was already captured 55 lines above and unused. The predicate now requires Observation::Accepted (this frame completed the in-flight transaction) AND the key naming which transaction it was. Also fixes the reviewer's nit on the warning text, which mattered more than it looked. The sentence "on this model a receiver cannot move to a band the other receiver already holds" fired on every Icom model, including single-receiver ones where it is simply untrue. The refusal is now stated generically and the dual-receiver cause appended only when the profile has receivers > 1. The band-panel path in #4840 reaches this same warning on an IC-705, so the wrong sentence was reachable in ordinary use, not only in theory. Tests: a new unmatched-FA row delivers two NGs — the first matches and must still correct exactly once, the second arrives with nothing in flight and must publish nothing and warn nobody. It asserts the stale key really does still read "frequency" first, so the test pins the actual mechanism rather than a proxy. Verified by mutation, not a green re-run: removing the Accepted gate fails both new assertions and passes every pre-existing one, which is why this needed its own row — the Accepted path passes with or without the fix. Co-Authored-By: Claude Opus 5 --- src/core/backends/icom/IcomCivBackend.cpp | 35 ++++++++++--- tests/icom_incident_telemetry_test.cpp | 62 +++++++++++++++++++++++ 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/src/core/backends/icom/IcomCivBackend.cpp b/src/core/backends/icom/IcomCivBackend.cpp index a857a7151..8beb7ef27 100644 --- a/src/core/backends/icom/IcomCivBackend.cpp +++ b/src/core/backends/icom/IcomCivBackend.cpp @@ -1878,7 +1878,22 @@ void IcomCivBackend::onCivFrame(const CivFrame& frame, // that is the case with hardware evidence and a known-good restoration // value (m_frequencyHz, which is radio-authoritative). Other refused // writes are a separate question and are left alone rather than guessed at. - if (frame.isNg() && m_civScheduler.stats().lastCompletedKey == "frequency" + // ⚠ BOTH halves of the predicate are load-bearing, and `lastCompletedKey` + // alone is NOT enough. observe() sets it only when a frame MATCHES the + // in-flight transaction; an unmatched FA returns Observation::Unmatched and + // leaves the key at its previous value. Frequency writes are the most + // common transaction, so `lastCompletedKey == "frequency"` is usually true + // from the last real tune — and a later stray or duplicate NG, or an NG for + // a transaction that already expired, would fire this block with no + // frequency write refused at all: a false "the radio refused the tune" + // toast plus a redundant re-assert. That is precisely the lying-indicator + // failure this block exists to remove, inverted. + // + // Observation::Accepted is the signal that THIS frame completed the + // in-flight transaction; the key then says WHICH transaction it was. + if (frame.isNg() + && observation == IcomCivScheduler::Observation::Accepted + && m_civScheduler.stats().lastCompletedKey == "frequency" && m_frequencyHz != 0) { // Re-assert the radio's real VFO one event-loop turn later, exactly as // the out-of-band gate in setSliceFrequency() and the refused mode in @@ -1894,11 +1909,19 @@ void IcomCivBackend::onCivFrame(const CivFrame& frame, delta.frequency = actualMhz; emit sliceChanged(sliceId(), delta); }); - emit configurationWarning( - tr("The radio refused the tune. It is still on %1 MHz — on this " - "model a receiver cannot move to a band the other receiver " - "already holds.") - .arg(actualMhz, 0, 'f', 6)); + // The dual-receiver explanation is TRUE ONLY WHERE THERE ARE TWO. + // This block fires on every Icom model, so an IC-705 refusing a write + // for some other reason was being handed a reason that cannot apply to + // it. State the refusal generically and append the cause only where the + // profile actually has a second receiver to collide with. + QString why = tr("The radio refused the tune. It is still on %1 MHz.") + .arg(actualMhz, 0, 'f', 6); + if (m_model && m_model->receivers > 1) { + why += QLatin1Char(' '); + why += tr("On this model a receiver cannot move to a band the " + "other receiver already holds."); + } + emit configurationWarning(why); } noteControlSeen(frame.cmd, frame.sub, frame.hasSub); diff --git a/tests/icom_incident_telemetry_test.cpp b/tests/icom_incident_telemetry_test.cpp index 4c1544eb9..341e41ff2 100644 --- a/tests/icom_incident_telemetry_test.cpp +++ b/tests/icom_incident_telemetry_test.cpp @@ -227,5 +227,67 @@ int main(int argc, char** argv) "frequency the radio rejected"); } + // AN UNMATCHED FA MUST NOT FIRE THE CORRECTION. + // + // The regression this pins: gating only on `lastCompletedKey == "frequency"` + // is wrong, because observe() sets that key ONLY when a frame matches the + // in-flight transaction. An unmatched FA returns Observation::Unmatched and + // leaves the key at its previous value — and since frequency writes are the + // most common transaction, the key is usually "frequency" from the last real + // tune. So a stray or duplicate NG, or an NG for a transaction that already + // expired, would fire the block with NO frequency write refused: a false + // "the radio refused the tune" toast and a redundant re-assert. + // + // That is the lying-indicator failure this fix exists to remove, inverted — + // which is why it gets its own row rather than being left to the Accepted + // path above (that one passes either way, with or without the gate). + { + constexpr std::uint64_t kHeldHz = 145'030'000; + IcomCivBackend strayBackend; + std::vector published; + QObject::connect(&strayBackend, &IRadioBackend::sliceChanged, &app, + [&published](int, const SliceDelta& delta) { + if (delta.frequency) + published.push_back(*delta.frequency); + }); + QStringList warnings; + QObject::connect(&strayBackend, &IRadioBackend::configurationWarning, + &app, [&warnings](const QString& w) { warnings << w; }); + + IcomCivBackendTestAccess::prepareOutstandingFrequencyWrite( + strayBackend, *ic705, kGeneration, kHeldHz); + + CivFrame refused; + refused.to = kControllerAddress; + refused.from = ic705->civAddress; + refused.cmd = kCivNg; + + // First FA: matches the in-flight write, retires it, corrects the + // display. This is the legitimate case and it must still work. + IcomCivBackendTestAccess::deliver(strayBackend, refused, kGeneration); + QCoreApplication::processEvents(); + const std::size_t afterReal = published.size(); + const int warningsAfterReal = warnings.size(); + + check(afterReal == 1 && warningsAfterReal == 1, + "the matched FA still corrects exactly once"); + + // Second FA: nothing is in flight now, so observe() returns Unmatched + // and leaves lastCompletedKey at "frequency" from the write above. + // Under the old predicate this fires again; under the Accepted gate it + // must do nothing at all. + check(IcomCivBackendTestAccess::lastCompletedKey(strayBackend) + == "frequency", + "the stale key really does still read \"frequency\""); + + IcomCivBackendTestAccess::deliver(strayBackend, refused, kGeneration); + QCoreApplication::processEvents(); + + check(published.size() == afterReal, + "an unmatched FA republishes NOTHING (no redundant re-assert)"); + check(warnings.size() == warningsAfterReal, + "an unmatched FA does not tell the operator a tune was refused"); + } + return failures == 0 ? 0 : 1; }