From ba59437f44f686e70dec4058bc6fe4280dc63c5d Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sat, 8 Aug 2026 18:28:52 +0200 Subject: [PATCH] Fixes --- Firmware/CMakeLists.txt | 2 + .../source/app_esp32_loader_service.cpp | 2 +- .../private/app/private/app_ledger.h | 28 ++- Modules/app-module/source/app_scheduler.cpp | 115 +++++++---- Modules/app-module/source/manager.cpp | 48 +++-- .../lvgl_window_manager/window_manager.h | 6 +- .../source/window_manager.cpp | 181 +++++++++++++----- Tactility/CMakeLists.txt | 3 + TactilityC/Include/tt_app_fileselection.h | 4 +- .../include/tactility/preferences.h | 9 +- .../include/tactility/properties_file.h | 17 +- .../include/tactility/system_event.h | 25 ++- TactilityKernel/source/bundle.cpp | 13 +- TactilityKernel/source/preferences.cpp | 88 ++++++++- TactilityKernel/source/properties_file.cpp | 61 +++++- TactilityKernel/source/system_event.cpp | 118 ++++++++++-- .../Source/PreferencesTest.cpp | 90 +++++++++ .../Source/PropertiesFileTest.cpp | 72 +++++++ .../Source/SystemEventTest.cpp | 96 ++++++++++ 19 files changed, 822 insertions(+), 156 deletions(-) diff --git a/Firmware/CMakeLists.txt b/Firmware/CMakeLists.txt index 5efbbefe6..ea82c6574 100644 --- a/Firmware/CMakeLists.txt +++ b/Firmware/CMakeLists.txt @@ -96,6 +96,8 @@ else () Tactility TactilityFreeRtos lvgl-module + lvgl-window-manager-module + app-module crypt-module gps-module gps-generic-module diff --git a/Modules/app-esp32-module/source/app_esp32_loader_service.cpp b/Modules/app-esp32-module/source/app_esp32_loader_service.cpp index ee510d818..ab8009828 100644 --- a/Modules/app-esp32-module/source/app_esp32_loader_service.cpp +++ b/Modules/app-esp32-module/source/app_esp32_loader_service.cpp @@ -151,7 +151,7 @@ void destroy_service(const ServiceManifest*, void*) { } // namespace -extern ServiceManifest loader_service_manifest = { +ServiceManifest loader_service_manifest = { .id = APP_LOADER_PATH_SERVICE_ID, .create_service = create_service, .destroy_service = destroy_service, diff --git a/Modules/app-module/private/app/private/app_ledger.h b/Modules/app-module/private/app/private/app_ledger.h index c7907cb13..61f38d714 100644 --- a/Modules/app-module/private/app/private/app_ledger.h +++ b/Modules/app-module/private/app/private/app_ledger.h @@ -6,12 +6,32 @@ #include #include +#include #include #include #include #include +/** + * A dedicated (not the task's shared default FreeRTOS notification, which app_event.cpp's + * AppEventSubscription also uses - an unrelated event delivered to the same task could + * otherwise unblock a waiter early) completion signal for one app instance's task, given as the + * literal last action app_task_main() takes before vTaskDelete(). Heap-allocated with its own + * refcount (protected by app_ledger().mutex, not atomic) rather than owned by the ledger + * entry, since app_task_main() always erases that entry - and may run its exit path entirely - + * before app_scheduler_stop() ever looks for it: whichever side (the exiting task, or a + * concurrent app_scheduler_stop() that found the entry in time and is waiting on `semaphore`) + * finishes with it last is the one that deletes `semaphore` and frees this struct. + */ +struct AppCompletionSignal { + SemaphoreHandle_t semaphore; + /** Starts at 1, owned by app_task_main() until its own exit. app_scheduler_stop() takes an + * additional reference for as long as it's waiting on `semaphore`, if it finds the instance + * still running. Reaching 0 means deletion. */ + int refcount = 1; +}; + /** A registered/running app instance, as tracked internally by app-module. */ struct AppInstanceRecord { uint32_t id; @@ -25,11 +45,9 @@ struct AppInstanceRecord { * app_manager_start_for_result() - the instance that receives this child's APP_EVENT_RESULT. */ uint32_t parent_id = 0; - /** The task currently blocked in app_scheduler_stop() for this instance, if any - notified - * (via xTaskNotifyGive()) as the literal last action app_task_main() takes before - * vTaskDelete(), so app_scheduler_stop() can't observe completion before the task has - * actually finished running. See app_scheduler.cpp. */ - TaskHandle_t stop_waiter = nullptr; + /** This instance's completion signal - see AppCompletionSignal. Set once by + * app_scheduler_start(), never reassigned. */ + AppCompletionSignal* completion = nullptr; }; struct AppLedger { diff --git a/Modules/app-module/source/app_scheduler.cpp b/Modules/app-module/source/app_scheduler.cpp index 70ce18eaf..948222e04 100644 --- a/Modules/app-module/source/app_scheduler.cpp +++ b/Modules/app-module/source/app_scheduler.cpp @@ -35,6 +35,7 @@ struct TaskContext { AppInstanceId app_instance_id; int argc; char** argv; + AppCompletionSignal* completion; }; void set_state(AppInstanceId app_instance_id, AppInstanceState state) { @@ -57,21 +58,44 @@ void set_task(AppInstanceId app_instance_id, TaskHandle_t task) { mutex_unlock(&ledger.mutex); } -// Registers the calling task to be notified when app_instance_id's task actually finishes -// running (see app_task_main()'s exit path), and reports whether there's anything to wait for. -// @return true if the instance's ledger entry still exists (a wait was registered); false if -// the task has already fully finished (and already given any notification it would have) - -// there is nothing left to wait for. -bool register_stop_waiter(AppInstanceId app_instance_id) { +void set_completion(AppInstanceId app_instance_id, AppCompletionSignal* completion) { auto& ledger = app_ledger(); mutex_lock(&ledger.mutex); auto iterator = ledger.instances.find(app_instance_id); - bool exists = iterator != ledger.instances.end(); - if (exists) { - iterator->second.stop_waiter = xTaskGetCurrentTaskHandle(); + if (iterator != ledger.instances.end()) { + iterator->second.completion = completion; } mutex_unlock(&ledger.mutex); - return exists; +} + +// Takes a reference on app_instance_id's completion signal (see AppCompletionSignal), for the +// caller to wait on. @return the signal to wait on, or NULL if the instance has already fully +// finished (its ledger entry - and so its reference to the signal - is already gone) and so +// there's nothing left to wait for. +AppCompletionSignal* acquire_completion_signal(AppInstanceId app_instance_id) { + auto& ledger = app_ledger(); + mutex_lock(&ledger.mutex); + auto iterator = ledger.instances.find(app_instance_id); + AppCompletionSignal* completion = nullptr; + if (iterator != ledger.instances.end()) { + completion = iterator->second.completion; + completion->refcount++; + } + mutex_unlock(&ledger.mutex); + return completion; +} + +// Releases a reference taken by acquire_completion_signal(), deleting the signal (and its +// semaphore) if this was the last one. +void release_completion_signal(AppCompletionSignal* completion) { + auto& ledger = app_ledger(); + mutex_lock(&ledger.mutex); + bool should_delete = (--completion->refcount == 0); + mutex_unlock(&ledger.mutex); + if (should_delete) { + vSemaphoreDelete(completion->semaphore); + delete completion; + } } const char* loader_service_id_for(AppLocationType type) { @@ -137,26 +161,28 @@ void app_task_main(void* context) { app_ledger_free_arguments(ctx->argc, ctx->argv); AppInstanceId app_instance_id = ctx->app_instance_id; + AppCompletionSignal* completion = ctx->completion; delete ctx; LOG_I(TAG, "Thread for %d finished", app_instance_id); - // Erase the ledger entry before self-deleting, capturing whoever's blocked in - // app_scheduler_stop() for this instance (if anyone) so they can be notified afterward. + // Erase the ledger entry before self-deleting - see "Reap self-terminated app tasks": + // nothing else is guaranteed to ever call app_scheduler_stop() for this instance (the + // common case is the app just closing itself), so this can't wait for that to happen. auto& ledger = app_ledger(); mutex_lock(&ledger.mutex); - auto iterator = ledger.instances.find(app_instance_id); - TaskHandle_t stop_waiter = (iterator != ledger.instances.end()) ? iterator->second.stop_waiter : nullptr; ledger.instances.erase(app_instance_id); mutex_unlock(&ledger.mutex); // Signal completion as the literal last action before this task ceases to exist, so - // app_scheduler_stop() can't observe "stopped" one step early (see its own comment) - - // unlike watching the ledger entry disappear, this can only happen once the task is truly - // done running. - if (stop_waiter != nullptr) { - xTaskNotifyGive(stop_waiter); - } + // app_scheduler_stop() can't observe "stopped" one step early - unlike watching the ledger + // entry disappear, this can only happen once the task is truly done running. A dedicated + // semaphore rather than this task's default FreeRTOS notification, since app_event.cpp's + // AppEventSubscription also uses that shared slot - an unrelated event (e.g. a child's + // APP_EVENT_RESULT) delivered to this same task could otherwise unblock a concurrent + // app_scheduler_stop() early. + xSemaphoreGive(completion->semaphore); + release_completion_signal(completion); // releases app_task_main()'s own reference vTaskDelete(nullptr); } @@ -181,9 +207,27 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, return load_result; } - auto* context = new (std::nothrow) TaskContext { loader, runtime, app_instance_id, argc, argv }; + auto* completion = new (std::nothrow) AppCompletionSignal(); + if (completion == nullptr) { + LOG_E(TAG, "Failed to allocate app"); + loader->unload(runtime); + app_ledger_free_arguments(argc, argv); + return ERROR_OUT_OF_MEMORY; + } + completion->semaphore = xSemaphoreCreateBinary(); + if (completion->semaphore == nullptr) { + LOG_E(TAG, "Failed to allocate app"); + delete completion; + loader->unload(runtime); + app_ledger_free_arguments(argc, argv); + return ERROR_OUT_OF_MEMORY; + } + + auto* context = new (std::nothrow) TaskContext { loader, runtime, app_instance_id, argc, argv, completion }; if (context == nullptr) { LOG_E(TAG, "Failed to allocate app"); + vSemaphoreDelete(completion->semaphore); + delete completion; loader->unload(runtime); app_ledger_free_arguments(argc, argv); return ERROR_OUT_OF_MEMORY; @@ -200,6 +244,8 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, BaseType_t create_result = xTaskCreate(app_task_main, task_name, 8192 / sizeof(StackType_t), context, tskIDLE_PRIORITY, &task_handle); if (create_result != pdPASS) { delete context; + vSemaphoreDelete(completion->semaphore); + delete completion; loader->unload(runtime); app_ledger_free_arguments(argc, argv); return ERROR_OUT_OF_MEMORY; @@ -207,6 +253,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, vTaskSuspend(task_handle); set_task(app_instance_id, task_handle); + set_completion(app_instance_id, completion); vTaskPrioritySet(task_handle, APP_TASK_PRIORITY); vTaskResume(task_handle); @@ -214,22 +261,22 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, } error_t app_scheduler_stop(AppInstanceId app_instance_id, TickType_t join_timeout) { - // Drain any stale notification credit before registering as the waiter - otherwise a - // leftover give from an unrelated earlier wait on this same task (e.g. a previous - // app_scheduler_stop() call that timed out and only got notified afterward) could make the - // take below return immediately for the wrong event. Mirrors app_event_await()'s same - // defensive drain. - ulTaskNotifyTake(pdTRUE, 0); - - if (register_stop_waiter(app_instance_id)) { + AppCompletionSignal* completion = acquire_completion_signal(app_instance_id); + if (completion != nullptr) { AppEvent event { .type = APP_EVENT_CLOSE, .timestamp = 0, .result = {} }; app_event_emit(app_instance_id, &event); - // Blocks until app_task_main() gives this notification as the literal last thing it - // does before vTaskDelete() - unlike polling the ledger for the task handle to clear, - // this can't observe "stopped" while the task is still mid-exit (still running its own - // cleanup/vTaskDelete()). - if (ulTaskNotifyTake(pdTRUE, join_timeout) == 0) { + // Blocks until app_task_main() gives this dedicated semaphore as the literal last + // thing it does before vTaskDelete() - unlike polling the ledger for the task handle to + // clear, this can't observe "stopped" while the task is still mid-exit (still running + // its own cleanup/vTaskDelete()). A dedicated semaphore rather than this task's default + // FreeRTOS notification, since app_event.cpp's AppEventSubscription also uses that + // shared slot - an unrelated event (e.g. a different child's APP_EVENT_RESULT) + // delivered to this same task could otherwise unblock this early. + BaseType_t taken = xSemaphoreTake(completion->semaphore, join_timeout); + release_completion_signal(completion); + + if (taken == pdFALSE) { LOG_W(TAG, "App instance %u did not stop in time", app_instance_id); return ERROR_TIMEOUT; } diff --git a/Modules/app-module/source/manager.cpp b/Modules/app-module/source/manager.cpp index 3c13821e7..ae91cbb0f 100644 --- a/Modules/app-module/source/manager.cpp +++ b/Modules/app-module/source/manager.cpp @@ -259,8 +259,16 @@ void app_manager_install_path_scan(void) { app_fs_list_direct_subdirectories(root, found_app_dirs); } + // Snapshot of what's already registered, taken once so the rest of this scan can run without holding registry.mutex mutex_lock(®istry.mutex); + std::unordered_map known_paths; // id -> path + for (const auto& [id, record] : registry.scanned) { + known_paths.emplace(id, record->path); + } + mutex_unlock(®istry.mutex); + // Stat each manifest and parse it entirely without registry.mutex held (due to filesystem IO being slow) + std::vector> new_records; for (const auto& app_dir : found_app_dirs) { auto manifest_path = app_dir + "/manifest.properties"; if (!app_fs_is_file(manifest_path)) { @@ -273,7 +281,7 @@ void app_manager_install_path_scan(void) { continue; } - if (registry.scanned.contains(metadata.app_id)) { + if (known_paths.contains(metadata.app_id)) { continue; // already registered by an earlier scan } @@ -288,29 +296,41 @@ void app_manager_install_path_scan(void) { .location = { APP_LOCATION_PATH, const_cast(record->path.c_str()) }, .flags = 0, }; - - if (app_manager_add(&record->manifest) != ERROR_NONE) { - LOG_E(TAG, "Failed to register app %s (duplicate id?)", record->id.c_str()); - continue; - } - - registry.scanned[record->id] = std::move(record); + new_records.push_back(std::move(record)); } - // Anything a previous scan registered whose directory has since disappeared (e.g. an SD - // card was removed) just gets unregistered - no file deletion, no touching running - // instances, that's app_install()/app_uninstall()'s job, not scanning's. + // Anything a previous scan registered whose directory has since disappeared gets unregistered below. std::vector missing_ids; - for (const auto& [id, record] : registry.scanned) { - if (!app_fs_is_directory(record->path)) { + for (const auto& [id, path] : known_paths) { + if (!app_fs_is_directory(path)) { missing_ids.push_back(id); } } + + // app_manager_add()/app_manager_remove() take app-module's own ledger mutex internally - + // calling them while holding registry.mutex would establish a registry.mutex -> ledger- + // mutex lock order that any future opposite-order path would deadlock against, so these + // also run with registry.mutex released. registry.mutex is taken only afterward, briefly, + // to publish the results (plain in-memory map updates, no I/O or other locks involved). for (const auto& id : missing_ids) { app_manager_remove(id.c_str()); - registry.scanned.erase(id); + } + std::vector> added_records; + for (auto& record : new_records) { + if (app_manager_add(&record->manifest) == ERROR_NONE) { + added_records.push_back(std::move(record)); + } else { + LOG_E(TAG, "Failed to register app %s (duplicate id?)", record->id.c_str()); + } } + mutex_lock(®istry.mutex); + for (const auto& id : missing_ids) { + registry.scanned.erase(id); + } + for (auto& record : added_records) { + registry.scanned[record->id] = std::move(record); + } mutex_unlock(®istry.mutex); } diff --git a/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h b/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h index 7a4ba6044..7a5c243e0 100644 --- a/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h +++ b/Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h @@ -1,6 +1,9 @@ // SPDX-License-Identifier: Apache-2.0 #pragma once +#include "../../../app-module/include/app/instance.h" + + #include #include @@ -80,12 +83,13 @@ typedef void (*WindowCreateWidgetsFn)(lv_obj_t* root, void* user_data); * Creates a new window on top of the stack (last created = topmost). Deletes the previously * topmost window's widgets (if any) and builds this window's widgets immediately via * @a create_widgets - only the topmost window ever has live widgets. + * @param[in] app_instance_id the application instance this window belongs to, should not be 0 * @param[in] user_data opaque; passed back to @a create_widgets on every call, including a * later rebuild triggered by window_manager_remove() - see its @warning about which thread that * can run on. Typically the calling app's own Context*. * @return the new window's id, or 0 if window_manager_start() hasn't been called */ -WindowId window_manager_create(uint32_t app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data); +WindowId window_manager_create(AppInstanceId app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data); /** * Removes a window, wherever it is in the stack - not necessarily the topmost one. If it was diff --git a/Modules/lvgl-window-manager-module/source/window_manager.cpp b/Modules/lvgl-window-manager-module/source/window_manager.cpp index 7b1d8df31..f182f2149 100644 --- a/Modules/lvgl-window-manager-module/source/window_manager.cpp +++ b/Modules/lvgl-window-manager-module/source/window_manager.cpp @@ -1,42 +1,66 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include "../../app-module/include/app/instance.h" + #include #include #include +#include #include +#include #include constexpr auto* TAG = "window_manager"; namespace { +/** + * A dedicated (not the waiting task's shared default FreeRTOS notification, which other + * subsystems - e.g. app_event.cpp's AppEventSubscription - also use; an unrelated notification + * delivered to the same task could otherwise unblock a wait early) completion signal for one + * window_manager_await_state_change() call. Heap-allocated with its own refcount (protected by + * WindowManagerState::mutex, not atomic) rather than owned solely by the WindowRecord, since + * window_manager_create()/remove() claim (read + clear) a window's signal under the lock but + * give it after releasing that lock - refcounting lets whichever side (the waiting task waking + * up, or the claimer after it gives) finishes last safely delete it. + */ +struct WindowWaitSignal { + SemaphoreHandle_t semaphore; + /** Starts at 1, owned by window_manager_await_state_change() until it's done waiting. + * Whoever claims this signal from a WindowRecord (see claim_waiter_locked()) takes an + * additional reference for as long as it takes to give the semaphore. Reaching 0 means + * deletion. */ + int refcount = 1; +}; + struct WindowRecord { WindowId id; uint32_t app_instance_id; WindowCreateWidgetsFn create_widgets; void* user_data; - /** Task blocked in window_manager_await_state_change() for this specific window, if any - - * see that function's @warning on at most one concurrent awaiter per window. Per-window - * rather than a single manager-wide slot, since a stacked window manager serving several - * app tasks can have more than one window (though only ever one of them topmost/GRANTED at - * a time) with a live await() call outstanding. */ - TaskHandle_t waiting_task = nullptr; + /** Set by window_manager_await_state_change() for this specific window, if a task is + * currently blocked there - see that function's @warning on at most one concurrent awaiter + * per window. Per-window rather than a single manager-wide slot, since a stacked window + * manager serving several app tasks can have more than one window (though only ever one of + * them topmost/GRANTED at a time) with a live await() call outstanding. */ + WindowWaitSignal* waiting_signal = nullptr; }; struct WindowManagerState { /** Mutex for read/write operations. Shortly held. */ Mutex mutex {}; - /** Serializes the full start()/stop() transition (including the LVGL work done with - * `mutex` released) so two concurrent starts can't both pass the `started` check and each - * create their own root widget, and a concurrent stop can't run while a start is still - * mid-flight. Never held across a create_widgets()/screen_init() callback - those only - * reach window_manager_create()/remove(), not start()/stop() - so there's no lock-order - * risk with `mutex` or the LVGL lock. */ + /** Serializes the full start()/stop()/create()/remove() transitions against each other, + * including the LVGL work done with `mutex` released (and any create_widgets()/ + * screen_init() callback invoked as part of that work). Without this, e.g. + * window_manager_stop() could delete real_root_widget/content_root_widget/top_widget + * between a concurrent create()/remove() capturing one of those pointers under `mutex` and + * actually using it via build_window_widget()/delete_widget() after releasing `mutex` - + * touching an LVGL object it no longer holds a valid reference to. */ Mutex lifecycle_mutex {}; bool started = false; @@ -91,6 +115,38 @@ void delete_widget(lv_obj_t* widget) { lvgl_unlock(); } +// Call while holding WindowManagerState::mutex. Transfers ownership of `window`'s waiting +// signal (if any) to the caller, taking an additional reference on the caller's behalf - the +// caller must eventually pass the result to give_and_release() exactly once, outside the lock. +WindowWaitSignal* claim_waiter_locked(WindowRecord& window) { + WindowWaitSignal* signal = window.waiting_signal; + window.waiting_signal = nullptr; + if (signal != nullptr) { + signal->refcount++; + } + return signal; +} + +// Gives `signal`'s semaphore (waking window_manager_await_state_change() if it's still +// waiting) and releases the caller's reference (see claim_waiter_locked()), deleting the +// signal if that was the last one. No-op if `signal` is NULL. +void give_and_release(WindowWaitSignal* signal) { + if (signal == nullptr) { + return; + } + + xSemaphoreGive(signal->semaphore); + + auto& s = state(); + mutex_lock(&s.mutex); + bool should_delete = (--signal->refcount == 0); + mutex_unlock(&s.mutex); + if (should_delete) { + vSemaphoreDelete(signal->semaphore); + delete signal; + } +} + } // namespace extern "C" { @@ -178,12 +234,12 @@ error_t window_manager_stop(void) { return ERROR_NONE; } lv_obj_t* widget = s.real_root_widget; - // Collect every window's waiter before clearing - normally at most the topmost window's is + // Claim every window's waiter before clearing - normally at most the topmost window's is // ever set, but every window is being torn down here, so every one is checked. - std::vector waiters; - for (const auto& window : s.windows) { - if (window.waiting_task != nullptr) { - waiters.push_back(window.waiting_task); + std::vector waiters; + for (auto& window : s.windows) { + if (auto* signal = claim_waiter_locked(window); signal != nullptr) { + waiters.push_back(signal); } } s.real_root_widget = nullptr; @@ -193,8 +249,8 @@ error_t window_manager_stop(void) { s.started = false; mutex_unlock(&s.mutex); - for (TaskHandle_t waiter : waiters) { - xTaskNotifyGive(waiter); + for (WindowWaitSignal* waiter : waiters) { + give_and_release(waiter); } // Deleting the real widget cascades to everything under it - chrome and top_widget alike. @@ -204,31 +260,35 @@ error_t window_manager_stop(void) { return ERROR_NONE; } -WindowId window_manager_create(uint32_t app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data) { +WindowId window_manager_create(AppInstanceId app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data) { + if (app_instance_id == 0) { + return 0; + } + auto& s = state(); + // See lifecycle_mutex's comment - blocks a concurrent window_manager_stop() (or another + // create()/remove()) from touching real_root_widget/content_root_widget/top_widget while + // this call still holds pointers to them. + mutex_lock(&s.lifecycle_mutex); + mutex_lock(&s.mutex); if (!s.started) { mutex_unlock(&s.mutex); + mutex_unlock(&s.lifecycle_mutex); return 0; } lv_obj_t* content = s.content_root_widget; lv_obj_t* old_top_widget = s.top_widget; - // The current topmost window (if any) is about to be superseded - transfer its waiter (if + // The current topmost window (if any) is about to be superseded - claim its waiter (if // any) here so it gets notified below, since it's no longer topmost after this. - TaskHandle_t waiter = nullptr; - if (!s.windows.empty()) { - waiter = s.windows.back().waiting_task; - s.windows.back().waiting_task = nullptr; - } + WindowWaitSignal* waiter = !s.windows.empty() ? claim_waiter_locked(s.windows.back()) : nullptr; s.top_widget = nullptr; WindowId new_id = s.next_id++; s.windows.push_back(WindowRecord { new_id, app_instance_id, create_widgets, user_data }); mutex_unlock(&s.mutex); - if (waiter != nullptr) { - xTaskNotifyGive(waiter); - } + give_and_release(waiter); delete_widget(old_top_widget); lv_obj_t* new_widget = build_window_widget(content, create_widgets, user_data); @@ -245,25 +305,32 @@ WindowId window_manager_create(uint32_t app_instance_id, WindowCreateWidgetsFn c // another app thread) - discard what we just made. delete_widget(new_widget); + mutex_unlock(&s.lifecycle_mutex); return new_id; } void window_manager_remove(WindowId id) { auto& s = state(); + // See lifecycle_mutex's comment - blocks a concurrent window_manager_stop() (or another + // create()/remove()) from touching real_root_widget/content_root_widget/top_widget while + // this call still holds pointers to them. + mutex_lock(&s.lifecycle_mutex); + mutex_lock(&s.mutex); auto iterator = std::find_if(s.windows.begin(), s.windows.end(), [id](const WindowRecord& window) { return window.id == id; }); if (iterator == s.windows.end()) { mutex_unlock(&s.mutex); + mutex_unlock(&s.lifecycle_mutex); return; } bool was_topmost = (iterator + 1 == s.windows.end()); // The window being removed owns its own waiter (if any) - a waiter is only ever registered // while its window is topmost (see window_manager_await_state_change()), and if this window // later stopped being topmost without being removed, window_manager_create() would already - // have transferred/cleared it - so a buried window's waiting_task is always already null. - TaskHandle_t waiter = iterator->waiting_task; + // have claimed/cleared it - so a buried window's waiting_signal is always already null. + WindowWaitSignal* waiter = claim_waiter_locked(*iterator); s.windows.erase(iterator); lv_obj_t* content = s.content_root_widget; @@ -285,12 +352,11 @@ void window_manager_remove(WindowId id) { } mutex_unlock(&s.mutex); - if (waiter != nullptr) { - xTaskNotifyGive(waiter); - } + give_and_release(waiter); if (!was_topmost) { // A buried window was removed - the topmost window's widgets are unaffected. + mutex_unlock(&s.lifecycle_mutex); return; } @@ -306,6 +372,8 @@ void window_manager_remove(WindowId id) { mutex_unlock(&s.mutex); delete_widget(new_widget); + + mutex_unlock(&s.lifecycle_mutex); } WindowState window_manager_get_state(WindowId id) { @@ -319,36 +387,51 @@ WindowState window_manager_get_state(WindowId id) { WindowState window_manager_await_state_change(WindowId id, TickType_t timeout) { auto& s = state(); + // Dedicated semaphore rather than this task's default FreeRTOS notification - other + // subsystems (e.g. app_event.cpp's AppEventSubscription) use that same shared slot, so an + // unrelated notification delivered to this task could otherwise wake this wait early. + auto* signal = new (std::nothrow) WindowWaitSignal(); + if (signal == nullptr) { + return window_manager_get_state(id); + } + signal->semaphore = xSemaphoreCreateBinary(); + if (signal->semaphore == nullptr) { + delete signal; + return window_manager_get_state(id); + } + mutex_lock(&s.mutex); bool is_top = !s.windows.empty() && s.windows.back().id == id; if (!is_top) { mutex_unlock(&s.mutex); + vSemaphoreDelete(signal->semaphore); + delete signal; return WINDOW_STATE_REVOKED; } // At most one concurrent awaiter per window - see the @warning on this function. - check(s.windows.back().waiting_task == nullptr); - s.windows.back().waiting_task = xTaskGetCurrentTaskHandle(); + check(s.windows.back().waiting_signal == nullptr); + s.windows.back().waiting_signal = signal; mutex_unlock(&s.mutex); - ulTaskNotifyTake(pdTRUE, timeout); + xSemaphoreTake(signal->semaphore, timeout); - /* Deregister ourselves if a create()/remove() hasn't already claimed us (the ordinary, intended wakeup) - * Otherwise a later create()/remove() could notify a task that's no longer waiting here: - * a use-after-exit on the handle if this task is gone, or a stale wakeup the next time it waits. - * Re-locate the record by id - it may have been erased (window_manager_remove()) while we waited. */ + // Deregister ourselves if a create()/remove() hasn't already claimed us (the ordinary, + // intended wakeup) - otherwise a later create()/remove() could read a signal that's already + // been given away here. Re-locate the record by id - it may have been erased + // (window_manager_remove()) while we waited. Either way, release our own reference: + // whichever side (us or a claimer) does this last is the one that actually deletes it. mutex_lock(&s.mutex); auto iterator = std::find_if(s.windows.begin(), s.windows.end(), [id](const WindowRecord& window) { return window.id == id; }); - if (iterator != s.windows.end() && iterator->waiting_task == xTaskGetCurrentTaskHandle()) { - iterator->waiting_task = nullptr; + if (iterator != s.windows.end() && iterator->waiting_signal == signal) { + iterator->waiting_signal = nullptr; } + bool should_delete = (--signal->refcount == 0); mutex_unlock(&s.mutex); - - /* create()/remove() read+clear `waiting_task` under the lock but call xTaskNotifyGive() - * after releasing it, so a notification can still land on us right around the timeout - * boundary regardless of which branch above ran. Drain it now (non-blocking) so it doesn't - * linger and cause a spurious immediate return the next time this task awaits. */ - ulTaskNotifyTake(pdTRUE, 0); + if (should_delete) { + vSemaphoreDelete(signal->semaphore); + delete signal; + } return window_manager_get_state(id); } diff --git a/Tactility/CMakeLists.txt b/Tactility/CMakeLists.txt index e1668da93..5a6897538 100644 --- a/Tactility/CMakeLists.txt +++ b/Tactility/CMakeLists.txt @@ -8,6 +8,9 @@ list(APPEND REQUIRES_LIST TactilityKernel TactilityFreeRtos lvgl-module + lvgl-window-manager-module + app-module + app-esp32-module crypt-module gps-module gps-generic-module diff --git a/TactilityC/Include/tt_app_fileselection.h b/TactilityC/Include/tt_app_fileselection.h index 659a06d28..5916a067f 100644 --- a/TactilityC/Include/tt_app_fileselection.h +++ b/TactilityC/Include/tt_app_fileselection.h @@ -8,13 +8,13 @@ extern "C" { /** * Show a file selection dialog that allows the user to select an existing file. - * @return the launch ID of the dialog, which can be compared in onResult to identify the source + * @return the launch ID of the dialog */ AppInstanceId tt_app_fileselection_start_for_existing_file(AppInstanceId app_id); /** * Show a file selection dialog that allows the user to select a new or existing file. - * @return the launch ID of the dialog, which can be compared in onResult to identify the source + * @return the launch ID of the dialog */ AppInstanceId tt_app_fileselection_start_for_existing_or_new_file(AppInstanceId app_id); diff --git a/TactilityKernel/include/tactility/preferences.h b/TactilityKernel/include/tactility/preferences.h index 908086bf4..bea2031cd 100644 --- a/TactilityKernel/include/tactility/preferences.h +++ b/TactilityKernel/include/tactility/preferences.h @@ -21,12 +21,13 @@ extern "C" { typedef struct Preferences Preferences; /** - * Open (or create) a preferences store backed by the properties file at @a path. The file is + * Open (or create) a preferences store backed by the properties file at @a path. The parent + * directory is created (recursively, like mkdir -p) if it doesn't already exist. The file is * read into memory now; changes made with preferences_put_*() are only written back to disk by * preferences_close(). - * @param[in] path absolute or relative file path (e.g. "/data/settings.properties") - the - * parent directory must already exist - * @return the new instance, or NULL on allocation failure + * @param[in] path absolute or relative file path (e.g. "/data/settings.properties") + * @return the new instance, or NULL if the parent directory couldn't be created, or on + * allocation failure */ Preferences* preferences_open(const char* path); diff --git a/TactilityKernel/include/tactility/properties_file.h b/TactilityKernel/include/tactility/properties_file.h index a08499c03..2df582f3a 100644 --- a/TactilityKernel/include/tactility/properties_file.h +++ b/TactilityKernel/include/tactility/properties_file.h @@ -26,13 +26,22 @@ typedef struct PropertiesFile PropertiesFile; * made with properties_file_set() are only written back to disk by properties_file_close(). * @param[in] path absolute or relative file path (e.g. "/data/settings.properties") - the * parent directory must already exist - * @return the new instance, or NULL on allocation failure + * @return the new instance, or NULL on allocation failure, or NULL if @a path exists but a + * genuine I/O error interrupted reading it (a missing file is not an error - the instance + * starts out empty in that case) */ PropertiesFile* properties_file_open(const char* path); -/** Writes any pending properties_file_set() changes to the backing file, then releases the - * instance. */ -void properties_file_close(PropertiesFile* file); +/** + * Writes any pending properties_file_set() changes to the backing file (atomically - via a + * temporary file in the same directory, renamed over the real path - so a write failure leaves + * the previous on-disk content untouched rather than a truncated/partial file), then releases + * the instance either way. + * @retval ERROR_NONE the backing file was fully updated + * @retval ERROR_RESOURCE writing failed (full filesystem, I/O error, ...) - the previous + * on-disk content, if any, is unchanged; the in-memory changes are lost along with the instance + */ +error_t properties_file_close(PropertiesFile* file); bool properties_file_has(const PropertiesFile* file, const char* key); diff --git a/TactilityKernel/include/tactility/system_event.h b/TactilityKernel/include/tactility/system_event.h index 826da6542..2d5356f1c 100644 --- a/TactilityKernel/include/tactility/system_event.h +++ b/TactilityKernel/include/tactility/system_event.h @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 #pragma once +#include #include #include @@ -161,6 +162,15 @@ struct SystemEventSubscription { uint32_t sequence; uint32_t consumed_sequence; + /** Number of tasks currently blocked in system_event_await() on `semaphore` - + * system_event_unsubscribe() waits for this to reach 0 before deleting it, since + * FreeRTOS requires no task be blocked on a semaphore when it's deleted. */ + int waiter_count; + /** Set by system_event_unsubscribe() before it gives `semaphore` and waits, so a task + * already blocked in system_event_await() bails out (ERROR_INVALID_STATE) instead of + * waiting out its full timeout. Reset on the next system_event_subscribe(). */ + bool cancelled; + struct SystemEventSubscription* next; } internal; }; @@ -180,6 +190,10 @@ error_t system_event_subscribe(struct SystemEventSubscription* sub); /** * Remove a previously registered poll subscription. * @warning Does not work in ISR context. + * @warning Blocks (briefly - not for the full duration of anyone's timeout) until any task + * currently blocked in system_event_await() on @a sub has woken up and left, so it's safe to + * delete the subscription's semaphore before this call returns. A blocked awaiter is woken + * (with ERROR_INVALID_STATE) as part of this call rather than left to time out on its own. * @param[in] sub subscription to remove, as passed to system_event_subscribe() * @return ERROR_NONE on success, ERROR_NOT_FOUND if no matching subscription exists */ @@ -187,9 +201,18 @@ error_t system_event_unsubscribe(struct SystemEventSubscription* sub); /** * Blocks the calling task until a new event arrives for @a sub, or timeout elapses. + * @warning Poll subscriptions coalesce to the latest event, they are not a queue: if + * system_event_emit() is called more than once for @a sub->event.type between two + * system_event_await() calls, only the most recent event's data/timestamp is visible via + * system_event_get_data()/system_event_get_timestamp() afterward - intermediate events are + * silently overwritten, never delivered. Use system_event_callback_add() instead if every + * individual event matters. * @param[in,out] sub subscription to wait on, as passed to system_event_subscribe() * @param[in] timeout max ticks to wait - * @return ERROR_NONE if an event arrived, ERROR_TIMEOUT if the timeout elapsed + * @retval ERROR_NONE an event arrived + * @retval ERROR_TIMEOUT @a timeout elapsed first + * @retval ERROR_INVALID_STATE another task called system_event_unsubscribe() on @a sub while + * this call was blocked */ error_t system_event_await(struct SystemEventSubscription* sub, TickType_t timeout); diff --git a/TactilityKernel/source/bundle.cpp b/TactilityKernel/source/bundle.cpp index 92b49984f..20f0eff82 100644 --- a/TactilityKernel/source/bundle.cpp +++ b/TactilityKernel/source/bundle.cpp @@ -41,8 +41,19 @@ Bundle* bundle_alloc(void) { Bundle* bundle_clone(const Bundle* bundle) { auto* clone = new (std::nothrow) Bundle(); - if (clone != nullptr) { + if (clone == nullptr) { + return nullptr; + } + // The Bundle allocation above is nothrow, but copy-assigning `entries` (allocating a node + // and copying the key/value_string for every entry) is not - std::bad_alloc could still + // escape mid-copy. Callers only ever check for a NULL return, so convert that into the + // documented nullptr-on-failure contract instead of letting it propagate out of this + // extern "C" function (which would be undefined behavior). + try { clone->entries = bundle->entries; + } catch (...) { + delete clone; + return nullptr; } return clone; } diff --git a/TactilityKernel/source/preferences.cpp b/TactilityKernel/source/preferences.cpp index ee77e436b..555b36f78 100644 --- a/TactilityKernel/source/preferences.cpp +++ b/TactilityKernel/source/preferences.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -57,6 +58,60 @@ bool split_tag(const std::string& tagged_value, std::string& tag, std::string& r return true; } +// Rejects anything but an exact "0" or "1" - a manually edited or corrupted properties file +// could otherwise have e.g. "b:garbage" silently read back as false. +bool parse_bool(const std::string& raw_value, bool& out) { + if (raw_value == "0") { + out = false; + return true; + } + if (raw_value == "1") { + out = true; + return true; + } + return false; +} + +// strtol()'s own error signaling (errno/end pointer) is easy to get wrong by omission: called +// naively, it silently accepts trailing garbage ("42abc"), out-of-range input (clamped to +// LONG_MIN/LONG_MAX instead of failing), and - since `long` can be wider than int32_t (e.g. on +// the posix simulator, where `long` is 64-bit) - a value that overflows int32_t but not `long` +// would silently truncate on the narrowing cast instead of being rejected. +bool parse_int32(const std::string& raw_value, int32_t& out) { + if (raw_value.empty()) { + return false; + } + errno = 0; + char* end = nullptr; + long parsed = std::strtol(raw_value.c_str(), &end, 10); + if (errno == ERANGE || end != raw_value.c_str() + raw_value.size()) { + return false; + } + if (parsed < INT32_MIN || parsed > INT32_MAX) { + return false; + } + out = static_cast(parsed); + return true; +} + +// See parse_int32() - same reasoning, with strtoll()/`long long`/int64_t. +bool parse_int64(const std::string& raw_value, int64_t& out) { + if (raw_value.empty()) { + return false; + } + errno = 0; + char* end = nullptr; + long long parsed = std::strtoll(raw_value.c_str(), &end, 10); + if (errno == ERANGE || end != raw_value.c_str() + raw_value.size()) { + return false; + } + if (parsed < INT64_MIN || parsed > INT64_MAX) { + return false; + } + out = static_cast(parsed); + return true; +} + bool ensure_directory(const std::string& path) { struct stat info {}; if (stat(path.c_str(), &info) == 0) { @@ -75,6 +130,16 @@ bool ensure_directory_recursive(const std::string& path) { return ensure_directory(path); } +// "" if @a path has no directory component (e.g. a bare filename) - nothing to create in that +// case, the current/root directory already exists. +std::string parent_directory(const std::string& path) { + size_t slash = path.find_last_of('/'); + if (slash == std::string::npos) { + return ""; + } + return path.substr(0, slash); +} + } // namespace // Definition of the opaque handle declared in tactility/preferences.h - C callers only ever @@ -109,6 +174,11 @@ bool try_get_tagged(const PropertiesFile* file, const char* key, std::string& ta extern "C" { Preferences* preferences_open(const char* path) { + std::string directory = parent_directory(path); + if (!directory.empty() && !ensure_directory_recursive(directory)) { + return nullptr; + } + PropertiesFile* file = properties_file_open(path); if (file == nullptr) { return nullptr; @@ -129,17 +199,20 @@ void preferences_close(Preferences* preferences) { bool preferences_has_bool(const Preferences* preferences, const char* key) { std::string tag, raw_value; - return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "b"; + bool value; + return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "b" && parse_bool(raw_value, value); } bool preferences_has_int32(const Preferences* preferences, const char* key) { std::string tag, raw_value; - return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i32"; + int32_t value; + return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i32" && parse_int32(raw_value, value); } bool preferences_has_int64(const Preferences* preferences, const char* key) { std::string tag, raw_value; - return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i64"; + int64_t value; + return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i64" && parse_int64(raw_value, value); } bool preferences_has_string(const Preferences* preferences, const char* key) { @@ -152,8 +225,7 @@ bool preferences_opt_bool(const Preferences* preferences, const char* key, bool* if (!try_get_tagged(preferences->file, key, tag, raw_value) || tag != "b") { return false; } - *out_value = (raw_value == "1"); - return true; + return parse_bool(raw_value, *out_value); } bool preferences_opt_int32(const Preferences* preferences, const char* key, int32_t* out_value) { @@ -161,8 +233,7 @@ bool preferences_opt_int32(const Preferences* preferences, const char* key, int3 if (!try_get_tagged(preferences->file, key, tag, raw_value) || tag != "i32") { return false; } - *out_value = static_cast(std::strtol(raw_value.c_str(), nullptr, 10)); - return true; + return parse_int32(raw_value, *out_value); } bool preferences_opt_int64(const Preferences* preferences, const char* key, int64_t* out_value) { @@ -170,8 +241,7 @@ bool preferences_opt_int64(const Preferences* preferences, const char* key, int6 if (!try_get_tagged(preferences->file, key, tag, raw_value) || tag != "i64") { return false; } - *out_value = static_cast(std::strtoll(raw_value.c_str(), nullptr, 10)); - return true; + return parse_int64(raw_value, *out_value); } error_t preferences_opt_string(const Preferences* preferences, const char* key, char* out_value, size_t out_value_size) { diff --git a/TactilityKernel/source/properties_file.cpp b/TactilityKernel/source/properties_file.cpp index 418bb191b..c2203bab5 100644 --- a/TactilityKernel/source/properties_file.cpp +++ b/TactilityKernel/source/properties_file.cpp @@ -48,7 +48,10 @@ namespace { // close(). Mirrors Tactility's loadPropertiesFile(): "#"-prefixed and blank lines are skipped; // a "[section]" line becomes a literal prefix (verbatim, brackets included) prepended to every // subsequent key, until the next "[section]" line replaces it. -void load_from_file(PropertiesFile* file) { +// @return false if the file exists but a genuine I/O error interrupted reading it (fgetc()'s +// EOF return doesn't by itself distinguish clean end-of-file from a read error - ferror() after +// the loop does); true otherwise, including for a missing file. +bool load_from_file(PropertiesFile* file) { FileMutex mutex {}; file_mutex_get(&mutex, file->path.c_str()); file_mutex_lock(&mutex); @@ -56,7 +59,7 @@ void load_from_file(PropertiesFile* file) { FILE* handle = std::fopen(file->path.c_str(), "r"); if (handle == nullptr) { file_mutex_unlock(&mutex); - return; + return true; } std::string key_prefix; @@ -94,28 +97,62 @@ void load_from_file(PropertiesFile* file) { } flush_line(); + bool read_ok = std::ferror(handle) == 0; std::fclose(handle); file_mutex_unlock(&mutex); + + if (!read_ok) { + LOG_E(TAG, "Failed to read %s", file->path.c_str()); + } + return read_ok; } -void save_to_file(const PropertiesFile* file) { +// Writes to a temporary file in the same directory, then atomically replaces the real path - +// opening the real path directly with "w" would truncate it immediately, so any failure +// partway through (full filesystem, I/O error, a reset before close) would discard the +// previously-good content instead of leaving it intact. Same directory so rename() stays on one +// filesystem, which is what makes it atomic. +// @return true if the backing file was fully replaced with the current entries; false (leaving +// the previous on-disk content untouched) if any step failed. +bool save_to_file(const PropertiesFile* file) { FileMutex mutex {}; file_mutex_get(&mutex, file->path.c_str()); file_mutex_lock(&mutex); - FILE* handle = std::fopen(file->path.c_str(), "w"); + std::string temp_path = file->path + ".tmp"; + + FILE* handle = std::fopen(temp_path.c_str(), "w"); if (handle == nullptr) { - LOG_E(TAG, "Failed to open %s", file->path.c_str()); + LOG_E(TAG, "Failed to open %s", temp_path.c_str()); file_mutex_unlock(&mutex); - return; + return false; } for (const auto& [key, value] : file->entries) { std::fprintf(handle, "%s=%s\n", key.c_str(), value.c_str()); } - std::fclose(handle); + // Order matters: ferror()/fflush() need the still-open handle, fclose() consumes it. + bool write_ok = std::ferror(handle) == 0; + bool flush_ok = std::fflush(handle) == 0; + bool close_ok = std::fclose(handle) == 0; + + if (!write_ok || !flush_ok || !close_ok) { + LOG_E(TAG, "Failed to write %s", temp_path.c_str()); + std::remove(temp_path.c_str()); + file_mutex_unlock(&mutex); + return false; + } + + if (std::rename(temp_path.c_str(), file->path.c_str()) != 0) { + LOG_E(TAG, "Failed to replace %s", file->path.c_str()); + std::remove(temp_path.c_str()); + file_mutex_unlock(&mutex); + return false; + } + file_mutex_unlock(&mutex); + return true; } } // namespace @@ -128,13 +165,17 @@ PropertiesFile* properties_file_open(const char* path) { return nullptr; } file->path = path; - load_from_file(file); + if (!load_from_file(file)) { + delete file; + return nullptr; + } return file; } -void properties_file_close(PropertiesFile* file) { - save_to_file(file); +error_t properties_file_close(PropertiesFile* file) { + bool saved = save_to_file(file); delete file; + return saved ? ERROR_NONE : ERROR_RESOURCE; } bool properties_file_has(const PropertiesFile* file, const char* key) { diff --git a/TactilityKernel/source/system_event.cpp b/TactilityKernel/source/system_event.cpp index 403845b7a..4207482ac 100644 --- a/TactilityKernel/source/system_event.cpp +++ b/TactilityKernel/source/system_event.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include @@ -186,6 +187,8 @@ error_t system_event_subscribe(SystemEventSubscription* sub) { sub->internal.semaphore = semaphore; sub->internal.sequence = 0; sub->internal.consumed_sequence = 0; + sub->internal.waiter_count = 0; + sub->internal.cancelled = false; sub->event.data_len = 0; sub->internal.next = poll_subscriptions; poll_subscriptions = sub; @@ -197,6 +200,7 @@ error_t system_event_subscribe(SystemEventSubscription* sub) { error_t system_event_unsubscribe(SystemEventSubscription* sub) { error_t result = ERROR_NOT_FOUND; + SemaphoreHandle_t semaphore_to_delete = nullptr; mutex_lock(&poll_subscriptions_mutex.handle); for (SystemEventSubscription** link = &poll_subscriptions; *link != nullptr; link = &(*link)->internal.next) { @@ -206,37 +210,109 @@ error_t system_event_unsubscribe(SystemEventSubscription* sub) { break; } } - mutex_unlock(&poll_subscriptions_mutex.handle); - if (result == ERROR_NONE) { - // Unlinked first, so notify_poll_subscribers() can no longer reach this semaphore - // before it's deleted. - vSemaphoreDelete(sub->internal.semaphore); + // Unlinked first, so notify_poll_subscribers() can no longer reach this subscription. + // Mark it cancelled (checked by system_event_await()'s loop) and capture the semaphore + // handle into a local variable rather than deleting it via sub->internal.semaphore + // directly - a concurrent system_event_subscribe() re-registering this same `sub` after + // this point would overwrite that field with a freshly created semaphore, and we must + // not delete the wrong (newly active) one. + sub->internal.cancelled = true; + semaphore_to_delete = sub->internal.semaphore; sub->internal.semaphore = nullptr; } + mutex_unlock(&poll_subscriptions_mutex.handle); + + if (result != ERROR_NONE) { + return result; + } + + // Nudge any task already blocked in system_event_await() (it captured its own local copy + // of this same semaphore handle before this point, so it's unaffected by the field having + // just been cleared above) so it re-checks `cancelled` and bails out now instead of waiting + // out its full timeout, then wait for it to actually leave the semaphore before deleting it + // - FreeRTOS requires no task be blocked on a semaphore when it's deleted. + xSemaphoreGive(semaphore_to_delete); + while (true) { + mutex_lock(&poll_subscriptions_mutex.handle); + bool still_waiting = sub->internal.waiter_count > 0; + mutex_unlock(&poll_subscriptions_mutex.handle); + + if (!still_waiting) { + break; + } + delay_ticks(pdMS_TO_TICKS(10)); + } + + vSemaphoreDelete(semaphore_to_delete); + + // Reset so a future system_event_subscribe() re-registering this same `sub` isn't left + // pre-cancelled (subscribe() also resets this itself, defensively). + sub->internal.cancelled = false; + + return ERROR_NONE; +} + +error_t system_event_await(SystemEventSubscription* sub, TickType_t timeout) { + mutex_lock(&poll_subscriptions_mutex.handle); + SemaphoreHandle_t semaphore = sub->internal.semaphore; + sub->internal.waiter_count++; + mutex_unlock(&poll_subscriptions_mutex.handle); + + error_t result = ERROR_NONE; + + // sequence/consumed_sequence are written by notify_poll_subscribers() under + // poll_subscriptions_mutex - read (and, on a match, updated) under the same lock each + // iteration, rather than compared lock-free, so a concurrent emit can't land between an + // unlocked read and this loop acting on it. + // + // Compare against consumed_sequence, not a sequence snapshot taken now - an emit that + // landed between system_event_subscribe() and this call already incremented sequence and + // gave the semaphore, so that event is pending but unconsumed. Snapshotting "now" would + // make the loop wait for yet another event instead of returning this already-pending one. + while (true) { + mutex_lock(&poll_subscriptions_mutex.handle); + bool pending = sub->internal.sequence != sub->internal.consumed_sequence; + bool cancelled = sub->internal.cancelled; + if (pending) { + sub->internal.consumed_sequence = sub->internal.sequence; + } + mutex_unlock(&poll_subscriptions_mutex.handle); + + if (pending) { + break; + } + if (cancelled) { + result = ERROR_INVALID_STATE; + break; + } + if (xSemaphoreTake(semaphore, timeout) == pdFALSE) { + result = ERROR_TIMEOUT; + break; + } + } + + mutex_lock(&poll_subscriptions_mutex.handle); + sub->internal.waiter_count--; + mutex_unlock(&poll_subscriptions_mutex.handle); return result; } -error_t system_event_await(SystemEventSubscription* sub, TickType_t timeout) { - uint32_t old_sequence = sub->internal.sequence; - - while (sub->internal.sequence == old_sequence) { - if (xSemaphoreTake(sub->internal.semaphore, timeout) == pdFALSE) { - return ERROR_TIMEOUT; - } - } - - sub->internal.consumed_sequence = sub->internal.sequence; - return ERROR_NONE; -} - error_t system_event_get_data(SystemEventSubscription* sub, uint8_t* data, size_t data_len) { + // sub->event.* is written by notify_poll_subscribers() under poll_subscriptions_mutex - + // the length check and the copy must happen as one snapshot under the same lock, otherwise + // a concurrent emit could grow data_len (or overwrite data) between the check and the + // memcpy below. + mutex_lock(&poll_subscriptions_mutex.handle); + error_t result = ERROR_NONE; if (data_len < sub->event.data_len) { - return ERROR_BUFFER_OVERFLOW; + result = ERROR_BUFFER_OVERFLOW; + } else { + std::memcpy(data, sub->event.data, sub->event.data_len); } - std::memcpy(data, sub->event.data, sub->event.data_len); - return ERROR_NONE; + mutex_unlock(&poll_subscriptions_mutex.handle); + return result; } } // extern "C" diff --git a/Tests/TactilityKernel/Source/PreferencesTest.cpp b/Tests/TactilityKernel/Source/PreferencesTest.cpp index 2c1d28987..635c7afd4 100644 --- a/Tests/TactilityKernel/Source/PreferencesTest.cpp +++ b/Tests/TactilityKernel/Source/PreferencesTest.cpp @@ -3,6 +3,8 @@ #include #include +#include +#include namespace { @@ -22,6 +24,19 @@ bool file_exists(const char* path) { return true; } +bool is_directory(const char* path) { + struct stat info {}; + return stat(path, &info) == 0 && (info.st_mode & S_IFMT) == S_IFDIR; +} + +// Writes a raw properties file directly (bypassing preferences_put_*()) so a test can exercise +// a hand-crafted/corrupted payload that preferences_put_*() itself would never produce. +void write_raw(const char* path, const char* content) { + FILE* file = std::fopen(path, "w"); + std::fputs(content, file); + std::fclose(file); +} + } // namespace TEST_CASE("preferences_open_path on a missing file starts out empty, without creating it") { @@ -98,6 +113,47 @@ TEST_CASE("has_*/opt_* reject a key stored with a different type") { preferences_close(preferences); } +TEST_CASE("has_*/opt_* reject malformed scalar payloads instead of misparsing them") { + ScratchFile scratch; + write_raw(TEST_PATH, + "bad_bool=b:garbage\n" + "bad_bool_2=b:2\n" + "trailing_junk=i32:42abc\n" + "int32_overflow=i32:5000000000\n" + "int64_overflow=i64:99999999999999999999\n" + "empty_int=i32:\n"); + + Preferences* preferences = preferences_open(TEST_PATH); + + // "b:garbage" must not silently read back as false - has_bool()/opt_bool() must agree it's + // not a valid bool at all. + CHECK_FALSE(preferences_has_bool(preferences, "bad_bool")); + bool bool_out = true; + CHECK_FALSE(preferences_opt_bool(preferences, "bad_bool", &bool_out)); + CHECK_FALSE(preferences_has_bool(preferences, "bad_bool_2")); + CHECK_FALSE(preferences_opt_bool(preferences, "bad_bool_2", &bool_out)); + + // "42abc" must not silently parse as 42 - the full payload must be consumed. + CHECK_FALSE(preferences_has_int32(preferences, "trailing_junk")); + int32_t int32_out = 0; + CHECK_FALSE(preferences_opt_int32(preferences, "trailing_junk", &int32_out)); + + // Fits in a (64-bit, on this platform) `long` but overflows int32_t - must not silently + // truncate on the narrowing cast. + CHECK_FALSE(preferences_has_int32(preferences, "int32_overflow")); + CHECK_FALSE(preferences_opt_int32(preferences, "int32_overflow", &int32_out)); + + // Overflows even a 64-bit integer - strtoll() itself reports ERANGE. + CHECK_FALSE(preferences_has_int64(preferences, "int64_overflow")); + int64_t int64_out = 0; + CHECK_FALSE(preferences_opt_int64(preferences, "int64_overflow", &int64_out)); + + CHECK_FALSE(preferences_has_int32(preferences, "empty_int")); + CHECK_FALSE(preferences_opt_int32(preferences, "empty_int", &int32_out)); + + preferences_close(preferences); +} + TEST_CASE("a string value with embedded newlines and backslashes survives a reopen") { ScratchFile scratch; @@ -151,3 +207,37 @@ TEST_CASE("put_* on an already-closed value is visible without reopening") { CHECK_EQ(out, 2); preferences_close(final_instance); } + +TEST_CASE("preferences_open creates missing parent directories (recursively) and persists into them") { + const char* nested_dir_a = "/tmp/tactility_kernel_preferences_test_nested"; + const char* nested_dir_b = "/tmp/tactility_kernel_preferences_test_nested/a"; + const char* nested_dir_c = "/tmp/tactility_kernel_preferences_test_nested/a/b"; + const char* nested_path = "/tmp/tactility_kernel_preferences_test_nested/a/b/settings.properties"; + + std::remove(nested_path); + rmdir(nested_dir_c); + rmdir(nested_dir_b); + rmdir(nested_dir_a); + REQUIRE_FALSE(is_directory(nested_dir_a)); + + Preferences* preferences = preferences_open(nested_path); + REQUIRE_NE(preferences, nullptr); + CHECK(is_directory(nested_dir_a)); + CHECK(is_directory(nested_dir_b)); + CHECK(is_directory(nested_dir_c)); + + preferences_put_int32(preferences, "count", 7); + preferences_close(preferences); + CHECK(file_exists(nested_path)); + + Preferences* reopened = preferences_open(nested_path); + int32_t out = 0; + CHECK(preferences_opt_int32(reopened, "count", &out)); + CHECK_EQ(out, 7); + preferences_close(reopened); + + std::remove(nested_path); + rmdir(nested_dir_c); + rmdir(nested_dir_b); + rmdir(nested_dir_a); +} diff --git a/Tests/TactilityKernel/Source/PropertiesFileTest.cpp b/Tests/TactilityKernel/Source/PropertiesFileTest.cpp index 6f7a4b579..520b9b47d 100644 --- a/Tests/TactilityKernel/Source/PropertiesFileTest.cpp +++ b/Tests/TactilityKernel/Source/PropertiesFileTest.cpp @@ -4,6 +4,8 @@ #include #include #include +#include +#include #include #include @@ -44,6 +46,19 @@ TEST_CASE("properties_file_open on a missing file starts out empty, without crea properties_file_close(file); } +TEST_CASE("properties_file_open returns NULL when a genuine I/O error interrupts reading") { + // fopen() on a directory succeeds on Linux, but the first read fails with EISDIR and sets + // the stream's error indicator - a deterministic way to exercise load_from_file()'s + // ferror() check without needing real storage-hardware fault injection. + const char* dir_path = "/tmp/tactility_kernel_properties_file_test_is_a_directory"; + rmdir(dir_path); + REQUIRE_EQ(mkdir(dir_path, 0777), 0); + + CHECK_EQ(properties_file_open(dir_path), nullptr); + + rmdir(dir_path); +} + TEST_CASE("set/has/get round-trip, and close persists while unclosed changes don't") { ScratchFile scratch; @@ -154,6 +169,63 @@ TEST_CASE("a [section] line prefixes every following key until the next section" properties_file_close(file); } +TEST_CASE("properties_file_close reports ERROR_NONE on success") { + ScratchFile scratch; + + PropertiesFile* file = properties_file_open(TEST_PATH); + properties_file_set(file, "key", "value"); + + CHECK_EQ(properties_file_close(file), ERROR_NONE); +} + +TEST_CASE("properties_file_close reports ERROR_RESOURCE when the parent directory doesn't exist") { + const char* path = "/tmp/tactility_kernel_properties_file_test_missing_dir/settings.properties"; + std::remove(path); // no-op if the directory doesn't exist, which is the point of this test + + // Missing directory is not an error for open() - it starts out empty, same as a missing + // file (see the "starts out empty" test above). + PropertiesFile* file = properties_file_open(path); + REQUIRE_NE(file, nullptr); + properties_file_set(file, "key", "value"); + + // close()'s save can't create its temp file in a directory that doesn't exist. + CHECK_EQ(properties_file_close(file), ERROR_RESOURCE); +} + +TEST_CASE("a failed close leaves previously-saved content on disk untouched") { + const char* dir = "/tmp/tactility_kernel_properties_file_readonly_test"; + const char* path = "/tmp/tactility_kernel_properties_file_readonly_test/settings.properties"; + + mkdir(dir, 0777); + chmod(dir, 0777); + std::remove(path); + + { + PropertiesFile* file = properties_file_open(path); + properties_file_set(file, "key", "original"); + REQUIRE_EQ(properties_file_close(file), ERROR_NONE); + } + + // Read-only directory - save_to_file()'s temp file can't be created there, so the close + // below must fail without disturbing the "original" content already on disk. + REQUIRE_EQ(chmod(dir, 0555), 0); + + PropertiesFile* file = properties_file_open(path); + properties_file_set(file, "key", "corrupted"); + CHECK_EQ(properties_file_close(file), ERROR_RESOURCE); + + chmod(dir, 0777); // restore write access for the check below and for cleanup + + PropertiesFile* reloaded = properties_file_open(path); + char buffer[32]; + CHECK_EQ(properties_file_get(reloaded, "key", buffer, sizeof(buffer)), ERROR_NONE); + CHECK_EQ(std::strcmp(buffer, "original"), 0); + properties_file_close(reloaded); + + std::remove(path); + rmdir(dir); +} + TEST_CASE("properties_file_for_each visits every key exactly once") { ScratchFile scratch; diff --git a/Tests/TactilityKernel/Source/SystemEventTest.cpp b/Tests/TactilityKernel/Source/SystemEventTest.cpp index fa7c8e09d..750c66cbc 100644 --- a/Tests/TactilityKernel/Source/SystemEventTest.cpp +++ b/Tests/TactilityKernel/Source/SystemEventTest.cpp @@ -257,6 +257,31 @@ TEST_CASE("system_event_subscribe/_await deliver the event payload by value") { CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NOT_FOUND); } +TEST_CASE("system_event_await returns a matching event that arrived before it started waiting") { + SystemEventSubscription sub {}; + sub.event.type = KERNEL_EVENT_NETWORK_CONNECTED; + CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE); + + // Same-thread emit, no background thread needed: unlike the "detects a change after it + // starts waiting" tests above, this is exactly the case system_event_await() must handle - + // sequence already moved ahead of consumed_sequence before await() is even called. + NetworkConnectedEvent connected { .device = nullptr, .ipv4_addr = 0x0A000001, .gateway = 0x0A0000FE }; + CHECK_EQ(system_event_emit(KERNEL_EVENT_NETWORK_CONNECTED, &connected, sizeof(connected)), ERROR_NONE); + + CHECK_EQ(system_event_await(&sub, 0), ERROR_NONE); + + NetworkConnectedEvent received {}; + CHECK_EQ(system_event_get_data(&sub, reinterpret_cast(&received), sizeof(received)), ERROR_NONE); + CHECK_EQ(received.ipv4_addr, connected.ipv4_addr); + CHECK_EQ(received.gateway, connected.gateway); + + // The pending event was consumed by the call above - a second await() with no further + // emit must time out rather than returning the same event again. + CHECK_EQ(system_event_await(&sub, 0), ERROR_TIMEOUT); + + system_event_unsubscribe(&sub); +} + TEST_CASE("system_event_await times out when no matching event has arrived") { SystemEventSubscription sub {}; sub.event.type = KERNEL_EVENT_TIME_CHANGED; @@ -341,3 +366,74 @@ TEST_CASE("system_event_get_data on a subscription with no payload copies nothin system_event_unsubscribe(&sub); } + +// Regression coverage for system_event_unsubscribe() racing a task blocked in +// system_event_await() on the same subscription, and for reusing a subscription node after +// unsubscribing it - see the @warning on system_event_unsubscribe() in system_event.h. + +TEST_CASE("system_event_unsubscribe wakes a task blocked in system_event_await with ERROR_INVALID_STATE") { + SystemEventSubscription sub {}; + sub.event.type = KERNEL_EVENT_SERVICE_STARTED; + CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE); + + auto* thread = thread_alloc_full( + "system-event-awaiter", + 4096, + [](void* context) { + auto* awaited_sub = static_cast(context); + // Long timeout - the point is that unsubscribe() wakes this early, not that it + // eventually times out on its own. + return static_cast(system_event_await(awaited_sub, pdMS_TO_TICKS(5000))); + }, + &sub, + -1 + ); + CHECK_EQ(thread_start(thread), ERROR_NONE); + + // Give the awaiter task a moment to actually reach xSemaphoreTake() before unsubscribing - + // otherwise this test wouldn't exercise the "already blocked" race at all. + delay_millis(20); + + // Must return promptly (nudging the blocked awaiter awake), not by waiting out its timeout. + TickType_t before = get_ticks(); + CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE); + CHECK_LT(get_ticks() - before, pdMS_TO_TICKS(1000)); + + CHECK_EQ(thread_join(thread, 2, 1), ERROR_NONE); + CHECK_EQ(thread_get_return_code(thread), ERROR_INVALID_STATE); + thread_free(thread); + + // A second unsubscribe() has nothing left to do. + CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NOT_FOUND); +} + +TEST_CASE("a subscription node can be re-subscribed after system_event_unsubscribe") { + SystemEventSubscription sub {}; + sub.event.type = KERNEL_EVENT_SERVICE_STOPPED; + + CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE); + CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE); + + // Re-registering the same node (same storage, not a fresh SystemEventSubscription) must + // work as if it were new - a fresh semaphore, and no leftover `cancelled` state from the + // unsubscribe() above causing an immediate spurious ERROR_INVALID_STATE below. + CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE); + + auto* thread = thread_alloc_full( + "system-event-emitter", + 4096, + [](void*) { + delay_millis(20); + system_event_emit(KERNEL_EVENT_SERVICE_STOPPED, nullptr, 0); + return 0; + }, + nullptr, + -1 + ); + CHECK_EQ(thread_start(thread), ERROR_NONE); + CHECK_EQ(system_event_await(&sub, pdMS_TO_TICKS(2000)), ERROR_NONE); + CHECK_EQ(thread_join(thread, 2, 1), ERROR_NONE); + thread_free(thread); + + CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE); +}