refactor(warnings): unify startup warning checks into a shared state machine#7498
Draft
raphaelcoeffic wants to merge 6 commits into
Draft
refactor(warnings): unify startup warning checks into a shared state machine#7498raphaelcoeffic wants to merge 6 commits into
raphaelcoeffic wants to merge 6 commits into
Conversation
raphaelcoeffic
force-pushed
the
model-load-state-machine
branch
from
June 26, 2026 10:23
caacbe9 to
1c8721b
Compare
…ne (#7499) On a model switch, postModelLoad() ran the throttle/switch warning checks synchronously from inside an LVGL event callback, each spinning MainWindow::blockUntilClose() which re-enters lv_timer_handler() from deep in the call chain. On hardware that re-entrant stack overflows the menus task and hard-faults (intermittent freeze on TX16S internal MPM); the simulator's larger stack hid it. Replace the nested blocking loop on COLORLCD with a core-owned, GUI-agnostic state machine (model_load_sm) polled once per perMain() (flat stack, no re-entrancy). It walks the post-load checks (SD / throttle / switches / failsafe / multi / checklist) and only once all are cleared runs the deferred model-load tail (postModelLoadFinish: pulsesStart + ...). Pulses stay stopped until then, preserving the failsafe-hold safety guarantee. A thin COLORLCD view (model_load_view) mirrors the active state into the existing warning dialogs and feeds key presses back as an acknowledge. Decouple the warning predicates from input refresh so they are pure reads: refreshInputsForWarnings() (getADC + evalInputs + getMovedSwitch) is now called by the drivers (state machine tick and the boot/flightReset blocking loops), and the warning dialogs become display-only views. Boot and flightReset keep their non-re-entrant blocking checkAll() and reuse the same predicates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
raphaelcoeffic
force-pushed
the
model-load-state-machine
branch
from
June 26, 2026 10:23
1c8721b to
269ac63
Compare
raphaelcoeffic
changed the title
1 task
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA warning-check state machine replaces blocking warning handling. It separates warning predicates from input refresh, routes warning display through platform-specific view sync helpers, and updates startup, model-load, and stick-mode flows to arm and advance the new sequence. ChangesModel-load warning flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Tested ok on a previously failing test hardware (TX16S with exact sequence from #5011). Doesn't hardfault with this branch |
Collaborator
|
Seems like overkill - why not just pause widget refresh in selectModel: // Pause widget refresh
MainWindow::instance()->enableWidgetRefresh(false);
// Delete old main view layout
LayoutFactory::deleteCustomScreens();
loadModel(g_eeGeneral.currModelFilename, true);
modelslist.setCurrentModel(model);
// Load new main view layout
LayoutFactory::loadCustomScreens();
// Enable widget refresh
MainWindow::instance()->enableWidgetRefresh(true); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
radio/src/switches.cpp (1)
880-910: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
bad_potsis only assigned whenpotsWarnModeis set.The out-param is left untouched when
g_model.potsWarnModeis false, so it relies on every caller pre-initializing it. All current callers do (= 0), but a future caller passing an uninitialized variable would read garbage. A one-line guarantee at the top keeps the predicate self-contained.🛡️ Initialize unconditionally
bool isSwitchWarningRequired(uint16_t &bad_pots) { bool warn = false; + bad_pots = 0; for (int i = 0; i < switchGetMaxAllSwitches(); i++) {Then drop the redundant
bad_pots = 0;inside thepotsWarnModeblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/switches.cpp` around lines 880 - 910, The out-parameter bad_pots in isSwitchWarningRequired() is only initialized when g_model.potsWarnMode is enabled, so make the predicate self-contained by setting bad_pots to 0 at the start of the function regardless of mode. Then remove the redundant reset inside the potsWarnMode block and keep the rest of the switch/pot warning logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/gui/colorlcd/model_load_view.cpp`:
- Around line 58-66: The widget-refresh guard in modelLoadViewSync() is
currently applied too late and the MainWindow refresh toggle is only a boolean,
so it can briefly render swapped model data and be clobbered by other users like
standalone_lua.cpp. Move the guard logic so it runs before
MainWindow::instance()->run() in perMain(), and update
MainWindow::enableWidgetRefresh()/widgetRefreshEnabled() to use a
reference-counted state instead of a simple flag so multiple callers can
independently disable and restore refresh safely.
In `@radio/src/model_load_sm.cpp`:
- Around line 172-181: The MLS_START_PULSES flow in model_load_sm.cpp has the
silence window starting too late, so audio triggered by postModelLoadFinish()
can escape unmuted; move START_SILENCE_PERIOD() to run before
postModelLoadFinish() while keeping PLAY_MODEL_NAME() first, and leave the state
transitions in the same MLS_START_PULSES branch.
---
Outside diff comments:
In `@radio/src/switches.cpp`:
- Around line 880-910: The out-parameter bad_pots in isSwitchWarningRequired()
is only initialized when g_model.potsWarnMode is enabled, so make the predicate
self-contained by setting bad_pots to 0 at the start of the function regardless
of mode. Then remove the redundant reset inside the potsWarnMode block and keep
the rest of the switch/pot warning logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f664f8b6-5e76-4bfd-8728-e25002742246
📒 Files selected for processing (16)
radio/src/CMakeLists.txtradio/src/edgetx.cppradio/src/edgetx.hradio/src/gui/colorlcd/CMakeLists.txtradio/src/gui/colorlcd/controls/switch_warn_dialog.cppradio/src/gui/colorlcd/controls/switch_warn_dialog.hradio/src/gui/colorlcd/libui/view_text.cppradio/src/gui/colorlcd/libui/view_text.hradio/src/gui/colorlcd/model_load_view.cppradio/src/gui/colorlcd/model_load_view.hradio/src/main.cppradio/src/model_load_sm.cppradio/src/model_load_sm.hradio/src/storage/storage.hradio/src/storage/storage_common.cppradio/src/switches.cpp
Collaborator
This seems indeed much simpler (and carry less risk) and does also fix the issue in my testing |
Member
Author
@philmoz Absolutely! This PR was not really intended as a fix only for this bug. So I would highly prefer your minimal patch as the fix, and continue with this PR to get rid of the deep re-entrant LVGL warning that have been a pain from my point of view for a long time. |
Collaborator
I'm not against a state machine; but I think it goes beyond just the model switch. |
Member
Author
Agreed. IMHO, the way LVGL is used in these scenarios is not correct: we have to by-pass a few things to make it work, and the call stack gets very deep, which is a permanent risk of hitting a stack overflow. For my part, I find state-machines simpler to reason with than deeply nested calls. Also, I have the feeling we have quite some duplicated logic btw. colour and b&w here that we could unify. |
…y points The post-model-load warning state machine (PR #7498) only covered the COLORLCD runtime model switch. Generalise it into one shared "warning checks" state machine driving every entry point on both GUIs, and delete the parallel blocking implementation entirely. State machine (radio/src/warning_checks.{h,cpp}, renamed from model_load_sm): add WarningCheckContext (MODEL_SWITCH / BOOT / FLIGHT_RESET / STICK_MODE) with a per-context terminal action - restart pulses + model-load tail, start pulses, nothing, or restart the mixer. STICK_MODE runs the throttle check only. Terminal state renamed MLS_START_PULSES -> WCS_COMPLETE (it no longer always starts pulses). - B&W GUI is now a second view (gui/common/stdlcd/warning_checks_view.*) over the same machine, driven from the !COLORLCD perMain block and gating guiMain off while a warning is shown. drawSwitchWarningScreen() extracted from the old blocking checkSwitches() loop. - Storage dispatch generalised: every runtime switch (both GUIs) arms the machine instead of running the blocking checkAll(). - Boot routes through the machine (WCC_BOOT) after a small blocking checkBootSpecificWarnings() pre-step (keys-stuck, RTC battery, ext. antenna); the boot pulsesStart() is deferred to the terminal. - flightReset and the radio-setup stick-mode change (all 3 GUIs) route through the machine too; the latter removes a re-entrant LVGL call. Delete the now-unused blocking path: checkAll, both checkThrottleStick, both checkSwitches, checkFailsafe, checkMultiLowPower, checkSDfreeStorage, the B&W readModelNotes (-> setModelChecklistFilename helper) and the COLORLCD readChecklist. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Relocate the warning-check functions out of the catch-all edgetx.cpp into
warning_checks.cpp, next to the state machine that drives them:
- refreshInputsForWarnings()
- isThrottleWarningAlertNeeded(), isFailsafeWarningRequired(),
isMultiLowPowerWarningRequired() (the pure predicates the SM queries)
- checkAlarm()
- checkBootSpecificWarnings() (was static; now exposed via the header so
edgeTxInit() can still call it) and its static helper checkRTCBattery()
Declarations move from edgetx.h to warning_checks.h. Pure code move, no
behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
raphaelcoeffic
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/gui/colorlcd/warning_checks_view.cpp`:
- Around line 43-46: Keep teardown suppression enabled until the dialog close
callback has actually run. In warning_checks_view.cpp, update the teardown flow
around onShownClosed() and the code that calls deleteLater() so s_tearingDown is
not cleared immediately after scheduling deletion; instead clear it only after
the close callback has completed. This ensures onShownClosed() can still
distinguish view-initiated teardown and avoids calling acknowledgeWarningCheck()
or clearing s_shown after a replacement dialog is installed.
In `@radio/src/gui/common/stdlcd/warning_checks_view.cpp`:
- Around line 102-104: The warning acknowledgement in warningChecksRun()
currently uses a raw truthy event_t check, which allows non-key events like
EVT_ENTRY to dismiss the warning. Update warning_checks_view.cpp to gate
acknowledgeWarningCheck() behind the existing key-event predicate(s) used by the
view contract, so only actual user key presses can acknowledge the warning.
In `@radio/src/main.cpp`:
- Around line 561-564: The COLORLCD flow in guiMain currently keeps refreshing
widgets even when a warning check is active, unlike the B&W path. Update guiMain
(and the related event-handling path it shares with the
MainWindow::instance()->run() call) so it mirrors the existing
activeWarningCheck() guard used in the non-COLORLCD branch and skips guiMain(0)
while warnings are non-idle; apply the same change in the other referenced
COLORLCD block as well.
In `@radio/src/warning_checks.cpp`:
- Around line 53-58: The switch-warning refresh lost the flight-mode/mix update
that used to happen in the old predicate, so `refreshInputsForWarnings()` must
now perform that same refresh before `getMovedSwitch()` when
`mixerTaskRunning()` is false. Update `refreshInputsForWarnings()` to invoke the
existing flight-mode/mix evaluation logic alongside
`evalInputs(e_perout_mode_notrainer)` so warning checks see current switch state
during model load and mixer توقف conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 957c53d7-7049-4de8-afde-991fd02c46da
📒 Files selected for processing (22)
radio/src/CMakeLists.txtradio/src/edgetx.cppradio/src/edgetx.hradio/src/gui/128x64/radio_setup.cppradio/src/gui/212x64/radio_setup.cppradio/src/gui/colorlcd/CMakeLists.txtradio/src/gui/colorlcd/libui/view_text.cppradio/src/gui/colorlcd/libui/view_text.hradio/src/gui/colorlcd/radio/radio_setup.cppradio/src/gui/colorlcd/warning_checks_view.cppradio/src/gui/colorlcd/warning_checks_view.hradio/src/gui/common/stdlcd/CMakeLists.txtradio/src/gui/common/stdlcd/menus_common.hradio/src/gui/common/stdlcd/view_text.cppradio/src/gui/common/stdlcd/warning_checks_view.cppradio/src/gui/common/stdlcd/warning_checks_view.hradio/src/main.cppradio/src/storage/storage.hradio/src/storage/storage_common.cppradio/src/switches.cppradio/src/warning_checks.cppradio/src/warning_checks.h
💤 Files with no reviewable changes (2)
- radio/src/edgetx.h
- radio/src/gui/colorlcd/libui/view_text.cpp
✅ Files skipped from review due to trivial changes (1)
- radio/src/gui/common/stdlcd/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- radio/src/storage/storage.h
- radio/src/gui/colorlcd/libui/view_text.h
The WCS_COMPLETE terminal called PLAY_MODEL_NAME() before START_SILENCE_PERIOD(), inverting the pre-refactor order where checkAll() ended with START_SILENCE_PERIOD() and the caller then ran PLAY_MODEL_NAME() and the model-load restart. playModelName() is not gated by the silence period so this was benign, but reorder it to keep the sequence identical to the old blocking path (silence period, then model name, then terminal). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rdering) Two small fixes from the CodeRabbit review of #7498: - isSwitchWarningRequired(): initialise the bad_pots out-param at the top so the predicate is self-contained regardless of potsWarnMode (drop the redundant reset inside the potsWarnMode block). No behaviour change for the current sole caller, which already pre-zeroed it. - COLORLCD: suspend main-view widget refresh *before* MainWindow::run() instead of after. Split the edge-triggered enable/disable out of warningChecksViewSync() into warningChecksViewUpdateRefresh() and call it from perMain() ahead of run(), so the main view is never repainted for the tick on which a warning sequence becomes active. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The full-screen switch/pot warning rendering was B&W view code (already fenced !COLORLCD) stranded in the GUI-agnostic switches.cpp. Move it to its only caller in gui/common/stdlcd/warning_checks_view.cpp as a file-local function, alongside the B&W layout macros it needs. The dead COLORLCD branch of SWITCH_WARNING_LIST_X/Y and the public declaration in menus_common.h are dropped. No behavioural change; symmetric with the COLORLCD view owning its own rendering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
Seems ok - will take a closer look soon. @raphaelcoeffic - do you think some checks should run after exiting SD storage mode? |
Member
Author
Wait a bit, I refactored a bit more to get rid of the re-entrant LVGL main loop altogether. That should unify more between B&W and color UI as well.
Good question, I should look into that as well. No checks after reload seems inconsistent to me as well. |
raphaelcoeffic
marked this pull request as draft
July 1, 2026 10:21
Member
|
It should behave like a full model switch. Nobody knows what happened to the model file. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
While investigating the #7499 HardFault — the top-bar telemetry widget gets repainted against a half-swapped model during a blocking warning loop — it became clear the post-model-load warning checks were spread across several entry points as parallel blocking loops that re-enter the LVGL /
MainWindow::runstack.The minimal, targeted fix for #7499 is #7500 (pause widget refresh during model load, by @philmoz). This PR is the follow-up refactor that removes the underlying re-entrant blocking machinery and unifies all the startup/safety warning checks behind one state machine. It does not itself close #7499.
What this does
Replace the scattered nested-blocking warning loops with a single core-owned, GUI-agnostic state machine (
radio/src/warning_checks.{h,cpp}), polled once perperMain()— flat stack, no re-entrantlv_timer_handler. Every entry point routes through it via aWarningCheckContext, each with its own terminal action:postModelLoadFinish).checkBootSpecificWarnings: keys-stuck, RTC battery, ext. antenna); the bootpulsesStart()is deferred to the terminal.The machine walks SD / throttle / switches / failsafe / multi / checklist and runs the context-specific terminal only once all are cleared, so pulses stay stopped until then — the existing "RX holds failsafe while a warning is shown" guarantee is preserved.
Each GUI is now a thin view over the same machine:
gui/colorlcd/warning_checks_view.*gui/common/stdlcd/warning_checks_view.*(gatesguiMainoff while a warning is shown;drawSwitchWarningScreen()extracted from the old blocking loop)Warning predicates are now pure reads, decoupled from input refresh:
refreshInputsForWarnings()(getADC+evalInputs+getMovedSwitch) is called once per tick by the driver; the views just render the predicates.Cleanup
checkAll, bothcheckThrottleStick, bothcheckSwitches,checkFailsafe,checkMultiLowPower,checkSDfreeStorage, the B&WreadModelNotes(→setModelChecklistFilenamehelper) and the COLORLCDreadChecklist.isThrottleWarningAlertNeeded,isFailsafeWarningRequired,isMultiLowPowerWarningRequired,refreshInputsForWarnings,checkAlarm,checkBootSpecificWarnings/checkRTCBattery) out of the catch-alledgetx.cppintowarning_checks.cpp, with their declarations inwarning_checks.h.Scope / notes
Testing
Relationship to #7499 / #7500
🤖 Generated with Claude Code
Summary by CodeRabbit