Fix deinit of async_contet_freertos. improve API docs for deinit. Add todo for bugs in threadsafe_background

src/rp2_common/pico_async_context/async_context_freertos.c

diff --git a/src/rp2_common/pico_async_context/async_context_freertos.c b/src/rp2_common/pico_async_context/async_context_freertos.c
index 02c688e..aff70e8 100644
--- a/src/rp2_common/pico_async_context/async_context_freertos.c
+++ b/src/rp2_common/pico_async_context/async_context_freertos.c
@@ -74,21 +74,32 @@
         __sev(); // it is possible regular code is waiting on a WFE on the other core
     } while (!self->task_should_exit);
     xSemaphoreGive(self->task_complete_sem);
-    vTaskDelete(NULL);
+    // Previously we called vTaskDelete(NULL) here, however this (self-delete) is always asynchronous and deferred
+    // to the idle task, which is a problem with static allocation, as async_context_freertos_deinit zeroes
+    // the context which actually contains the TCB, thus stomping on FreeRTOS's own internal (active) data structures,
+    // before the idle task gets a chance to clean it up. async_context_freertos_deinit must therefore be responsible
+    // for destroying this task in that case (and indeed it is simpler to do so there in all cases to make sure
+    // the timer is quiesced too)
+
+    // We must however do something other than return from a task, so we suspend ourself. Note however, that
+    // there is no guarantee this code actually executes before async_context_freertos_deinit proceeds since
+    // we have posted it a semaphore above, so the de-init code must not rely on this having happened.
+    vTaskSuspend(NULL);
 }
 
 static void async_context_freertos_wake_up(async_context_t *self_base) {
     async_context_freertos_t *self = (async_context_freertos_t *)self_base;
-    if (self->task_handle) {
+    TaskHandle_t task_handle = self->task_handle;
+    if (task_handle) {
         if (portCHECK_IF_IN_ISR()) {
-            vTaskNotifyGiveFromISR(self->task_handle, NULL);
+            vTaskNotifyGiveFromISR(task_handle, NULL);
             xSemaphoreGiveFromISR(self->work_needed_sem, NULL);
         } else {
             // we don't want to wake ourselves up (we will only ever be called
             // from the async_context_task if we own the lock, in which case processing
             // will already happen when the lock is finally unlocked
-            if (xTaskGetCurrentTaskHandle() != self->task_handle) {
-                xTaskNotifyGive(self->task_handle);
+            if (xTaskGetCurrentTaskHandle() != task_handle) {
+                xTaskNotifyGive(task_handle);
                 xSemaphoreGive(self->work_needed_sem);
             } else {
 #ifndef NDEBUG
@@ -195,23 +206,54 @@
 
 void async_context_freertos_deinit(async_context_t *self_base) {
     async_context_freertos_t *self = (async_context_freertos_t *)self_base;
-    if (self->task_handle) {
+    TaskHandle_t task_handle = self->task_handle;
+    // Ask the task to exit its loop, and wait for it to do so.
+    if (task_handle) {
         async_context_execute_sync(self_base, end_task_func, self_base);
         if (self->task_complete_sem) {
             xSemaphoreTake(self->task_complete_sem, portMAX_DELAY);
         }
     }
+    // Now the task has exited its loop, it will neither call worker functions nor re-arm
+    // the timer. At this point though both:
+    // a. The task may still be running on core (having not yet made it to the vTaskSuspend(NULL).
+    // b. Be targeted for notifications by the timer which is still active. This is fine though
+    //    as the task either is, or is about to be in vTaskSuspend(NULL) which isn't woken by notifications.
+
+    // First things first, let's now synchronously stop the timer...
     if (self->timer_handle) {
         xTimerDelete(self->timer_handle, 0);
 
-        // slight hoops to jump thru to make sure the timer is actually deleted before we
-        // free the remaining items below. this is needed for SMP and also if
-        // the current task has a higher/equal priority to the timer task
+        // Slight hoops to jump thru to make sure the timer has actually been deleted BEFORE we proceed
 
-        // 1. queue function which will notify us back to the timer task queue
+        // 1. Queue function which will notify us back to the timer task queue
         xTimerPendFunctionCall(timer_delete_sync_helper, (void *)xTaskGetCurrentTaskHandle(), 0, portMAX_DELAY);
-        // 2. wait for that function to execute (which will be after the timer deletion completes)
+        // 2. Wait for that function to execute (which will be after the timer deletion completes)
         ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
+        self->timer_handle = NULL;
+    }
+
+    // With the timer now stopped, there are no other current/future execution units referencing self->task_handle
+    // and we can proceed to delete the task...
+    if (task_handle) {
+        // ... however vTaskDelete is only synchronous if the task is not currently executing (on core) during the call
+
+        // 1. We don't care if not using static allocation, since nothing related to the task is stored in our
+        //    soon to be zeroed context.
+        // 2. We don't care if not using SMP since if the only core is currently executing vTaskDelete from this task,
+        //   then clearly the other task is not currently executing
+#if configSUPPORT_STATIC_ALLOCATION && ( configNUMBER_OF_CORES > 1 )
+        // Make sure the task cannot be re-scheduled again. This is asynchronous across cores...
+        vTaskSuspend(task_handle);
+        // ... so actually wait until it actually leaves the core if it was on it
+        while (xTaskGetCurrentTaskHandleForCore((BaseType_t)self->core.core_num) == task_handle) {
+            taskYIELD();
+        }
+#endif
+        // Now we can call vTaskDelete because in the static allocation case we know the task is
+        // no longer executing, so this call is guaranteed to complete synchronously, and in the non
+        // static allocation case we don't care if the call is asynchronous anyway.
+        vTaskDelete(task_handle);
     }
     if (self->lock_mutex) {
         vSemaphoreDelete(self->lock_mutex);
@@ -222,6 +264,7 @@
     if (self->task_complete_sem) {
         vSemaphoreDelete(self->task_complete_sem);
     }
+    // Finally clear the context now we know nothing is referencing it.
     memset(self, 0, sizeof(*self));
 }
 

src/rp2_common/pico_async_context/async_context_threadsafe_background.c

diff --git a/src/rp2_common/pico_async_context/async_context_threadsafe_background.c b/src/rp2_common/pico_async_context/async_context_threadsafe_background.c
index 50cb4ef..b90d1a4 100644
--- a/src/rp2_common/pico_async_context/async_context_threadsafe_background.c
+++ b/src/rp2_common/pico_async_context/async_context_threadsafe_background.c
@@ -233,10 +233,24 @@
 
 static void async_context_threadsafe_background_deinit(async_context_t *self_base) {
     async_context_threadsafe_background_t *self = (async_context_threadsafe_background_t *)self_base;
-    // todo we do not currently handle this correctly; we could, but seems like a rare case
+    // todo we do not currently handle this correctly; we could, but it seems like a rare use case
     assert(get_core_num() == self_base->core_num);
+
+    // todo this cleanup orderd is incorrect, because:
+    //      - low_priority_irq_handler() -> process_under_lock() re-arms the alarm
+    //      - alarm_handler() -> wake_up() set ths irq pending
+    //
+    //      i.e. there is no ordering of these two calls that is correct, and they must be split
+    //
+
     low_prio_irq_deinit(self);
+
+    // alarm_pool_cancel_alarm() is asynchronous when called from a different core to the pool owner,
+    // so we rely on the fact that the alarm_pool core is the same as our core (which is asserted on during init)
+    // to make this synchronous
     if (self->alarm_id > 0) alarm_pool_cancel_alarm(self->alarm_pool, self->alarm_id);
+
+    // todo also force_alarm_id is never cancelled, which is a leak if we dont own the alarm pool
 #if ASYNC_CONTEXT_THREADSAFE_BACKGROUND_MULTI_CORE
     if (self->alarm_pool_owned) {
         alarm_pool_destroy(self->alarm_pool);
@@ -373,5 +387,3 @@
         .wait_for_work_until = async_context_threadsafe_background_wait_for_work_until,
         .deinit = async_context_threadsafe_background_deinit,
 };
-
-

src/rp2_common/pico_async_context/include/pico/async_context.h

diff --git a/src/rp2_common/pico_async_context/include/pico/async_context.h b/src/rp2_common/pico_async_context/include/pico/async_context.h
index f544147..5ffac91 100644
--- a/src/rp2_common/pico_async_context/include/pico/async_context.h
+++ b/src/rp2_common/pico_async_context/include/pico/async_context.h
@@ -449,14 +449,16 @@
 }
 
 /*!
- * \brief End async_context processing, and free any resources
+ * \brief End async_context processing, and free any resources owned by the context
  * \ingroup pico_async_context
  *
- * \note The user should clean up any resources associated with workers
- * in the async_context themselves.
+ * It is the callers responsibility to ensure that no external async_context_ calls
+ * to this context (i.e. those not originating as a direct synchronous result of active
+ * pending/at_time work within this context) are made after (or concurrently with) calling this method.
  *
- * Asynchronous (non-polled) async_contexts guarantee that no
- * callback is being called once this method returns.
+ * The context, in turn, guarantees that on return, no active calls are being made, and no
+ * subsequent calls will be made, to any workers registered in this context
+ * so the caller can free any resources associated with those workers.
  *
  * \param context the async_context
  */