Skip to content

[panadapter] Honor radio display state and bound hidden pan rendering#4261

Merged
ten9876 merged 4 commits into
aethersdr:mainfrom
jensenpat:aether/pan-performance-authority
Jul 18, 2026
Merged

[panadapter] Honor radio display state and bound hidden pan rendering#4261
ten9876 merged 4 commits into
aethersdr:mainfrom
jensenpat:aether/pan-performance-authority

Conversation

@jensenpat

Copy link
Copy Markdown
Collaborator

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

Rank PR Owner Contribution to the regression
1 #2465 @jensenpat Added delayed reconcilers that treated stale client settings as desired truth and re-sent them after radio/profile status. It also pushed those values during startup and nudged waterfall rate twice.
2 #4126 @Ozy311 Correctly restored average/weighted-average decode, but copied the same client-authoritative reconcile pattern to both fields.
3 #4083 @rfoust Matched DSS retained history to the full 24,000-row waterfall history in both render modes, allocating and updating about 35.5 MiB per source even when 2D could not display it.
4 #4081 @rfoust Kept hidden Flex/Kiwi sources warm by state-swapping, retaining both deep histories, and feeding hidden Flex DSS from more than one cadence.

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

  • Remove stale DisplayFftAverage, DisplayFftFps,
    DisplayFftWeightedAvg, and DisplayWfLineDuration client settings.
  • Delete all four delayed reassert timers, cooldown maps, startup pushes, and
    the waterfall rate nudge.
  • Route live PanadapterModel status into SpectrumWidget and the Display
    panel with signal blockers, so status cannot echo a command back to the
    radio.
  • Preserve the existing adaptive-throttle exception: transient capped FPS and
    line duration are held out of the widgets so the pre-cap radio values remain
    the restore targets.
  • Document these four fields explicitly as radio-owned.

Bounded rendering work

  • Retain deep DSS scrollback only for the visible source in 3D mode.
  • Release DSS retained storage immediately on 3D -> 2D.
  • Keep only small visible snapshots for hidden Flex/Kiwi sources; discard deep
    cached histories on source/profile switches and rebuild them from fresh rows
    for the newly visible source.
  • Return before hidden waterfall color conversion, viewport writes, DSS work,
    scheduler requests, or QRhi redraws.
  • Cache the latest hidden Flex FFT bins for an immediate trace switch, but do
    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:

  • per-pan 2D/3D FFT, GPU frame, overlay, upload, waterfall, and DSS rates/costs;
  • visible versus hidden Flex/Kiwi work;
  • current and cached RGB/DSS allocation totals;
  • shared render-scheduler requests, flushes, and coalescing;
  • every WAVE scope's append, paint, and frame cadence;
  • non-overlapping headline GUI-thread timings.

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.

Scenario Before After Result
2D DSS retained-history bytes 37,248,000 0 Eliminated in a mode that cannot display it
2D total DSS bytes 38,169,600 921,600 -97.6%
2D DSS history writes 24.82 rows/s 0 rows/s Eliminated
Hidden-source RGB allocation 275,650,816 139,906,816 -49.2%
Hidden-source DSS allocation 75,417,600 921,600 -98.8%
Hidden RGB history writes 24.82 rows/s 0 rows/s Eliminated
Hidden DSS live/history writes 49.62 / 49.62 rows/s 0 / 0 rows/s Eliminated
Hidden waterfall processing 3.416 ms/s 0.00004 ms/s Packet admission only
Hidden GPU frames 24.70 frames/s 0 frames/s Eliminated
Hidden scheduler requests active 0 Eliminated
Visible 3D DSS retained bytes 37,248,000 37,248,000 Feature behavior retained
Visible 3D DSS history cadence 24.81 rows/s 24.79 rows/s Feature behavior retained

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:

AetherSDR 3D pan and waterfall proof

