fix(icom): a refused tune must not read as a successful one

src/core/backends/icom/IcomCivBackend.cpp

@@ -1858,6 +1858,72 @@ 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.
+    // ⚠ 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
+        // 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<double>(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);
+        });
+        // 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);
 
     switch (frame.cmd) {

tests/icom_incident_telemetry_test.cpp

@@ -7,10 +7,13 @@
 #include "core/backends/icom/IcomCivBackend.h"
 
 #include <QCoreApplication>
+#include <QStringList>
 #include <QVariantMap>
 
+#include <cmath>
 #include <cstdint>
 #include <cstdio>
+#include <string>
 #include <vector>
 
 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,122 @@ 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<double> 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<long long>(kHeldHz),
+              "a refused tune republishes the radio's real VFO, not the "
+              "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<double> 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;
 }