Merge branch 'fix/esp_timer_task_dispatch_wedge_v6.1' into 'release/v6.1'

fix(esp_timer): Fix esp_timer task dispatch stall (v6.1)

See merge request espressif/esp-idf!51610

components/esp_timer/private_include/esp_timer_impl.h

@@ -142,9 +142,18 @@ void esp_timer_impl_init_system_time(void);
 
 #if CONFIG_ESP_TIMER_SUPPORTS_ISR_DISPATCH_METHOD
 /**
- * @brief Set the next alarm if there is such an alarm in the cached array.
+ * @brief Check which alarm slots have fired and disarm them.
  *
- * @note Available only when CONFIG_ESP_TIMER_SUPPORTS_ISR_DISPATCH_METHOD is enabled.
+ * For each dispatch method (TASK and ISR), checks if the cached alarm deadline
+ * has expired. If so, clears the deadline (resets to UINT64_MAX) and sets the
+ * corresponding bit in the returned mask. After processing both slots,
+ * reprograms the hardware alarm to the earliest remaining cached deadline.
+ *
+ * Must be called only from ISR context.
+ *
+ * @return Bitmask of the dispatch methods whose alarm was due:
+ *         bit 0 set means the TASK-dispatch alarm fired,
+ *         bit 1 set means the ISR-dispatch alarm fired.
  */
-void esp_timer_impl_try_to_set_next_alarm(void);
+uint32_t esp_timer_impl_claim_due_alarms(void);
 #endif

components/esp_timer/src/esp_timer.c

@@ -482,13 +482,11 @@ static ESP_TIMER_IRAM_ATTR void timer_list_unlock(esp_timer_dispatch_t timer_typ
 }
 
 #ifdef CONFIG_ESP_TIMER_SUPPORTS_ISR_DISPATCH_METHOD
-static ESP_TIMER_IRAM_ATTR bool timer_process_alarm(esp_timer_dispatch_t dispatch_method)
-#else
-static bool timer_process_alarm(esp_timer_dispatch_t dispatch_method)
+ESP_TIMER_IRAM_ATTR
 #endif
+static void timer_process_alarm(esp_timer_dispatch_t dispatch_method)
 {
     timer_list_lock(dispatch_method);
-    bool processed = false;
     esp_timer_handle_t it;
     while (1) {
         it = LIST_FIRST(&s_timers[dispatch_method]);
@@ -498,7 +496,6 @@ static bool timer_process_alarm(esp_timer_dispatch_t dispatch_method)
             break;
         }
         ESP_COMPILER_DIAGNOSTIC_POP("-Wanalyzer-use-after-free")
-        processed = true;
         LIST_REMOVE(it, list_entry);
         if (it->event_id == EVENT_ID_DELETE_TIMER) {
             // It is handled only by ESP_TIMER_TASK (see esp_timer_delete()).
@@ -540,17 +537,9 @@ static bool timer_process_alarm(esp_timer_dispatch_t dispatch_method)
 #endif
         }
     } // while(1)
-    if (it) {
-        if (dispatch_method == ESP_TIMER_TASK || (dispatch_method != ESP_TIMER_TASK && processed == true)) {
-            esp_timer_impl_set_alarm_id(it->alarm, dispatch_method);
-        }
-    } else {
-        if (processed) {
-            esp_timer_impl_set_alarm_id(UINT64_MAX, dispatch_method);
-        }
-    }
+    uint64_t next_alarm = (it != NULL) ? it->alarm : UINT64_MAX;
+    esp_timer_impl_set_alarm_id(next_alarm, dispatch_method);
     timer_list_unlock(dispatch_method);
-    return processed;
 }
 
 static void timer_task(void* arg)
@@ -568,24 +557,30 @@ ESP_TIMER_IRAM_ATTR void esp_timer_isr_dispatch_need_yield(void)
     assert(xPortInIsrContext());
     s_isr_dispatch_need_yield = pdTRUE;
 }
+
 #endif
 
 static void ESP_TIMER_IRAM_ATTR timer_alarm_handler(void* arg)
 {
     BaseType_t xHigherPriorityTaskWoken = pdFALSE;
-    bool isr_timers_processed = false;
 
 #ifdef CONFIG_ESP_TIMER_SUPPORTS_ISR_DISPATCH_METHOD
-    esp_timer_impl_try_to_set_next_alarm();
-    // process timers with ISR dispatch method
-    isr_timers_processed = timer_process_alarm(ESP_TIMER_ISR);
+    uint32_t due_alarm_mask = esp_timer_impl_claim_due_alarms();
+
+    if (due_alarm_mask & (1U << ESP_TIMER_ISR)) {
+        timer_process_alarm(ESP_TIMER_ISR);
+    }
+
     xHigherPriorityTaskWoken = s_isr_dispatch_need_yield;
     s_isr_dispatch_need_yield = pdFALSE;
-#endif
 
-    if (isr_timers_processed == false) {
+    if (due_alarm_mask & (1U << ESP_TIMER_TASK)) {
         vTaskNotifyGiveFromISR(s_timer_task, &xHigherPriorityTaskWoken);
     }
+#else
+    vTaskNotifyGiveFromISR(s_timer_task, &xHigherPriorityTaskWoken);
+#endif
+
     if (xHigherPriorityTaskWoken == pdTRUE) {
         portYIELD_FROM_ISR();
     }

components/esp_timer/src/esp_timer_impl_common.c

@@ -38,29 +38,28 @@ void ESP_TIMER_IRAM_ATTR esp_timer_impl_set_alarm(uint64_t timestamp)
 }
 
 #ifdef CONFIG_ESP_TIMER_SUPPORTS_ISR_DISPATCH_METHOD
-void ESP_TIMER_IRAM_ATTR esp_timer_impl_try_to_set_next_alarm(void)
+/* Reprogram the hardware alarm to the nearest pending deadline,
+ * i.e. MIN(timestamp_id[0], timestamp_id[1]). The timestamp_id[] cache is not modified.
+ * Must be called with s_time_update_lock held. */
+static inline void ESP_TIMER_IRAM_ATTR esp_timer_impl_rearm_alarm(void)
+{
+    esp_timer_impl_set_alarm_id(timestamp_id[ESP_TIMER_TASK], ESP_TIMER_TASK);
+}
+
+uint32_t ESP_TIMER_IRAM_ATTR esp_timer_impl_claim_due_alarms(void)
 {
     portENTER_CRITICAL_ISR(&s_time_update_lock);
-    unsigned now_alarm_idx;  // ISR is called due to this current alarm
-    unsigned next_alarm_idx; // The following alarm after now_alarm_idx
-    if (timestamp_id[0] < timestamp_id[1]) {
-        now_alarm_idx = 0;
-        next_alarm_idx = 1;
-    } else {
-        now_alarm_idx = 1;
-        next_alarm_idx = 0;
-    }
-
-    if (timestamp_id[next_alarm_idx] != UINT64_MAX) {
-        // The following alarm is valid and can be used.
-        // Remove the current alarm from consideration.
-        esp_timer_impl_set_alarm_id(UINT64_MAX, now_alarm_idx);
-    } else {
-        // There is no the following alarm.
-        // Remove the current alarm from consideration as well.
-        timestamp_id[now_alarm_idx] = UINT64_MAX;
+    uint64_t now = esp_timer_impl_get_time();
+    uint32_t due_mask = 0;
+    for (unsigned alarm_id = 0; alarm_id < sizeof(timestamp_id) / sizeof(timestamp_id[0]); ++alarm_id) {
+        if (timestamp_id[alarm_id] <= now) {
+            due_mask |= 1U << alarm_id;
+            timestamp_id[alarm_id] = UINT64_MAX;
+        }
     }
+    esp_timer_impl_rearm_alarm();
     portEXIT_CRITICAL_ISR(&s_time_update_lock);
+    return due_mask;
 }
 #endif
 

components/esp_timer/test_apps/main/test_esp_timer.c

@@ -22,6 +22,7 @@
 #include "test_utils.h"
 #include "esp_freertos_hooks.h"
 #include "esp_rom_sys.h"
+#include "esp_task_wdt.h"
 /* include performance pass standards header file */
 #include "esp_timer_performance.h"
 
@@ -874,6 +875,7 @@ TEST_CASE("esp_timer_impl_set_alarm and using start_once do not lead that the Sy
 
 #endif // !defined(CONFIG_FREERTOS_UNICORE) && SOC_DPORT_WORKAROUND
 
+#ifdef CONFIG_IDF_TARGET_ESP32
 TEST_CASE("Test case when esp_timer_impl_set_alarm needs set timer < now_time", "[esp_timer]")
 {
     esp_timer_impl_advance(50331648); // 0xefffffff/80 = 50331647
@@ -891,6 +893,7 @@ TEST_CASE("Test case when esp_timer_impl_set_alarm needs set timer < now_time",
     printf("alarm_reg = 0x%llx, count_reg 0x%llx\n", alarm_reg, count_reg);
     TEST_ASSERT(alarm_reg <= (count_reg + offset));
 }
+#endif // CONFIG_IDF_TARGET_ESP32
 
 static void timer_callback5(void* arg)
 {
@@ -1428,6 +1431,76 @@ TEST_CASE("Test ISR dispatch callbacks are not blocked even if TASK callbacks ta
     vTaskDelay(3); // wait for the esp_timer task to delete all timers
 }
 
+static volatile uint32_t task_timer_count;
+static volatile uint32_t isr_timer_count;
+
+static void task_timer_count_cb(void *arg)
+{
+    task_timer_count++;
+}
+
+static void IRAM_ATTR isr_timer_count_cb(void *arg)
+{
+    isr_timer_count++;
+}
+
+#define NUM_ISR_TIMERS 5
+
+TEST_CASE("TASK dispatch timers not stalled by ISR dispatch timers on shared alarm", "[esp_timer][isr_dispatch][timeout=120]")
+{
+    task_timer_count = 0;
+    isr_timer_count = 0;
+
+    esp_timer_handle_t task_timer;
+    esp_timer_handle_t isr_timers[NUM_ISR_TIMERS];
+
+    const esp_timer_create_args_t task_args = {
+        .callback = task_timer_count_cb,
+        .dispatch_method = ESP_TIMER_TASK,
+        .name = "task_timer",
+    };
+
+    TEST_ESP_OK(esp_timer_create(&task_args, &task_timer));
+
+    for (int i = 0; i < NUM_ISR_TIMERS; i++) {
+        const esp_timer_create_args_t isr_args = {
+            .callback = isr_timer_count_cb,
+            .dispatch_method = ESP_TIMER_ISR,
+            .name = "isr_timer",
+        };
+        TEST_ESP_OK(esp_timer_create(&isr_args, &isr_timers[i]));
+    }
+
+    /* One TASK timer at 1s. Multiple ISR timers at harmonic periods that
+     * frequently collide with the TASK alarm (1s, 500ms, 333ms, 250ms, 200ms).
+     * This maximizes the shared-alarm race window that triggers the stall. */
+    TEST_ESP_OK(esp_timer_start_periodic(task_timer, 1 * SEC));
+    TEST_ESP_OK(esp_timer_start_periodic(isr_timers[0], 1 * SEC));
+    TEST_ESP_OK(esp_timer_start_periodic(isr_timers[1], SEC / 2));
+    TEST_ESP_OK(esp_timer_start_periodic(isr_timers[2], SEC / 3));
+    TEST_ESP_OK(esp_timer_start_periodic(isr_timers[3], SEC / 4));
+    TEST_ESP_OK(esp_timer_start_periodic(isr_timers[4], SEC / 5));
+
+    for (int i = 0; i < 20; i++) {
+        uint32_t prev_task = task_timer_count;
+        vTaskDelay(pdMS_TO_TICKS(5000));
+        esp_task_wdt_reset();
+        uint32_t cur_task = task_timer_count;
+        uint32_t cur_isr = isr_timer_count;
+        printf("check %d/20: task_timer_count=%" PRIu32 " isr_timer_count=%" PRIu32 "\n",
+               i + 1, cur_task, cur_isr);
+        // Verify TASK-dispatch callback is still being invoked (not stalled)
+        TEST_ASSERT_GREATER_THAN_UINT32(prev_task, cur_task);
+    }
+
+    TEST_ESP_OK(esp_timer_stop(task_timer));
+    TEST_ESP_OK(esp_timer_delete(task_timer));
+    for (int i = 0; i < NUM_ISR_TIMERS; i++) {
+        TEST_ESP_OK(esp_timer_stop(isr_timers[i]));
+        TEST_ESP_OK(esp_timer_delete(isr_timers[i]));
+    }
+}
+
 #endif // CONFIG_ESP_TIMER_SUPPORTS_ISR_DISPATCH_METHOD
 
 typedef struct {