Validation

  • cmake --build build -j8
  • ctest --test-dir build --output-on-failure — 113/113 passed; one intentional quarantine skip
  • Focused DSS, pan model, and profile-load command tests
  • Live bridge proof in 2D, 3D, and hidden-Flex-behind-Kiwi scenarios
  • Radio connected after the run with transmitting=false; no TX performed
  • python3 tools/gen_bridge_docs.py --check
  • python3 tools/check_engine_boundary.py --strict — zero blockers
  • python3 tools/check_a11y.py — no new findings; existing warning baseline only
  • git diff --check
  • Signed commit verified with the maintainer ED25519 key

Related: #4221, #4222, #4223

👨🏼‍💻 Generated with OpenAI Codex (GPT-5.6 Sol) and tested by @jensenpat

@jensenpat jensenpat self-assigned this Jul 16, 2026
@jensenpat jensenpat changed the title Honor radio display state and bound hidden pan rendering [pan] Honor radio display state and bound hidden pan rendering Jul 16, 2026
@jensenpat jensenpat changed the title [pan] Honor radio display state and bound hidden pan rendering [panadapter] Honor radio display state and bound hidden pan rendering Jul 16, 2026
@jensenpat
jensenpat marked this pull request as ready for review July 16, 2026 15:36
@jensenpat
jensenpat requested review from a team as code owners July 16, 2026 15:36
@jensenpat jensenpat added the priority: high High priority label Jul 16, 2026

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — the waterfallHiddenHistoryRows increment is unreachable (function already returned for hidden sources), so the documented waterfallHiddenHistoryRowsPerSec field is structurally always 0.

Non-blocking notes

  • AutomationServer renderstats: when the first SpectrumWidget's scheduler reports enabled=false, the loop calls renderSchedulerStatsSnapshot(reset) again on subsequent widgets — a harmless double-reset of the shared scheduler counters, but worth a comment.
  • SpectrumOverlayMenu::syncPanProcessingSettings dereferences m_avgLabel/m_fpsLabel after only null-checking the sliders; this matches the existing syncDisplaySettings pattern, so it's consistent, just noting it.

🤖 aethersdr-agent · cost: $6.5276 · model: claude-opus-4-8

Comment thread src/gui/SpectrumWidget.cpp Outdated

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: the waterfallHiddenHistoryRows increment is dead code (unreachable after the early return), so the waterfallHiddenHistoryRowsPerSec counter the docs advertise can never be non-zero.
  • appendDssHistoryRow: passes !m_waterfallWriteVisible as hiddenStream, but that's always false on the only reachable path — so dssHiddenHistoryRows never 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 beginWaterfallStreamWrite admission point where hidden work is actually observable.

🤖 aethersdr-agent · cost: $4.5299 · model: claude-opus-4-8

Comment thread src/gui/SpectrumWidget.cpp Outdated
Comment thread src/gui/SpectrumWidget.cpp Outdated
@jensenpat jensenpat linked an issue Jul 16, 2026 that may be closed by this pull request
rfoust
rfoust previously requested changes Jul 17, 2026

@rfoust rfoust left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the PR's merge result (e19dfbab) against current upstream/main (5371ec03). The build and runnable tests pass, but I found two blockers:

  1. Adaptive throttling can overwrite newer radio/profile state.
    wirePanDisplayStatus() ignores every reported FPS and waterfall-duration update while m_adaptiveThrottleActive, not just adaptive-cap echoes. When throttling ends, adaptiveThrottleChanged(false) sends the values retained in SpectrumWidget back 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.

  2. This PR is configured to close #4221, but it does not implement that issue's waterfall-smoothness fix.
    The GPU path still derives rowOffset directly from integer m_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.
  • ping and get renderstats reset returned 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.

@jensenpat jensenpat added the help wanted Extra attention needed label Jul 17, 2026
@Ozy311

