Skip to content

[codex] Consolidate profile load hold timing#3573

Open
rfoust wants to merge 1 commit into
aethersdr:mainfrom
rfoust:codex/profile-load-followup-3572
Open

[codex] Consolidate profile load hold timing#3573
rfoust wants to merge 1 commit into
aethersdr:mainfrom
rfoust:codex/profile-load-followup-3572

Conversation

@rfoust

@rfoust rfoust commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #3572. This follow-up consolidates the profile-load radio-state write hold into the shared profile-load command helper, derives the delayed pan-dimension flush and post-hold recovery timers from that same hold window, and names the internal suppressed-write sentinel so it is clear the callback is not reporting a SmartSDR protocol response. The goal is to keep AetherSDR from sending profile-owned slice/panadapter state back to the radio while the radio is still restoring its saved profile/session layout.

Constitution principle honored

Principle I - FlexLib Is The Protocol Authority. The change keeps profile recall radio-state timing centralized around the radio-authoritative profile-load path instead of scattering independent magic delays through the client.

Test plan

  • Local build passes (cmake --build build --target profile_load_command_test AetherSDR aprs_packet_test aprs_messenger_test -j4)
  • Behavior verified on a real radio if applicable
  • Existing tests pass (CI)
  • Reproduction steps documented if user-reported bug

Local validation run:

  • ctest --test-dir build --output-on-failure -R 'profile_load_command_test|profile_transfer_test|radio_status_ownership_test|slice_recreate_policy_test|transmit_model_test'
  • ctest --test-dir build --output-on-failure -E '^theme_manager_test$'
  • git diff --check

Checklist

  • Commits are signed (docs/COMMIT-SIGNING.md)
  • No new flat-key AppSettings calls - use nested-JSON-under-one-key (Principle V)
  • All meter UI uses MeterSmoother (Principle II)
  • Documentation updated if user-visible behavior changed (no user-facing docs needed; internal timing comment/test updated)
  • Security-sensitive changes reference a GHSA if applicable

@rfoust rfoust force-pushed the codex/profile-load-followup-3572 branch from 0e603f3 to c4263b3 Compare June 14, 2026 05:29
@rfoust rfoust force-pushed the codex/profile-load-followup-3572 branch from c4263b3 to b731615 Compare June 14, 2026 23:43
@rfoust rfoust self-assigned this Jun 14, 2026
@rfoust rfoust marked this pull request as ready for review June 14, 2026 23:45
@rfoust rfoust requested review from a team as code owners June 14, 2026 23:45
Copilot AI review requested due to automatic review settings June 14, 2026 23:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR centralizes profile-load “radio-state write hold” timing into the shared profile-load command helper so that dependent recovery timers (deferred pan-dimension flush and post-hold recovery) stay derived from the same source of truth, reducing duplicated magic delays across model/UI code.

Changes:

  • Hoists kProfileLoadStateWriteHoldMs (and derived recovery delays) into ProfileLoadCommand.h and updates call sites to use the shared constants.
  • Replaces the hard-coded suppressed-write callback code with a named sentinel constant.
  • Extends profile_load_command_test to assert the derived recovery delays are ordered after the hold window.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/profile_load_command_test.cpp Adds assertions ensuring recovery delays remain ordered relative to the shared hold duration.
src/models/RadioModel.cpp Uses the named suppressed-write sentinel (and removes the local duplicated hold constant).
src/models/ProfileLoadCommand.h Introduces shared profile-load timing constants and a named suppressed-command sentinel.
src/gui/MainWindow_Wiring.cpp Removes duplicated hold constants and derives delayed recovery singleShot timings from shared constants.

Comment thread src/models/RadioModel.cpp
Comment on lines 3653 to 3656
if (cb) {
cb(0x50000061, QStringLiteral("suppressed during profile load"));
cb(kProfileLoadSuppressedCommandCode,
QStringLiteral("suppressed during profile load"));
}

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

Reviewed the diff against the branch — this is a clean, well-scoped consolidation. Thanks @rfoust. 🙏

What's good

  • The hold window is now a single source of truth. kProfileLoadStateWriteHoldMs moves into ProfileLoadCommand.h, and the local copies in both RadioModel.cpp and MainWindow_Wiring.cpp (two functions) are removed. Both consumers already #include "ProfileLoadCommand.h", so the shared constant resolves cleanly.
  • Behavior is preserved exactly: the derived timers reproduce the old magic delays (10000 + 1000 = 11000, 10000 + 1250 = 11250), so the two QTimer::singleShot calls are unchanged at runtime while no longer being free-floating literals.
  • The static_cast<int> on the qint64 base is safe at these magnitudes (~11k, far under INT_MAX), and QTimer::singleShot takes int, so the narrowing is correct.
  • Naming kProfileLoadSuppressedCommandCode and documenting that it is not a SmartSDR protocol code is a genuine readability win at the suppression site.
  • The two new test invariants (deferred pan flush > hold, post-hold recovery > deferred flush) lock in the ordering relationship, so a future edit to the base hold can't silently invert the sequence. Nice touch.

On the Copilot finding (RadioModel.cpp:3654) — the concern that the suppressed-write path invokes cb(...) with a non-zero code while returning 0, which callbacks may misread as a real radio error, is a legitimate observation. But it's pre-existing behavior: this PR only renames the literal and adds a clarifying comment; it doesn't alter the callback contract. So it's not a blocker for this consolidation. If you agree it's worth addressing, it'd be cleaner as a focused follow-up — either report suppression as a benign completion (cb(0, ...)) or skip the callback entirely — after confirming what the registered callbacks actually do with code != 0.

No scope creep, no AppSettings/RAII concerns, includes are correct. Looks good to me.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Profile-load hardening: follow-up tidy-ups from #3563 review

2 participants