[panadapter] Honor radio display state and bound hidden pan rendering#4261
Conversation
There was a problem hiding this comment.
Nicely structured cleanup. Replacing the four delayed-reconcile timer/cooldown maps with per-pan status connections gated by QSignalBlocker is a real simplification, and routing live PanadapterModel status through setFftFps/setWfLineDuration (with the adaptive-throttle hold preserved) is the right way to make the radio authoritative without echoing commands back. The hidden-source bounding in SpectrumWidget reads carefully — early returns guard the color-conversion/DSS/history work, and discardRetainedHistory() is called on every source/profile switch. The DssRenderer byte-accounting uses sizeof only on genuinely fixed std::array members and .capacity() on the QVectors, and the new setHistoryCapacityRows(0) release is covered by a test. Connection lifetimes are safe (lambdas use sw as the receiver context). Docs/AGENTS.md updates are consistent with the code.
One instrumentation nit; nothing blocking.
Polish
SpectrumWidget::appendHistoryRow— thewaterfallHiddenHistoryRowsincrement is unreachable (function already returned for hidden sources), so the documentedwaterfallHiddenHistoryRowsPerSecfield is structurally always 0.
Non-blocking notes
AutomationServerrenderstats: when the firstSpectrumWidget's scheduler reportsenabled=false, the loop callsrenderSchedulerStatsSnapshot(reset)again on subsequent widgets — a harmless double-reset of the shared scheduler counters, but worth a comment.SpectrumOverlayMenu::syncPanProcessingSettingsdereferencesm_avgLabel/m_fpsLabelafter only null-checking the sliders; this matches the existingsyncDisplaySettingspattern, so it's consistent, just noting it.
🤖 aethersdr-agent · cost: $6.5276 · model: claude-opus-4-8
There was a problem hiding this comment.
Solid, well-structured rollup. The reconcile-timer removal is a genuine simplification — swapping four families of debounced re-assert timers + cooldown maps for signal-blocked live-status wiring (wirePanDisplayStatus) is the right shape, and routing status through QSignalBlocker in syncPanProcessingSettings closes the command-echo loop cleanly. The hidden-source/DSS bounding reads correctly: early returns before color conversion / viewport writes / DSS work for hidden streams, discardRetainedHistory on source/profile switch, and DSS capacity gated to 3D. AGENTS.md authority table is updated to match, member defaults are all in-class initialized, and CI is green (build/CodeQL/a11y/engine-boundary all pass). No QSettings misuse, RAII scope guards used for the ingest-cost timers.
Only minor instrumentation nits below — nothing blocking.
Polish
appendHistoryRow: thewaterfallHiddenHistoryRowsincrement is dead code (unreachable after the earlyreturn), so thewaterfallHiddenHistoryRowsPerSeccounter the docs advertise can never be non-zero.appendDssHistoryRow: passes!m_waterfallWriteVisibleashiddenStream, but that's alwaysfalseon the only reachable path — sodssHiddenHistoryRowsnever counts here.
Non-blocking note
- These two "hidden history" counters are documented as exposing #4081/#4083 retention, but post-change hidden sources structurally never write history, so the counters are tautologically 0 rather than measuring it. Consider either dropping them or moving the hidden accounting to the
beginWaterfallStreamWriteadmission point where hidden work is actually observable.
🤖 aethersdr-agent · cost: $4.5299 · model: claude-opus-4-8
rfoust
left a comment
There was a problem hiding this comment.
I reviewed the PR's merge result (e19dfbab) against current upstream/main (5371ec03). The build and runnable tests pass, but I found two blockers:
-
Adaptive throttling can overwrite newer radio/profile state.
wirePanDisplayStatus()ignores every reported FPS and waterfall-duration update whilem_adaptiveThrottleActive, not just adaptive-cap echoes. When throttling ends,adaptiveThrottleChanged(false)sends the values retained inSpectrumWidgetback to the radio. If a profile load or another client changes those radio-authoritative values while throttling is active, the stale pre-throttle values can be restored over the newer state.Please distinguish adaptive-cap echoes from legitimate radio/profile updates, or explicitly track and invalidate the restore target. A regression test should cover a reported setting change while the cap is active followed by throttle release.
-
This PR is configured to close #4221, but it does not implement that issue's waterfall-smoothness fix.
The GPU path still derivesrowOffsetdirectly from integerm_wfWriteRow, and the software path still draws whole ring-buffer rows. There is no time-based/sub-pixel interpolation, so the reported stepping remains. Please unlink #4221 from the closing references or implement the proposed interpolation separately.
The existing unresolved hidden-history-counter threads are non-blocking instrumentation cleanup and I won't duplicate them here.
Validation completed:
- RelWithDebInfo macOS build passed.
- 112/112 runnable tests passed; one intentional quarantine test skipped.
- Engine-boundary, accessibility, bridge-documentation, and diff checks passed.
- Offscreen no-autoconnect startup passed.
pingandget renderstats resetreturned valid bridge responses.- No radio connection or transmit action was used.
Once the authority race is fixed and #4221's linkage is corrected, I'm happy to take another pass.
Cross-platform runtime verification (Linux / macOS / Windows)Ran a ~45-minute leak soak of this PR on all three platforms, RX-only against a FLEX-6700 (and a 6500), driving the in-app automation bridge. Sampled the app's own Builds: Linux (Qt 6.10.3, from source at this PR's head) · macOS (from source) · Windows (the CI The bounded pan memory holds — identically — on every OS
No growth, no drift in the DSS store — switching 2D↔3D moves between exactly those two ceilings and holds. Waterfall allocation is likewise bounded. The "bound hidden rendering / honor display state" behavior does what it says, cross-platform. Residual RSS — a pre-existing, Linux-only creep (not this PR)Process RSS was flat on macOS and near-flat on Windows (+0.5 MB/min), but on Linux it creeps in 3D while
NetThe bounded-DSS behavior this PR targets holds byte-identically on all three OSes and is unchanged. The only RSS growth is a pre-existing, Linux-only creep that sits outside the DSS store — present on 73, Ozy K6OZY |
|
Reviewed independently alongside @rfoust's CHANGES_REQUESTED — confirmed blocker #1 directly ( New functional regression, same severity class as the existing blockers: 1. The hidden-Flex path no longer feeds the DSS live ring, contradicting this PR's own stated goal. 2. Asymmetric guard placement makes Kiwi-source switching non-immediate while Flex stays immediate. Smaller correctness gaps: 3. 4. Stale comments in three untouched files. Architectural concern, worth discussing before merge rather than blocking on: 5. Nothing prevents a fifth recurrence of this exact bug class. The root-cause table blames four prior PRs for independently reintroducing "client persists radio-authoritative state." This fix, like the others, is purely instance-level — four deleted 6. Cleanup, non-blocking:
One process note for whoever's tracking merge readiness: this touches Happy to re-review once rfoust's two blockers and #1 above are addressed. |
ten9876
left a comment
There was a problem hiding this comment.
Maintainer review (Tier 1 / AGENTS.md gate) — confirming the blockers
I independently ran a high-effort pass and verified the substantive findings against the code. I'm not re-blocking on top of @rfoust's CHANGES_REQUESTED, but flagging that — because this PR touches AGENTS.md (Tier 1 per CODEOWNERS) — the Tier 1 approval it needs from me specifically is gated on the same items. The direction is good and @Ozy311's 3-platform leak soak is reassuring; these must land first.
Blockers (verified)
- Adaptive throttle can overwrite newer radio/profile state (rfoust #1) — confirmed inline below.
wirePanDisplayStatusdrops every fps/wf-duration update whilem_adaptiveThrottleActive, andadaptiveThrottleChanged(false)(MainWindow.cpp:1759-1792) restores the retained pre-throttle value, so a profile load / second client changing those during throttling is lost. - Hidden-Flex DSS live ring is no longer fed (NF0T #1) — confirmed: the
pushDssRowForWaterfallStream(false, m_bins)call inupdateSpectrum's hidden-Flex branch is replaced with a bare early return, reintroducing the ~96-frame refill staleness the deleted code prevented and contradicting the PR's own "small live rings stay warm for an immediate toggle" claim. It also makesdssHiddenLiveRowsPerSecstructurally always 0 — the same dead-counter pattern the bot threads flagged atSpectrumWidget.cpp:4120/:3989, just for a third counter. - Closes #4221 but doesn't implement its waterfall-smoothness fix (rfoust #2) — the GPU path still derives
rowOffsetfrom integerm_wfWriteRow; no sub-pixel/time interpolation. Unlink #4221 or implement separately.
Correctness (should fix)
weightedAveragemissing the unknown-guard (NF0T #3) — confirmed inline below.- Asymmetric guard makes Kiwi source-switch non-immediate (NF0T #2) —
updateSpectrumupdates Flex's cached trace before its hidden early-return, butupdateKiwiSdrWaterfallRow's guard is at the top, so switching to Kiwi shows a stale trace where Flex is immediate.
Instrumentation / cleanup (non-blocking)
- Three hidden-history counters are structurally always 0 (
waterfallHiddenHistoryRows@4120,dssHiddenHistoryRows@3989 — already in the bot threads — plusdssHiddenLiveRowsPerSecfrom #2). The bridge telemetry documented as exposing hidden-source retention can never report nonzero. - Dead conditional in
updateSpectrum:if (m_kiwiSdrWaterfallActive){…return;}immediately followed byif (!m_kiwiSdrWaterfallActive){…}(second always true). - Stale comments still describe the deleted reconcile mechanism in
PanadapterModel.h/.cppandRadioModel.h(not in this diff, so a follow-up doc pass);CONTRIBUTING.md's radio-authoritative quick-reference wasn't synced toAGENTS.md. - Duplication:
get renderstatsre-implements theget panstats/get wavestatswidget-enum loop;syncPanProcessingSettingsduplicatessyncDisplaySettings.
Altitude — worth addressing given this is the second rollup for this class
The fix is instance-level (4 hand-written connections that must each remember not to write back), and the sibling setters right next to the fixed ones (setWfColorGain/setWfBlackLevel) still persist to AppSettings — the exact pattern a future contributor copies (NF0T #5). A one-line anti-pattern note in AGENTS.md ("don't write radio-echoed status into a setter that also persists") would help break the recurrence more than another instance fix.
Happy to give the Tier 1 approval once the two blockers + the hidden-DSS regression are fixed and @rfoust/@NF0T re-review. Thanks for the thorough root-cause writeup, @jensenpat.
…cs (aethersdr#4261). Principle II. Fixes the two blockers + correctness/doc items from @rfoust's CHANGES_REQUESTED and @NF0T's review: - Blocker #1 (throttle overwrites newer radio state): wirePanDisplayStatus no longer drops *every* fps/waterfall-line-duration report while throttled — it suppresses only the adaptive cap's own echo (fps == m_adaptiveFpsCap, line duration == adaptiveWfMsForCap(cap)) and applies any other reported value, so a profile load / second client change during throttle survives the lift instead of being overwritten by the stale restore target. The decision is a pure applyThrottledDisplayReport() in DisplayStatusGate.h, unit-tested by display_status_gate_test (rfoust's requested regression coverage). adaptiveWfMsForCap() promoted to public for the line-duration echo check. - Blocker #2 (hidden-Flex DSS live ring not fed): restore the pushDssRowForWaterfallStream(false, m_bins) feed in updateSpectrum's Kiwi-active branch so the small hidden-Flex DSS live ring stays warm for an immediate source switch (the deep retained history stays released for hidden sources, capacity 0, so no scrollback is retained — the PR's own stated intent). Re-activates the dssHiddenLiveRowsPerSec telemetry. - weightedAverage unknown guard: PanadapterModel gains weightedAverageKnown() (set once the radio reports weighted_average), and wirePanDisplayStatus guards the handler + seed on it, so a fresh pan no longer paints a definitive unchecked box before status arrives (matches the average/-1 sentinel pattern). - Docs: AGENTS.md gains an explicit anti-pattern note (don't write a radio-echoed status value into a persisting setter — the recurring aethersdr#4261 class); CONTRIBUTING.md's radio-authoritative quick-reference synced to match; stale "MainWindow can reconcile the user's desired value" comments in PanadapterModel.{h,cpp} and RadioModel.h updated to the radio-authoritative model this PR establishes. Full app builds clean; display_status_gate_test (8), dss_renderer_test, and flex_waveform_model_test pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed fixes for the blockers + correctness/doc items in Fixed
Needs a one-click UI action (Blocker #3)
Deliberately NOT blind-edited (hot-path render + refactor risk — precise guidance instead)These are @NF0T's non-blocking items; I left them for you since they touch the render hot path or are pure-refactor risk I shouldn't take without your context:
Happy to take the Kiwi-asymmetry + counter-drop in a follow-up commit if you'd rather I do them than fold them into your next push. Thanks @jensenpat / @rfoust / @NF0T. |
…ersdr#4261). Principle VIII. waterfallHiddenHistoryRows and dssHiddenHistoryRows can never be nonzero: appendHistoryRow early-returns for a hidden source before its RGB-history write, and retainDssHistoryRow early-returns for hidden sources (their retained DSS history capacity is 0). Both counters — and their always-0 waterfallHiddenHistoryRowsPerSec / dssHiddenHistoryRowsPerSec bridge fields documented as "exposes hidden-source retention" — were misleading telemetry. Remove the two struct fields, the increments, the bridge fields, retainDssHistoryRow's now-unused hiddenStream parameter (and its two call sites), and the doc rows. dssHiddenLiveRows is kept — it is meaningful again now that the hidden-Flex DSS live ring is fed (prior commit), exposing the aethersdr#4081 live-ring warming. Builds clean; dss_renderer_test / display_status_gate_test pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Follow-up in
Correction on the Kiwi source-switch asymmetry (@NF0T #2): I read the derivation before touching it, and my earlier "move the guard after the trace derivation" suggestion doesn't hold up. Not done (larger mechanical / pure-refactor, all @NF0T non-blocking): the Net: both blockers + the correctness (weightedAvg guard) + docs + dead-counter cleanup are in ( |
# Conflicts: # CMakeLists.txt
Red-Team Review — PR #4261Adversarial review, not a behavior-preservation pass. I read AGENTS.md/CONTRIBUTING.md/CODEOWNERS, pulled live PR metadata/reviews/reviewThreads (refreshed twice), built the PR head in an isolated worktree, and independently verified the two commits that claim to fix the standing review blockers rather than trusting the commit messages. Two prior human reviewers (@rfoust, @NF0T) and the Tier‑1 maintainer (@ten9876) have already done a thorough substantive pass on the panadapter/DSS logic itself — I did not find new code-correctness bugs beyond what they'd already surfaced, and I verified their claimed fixes are real. My findings are almost entirely about the PR's current mergeable/process state, which isn't visible from the code diff alone. Findings1. [BLOCKING] PR cannot currently merge — 2. [BLOCKING] @rfoust's 3. [BLOCKING] Tier‑1 approval not yet granted. This PR touches No new blocking code-correctness findings from my own read. I verified the two substantive fixes directly:
Non-blocking (already tracked; my read agrees with the severity calls already made)
Checks runMerge-base/ancestry analysis, CI stateUnknown/none reported — Review limitsDid not run Summary: 3 blocking findings, all process/mergeability state rather than new code bugs — the panadapter/DSS logic has already been reviewed hard by two humans and a Tier‑1 maintainer, and the fixes they demanded check out on independent verification. Needs: (1) a rebase onto current 🤖 Generated with Claude Code |
Changes requested have been implemented.
ten9876
left a comment
There was a problem hiding this comment.
Maintainer approval (Tier 1 / AGENTS.md gate) — blockers verified fixed
Ran a final high-effort pass over the merged head (0cc0fce8) after the rebase-onto-main. All blockers and the correctness items are addressed, and the merge with current main integrated cleanly without mangling any of the fixes.
Verified on the merged head
- Blocker #1 — throttle overwrites newer radio/profile state (@rfoust #1): fixed via the pure
applyThrottledDisplayReport()inDisplayStatusGate.h— it suppresses only the adaptive cap's own echo (throttleActive && reported == cap) and applies any other reported value, so a profile load / second-client change during throttle survives the lift instead of being overwritten by the stale restore target. Wired into both live handlers and the seed path (fps →m_adaptiveFpsCap, line-duration →adaptiveWfMsForCap(cap)). Requested regression coverage lands indisplay_status_gate_test.cpp(registered in CMake, runs in ctest; covers "real update ≠ cap survives the lift", both fps and line-duration). - Blocker #2 — hidden-Flex DSS live ring starved (@NF0T #1):
pushDssRowForWaterfallStream(false, m_bins)restored inupdateSpectrum's Kiwi-active branch; deep retained history stays released (capacity 0), so the live ring stays warm for an immediate toggle with no scrollback retained.dssHiddenLiveRowsPerSectelemetry is meaningful again. - Blocker #3 — #4221 linkage (@rfoust #2): #4221 unlinked —
closingIssuesReferencesis now empty, so merging no longer auto-closes it without the smoothness fix. - Correctness #4 — weightedAverage unknown-guard (@NF0T #3):
weightedAverageKnown()added; verified the known-flag is set beforeweightedAverageReportedis emitted, so the guarded handler never drops the first real value and a fresh pan no longer paints a definitive unchecked box before status arrives. - Cleanup: the two structurally-dead hidden-history counters (and
retainDssHistoryRow's always-falsehiddenStreamparam) are removed — resolves the bot threads. AGENTS.md gains the anti-pattern note; CONTRIBUTING.md quick-reference synced; stale reconcile comments updated.
Non-blocking (accepted)
- #5 — Kiwi source-switch trace (@NF0T #2):
updateKiwiSdrWaterfallRowkeeps its top-of-function hidden early-return by design (background rows must not consume GUI-thread render budget; last snapshot is retained and it resumes on the first fresh row). Accepting the deliberate asymmetry with Flex rather than blocking on it. - Narrow residual in the throttle gate: a legitimate update whose value exactly equals the transient cap is treated as the echo — the known limitation of the "ignore only that echo" approach, acceptable.
Approving
Tier 1 / AGENTS.md approval granted. Merge remains gated on CI going green (build / analyze / windows still in flight at approval time) per branch protection. Thanks for the thorough root-cause work, @jensenpat, and the reviews, @rfoust / @NF0T.
QA test plan — run against the automation bridge (merged head
|
| Area | Method | Result |
|---|---|---|
| Test suite | ctest (17 tests) |
✅ 100% pass, incl. display_status_gate_test (8-case throttle regression) + dss_renderer_test |
| #2 hidden-Flex DSS ring | live Flex → inject Kiwi → Flex hidden | ✅ dssHiddenLiveRowsPerSec 0 → 4.14 with waterfallSource: kiwi — ring stays warm on live data |
| #2 switch-back immediacy | inject native → Flex visible |
✅ dssVisibleRows: 96 immediately — no ~96-frame refill gap (the fix's exact promise) |
| Display-status authority (#1 core / #4) | connect FLEX, read renderstats |
✅ fftAverage 0 → 50, waterfallLineDurationMs 100 → 80 — radio values override local defaults |
| #1 throttle gate | unit test | ✅ logic covered (8 cases); not forceable offscreen (no sustained render pressure) |
| #4 weightedAverage guard | code + authority path | ✅ known-flag set before weightedAverageReported emit; rides the proven authority path |
| #5 Kiwi trace immediacy | code review | ⚪ non-blocking; deliberate hidden early-return retained by design |
| Stability / leak | log + RSS across repeated source switches | ✅ no crashes/asserts; RSS steady ~399 MB, waterfallAllocatedBytes stable ~107 MB |
| Removed dead counters | renderstats schema |
✅ waterfallHiddenHistoryRowsPerSec / dssHiddenHistoryRowsPerSec gone from the per-pan block |
One minor finding (non-blocking)
renderstats.totals.hiddenDssHistoryRowsPerSec still exists and aggregates the per-pan dssHiddenHistoryRowsPerSec field that the drop commit (aaefb3ef) removed — so it now reads a missing key and is structurally always 0: the same "misleading always-0 telemetry" the drop commit set out to eliminate, just left behind in the totals roll-up. Cosmetic — a one-line follow-up (drop the hiddenDssHistoryRowsPerSec accumulator + its totals key in AutomationServer.cpp), not a merge blocker.
Verdict
The substantive behavior works as designed on live data — most importantly blocker #2 (hidden-Flex DSS ring stays warm; instant switch-back) is confirmed end-to-end, and the radio-authoritative display-status path demonstrably overrides local defaults. No regressions, no leak, no crashes. Approval stands; the only new item is the cosmetic leftover telemetry field above.
QA driven via the automation bridge; RX-only, no TX action taken.
Summary
This draft rolls up the performance/authority follow-up to #2465, #4126,
#4083, and #4081.
It makes the radio/profile authoritative for FFT average, FPS,
weighted-average, and waterfall line duration; bounds DSS scrollback to the
visible 3D mode that can use it; stops hidden Flex/Kiwi sources from performing
waterfall/DSS/render work; and adds one automation snapshot for pan, waterfall,
3DSS, shared-scheduler, and WAVE analysis.
Root-cause deep dive
The shared scheduler introduced around #4139 was not an unconditional 60 FPS
source. Live bridge counters showed it batching and pacing requests according
to actual dirty work. The significant amplification was upstream: stale radio
commands, duplicate/hidden history work, and redraw requests produced by data
that was not visible.
What changed
Radio/profile authority
DisplayFftAverage,DisplayFftFps,DisplayFftWeightedAvg, andDisplayWfLineDurationclient settings.the waterfall rate nudge.
PanadapterModelstatus intoSpectrumWidgetand the Displaypanel with signal blockers, so status cannot echo a command back to the
radio.
line duration are held out of the widgets so the pre-cap radio values remain
the restore targets.
Bounded rendering work
cached histories on source/profile switches and rebuild them from fresh rows
for the newly visible source.
scheduler requests, or QRhi redraws.
not let hidden FFT frames create fallback-waterfall rows or redraws.
The explicit tradeoff is that a newly selected source rebuilds deep scrollback
from fresh data instead of continuously paying CPU and memory to maintain an
invisible history. The small viewport snapshot still makes the toggle
immediate.
Automation coverage
get renderstats [reset]now returns, in one interval-aligned snapshot:Live before/after proof
Hardware: FLEX-8400M, firmware 4.2.18.41174, Apple M5/Metal, one pan,
receive-only. Each statistic is a fixed bridge counter window; measured time is
instrumented GUI-thread work, not whole-process CPU percentage.
WAVE remained independently visible in the same combined snapshots at roughly
95-100 appends/s and 19-21 paints/s. Its measured paint cost varied around
2.4-3.9 ms/s; this patch instruments WAVE but does not claim a WAVE rendering
optimization.
Proof
The radio-owned Display values are visible alongside the live 3D stacked trace
and waterfall after the change:
Validation
cmake --build build -j8ctest --test-dir build --output-on-failure— 113/113 passed; one intentional quarantine skiptransmitting=false; no TX performedpython3 tools/gen_bridge_docs.py --checkpython3 tools/check_engine_boundary.py --strict— zero blockerspython3 tools/check_a11y.py— no new findings; existing warning baseline onlygit diff --checkRelated: #4221, #4222, #4223
👨🏼💻 Generated with OpenAI Codex (GPT-5.6 Sol) and tested by @jensenpat