[codex] Harden profile load recovery#3563
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens profile loading and session recovery to avoid AetherSDR pushing profile-owned slice/pan/waterfall state while the radio is restoring a profile, and adds recovery hooks to re-arm client-owned state after profile recall.
Changes:
- Fix Profile Manager TX profile load command to use
profile tx loadand add test coverage for TX/mic profile-load commands. - Introduce a profile-load lifecycle (
profileLoadStarted/profileLoadCompleted) and suppress/defer profile-owned radio-state writes during global profile restore. - Defer pan dimension pushes and adjust noise-floor auto-adjust behavior to avoid dBm scale drift during profile rebuilds; re-arm DAX/TCI state after profile load.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/transmit_model_test.cpp | Adds assertions that TX/mic profile loads send the correct profile tx/mic load commands. |
| src/models/RadioModel.h | Exposes sliceMayBelongToUs() and adds profile-load lifecycle signals and a write-hold state field. |
| src/models/RadioModel.cpp | Implements profile-load detection, write suppression during holds, profile refresh via profile * info, and slice ownership helper. |
| src/gui/SpectrumWidget.h | Adds APIs/state to hold auto noise-floor adjustment and detect pending auto-floor-driven dBm range changes. |
| src/gui/SpectrumWidget.cpp | Implements noise-floor auto-adjust hold logic and echo clearing during holds. |
| src/gui/ProfileManagerDialog.cpp | Fixes TX profile load command to profile tx load. |
| src/gui/MainWindow.h | Adds profile-load hold state and helpers for pan-dimension deferral and recovery passes. |
| src/gui/MainWindow.cpp | Suppresses adaptive throttle restore, bandstack autosave, and reconcile paths during profile-load holds. |
| src/gui/MainWindow_Wiring.cpp | Adds profile-load recovery scheduling, defers pan dimension writes, constrains dBm-range pushes, and re-arms hosted DAX/streams. |
| src/gui/MainWindow_Session.cpp | Wires RadioModel profile-load lifecycle signals into MainWindow recovery/hold behavior; routes pan dimension writes through deferral helper. |
| src/core/TciServer.h | Adds rearmDaxForProfileLoad() API. |
| src/core/TciServer.cpp | Implements TCI DAX re-arming after profile loads (stream removal/reset + ensure). |
| CMakeLists.txt | Registers transmit_model_test with CTest. |
There was a problem hiding this comment.
Thanks for this, @rfoust — this is a careful, well-reasoned hardening pass. I traced the main paths and it holds together. CI is green on all six checks (Linux/macOS/Windows/CodeQL/accessibility).
Copilot finding — false positive, but your response was the right move
The Copilot comment claimed parseProfileLoadCommand() returns the whole command in captured(1). That's a misread of the original raw-string regex (it had no leading capture group), so the original was in fact returning the type. Your refactor in 631783ff is the better outcome regardless: the new src/models/ProfileLoadCommand.h helper is correct and unambiguous —
^\s*profile\s+(global|tx|mic)\s+load\s+"([^"]*)"\s*$
group 1 = type, group 2 = name, .toLower() normalizes, and profile_load_command_test.cpp covers global/tx/mic + the topology-decision + the non-load reject case. Pulling an implicit parser into a tested shared helper is exactly the right call.
What I verified
- The headline bug fix is real and correct.
ProfileManagerDialognow sendsprofile tx load(lines 168/181), matching the applet path; the FlexLibRadio.csreferences for thetransmit-vs-txand create-only semantics are accurate. - Suppression boundary is sound.
isProfileOwnedRadioStateWrite()correctly letsxpixels/ypixelsthrough the low-level guard (so they route through the coalescingrequestPanDimensionsForRadio()deferral) while blocking otherslice set/display pan setwrites — and the hold is time-bounded (10 s) so a missedprofileLoadCompletedcan't wedge writes permanently. - Noise-floor hold is consistent:
noiseFloorAutoAdjustHeld()resets the baseline once on expiry; the local-onlydbmRangeChangeRequestedpath correctly avoids touchingPanadapterStream's decoder scale. The chase-loop reasoning checks out. - Nice catch: registering
transmit_model_testwithadd_test()— it was built but never run before this PR.
Minor (non-blocking)
-
Duplicated
profileLoadMayRebuildRadioTopology. It's definedinlineinProfileLoadCommand.h(== "global") and again inMainWindow_Wiring.cpp's anonymous namespace (case-insensitivecompare). No ODR clash (different namespaces) and they agree in practice since the type is pre-lowercased, but it's two sources of truth for the same rule — if "topology profile" ever broadens pastglobal, one copy can drift. SuggestMainWindow_Wiring.cppinclude the header and drop its local copy. -
Recovery cascade is a set of uncancelable
singleShottimers (350/1200/1500/2500/3500/11000/11250 ms) keyed to the 10 s hold. Each lambda guards onm_shuttingDown/isConnected, so back-to-back profile loads are safe but stack redundant timers. Fine as-is; just noting it if profile recall can be issued rapidly. -
Question: suppressed writes invoke
cb(0x50000061, "suppressed during profile load")— a non-zero (failure-looking) code. The suppressed commands are fire-and-forget slice/display writes, so I don't think any caller surfaces this or retries, but can you confirm no callback path treats that as a user-visible error?
None of these block. Solid work — the scope matches the stated goals and stays out of radio-owned profile content.
🤖 aethersdr-agent · cost: $7.3314 · model: claude-opus-4-8
|
Follow-up to @aethersdr-agent review: pushed 79926f4.
Validated locally:
|
There was a problem hiding this comment.
Thanks for this, @rfoust — this is a careful, well-documented hardening pass. The core bug (profile transmit load → profile tx load in ProfileManagerDialog) is correctly targeted, and the broader profile-load write-hold lifecycle is consistently threaded through the slice/pan/waterfall/DAX paths. The inline comments explaining why each write is deferred are genuinely helpful for future maintainers.
I reviewed against the repo's conventions and looked specifically for bugs, null/leak risks, scope creep, and boundary handling.
Copilot finding — resolved (false positive against current code)
Copilot flagged parseProfileLoadCommand() returning the wrong capture groups. As you note, the refactor in 631783ff settles this: the shared ProfileLoadCommand.h regex ^\s*profile\s+(global|tx|mic)\s+load\s+"([^"]*)"\s*$ cleanly puts the type in captured(1) and the name in captured(2), and profile_load_command_test.cpp covers global/tx/mic plus the topology-decision and the non-load reject case. Topology detection (== "global") works correctly. No action needed.
Looks good
refreshProfiles()switch to FlexLib-styleprofile {global,tx,mic} info— the existinghandleProfileStatusRaw()status parser keys offlist=/current=fields the radio pushes, so theinforequest shape is compatible.- DAX/IQ/TCI re-arm paths correctly gate on
sliceMayBelongToUs()and avoid restoring radio-owned settings, matching the stated non-goals. - Null-guarding follows existing patterns (
panStream()access underpan->panStreamId()checks); no new deref risks introduced. add_test(NAME transmit_model_test)registration and the new TX/mic assertions are a welcome fix — that executable was built but never registered.- The deferred-pan-dimension flush sequence (1500/3500/11000 ms) does cover late deferrals before/after the hold expires, so no dimension pushes are lost.
Minor (non-blocking)
-
Magic-number coupling.
kProfileLoadStateWriteHoldMs = 10000is duplicated in three places (RadioModel.cppfile scope, plus two localconstexprinMainWindow_Wiring.cpp), and the recovery flush/reacquire timers (11000/11250) are implicitly tuned to outlast that 10s hold. If someone later shortens the hold, those flush timers won't track it. Consider a single shared constant (or at least a comment at the timer sites noting the dependency on the hold duration). -
Suppressed writes are silently dropped, not replayed. During the hold window,
slice set/display pan set/waterfall setwrites are dropped with a syntheticcb(0x50000061, …). The PR body documents this for active-slice selection, which is the right call — just flagging that a user gesture landing inside the ~10s post-recall window is discarded rather than deferred-and-replayed. Acceptable as a deliberate tradeoff; worth keeping in mind if users report "my tweak right after a profile load didn't take."
Neither is blocking. Nice work — leaving this as a comment-only review. (CI was still in progress at review time; worth a glance once build/check-windows/check-macos/analyze finish.)
🤖 aethersdr-agent · cost: $3.0831 · model: claude-opus-4-8
79926f4 to
dbe51a5
Compare
Hardens profile loading / session recovery so AetherSDR stops fighting the radio during global profile restore: fixes the Profile Manager TX command (profile transmit load -> profile tx load), adds a timestamp-bounded profile-load write-hold (self-expiring, can't wedge), suppresses/defers profile-owned slice/pan/waterfall writes during restore, holds noise-floor auto-adjust, and re-arms client-owned DAX/IQ/TCI/RX-audio state after recall without touching radio-owned settings. New profile-load lifecycle methods live in MainWindow_Wiring.cpp with signal wiring in MainWindow_Session.cpp (architecture-conformant). Tested: profile_load_command_test, transmit_model_test. Co-authored-by: rfoust <rfoust@users.noreply.github.com>
Summary
This PR hardens profile loading and session recovery so AetherSDR does not fight the radio while the radio is restoring profile-owned state.
The main fixes are:
profile transmit load, while FlexLib/radio usage isprofile tx load. The TX applet path already used the correct command; this makes Profile Manager match it.profile global info,profile tx info, andprofile mic info.xpixels/ypixelspushes until after the global profile load settles. The helper is documented so future lifecycle/resize code does not bypass the deferral and dirty the radio's GUIClient restore snapshot.min_dbm=-130 max_dbm=-40for radio-restored pans on startup/profile recall. The radio's saved pan dBm range is authoritative; the default remains only for explicitly created new panadapters.PanadapterStream's decoder scale. The stream decoder must use the radio's actual min/max dBm range.Root Cause
Several profile-load paths were either using the wrong radio command or were treating profile restore like an ordinary slice/pan update. Global profile recall can rebuild radio-owned topology and live stream state. If AetherSDR pushes display/slice settings too early, it can dirty the radio's restored GUIClient/session state. Separately, the client-side auto noise-floor logic could move the local dBm scale while the stream decoder still needed to use the radio's true min/max dBm range, causing scale drift/chase behavior.
Intentional Non-Goals
This does not make AetherSDR save or override radio-owned profile settings. Frequency, mode, filters, antennas, TX power, tune power, pan count, and saved pan dBm profile contents remain radio-authoritative.
Normal user-driven active-slice selection still works. During the global profile restore window only, outgoing
slice set ... active=1writes are suppressed so profile restore does not dirty radio session state.Validation
git fetch upstreamupstream/main(d43a77a7) before committingcmake --build build --target AetherSDR -j4ctest --test-dir build --output-on-failure -R 'profile_transfer_test|radio_status_ownership_test|slice_recreate_policy_test|transmit_model_test'cmake --build build --target wfm_dsp_test -j4ctest --test-dir build --output-on-failure -E '^theme_manager_test$'git diff --checktheme_manager_testwas excluded from the broad local suite because this sandbox cannot write the macOS AppSettings temp file path it uses. The profile-focused tests and the remaining 34 tests passed on the refreshed upstream base.