From 3fa5e1c8bf5202144bae662fdbda088237953515 Mon Sep 17 00:00:00 2001 From: Ozy K6OZY Date: Sat, 11 Jul 2026 23:59:31 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(spectrum):=20defer,=20never=20drop,=20p?= =?UTF-8?q?rofile-owned=20pan=20writes=20(#4142)=20=E2=80=94=20Principle?= =?UTF-8?q?=20II.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RadioModel::sendCmd() drops any command isProfileOwnedRadioStateWrite() classifies as profile-owned while the profile-load hold is armed: it returns before a TCP sequence number is allocated, so the command never reaches the wire. Every "display pan set center=..." is classified that way. Direct frequency entry emits an atomic pair — "slice tune " (passes) and "display pan set center=" (silently dropped). The slice retunes; the radio never retunes the waterfall tile stream. The waterfall then goes black, and the renderer is NOT at fault: PanadapterStream decodes each VITA-49 tile.s own FrameLowFreq/BinBandwidth and remapHistoryRowInto() projects every row by that row.s own span. The liar is the optimistic local update, which advanced the pan center to the typed frequency while the radio stayed put. Honest tiles were projected into a view that falsely claimed the new center, so the non-overlapping region was correctly black — and baked into history. Defer, never drop, mirroring the codebase.s own idiom (requestPanDimensionsForRadio / flushPendingProfileLoadPanDimensions): - RadioModel::requestPanCenter(panId, center, bandwidth=-1) is now the only supported way to write a pan center. While the hold is armed it coalesces the request per pan and returns false WITHOUT advancing local model state; the queue carries the REQUESTED value, since the model still holds the old center at flush time. Coalescing is field-wise so a later center-only request cannot erase a bandwidth an earlier zoom asked for. - flushPendingProfileLoadPanCenters() replays them, re-resolving against the LIVE pan topology so a pan the profile removed (or replaced under a new id) never matches. Hooked into the existing ordered flush pass after dimensions (bin size is bandwidth/xpixels). No fourth timer. - All ten pan-center writers now route through it — typed-frequency reveal (the reported bug), pan-follow, centerActiveSliceInPanadapter, applyPanRangeRequest (zoom/drag, also silently dropped today), snapCenterLockForSlice, the channel-strip applet, edge-pan drag, DigitalModes, ATU pre-tune, and automation. Fixes the class, not the instance. - Local state may only advance when a command actually reaches the wire. Sites that also move the view directly now gate that on the return value. - sendCmd().s signature and guard are UNTOUCHED. Its suppression log is escalated qCDebug -> qCWarning: the backstop stays as defense-in-depth, but anything still reaching it is now a bug we want to see rather than silently swallow. - isProfileOwnedRadioStateWrite() moves out of RadioModel.cpp.s anonymous namespace into header-only ProfileLoadCommand.h (pure move, no behaviour change) so the classification contract is testable; profile_load_command_test covers it. Non-regression proof: every write this introduces lands AFTER hold expiry — a window in which the current code already permits arbitrary pan writes and already schedules one. flushPendingProfileLoadPanCenters() early-returns while the hold is armed, so inside the hold this puts ZERO additional bytes on the wire. It is therefore structurally incapable of reintroducing the missing-slices corruption that the hold (PR #3563) exists to prevent. Co-authored-by: Don @ cloaked.agency --- src/gui/AtuPreTuneDialog.cpp | 30 ++---- src/gui/MainWindow.cpp | 46 +++++---- src/gui/MainWindow_DigitalModes.cpp | 12 +-- src/gui/MainWindow_Wiring.cpp | 87 ++++++++++------ src/models/ProfileLoadCommand.h | 40 ++++++++ src/models/RadioModel.cpp | 151 +++++++++++++++++++++------- src/models/RadioModel.h | 52 ++++++++++ tests/profile_load_command_test.cpp | 60 +++++++++++ 8 files changed, 366 insertions(+), 112 deletions(-) diff --git a/src/gui/AtuPreTuneDialog.cpp b/src/gui/AtuPreTuneDialog.cpp index 25183e640..9687ee8eb 100644 --- a/src/gui/AtuPreTuneDialog.cpp +++ b/src/gui/AtuPreTuneDialog.cpp @@ -642,16 +642,10 @@ void AtuPreTuneDialog::beginNextPoint() if (firstOfBand && !m_originalPanId.isEmpty() && p.bandHighMhz > p.bandLowMhz) { const double center = (p.bandLowMhz + p.bandHighMhz) / 2.0; const double width = (p.bandHighMhz - p.bandLowMhz) * 1.10; - const QString centerStr = QString::number(center, 'f', 6); - const QString widthStr = QString::number(width, 'f', 6); - if (auto* pan = m_radio->panadapter(m_originalPanId)) { - // Local model nudge before the radio echo (aetherd RFC 2.3: the - // normalized setter replaces applyPanStatus({center,bandwidth})). - pan->setCenterBandwidth(center, width); - } - m_radio->sendCommand( - QString("display pan set %1 center=%2 bandwidth=%3") - .arg(m_originalPanId, centerStr, widthStr)); + // Center and bandwidth together, and deferred rather than dropped if a + // profile load is holding radio-state writes (#4142). The local model + // advances only when the command reaches the wire. + m_radio->requestPanCenter(m_originalPanId, center, width); } // Move slice to target. SliceModel::setFrequency uses autopan=0 — no recenter. @@ -885,16 +879,12 @@ void AtuPreTuneDialog::restoreOriginalFrequency() // Restore the panadapter zoom captured at sweep start — same // optimistic-update pattern used for band transitions. if (!m_originalPanId.isEmpty() && m_originalPanBandwidthMhz > 0.0) { - const QString centerStr = QString::number(m_originalPanCenterMhz, 'f', 6); - const QString widthStr = QString::number(m_originalPanBandwidthMhz, 'f', 6); - if (auto* pan = m_radio->panadapter(m_originalPanId)) { - // Local model nudge before the radio echo (aetherd RFC 2.3: the - // normalized setter replaces applyPanStatus({center,bandwidth})). - pan->setCenterBandwidth(m_originalPanCenterMhz, m_originalPanBandwidthMhz); - } - m_radio->sendCommand( - QString("display pan set %1 center=%2 bandwidth=%3") - .arg(m_originalPanId, centerStr, widthStr)); + // Restoring the pre-sweep zoom is a user-visible promise: dropping it + // during a profile load would strand the pan on the sweep's band view. + // Deferred and replayed instead (#4142). + m_radio->requestPanCenter(m_originalPanId, + m_originalPanCenterMhz, + m_originalPanBandwidthMhz); } } diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp index ab6db6f99..2500de1aa 100644 --- a/src/gui/MainWindow.cpp +++ b/src/gui/MainWindow.cpp @@ -6080,28 +6080,28 @@ void MainWindow::applyPanRangeRequest(const QString& panId, double centerMhz, } auto* pan = m_radioModel.panadapter(panId); - const QString centerStr = QString::number(centerMhz, 'f', 6); - const QString bandwidthStr = QString::number(bandwidthMhz, 'f', 6); if (pan) { if (qFuzzyCompare(pan->centerMhz(), centerMhz) && qFuzzyCompare(pan->bandwidthMhz(), bandwidthMhz)) { return; } - // Update both values together before the radio echo arrives. Explicit - // zoom workflows are especially sensitive to center/bandwidth skew; - // splitting them produced the P1/P2 waterfall-loss and zoom-drift bugs. - // (aetherd RFC 2.3: setCenterBandwidth replaces applyPanStatus here.) - pan->setCenterBandwidth(centerMhz, bandwidthMhz); } - m_radioModel.sendCommand( - QString("display pan set %1 center=%2 bandwidth=%3") - .arg(panId, centerStr, bandwidthStr)); + // Center and bandwidth travel together in one command. Explicit zoom + // workflows are especially sensitive to center/bandwidth skew; splitting + // them produced the P1/P2 waterfall-loss and zoom-drift bugs. + // + // #4142: this pair is classified profile-owned, so it was silently DROPPED + // during a profile load — zoom and drag, not just typed frequency entry. + // requestPanCenter() defers and replays it, and only advances the local + // model when the command actually reaches the wire. + const bool sent = m_radioModel.requestPanCenter(panId, centerMhz, bandwidthMhz); qDebug() << "Pan range request:" << source << "center" << centerMhz - << "bandwidth" << bandwidthMhz; + << "bandwidth" << bandwidthMhz + << (sent ? "" : "(deferred: profile load)"); } void MainWindow::setActiveSlice(int sliceId) @@ -7898,16 +7898,24 @@ void MainWindow::centerActiveSliceInPanadapter(bool forceRadioCenter, double cen m_panStack->setActivePan(applet->panId()); } - // Keep the local spectrum centered immediately so the active slice marker - // is visible before the radio's status echo arrives. - sw->setFrequencyRange(targetMhz, bandwidthMhz); - pushSliceFrequencyToOverlays(s, targetMhz); - + // #4142 — ask the radio FIRST, so the local view can only advance if the + // command actually reached the wire. During a profile load this center write + // is suppressed; centering the spectrum on a center the radio never took is + // exactly what projects honest tiles into a lying frame and bakes black rows + // into waterfall history. requestPanCenter() defers and replays it instead. + bool centerDeferred = false; if (forceRadioCenter && m_radioModel.isConnected()) { - m_radioModel.sendCommand( - QString("display pan set %1 center=%2") - .arg(s->panId()).arg(targetMhz, 0, 'f', 6)); + centerDeferred = !m_radioModel.requestPanCenter(s->panId(), targetMhz); + } + + // Keep the local spectrum centered immediately so the active slice marker is + // visible before the radio's status echo arrives — unless the center was + // deferred, in which case the pan must keep showing truthful spectrum for the + // span the radio still has. + if (!centerDeferred) { + sw->setFrequencyRange(targetMhz, bandwidthMhz); } + pushSliceFrequencyToOverlays(s, targetMhz); TuneCenteringResult result; result.oldCenterMhz = pan ? pan->centerMhz() : targetMhz; diff --git a/src/gui/MainWindow_DigitalModes.cpp b/src/gui/MainWindow_DigitalModes.cpp index c5b78d71e..c326dd991 100644 --- a/src/gui/MainWindow_DigitalModes.cpp +++ b/src/gui/MainWindow_DigitalModes.cpp @@ -1194,18 +1194,18 @@ void MainWindow::activateWFM(int sliceId) m_wfmSliceId = sliceId; // Centre the pan (and with it the DAX IQ stream) on the slice — once. - // applyPanStatus updates the local model immediately so offsets computed - // before the radio echoes the new centre are already correct. + // requestPanCenter() updates the local model as it puts the command on the + // wire, so offsets computed before the radio echoes the new centre are + // already correct — and during a profile load it defers the write instead of + // letting it be dropped, which would have left the DAX IQ stream centred + // somewhere the client no longer believed it was (#4142). auto centerPanAtSlice = [this, s]() { const QString panId = s->panId(); if (panId.isEmpty()) return; const double freq = s->frequency(); auto* pan = m_radioModel.panadapter(panId); if (pan && qFuzzyCompare(pan->centerMhz(), freq)) return; - const QString freqStr = QString::number(freq, 'f', 6); - if (pan) pan->setCenterBandwidth(freq, -1.0); // aetherd RFC 2.3 - m_radioModel.sendCommand( - QString("display pan set %1 center=%2").arg(panId, freqStr)); + m_radioModel.requestPanCenter(panId, freq); }; centerPanAtSlice(); diff --git a/src/gui/MainWindow_Wiring.cpp b/src/gui/MainWindow_Wiring.cpp index edb3aaf56..c9094b64b 100644 --- a/src/gui/MainWindow_Wiring.cpp +++ b/src/gui/MainWindow_Wiring.cpp @@ -467,23 +467,31 @@ bool MainWindow::snapCenterLockForSlice(SliceModel* slice, double mhz, bool send bandwidthMhz > 0.0 ? std::max(mhz, bandwidthMhz / 2.0) : mhz; bool changed = false; - if (sw && bandwidthMhz > 0.0 - && (!qFuzzyCompare(sw->centerMhz(), targetCenterMhz) - || !qFuzzyCompare(sw->bandwidthMhz(), bandwidthMhz))) { - sw->setFrequencyRangeImmediate(targetCenterMhz, bandwidthMhz); - changed = true; - } - const bool modelNeedsCenter = !qFuzzyCompare(pan->centerMhz(), targetCenterMhz); + bool centerDeferred = false; + if (!kiwiDisplayActive && modelNeedsCenter) { - pan->setCenterBandwidth(targetCenterMhz, -1.0); // aetherd RFC 2.3 - changed = true; + if (sendCommand) { + // #4142 — defer, never drop. requestPanCenter() advances the local + // model only when the command actually reaches the wire, and queues + // it for replay when a profile load is holding radio-state writes. + centerDeferred = !m_radioModel.requestPanCenter(panId, targetCenterMhz); + } else { + // Local-only snap: the caller explicitly wants no radio write, so + // there is no wire command for the model to diverge from. + pan->setCenterBandwidth(targetCenterMhz, -1.0); // aetherd RFC 2.3 + } + changed = !centerDeferred; } - if (sendCommand && !kiwiDisplayActive && modelNeedsCenter) { - m_radioModel.sendCommand( - QStringLiteral("display pan set %1 center=%2") - .arg(panId, QString::number(targetCenterMhz, 'f', 6))); + // Never move the visible span while the radio stays put. A view that claims + // a center the radio never took is precisely what projects honest tiles into + // a lying frame and bakes black rows into waterfall history (#4142). + if (!centerDeferred && sw && bandwidthMhz > 0.0 + && (!qFuzzyCompare(sw->centerMhz(), targetCenterMhz) + || !qFuzzyCompare(sw->bandwidthMhz(), bandwidthMhz))) { + sw->setFrequencyRangeImmediate(targetCenterMhz, bandwidthMhz); + changed = true; } return changed; @@ -1952,6 +1960,15 @@ void MainWindow::scheduleProfileLoadRecovery(const QString& profileType, }); QTimer::singleShot(kProfileLoadDeferredPanFlushDelayMs, this, [this]() { flushPendingProfileLoadPanDimensions(); + // Ordered after dimensions on the SAME timer — no fourth scheduler. + // Bin size is bandwidth/xpixels, so the pan must know its pixel geometry + // before it is recentered. This timer is the first flush point past hold + // expiry; the two earlier dimension flushes (1500/3500 ms) run INSIDE the + // hold, where xpixels/ypixels are exempt but a center write is not. + // flushPendingProfileLoadPanCenters() re-checks the hold itself and + // early-returns while it is armed, so no center can escape early even if + // this call site were moved. (#4142) + m_radioModel.flushPendingProfileLoadPanCenters(); }); QTimer::singleShot(kProfileLoadPostHoldRecoveryDelayMs, this, [this]() { m_profileLoadPendingFftYpixels.clear(); @@ -2411,13 +2428,13 @@ void MainWindow::wirePanadapter(PanadapterApplet* applet) } if (auto* pan = m_radioModel.panadapter(applet->panId())) { center = std::max(center, pan->bandwidthMhz() / 2.0); - // The radio may acknowledge this command without echoing a display - // status to the setting client. Keep the canonical model aligned - // with the visible pan so TCI dds: follows the IQ center. - pan->setCenterBandwidth(center, -1.0); } - m_radioModel.sendCommand( - QString("display pan set %1 center=%2").arg(applet->panId()).arg(center, 0, 'f', 6)); + // The radio may acknowledge this command without echoing a display status + // to the setting client, so requestPanCenter() keeps the canonical model + // aligned with the wire command (TCI dds: follows this center) — and + // during a profile load it defers rather than letting sendCmd drop it + // silently (#4142). + m_radioModel.requestPanCenter(applet->panId(), center); }); // Band/Segment Zoom toggle off the pan's radio-authoritative model state // (togglePanZoomModeForPan) — shared with the keyboard/MIDI shortcuts @@ -3214,9 +3231,11 @@ void MainWindow::wirePanadapter(PanadapterApplet* applet) // In-window drag moves pass the unchanged center purely to tune // without reveal — only emit the pan command when it actually moves. if (!qFuzzyCompare(centerMhz, pan->centerMhz())) { - m_radioModel.sendCommand( - QString("display pan set %1 center=%2") - .arg(pan->panId()).arg(centerMhz, 0, 'f', 6)); + // Deferred rather than dropped during a profile load (#4142). + // The SpectrumWidget owns its own view during an edge-pan drag, + // so the gesture still tracks the cursor; the radio catches up + // when the deferred center flushes. + m_radioModel.requestPanCenter(pan->panId(), centerMhz); } } queueActiveSliceForSpectrumTarget(target->sliceId()); @@ -3665,10 +3684,12 @@ MainWindow::TuneCenteringResult MainWindow::revealFrequencyIfNeeded( ? kPanFollowAnimationDurationMs : 0; - pan->setCenterBandwidth(result.newCenterMhz, -1.0); // aetherd RFC 2.3 - m_radioModel.sendCommand( - QString("display pan set %1 center=%2") - .arg(pan->panId()).arg(result.newCenterMhz, 0, 'f', 6)); + // #4142 — defer, never drop. During a profile load the pan center write + // is suppressed at the wire; advancing the local center anyway is what + // made the waterfall go black (the view claimed a center the radio never + // took). requestPanCenter() applies the model update only when the + // command actually goes out, and replays it once the hold lifts. + m_radioModel.requestPanCenter(pan->panId(), result.newCenterMhz); return result; } @@ -3752,13 +3773,13 @@ MainWindow::TuneCenteringResult MainWindow::revealFrequencyIfNeeded( ? kPanFollowAnimationDurationMs : 0; - // Apply the center optimistically so the owning spectrum repaints - // immediately; SpectrumWidget decides whether that becomes a short - // retargetable animation or an immediate snap based on shift size. - pan->setCenterBandwidth(result.newCenterMhz, -1.0); // aetherd RFC 2.3 - m_radioModel.sendCommand( - QString("display pan set %1 center=%2") - .arg(pan->panId()).arg(result.newCenterMhz, 0, 'f', 6)); + // Apply the center so the owning spectrum repaints immediately; + // SpectrumWidget decides whether that becomes a short retargetable animation + // or an immediate snap based on shift size. During a profile load this + // defers instead (#4142) — the model does not advance ahead of the radio, so + // the pan keeps showing truthful spectrum for its real span until the + // deferred center flushes. + m_radioModel.requestPanCenter(pan->panId(), result.newCenterMhz); return result; } diff --git a/src/models/ProfileLoadCommand.h b/src/models/ProfileLoadCommand.h index 7f2115056..9062688b0 100644 --- a/src/models/ProfileLoadCommand.h +++ b/src/models/ProfileLoadCommand.h @@ -1,7 +1,9 @@ #pragma once +#include #include #include +#include #include namespace AetherSDR { @@ -44,4 +46,42 @@ inline ProfileLoadCommand parseProfileLoadCommand(const QString& command) }; } +// Classifies a command as a write to radio state that a profile load owns and +// will rebuild. RadioModel::sendCmd() consults this as a low-level backstop and +// suppresses matches while the profile-load hold is armed. +// +// Callers must not rely on that backstop to swallow user-initiated writes: a +// suppressed command never reaches the wire and is lost. Route pan center / +// bandwidth writes through RadioModel::requestPanCenter(), which defers and +// coalesces them until the hold lifts instead of dropping them (#4142). +inline bool isProfileOwnedRadioStateWrite(const QString& command) +{ + const QString trimmed = command.trimmed(); + const QStringList tokens = trimmed.simplified().split(QLatin1Char(' '), Qt::SkipEmptyParts); + if (tokens.size() >= 5 + && tokens[0] == QStringLiteral("display") + && tokens[1] == QStringLiteral("pan") + && tokens[2] == QStringLiteral("set")) { + bool hasPixelDimension = false; + for (int i = 4; i < tokens.size(); ++i) { + const QString key = tokens[i].section(QLatin1Char('='), 0, 0); + if (key != QStringLiteral("xpixels") && key != QStringLiteral("ypixels")) { + return true; + } + hasPixelDimension = true; + } + // xpixels/ypixels are allowed past this low-level guard because + // MainWindow queues them during a profile load and flushes them after + // the radio accepts the profile. Do not bypass + // MainWindow::requestPanDimensionsForRadio(); early dimension writes + // can cause the radio to save a partial GUIClient slice layout. + return !hasPixelDimension; + } + + return trimmed.startsWith(QStringLiteral("slice set ")) + || trimmed.startsWith(QStringLiteral("display pan set ")) + || trimmed.startsWith(QStringLiteral("display panafall set ")) + || trimmed.startsWith(QStringLiteral("display waterfall set ")); +} + } // namespace AetherSDR diff --git a/src/models/RadioModel.cpp b/src/models/RadioModel.cpp index 3691ea348..7fc8604e8 100644 --- a/src/models/RadioModel.cpp +++ b/src/models/RadioModel.cpp @@ -103,35 +103,9 @@ bool statusFlagSet(const QMap& kvs, const QString& key) || value.compare(QStringLiteral("true"), Qt::CaseInsensitive) == 0; } -bool isProfileOwnedRadioStateWrite(const QString& command) -{ - const QString trimmed = command.trimmed(); - const QStringList tokens = trimmed.simplified().split(QLatin1Char(' '), Qt::SkipEmptyParts); - if (tokens.size() >= 5 - && tokens[0] == QStringLiteral("display") - && tokens[1] == QStringLiteral("pan") - && tokens[2] == QStringLiteral("set")) { - bool hasPixelDimension = false; - for (int i = 4; i < tokens.size(); ++i) { - const QString key = tokens[i].section(QLatin1Char('='), 0, 0); - if (key != QStringLiteral("xpixels") && key != QStringLiteral("ypixels")) { - return true; - } - hasPixelDimension = true; - } - // xpixels/ypixels are allowed past this low-level guard because - // MainWindow queues them during a profile load and flushes them after - // the radio accepts the profile. Do not bypass - // MainWindow::requestPanDimensionsForRadio(); early dimension writes - // can cause the radio to save a partial GUIClient slice layout. - return !hasPixelDimension; - } - - return trimmed.startsWith(QStringLiteral("slice set ")) - || trimmed.startsWith(QStringLiteral("display pan set ")) - || trimmed.startsWith(QStringLiteral("display panafall set ")) - || trimmed.startsWith(QStringLiteral("display waterfall set ")); -} +// isProfileOwnedRadioStateWrite() moved to ProfileLoadCommand.h so the +// classification contract has a light, dependency-free test target +// (profile_load_command_test). Behaviour unchanged. (#4142) void appendUniqueAntennaToken(QStringList& tokens, const QString& token) { @@ -2755,11 +2729,115 @@ void RadioModel::setPanCenter(double centerMhz) // out-of-range center would be optimistically stored and advertised via // TCI dds: even though the radio rejects it. centerMhz = std::max(centerMhz, pan->bandwidthMhz() / 2.0); - pan->setCenterBandwidth(centerMhz, -1.0); } - sendCmd( - QString("display pan set %1 center=%2") - .arg(m_activePanId).arg(centerMhz, 0, 'f', 6)); + requestPanCenter(m_activePanId, centerMhz); +} + +bool RadioModel::requestPanCenter(const QString& panId, + double centerMhz, + double bandwidthMhz) +{ + if (panId.isEmpty()) { + return false; + } + + // A pan center is profile-owned radio state, so sendCmd() would DROP this + // while a profile load is rebuilding the radio's topology. Defer it instead: + // queue the REQUESTED value and replay it once the hold lifts. + // + // Gate on exactly the predicate sendCmd() guards on, so "if sendCmd would + // drop it, we queue it" is an identity rather than an approximation — two + // separate clocks could skew and re-open the same silent drop. + if (profileLoadRadioStateWritesHeld()) { + PendingPanCenter& pending = m_pendingProfileLoadPanCenters[panId]; + pending.centerMhz = centerMhz; + if (bandwidthMhz > 0.0) { + pending.bandwidthMhz = bandwidthMhz; + } + + // Deliberately do NOT touch PanadapterModel here. The optimistic local + // update is the second half of #4142: with the command dropped, a client + // that advanced its own center claimed a center the radio never took. + // Honest VITA-49 tiles (each carrying its own FrameLowFreq/BinBandwidth) + // were then projected into a view that lied about its span, and the + // non-overlapping region rendered black — permanently, into history. + // Local state may only advance when a command actually reaches the wire. + qCDebug(lcProtocol).noquote() + << "RadioModel: deferring pan center during profile load" + << QStringLiteral("pan=%1").arg(panId) + << QStringLiteral("center=%1").arg(centerMhz, 0, 'f', 6); + return false; + } + + sendPanCenterToRadio(panId, centerMhz, bandwidthMhz); + return true; +} + +void RadioModel::sendPanCenterToRadio(const QString& panId, + double centerMhz, + double bandwidthMhz) +{ + if (PanadapterModel* pan = panadapter(panId)) { + // Keep the canonical model aligned with the command we are about to put + // on the wire: the radio may ACK without echoing a display status back + // to the setting client, and TCI dds: follows this center. + pan->setCenterBandwidth(centerMhz, bandwidthMhz > 0.0 ? bandwidthMhz : -1.0); + } + + // Center and bandwidth must travel together when both are requested; + // splitting them produced the P1/P2 waterfall-loss and zoom-drift bugs. + const QString command = bandwidthMhz > 0.0 + ? QString("display pan set %1 center=%2 bandwidth=%3") + .arg(panId) + .arg(centerMhz, 0, 'f', 6) + .arg(bandwidthMhz, 0, 'f', 6) + : QString("display pan set %1 center=%2") + .arg(panId) + .arg(centerMhz, 0, 'f', 6); + + sendCommand(command); +} + +void RadioModel::flushPendingProfileLoadPanCenters() +{ + if (m_pendingProfileLoadPanCenters.isEmpty() || !isConnected()) { + return; + } + + // THE NON-REGRESSION PROOF, in one branch: while the hold is armed this + // returns without sending, so a deferred pan center can never put a byte on + // the wire inside the hold window. Nothing here can reintroduce the + // missing-slices corruption the hold exists to prevent. + // + // Returning WITHOUT clearing is deliberate. The hold can only still be armed + // because a further topology-rebuilding profile load re-armed it — and that + // load scheduled its own post-hold flush, which will replay these. Dropping + // them here would be the exact bug this method exists to fix. + if (profileLoadRadioStateWritesHeld()) { + return; + } + + const QHash pending = + std::exchange(m_pendingProfileLoadPanCenters, {}); + + int sent = 0; + for (auto it = pending.cbegin(); it != pending.cend(); ++it) { + // Re-resolve against the LIVE pan topology. A pan that the profile load + // removed — or replaced under a new id — simply never matches, so a + // stale center can never be applied to a pan that no longer exists. + if (!panadapter(it.key())) { + continue; + } + + sendPanCenterToRadio(it.key(), it.value().centerMhz, it.value().bandwidthMhz); + ++sent; + } + + if (sent > 0) { + qCDebug(lcProtocol).noquote() + << "RadioModel: flushed deferred profile-load pan centers" + << QStringLiteral("count=%1").arg(sent); + } } void RadioModel::setPanDbmRange(float minDbm, float maxDbm) @@ -4850,7 +4928,12 @@ quint32 RadioModel::sendCmd(const QString& command, ResponseCallback cb) if (!profileLoad.valid && profileLoadRadioStateWritesHeld() && isProfileOwnedRadioStateWrite(command)) { - qCDebug(lcProtocol).noquote() + // Defense-in-depth backstop only. A command that reaches here is + // DROPPED — it never gets a sequence number and never reaches the wire. + // Callers that carry user intent must defer instead (see + // requestPanCenter()), so anything still landing here is a bug we want + // to see rather than silently swallow. (#4142) + qCWarning(lcProtocol).noquote() << "RadioModel: suppressing profile-load radio-state write" << command; if (cb) { diff --git a/src/models/RadioModel.h b/src/models/RadioModel.h index 4e0a3abb6..0850e516f 100644 --- a/src/models/RadioModel.h +++ b/src/models/RadioModel.h @@ -444,6 +444,39 @@ class RadioModel : public QObject { void setPanBandwidth(double bandwidthMhz); void setPanCenter(double centerMhz); void setPanDbmRange(float minDbm, float maxDbm); + + // #4142 — the ONLY supported way to write a pan center (and, optionally, a + // coupled bandwidth) to the radio. + // + // `display pan set center=…` is classified as a profile-owned radio + // state write, so sendCmd() DROPS it while the profile-load hold is armed: + // it returns before a sequence number is allocated and the command never + // reaches the wire. A user action that lands in that window (typed + // frequency, zoom, drag, ATU sweep, automation) was silently lost, and the + // client kept its optimistic center — leaving the pan permanently claiming + // a center the radio never took. + // + // requestPanCenter() defers instead of dropping: while the hold is armed it + // coalesces the request per pan (last-write-wins) and returns false WITHOUT + // advancing local model state, so the client never claims a center the radio + // does not have. flushPendingProfileLoadPanCenters() replays it once the + // hold lifts. + // + // Pass bandwidthMhz > 0 to set center and bandwidth coherently in one + // command (zoom paths must never split the pair); pass <= 0 to leave the + // radio's bandwidth untouched. + // + // Returns true if the command reached the wire, false if it was deferred. + // Callers that also advance view state optimistically must gate that on the + // return value, or they will re-create the black-waterfall divergence. + bool requestPanCenter(const QString& panId, + double centerMhz, + double bandwidthMhz = -1.0); + + // Replays deferred pan-center writes. Hard invariant: this early-returns + // while the hold is armed, so no deferred center can reach the wire inside + // the hold window. + void flushPendingProfileLoadPanCenters(); void setPanWnb(bool on); void setPanWnbLevel(int level); void setPanRfGain(int gain); @@ -683,6 +716,12 @@ private slots: bool m_wfAutoBlackOn{true}; // mirrors the client auto-black on/off bool m_wfAutoBlackRadioSide{false}; // false = client-side, true = radio-side bool profileLoadRadioStateWritesHeld() const; + // Raw sender for a pan center (+ optional coupled bandwidth). Applies the + // local model update and puts the command on the wire together, so the two + // can never diverge. Everything else must go through requestPanCenter(). + void sendPanCenterToRadio(const QString& panId, + double centerMhz, + double bandwidthMhz); void registerAsGuiClient(const QString& clientId); void disconnectPendingClientsThen(std::function continuation); // LAN-only: subscribe to radio+client topics early, wait 400 ms for @@ -1054,6 +1093,19 @@ private slots: int m_rttyMarkDefault{2125}; quint32 m_txClientHandle{0}; // handle of the client that owns TX qint64 m_profileLoadRadioStateWriteHoldUntilMs{0}; + + // #4142 — pan-center writes deferred while the profile-load hold is armed. + // bandwidthMhz <= 0 means "center only; leave the radio's bandwidth alone". + struct PendingPanCenter { + double centerMhz{0.0}; + double bandwidthMhz{-1.0}; + }; + // panId → last requested center. Coalesced last-write-wins, but field-wise: + // a later center-only request must NOT erase a bandwidth an earlier zoom + // asked for, or that zoom's bandwidth would be the very thing we set out to + // stop dropping. + QHash m_pendingProfileLoadPanCenters; + QMap m_clientInfoMap; // handle → full client info std::function m_multiFlexContinuation; // saved continuation during conflict pause QMap m_clientStations; // handle → station name (legacy, kept in sync) diff --git a/tests/profile_load_command_test.cpp b/tests/profile_load_command_test.cpp index af444c462..144394e4b 100644 --- a/tests/profile_load_command_test.cpp +++ b/tests/profile_load_command_test.cpp @@ -61,5 +61,65 @@ int main() check(kProfileLoadPostHoldRecoveryDelayMs > kProfileLoadDeferredPanFlushDelayMs, "post-hold recovery runs after deferred pan flush"); + // ── isProfileOwnedRadioStateWrite() — the classification contract (#4142) ── + // + // sendCmd() DROPS every command this returns true for while the profile-load + // hold is armed: it returns before a sequence number is allocated, so the + // command never reaches the wire. These cases pin down exactly which writes + // are lost, and therefore which ones MUST be deferred by + // RadioModel::requestPanCenter() rather than handed to sendCmd() and hoped for. + + // A pan center is profile-owned — this is the write that #4142 lost. Direct + // frequency entry emits an atomic pair; the `slice tune` half survives and the + // `center=` half was silently dropped, so the slice retuned and the pan did not. + check(isProfileOwnedRadioStateWrite( + QStringLiteral("display pan set 0x40000000 center=7.086000")), + "pan center write is profile-owned (the #4142 drop)"); + check(isProfileOwnedRadioStateWrite( + QStringLiteral("display pan set 0x40000000 center=7.086000 bandwidth=0.200000")), + "coupled pan center+bandwidth write is profile-owned (zoom/drag also dropped)"); + check(isProfileOwnedRadioStateWrite( + QStringLiteral("display pan set 0x40000000 bandwidth=0.200000")), + "pan bandwidth write is profile-owned"); + + // xpixels/ypixels are the ONE exemption, and only because the client alone + // knows its pixel geometry — the display is broken until they are sent. + // MainWindow defers and coalesces them (requestPanDimensionsForRadio); this + // exemption is a necessity, NOT evidence that early pan writes are safe. + check(!isProfileOwnedRadioStateWrite( + QStringLiteral("display pan set 0x40000000 xpixels=1920 ypixels=800")), + "pan pixel dimensions are exempt (client-owned display geometry)"); + check(!isProfileOwnedRadioStateWrite( + QStringLiteral("display pan set 0x40000000 xpixels=1920")), + "xpixels alone is exempt"); + + // A single non-pixel field anywhere in the argument list re-arms the guard: + // the exemption is all-or-nothing, so center can never ride in on a dimension + // write's coat-tails. + check(isProfileOwnedRadioStateWrite( + QStringLiteral("display pan set 0x40000000 xpixels=1920 center=7.086000")), + "pixel dimensions mixed with a center write are NOT exempt"); + + // Reads are never suppressed — only writes. + check(!isProfileOwnedRadioStateWrite(QStringLiteral("display pan info 0x40000000")), + "pan info read is not a profile-owned write"); + check(!isProfileOwnedRadioStateWrite(QStringLiteral("sub slice all")), + "subscription is not a profile-owned write"); + + // `slice tune` is NOT profile-owned: it is the half of the typed-frequency + // pair that survived the hold. That asymmetry is the bug — the slice moved + // and the pan could not follow. + check(!isProfileOwnedRadioStateWrite(QStringLiteral("slice tune 0 7.086000 autopan=0")), + "slice tune survives the hold (the surviving half of the #4142 pair)"); + check(isProfileOwnedRadioStateWrite(QStringLiteral("slice set 0 rfgain=20")), + "slice set write is profile-owned"); + + check(isProfileOwnedRadioStateWrite( + QStringLiteral("display panafall set 0x40000000 center=7.086000")), + "panafall write is profile-owned"); + check(isProfileOwnedRadioStateWrite( + QStringLiteral("display waterfall set 0x42000000 color_gain=50")), + "waterfall write is profile-owned"); + return failures == 0 ? 0 : 1; } From 8d24871970af6d82f17a20d5e2e9022c11c41a52 Mon Sep 17 00:00:00 2001 From: Ozy K6OZY Date: Sun, 12 Jul 2026 18:30:42 -0700 Subject: [PATCH 2/2] fix(spectrum): never strand a deferred pan write when the radio hangs (#4142) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the defer-never-drop change, found by live 6700 testing. RadioModel arms the profile-load hold when the profile command is SENT (RadioModel.cpp), but MainWindow::scheduleProfileLoadRecovery() — which schedules every flush timer, including the deferred pan-center replay — runs on profileLoadCompleted, which is emitted on ACK ONLY. Those two are not equivalent. Loading an 8-pan/8-slice global profile on a real FLEX-6700 stalls the radio for 4-5 s; it misses five consecutive pings, the client force-disconnects, and the profile load is NEVER ACKed (5/5 runs). In that case the hold armed but the replay was never scheduled, so a pan center deferred during the load would have been stranded forever — turning a dropped user command into a lost one. No unit test could have caught this; it took real hardware. Fix, as a stronger invariant: whoever DEFERS a write owns scheduling its replay. armProfileLoadPanCenterFlush() is called from the defer path itself, never from the ACK path, and re-arms itself because the hold can be extended after arming (by the ACK, or by a second profile load). onDisconnected() now VOIDS the queue loudly rather than stranding it: the session those requests belonged to is gone, the radio rebuilds its own topology on reconnect, and replaying a pre-disconnect center could land a stale frequency on a pan that is no longer the same pan. Also narrows the sendCmd() suppression warning. Escalating the whole backstop to qCWarning cried wolf: active-slice reasserts, TX-slice reasserts and panafall/waterfall auto-black defaults are model-echo writers that #3563 suppresses BY DESIGN, and four of them fire on every profile load. Warn only for pan geometry (center=/bandwidth=), which is the class now routed through requestPanCenter() — one of those reaching the backstop means a caller bypassed the defer path and a user command is being lost. Co-authored-by: Don @ cloaked.agency --- src/models/RadioModel.cpp | 94 ++++++++++++++++++++++++++++++++++----- src/models/RadioModel.h | 4 ++ 2 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/models/RadioModel.cpp b/src/models/RadioModel.cpp index 7fc8604e8..e1bc672db 100644 --- a/src/models/RadioModel.cpp +++ b/src/models/RadioModel.cpp @@ -2766,6 +2766,16 @@ bool RadioModel::requestPanCenter(const QString& panId, << "RadioModel: deferring pan center during profile load" << QStringLiteral("pan=%1").arg(panId) << QStringLiteral("center=%1").arg(centerMhz, 0, 'f', 6); + + // Whoever defers a write owns scheduling its replay. Do NOT rely on the + // profile-load ACK to schedule the flush: the hold is armed when the + // profile-load command is SENT, but MainWindow's recovery pass (and its + // flush timers) only runs on profileLoadCompleted, which is emitted on + // ACK. A large topology (8 pans / 8 slices, verified on a 6700) can stall + // the radio long enough that it misses pings and the client force- + // disconnects BEFORE the ACK ever arrives — in which case no flush would + // ever have been scheduled and this request would be stranded forever. + armProfileLoadPanCenterFlush(); return false; } @@ -2798,9 +2808,38 @@ void RadioModel::sendPanCenterToRadio(const QString& panId, sendCommand(command); } +void RadioModel::armProfileLoadPanCenterFlush() +{ + // Re-arm for exactly when the hold lifts. The hold can be EXTENDED after we + // arm (the ACK pushes it out again, and a second profile load pushes it out + // further), so the flush re-checks and re-arms itself rather than trusting a + // single deadline computed up front. + const qint64 remainingMs = + m_profileLoadRadioStateWriteHoldUntilMs - QDateTime::currentMSecsSinceEpoch(); + const int delayMs = static_cast(std::max(remainingMs, 0)) + 100; + + QTimer::singleShot(delayMs, this, [this]() { + flushPendingProfileLoadPanCenters(); + }); +} + void RadioModel::flushPendingProfileLoadPanCenters() { - if (m_pendingProfileLoadPanCenters.isEmpty() || !isConnected()) { + if (m_pendingProfileLoadPanCenters.isEmpty()) { + return; + } + + if (!isConnected()) { + // The session these requests belonged to is gone. Their pan ids and the + // user's intent both died with it, and the radio will rebuild its own + // topology on reconnect — replaying a pre-disconnect center could land a + // stale frequency on a pan that is no longer the same pan. Void them + // loudly rather than stranding or misapplying them. (onDisconnected() + // normally clears these first; this is the backstop.) + qCWarning(lcProtocol).noquote() + << "RadioModel: discarding" << m_pendingProfileLoadPanCenters.size() + << "deferred pan center(s) — disconnected before the profile load settled"; + m_pendingProfileLoadPanCenters.clear(); return; } @@ -2809,11 +2848,12 @@ void RadioModel::flushPendingProfileLoadPanCenters() // the wire inside the hold window. Nothing here can reintroduce the // missing-slices corruption the hold exists to prevent. // - // Returning WITHOUT clearing is deliberate. The hold can only still be armed - // because a further topology-rebuilding profile load re-armed it — and that - // load scheduled its own post-hold flush, which will replay these. Dropping - // them here would be the exact bug this method exists to fix. + // Returning WITHOUT clearing is deliberate — we re-arm instead. The hold is + // still armed only because a further topology-rebuilding profile load pushed + // it out; dropping the request here would be the exact bug this method exists + // to fix. if (profileLoadRadioStateWritesHeld()) { + armProfileLoadPanCenterFlush(); return; } @@ -3812,6 +3852,19 @@ void RadioModel::onDisconnected() { qCDebug(lcProtocol) << "RadioModel: disconnected"; + // #4142 — void any pan centers deferred during a profile load. The session + // they belonged to is gone: the radio rebuilds its topology on reconnect, so + // a queued center could land a stale frequency on a pan that is no longer the + // same pan. This is a real path, not a theoretical one — a large profile + // (8 pans / 8 slices on a 6700) can stall the radio past the ping timeout and + // force a disconnect BEFORE the profile load is ever ACKed. + if (!m_pendingProfileLoadPanCenters.isEmpty()) { + qCWarning(lcProtocol).noquote() + << "RadioModel: discarding" << m_pendingProfileLoadPanCenters.size() + << "deferred pan center(s) — disconnected before the profile load settled"; + m_pendingProfileLoadPanCenters.clear(); + } + // Release sleep inhibition on disconnect (#1420) m_sleepInhibitor.release(); @@ -4930,12 +4983,31 @@ quint32 RadioModel::sendCmd(const QString& command, ResponseCallback cb) && isProfileOwnedRadioStateWrite(command)) { // Defense-in-depth backstop only. A command that reaches here is // DROPPED — it never gets a sequence number and never reaches the wire. - // Callers that carry user intent must defer instead (see - // requestPanCenter()), so anything still landing here is a bug we want - // to see rather than silently swallow. (#4142) - qCWarning(lcProtocol).noquote() - << "RadioModel: suppressing profile-load radio-state write" - << command; + // + // Warn only for pan geometry, which is the class routed through + // requestPanCenter(): one of those arriving here means a caller bypassed + // the defer path and a user command is being lost — a real bug (#4142). + // + // The other suppressions on this path are model-echo/reconcile writers + // that #3563 drops BY DESIGN (active-slice and TX-slice reasserts, + // panafall/waterfall auto-black defaults). Several fire on every profile + // load, so warning on them would cry wolf and bury the one line that + // actually means something. + const bool panGeometryWrite = + command.startsWith(QStringLiteral("display pan set ")) + && (command.contains(QStringLiteral("center=")) + || command.contains(QStringLiteral("bandwidth="))); + + if (panGeometryWrite) { + qCWarning(lcProtocol).noquote() + << "RadioModel: DROPPED a pan geometry write during profile load —" + << "this should have been deferred via requestPanCenter()" + << command; + } else { + qCDebug(lcProtocol).noquote() + << "RadioModel: suppressing profile-load radio-state write" + << command; + } if (cb) { cb(kProfileLoadSuppressedCommandCode, QStringLiteral("suppressed during profile load")); diff --git a/src/models/RadioModel.h b/src/models/RadioModel.h index 0850e516f..c9bbc33c8 100644 --- a/src/models/RadioModel.h +++ b/src/models/RadioModel.h @@ -722,6 +722,10 @@ private slots: void sendPanCenterToRadio(const QString& panId, double centerMhz, double bandwidthMhz); + // Schedules the deferred-pan-center replay for when the hold lifts. Armed by + // the act of deferring, NOT by the profile-load ACK — a large topology can + // stall the radio into a disconnect before it ever ACKs. (#4142) + void armProfileLoadPanCenterFlush(); void registerAsGuiClient(const QString& clientId); void disconnectPendingClientsThen(std::function continuation); // LAN-only: subscribe to radio+client topics early, wait 400 ms for