Refactored events - #621
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change introduces caller-owned task event groups with exclusive readiness bits and non-blocking app and system event polling. Wi-Fi drivers replace callback arrays with FIFO subscriptions, radio control APIs, and event-group signaling. The Wi-Fi service and Wi-Fi applications consume device events through dedicated polling flows. Application entry points migrate to explicit event-group loops. Tests cover combined event readiness, polling, timeouts, subscription lifecycle, and driver behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
Tactility/Source/app/grovesettings/GroveSettings.cpp (1)
96-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a shared event-loop helper for the repeated boilerplate.
This exact 15-line sequence (construct group, subscribe, wait, drain, unsubscribe, destruct) now appears in every migrated app. A small RAII helper in the app framework would keep the per-app code to the event switch only, and would centralize future changes such as error handling of
app_event_subscribe()or drain semantics. This can be deferred to a follow-up.Modules/app-module/source/symbols.cpp (1)
25-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate
task_event_group_*exports.
root_modulestarts before other modules, and global resolution returns the first matching symbol without rejecting duplicates. Keep these symbols inKERNEL_SYMBOLSonly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a3f589b8-6782-424f-8594-097d04338b0d
📒 Files selected for processing (69)
.claude/rules/app-framework.mdDocumentation/ideas.mdModules/app-module/include/app/event.hModules/app-module/source/event.cppModules/app-module/source/symbols.cppModules/app-module/tests/source/app_event_test.cppModules/app-module/tests/source/app_manager_test.cppModules/app-module/tests/source/combined_event_test.cppPlatforms/platform-esp32/CMakeLists.txtPlatforms/platform-esp32/source/drivers/esp32_wifi.cppPlatforms/platform-esp32/source/drivers/esp32_wifi_pinned.cppPlatforms/platform-posix/source/drivers/mock_wifi.cppTactility/Include/Tactility/service/wifi/Wifi.hTactility/Source/Tactility.cppTactility/Source/app/addgps/AddGps.cppTactility/Source/app/alertdialog/AlertDialog.cppTactility/Source/app/appdetails/AppDetails.cppTactility/Source/app/apphub/AppHubApp.cppTactility/Source/app/apphubdetails/AppHubDetailsApp.cppTactility/Source/app/applist/AppList.cppTactility/Source/app/appsettings/AppSettings.cppTactility/Source/app/apwebserver/ApWebServer.cppTactility/Source/app/audiosettings/AudioSettings.cppTactility/Source/app/boot/Boot.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/btpeersettings/BtPeerSettings.cppTactility/Source/app/chat/ChatApp.cppTactility/Source/app/crashdiagnostics/CrashDiagnostics.cppTactility/Source/app/development/Development.cppTactility/Source/app/files/FilesApp.cppTactility/Source/app/fileselection/FileSelection.cppTactility/Source/app/gpssettings/GpsSettings.cppTactility/Source/app/grovesettings/GroveSettings.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/imageviewer/ImageViewer.cppTactility/Source/app/inputdialog/InputDialog.cppTactility/Source/app/kerneldisplay/KernelDisplay.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/notes/Notes.cppTactility/Source/app/power/Power.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/selectiondialog/SelectionDialog.cppTactility/Source/app/settings/Settings.cppTactility/Source/app/setup/Setup.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timedatesettings/TimeDateSettings.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/app/trackball/TrackballSettings.cppTactility/Source/app/usbsettings/UsbSettings.cppTactility/Source/app/webserversettings/WebServerSettings.cppTactility/Source/app/wifiapsettings/WifiApSettings.cppTactility/Source/app/wificonnect/WifiConnect.cppTactility/Source/app/wifimanage/WifiManage.cppTactility/Source/service/espnow/EspNowHostedTransport.cppTactility/Source/service/wifi/Wifi.cppTactilityKernel/include/tactility/concurrent/task_event_group.hTactilityKernel/include/tactility/drivers/wifi.hTactilityKernel/include/tactility/system_event.hTactilityKernel/source/concurrent/task_event_group.cppTactilityKernel/source/drivers/wifi.cppTactilityKernel/source/symbols.cTactilityKernel/source/system_event.cppTactilityKernel/tests/source/system_event_test.cppTactilityKernel/tests/source/task_event_group_test.cppTests/SdkIntegration/main/Source/main.c
💤 Files with no reviewable changes (2)
- Tactility/Source/Tactility.cpp
- Tactility/Include/Tactility/service/wifi/Wifi.h
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/service/wifi/Wifi.cpp (1)
537-539: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail service startup when the Wi-Fi event consumer cannot start.
When
startWifiEventThread()fails,onStart()only logs the error and still returns success. The service then exposes a started device without consuming scan, connection, or disconnection events. Auto-connect and connection-state updates will not work.Stop and release the device, clear the pause callback, and return startup failure instead of entering this degraded state.
Proposed direction
if (!startWifiEventThread()) { LOG_E(TAG, "Failed to subscribe to WiFi events"); + device_stop(state.device); + device_put(state.device); + state.device = nullptr; + wifi_auto_scan_set_paused_function(nullptr); + return false; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6484b589-88b6-482b-8299-f0e4a9ee4d00
📒 Files selected for processing (39)
Modules/app-module/include/app/event.hModules/app-module/source/event.cppModules/app-module/source/symbols.cppPlatforms/platform-esp32/source/drivers/esp32_wifi.cppPlatforms/platform-posix/source/drivers/mock_wifi.cppTactility/Source/Tactility.cppTactility/Source/app/apphub/AppHubApp.cppTactility/Source/app/applist/AppList.cppTactility/Source/app/appsettings/AppSettings.cppTactility/Source/app/apwebserver/ApWebServer.cppTactility/Source/app/audiosettings/AudioSettings.cppTactility/Source/app/boot/Boot.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/chat/ChatApp.cppTactility/Source/app/crashdiagnostics/CrashDiagnostics.cppTactility/Source/app/development/Development.cppTactility/Source/app/display/Display.cppTactility/Source/app/fileselection/FileSelection.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/imageviewer/ImageViewer.cppTactility/Source/app/inputdialog/InputDialog.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/power/Power.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/selectiondialog/SelectionDialog.cppTactility/Source/app/settings/Settings.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/app/trackball/TrackballSettings.cppTactility/Source/app/usbsettings/UsbSettings.cppTactility/Source/app/webserversettings/WebServerSettings.cppTactility/Source/app/wifimanage/WifiManage.cppTactility/Source/service/wifi/Wifi.cppTactilityKernel/include/tactility/drivers/wifi.hTactilityKernel/source/system_event.cpp
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Tactility/Source/app/appsettings/AppSettings.cpp (1)
93-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the repeated event-loop block into one helper. Every reviewed application repeats the same sequence: construct a
TaskEventGroup, fill anAppEventSubscription, wait withtask_event_group_wait_any(), drain withapp_event_poll(), and stop onAPP_EVENT_CLOSE. The stack outline lists about 40 files with this block. Each copy also ignores theapp_event_subscribe()result, so a fix must be applied 40 times. A helper such asapp_event_run_until_close(appInstanceId, TaskEventGroup*, AppEventSubscription*), or a small RAII wrapper for subscribe/unsubscribe, removes the duplication and centralizes error handling.
Tactility/Source/app/appsettings/AppSettings.cpp#L93-L121: replace the inline loop with the shared helper; this is the representative copy.Tactility/Source/app/apwebserver/ApWebServer.cpp#L129-L153: replace the inline loop with the shared helper.Tactility/Source/app/audiosettings/AudioSettings.cpp#L218-L242: replace the inline loop with the shared helper, then keep the audio-pubsub unsubscribe in place.Tactility/Source/app/development/Development.cpp#L177-L216: replace the inline loop with the shared helper, then keep the timer stop in place.Tactility/Source/app/fileselection/FileSelection.cpp#L65-L86: replace thewhile (true)variant with the shared helper.Tactility/Source/app/i2cscanner/I2cScanner.cpp#L391-L416: replace the inline loop with the shared helper, and move thestopScanningIfRunning()call after the helper returns.Tactility/Source/app/localesettings/LocaleSettings.cpp#L148-L172: replace the inline loop with the shared helper.Tactility/Source/app/screenshot/Screenshot.cpp#L253-L277: replace the inline loop with the shared helper.Tactility/Source/app/selectiondialog/SelectionDialog.cpp#L107-L128: replace thewhile (true)variant with the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: afb1c830-8878-4fc7-8788-db77c529c285
📒 Files selected for processing (33)
Documentation/ideas.mdModules/app-module/include/app/event.hModules/app-module/source/event.cppModules/app-module/source/symbols.cppTactility/Source/app/apphub/AppHubApp.cppTactility/Source/app/applist/AppList.cppTactility/Source/app/appsettings/AppSettings.cppTactility/Source/app/apwebserver/ApWebServer.cppTactility/Source/app/audiosettings/AudioSettings.cppTactility/Source/app/boot/Boot.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/chat/ChatApp.cppTactility/Source/app/crashdiagnostics/CrashDiagnostics.cppTactility/Source/app/development/Development.cppTactility/Source/app/display/Display.cppTactility/Source/app/fileselection/FileSelection.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/imageviewer/ImageViewer.cppTactility/Source/app/inputdialog/InputDialog.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/power/Power.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/selectiondialog/SelectionDialog.cppTactility/Source/app/settings/Settings.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/app/trackball/TrackballSettings.cppTactility/Source/app/usbsettings/UsbSettings.cppTactility/Source/app/webserversettings/WebServerSettings.cpp
💤 Files with no reviewable changes (3)
- Modules/app-module/include/app/event.h
- Modules/app-module/source/symbols.cpp
- Modules/app-module/source/event.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Documentation/ideas.md
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| window_manager_remove(window); | ||
| app_event_unsubscribe(&sub); | ||
| task_event_group_destruct(&event_group); | ||
|
|
||
| service::espnow::unsubscribeReceiver(ctx.receiveSubscription); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Unsubscribe the ESP-NOW receiver before destroying the window.
onReceive() can call ctx->view.displayMessage() from the receiver callback. The current order removes the window before unsubscribeReceiver() runs. A packet received during this interval can access destroyed LVGL widgets.
Unsubscribe the receiver, and ensure that unsubscription quiesces in-flight callbacks, before window_manager_remove(window).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tactility/Source/service/wifi/Wifi.cpp (2)
104-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle pending radio states explicitly.
isRadioOn()returnsfalsefor bothWIFI_RADIO_STATE_OFFandWIFI_RADIO_STATE_ON_PENDING. If the radio isWIFI_RADIO_STATE_ON_PENDINGandenabledisfalse,dispatchSetEnabled()returns without callingwifi_set_radio_off(). The pending enable can then complete after the user requested disable. Query the fullWifiRadioStateand handleWIFI_RADIO_STATE_ON_PENDINGandWIFI_RADIO_STATE_OFF_PENDINGas transitions.Proposed direction
- if (enabled == isRadioOn()) { + WifiRadioState radio = WIFI_RADIO_STATE_OFF; + if (wifi_get_radio_state(state.device, &radio) != ERROR_NONE) { + LOG_E(TAG, "Failed to query WiFi radio state"); + return; + } + if ((enabled && radio == WIFI_RADIO_STATE_ON) || + (!enabled && radio == WIFI_RADIO_STATE_OFF)) { LOG_W(TAG, "Can't enable/disable from current state"); return; }Also applies to: 179-182
539-541: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail startup when the event consumer cannot start.
When
startWifiEventThread()returnsfalse,onStart()only logs the failure and continues. The service then reports successful startup with an active device but no consumer for scan-finished or station-connection-result events. Auto-connect and connection-state cleanup will not run. Retry the subscription or stop and release the device before returningfalse.Proposed cleanup
if (!startWifiEventThread()) { LOG_E(TAG, "Failed to subscribe to WiFi events"); + device_stop(state.device); + device_put(state.device); + state.device = nullptr; + wifi_auto_scan_set_paused_function(nullptr); + return false; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3c82151-f72f-4680-9f0f-05fe5af511f7
📒 Files selected for processing (50)
Devices/btt-panda-touch/bigtreetech,panda-touch.dtsDevices/cyd-2432s024c/cyd,2432s024c.dtsDevices/cyd-2432s024r/cyd,2432s024r.dtsDevices/cyd-2432s028r/cyd,2432s028r.dtsDevices/cyd-2432s028rv3/cyd,2432s028rv3.dtsDevices/cyd-2432s032c/cyd,2432s032c.dtsDevices/cyd-3248s035c/cyd,3248s035c.dtsDevices/cyd-4848s040c/cyd,4848s040c.dtsDevices/cyd-8048s043c/cyd,8048s043c.dtsDevices/cyd-e32r28t/cyd,e32r28t.dtsDevices/cyd-e32r32p/cyd,e32r32p.dtsDevices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dtsDevices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dtsDevices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dtsDevices/elecrow-crowpanel-basic-28/elecrow,crowpanel-basic-28.dtsDevices/elecrow-crowpanel-basic-35/elecrow,crowpanel-basic-35.dtsDevices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dtsDevices/es3c28p/es3c28p.dtsDevices/generic-esp32/generic,esp32.dtsDevices/generic-esp32c6/generic,esp32c6.dtsDevices/generic-esp32s3/generic,esp32s3.dtsDevices/guition-jc2432w328c/guition,jc2432w328c.dtsDevices/guition-jc8048w550c/guition,jc8048w550c.dtsDevices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dtsDevices/lilygo-tdeck-max/lilygo,tdeck-max.dtsDevices/lilygo-tdeck-plus/lilygo,tdeck-plus.dtsDevices/lilygo-tdeck-pro/lilygo,tdeck-pro.dtsDevices/lilygo-tdeck/lilygo,tdeck.dtsDevices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dtsDevices/lilygo-tdisplay/lilygo,tdisplay.dtsDevices/lilygo-tdongle-s3/lilygo,tdongle-s3.dtsDevices/lilygo-thmi/lilygo,thmi.dtsDevices/lilygo-tlora-pager/lilygo,tlora-pager.dtsDevices/m5stack-cardputer-adv/m5stack,cardputer-adv.dtsDevices/m5stack-cardputer/m5stack,cardputer.dtsDevices/m5stack-core2/m5stack,core2.dtsDevices/m5stack-cores3/m5stack,cores3.dtsDevices/m5stack-papers3/m5stack,papers3.dtsDevices/m5stack-stackchan/m5stack,stackchan.dtsDevices/m5stack-stickc-plus2/m5stack,stickc-plus2.dtsDevices/m5stack-sticks3/m5stack,sticks3.dtsDevices/tulip-4r11/tulip-4r11.dtsDevices/unphone/unphone.dtsDevices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dtsDevices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dtsDevices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dtsDevices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dtsDevices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dtsDevices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dtsTactility/Source/service/wifi/Wifi.cpp
💤 Files with no reviewable changes (49)
- Devices/cyd-2432s032c/cyd,2432s032c.dts
- Devices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dts
- Devices/generic-esp32s3/generic,esp32s3.dts
- Devices/cyd-2432s028r/cyd,2432s028r.dts
- Devices/cyd-8048s043c/cyd,8048s043c.dts
- Devices/lilygo-tdeck-max/lilygo,tdeck-max.dts
- Devices/cyd-3248s035c/cyd,3248s035c.dts
- Devices/cyd-2432s024c/cyd,2432s024c.dts
- Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts
- Devices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dts
- Devices/m5stack-cardputer-adv/m5stack,cardputer-adv.dts
- Devices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dts
- Devices/m5stack-sticks3/m5stack,sticks3.dts
- Devices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dts
- Devices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dts
- Devices/guition-jc8048w550c/guition,jc8048w550c.dts
- Devices/lilygo-tdeck/lilygo,tdeck.dts
- Devices/generic-esp32/generic,esp32.dts
- Devices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dts
- Devices/btt-panda-touch/bigtreetech,panda-touch.dts
- Devices/lilygo-thmi/lilygo,thmi.dts
- Devices/guition-jc2432w328c/guition,jc2432w328c.dts
- Devices/cyd-2432s028rv3/cyd,2432s028rv3.dts
- Devices/lilygo-tdongle-s3/lilygo,tdongle-s3.dts
- Devices/elecrow-crowpanel-basic-28/elecrow,crowpanel-basic-28.dts
- Devices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dts
- Devices/m5stack-papers3/m5stack,papers3.dts
- Devices/m5stack-stickc-plus2/m5stack,stickc-plus2.dts
- Devices/m5stack-cardputer/m5stack,cardputer.dts
- Devices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dts
- Devices/m5stack-core2/m5stack,core2.dts
- Devices/cyd-e32r32p/cyd,e32r32p.dts
- Devices/generic-esp32c6/generic,esp32c6.dts
- Devices/lilygo-tdisplay/lilygo,tdisplay.dts
- Devices/tulip-4r11/tulip-4r11.dts
- Devices/m5stack-cores3/m5stack,cores3.dts
- Devices/unphone/unphone.dts
- Devices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dts
- Devices/lilygo-tdeck-plus/lilygo,tdeck-plus.dts
- Devices/lilygo-tdeck-pro/lilygo,tdeck-pro.dts
- Devices/lilygo-tlora-pager/lilygo,tlora-pager.dts
- Devices/elecrow-crowpanel-basic-35/elecrow,crowpanel-basic-35.dts
- Devices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dts
- Devices/m5stack-stackchan/m5stack,stackchan.dts
- Devices/cyd-e32r28t/cyd,e32r28t.dts
- Devices/es3c28p/es3c28p.dts
- Devices/cyd-4848s040c/cyd,4848s040c.dts
- Devices/cyd-2432s024r/cyd,2432s024r.dts
- Devices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dts
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Modules/app-module/source/event.cpp (1)
25-48: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject every existing subscription before linking it.
Line 35 only rejects a duplicate when
subis the list head. Ifsubis already later in the list, this code creates a cycle.app_event_emit()then loops forever while holdingsubscriptions_mutex.Traverse the list for
subbefore assigningsub->internal.next. Release the newly claimed bit before returningERROR_INVALID_STATE.Proposed fix
- if (subscriptions == sub) { + for (auto* existing = subscriptions; existing != nullptr; existing = existing->internal.next) { + if (existing != sub) { + continue; + } mutex_unlock(&subscriptions_mutex.handle); task_event_group_release_bit(event_group, bit); return ERROR_INVALID_STATE; }Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
318-319: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSynchronize timer callbacks before
window_manager_remove.Both applications stop a timer and immediately destroy the window. An in-flight callback can still access the app context and LVGL widgets. Add callback completion synchronization before removing the window.
Tactility/Source/app/gpssettings/GpsSettings.cpp#L318-L319: wait for the GPS state-update callback to finish before removing the window.Tactility/Source/app/power/Power.cpp#L271-L272: wait for the power UI-update callback to finish before removing the window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad18a9c0-6146-48d5-bc7f-851e7ffb701b
📒 Files selected for processing (53)
.claude/rules/app-framework.mdDocumentation/ideas.mdModules/app-module/include/app/event.hModules/app-module/include/app/loader.hModules/app-module/source/app_internal_loader.cppModules/app-module/source/event.cppModules/app-module/source/symbols.cppModules/app-module/tests/source/app_event_test.cppModules/app-module/tests/source/app_manager_test.cppModules/app-module/tests/source/combined_event_test.cppTactility/Source/app/addgps/AddGps.cppTactility/Source/app/alertdialog/AlertDialog.cppTactility/Source/app/appdetails/AppDetails.cppTactility/Source/app/apphub/AppHubApp.cppTactility/Source/app/apphubdetails/AppHubDetailsApp.cppTactility/Source/app/applist/AppList.cppTactility/Source/app/appsettings/AppSettings.cppTactility/Source/app/apwebserver/ApWebServer.cppTactility/Source/app/audiosettings/AudioSettings.cppTactility/Source/app/boot/Boot.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/btpeersettings/BtPeerSettings.cppTactility/Source/app/chat/ChatApp.cppTactility/Source/app/crashdiagnostics/CrashDiagnostics.cppTactility/Source/app/development/Development.cppTactility/Source/app/display/Display.cppTactility/Source/app/files/FilesApp.cppTactility/Source/app/fileselection/FileSelection.cppTactility/Source/app/gpssettings/GpsSettings.cppTactility/Source/app/grovesettings/GroveSettings.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/imageviewer/ImageViewer.cppTactility/Source/app/inputdialog/InputDialog.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/notes/Notes.cppTactility/Source/app/power/Power.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/selectiondialog/SelectionDialog.cppTactility/Source/app/settings/Settings.cppTactility/Source/app/setup/Setup.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timedatesettings/TimeDateSettings.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/app/trackball/TrackballSettings.cppTactility/Source/app/usbsettings/UsbSettings.cppTactility/Source/app/webserversettings/WebServerSettings.cppTactility/Source/app/wifiapsettings/WifiApSettings.cppTactility/Source/app/wificonnect/WifiConnect.cppTactility/Source/app/wifimanage/WifiManage.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Documentation/ideas.md
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes