Refactor app loading and window management - #609
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:
📝 WalkthroughWalkthroughThis change adds application installation, metadata parsing, memory and ELF loading, task scheduling, lifecycle events, and manager APIs. It adds an LVGL window manager with stacked windows and state waiting. System events now support renamed callback APIs and caller-owned polling subscriptions. Build and SDK configuration now include the new modules and packaging paths. Tests cover application management, app events, and system-event behavior. ESP32 device settings disable selected PSRAM execution and data placement. 🚥 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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
Modules/lvgl-window-manager/source/window_manager.cpp-79-84 (1)
79-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor the
window_manager_configure()lifecycle contract.The header states that this call has no effect after startup. This implementation replaces
s.screen_initwhile started. The replacement becomes observable after the next stop/start cycle.Return without changing
s.screen_initwhen the manager is started. Coordinate this with a starting lifecycle state so a concurrent start cannot bypass the pre-start requirement.TactilityKernel/include/tactility/system_event.h-139-140 (1)
139-140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPermit callers to read event output fields.
The warning prohibits reading
timestamp,data, anddata_len. These fields contain the result ofsystem_event_await(). The tests also readdataanddata_len.Document
task,sequence,consumed_sequence, andnextas internal. Permit reads of the event output fields after a successful await.Modules/app-module/source/app_scheduler.cpp-119-164 (1)
119-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
tskNO_AFFINITYfor the app thread affinity.
thread_set_affinity(thread, affinity)stores the raw affinity value, so this passes-1toxTaskCreatePinnedToCoreon ESP-IDF instead of the port’s documented no-affinity value. Keep the-1as the default/compatibility comment if needed, but passtskNO_AFFINITYwhen creating the task.Modules/app-module/source/event.cpp-96-114 (1)
96-114: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRoute app event notifications to a reserved notification index.
Current usages include polling subscriptions, lvTask, libTask waiting tasks, and the USB host task. They call the same non-indexed notification APIs or
ulTaskNotifyTake(pdTRUE, 0)and would have their notification credit cleared byapp_event_awaiton a shared index 0 slot. UsexTaskNotifyGiveIndexed/ulTaskNotifyTakeIndexedorulTaskNotifyTakeIndexed(pdFALSE, 0)with an app-only index.Modules/app-module/source/app_install.cpp-237-238 (1)
237-238: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe lock comment states the inverse of the real contract.
uninstall_locked()never takesinstall_registry().mutex. Both callers take it first:app_install()at line 329 andapp_uninstall()at line 379. The_lockedsuffix also implies the caller holds the lock.A future caller that follows this comment either deadlocks or mutates the registry unlocked.
📝 Proposed fix
-// Takes install_registry().mutex - caller must not already hold it. +// Caller must already hold install_registry().mutex. error_t uninstall_locked(const std::string& app_id) {Modules/app-module/include/app/manager.h-36-42 (1)
36-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the malformed sentence and widen the documented constraint.
Lines 38-39 read "Safe to call app_manager_add()/_remove() from within
@avisitor is NOT guaranteed", which is not a grammatical sentence.The constraint is also narrower than the implementation allows.
app_manager_for_each_manifest()inmanager.cpprunsvisitorwhile it holds the ledger mutex. Anyapp_manager_*call from the visitor can deadlock, not onlyapp_manager_add()/app_manager_remove().📝 Proposed doc fix
- * Calls `@a` visitor once for every registered manifest (e.g. for AppList/Settings to enumerate - * apps to show). Iteration order is unspecified. Safe to call app_manager_add()/_remove() from - * within `@a` visitor is NOT guaranteed - do not mutate the registry from inside the callback. + * Calls `@a` visitor once for every registered manifest (e.g. for AppList/Settings to enumerate + * apps to show). Iteration order is unspecified. + * `@warning` `@a` visitor runs with app-module's internal registry lock held. Do not call any + * app_manager_*() function from inside `@a` visitor - copy out what you need and act on it after + * this call returns.Modules/app-module/source/manager.cpp-124-126 (1)
124-126: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a kernel-provided tick conversion for the stop timeout.
app_scheduler_stop()expectsTickType_t;TactilityKernel/include/tactility/freertos/task.hdefines FreeRTOS task primitives but does not definepdMS_TO_TICKS. If the FreeRTOS include chain changes, this call can fail. Define the 2000 ms value in milliseconds and convert it with FreeRTOS’s tick conversion helper or the local constant used by the FreeRTOS wrapper.Modules/app-module/include/app/module.h-4-8 (1)
4-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the Module definition before exposing
app_module.
TactilityKernel/include/tactility/module.hdefinesstruct Module.Modules/app-module/include/app/module.h,Modules/app-esp32-module/include/app_esp32/module.h,Modules/gps-module/include/gps/module.h, andModules/lvgl-module/include/lvgl/module.hdeclareextern struct Module foo_module;without including it, whileModules/crypt-module/include/crypt/module.hincludestactility/module.hbefore its declaration. IncludeTactilityKernel/include/tactility/module.hin the headers that need the fullModuletype, or change this forward declaration into the same style used by the other leaf module headers if the incomplete type is intentional.
🧹 Nitpick comments (7)
Modules/app-module/private/app/private/app_scheduler.h (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
<stdint.h>in a header that declares C linkage.The file guards declarations with
extern "C", which signals C consumers.<cstdint>compiles only in C++. Use<stdint.h>to keep the header usable from C, and to matchinclude/app/event.h.♻️ Proposed change
-#include <cstdint> +#include <stdint.h>Modules/app-module/source/app_internal_loader.cpp (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
externonServiceManifestdefinitions in both loader services. Anexterndeclaration with an initializer is a definition, so the keyword has no effect and compilers can warn about it. The same pattern was copied into both modules. Declare each manifest in a header and define it withoutextern.
Modules/app-module/source/app_internal_loader.cpp#L42-L48: removeexternfrom theapp_internal_loader_service_manifestdefinition.Modules/app-esp32-module/source/app_esp32_loader_service.cpp#L135-L141: removeexternfrom theloader_service_manifestdefinition.Modules/app-module/include/app/location.h (1)
1-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the SPDX header and document the enum values.
Two small gaps in this new header:
- Line 1 has no
// SPDX-License-Identifier: Apache-2.0. Every other new header in this module has it.- Line 14 points readers to
AppLocationType, but that enum documents nothing. State whatlocationholds for each type.📝 Proposed fix
+// SPDX-License-Identifier: Apache-2.0 `#pragma` once `#ifdef` __cplusplus extern "C" { `#endif` enum AppLocationType { + /** `location` points at an in-memory app image. */ APP_LOCATION_MEMORY, + /** `location` is a NULL-terminated path to the app's install directory. */ APP_LOCATION_PATH, };Modules/app-module/include/app/metadata.h (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the SPDX license identifier.
Every other new file in this change set starts with
// SPDX-License-Identifier: Apache-2.0. This header omits it.📄 Proposed fix
+// SPDX-License-Identifier: Apache-2.0 `#pragma` onceModules/app-module/private/app/private/app_metadata_parsing_internal.h (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
<cstddef>and qualifystd::size_t.
size_tappears unqualified in the global namespace.<map>and<string>only guaranteestd::size_t. Mainstream implementations also declare::size_t, so this compiles today, but the include is not guaranteed.♻️ Proposed change
+#include <cstddef> `#include` <map> `#include` <string>-bool app_metadata_copy_bounded(char* dest, size_t dest_size, const std::string& value); +bool app_metadata_copy_bounded(char* dest, std::size_t dest_size, const std::string& value);Modules/app-module/source/app_metadata_parsing_v2.cpp (1)
10-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared parse routine driven by a key table.
app_metadata_parse_v2andapp_metadata_parse_v1differ only in the six property key strings. Every future metadata field needs the same edit in both files. A single routine that accepts a struct of key names removes the duplication.The internal header documents that the per-format split mirrors the previous
tt::appparser, so this is optional.Modules/app-esp32-module/source/module.cpp (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefix the exported manifest symbol with the module name.
loader_service_manifestis a global symbol with C linkage. Other modules in the image can export the same generic name, which causes a duplicate-symbol link error.Modules/app-module/source/module.cppuses the prefixed nameapp_internal_loader_service_manifest. Rename this symbol and its definition toapp_esp32_loader_service_manifest.♻️ Proposed change
-extern ServiceManifest loader_service_manifest; +extern ServiceManifest app_esp32_loader_service_manifest; static error_t start() { - return service_manager_add(&loader_service_manifest, /*auto_start=*/true); + return service_manager_add(&app_esp32_loader_service_manifest, /*auto_start=*/true); } static error_t stop() { - return service_manager_remove(loader_service_manifest.id); + return service_manager_remove(app_esp32_loader_service_manifest.id); }Rename the definition in
Modules/app-esp32-module/source/app_esp32_loader_service.cppto match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d42aa53d-a21f-4126-9334-befd7c40d4df
📒 Files selected for processing (42)
Devices/lilygo-tlora-pager/source/module.cppDocumentation/ideas.mdModules/app-esp32-module/CMakeLists.txtModules/app-esp32-module/devicetree.yamlModules/app-esp32-module/include/app_esp32/module.hModules/app-esp32-module/source/app_esp32_loader_service.cppModules/app-esp32-module/source/module.cppModules/app-module/CMakeLists.txtModules/app-module/devicetree.yamlModules/app-module/include/app/event.hModules/app-module/include/app/install.hModules/app-module/include/app/instance.hModules/app-module/include/app/loader.hModules/app-module/include/app/location.hModules/app-module/include/app/manager.hModules/app-module/include/app/manifest.hModules/app-module/include/app/metadata.hModules/app-module/include/app/module.hModules/app-module/private/app/private/app_ledger.hModules/app-module/private/app/private/app_metadata_parsing_internal.hModules/app-module/private/app/private/app_scheduler.hModules/app-module/source/app_install.cppModules/app-module/source/app_internal_loader.cppModules/app-module/source/app_metadata_parsing.cppModules/app-module/source/app_metadata_parsing_v1.cppModules/app-module/source/app_metadata_parsing_v2.cppModules/app-module/source/app_scheduler.cppModules/app-module/source/event.cppModules/app-module/source/manager.cppModules/app-module/source/module.cppModules/lvgl-window-manager/CMakeLists.txtModules/lvgl-window-manager/devicetree.yamlModules/lvgl-window-manager/include/lvgl_window_manager/module.hModules/lvgl-window-manager/include/lvgl_window_manager/window_manager.hModules/lvgl-window-manager/source/module.cppModules/lvgl-window-manager/source/window_manager.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/service/rtctime/RtcTimeService.cppTactility/Source/service/wifi/Wifi.cppTactilityKernel/include/tactility/system_event.hTactilityKernel/source/system_event.cppTests/TactilityKernel/Source/SystemEventTest.cpp
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
TactilityKernel/include/tactility/system_event.h (2)
183-189: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReturn a synchronized event snapshot from
system_event_await().
system_event_await()only returnsERROR_NONE/ERROR_TIMEOUT; callers cannot readsub->timestamp,sub->data, orsub->data_lenthrough the documented API because those fields are marked internal. The subscription is also latest-value based, so multiple emits before a secondawait()can overwrite the buffer. Add a supported read/snapshot API and document whether poll events are queued or coalesced.
175-181: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize unsubscription with waiters and re-registration.
system_event_unsubscribe()unlockspoll_subscriptions_mutexbefore deletingsub->semaphore. A task can still be blocked insystem_event_await()onsub->semaphore, and FreeRTOS requires callers not delete a semaphore while tasks are blocked on it. A concurrent re-registration can also replacesub->semaphorebefore the unsubscribe deletes. Keep the protection held until no one can wait onsub->semaphore, or add cancellation and reference tracking before deletion. Add regression coverage for removal during a blocked await and reuse of the same subscription node.Modules/lvgl-window-manager-module/source/window_manager.cpp (1)
61-85: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
window_manager_stop()can still delete widgets that a concurrent create or remove is using.The
lifecycle_mutexadded in this revision serializeswindow_manager_start()againstwindow_manager_stop(). It does not coverwindow_manager_create()orwindow_manager_remove(). Those functions capturecontent_root_widgetand widget pointers unders.mutex, releases.mutex, and then pass the captured pointers tobuild_window_widget()anddelete_widget().
window_manager_stop()deletesreal_root_widgetafter it releasess.mutex. Deletion cascades tocontent_root_widgetandtop_widget. If stop acquires the LVGL lock first, the followinglv_obj_create(content)at line 66 orlv_obj_delete(widget)at line 83 operates on a freedlv_obj_t.Take
lifecycle_mutexinwindow_manager_create()andwindow_manager_remove()as well, or re-validatestartedand the captured pointers unders.mutexwhile holding the LVGL lock before each LVGL call.Run the following script to confirm that no other synchronization guards this path:
#!/bin/bash # Description: Check which window-manager functions take lifecycle_mutex, and inspect the lvgl lock implementation. rg -n -C3 'lifecycle_mutex' Modules/lvgl-window-manager-module/source/window_manager.cpp echo "=== lvgl_lock / lvgl_unlock definitions ===" rg -n -C8 '\b(lvgl_lock|lvgl_unlock)\s*\(' Modules/lvgl-module
🧹 Nitpick comments (3)
Modules/app-module/source/manager.cpp (1)
262-314: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftDo not hold
registry.mutexacross filesystem I/O and cross-registry calls.The scan holds
registry.mutexfrom Line 262 to Line 314. Inside that critical section it performs three kinds of blocking or nested work:
- Line 266 and Line 305 call
app_fs_is_file/app_fs_is_directory, whichstateach path and take aFileMutex.- Line 271 calls
app_metadata_parse, which opens and reads the manifest file and also takes aFileMutex.- Line 292 and Line 310 call
app_manager_add/app_manager_remove, which take the ledger mutex.Two effects follow. First, a scan over a slow medium such as an SD card blocks
app_manager_install_path_addfor the whole duration. Second, the code establishes aregistry.mutex→FileMutexandregistry.mutex→ ledger-mutex lock order. Any future path that takes those locks in the opposite order deadlocks.The function already demonstrates the safer pattern at Lines 253-255. Apply it to the rest: do the
statcalls and the manifest parsing outside the lock, then takeregistry.mutexonly to publish the resulting records.Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h (1)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
app_instance_idor remove it.
window_manager_createstores the caller-suppliedapp_instance_idbut the header does not document it and it is never read elsewhere. Add a@param[in] app_instance_idline, or remove the parameter if the value has no consumer.Modules/lvgl-window-manager-module/source/window_manager.cpp (1)
304-319: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a dedicated notification index for window manager waiters.
ulTaskNotifyTake()andxTaskNotifyGive()use notification index 0 by default. If a window-manager waiter also uses task index 0 for its own events, the wait drains those unrelated notifications. Use a dedicated index, such as index 1 (FreeRTOS config already setsconfigTASK_NOTIFICATION_ARRAY_ENTRIESto 2), and update the waiters and notification senders to use matching indexed APIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07e2b5b3-3c60-4ebb-865f-a8f10170a4cb
📒 Files selected for processing (35)
Buildscripts/TactilitySDK/CMakeLists.txtBuildscripts/TactilitySDK/TactilitySDK.cmakeBuildscripts/release-sdk.pyCMakeLists.txtDevices/lilygo-tdeck-plus/device.propertiesDocumentation/ideas.mdModules/app-esp32-module/source/app_esp32_loader_service.cppModules/app-module/include/app/instance.hModules/app-module/include/app/manager.hModules/app-module/include/app/metadata.hModules/app-module/include/app/scheduler.hModules/app-module/private/app/private/app_fs.hModules/app-module/private/app/private/app_ledger.hModules/app-module/private/app/private/app_scheduler.hModules/app-module/source/app_install.cppModules/app-module/source/app_metadata_parsing.cppModules/app-module/source/app_metadata_parsing_v1.cppModules/app-module/source/app_metadata_parsing_v2.cppModules/app-module/source/app_scheduler.cppModules/app-module/source/manager.cppModules/app-module/source/symbols.cppModules/lvgl-window-manager-module/CMakeLists.txtModules/lvgl-window-manager-module/devicetree.yamlModules/lvgl-window-manager-module/include/lvgl_window_manager/module.hModules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.hModules/lvgl-window-manager-module/source/symbols.cppModules/lvgl-window-manager-module/source/window_manager.cppTactilityKernel/include/tactility/system_event.hTactilityKernel/source/system_event.cppTests/CMakeLists.txtTests/Tactility/CMakeLists.txtTests/app-module/CMakeLists.txtTests/app-module/Source/AppEventTest.cppTests/app-module/Source/AppManagerTest.cppTests/app-module/Source/Main.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- Modules/app-module/private/app/private/app_ledger.h
- Modules/app-module/source/app_metadata_parsing_v1.cpp
- Modules/app-module/source/app_metadata_parsing_v2.cpp
- Modules/app-module/private/app/private/app_scheduler.h
- Modules/app-module/include/app/metadata.h
- Modules/app-esp32-module/source/app_esp32_loader_service.cpp
- TactilityKernel/source/system_event.cpp
- Modules/app-module/source/app_install.cpp
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
Modules/app-esp32-module/LICENSE-Apache-2.0.md (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExclude the Apache license files from Markdown lint.
The three added license files trigger the same MD001 and MD003 warnings. Preserve the legal boilerplate and exclude these files from the Markdown rules.
Modules/app-esp32-module/LICENSE-Apache-2.0.md#L7-L9: Exclude this license file from Markdown lint.Modules/app-module/LICENSE-Apache-2.0.md#L7-L9: Exclude this license file from Markdown lint.Modules/lvgl-window-manager-module/LICENSE-Apache-2.0.md#L7-L9: Exclude this license file from Markdown lint.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34578dc8-8095-4c70-abc3-82ad0e591548
📒 Files selected for processing (48)
Buildscripts/TactilitySDK/CMakeLists.txtDocumentation/ideas.mdModules/app-esp32-module/LICENSE-Apache-2.0.mdModules/app-module/LICENSE-Apache-2.0.mdModules/app-module/include/app/location.hModules/app-module/include/app/manager.hModules/app-module/include/app/metadata.hModules/app-module/include/app/module.hModules/app-module/include/app/paths.hModules/app-module/private/app/private/app_fs.hModules/app-module/private/app/private/app_ledger.hModules/app-module/private/app/private/app_scheduler.hModules/app-module/source/app_install.cppModules/app-module/source/app_internal_loader.cppModules/app-module/source/app_paths.cppModules/app-module/source/app_scheduler.cppModules/app-module/source/symbols.cppModules/lvgl-window-manager-module/LICENSE-Apache-2.0.mdModules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.hModules/lvgl-window-manager-module/source/window_manager.cppTactilityC/Include/tt_app.hTactilityC/Include/tt_app_alertdialog.hTactilityC/Include/tt_app_fileselection.hTactilityC/Include/tt_app_selectiondialog.hTactilityC/Include/tt_bundle.hTactilityC/Include/tt_preferences.hTactilityC/Source/tt_app.cppTactilityC/Source/tt_app_alertdialog.cppTactilityC/Source/tt_app_fileselection.cppTactilityC/Source/tt_app_selectiondialog.cppTactilityC/Source/tt_bundle.cppTactilityC/Source/tt_init.cppTactilityC/Source/tt_preferences.cppTactilityKernel/include/tactility/bundle.hTactilityKernel/include/tactility/preferences.hTactilityKernel/include/tactility/properties_file.hTactilityKernel/include/tactility/system_event.hTactilityKernel/source/bundle.cppTactilityKernel/source/preferences.cppTactilityKernel/source/properties_file.cppTactilityKernel/source/symbols.cTactilityKernel/source/system_event.cppTests/TactilityKernel/Source/BundleTest.cppTests/TactilityKernel/Source/PreferencesTest.cppTests/TactilityKernel/Source/PropertiesFileTest.cppTests/TactilityKernel/Source/SystemEventTest.cppTests/app-module/Source/AppManagerTest.cppTests/app-module/Source/Main.cpp
💤 Files with no reviewable changes (8)
- TactilityC/Include/tt_preferences.h
- TactilityC/Source/tt_preferences.cpp
- TactilityC/Include/tt_bundle.h
- TactilityC/Source/tt_bundle.cpp
- TactilityC/Include/tt_app.h
- Modules/app-module/private/app/private/app_scheduler.h
- TactilityC/Source/tt_init.cpp
- TactilityC/Source/tt_app.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- Modules/app-module/include/app/module.h
- Modules/app-module/source/app_internal_loader.cpp
- Modules/app-module/private/app/private/app_ledger.h
- Modules/app-module/source/symbols.cpp
- Modules/app-module/private/app/private/app_fs.h
- Buildscripts/TactilitySDK/CMakeLists.txt
- Tests/app-module/Source/AppManagerTest.cpp
- Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h
- Modules/app-module/include/app/metadata.h
- Modules/app-module/include/app/location.h
- Modules/lvgl-window-manager-module/source/window_manager.cpp
- Modules/app-module/include/app/manager.h
- Modules/app-module/source/app_install.cpp
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
Tests/TactilityKernel/Source/PreferencesTest.cpp (1)
233-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the reopened handle like the first one.
Line 224 uses
REQUIRE_NE(preferences, nullptr). Line 233 omits the same check forreopened. If the reopen fails, Line 235 passesnullptrintopreferences_opt_int32, which dereferences it and crashes the test binary instead of reporting a failed assertion.♻️ Proposed change
Preferences* reopened = preferences_open(nested_path); + REQUIRE_NE(reopened, nullptr); int32_t out = 0; CHECK(preferences_opt_int32(reopened, "count", &out));Tests/TactilityKernel/Source/SystemEventTest.cpp (1)
402-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo
thread_joincalls use a 2-tick timeout. Both new tests join a worker thread with a 2-tick timeout and a 1-tick poll interval. That margin does not cover the worker's return path and thread-completion bookkeeping on a loaded CI runner, so either join can fail for a scheduling reason rather than a code defect.
Tests/TactilityKernel/Source/SystemEventTest.cpp#L402-L404: raise the join timeout for the awaiter thread; Line 400 already bounds the timing the test cares about.Tests/TactilityKernel/Source/SystemEventTest.cpp#L435-L436: raise the join timeout for the emitter thread; the await at Line 434 already bounds the timing.Use
pdMS_TO_TICKS(2000)for the timeout andpdMS_TO_TICKS(1)for the poll interval at both sites.Modules/lvgl-window-manager-module/source/window_manager.cpp (2)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the module include path for
app/instance.h.
lvgl-window-manager-moduleincludesapp-modulethroughREQUIRES, andapp-moduleexportsinclude/. Addapp-moduletoREQUIRESand change bothapp/instance.hincludes inModules/lvgl-window-manager-moduleto#include <app/instance.h>instead of relative../../..//../../app-module/include/...paths.
273-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the non-reentrancy limit for window-manager callbacks.
lifecycle_mutexis held duringscreen_init(),create_widgets(), and the rebuild path inwindow_manager_remove(). BecauseMutexis not recursive, calling any window-manager API from these callbacks can deadlock. Add this restriction toWindowManagerScreenInitFnandWindowCreateWidgetsFninwindow_manager.h.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bb353d5-9b14-478c-b6d1-af16d83ad636
📒 Files selected for processing (19)
Firmware/CMakeLists.txtModules/app-esp32-module/source/app_esp32_loader_service.cppModules/app-module/private/app/private/app_ledger.hModules/app-module/source/app_scheduler.cppModules/app-module/source/manager.cppModules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.hModules/lvgl-window-manager-module/source/window_manager.cppTactility/CMakeLists.txtTactilityC/Include/tt_app_fileselection.hTactilityKernel/include/tactility/preferences.hTactilityKernel/include/tactility/properties_file.hTactilityKernel/include/tactility/system_event.hTactilityKernel/source/bundle.cppTactilityKernel/source/preferences.cppTactilityKernel/source/properties_file.cppTactilityKernel/source/system_event.cppTests/TactilityKernel/Source/PreferencesTest.cppTests/TactilityKernel/Source/PropertiesFileTest.cppTests/TactilityKernel/Source/SystemEventTest.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- TactilityKernel/source/bundle.cpp
- Modules/app-esp32-module/source/app_esp32_loader_service.cpp
- TactilityKernel/include/tactility/preferences.h
- TactilityC/Include/tt_app_fileselection.h
- TactilityKernel/source/preferences.cpp
- Modules/app-module/source/manager.cpp
- TactilityKernel/source/system_event.cpp
- Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TactilityKernel/source/system_event.cpp (1)
238-275: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWake every blocked awaiter during removal.
xSemaphoreGive()unblocks only one task waiting on a binary semaphore. If two tasks callsystem_event_await()on the same subscription,system_event_unsubscribe()wakes one and then waits forwaiter_countto reach zero indefinitely while the other task remains blocked.Signal each blocked awaiter, or reject concurrent
system_event_await()calls on one subscription and document that restriction.
♻️ Duplicate comments (1)
TactilityKernel/source/bundle.cpp (1)
47-47: 🩺 Stability & Availability | 🟠 MajorRestore allocation-failure handling for
bundle_clone.Line 47 copies an
std::unordered_mapwhose nodes, keys, and string values can allocate.new (std::nothrow)only protects the initialBundleallocation. If this copy fails,std::bad_allocescapes instead of returningnullptr.Restore a fallible clone path. When exceptions are enabled, catch the allocation failure, free
clone, and returnnullptr. When exceptions are disabled, use a non-throwing storage and copy design.Possible fix when C++ exceptions are enabled
- clone->entries = bundle->entries; + try { + clone->entries = bundle->entries; + } catch (const std::bad_alloc&) { + delete clone; + return nullptr; + }Run this verification script:
#!/usr/bin/env bash set -euo pipefail echo "== Exception configuration ==" rg -n -C 3 \ 'fno-exceptions|fexceptions|CMAKE_CXX_FLAGS|CXX_FLAGS|CXX_STANDARD|exceptions' \ . || true echo "== Bundle definition and clone callers ==" rg -n -C 6 \ '\bbundle_clone\s*\(|\bstruct Bundle\b|std::unordered_map|entries' \ TactilityKernel Tests Modules || true
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 280abc30-8250-42d6-a7ad-ad8bd0e0772c
📒 Files selected for processing (13)
Modules/app-module/source/app_scheduler.cppModules/lvgl-window-manager-module/CMakeLists.txtModules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.hModules/lvgl-window-manager-module/source/window_manager.cppTactility/CMakeLists.txtTactilityKernel/include/tactility/properties_file.hTactilityKernel/include/tactility/system_event.hTactilityKernel/source/bundle.cppTactilityKernel/source/properties_file.cppTactilityKernel/source/system_event.cppTests/TactilityKernel/Source/PreferencesTest.cppTests/TactilityKernel/Source/PropertiesFileTest.cppTests/TactilityKernel/Source/SystemEventTest.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- Modules/lvgl-window-manager-module/CMakeLists.txt
- Tests/TactilityKernel/Source/PreferencesTest.cpp
- TactilityKernel/include/tactility/properties_file.h
- Modules/app-module/source/app_scheduler.cpp
- TactilityKernel/source/properties_file.cpp
- Tests/TactilityKernel/Source/PropertiesFileTest.cpp
- Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h
- Tests/TactilityKernel/Source/SystemEventTest.cpp
- Tactility/CMakeLists.txt
- Modules/lvgl-window-manager-module/source/window_manager.cpp
1 participant
Summary by CodeRabbit