From 6b08e8b44966fadba83e09cbc47f6f86bac03e51 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 20 May 2019 19:07:28 +0800 Subject: [PATCH] esp_timer: handle esp_timer_delete in timer task Closes https://github.com/espressif/esp-idf/issues/3458 --- components/esp32/test/test_esp_timer.c | 55 ++++++++++++++++++++++ components/esp_common/src/esp_timer.c | 63 ++++++++------------------ 2 files changed, 74 insertions(+), 44 deletions(-) diff --git a/components/esp32/test/test_esp_timer.c b/components/esp32/test/test_esp_timer.c index b46cca560..e3444503a 100644 --- a/components/esp32/test/test_esp_timer.c +++ b/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(); diff --git a/components/esp_common/src/esp_timer.c b/components/esp_common/src/esp_timer.c index 616cc2545..82a2d0b85 100644 --- a/components/esp_common/src/esp_timer.c +++ b/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; }