Ozy311 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

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 get renderstats byte accounting (dssAllocatedBytes / waterfallAllocatedBytes) and process RSS every 15 s, in the 3D waterfall render mode.

Builds: Linux (Qt 6.10.3, from source at this PR's head) · macOS (from source) · Windows (the CI Windows-Installer portable artifact, verified by SHA-256).

The bounded pan memory holds — identically — on every OS

dssAllocatedBytes is pinned flat for the entire soak and is byte-identical across all three platforms:

Render mode dssAllocatedBytes
2D 921,600
3D waterfall 38,169,600 (36.4 MB)

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 dssAllocatedBytes stays pinned — i.e. the growth is outside the DSS store this PR governs. I A/B'd against current main (pre-this-PR) under identical conditions, comparing the same steady-state window (10–30 min elapsed, warm-up excluded):

Build (Linux, 3D) RSS steady drift, matched window
this PR 1.54 MB/min
main (baseline) 1.11 MB/min

main drifts too, so the Linux RSS creep is pre-existing, not introduced here — and it's Linux-only (macOS/Windows flat), consistent with a driver/allocator (Mesa/glibc) interaction rather than the spectrum code. This PR's Linux slope runs above main's over the matched window (1.54 vs 1.11 MB/min, ~40% higher), but that's a single run and within the run-to-run variance I'd expect from that kind of effect — I wouldn't read it as a regression without repeats. Glad to open a separate tracking issue if it's useful.

Net

The 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 main as well. RX-only throughout — no transmit.

73, Ozy K6OZY
AI compute partnership: cloaked.agency — (model: claude-opus-4-8)

@NF0T

NF0T commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Reviewed independently alongside @rfoust's CHANGES_REQUESTED — confirmed blocker #1 directly (wirePanDisplayStatus() really does drop fps/waterfall-line-duration updates while m_adaptiveThrottleActive, and adaptiveThrottleChanged(false, ...) in MainWindow.cpp:1759-1792 really does push the stale cached value back to the radio when throttle lifts). Adding a few things my pass turned up that aren't covered by rfoust's review or the two bot passes:

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. updateSpectrum()'s hidden-Flex branch used to call pushDssRowForWaterfallStream(false, m_bins) every frame specifically to keep the hidden native/Flex DSS 3D ring warm — there's a deleted comment explaining this existed to solve a ~96-frame refill staleness problem. The diff replaces that call with a bare early return, so nothing feeds that ring anymore, reintroducing the exact staleness the removed code prevented — and directly contradicting the new comment promising the small live rings stay "warm for an immediate toggle." Knock-on effect: pushDssRowForWaterfallStream()'s hidden-source branch becomes unreachable dead code, so the new dssHiddenLiveRowsPerSec telemetry (documented as exposing "#4081 amplification") can never report nonzero — a third instance of the "hidden counter structurally always 0" pattern the bot reviews already flagged twice elsewhere, just not caught for this counter.

2. Asymmetric guard placement makes Kiwi-source switching non-immediate while Flex stays immediate. updateSpectrum() updates Flex's cached trace before its hidden-source early return; updateKiwiSdrWaterfallRow()'s equivalent guard sits at the top, before any Kiwi trace derivation. Switching to Kiwi shows a stale trace where switching to Flex is immediate — an inconsistency between two source types the PR otherwise treats symmetrically.

Smaller correctness gaps:

3. weightedAverage is missing the "unknown" guard the other three fields have. In wirePanDisplayStatus(), average/fps/lineDuration all guard on "not yet known" (>= 0/> 0), but setFftWeightedAvg(weighted) runs unconditionally and PanadapterModel::m_weightedAverage defaults to false with no unknown sentinel — a freshly-constructed pan whose real radio-side value is true shows the checkbox as false until the first status packet.

4. Stale comments in three untouched files. PanadapterModel.h/.cpp and RadioModel.h still describe the deleted reconcile mechanism this PR inverts ("so MainWindow can reconcile the user's desired value..."). Worth a follow-up doc pass so future readers of those files don't conclude the opposite of what's now true.

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 AppSettings keys, four hand-written connections that must each remember not to write back — with no lint rule, CI check, or distinct type to catch it structurally. Notably, the sibling setters right next to the fixed ones (setWfColorGain/setWfBlackLevel) still persist to AppSettings — the established local pattern a future contributor would copy. Given this is the second rollup fix for the same recurring class, might be worth a one-line note in AGENTS.md calling out the specific anti-pattern (writing radio-echoed status into a setter that also persists) rather than relying on the next contributor rediscovering it.

6. CONTRIBUTING.md's parallel "radio-authoritative" quick-reference wasn't updated to match AGENTS.md's more complete list — same gap as #5 but at the doc layer.

Cleanup, non-blocking:

  • get renderstats duplicates the widget-enumeration/dedupe loop get panstats/get wavestats already implement, rather than reusing it.
  • syncPanProcessingSettings duplicates syncDisplaySettings's widget-setting code for average/fps/weighted-avg instead of a shared helper.
  • The 3-statement discard sequence in discardRetainedHistory's two callers (setKiwiSdrWaterfallActive/setKiwiSdrWaterfallProfile) is copy-pasted rather than factored.
  • On the exact hot path this PR optimizes, hidden frames still pay for a redundant flag re-check plus constructing/starting an ingest-cost timer that measures ~0 work before returning.
  • A dead conditional left over from the refactor in updateSpectrum(): if (m_kiwiSdrWaterfallActive) {...return;} immediately followed by if (!m_kiwiSdrWaterfallActive) {...} — the second is now always true.

One process note for whoever's tracking merge readiness: this touches AGENTS.md, which is Tier 1 per CODEOWNERS — @rfoust's review satisfies the Tier 3 gate, but merging will still need @ten9876 specifically for the Tier 1 gate.

Happy to re-review once rfoust's two blockers and #1 above are addressed.

@NF0T NF0T self-assigned this Jul 17, 2026

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. Adaptive throttle can overwrite newer radio/profile state (rfoust #1) — confirmed inline below. wirePanDisplayStatus drops every fps/wf-duration update while m_adaptiveThrottleActive, and adaptiveThrottleChanged(false) (MainWindow.cpp:1759-1792) restores the retained pre-throttle value, so a profile load / second client changing those during throttling is lost.
  2. Hidden-Flex DSS live ring is no longer fed (NF0T #1) — confirmed: the pushDssRowForWaterfallStream(false, m_bins) call in updateSpectrum'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 makes dssHiddenLiveRowsPerSec structurally always 0 — the same dead-counter pattern the bot threads flagged at SpectrumWidget.cpp:4120/:3989, just for a third counter.
  3. Closes #4221 but doesn't implement its waterfall-smoothness fix (rfoust #2) — the GPU path still derives rowOffset from integer m_wfWriteRow; no sub-pixel/time interpolation. Unlink #4221 or implement separately.

Correctness (should fix)

  1. weightedAverage missing the unknown-guard (NF0T #3) — confirmed inline below.
  2. Asymmetric guard makes Kiwi source-switch non-immediate (NF0T #2) — updateSpectrum updates Flex's cached trace before its hidden early-return, but updateKiwiSdrWaterfallRow'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 — plus dssHiddenLiveRowsPerSec from #2). The bridge telemetry documented as exposing hidden-source retention can never report nonzero.
  • Dead conditional in updateSpectrum: if (m_kiwiSdrWaterfallActive){…return;} immediately followed by if (!m_kiwiSdrWaterfallActive){…} (second always true).
  • Stale comments still describe the deleted reconcile mechanism in PanadapterModel.h/.cpp and RadioModel.h (not in this diff, so a follow-up doc pass); CONTRIBUTING.md's radio-authoritative quick-reference wasn't synced to AGENTS.md.
  • Duplication: get renderstats re-implements the get panstats/get wavestats widget-enum loop; syncPanProcessingSettings duplicates syncDisplaySettings.

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.

Comment thread src/gui/MainWindow_Wiring.cpp Outdated
Comment thread src/gui/MainWindow_Wiring.cpp Outdated
…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>
@ten9876

ten9876 commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Pushed fixes for the blockers + correctness/doc items in 44c35ec6 (builds clean; display_status_gate_test 8/8, dss_renderer_test, flex_waveform_model_test pass).

Fixed

  • Blocker PHONE applet: DEXP commands rejected on firmware v1.4.0.0 #1 (throttle overwrites newer radio state)wirePanDisplayStatus no longer drops every fps / wf-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 value, so a profile load / second-client change during throttle survives the lift instead of being clobbered by the stale restore target. The decision is a pure applyThrottledDisplayReport() in DisplayStatusGate.h, covered by the new display_status_gate_test@rfoust's requested regression test. adaptiveWfMsForCap() promoted to public for the line-duration echo check.
  • Blocker build(deps): Bump github/codeql-action from 3 to 4 #2 (hidden-Flex DSS live ring not fed) — restored the pushDssRowForWaterfallStream(false, m_bins) feed in updateSpectrum's Kiwi-active branch, warming the small hidden-Flex DSS live ring for an immediate 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 dssHiddenLiveRowsPerSec. (@NF0T PHONE applet: DEXP commands rejected on firmware v1.4.0.0 #1)
  • weightedAverage unknown guardPanadapterModel::weightedAverageKnown() (set once the radio reports weighted_average); handler + seed guard on it, so a fresh pan no longer paints a definitive unchecked box. (@NF0T build(deps): Bump actions/checkout from 4 to 6 #3)
  • DocsAGENTS.md anti-pattern note (don't write a radio-echoed status value into a persisting setter — the recurring class); CONTRIBUTING.md radio-authoritative reference synced; stale "MainWindow can reconcile the user's desired value" comments in PanadapterModel.{h,cpp} / RadioModel.h corrected. (@NF0T APD status indicators need refinement #4/Implement XVTR band sub-menu #5/Multi-client: AetherSDR interferes with other connected clients #6)

Needs a one-click UI action (Blocker #3)

#4221 is a manually-linked closing issue ("Related: #4221" in the body is not a closing keyword), and GitHub's API has no mutation to remove a manual link — please unlink it from the Development sidebar (or implement the sub-pixel interpolation separately). (@rfoust #2)

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:

  • Kiwi source-switch asymmetry (@NF0T build(deps): Bump github/codeql-action from 3 to 4 #2): move updateKiwiSdrWaterfallRow's if (!m_kiwiSdrWaterfallActive) return; to after the Kiwi trace derivation (mirroring how updateSpectrum updates m_bins before its hidden early-return), so switching to Kiwi isn't stale for a frame.
  • Dead hidden-history counters: waterfallHiddenHistoryRows (appendHistoryRow early-returns for hidden) and dssHiddenHistoryRows (hidden capacity is 0) are 0 by construction. Drop the two fields + their panstatsSnapshot/automation-bridge.md rows. (dssHiddenLiveRows is now meaningful again after Blocker build(deps): Bump github/codeql-action from 3 to 4 #2 — keep it.)
  • Dead conditional flatten: after the if (m_kiwiSdrWaterfallActive) { … return; }, the several following if (!m_kiwiSdrWaterfallActive) blocks in updateSpectrum are always true — de-indent when convenient (larger mechanical change).
  • Dedups: get renderstats vs the get panstats/get wavestats enum loop; syncPanProcessingSettings vs syncDisplaySettings; discardRetainedHistory's two call sites.

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>
@ten9876

ten9876 commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Follow-up in aaefb3ef:

  • Dead hidden-history counters removedwaterfallHiddenHistoryRows and dssHiddenHistoryRows (both structurally always 0) and their always-0 bridge fields are gone, along with retainDssHistoryRow's now-unused hiddenStream param and the misleading doc rows. dssHiddenLiveRows is kept — it's meaningful again now that the hidden-Flex DSS live ring is fed.

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. updateSpectrum caches raw, frame-agnostic bins (m_bins), so it can update them while hidden. But updateKiwiSdrWaterfallRow derives m_kiwiSdrFftTrace by mapping the Kiwi row into the currently-visible frame (m_centerMhz/destWidth off the visible surface). While Kiwi is hidden the visible frame is Flex's, so deriving the Kiwi trace there would map it onto the wrong frame — worse than the one-frame snapshot staleness. A true symmetric fix would cache the raw Kiwi row (not the derived trace) and remap on switch, which is a larger change than a guard-move. Left as-is — flagging so it's a conscious decision rather than an oversight.

Not done (larger mechanical / pure-refactor, all @NF0T non-blocking): the if (!m_kiwiSdrWaterfallActive) de-indent in updateSpectrum, and the get renderstats / syncPanProcessingSettings / discardRetainedHistory dedups. Happy to take those in a dedicated cleanup pass if wanted, but I'd rather not fold a large de-indent into this review round.

Net: both blockers + the correctness (weightedAvg guard) + docs + dead-counter cleanup are in (44c35ec6, aaefb3ef); remaining is the one-click #4221 UI unlink and the optional cleanups above.

@K5PTB

K5PTB commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Red-Team Review — PR #4261

Adversarial 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.


Findings

1. [BLOCKING] PR cannot currently merge — mergeable: CONFLICTING, mergeStateStatus: DIRTY.
Confirmed live via GraphQL as of the current head (aaefb3ef). Root cause: the PR's recorded baseRefOid is pinned to b9735c79 ("Add a cross-needle power and SWR meter applet") — but that work merged into main separately, as #4246, via a different (squash) commit on 2026‑07‑14. main has since advanced 10 more commits past that point (currently b6101983), and this branch has never been rebased/synced against it. I confirmed the content itself hasn't diverged — CrossNeedleMeterWidget.cpp/.h are byte-identical between the PR's inherited copy and current main — so this is very likely a mechanical rebase, not a real logic conflict, but it's a hard blocker as-is: the merge button is disabled right now. It also plausibly explains why gh pr checks reports zero checks on the branch — CI frequently declines to run required workflows against an unmergeable PR. (The earlier bot review's "CI is green" refers to an older commit, before this state existed.)
Fix: rebase/update the branch onto current main and confirm CI runs fresh on the resulting head.

2. [BLOCKING] @rfoust's CHANGES_REQUESTED is still the active, undismissed review state — and one of the two blockers is still live.
Of rfoust's two blockers: #1 (adaptive-throttle overwriting newer radio state) is fixed and verified (see below). #2 — the PR is configured to close #4221 ("Waterfall a little jumpy/not smooth", still OPEN) without implementing that fix — is not resolved. closingIssuesReferences still lists #4221. @ten9876's 7/18 comment notes this needs a manual "unlink from Development sidebar" action (the API can't do it) — that hasn't happened yet. As-is, merging will silently auto-close #4221 with the underlying jumpiness unfixed.
Fix: unlink #4221 from the Development sidebar, or implement the sub-pixel/time interpolation rfoust described as the alternative.

3. [BLOCKING] Tier‑1 approval not yet granted. This PR touches AGENTS.md (Tier 1 per CODEOWNERS, gated to @ten9876). His 7/18 review is COMMENTED, not APPROVED — explicitly withheld pending the blockers. No approval has landed since the two follow-up commits.

No new blocking code-correctness findings from my own read. I verified the two substantive fixes directly:

  • applyThrottledDisplayReport() (DisplayStatusGate.h) correctly distinguishes the adaptive cap's own echo from a genuine newer radio/profile update, and the call sites pass the live cap, not a stale snapshot. Built and ran display_status_gate_test myself: 8/8 pass.
  • The hidden-Flex DSS live-ring restore and the weightedAverageKnown() unknown-sentinel guard both read correctly against the surrounding code. Built and ran dss_renderer_test: pass.
  • AGENTS.md/CONTRIBUTING.md doc claims checked against the actual diff hunks — both real, accurate, well-worded.

Non-blocking (already tracked; my read agrees with the severity calls already made)

  • Kiwi source-switch trace asymmetry (@NF0T build(deps): Bump github/codeql-action from 3 to 4 #2) — deliberately left as-is; the maintainer's technical reasoning (remapping onto the wrong visible frame would be worse than the current one-frame staleness) holds up on my read. Worth a short code comment recording why, so it isn't "fixed" into a real bug later.
  • Dead conditional in updateSpectrum() (always-true if (!m_kiwiSdrWaterfallActive) after the early return) — cosmetic, deferred.
  • Duplication: get renderstats vs. the get panstats/get wavestats enum loop; syncPanProcessingSettings vs. syncDisplaySettings; discardRetainedHistory's two call sites.
  • Architectural (@NF0T Implement XVTR band sub-menu #5): second instance-level rollup fix for the same "client persists radio-authoritative state" bug class. The new AGENTS.md anti-pattern note is a real mitigation, but sibling setters (setWfColorGain/setWfBlackLevel) still persist to AppSettings — the pattern a future contributor would copy. Worth a maintainer discussion (structural enforcement, not this PR's job) rather than a blocker here.

Checks run

Merge-base/ancestry analysis, closingIssuesReferences (GraphQL), gh pr checks. Isolated worktree build of the PR head (clean configure); targeted build + run of display_status_gate_test (8/8) and dss_renderer_test (pass). Diffed the two fix commits against AGENTS.md/CONTRIBUTING.md/DisplayStatusGate.h/MainWindow_Wiring.cpp/PanadapterModel.{h,cpp} to verify claims rather than trust commit messages. reviewThreads/review state refreshed a second time immediately before writing this — no new activity in between.

CI state

Unknown/none reportedgh pr checks returns zero checks on the current head, almost certainly downstream of finding #1.

Review limits

Did not run tools/check_engine_boundary.py, tools/gen_bridge_docs.py --check, or tools/check_a11y.py myself (time-boxed; CI would normally cover these and isn't currently running against this head). Did not do a full app build/run or exercise the live automation bridge; relied on @jensenpat's live-radio proof and @Ozy311's independent 3-platform soak, both of which read as credible and specific.


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 main, (2) the #4221 link resolved, (3) @ten9876's formal Tier‑1 approval.

🤖 Generated with Claude Code

@ten9876
ten9876 dismissed rfoust’s stale review July 18, 2026 15:15

Changes requested have been implemented.

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() in DisplayStatusGate.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 in display_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 in updateSpectrum'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. dssHiddenLiveRowsPerSec telemetry is meaningful again.
  • Blocker #3#4221 linkage (@rfoust #2): #4221 unlinked — closingIssuesReferences is 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 before weightedAverageReported is 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-false hiddenStream param) 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): updateKiwiSdrWaterfallRow keeps 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.

@ten9876

ten9876 commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

QA test plan — run against the automation bridge (merged head 0cc0fce8)

Built the merged head in an isolated worktree (ccache, all cores), launched offscreen with a scratch HOME (no auto-connect), explicit socket, token auth, and TX disabled. Exercised the changed paths against the live FLEX-8600 (RX only) with deterministic Kiwi activation via the dss inject verb. Instance + worktree torn down afterward; FLEX slot released.

Results

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.

@ten9876
ten9876 merged commit 40935b6 into aethersdr:main Jul 18, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Extra attention needed priority: high High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants