From de46401d85470254b6bde7777449d15957a485ef Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sun, 23 Feb 2025 16:08:00 +0100 Subject: [PATCH] Loader refactored (#235) - Moved all Loader functionality into Loader class - Improvement for Dispatcher construction - Dispatcher and DispatcherThread: you can now specify the timeout when calling `dispatch()`. Default timeout is max timeout. --- Documentation/ideas.md | 3 +- .../Tactility/service/loader/Loader_i.h | 25 +- Tactility/Source/service/loader/Loader.cpp | 341 +++++++++--------- TactilityCore/Include/Tactility/Dispatcher.h | 4 +- .../Include/Tactility/DispatcherThread.h | 2 +- TactilityCore/Include/Tactility/PubSub.h | 4 +- TactilityCore/Source/Dispatcher.cpp | 4 +- TactilityCore/Source/DispatcherThread.cpp | 10 +- 8 files changed, 203 insertions(+), 190 deletions(-) diff --git a/Documentation/ideas.md b/Documentation/ideas.md index 083f9739..152637ee 100644 --- a/Documentation/ideas.md +++ b/Documentation/ideas.md @@ -35,8 +35,7 @@ - All drivers (e.g. display, touch, etc.) should call stop() in their destructor, or at least assert that they should not be running. # Nice-to-haves -- CoreS3 has a hardware issue that prevents mounting SD cards while using the display too: allow USB Mass Storage to use `/data` instead? Perhaps give the USB settings app a drop down to select the root filesystem to attach. -- Give external app a different icon. Allow an external app update their id, icon, type and name once they are running(, and persist that info?). Loader will need to be able to find app by (external) location. +- Give external app a different icon. Allow an external app update their id, icon, type and name once they are running(and persist that info?). Loader will need to be able to find app by (external) location. - Audio player app - Audio recording app - OTA updates diff --git a/Tactility/Private/Tactility/service/loader/Loader_i.h b/Tactility/Private/Tactility/service/loader/Loader_i.h index 3ec659db..8e268ede 100644 --- a/Tactility/Private/Tactility/service/loader/Loader_i.h +++ b/Tactility/Private/Tactility/service/loader/Loader_i.h @@ -24,10 +24,7 @@ public: std::shared_ptr _Nullable parameters; LoaderMessageAppStart() = default; - - LoaderMessageAppStart(LoaderMessageAppStart& other) : - id(other.id), - parameters(other.parameters) {} + LoaderMessageAppStart(LoaderMessageAppStart& other) = default; LoaderMessageAppStart(const std::string& id, std::shared_ptr parameters) : id(id), @@ -37,16 +34,22 @@ public: ~LoaderMessageAppStart() = default; }; +class LoaderMessageAppStop { +public: + std::string id; + + LoaderMessageAppStop() = default; + LoaderMessageAppStop(LoaderMessageAppStop& other) = default; + + LoaderMessageAppStop(const std::string& id) : id(id) {} + + ~LoaderMessageAppStop() = default; +}; + // endregion LoaderMessage struct Loader { - std::shared_ptr pubsubExternal = std::make_shared(); - Mutex mutex = Mutex(Mutex::Type::Recursive); - std::stack> appStack; - /** The dispatcher thread needs a callstack large enough to accommodate all the dispatched methods. - * This includes full LVGL redraw via Gui::redraw() - */ - std::unique_ptr dispatcherThread = std::make_unique("loader_dispatcher", 6144); // Files app requires ~5k + }; } // namespace diff --git a/Tactility/Source/service/loader/Loader.cpp b/Tactility/Source/service/loader/Loader.cpp index 64a67c5a..88421938 100644 --- a/Tactility/Source/service/loader/Loader.cpp +++ b/Tactility/Source/service/loader/Loader.cpp @@ -4,12 +4,13 @@ #include "Tactility/service/loader/Loader_i.h" #include +#include #include #ifdef ESP_PLATFORM -#include "Tactility/app/ElfApp.h" #include #include +#include #else #include "Tactility/lvgl/LvglSync.h" #endif @@ -17,71 +18,9 @@ namespace tt::service::loader { #define TAG "loader" +#define LOADER_TIMEOUT (100 / portTICK_PERIOD_MS) -enum class LoaderStatus { - Ok, - ErrorAppStarted, - ErrorUnknownApp, - ErrorInternal, -}; - -typedef struct { - LoaderEventType type; -} LoaderEventInternal; - -// Forward declarations -static void onStartAppMessage(std::shared_ptr message); -static void onStopAppMessage(TT_UNUSED std::shared_ptr message); -static void stopAppInternal(); -static LoaderStatus startAppInternal(const std::string& id, std::shared_ptr _Nullable parameters); - -static Loader* loader_singleton = nullptr; - -static Loader* loader_alloc() { - assert(loader_singleton == nullptr); - loader_singleton = new Loader(); - return loader_singleton; -} - -static void loader_free() { - assert(loader_singleton != nullptr); - delete loader_singleton; - loader_singleton = nullptr; -} - -void startApp(const std::string& id, std::shared_ptr parameters) { - TT_LOG_I(TAG, "Start app %s", id.c_str()); - assert(loader_singleton); - auto message = std::make_shared(id, std::move(parameters)); - loader_singleton->dispatcherThread->dispatch(onStartAppMessage, message); -} - -void stopApp() { - TT_LOG_I(TAG, "Stop app"); - tt_check(loader_singleton); - loader_singleton->dispatcherThread->dispatch(onStopAppMessage, nullptr); -} - -std::shared_ptr _Nullable getCurrentAppContext() { - assert(loader_singleton); - loader_singleton->mutex.lock(); - auto app = loader_singleton->appStack.top(); - loader_singleton->mutex.unlock(); - return app; -} - -std::shared_ptr _Nullable getCurrentApp() { - auto app_context = getCurrentAppContext(); - return app_context != nullptr ? app_context->getApp() : nullptr; -} - -std::shared_ptr getPubsub() { - assert(loader_singleton); - // it's safe to return pubsub without locking - // because it's never freed and loader is never exited - // also the loader instance cannot be obtained until the pubsub is created - return loader_singleton->pubsubExternal; -} +extern const ServiceManifest manifest; static const char* appStateToString(app::State state) { switch (state) { @@ -101,64 +40,82 @@ static const char* appStateToString(app::State state) { } } -static void transitionAppToState(std::shared_ptr app, app::State state) { - const app::AppManifest& manifest = app->getManifest(); - const app::State old_state = app->getState(); +// region AppManifest - TT_LOG_I( - TAG, - "App \"%s\" state: %s -> %s", - manifest.id.c_str(), - appStateToString(old_state), - appStateToString(state) - ); +class LoaderService final : public Service { - switch (state) { - using enum app::State; - case Initial: - break; - case Started: - app->getApp()->onCreate(*app); - break; - case Showing: { - LoaderEvent event_showing = { .type = LoaderEventTypeApplicationShowing }; - loader_singleton->pubsubExternal->publish(&event_showing); - break; - } - case Hiding: { - LoaderEvent event_hiding = { .type = LoaderEventTypeApplicationHiding }; - loader_singleton->pubsubExternal->publish(&event_hiding); - break; - } - case Stopped: - // TODO: Verify manifest - app->getApp()->onDestroy(*app); - break; +private: + + std::shared_ptr pubsubExternal = std::make_shared(); + Mutex mutex = Mutex(Mutex::Type::Recursive); + std::stack> appStack; + /** The dispatcher thread needs a callstack large enough to accommodate all the dispatched methods. + * This includes full LVGL redraw via Gui::redraw() + */ + std::unique_ptr dispatcherThread = std::make_unique("loader_dispatcher", 6144); // Files app requires ~5k + + static void onStartAppMessageCallback(std::shared_ptr message); + static void onStopAppMessageCallback(std::shared_ptr message); + + void onStartAppMessage(const std::string& id, std::shared_ptr parameters); + void onStopAppMessage(const std::string& id); + + void transitionAppToState(const std::shared_ptr& app, app::State state); + +public: + + void onStart(TT_UNUSED ServiceContext& service) final { + dispatcherThread->start(); } - app->setState(state); + void onStop(TT_UNUSED ServiceContext& service) final { + // Send stop signal to thread and wait for thread to finish + mutex.withLock([this]() { + dispatcherThread->stop(); + }); + } + + void startApp(const std::string& id, std::shared_ptr parameters); + void stopApp(); + std::shared_ptr _Nullable getCurrentAppContext(); + + std::shared_ptr getPubsub() const { return pubsubExternal; } +}; + +std::shared_ptr _Nullable optScreenshotService() { + return service::findServiceById(manifest.id); } -static LoaderStatus startAppWithManifestInternal( - const std::shared_ptr& manifest, - const std::shared_ptr _Nullable& parameters -) { - tt_check(loader_singleton != nullptr); +void LoaderService::onStartAppMessageCallback(std::shared_ptr message) { + auto start_message = std::reinterpret_pointer_cast(message); + auto& id = start_message->id; + auto& parameters = start_message->parameters; - TT_LOG_I(TAG, "Start with manifest %s", manifest->id.c_str()); + optScreenshotService()->onStartAppMessage(id, parameters); +} - auto lock = loader_singleton->mutex.asScopedLock(); - if (!lock.lock(50 / portTICK_PERIOD_MS)) { - return LoaderStatus::ErrorInternal; +void LoaderService::onStartAppMessage(const std::string& id, std::shared_ptr parameters) { + + TT_LOG_I(TAG, "Start by id %s", id.c_str()); + + auto app_manifest = app::findAppById(id); + if (app_manifest == nullptr) { + TT_LOG_E(TAG, "App not found: %s", id.c_str()); + return; } - auto previous_app = !loader_singleton->appStack.empty() ? loader_singleton->appStack.top() : nullptr; + auto lock = mutex.asScopedLock(); + if (!lock.lock(LOADER_TIMEOUT)) { + TT_LOG_E(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED); + return; + } - auto new_app = std::make_shared(manifest, parameters); + auto previous_app = !appStack.empty() ? appStack.top() : nullptr; + auto new_app = std::make_shared(app_manifest, parameters); - new_app->mutableFlags().showStatusbar = (manifest->type != app::Type::Boot); + new_app->mutableFlags().showStatusbar = (app_manifest->type != app::Type::Boot); - loader_singleton->appStack.push(new_app); + appStack.push(new_app); transitionAppToState(new_app, app::State::Initial); transitionAppToState(new_app, app::State::Started); @@ -170,44 +127,24 @@ static LoaderStatus startAppWithManifestInternal( transitionAppToState(new_app, app::State::Showing); LoaderEvent event_external = { .type = LoaderEventTypeApplicationStarted }; - loader_singleton->pubsubExternal->publish(&event_external); - - return LoaderStatus::Ok; + pubsubExternal->publish(&event_external); } -static void onStartAppMessage(std::shared_ptr message) { - auto start_message = std::reinterpret_pointer_cast(message); - startAppInternal(start_message->id, start_message->parameters); +void LoaderService::onStopAppMessageCallback(std::shared_ptr message) { + TT_LOG_I(TAG, "OnStopAppMessageCallback"); + auto stop_message = std::reinterpret_pointer_cast(message); + optScreenshotService()->onStopAppMessage(stop_message->id); } -static void onStopAppMessage(TT_UNUSED std::shared_ptr message) { - stopAppInternal(); -} +void LoaderService::onStopAppMessage(const std::string& id) { -static LoaderStatus startAppInternal( - const std::string& id, - std::shared_ptr _Nullable parameters -) { - TT_LOG_I(TAG, "Start by id %s", id.c_str()); - - auto manifest = app::findAppById(id); - if (manifest == nullptr) { - TT_LOG_E(TAG, "App not found: %s", id.c_str()); - return LoaderStatus::ErrorUnknownApp; - } else { - return startAppWithManifestInternal(manifest, parameters); - } -} - -static void stopAppInternal() { - tt_check(loader_singleton != nullptr); - - auto lock = loader_singleton->mutex.asScopedLock(); - if (!lock.lock(50 / portTICK_PERIOD_MS)) { + auto lock = mutex.asScopedLock(); + if (!lock.lock(LOADER_TIMEOUT)) { + TT_LOG_E(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED); return; } - size_t original_stack_size = loader_singleton->appStack.size(); + size_t original_stack_size = appStack.size(); if (original_stack_size == 0) { TT_LOG_E(TAG, "Stop app: no app running"); @@ -215,7 +152,12 @@ static void stopAppInternal() { } // Stop current app - auto app_to_stop = loader_singleton->appStack.top(); + auto app_to_stop = appStack.top(); + + if (app_to_stop->getManifest().id != id) { + TT_LOG_E(TAG, "Stop app: id mismatch (wanted %s but found %s on top of stack)", id.c_str(), app_to_stop->getManifest().id.c_str()); + return; + } if (original_stack_size == 1 && app_to_stop->getManifest().type != app::Type::Boot) { TT_LOG_E(TAG, "Stop app: can't stop root app"); @@ -232,7 +174,7 @@ static void stopAppInternal() { transitionAppToState(app_to_stop, app::State::Hiding); transitionAppToState(app_to_stop, app::State::Stopped); - loader_singleton->appStack.pop(); + appStack.pop(); // We only expect the app to be referenced within the current scope if (app_to_stop.use_count() > 1) { @@ -250,8 +192,8 @@ static void stopAppInternal() { std::shared_ptr instance_to_resume; // If there's a previous app, resume it - if (!loader_singleton->appStack.empty()) { - instance_to_resume = loader_singleton->appStack.top(); + if (!appStack.empty()) { + instance_to_resume = appStack.top(); assert(instance_to_resume); transitionAppToState(instance_to_resume, app::State::Showing); } @@ -261,7 +203,7 @@ static void stopAppInternal() { // WARNING: After this point we cannot change the app states from this method directly anymore as we don't have a lock! LoaderEvent event_external = { .type = LoaderEventTypeApplicationStopped }; - loader_singleton->pubsubExternal->publish(&event_external); + pubsubExternal->publish(&event_external); if (instance_to_resume != nullptr) { if (result_set) { @@ -289,33 +231,96 @@ static void stopAppInternal() { } } -// region AppManifest +void LoaderService::transitionAppToState(const std::shared_ptr& app, app::State state) { + const app::AppManifest& app_manifest = app->getManifest(); + const app::State old_state = app->getState(); -class LoaderService final : public Service { + TT_LOG_I( + TAG, + "App \"%s\" state: %s -> %s", + app_manifest.id.c_str(), + appStateToString(old_state), + appStateToString(state) + ); -public: - - void onStart(TT_UNUSED ServiceContext& service) final { - tt_check(loader_singleton == nullptr); - loader_singleton = loader_alloc(); - loader_singleton->dispatcherThread->start(); - } - - void onStop(TT_UNUSED ServiceContext& service) final { - tt_check(loader_singleton != nullptr); - - // Send stop signal to thread and wait for thread to finish - if (!loader_singleton->mutex.lock(2000 / portTICK_PERIOD_MS)) { - TT_LOG_W(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT, "loader_stop"); + switch (state) { + using enum app::State; + case Initial: + break; + case Started: + app->getApp()->onCreate(*app); + break; + case Showing: { + LoaderEvent event_showing = { .type = LoaderEventTypeApplicationShowing }; + pubsubExternal->publish(&event_showing); + break; } - loader_singleton->dispatcherThread->stop(); - - loader_singleton->mutex.unlock(); - - loader_free(); - loader_singleton = nullptr; + case Hiding: { + LoaderEvent event_hiding = { .type = LoaderEventTypeApplicationHiding }; + pubsubExternal->publish(&event_hiding); + break; + } + case Stopped: + // TODO: Verify manifest + app->getApp()->onDestroy(*app); + break; } -}; + + app->setState(state); +} + +void LoaderService::startApp(const std::string& id, std::shared_ptr parameters) { + auto message = std::make_shared(id, std::move(parameters)); + dispatcherThread->dispatch(onStartAppMessageCallback, message); +} + +void LoaderService::stopApp() { + TT_LOG_I(TAG, "stopApp()"); + auto id = getCurrentAppContext()->getManifest().id; + auto message = std::make_shared(id); + dispatcherThread->dispatch(onStopAppMessageCallback, message); + TT_LOG_I(TAG, "dispatched"); +} + +std::shared_ptr _Nullable LoaderService::getCurrentAppContext() { + auto lock = mutex.asScopedLock(); + lock.lock(); + return appStack.top(); +} + +// region Public API + +void startApp(const std::string& id, std::shared_ptr parameters) { + TT_LOG_I(TAG, "Start app %s", id.c_str()); + auto service = optScreenshotService(); + assert(service); + service->startApp(id, std::move(parameters)); +} + +void stopApp() { + TT_LOG_I(TAG, "Stop app"); + auto service = optScreenshotService(); + service->stopApp(); +} + +std::shared_ptr _Nullable getCurrentAppContext() { + auto service = optScreenshotService(); + assert(service); + return service->getCurrentAppContext(); +} + +std::shared_ptr _Nullable getCurrentApp() { + auto app_context = getCurrentAppContext(); + return app_context != nullptr ? app_context->getApp() : nullptr; +} + +std::shared_ptr getPubsub() { + auto service = optScreenshotService(); + assert(service); + return service->getPubsub(); +} + +// endregion Public API extern const ServiceManifest manifest = { .id = "Loader", diff --git a/TactilityCore/Include/Tactility/Dispatcher.h b/TactilityCore/Include/Tactility/Dispatcher.h index dc17caea..362f2a68 100644 --- a/TactilityCore/Include/Tactility/Dispatcher.h +++ b/TactilityCore/Include/Tactility/Dispatcher.h @@ -38,7 +38,7 @@ private: }; Mutex mutex; - std::queue> queue; + std::queue> queue = {}; EventFlag eventFlag; public: @@ -51,7 +51,7 @@ public: * @param[in] function the function to execute elsewhere * @param[in] context the data to pass onto the function */ - void dispatch(Function function, std::shared_ptr context); + void dispatch(Function function, std::shared_ptr context, TickType_t timeout = portMAX_DELAY); /** * Consume 1 or more dispatched function (if any) until the queue is empty. diff --git a/TactilityCore/Include/Tactility/DispatcherThread.h b/TactilityCore/Include/Tactility/DispatcherThread.h index e653b8e4..64221b77 100644 --- a/TactilityCore/Include/Tactility/DispatcherThread.h +++ b/TactilityCore/Include/Tactility/DispatcherThread.h @@ -19,7 +19,7 @@ public: /** * Dispatch a message. */ - void dispatch(Dispatcher::Function function, std::shared_ptr context); + void dispatch(Dispatcher::Function function, std::shared_ptr context, TickType_t timeout = portMAX_DELAY); /** Start the thread (blocking). */ void start(); diff --git a/TactilityCore/Include/Tactility/PubSub.h b/TactilityCore/Include/Tactility/PubSub.h index 19b2ce83..5c5c332d 100644 --- a/TactilityCore/Include/Tactility/PubSub.h +++ b/TactilityCore/Include/Tactility/PubSub.h @@ -35,7 +35,9 @@ public: PubSub() = default; ~PubSub() { - tt_check(items.empty()); + if (!items.empty()) { + TT_LOG_W("Loader", "Destroying PubSub with %d active subscriptions", items.size()); + } } /** Start receiving messages at the specified handle (Threadsafe, Re-entrable) diff --git a/TactilityCore/Source/Dispatcher.cpp b/TactilityCore/Source/Dispatcher.cpp index 0a5cbf52..0fa41cee 100644 --- a/TactilityCore/Source/Dispatcher.cpp +++ b/TactilityCore/Source/Dispatcher.cpp @@ -15,10 +15,10 @@ Dispatcher::~Dispatcher() { mutex.unlock(); } -void Dispatcher::dispatch(Function function, std::shared_ptr context) { +void Dispatcher::dispatch(Function function, std::shared_ptr context, TickType_t timeout) { auto message = std::make_shared(function, std::move(context)); // Mutate - if (mutex.lock(1000 / portTICK_PERIOD_MS)) { + if (mutex.lock(timeout)) { queue.push(std::move(message)); if (queue.size() == BACKPRESSURE_WARNING_COUNT) { TT_LOG_W(TAG, "Backpressure: You're not consuming fast enough (100 queued)"); diff --git a/TactilityCore/Source/DispatcherThread.cpp b/TactilityCore/Source/DispatcherThread.cpp index 5487c39e..7410b533 100644 --- a/TactilityCore/Source/DispatcherThread.cpp +++ b/TactilityCore/Source/DispatcherThread.cpp @@ -25,12 +25,16 @@ DispatcherThread::~DispatcherThread() { void DispatcherThread::_threadMain() { do { - dispatcher.consume(1000 / portTICK_PERIOD_MS); + /** + * If this value is too high (e.g. 1 second) then the dispatcher destroys too slowly when the simulator exits. + * This causes the problems with other services doing an update (e.g. Statusbar) and calling into destroyed mutex in the global scope. + */ + dispatcher.consume(100 / portTICK_PERIOD_MS); } while (!interruptThread); } -void DispatcherThread::dispatch(Dispatcher::Function function, std::shared_ptr context) { - dispatcher.dispatch(function, std::move(context)); +void DispatcherThread::dispatch(Dispatcher::Function function, std::shared_ptr context, TickType_t timeout) { + dispatcher.dispatch(function, std::move(context), timeout); } void DispatcherThread::start() {