Thread+locking improvements and more (#386)

Documentation/ideas.md

@@ -2,14 +2,13 @@
 
 ## Before release
 
-- App Hub
 - Fix Wi-Fi password(less) decryption crash
 - Make better esp_lcd driver (and test all devices)
 
 ## Higher Priority
 
 - Calculator bugs (see GitHub issue)
-- Store last synced timestamp in NVS (see how HTTPS client example app)
+- Expose http::download() and main dispatcher to TactiltyC.
 - External app loading: Check the version of Tactility and check ESP target hardware to check for compatibility
   Check during installation process, but also when starting (SD card might have old app install from before Tactility OS update)
 - Make a URL handler. Use it for handling local files. Match file types with apps.

Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h

@@ -6,6 +6,11 @@
 
 namespace tt::hal::sdcard {
 
+/**
+ * Warning: getLock() does not have to be used when calling any of the functions of this class.
+ * The lock is only used for file access on the path where the SD card is mounted.
+ * This is mainly used when accessing the SD card on a shared SPI bus.
+ */
 class SdCardDevice : public Device {
 
 public:
@@ -33,15 +38,28 @@ public:
 
     Type getType() const final { return Type::SdCard; };
 
+    /**
+     * Mount the device.
+     * @param mountPath the path to mount at
+     * @return true on successful mount
+     */
     virtual bool mount(const std::string& mountPath) = 0;
+
+    /**
+     * Unmount the device.
+     * @return true on successful unmount
+     */
     virtual bool unmount() = 0;
+
     virtual State getState(TickType_t timeout = portMAX_DELAY) const = 0;
-    /** Return empty string when not mounted or the mount path if mounted */
+
+    /** @return empty string when not mounted or the mount path if mounted */
     virtual std::string getMountPath() const = 0;
 
-    /** Non-null lock */
+    /** @return non-null lock, used by code that wants to access files on the mount path of this SD card */
     virtual std::shared_ptr<Lock> getLock() const = 0;
 
+    /** @return the MountBehaviour of this device */
     virtual MountBehaviour getMountBehaviour() const { return mountBehaviour; }
 
     /** @return true if the SD card was mounted, returns false when it was not or when a timeout happened. */

Tactility/Source/Tactility.cpp

@@ -101,6 +101,8 @@ namespace app {
 
 // List of all apps excluding Boot app (as Boot app calls this function indirectly)
 static void registerInternalApps() {
+    TT_LOG_I(TAG, "Registering internal apps");
+
     addAppManifest(app::alertdialog::manifest);
     addAppManifest(app::appdetails::manifest);
     addAppManifest(app::apphub::manifest);
@@ -180,6 +182,8 @@ static void registerInstalledApp(std::string path) {
 }
 
 static void registerInstalledApps(const std::string& path) {
+    TT_LOG_I(TAG, "Registering apps from %s", path.c_str());
+
     file::listDirectory(path, [&path](const auto& entry) {
         auto absolute_path = std::format("{}/{}", path, entry.d_name);
         if (file::isDirectory(absolute_path)) {
@@ -200,18 +204,12 @@ static void registerInstalledAppsFromSdCards() {
     auto sdcard_devices = hal::findDevices<hal::sdcard::SdCardDevice>(hal::Device::Type::SdCard);
     for (const auto& sdcard : sdcard_devices) {
         if (sdcard->isMounted()) {
+            TT_LOG_I(TAG, "Registering apps from %s", sdcard->getMountPath().c_str());
             registerInstalledAppsFromSdCard(sdcard);
         }
     }
 }
 
-static void registerInstalledAppsFromData() {
-    auto app_path = "/data/app";
-    if (file::isDirectory(app_path)) {
-        registerInstalledApps(app_path);
-    }
-}
-
 static void registerAndStartSecondaryServices() {
     TT_LOG_I(TAG, "Registering and starting system services");
     addService(service::loader::manifest);
@@ -242,7 +240,6 @@ void registerApps() {
         registerInstalledApps(data_apps_path);
     }
     registerInstalledAppsFromSdCards();
-    registerInstalledAppsFromData();
 }
 
 void run(const Configuration& config) {

Tactility/Source/hal/sdcard/SdCardMounting.cpp

@@ -9,12 +9,10 @@ constexpr auto* TAG = "SdCardMounting";
 constexpr auto* TT_SDCARD_MOUNT_POINT = "/sdcard";
 
 static void mount(const std::shared_ptr<SdCardDevice>& sdcard, const std::string& path) {
-    sdcard->getLock()->withLock([&sdcard, &path] {
-       TT_LOG_I(TAG, "Mounting sdcard at %s", path.c_str());
-       if (!sdcard->mount(path)) {
-           TT_LOG_W(TAG, "SD card mount failed for %s (init can continue)", path.c_str());
-       }
-   });
+   TT_LOG_I(TAG, "Mounting sdcard at %s", path.c_str());
+   if (!sdcard->mount(path)) {
+       TT_LOG_W(TAG, "SD card mount failed for %s (init can continue)", path.c_str());
+   }
 }
 
 static std::string getMountPath(int index, int count) {

Tactility/Source/hal/sdcard/SdmmcDevice.cpp

@@ -58,6 +58,9 @@ bool SdmmcDevice::mountInternal(const std::string& newMountPath) {
 }
 
 bool SdmmcDevice::mount(const std::string& newMountPath) {
+    auto lock = getLock()->asScopedLock();
+    lock.lock();
+
     if (mountInternal(newMountPath)) {
         TT_LOG_I(TAG, "Mounted at %s", newMountPath.c_str());
         sdmmc_card_print_info(stdout, card);
@@ -69,6 +72,9 @@ bool SdmmcDevice::mount(const std::string& newMountPath) {
 }
 
 bool SdmmcDevice::unmount() {
+    auto lock = getLock()->asScopedLock();
+    lock.lock();
+
     if (card == nullptr) {
         TT_LOG_E(TAG, "Can't unmount: not mounted");
         return false;

Tactility/Source/hal/sdcard/SpiSdCardDevice.cpp

@@ -84,6 +84,9 @@ bool SpiSdCardDevice::mountInternal(const std::string& newMountPath) {
 }
 
 bool SpiSdCardDevice::mount(const std::string& newMountPath) {
+    auto lock = getLock()->asScopedLock();
+    lock.lock();
+
     if (!applyGpioWorkAround()) {
         TT_LOG_E(TAG, "Failed to apply GPIO work-around");
         return false;
@@ -100,6 +103,9 @@ bool SpiSdCardDevice::mount(const std::string& newMountPath) {
 }
 
 bool SpiSdCardDevice::unmount() {
+    auto lock = getLock()->asScopedLock();
+    lock.lock();
+
     if (card == nullptr) {
         TT_LOG_E(TAG, "Can't unmount: not mounted");
         return false;

Tactility/Source/lvgl/Statusbar.cpp

@@ -48,14 +48,6 @@ typedef struct {
     PubSub<void*>::SubscriptionHandle pubsub_subscription;
 } Statusbar;
 
-static bool statusbar_lock(TickType_t timeoutTicks = portMAX_DELAY) {
-    return statusbar_data.mutex.lock(timeoutTicks);
-}
-
-static bool statusbar_unlock() {
-    return statusbar_data.mutex.unlock();
-}
-
 static void statusbar_constructor(const lv_obj_class_t* class_p, lv_obj_t* obj);
 static void statusbar_destructor(const lv_obj_class_t* class_p, lv_obj_t* obj);
 static void statusbar_event(const lv_obj_class_t* class_p, lv_event_t* event);
@@ -111,10 +103,12 @@ static const lv_obj_class_t statusbar_class = {
 
 static void statusbar_pubsub_event(Statusbar* statusbar) {
     TT_LOG_D(TAG, "Update event");
-    if (lock(portMAX_DELAY)) {
+    if (lock(defaultLockTime)) {
         update_main(statusbar);
         lv_obj_invalidate(&statusbar->obj);
         unlock();
+    } else {
+        TT_LOG_W(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT, "Statusbar");
     }
 }
 
@@ -184,7 +178,7 @@ lv_obj_t* statusbar_create(lv_obj_t* parent) {
     obj_set_style_bg_invisible(left_spacer);
     lv_obj_set_flex_grow(left_spacer, 1);
 
-    statusbar_lock(portMAX_DELAY);
+    statusbar_data.mutex.lock(portMAX_DELAY);
     for (int i = 0; i < STATUSBAR_ICON_LIMIT; ++i) {
         auto* image = lv_image_create(obj);
         lv_obj_set_size(image, STATUSBAR_ICON_SIZE, STATUSBAR_ICON_SIZE);
@@ -194,7 +188,7 @@ lv_obj_t* statusbar_create(lv_obj_t* parent) {
 
         update_icon(image, &(statusbar_data.icons[i]));
     }
-    statusbar_unlock();
+    statusbar_data.mutex.unlock();
 
     return obj;
 }
@@ -212,11 +206,11 @@ static void update_time(Statusbar* statusbar) {
 static void update_main(Statusbar* statusbar) {
     update_time(statusbar);
 
-    if (statusbar_lock(200 / portTICK_PERIOD_MS)) {
+    if (statusbar_data.mutex.lock(200 / portTICK_PERIOD_MS)) {
         for (int i = 0; i < STATUSBAR_ICON_LIMIT; ++i) {
             update_icon(statusbar->icons[i], &(statusbar_data.icons[i]));
         }
-        statusbar_unlock();
+        statusbar_data.mutex.unlock();
     }
 }
 
@@ -236,7 +230,7 @@ static void statusbar_event(TT_UNUSED const lv_obj_class_t* class_p, lv_event_t*
 }
 
 int8_t statusbar_icon_add(const std::string& image) {
-    statusbar_lock();
+    statusbar_data.mutex.lock();
     int8_t result = -1;
     for (int8_t i = 0; i < STATUSBAR_ICON_LIMIT; ++i) {
         if (!statusbar_data.icons[i].claimed) {
@@ -248,8 +242,8 @@ int8_t statusbar_icon_add(const std::string& image) {
             break;
         }
     }
+    statusbar_data.mutex.unlock();
     statusbar_data.pubsub->publish(nullptr);
-    statusbar_unlock();
     return result;
 }
 
@@ -260,37 +254,35 @@ int8_t statusbar_icon_add() {
 void statusbar_icon_remove(int8_t id) {
     TT_LOG_D(TAG, "id %d: remove", id);
     tt_check(id >= 0 && id < STATUSBAR_ICON_LIMIT);
-    statusbar_lock();
+    statusbar_data.mutex.lock();
     StatusbarIcon* icon = &statusbar_data.icons[id];
     icon->claimed = false;
     icon->visible = false;
     icon->image = "";
+    statusbar_data.mutex.unlock();
     statusbar_data.pubsub->publish(nullptr);
-    statusbar_unlock();
 }
 
 void statusbar_icon_set_image(int8_t id, const std::string& image) {
     TT_LOG_D(TAG, "id %d: set image %s", id, image.empty() ? "(none)" : image.c_str());
     tt_check(id >= 0 && id < STATUSBAR_ICON_LIMIT);
-    if (statusbar_lock()) {
-        StatusbarIcon* icon = &statusbar_data.icons[id];
-        tt_check(icon->claimed);
-        icon->image = image;
-        statusbar_data.pubsub->publish(nullptr);
-        statusbar_unlock();
-    }
+    statusbar_data.mutex.lock();
+    StatusbarIcon* icon = &statusbar_data.icons[id];
+    tt_check(icon->claimed);
+    icon->image = image;
+    statusbar_data.mutex.unlock();
+    statusbar_data.pubsub->publish(nullptr);
 }
 
 void statusbar_icon_set_visibility(int8_t id, bool visible) {
     TT_LOG_D(TAG, "id %d: set visibility %d", id, visible);
     tt_check(id >= 0 && id < STATUSBAR_ICON_LIMIT);
-    if (statusbar_lock()) {
-        StatusbarIcon* icon = &statusbar_data.icons[id];
-        tt_check(icon->claimed);
-        icon->visible = visible;
-        statusbar_data.pubsub->publish(nullptr);
-        statusbar_unlock();
-    }
+    statusbar_data.mutex.lock();
+    StatusbarIcon* icon = &statusbar_data.icons[id];
+    tt_check(icon->claimed);
+    icon->visible = visible;
+    statusbar_data.mutex.unlock();
+    statusbar_data.pubsub->publish(nullptr);
 }
 
 } // namespace

Tactility/Source/service/wifi/WifiBootSplashInit.cpp

@@ -10,6 +10,7 @@
 #include <map>
 #include <string>
 #include <vector>
+#include <Tactility/Tactility.h>
 #include <Tactility/hal/sdcard/SdCardDevice.h>
 
 namespace tt::service::wifi {
@@ -115,14 +116,16 @@ static void importWifiApSettings(std::shared_ptr<hal::sdcard::SdCardDevice> sdca
 }
 
 void bootSplashInit() {
-    const auto sdcards = hal::findDevices<hal::sdcard::SdCardDevice>(hal::Device::Type::SdCard);
-    for (auto& sdcard : sdcards) {
-        if (sdcard->isMounted()) {
-            importWifiApSettings(sdcard);
-        } else {
-            TT_LOG_W(TAG, "Skipping unmounted SD card %s", sdcard->getMountPath().c_str());
+    getMainDispatcher().dispatch([] {
+        const auto sdcards = hal::findDevices<hal::sdcard::SdCardDevice>(hal::Device::Type::SdCard);
+        for (auto& sdcard : sdcards) {
+            if (sdcard->isMounted()) {
+                importWifiApSettings(sdcard);
+            } else {
+                TT_LOG_W(TAG, "Skipping unmounted SD card %s", sdcard->getMountPath().c_str());
+            }
         }
-    }
+    });
 }
 
 }