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

@@ -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

Tactility/Private/Tactility/service/loader/Loader_i.h

@@ -24,10 +24,7 @@ public:
     std::shared_ptr<const Bundle> _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<const Bundle> 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<PubSub> pubsubExternal = std::make_shared<PubSub>();
-    Mutex mutex = Mutex(Mutex::Type::Recursive);
-    std::stack<std::shared_ptr<app::AppInstance>> 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> dispatcherThread = std::make_unique<DispatcherThread>("loader_dispatcher", 6144); // Files app requires ~5k
+
 };
 
 } // namespace

Tactility/Source/service/loader/Loader.cpp

@@ -4,12 +4,13 @@
 #include "Tactility/service/loader/Loader_i.h"
 
 #include <Tactility/service/ServiceManifest.h>
+#include <Tactility/service/ServiceRegistry.h>
 #include <Tactility/RtosCompat.h>
 
 #ifdef ESP_PLATFORM
-#include "Tactility/app/ElfApp.h"
 #include <Tactility/TactilityHeadless.h>
 #include <esp_heap_caps.h>
+#include <utility>
 #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<void> message);
-static void onStopAppMessage(TT_UNUSED std::shared_ptr<void> message);
-static void stopAppInternal();
-static LoaderStatus startAppInternal(const std::string& id, std::shared_ptr<const Bundle> _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<const Bundle> parameters) {
-    TT_LOG_I(TAG, "Start app %s", id.c_str());
-    assert(loader_singleton);
-    auto message = std::make_shared<LoaderMessageAppStart>(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<app::AppContext> _Nullable getCurrentAppContext() {
-    assert(loader_singleton);
-    loader_singleton->mutex.lock();
-    auto app = loader_singleton->appStack.top();
-    loader_singleton->mutex.unlock();
-    return app;
-}
-
-std::shared_ptr<app::App> _Nullable getCurrentApp() {
-    auto app_context = getCurrentAppContext();
-    return app_context != nullptr ? app_context->getApp() : nullptr;
-}
-
-std::shared_ptr<PubSub> 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::AppInstance> 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<PubSub> pubsubExternal = std::make_shared<PubSub>();
+    Mutex mutex = Mutex(Mutex::Type::Recursive);
+    std::stack<std::shared_ptr<app::AppInstance>> 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> dispatcherThread = std::make_unique<DispatcherThread>("loader_dispatcher", 6144); // Files app requires ~5k
+
+    static void onStartAppMessageCallback(std::shared_ptr<void> message);
+    static void onStopAppMessageCallback(std::shared_ptr<void> message);
+
+    void onStartAppMessage(const std::string& id, std::shared_ptr<const Bundle> parameters);
+    void onStopAppMessage(const std::string& id);
+
+    void transitionAppToState(const std::shared_ptr<app::AppInstance>& 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<const Bundle> parameters);
+    void stopApp();
+    std::shared_ptr<app::AppContext> _Nullable getCurrentAppContext();
+
+    std::shared_ptr<PubSub> getPubsub() const { return pubsubExternal; }
+};
+
+std::shared_ptr<LoaderService> _Nullable optScreenshotService() {
+    return service::findServiceById<LoaderService>(manifest.id);
 }
 
-static LoaderStatus startAppWithManifestInternal(
-    const std::shared_ptr<app::AppManifest>& manifest,
-    const std::shared_ptr<const Bundle> _Nullable& parameters
-) {
-    tt_check(loader_singleton != nullptr);
+void LoaderService::onStartAppMessageCallback(std::shared_ptr<void> message) {
+    auto start_message = std::reinterpret_pointer_cast<LoaderMessageAppStart>(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<const Bundle> 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<app::AppInstance>(manifest, parameters);
+    auto previous_app = !appStack.empty() ? appStack.top() : nullptr;
+    auto new_app = std::make_shared<app::AppInstance>(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<void> message) {
-    auto start_message = std::reinterpret_pointer_cast<LoaderMessageAppStart>(message);
-    startAppInternal(start_message->id, start_message->parameters);
+void LoaderService::onStopAppMessageCallback(std::shared_ptr<void> message) {
+    TT_LOG_I(TAG, "OnStopAppMessageCallback");
+    auto stop_message = std::reinterpret_pointer_cast<LoaderMessageAppStop>(message);
+    optScreenshotService()->onStopAppMessage(stop_message->id);
 }
 
-static void onStopAppMessage(TT_UNUSED std::shared_ptr<void> message) {
-    stopAppInternal();
-}
+void LoaderService::onStopAppMessage(const std::string& id) {
 
-static LoaderStatus startAppInternal(
-    const std::string& id,
-    std::shared_ptr<const Bundle> _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<app::AppInstance> 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::AppInstance>& 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<const Bundle> parameters) {
+    auto message = std::make_shared<LoaderMessageAppStart>(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<LoaderMessageAppStop>(id);
+    dispatcherThread->dispatch(onStopAppMessageCallback, message);
+    TT_LOG_I(TAG, "dispatched");
+}
+
+std::shared_ptr<app::AppContext> _Nullable LoaderService::getCurrentAppContext() {
+    auto lock = mutex.asScopedLock();
+    lock.lock();
+    return appStack.top();
+}
+
+// region Public API
+
+void startApp(const std::string& id, std::shared_ptr<const Bundle> 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<app::AppContext> _Nullable getCurrentAppContext() {
+    auto service = optScreenshotService();
+    assert(service);
+    return service->getCurrentAppContext();
+}
+
+std::shared_ptr<app::App> _Nullable getCurrentApp() {
+    auto app_context = getCurrentAppContext();
+    return app_context != nullptr ? app_context->getApp() : nullptr;
+}
+
+std::shared_ptr<PubSub> getPubsub() {
+    auto service = optScreenshotService();
+    assert(service);
+    return service->getPubsub();
+}
+
+// endregion Public API
 
 extern const ServiceManifest manifest = {
     .id = "Loader",

TactilityCore/Include/Tactility/Dispatcher.h

@@ -38,7 +38,7 @@ private:
     };
 
     Mutex mutex;
-    std::queue<std::shared_ptr<DispatcherMessage>> queue;
+    std::queue<std::shared_ptr<DispatcherMessage>> 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<void> context);
+    void dispatch(Function function, std::shared_ptr<void> context, TickType_t timeout = portMAX_DELAY);
 
     /**
      * Consume 1 or more dispatched function (if any) until the queue is empty.

TactilityCore/Include/Tactility/DispatcherThread.h

@@ -19,7 +19,7 @@ public:
     /**
      * Dispatch a message.
      */
-    void dispatch(Dispatcher::Function function, std::shared_ptr<void> context);
+    void dispatch(Dispatcher::Function function, std::shared_ptr<void> context, TickType_t timeout = portMAX_DELAY);
 
     /** Start the thread (blocking). */
     void start();

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)

TactilityCore/Source/Dispatcher.cpp

@@ -15,10 +15,10 @@ Dispatcher::~Dispatcher() {
     mutex.unlock();
 }
 
-void Dispatcher::dispatch(Function function, std::shared_ptr<void> context) {
+void Dispatcher::dispatch(Function function, std::shared_ptr<void> context, TickType_t timeout) {
     auto message = std::make_shared<DispatcherMessage>(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)");

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<void> context) {
-    dispatcher.dispatch(function, std::move(context));
+void DispatcherThread::dispatch(Dispatcher::Function function, std::shared_ptr<void> context, TickType_t timeout) {
+    dispatcher.dispatch(function, std::move(context), timeout);
 }
 
 void DispatcherThread::start() {