diff --git a/src/core/backends/icom/IcomCivBackend.cpp b/src/core/backends/icom/IcomCivBackend.cpp index 787a27171..8beb7ef27 100644 --- a/src/core/backends/icom/IcomCivBackend.cpp +++ b/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(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) { diff --git a/tests/icom_incident_telemetry_test.cpp b/tests/icom_incident_telemetry_test.cpp index b4f4b67eb..341e41ff2 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,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 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"); + } + + // 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; }