Merge branch 'bugfix/esp_timer_delete_from_cb' into 'master'

esp_timer: handle esp_timer_delete in timer task

Closes IDFGH-1143

See merge request idf/esp-idf!5037

components/esp32/test/test_esp_timer.c

@@ -536,6 +536,61 @@ TEST_CASE("Can delete timer from callback", "[esp_timer]")
     vSemaphoreDelete(args.notify_from_timer_cb);
 }
 
+
+typedef struct {
+    SemaphoreHandle_t delete_start;
+    SemaphoreHandle_t delete_done;
+    SemaphoreHandle_t test_done;
+    esp_timer_handle_t timer;
+} timer_delete_test_args_t;
+
+static void timer_delete_task(void* arg)
+{
+    timer_delete_test_args_t* args = (timer_delete_test_args_t*) arg;
+    xSemaphoreTake(args->delete_start, portMAX_DELAY);
+    printf("Deleting the timer\n");
+    esp_timer_delete(args->timer);
+    printf("Timer deleted\n");
+    xSemaphoreGive(args->delete_done);
+    vTaskDelete(NULL);
+}
+
+static void timer_delete_test_callback(void* arg)
+{
+    timer_delete_test_args_t* args = (timer_delete_test_args_t*) arg;
+    printf("Timer callback called\n");
+    xSemaphoreGive(args->delete_start);
+    xSemaphoreTake(args->delete_done, portMAX_DELAY);
+    printf("Callback complete\n");
+    xSemaphoreGive(args->test_done);
+}
+
+TEST_CASE("Can delete timer from a separate task, triggered from callback", "[esp_timer]")
+{
+    timer_delete_test_args_t args = {
+        .delete_start = xSemaphoreCreateBinary(),
+        .delete_done = xSemaphoreCreateBinary(),
+        .test_done = xSemaphoreCreateBinary(),
+    };
+
+    esp_timer_create_args_t timer_args = {
+            .callback = &timer_delete_test_callback,
+            .arg = &args
+    };
+    esp_timer_handle_t timer;
+    TEST_ESP_OK(esp_timer_create(&timer_args, &timer));
+    args.timer = timer;
+
+    xTaskCreate(timer_delete_task, "deleter", 4096, &args, 5, NULL);
+
+    esp_timer_start_once(timer, 100);
+    TEST_ASSERT(xSemaphoreTake(args.test_done, pdMS_TO_TICKS(1000)));
+
+    vSemaphoreDelete(args.delete_done);
+    vSemaphoreDelete(args.delete_start);
+    vSemaphoreDelete(args.test_done);
+}
+
 TEST_CASE("esp_timer_impl_advance moves time base correctly", "[esp_timer]")
 {
     ref_clock_init();

components/esp_common/src/esp_timer.c

@@ -39,12 +39,17 @@
 #endif
 #include "sys/queue.h"
 
+#define EVENT_ID_DELETE_TIMER   0xF0DE1E1E
+
 #define TIMER_EVENT_QUEUE_SIZE      16
 
 struct esp_timer {
     uint64_t alarm;
     uint64_t period;
-    esp_timer_cb_t callback;
+    union {
+        esp_timer_cb_t callback;
+        uint32_t event_id;
+    };
     void* arg;
 #if WITH_PROFILING
     const char* name;
@@ -77,23 +82,18 @@ static LIST_HEAD(esp_timer_list, esp_timer) s_timers =
 // all the timers
 static LIST_HEAD(esp_inactive_timer_list, esp_timer) s_inactive_timers =
         LIST_HEAD_INITIALIZER(s_timers);
-// used to keep track of the timer when executing the callback
-static esp_timer_handle_t s_timer_in_callback;
 #endif
 // task used to dispatch timer callbacks
 static TaskHandle_t s_timer_task;
 // counting semaphore used to notify the timer task from ISR
 static SemaphoreHandle_t s_timer_semaphore;
-// mutex which protects timers from deletion during callback execution
-static SemaphoreHandle_t s_timer_delete_mutex;
 
 #if CONFIG_SPIRAM_USE_MALLOC
-// memory for s_timer_semaphore and s_timer_delete_mutex
+// memory for s_timer_semaphore
 static StaticQueue_t s_timer_semaphore_memory;
-static StaticQueue_t s_timer_delete_mutex_memory;
 #endif
 
-// lock protecting s_timers, s_inactive_timers, s_timer_in_callback
+// lock protecting s_timers, s_inactive_timers
 static portMUX_TYPE s_timer_lock = portMUX_INITIALIZER_UNLOCKED;
 
 
@@ -164,15 +164,10 @@ esp_err_t esp_timer_delete(esp_timer_handle_t timer)
     if (timer_armed(timer)) {
         return ESP_ERR_INVALID_STATE;
     }
-    xSemaphoreTakeRecursive(s_timer_delete_mutex, portMAX_DELAY);
-#if WITH_PROFILING
-    if (timer == s_timer_in_callback) {
-        s_timer_in_callback = NULL;
-    }
-    timer_remove_inactive(timer);
-#endif
-    free(timer);
-    xSemaphoreGiveRecursive(s_timer_delete_mutex);
+    timer->event_id = EVENT_ID_DELETE_TIMER;
+    timer->alarm = esp_timer_get_time() + 50;
+    timer->period = 0;
+    timer_insert(timer);
     return ESP_OK;
 }
 
@@ -267,13 +262,17 @@ static void timer_process_alarm(esp_timer_dispatch_t dispatch_method)
     /* unused, provision to allow running callbacks from ISR */
     (void) dispatch_method;
 
-    xSemaphoreTakeRecursive(s_timer_delete_mutex, portMAX_DELAY);
     timer_list_lock();
     uint64_t now = esp_timer_impl_get_time();
     esp_timer_handle_t it = LIST_FIRST(&s_timers);
     while (it != NULL &&
             it->alarm < now) {
         LIST_REMOVE(it, list_entry);
+        if (it->event_id == EVENT_ID_DELETE_TIMER) {
+            free(it);
+            it = LIST_FIRST(&s_timers);
+            continue;
+        }
         if (it->period > 0) {
             it->alarm += it->period;
             timer_insert(it);
@@ -285,21 +284,14 @@ static void timer_process_alarm(esp_timer_dispatch_t dispatch_method)
         }
 #if WITH_PROFILING
         uint64_t callback_start = now;
-        s_timer_in_callback = it;
 #endif
         timer_list_unlock();
         (*it->callback)(it->arg);
         timer_list_lock();
         now = esp_timer_impl_get_time();
 #if WITH_PROFILING
-        /* The callback might have deleted the timer.
-         * If this happens, esp_timer_delete will set s_timer_in_callback
-         * to NULL.
-         */
-        if (s_timer_in_callback) {
-            s_timer_in_callback->times_triggered++;
-            s_timer_in_callback->total_callback_run_time += now - callback_start;
-        }
+        it->times_triggered++;
+        it->total_callback_run_time += now - callback_start;
 #endif
         it = LIST_FIRST(&s_timers);
     }
@@ -308,7 +300,6 @@ static void timer_process_alarm(esp_timer_dispatch_t dispatch_method)
         esp_timer_impl_set_alarm(first->alarm);
     }
     timer_list_unlock();
