fix(gui): stop baking optimistic zoom guesses into waterfall history - #5142
fix(gui): stop baking optimistic zoom guesses into waterfall history#5142tropo1234 wants to merge 5 commits into
Conversation
Every waterfall history row is permanently stamped with the frequency/ bandwidth it was captured at, but the four zoom gestures wrote their locally-computed, unconfirmed target straight into m_centerMhz/ m_bandwidthMhz for instant visual feedback before the backend echoed back. Any row captured in that window got a permanently wrong stamp that never self-corrected until it aged out of the ring buffer. Add m_confirmedCenterMhz/m_confirmedBandwidthMhz, updated only when setFrequencyRangeInternal() sees a real backend echo, and use that pair (not the on-screen guess) everywhere a row's pixels and stamp are built: updateWaterfallRow(), pushWaterfallRow() (Legacy/2D — pixel layout and stamp must agree with each other or the wrong signal shows at the wrong frequency), pushKiwiSdrWaterfallRow(), and appendHistoryRow()'s own fallback (now stampFrameForHistoryRow(), unit tested). Gesture handlers are untouched, so click/drag/pinch/wheel responsiveness is unchanged. Bench-verified on ANAN-G2: the newly-exposed edge during a zoom's multi-second round trip is now an honest, transient black region that fills in and never reappears, instead of a permanent scar. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
I could not find a linked issue — the body has no fixes/closes #NNNN and a search turns up nothing but this PR itself. So I reviewed against the PR's own stated intent: history rows must not be permanently stamped with an on-screen zoom guess the backend never confirmed. The premise is real and well-grounded — MainWindow_Wiring.cpp:696 and MainWindow_KiwiSdr.cpp:1321 both carry the "a lying frame … bakes black rows into waterfall history (#4142)" precedent the header comment cites, and RadioModel::requestPanCenter()'s -1.0 bandwidth argument does exist (MainWindow_Wiring.cpp:3764). That citation checks out.
The native tile path (updateWaterfallRow) is genuinely fixed and internally coherent — the destination window and the stamp both move to confirmed geometry, the source stays the tile's own real bounds, and the supplemental frame is correctly left alone. That hunk is the good part of this PR.
The FFT-derived path (pushWaterfallRow) is not, and the mechanism feeding m_confirmed* records values the widget itself has classified as untrustworthy. Details in blockers 1 and 2. Per GOVERNANCE.md a bug fix with a clear root cause doesn't need an RFC first, so no process objection — but this is a new piece of view-layer state and it deserves a stated repro.
Scope
| File / group | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
src/gui/SpectrumPreviewLogic.h |
Extracts the existing stamp ternary into stampFrameForHistoryRow() |
Yes | In scope |
src/gui/SpectrumWidget.h |
Adds m_confirmedCenterMhz/m_confirmedBandwidthMhz + doc comment |
Yes | In scope |
src/gui/SpectrumWidget.cpp |
Sets confirmed pair in setFrequencyRangeInternal; switches 4 read sites |
Yes | In scope (correctness issues below, not scope) |
tests/spectrum_preview_logic_test.cpp |
New testStampFrameForHistoryRow(), wired into main() |
Yes | In scope (coverage gap below) |
Everything in the diff is explained by the stated fix. No CHANGELOG.md entry (correct — don't add one), no new public/protocol surface, no settings keys, no UI defaults changed, nothing deleted that guarded a named symptom. CodeGuard reported nothing on any of the four changed files; all ten of its findings are in prototypes/, tools/, MidiSettings.cpp, RigctlProtocol.cpp and MainWindow.cpp, none of which this PR touches — nothing to fold in.
Blockers
-
m_confirmed*is written before the three guards that exist precisely to reject untrustworthy geometry —SpectrumWidget.cpp:7484-7485, inline comment below. The assignment is unconditionally first in the function, so the drag-hold return (7498), the settle-pending return (7507-7513) and the stale-echo return (7534-7539) all record their rejected value as "confirmed truth" before returning. The comment sitting directly above the assignment claims the opposite in so many words: "The three OTHER early returns below (drag-hold, settle-pending, stale-echo) do NOT get this treatment." They do. That's not just a stale comment — the settle-pending guard's own text says Flex "can echo older center-only statuses after a combined center+bandwidth command", and those are now the coordinates every subsequentappendHistoryRow()andupdateWaterfallRow()uses to lay out and label rows. A fix is to move the pair to the point past all holds (just afterm_deferredRangeValid = false;at 7517) — or, if the intent really is "record even when the no-op guard fires", to record before only the equality guard and re-record after the holds. -
pushWaterfallRow()now labels the same pixels three different ways, and the bins are not in the confirmed frame —SpectrumWidget.cpp:12229and:12238, inline comments below. In one call: the history row is stamped confirmed (12265 → the new fallback at 5451-5454), the visible row is stamped on-screen (12268 →appendVisibleRow's unchanged fallback at5372-5374), and the DSS row resolves to preview-base-or-on-screen (12266 →resolvedUntaggedDssFrame, 5233-5250). Before this PR all three agreed onm_centerMhz. Worse, the bins themselves live in the on-screen frame by construction:reprojectSpectrum()(7303-7418) resamplesm_binsinto the new optimistic frame at gesture time, andupdateSpectrum()(8212-8292) stores frameless incoming bins as the current display frame. So during the divergence window a TX-mask / RX-stale fallback row's pixels span the guessed frame while its history stamp says the confirmed frame —remapHistoryRowInto()will shift it, and the row moves horizontally the moment the viewport rebuilds. That is the "wrong signal at the wrong frequency, not just a black gap" outcome this PR's own comment at 8517-8520 warns about, arriving in the one path where the diff doesn't control the pixel layout. Two sub-notes: the inline comment at 12226 ("the pixel layout here must use the same values") is inaccurate —panStartMhzin this function feeds only the TX mask; the bin→column mapping at 12245-12258 is proportional and ignores it entirely. And that TX mask is the concrete casualty: 12236-12240 now tests confirmed-frame frequencies against columns laid out in the on-screen frame, so the blackout covers the wrong columns during any zoom that hasn't landed.
Nits (non-blocking)
- The test would pass against unfixed
main.stampFrameForHistoryRow()is a byte-for-byte extraction of the ternary that was already atSpectrumWidget.cpp:5452-5454; the new test pins the extraction, not the behavior change. Nothing in the suite covers "a zoom guess is not stamped into history" — the entirem_confirmed*mechanism is untested. The three invalid-frame cases are a nice touch and matchFrequencyFrame::isValid()(finite, both> 0.0), so keep them, but consider a case that would fail ifm_confirmed*were replaced bym_centerMhzagain. - A fourth notion of "the frame."
m_frequencyPreviewBaseCenterMhz/…BandwidthMhz(set byupdateFrequencyPreview(), 6218-6254) already captures the pre-gesture frame for exactly this window, and the DSS path already consumes it viaresolvedUntaggedDssFrame(). Reusing that would have kept the 2D and DSS paths on one answer instead of two that can disagree (preview base is captured once at gesture start;m_confirmed*tracks every echo, including rejected ones). - The default initializers
{14.225}/{0.200}duplicatem_centerMhz/m_bandwidthMhz's literals two lines up; if one is ever retuned they'll silently drift apart.
What I tried to break (and didn't)
updateWaterfallRow()self-consistency. Walked 8513-8700: destination window (8521, 8527), primary stamp (8572), blanker substitution (waterfallBlankerFrameBundleForOutput, 8611-8616),appendHistoryRow(8674-8681) andappendVisibleRow(8691-8698) all carryoutputFrames.primaryFrameexplicitly, so theappendHistoryRowfallback never fires here and layout/stamp/visible agree. The supplemental frame stays derived fromlowFreqMhz/highFreqMhzas the comment claims. This hunk survives.pushKiwiSdrWaterfallRow()'s changed fallback. Its stamp is used, and the visible line is explicitly remapped fromrowCenter/rowBandwidthintom_centerMhz/m_bandwidthMhz(12324-12330), so unlike blocker 2 the two stay consistent. The fallback branch only fires when the Kiwi stream supplies no row frame; the injection path at 1668-1672 passes an explicit one. Clean.FrequencyFrame::isValid()against the new helper. NaN center, zero and negative bandwidth all fall through toconfirmedas the test asserts; aconfirmedpair that is itself invalid (e.g. bandwidth 0 before the first geometry push) writes a zero stamp, but that's the pre-existing behavior of the ternary this replaced, not a regression.- Sibling call sites. Grepped every writer of
m_centerMhz/m_bandwidthMhz(2251, 7593, 7643/7648, 9905, 10339, 10473, 10742, 11438, 11618) and everyappendHistoryRow()caller. The four zoom handlers do write optimistically exactly as the header comment claims. The pan-drag (10473), edge-pan (10339) and "Center Slice" (9905) writers also advance the display without confirmation and are not covered by the header comment's "four zoom gesture handlers" framing — worth correcting in the comment, though the mechanism handles them the same way. - CI. All four checks green on
1145b535. Note thatctestinci.ymlis-R-filtered, so green here means the named subset passed on this merge base, not the suite.
Everything above is reasoned from reading the head checkout at /tmp/aetherclaude/pr-5142 — I'm headless, with no build and no way to drive the GUI, so none of it is reproduced at runtime. Blocker 2 in particular is a code-path argument; if you can show that pushWaterfallRow()'s bins are already in the confirmed frame when it runs, it collapses and I'd like to see that.
Recommendation
Request changes. The native-tile half of this is a real fix and I'd take it as-is. The two blockers are both in the other half: the confirmed pair is recorded from values the widget deliberately rejects (blocker 1, and the comment asserts the opposite of what the code does), and pushWaterfallRow() ends up with its data in one frame and its stamp in another (blocker 2). Concrete next step: move the m_confirmed* assignment past the three holds, and either revert the two pushWaterfallRow() hunks to m_centerMhz/m_bandwidthMhz or convert appendVisibleRow's fallback and the DSS row in that function together so all three consumers agree. A test that fails with m_confirmed* swapped back to m_centerMhz would make the fix stick.
Thanks for this — the problem is real, the header comment is unusually good documentation of intent, and the updateWaterfallRow hunk is exactly right.
🤖 aethersdr-agent · cost: $3.9553 · model: claude-opus-5
| m_confirmedCenterMhz = centerMhz; | ||
| m_confirmedBandwidthMhz = bandwidthMhz; |
There was a problem hiding this comment.
Blocker 1. This runs before every return in the function — including the three the comment above says are excluded.
- drag-hold, line 7498:
deferIncomingRange(...); return; - settle-pending, 7507-7513:
deferIncomingRange(...); return; - stale-echo, 7534-7539: bare
return;
All three reject the incoming value as not-currently-true, and all three have already written it to m_confirmed* by the time they run. The settle-pending guard's own comment says Flex "can echo older center-only statuses after a combined center+bandwidth command" — those stale centers now become the geometry updateWaterfallRow() lays rows out in and appendHistoryRow() stamps them with, until the next accepted push.
Suggest moving the pair to just after m_deferredRangeValid = false; (7517), which is the point the function itself declares as "past every hold: this value is being applied." If the no-op-guard case at 7488 genuinely needs it too, record there specifically rather than ahead of all four.
Either way the comment needs to stop claiming an exclusion the code doesn't implement.
| // so the pixel layout here must use the same values or the row's data | ||
| // and its stamp would disagree about what span it covers. See | ||
| // m_confirmedCenterMhz's own declaration comment. | ||
| const double panStartMhz = m_confirmedCenterMhz - m_confirmedBandwidthMhz / 2.0; |
There was a problem hiding this comment.
Blocker 2. Two problems here.
First, the comment's premise is inverted: panStartMhz in this function feeds only the TX-mask test at 12236-12240. The actual bin→column mapping (12245-12258) is proportional over destWidth and never consults it. So this line does not "lay out the pixel data" at all.
Second, the bins are in the on-screen frame, not the confirmed one. reprojectSpectrum() (7303-7418) resamples m_bins into the new optimistic frame at gesture time, and updateSpectrum() (8212-8292) stores frameless incoming bins as the current display frame — that is the widget's standing convention. Stamping those pixels with the confirmed frame means the row's content and its label disagree during exactly the divergence window this PR is about, so remapHistoryRowInto() shifts it and the row jumps horizontally once the viewport rebuilds.
And within this one call the same scanline now gets three different frames: history = confirmed (12265), visible = on-screen (12268 → appendVisibleRow's untouched fallback at 5372-5374), DSS = preview-base-or-on-screen (12266 → resolvedUntaggedDssFrame, 5233-5250). Before this PR all three said m_centerMhz.
Either revert this path to m_centerMhz/m_bandwidthMhz, or convert the visible and DSS rows here in the same breath so all three agree.
| const double freqMhz = panStartMhz | ||
| + (static_cast<double>(x) / static_cast<double>(destWidth)) | ||
| * m_bandwidthMhz; | ||
| * m_confirmedBandwidthMhz; |
There was a problem hiding this comment.
Concrete casualty of blocker 2: freqMhz is now derived from confirmed geometry while the columns it gates are laid out in on-screen geometry (proportional mapping at 12245-12258). During a zoom gesture that hasn't landed, the TX blackout therefore masks the wrong columns — it will be offset or scaled relative to the actual passband. Pre-PR both sides used m_centerMhz/m_bandwidthMhz, so they agreed by construction.
| // optimistically" bandwidth argument, citing this exact "bakes black | ||
| // rows into waterfall history" failure) -- that fix doesn't reach here, | ||
| // where the actual row stamping happens. | ||
| double m_confirmedCenterMhz{14.225}; |
There was a problem hiding this comment.
Two small corrections to the (otherwise excellent) comment above:
- "the four zoom gesture handlers write their locally-computed target here immediately" — true (2251, 10742, 11438, 11618), but the pan-drag (10473), slice-drag edge-pan (10339) and "Center Slice" context action (9905) also advance
m_centerMhzahead of confirmation. The mechanism covers them; the comment reads as if it doesn't have to. - "see
setFrequencyRangeInternal()'s own comment for the one place this pair updates" — that comment currently asserts an exclusion the code doesn't implement (see blocker 1), so it shouldn't be cited as the explanation until it's fixed.
Also worth noting: m_frequencyPreviewBaseCenterMhz/…BandwidthMhz (updateFrequencyPreview(), 6218-6254) already holds the pre-gesture frame for this exact window, and resolvedUntaggedDssFrame() already consumes it for untagged DSS rows. Reusing it would avoid a second source of truth that can drift from the first.
| return 0; | ||
| } | ||
|
|
||
| int testStampFrameForHistoryRow() |
There was a problem hiding this comment.
The invalid-frame cases are good and correctly mirror FrequencyFrame::isValid() (finite + both > 0.0).
But this test would pass unchanged against main: stampFrameForHistoryRow() is a byte-for-byte extraction of the ternary already at SpectrumWidget.cpp:5452-5454 pre-PR. It pins the extraction, not the fix — the actual behavior change (fall back to confirmed rather than on-screen geometry) lives entirely in SpectrumWidget and has no coverage.
Something that fails if m_confirmed* is swapped back to m_centerMhz is what would keep this from regressing. If a widget-level test is impractical headless, a small logic seam — e.g. a free function taking (requested, confirmed, onScreen, isGestureActive) — would at least make the choice testable the way dssUntaggedRowUsesPreviewBase() already is.
|
Cisco CodeGuard — static analysis of this PR (10 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $4.5176 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
sampleWaterfallHistoryFrame reads from a caller-supplied buffer and returns an unsigned 8-bit frame count; it implements waterfall row offsets, making it the likely location for a memory-safety flaw if untrusted data drives those offsets. Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting. 🤖 aethersdr-agent · cost: $4.5176 · model: claude-opus-5 |
|
@aethersdr-agent Investigated both blockers. Frame Mismatch: I don't think this is actually a bug. Traced it through pushWaterfallRow() → appendHistoryRow() → stampFrameForHistoryRow(): the pixel layout at SpectrumWidget.cpp:12229 already uses m_confirmedCenterMhz/m_confirmedBandwidthMhz directly, the call passes no explicit frame to appendHistoryRow(), and that function's defaults (frameCenterMhz = -1.0, frameBandwidthMhz = -1.0) are exactly the "absent" case stampFrameForHistoryRow() falls back to the confirmed pair for — the same case the new unit test covers. Pixel layout and stamp both resolve to the same confirmed values consistently. Happy to be shown a case this misses. Timing Issue: real finding, but I don't think it's this PR's to fix. The leak is in MainWindow_Wiring.cpp:686-689 — a "local-only snap" path explicitly tagged // aetherd RFC 2.3, which fires PanadapterModel::infoChanged (connected straight to setFrequencyRange) for a value that's deliberately never sent to the radio. Checked docs/architecture/aetherd-iradiobackend-design.md: RFC 2.3 is "split the five mixed models" — a still-in-progress, 5-PR effort to separate operator-requested state from radio-confirmed state across the model layer. Whether a "confirmed, but never touched the radio" value should exist at all is exactly the question that refactor is meant to resolve, and it's explicitly not done yet. Folding a fix for it into a waterfall-history-stamping PR would be scope creep into an already-tracked, larger architectural question — flagging it here instead in case it's useful context for whoever picks up RFC 2.3's remaining PRs. |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
No linked issue; reviewed against its own stated intent. The defect is real and well characterised: waterfall rows carry a permanent frequency/bandwidth stamp, the zoom gestures write their unconfirmed target into m_centerMhz/m_bandwidthMhz for instant feedback, and any row captured in that window is scarred until it ages out of the 20-minute ring. Splitting "what the operator sees" from "what the backend confirmed" is the right fix, and keeping the gesture handlers untouched means responsiveness is unchanged.
Verified empirically on macOS/arm64: the full AetherSDR target builds clean and spectrum_preview_logic_test passes.
No blockers. One finding that limits the fix's reach, and two checks that came out clean.
What I verified rather than assumed
- The defaults match.
m_confirmedCenterMhz{14.225}/m_confirmedBandwidthMhz{0.200}are identical tom_centerMhz/m_bandwidthMhzatSpectrumWidget.h:1338-1339, so there is no window where rows get stamped0 MHzbefore the first echo. That was the first thing I went looking for and it is handled. - No self-confirmation.
SpectrumWidgetnever calls its ownsetFrequencyRange()/setFrequencyRangeImmediate()— I grepped the whole file. So none of the eleven directm_centerMhz =writes can reach the confirming path by accident; the gesture handlers stay optimistic-only, exactly as intended.
Finding — not blocking
Keyboard zoom still bakes a guess, because it confirms before it asks. (inline: SpectrumWidget.cpp:7484) The body scopes the problem to "the four zoom gestures (click/drag/pinch/wheel)", and for those four the fix is complete. There is a fifth, and it takes the newly-confirming path. MainWindow_Shortcuts.cpp:1359-1360:
sw->setFrequencyRange(newCenter, newBw); // now marks CONFIRMED
applyPanRangeRequest(s->panId(), newCenter, newBw, "pan-zoom"); // only now asks the backendnewCenter/newBw are computed locally from sw->centerMhz() and the model's min/max limits — an unconfirmed target, by the same definition the PR uses everywhere else — and the ordering is unambiguous: the confirm happens on the line before the request is sent. So a keyboard zoom stamps history with a guess that may be rate-snapped away, which is the defect this PR exists to remove.
The contrast that makes this clear is MainWindow.cpp:9887, which passes pan->centerMhz() / pan->bandwidthMhz() — actual model state, a legitimate confirmation. Two callers of the same entry point, one a backend echo and one an operator guess, and after this PR they are no longer equivalent.
That is the structural point worth naming: this change gives setFrequencyRange() a new meaning ("this geometry is real"), but the boundary between guess and echo actually lives one layer up, at the MainWindow call sites. Nothing in the signature tells a caller which contract they are satisfying, and the keyboard path is already on the wrong side of it.
Nits
stampFrameForHistoryRow()being extracted intoSpectrumPreviewLogic.hso it can be unit-tested is the right move, and the test is the only reason I could check the fallback behaviour at all. Worth noting the other three call sites (updateWaterfallRow,pushWaterfallRow,pushKiwiSdrWaterfallRow) remain untested — they are the ones where "pixel layout and stamp must agree with each other" matters, and that agreement is currently maintained by hand in three places.- The declaration comment for
m_confirmedCenterMhzis excellent — it names the failure, the one place the pair updates, and theRadioModel::requestPanCenter()precedent one layer up. Given finding 1, it is worth one more sentence saying that callers ofsetFrequencyRange()are asserting the geometry is real, since that is now load-bearing and only discoverable by reading this comment.
Verified vs. read
Built and ran: the full app target and spectrum_preview_logic_test. Traced: all eleven m_centerMhz write sites to confirm which are gesture paths and which are inside setFrequencyRangeInternal; every caller of both public setters across src/gui/. Read: the stamp/layout agreement argument in pushWaterfallRow. Not verified: nothing rendered and no ANAN hardware, so the bench result you describe — rate-snapped zoom exercising this race hard — rests on your session. The keyboard-zoom finding is from the call site's own ordering, not from observing a scarred row.
| // this function, which is why it's first. The three OTHER early | ||
| // returns below (drag-hold, settle-pending, stale-echo) do NOT get | ||
| // this treatment -- they legitimately don't represent "this value is | ||
| // confirmed truth right now" (see their own comments). | ||
| m_confirmedCenterMhz = centerMhz; | ||
| m_confirmedBandwidthMhz = bandwidthMhz; |
There was a problem hiding this comment.
Finding — this makes setFrequencyRange() mean "confirmed", and one existing caller cannot honour that.
MainWindow_Shortcuts.cpp:1359-1360, the keyboard pan-zoom shortcut:
sw->setFrequencyRange(newCenter, newBw);
applyPanRangeRequest(s->panId(), newCenter, newBw, "pan-zoom");newCenter comes from sw->centerMhz() clamped against panMinBandwidthMhz/panMaxBandwidthMhz, so it is a locally computed target — and the request to the backend goes out on the next line. After this change, that first call marks the guess as confirmed, and any waterfall row captured before the backend answers is stamped with it. If the backend rate-snaps to something else, that row is scarred exactly the way the four mouse gestures used to scar rows.
Compare MainWindow.cpp:9887, which passes pan->centerMhz() / pan->bandwidthMhz(). That is model state and a genuine confirmation. Same entry point, opposite contract.
I do not think the fix is wrong — it is right for everything it covers, and I verified the gesture handlers never reach here. The issue is that the guess/echo boundary is at the MainWindow layer, not inside SpectrumWidget, and the signature gives a caller no way to know which one they are asserting.
Two ways to close it, both small:
(a) Make the keyboard path optimistic like its mouse siblings — apply the visual change without confirming, and let the backend echo arrive through the normal path, which is what applyPanRangeRequest() already sets in motion.
(b) Make the contract explicit in the signature so it cannot be got wrong by accident:
// Callers assert this geometry is REAL — the backend has echoed it.
// For an operator gesture whose target the backend has not confirmed,
// use setFrequencyRangePreview() instead, which leaves
// m_confirmedCenterMhz/m_confirmedBandwidthMhz alone.
void setFrequencyRange(double centerMhz, double bandwidthMhz);(a) is the smaller diff; (b) is what stops the sixth caller reintroducing this. Either way the body's "the four zoom gestures" should become five, so the next person does not read the keyboard path as already covered.
zoomActivePanadapter() called sw->setFrequencyRange() directly, which is setFrequencyRangeInternal() -- marking the guess CONFIRMED on keypress, before the backend had done anything (ten9876, aethersdr#5142 review). Every other pan-range requester (SWR sweep, mouse/wheel/ pinch) only calls applyPanRangeRequest() and lets the confirmed value update when the echo arrives. Drop the direct call so keyboard zoom follows the same pattern.
|
@ten9876 the keyboard-zoom fix is in as 86348e7 — zoomActivePanadapter() was calling sw->setFrequencyRange() directly, which is setFrequencyRangeInternal() under the hood and marks the guess CONFIRMED on keypress before the backend does anything with it. Dropped that call so it now only calls applyPanRangeRequest(), same as every other pan-range path (SWR sweep, mouse/wheel/pinch). Let me know if you want another look. |
setFrequencyRangeInternal() wrote m_confirmedCenterMhz/m_confirmedBandwidthMhz unconditionally at the top of the function, before the drag-hold, settle-pending, and stale-echo early returns -- despite its own comment claiming those three were excluded. Move the write to run only for the no-op case and the real-application path, matching the documented intent (ten9876, aethersdr#5142 review, "Blocker 1"). pushWaterfallRow()'s TX-mask test computed panStartMhz from the confirmed frame, but the bins it's masking (m_bins) are kept in the on-screen frame by reprojectSpectrum()/updateSpectrum() -- the mask could blank the wrong columns relative to what's actually plotted during a zoom's divergence window. Revert panStartMhz and the mask's frequency step to on-screen geometry, which is what the bin data and the mask both need to agree on (ten9876, aethersdr#5142 review, "Blocker 2", first half). Not addressed: appendHistoryRow()'s own fallback still stamps this row with the confirmed frame while its pixel content is on-screen -- that's this path's original, intentional design (see stampFrameForHistoryRow's doc comment) and reverting it would undo the PR's point for this code path. Real tradeoff, no clean single answer; left for a maintainer call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@ten9876 Pushed 7cc9a67 — Blocker 1 fixed as suggested: moved m_confirmedCenterMhz/m_confirmedBandwidthMhz past the drag-hold/settle-pending/stale-echo returns in setFrequencyRangeInternal(), kept it in the no-op-guard branch since that case still needs it. On the frame-mismatch question in pushWaterfallRow(): panStartMhz and the appendHistoryRow() fallback both resolve to m_confirmedCenterMhz/m_confirmedBandwidthMhz, consistently with each other — but that's a level above the actual question. The bin→column interpolation a few lines below panStartMhz (the binF/binIdx block) never reads panStartMhz or any frame value at all — it's a plain proportional stretch of bins over destWidth. So what matters isn't whether the two labels agree with each other, it's whether either of them agrees with what bins itself actually is. Traced that at the TX call site: bins is *spectrumBins, and m_bins = *spectrumBins a few lines above it in the same function. reprojectSpectrum() (7303-7418) resamples m_bins to the incoming center/bandwidth every time setFrequencyRangeInternal() runs — to on-screen, not confirmed. So by the time pushWaterfallRow() runs, bins tracks on-screen geometry, and panStartMhz's confirmed label can describe a span the pixel data doesn't actually cover. Reverted panStartMhz (and the mask's frequency step) to on-screen so the TX mask lines up with what it's actually masking. What I haven't traced, and what would actually settle whether this matters in practice: what the DSP layer feeding spectrumBins computes those bins against in the first place — whether on-screen and confirmed ever diverge widely enough, for long enough, to reach this data before the display catches up. appendHistoryRow()'s own confirmed-frame stamp is untouched — that's intentional per stampFrameForHistoryRow()'s doc comment, and reverting it would reopen the original bug for this fallback path. Between that and the TX-mask fix, this leaves the row's stamp and its pixel content still potentially disagreeing during a zoom's divergence window, same as before — I didn't find a clean way to fix both without either the DSP context above or picking one of two real tradeoffs. @ten9876, worth your read on whether the mask fix alone is enough or whether the deeper stamp-vs-content question needs resolving too. |
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Blocking: live IC-7300MK2 testing shows this revision makes horizontal pan dragging materially worse and writes corrupted rows into retained waterfall history.
I tested exact PR head 7cc9a674 merged onto current origin/main (f41dc755) in a fresh macOS RelWithDebInfo build. The focused socket-free tests passed before deployment. On an IC-7300MK2, however, horizontal mouse/trackpad drags produced:
- wide, solid-black rectangular gaps committed into waterfall history;
- variegated/checkerboard rows as the drag continued; and
- the pan and waterfall snapping back to a different center after release.
The live spectrum remained continuous while the waterfall below it broke up, and the connection remained healthy. This is not consistent with packet loss or a harmless transient uncovered edge.
The source path explains the observed shape. IcomCivBackend::onCivFrame() emits each sweep's actual panCenterBandwidthChanged() immediately before spectrumFrameReady(). An Icom center-mode pan drag retunes the VFO, so the exact-span scope bounds move with those sweeps. During the drag, SpectrumWidget::setFrequencyRangeInternal() rejects those changing centers and leaves m_confirmedCenterMhz frozen. The changed updateWaterfallRow() then rasterizes the newly moved Icom sweep against that stale confirmed viewport, zero-filling frequencies outside the overlap, and stamps the primary row as though it covered the whole stale frame. remapHistoryRowInto() prefers that claimed primary-frame coverage, so the correctly framed supplemental row cannot repair the zeros. The black rectangles are therefore retained, not just preview artifacts.
This invalidates the prior conclusion that the native-tile hunk is universally coherent. Its Flex assumption that native tiles extend beyond pan edges does not hold for an Icom cooked scope sweep whose lowFreqMhz/highFreqMhz describe the exact data extent.
Please address before merge:
- Keep the producer-supplied frame authoritative for exact-span backend sweeps. If Flex oversized tiles need confirmed-viewport rasterization, distinguish that input contract instead of applying it to every
updateWaterfallRow()caller. - Ensure the primary row never claims coverage for pixels that were zero-filled because its source sweep did not cover that frame; otherwise supplemental coverage remains unreachable.
- Exercise the post-release Icom convergence path so it settles once on the final radio-accepted center rather than visibly rebasing through an intermediate sweep.
- Add a deterministic regression test for: initial confirmed center -> optimistic horizontal drag -> several delayed exact-span sweeps at moving centers -> release/final acknowledgement. Assert no black holes within each sweep's real coverage, correct retained-row stamps, and final view/radio convergence. The current helper test does not exercise
updateWaterfallRow()or this temporal sequence.
Required validation should include another IC-7300MK2 drag test plus regression checks for Flex and the other shared-widget producers (ANAN, HL2, Sim, and Kiwi). Only Icom is live-proven broken here, but the rendering code is shared.
Requesting changes because the defect affects a primary display surface and persists in visible history for every affected drag.
…covered updateWaterfallRow() crops every tile to the confirmed viewport and stamps the row with that frame. Correct for Flex, whose tiles extend beyond the pan edges, but an exact-span sweep covers only its own extent: the uncovered columns are zero-filled while the row still claims the full viewport, and remapHistoryRowInto() tests the primary frame first, so the supplemental row can never repair them. On an IC-7300MK2 every horizontal pan drag committed wide black rectangles to retained history (jensenpat, aethersdr#5142 review). Add primaryRowFrameForNativeTile(): crop only where the tile really covers the viewport, otherwise the producer's extent is authoritative. Flex is unchanged by construction. The rasterisation, the stamp and panStartMhz now all derive from that one frame, so they cannot drift apart. Not reproduced locally — Sim takes the same exact-span path but echoes the requested centre back within the drag's command interval, so its tile tracks the viewport and the stale claim is never read. Verified by reading and by unit test, which fails against the previous behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jensenpat's aethersdr#5142 review, item 4: the existing helper test does not exercise updateWaterfallRow() or the temporal sequence, so it cannot see the defect he reproduced on an IC-7300MK2. That defect is not a single bad decision -- it only appears once the viewport stops tracking the sweeps during a drag. Lift the column-to-bin mapping out of updateWaterfallRow()'s rasterisation loop into nativeTileBinForColumn(), and have the loop call it. One implementation, so a test cannot drift from the code it checks -- which matters here specifically, since the bug was a disagreement between the frame a row claims and the pixels it actually has. nativeTileRowIsFullyCovered() states the invariant items 1 and 2 asked for: a committed row claims no column it zero-filled. The new test drives his sequence -- confirmed centre, drag freezes it, delayed exact-span sweeps at moving centres, release -- over seven tile geometries, plus a table covering Flex, Icom, ANAN, HL2, Sim and Kiwi shapes since the render path is shared. It passes against this branch and fails 5 of 7 sweeps against the pre-fix behaviour of always taking the viewport frame. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Replacing my 2026-09-03 comment — items 1, 2 and 4 are now done, and 3 has a Items 1 and 2 — fixed in Item 4 — done in Rather than write a second copy of the mapping in a test, I lifted the The test drives your sequence — confirmed centre, drag freezes it, delayed It passes on this branch and fails 5 of the 7 sweeps against the pre-fix Item 3 — I'd like to leave this out of the PR. The defect is real and I can name it: The machinery is half present. That is new state and a behaviour change on the drag-release path of a shared What I still can't do: the IC-7300MK2 drag. I have no Icom hardware, so I |
|
Thanks for this — the fix itself is in good shape, and the CI break is a two-line scoping slip in the new test file, not anything wrong with the change to What failedOn
Root causeThe two new test functions added after the anonymous namespace closes are missing the
…and the same at lines 1244, 1271, 1274, 1292, 1307. Your earlier cases got this right ( Why Windows went greenNot a platform difference, and nothing MSVC-specific.
FixAdd the int testNativeTileDragSequenceNeverClaimsUncoveredSpan()
{
+ using namespace AetherSDR;
constexpr int kDestWidth = 800; int testNativeTileCoverageAcrossProducers()
{
+ using namespace AetherSDR;Verified — with those two lines added, the TU compiles clean at The test file only pulls in On the reviewer commentsThe four bot review items are all design questions about the Nice work on the invariant framing in 🤖 aethersdr-agent · cost: $6.5443 · model: claude-opus-5 |
jensenpat's aethersdr#5142 review, item 4: the existing helper test does not exercise updateWaterfallRow() or the temporal sequence, so it cannot see the defect he reproduced on an IC-7300MK2. That defect is not a single bad decision -- it only appears once the viewport stops tracking the sweeps during a drag. Lift the column-to-bin mapping out of updateWaterfallRow()'s rasterisation loop into nativeTileBinForColumn(), and have the loop call it. One implementation, so a test cannot drift from the code it checks -- which matters here specifically, since the bug was a disagreement between the frame a row claims and the pixels it actually has. nativeTileRowIsFullyCovered() states the invariant items 1 and 2 asked for: a committed row claims no column it zero-filled. The new test drives his sequence -- confirmed centre, drag freezes it, delayed exact-span sweeps at moving centres, release -- over seven tile geometries, plus a table covering Flex, Icom, ANAN, HL2, Sim and Kiwi shapes since the render path is shared. It passes against this branch and fails 5 of 7 sweeps against the pre-fix behaviour of always taking the viewport frame. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
982e869 to
20a3e36
Compare
3 participants
Summary
Every waterfall history row is permanently stamped with the frequency/
bandwidth it was captured at. But the four zoom gestures (click/drag/
pinch/wheel) write their locally-computed, unconfirmed target straight
into
m_centerMhz/m_bandwidthMhzfor instant visual feedback, beforethe backend has echoed back what it actually took. Any row captured in
that window got a permanently wrong stamp — a lasting scar (wrong signal
at the wrong frequency, or a black gap) that never self-corrects until
it ages out of the 20-minute ring buffer.
Adds
m_confirmedCenterMhz/m_confirmedBandwidthMhz, updated only whensetFrequencyRangeInternal()sees a real backend echo, and uses thatpair — not the on-screen guess — everywhere a row's pixels and stamp are
built:
updateWaterfallRow(),pushWaterfallRow()(pixel layout andstamp must agree with each other or the wrong signal shows at the wrong
frequency),
pushKiwiSdrWaterfallRow(), andappendHistoryRow()'s ownfallback (now
stampFrameForHistoryRow(), unit tested). Gesture handlersare untouched, so click/drag/pinch/wheel responsiveness is unchanged.
Shared
SpectrumWidgetcode — benefits every backend's waterfall, notjust ANAN's. Found during ANAN zoom bring-up (frequent rate-snapped zoom
requests exercise this race hard), bench-verified there, but not
ANAN-specific in cause or fix. Not part of #4970.
Constitution principle honored
N/A — GUI correctness fix, no persistence/protocol/transmit surface touched.
Test plan
a zoom's multi-second round trip is now an honest, transient black
region that fills in and never reappears, instead of a permanent
scar
testStampFrameForHistoryRowinspectrum_preview_logic_test.cpp— pins both the valid-per-row-framecase and the fallback-to-confirmed-frame case (zero bandwidth,
negative bandwidth, non-finite center all treated as "absent")
spectrum_preview_logic_testpasses, confirmed standalone on thisexact branch (built fresh against
main@3e467fa8, not inheritedfrom
anan-step2-submission)Checklist
AppSettingschangesintended behavior; the bug was never documented as a feature)