Revert app_event_loop_run()

Documentation/ideas.md

@@ -11,6 +11,7 @@
 
 ## Higher Priority
 
+- stopAppFromToolbar() in Tactility.cpp stops the top-most app. Change it so the toolbar knows for which app id it is created, so it can rely on that.
 - Warn if file operations are done from prohibited tasks (e.g. lvgl task)
 - AppHubApp: Prevent download callbacks from accessing a destroyed view.
 - Move USB host task stacks to SPIRAM when available: esp32_usbhost*.cpp

Modules/app-module/include/app/event.h

@@ -84,7 +84,8 @@ struct AppEventSubscription {
  * task_event_group_wait()/task_event_group_wait_any() on this group (OR sub->bit into the mask,
  * or use _wait_any() to include every subscription sharing it), then drain with app_event_poll().
  * @retval ERROR_NONE on success
- * @retval ERROR_RESOURCE @a event_group has no free bits left to claim
+ * @retval ERROR_RESOURCE @a event_group has no free bits left to claim; @a sub was not registered
+ * @retval ERROR_INVALID_STATE @a sub is already registered
  */
 error_t app_event_subscribe(struct AppEventSubscription* sub, struct TaskEventGroup* event_group);
 

Modules/app-module/source/event.cpp

@@ -28,12 +28,19 @@ error_t app_event_subscribe(AppEventSubscription* sub, TaskEventGroup* event_gro
         return claim_result;
     }
 
+    mutex_lock(&subscriptions_mutex.handle);
+
+    // Avoid cyclic subscription list that would loop forever
+    if (subscriptions == sub) {
+        mutex_unlock(&subscriptions_mutex.handle);
+        task_event_group_release_bit(event_group, bit);
+        return ERROR_INVALID_STATE;
+    }
+
     sub->bit = bit;
     sub->internal.event_group = event_group;
     sub->internal.head = 0;
     sub->internal.count = 0;
-
-    mutex_lock(&subscriptions_mutex.handle);
     sub->internal.next = subscriptions;
     subscriptions = sub;
     mutex_unlock(&subscriptions_mutex.handle);

Modules/app-module/source/symbols.cpp

@@ -22,14 +22,6 @@ const ModuleSymbol app_module_symbols[] = {
     DEFINE_MODULE_SYMBOL(app_event_unsubscribe),
     DEFINE_MODULE_SYMBOL(app_event_emit),
     DEFINE_MODULE_SYMBOL(app_event_poll),
-    // tactility/concurrent/task_event_group - needed to construct the group app_event_subscribe() takes
-    DEFINE_MODULE_SYMBOL(task_event_group_construct),
-    DEFINE_MODULE_SYMBOL(task_event_group_destruct),
-    DEFINE_MODULE_SYMBOL(task_event_group_claim_bit),
-    DEFINE_MODULE_SYMBOL(task_event_group_release_bit),
-    DEFINE_MODULE_SYMBOL(task_event_group_signal),
-    DEFINE_MODULE_SYMBOL(task_event_group_wait),
-    DEFINE_MODULE_SYMBOL(task_event_group_wait_any),
     // app/install
     DEFINE_MODULE_SYMBOL(app_get_install_path),
     DEFINE_MODULE_SYMBOL(app_install),

Platforms/platform-esp32/source/drivers/esp32_wifi.cpp

@@ -538,10 +538,18 @@ error_t api_event_subscribe(Device* device, WifiEventSubscription* sub, TaskEven
     if (claim_result != ERROR_NONE) {
         return claim_result;
     }
-    sub->internal.event_group = event_group;
-    sub->bit = bit;
 
     mutex_lock(&ctx->subscriptionsMutex);
+
+    // Avoid cyclic subscription list that would loop forever
+    if (ctx->subscriptions == sub) {
+        mutex_unlock(&ctx->subscriptionsMutex);
+        task_event_group_release_bit(event_group, bit);
+        return ERROR_INVALID_STATE;
+    }
+
+    sub->internal.event_group = event_group;
+    sub->bit = bit;
     sub->internal.next = ctx->subscriptions;
     ctx->subscriptions = sub;
     mutex_unlock(&ctx->subscriptionsMutex);
@@ -629,6 +637,19 @@ error_t stop_device(Device* device) {
         bring_down_wifi(ctx);
     }
 
+    // Release any subscribers that never unsubscribed: device_stop() doesn't wait for apps still
+    // using this device, so a later wifi_event_unsubscribe() would find no ctx and skip releasing
+    // the bit and destructing sub->internal.ring_mutex.
+    mutex_lock(&ctx->subscriptionsMutex);
+    for (WifiEventSubscription* sub = ctx->subscriptions; sub != nullptr;) {
+        WifiEventSubscription* next = sub->internal.next;
+        task_event_group_release_bit(sub->internal.event_group, sub->bit);
+        mutex_destruct(&sub->internal.ring_mutex);
+        sub = next;
+    }
+    ctx->subscriptions = nullptr;
+    mutex_unlock(&ctx->subscriptionsMutex);
+
     device_set_driver_data(device, nullptr);
     mutex_destruct(&ctx->subscriptionsMutex);
     mutex_destruct(&ctx->mutex);

Platforms/platform-posix/source/drivers/mock_wifi.cpp

@@ -55,15 +55,15 @@ struct PosixWifiCtx {
 
 void fireEvent(PosixWifiCtx* ctx, WifiEvent event) {
     mutex_lock(&ctx->subscriptionsMutex);
-    for (WifiEventSubscription* sub = ctx->subscriptions; sub != nullptr; sub = sub->next) {
-        mutex_lock(&sub->ring_mutex);
-        if (sub->count < WIFI_EVENT_QUEUE_CAPACITY) {
-            uint8_t tail = (sub->head + sub->count) % WIFI_EVENT_QUEUE_CAPACITY;
-            sub->queue[tail] = event;
-            sub->count++;
+    for (WifiEventSubscription* sub = ctx->subscriptions; sub != nullptr; sub = sub->internal.next) {
+        mutex_lock(&sub->internal.ring_mutex);
+        if (sub->internal.count < WIFI_EVENT_QUEUE_CAPACITY) {
+            uint8_t tail = (sub->internal.head + sub->internal.count) % WIFI_EVENT_QUEUE_CAPACITY;
+            sub->internal.queue[tail] = event;
+            sub->internal.count++;
         }
-        mutex_unlock(&sub->ring_mutex);
-        task_event_group_signal(sub->event_group, sub->bit);
+        mutex_unlock(&sub->internal.ring_mutex);
+        task_event_group_signal(sub->internal.event_group, sub->bit);
     }
     mutex_unlock(&ctx->subscriptionsMutex);
 }
@@ -266,12 +266,20 @@ error_t apiSetRadioOff(Device* device) {
 
     mutex_lock(&ctx->mutex);
     bool already_off = ctx->radioState == WIFI_RADIO_STATE_OFF;
+    bool was_connected = ctx->stationState != WIFI_STATION_STATE_DISCONNECTED;
     ctx->radioState = WIFI_RADIO_STATE_OFF;
     ctx->stationState = WIFI_STATION_STATE_DISCONNECTED;
     ctx->scanning = false;
     mutex_unlock(&ctx->mutex);
     if (already_off) return ERROR_NONE;
 
+    if (was_connected) {
+        WifiEvent station_event = {};
+        station_event.type = WIFI_EVENT_TYPE_STATION_STATE_CHANGED;
+        station_event.station_state = WIFI_STATION_STATE_DISCONNECTED;
+        fireEvent(ctx, station_event);
+    }
+
     WifiEvent radio_event = {};
     radio_event.type = WIFI_EVENT_TYPE_RADIO_STATE_CHANGED;
     radio_event.radio_state = WIFI_RADIO_STATE_OFF;
@@ -290,11 +298,19 @@ error_t apiEventSubscribe(Device* device, WifiEventSubscription* sub, TaskEventG
     if (claim_result != ERROR_NONE) {
         return claim_result;
     }
-    sub->event_group = event_group;
-    sub->bit = bit;
 
     mutex_lock(&ctx->subscriptionsMutex);
-    sub->next = ctx->subscriptions;
+
+    // Avoid cyclic subscription list that would loop forever
+    if (ctx->subscriptions == sub) {
+        mutex_unlock(&ctx->subscriptionsMutex);
+        task_event_group_release_bit(event_group, bit);
+        return ERROR_INVALID_STATE;
+    }
+
+    sub->internal.event_group = event_group;
+    sub->bit = bit;
+    sub->internal.next = ctx->subscriptions;
     ctx->subscriptions = sub;
     mutex_unlock(&ctx->subscriptionsMutex);
     return ERROR_NONE;
@@ -306,9 +322,9 @@ error_t apiEventUnsubscribe(Device* device, WifiEventSubscription* sub) {
 
     error_t result = ERROR_NOT_FOUND;
     mutex_lock(&ctx->subscriptionsMutex);
-    for (WifiEventSubscription** link = &ctx->subscriptions; *link != nullptr; link = &(*link)->next) {
+    for (WifiEventSubscription** link = &ctx->subscriptions; *link != nullptr; link = &(*link)->internal.next) {
         if (*link == sub) {
-            *link = sub->next;
+            *link = sub->internal.next;
             result = ERROR_NONE;
             break;
         }
@@ -316,7 +332,7 @@ error_t apiEventUnsubscribe(Device* device, WifiEventSubscription* sub) {
     mutex_unlock(&ctx->subscriptionsMutex);
 
     if (result == ERROR_NONE) {
-        task_event_group_release_bit(sub->event_group, sub->bit);
+        task_event_group_release_bit(sub->internal.event_group, sub->bit);
     }
     return result;
 }
@@ -364,6 +380,19 @@ error_t stopDevice(Device* device) {
         apiSetRadioOff(device);
     }
 
+    // Release any subscribers that never unsubscribed: device_stop() doesn't wait for apps still
+    // using this device, so a later wifi_event_unsubscribe() would find no ctx and skip releasing
+    // the bit and destructing sub->internal.ring_mutex.
+    mutex_lock(&ctx->subscriptionsMutex);
+    for (WifiEventSubscription* sub = ctx->subscriptions; sub != nullptr;) {
+        WifiEventSubscription* next = sub->internal.next;
+        task_event_group_release_bit(sub->internal.event_group, sub->bit);
+        mutex_destruct(&sub->internal.ring_mutex);
+        sub = next;
+    }
+    ctx->subscriptions = nullptr;
+    mutex_unlock(&ctx->subscriptionsMutex);
+
     device_set_driver_data(device, nullptr);
     mutex_destruct(&ctx->subscriptionsMutex);
     mutex_destruct(&ctx->mutex);

Tactility/Source/Tactility.cpp

@@ -141,7 +141,6 @@ namespace app {
     namespace boot { extern const ::AppManifest manifest; }
     namespace development { extern const ::AppManifest manifest; }
     namespace display { extern const ::AppManifest manifest; }
-    namespace kerneldisplay { extern const ::AppManifest manifest; }
     namespace files { extern const ::AppManifest manifest; }
     namespace fileselection { extern const ::AppManifest manifest; }
     namespace gpssettings { extern const ::AppManifest manifest; }
@@ -206,7 +205,7 @@ static void registerInternalApps() {
         app_manager_add(&app::audiosettings::manifest);
     }
     if (device_exists_of_type(&DISPLAY_TYPE)) {
-        app_manager_add(&app::kerneldisplay::manifest);
+        app_manager_add(&app::display::manifest);
     }
     app_manager_add(&app::files::manifest);
     app_manager_add(&app::fileselection::manifest);

Tactility/Source/app/display/Display.cpp

@@ -12,7 +12,6 @@
 #include <Tactility/settings/DisplaySettings.h>
 
 #include <app/event.h>
-#include <app/manager.h>
 #include <app/manifest.h>
 
 #include <lvgl_window_manager/window_manager.h>
@@ -25,11 +24,11 @@
 #include <sdkconfig.h>
 #endif
 
-namespace tt::app::kerneldisplay {
+namespace tt::app::display {
 
 extern const ::AppManifest manifest;
 
-constexpr auto* TAG = "KernelDisplay";
+constexpr auto* TAG = "Display";
 
 namespace {
 
@@ -351,4 +350,4 @@ extern const ::AppManifest manifest = {
     .location = { APP_LOCATION_MEMORY, reinterpret_cast<void*>(appMain) }
 };
 
-} // namespace tt::app::kerneldisplay
+} // namespace tt::app::display

Tactility/Source/app/wifimanage/WifiManage.cpp

@@ -72,11 +72,14 @@ static void onConnectToHidden() {
 }
 
 void updateView(Context* ctx) {
-    ctx->lock();
+    // Same lock order as createWidgets() (called with the LVGL lock already held, per the
+    // window-manager's WindowCreateWidgetsFn contract, then acquiring ctx->mutex) - acquiring
+    // these in the opposite order here would deadlock against a concurrent createWidgets() call.
     lvgl_lock();
+    ctx->lock();
     ctx->view.update();
-    lvgl_unlock();
     ctx->unlock();
+    lvgl_unlock();
 }
 
 void onWifiEvent(Context* ctx, WifiEvent event) {

Tactility/Source/service/wifi/Wifi.cpp

@@ -229,18 +229,7 @@ void dispatchConnect() {
 
     LOG_I(TAG, "Connecting to %s", target.ssid.c_str());
 
-    error_t result = wifi_station_connect(state.device, target.ssid.c_str(), target.password.c_str(), target.channel);
-    if (result != ERROR_NONE) {
-        LOG_E(TAG, "Failed to connect to %s (%s)", target.ssid.c_str(), error_to_string(result));
-        WifiEvent event = {};
-        event.type = WIFI_EVENT_TYPE_STATION_CONNECTION_RESULT;
-        // The driver couldn't even initiate the connection attempt; there's no
-        // more specific WifiStationConnectionError for that.
-        event.connection_error = WIFI_STATION_CONNECTION_ERROR_TIMEOUT;
-        publish(event);
-    }
-    // On success, WIFI_EVENT_TYPE_STATION_STATE_CHANGED / _CONNECTION_RESULT arrive
-    // asynchronously via onWifiDeviceEvent().
+    wifi_station_connect(state.device, target.ssid.c_str(), target.password.c_str(), target.channel);
 }
 
 void dispatchDisconnect() {
@@ -373,10 +362,6 @@ void onWifiDeviceEvent(Device* device, ::WifiEvent event) {
         default:
             break;
     }
-
-    // Forward the event as-is: subscribers inspect event.type and the
-    // relevant union field directly, same as this function does.
-    publish(event);
 }
 
 void autoScanSetPaused(bool paused) {

TactilityKernel/include/tactility/drivers/wifi.h

@@ -245,7 +245,8 @@ struct WifiApi {
      * mask, or use _wait_any() to include every subscription sharing it), then drain with
      * wifi_event_poll().
      * @retval ERROR_NONE on success
-     * @retval ERROR_RESOURCE @a event_group has no free bits left to claim
+     * @retval ERROR_RESOURCE @a event_group has no free bits left to claim; @a sub was not registered
+     * @retval ERROR_INVALID_STATE @a sub is already registered
      */
     error_t (*event_subscribe)(struct Device* device, struct WifiEventSubscription* sub, struct TaskEventGroup* event_group);
 
@@ -298,7 +299,8 @@ error_t wifi_station_get_rssi(struct Device* device, int32_t* rssi);
  * into the mask, or use _wait_any() to include every subscription sharing it), then drain with
  * wifi_event_poll().
  * @retval ERROR_NONE on success
- * @retval ERROR_RESOURCE @a event_group has no free bits left to claim
+ * @retval ERROR_RESOURCE @a event_group has no free bits left to claim; @a sub was not registered
+ * @retval ERROR_INVALID_STATE @a sub is already registered
  */
 error_t wifi_event_subscribe(struct Device* device, struct WifiEventSubscription* sub, struct TaskEventGroup* event_group);
 

TactilityKernel/source/system_event.cpp

@@ -173,15 +173,11 @@ error_t system_event_subscribe(SystemEventSubscription* sub, TaskEventGroup* eve
 
     mutex_lock(&poll_subscriptions_mutex.handle);
 
-    // Check-and-insert in one critical section: registering the same `sub` twice would link
-    // it into a list that already contains it, creating a cycle that notify_poll_subscribers()
-    // would then traverse forever while holding this same mutex.
-    for (SystemEventSubscription* existing = poll_subscriptions; existing != nullptr; existing = existing->internal.next) {
-        if (existing == sub) {
-            mutex_unlock(&poll_subscriptions_mutex.handle);
-            task_event_group_release_bit(event_group, bit);
-            return ERROR_INVALID_STATE;
-        }
+    // Avoid cyclic subscription list that would loop forever
+    if (poll_subscriptions == sub) {
+        mutex_unlock(&poll_subscriptions_mutex.handle);
+        task_event_group_release_bit(event_group, bit);
+        return ERROR_INVALID_STATE;
     }
 
     sub->internal.event_group = event_group;