-    xSemaphoreGiveRecursive(s_timer_delete_mutex);
 }
 
 static void timer_task(void* arg)
@@ -356,18 +347,6 @@ esp_err_t esp_timer_init(void)
         goto out;
     }
 
-#if CONFIG_SPIRAM_USE_MALLOC
-    memset(&s_timer_delete_mutex_memory, 0, sizeof(StaticQueue_t));
-    s_timer_delete_mutex = xSemaphoreCreateRecursiveMutexStatic(&s_timer_delete_mutex_memory);
-#else
-    s_timer_delete_mutex = xSemaphoreCreateRecursiveMutex();
-#endif
-    if (!s_timer_delete_mutex) {
-        err = ESP_ERR_NO_MEM;
-        goto out;
-    }
-
-
     int ret = xTaskCreatePinnedToCore(&timer_task, "esp_timer",
             ESP_TASK_TIMER_STACK, NULL, ESP_TASK_TIMER_PRIO, &s_timer_task, PRO_CPU_NUM);
     if (ret != pdPASS) {
@@ -391,10 +370,6 @@ out:
         vSemaphoreDelete(s_timer_semaphore);
         s_timer_semaphore = NULL;
     }
-    if (s_timer_delete_mutex) {
-        vSemaphoreDelete(s_timer_delete_mutex);
-        s_timer_delete_mutex = NULL;
-    }
     return ESP_ERR_NO_MEM;
 }