Merge branch 'bugfix/esp_timer_stucks_into_esp_timer_impl_set_alarm_v3.1' into 'release/v3.1'

esp_timer/esp32: Fix esp_timer_impl_set_alarm() when CPU and APB freqs are changed (v3.1)

See merge request espressif/esp-idf!7444

components/esp32/esp_timer_esp32.c

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include "sys/param.h"
 #include "esp_err.h"
 #include "esp_timer.h"
 #include "esp_system.h"
@@ -78,7 +79,7 @@
 /* ALARM_OVERFLOW_VAL is used as timer alarm value when there are not timers
  * enabled which need to fire within the next timer overflow period. This alarm
  * is used to perform timekeeping (i.e. to track timer overflows).
- * Due to the 0xffffffff cannot recognize the real overflow or the scenario that 
+ * Due to the 0xffffffff cannot recognize the real overflow or the scenario that
  * ISR happens follow set_alarm, so change the ALARM_OVERFLOW_VAL to resolve this problem.
  * Set it to 0xefffffffUL. The remain 0x10000000UL(about 3 second) is enough to handle ISR.
  */
@@ -127,13 +128,6 @@ static uint32_t s_timer_us_per_overflow;
 // will not increment s_time_base_us if this flag is set.
 static bool s_mask_overflow;
 
-//The timer_overflow_happened read alarm register to tell if overflow happened.
-//However, there is a monent that overflow happens, and before ISR function called
-//alarm register is set to another value, then you call timer_overflow_happened, 
-//it will return false.
-//So we store the overflow value when new alarm is to be set. 
-static bool s_overflow_happened;
-
 #ifdef CONFIG_PM_DFS_USE_RTC_TIMER_REF
 // If DFS is enabled, upon the first frequency change this value is set to the
 // difference between esp_timer value and RTC timer value. On every subsequent
@@ -150,14 +144,10 @@ portMUX_TYPE s_time_update_lock = portMUX_INITIALIZER_UNLOCKED;
 #define TIMER_IS_AFTER_OVERFLOW(a) (ALARM_OVERFLOW_VAL < (a) && (a) <= FRC_TIMER_LOAD_VALUE(1))
 
 // Check if timer overflow has happened (but was not handled by ISR yet)
-static inline bool IRAM_ATTR timer_overflow_happened()
+static inline bool IRAM_ATTR timer_overflow_happened(void)
 {
-    if (s_overflow_happened) {
-        return true;
-    }
-
     return ((REG_READ(FRC_TIMER_CTRL_REG(1)) & FRC_TIMER_INT_STATUS) != 0 &&
-              ((REG_READ(FRC_TIMER_ALARM_REG(1)) == ALARM_OVERFLOW_VAL && TIMER_IS_AFTER_OVERFLOW(REG_READ(FRC_TIMER_COUNT_REG(1))) && !s_mask_overflow) || 
+              ((REG_READ(FRC_TIMER_ALARM_REG(1)) == ALARM_OVERFLOW_VAL && TIMER_IS_AFTER_OVERFLOW(REG_READ(FRC_TIMER_COUNT_REG(1))) && !s_mask_overflow) ||
                (!TIMER_IS_AFTER_OVERFLOW(REG_READ(FRC_TIMER_ALARM_REG(1))) && TIMER_IS_AFTER_OVERFLOW(REG_READ(FRC_TIMER_COUNT_REG(1))))));
 }
 
@@ -176,17 +166,17 @@ static inline void IRAM_ATTR timer_count_reload(void)
     REG_WRITE(FRC_TIMER_LOAD_REG(1), REG_READ(FRC_TIMER_COUNT_REG(1)) - ALARM_OVERFLOW_VAL);
 }
 
-void esp_timer_impl_lock()
+void esp_timer_impl_lock(void)
 {
     portENTER_CRITICAL(&s_time_update_lock);
 }
 
-void esp_timer_impl_unlock()
+void esp_timer_impl_unlock(void)
 {
     portEXIT_CRITICAL(&s_time_update_lock);
 }
 
-uint64_t IRAM_ATTR esp_timer_impl_get_time()
+uint64_t IRAM_ATTR esp_timer_impl_get_time(void)
 {
     uint32_t timer_val;
     uint64_t time_base;
@@ -201,7 +191,7 @@ uint64_t IRAM_ATTR esp_timer_impl_get_time()
         ticks_per_us = s_timer_ticks_per_us;
 
         /* Read them again and compare */
-        /* In this function, do not call timer_count_reload() when overflow is ture.
+        /* In this function, do not call timer_count_reload() when overflow is true.
          * Because there's remain count enough to allow FRC_TIMER_COUNT_REG grow
          */
         if (REG_READ(FRC_TIMER_COUNT_REG(1)) > timer_val &&
@@ -222,35 +212,47 @@ uint64_t IRAM_ATTR esp_timer_impl_get_time()
 void IRAM_ATTR esp_timer_impl_set_alarm(uint64_t timestamp)
 {
     portENTER_CRITICAL(&s_time_update_lock);
-    // Alarm time relative to the moment when counter was 0
-    uint64_t time_after_timebase_us = timestamp - s_time_base_us;
-    // Adjust current time if overflow has happened
-    bool overflow = timer_overflow_happened();
-    uint64_t cur_count = REG_READ(FRC_TIMER_COUNT_REG(1));
-
-    if (overflow) {
-        assert(time_after_timebase_us > s_timer_us_per_overflow);
-        time_after_timebase_us -= s_timer_us_per_overflow;
-        s_overflow_happened = true;
-    }
-    // Calculate desired timer compare value (may exceed 2^32-1)
-    uint64_t compare_val = time_after_timebase_us * s_timer_ticks_per_us;
-    uint32_t alarm_reg_val = ALARM_OVERFLOW_VAL;
     // Use calculated alarm value if it is less than ALARM_OVERFLOW_VAL.
     // Note that if by the time we update ALARM_REG, COUNT_REG value is higher,
     // interrupt will not happen for another ALARM_OVERFLOW_VAL timer ticks,
     // so need to check if alarm value is too close in the future (e.g. <2 us away).
-    const uint32_t offset = s_timer_ticks_per_us * 2;
+    int32_t offset = s_timer_ticks_per_us * 2;
-    if (compare_val < ALARM_OVERFLOW_VAL) {
+    do {
-        if (compare_val < cur_count + offset) {
+        // Adjust current time if overflow has happened
-            compare_val = cur_count + offset;
+        if (timer_overflow_happened() ||
-            if (compare_val > ALARM_OVERFLOW_VAL) {
+            ((REG_READ(FRC_TIMER_COUNT_REG(1)) > ALARM_OVERFLOW_VAL) &&
-                compare_val = ALARM_OVERFLOW_VAL;
+            ((REG_READ(FRC_TIMER_CTRL_REG(1)) & FRC_TIMER_INT_STATUS) == 0))) {
-            }
+            // 1. timer_overflow_happened() checks overflow with the interrupt flag.
+            // 2. During several loops, the counter can be higher than the alarm and even step over ALARM_OVERFLOW_VAL boundary (the interrupt flag is not set).
+            timer_count_reload();
+            s_time_base_us += s_timer_us_per_overflow;
         }
-        alarm_reg_val = (uint32_t) compare_val;
+        s_mask_overflow = false;
-    }
+        int64_t cur_count = REG_READ(FRC_TIMER_COUNT_REG(1));
-    REG_WRITE(FRC_TIMER_ALARM_REG(1), alarm_reg_val);
+        // Alarm time relative to the moment when counter was 0
+        int64_t time_after_timebase_us = (int64_t)timestamp - s_time_base_us;
+        // Calculate desired timer compare value (may exceed 2^32-1)
+        int64_t compare_val = time_after_timebase_us * s_timer_ticks_per_us;
+
+        compare_val = MAX(compare_val, cur_count + offset);
+        uint32_t alarm_reg_val = ALARM_OVERFLOW_VAL;
+        if (compare_val < ALARM_OVERFLOW_VAL) {
+            alarm_reg_val = (uint32_t) compare_val;
+        }
+        REG_WRITE(FRC_TIMER_ALARM_REG(1), alarm_reg_val);
+        int64_t delta = (int64_t)alarm_reg_val - (int64_t)REG_READ(FRC_TIMER_COUNT_REG(1));
+        if (delta <= 0) {
+            /*
+                When the timestamp is a bit less than the current counter then the alarm = current_counter + offset.
+                But due to CPU_freq in some case can be equal APB_freq the offset time can not exceed the overhead
+                (the alarm will be less than the counter) and it leads to the infinity loop.
+                To exclude this behavior to the offset was added the delta to have the opportunity to go through it.
+            */
+            offset += abs((int)delta) + s_timer_ticks_per_us * 2;
+        } else {
+            break;
+        }
+    } while (1);
     portEXIT_CRITICAL(&s_time_update_lock);
 }
 
@@ -261,7 +263,6 @@ static void IRAM_ATTR timer_alarm_isr(void *arg)
     if (timer_overflow_happened()) {
         timer_count_reload();
         s_time_base_us += s_timer_us_per_overflow;
-        s_overflow_happened = false;
     }
     s_mask_overflow = false;
     // Clear interrupt status
@@ -269,6 +270,17 @@ static void IRAM_ATTR timer_alarm_isr(void *arg)
     // Set alarm to the next overflow moment. Later, upper layer function may
     // call esp_timer_impl_set_alarm to change this to an earlier value.
     REG_WRITE(FRC_TIMER_ALARM_REG(1), ALARM_OVERFLOW_VAL);
+    if ((REG_READ(FRC_TIMER_COUNT_REG(1)) > ALARM_OVERFLOW_VAL) &&
+        ((REG_READ(FRC_TIMER_CTRL_REG(1)) & FRC_TIMER_INT_STATUS) == 0)) {
+        /*
+            This check excludes the case when the alarm can be less than the counter.
+            Without this check, it is possible because DPORT uses 4-lvl, and users can use the 5 Hi-interrupt,
+            they can interrupt this function between FRC_TIMER_INT_CLR and setting the alarm = ALARM_OVERFLOW_VAL
+            that lead to the counter will go ahead leaving the alarm behind.
+        */
+        timer_count_reload();
+        s_time_base_us += s_timer_us_per_overflow;
+    }
     portEXIT_CRITICAL_ISR(&s_time_update_lock);
     // Call the upper layer handler
     (*s_alarm_handler)(arg);
@@ -336,7 +348,6 @@ void esp_timer_impl_advance(int64_t time_us)
     REG_WRITE(FRC_TIMER_ALARM_REG(1), 0);
     REG_WRITE(FRC_TIMER_LOAD_REG(1), 0);
     s_time_base_us += count / s_timer_ticks_per_us + time_us;
-    s_overflow_happened = false;
     portEXIT_CRITICAL(&s_time_update_lock);
 }
 
@@ -371,7 +382,7 @@ esp_err_t esp_timer_impl_init(intr_handler_t alarm_handler)
     return ESP_OK;
 }
 
-void esp_timer_impl_deinit()
+void esp_timer_impl_deinit(void)
 {
     esp_intr_disable(s_timer_interrupt_handle);
 
@@ -386,7 +397,7 @@ void esp_timer_impl_deinit()
 // FIXME: This value is safe for 80MHz APB frequency.
 // Should be modified to depend on clock frequency.
 
-uint64_t IRAM_ATTR esp_timer_impl_get_min_period_us()
+uint64_t IRAM_ATTR esp_timer_impl_get_min_period_us(void)
 {
     return 50;
 }

components/esp32/esp_timer_impl.h

@@ -38,7 +38,7 @@ esp_err_t esp_timer_impl_init(intr_handler_t alarm_handler);
 /**
  * @brief Deinitialize platform specific layer of esp_timer
  */
-void esp_timer_impl_deinit();
+void esp_timer_impl_deinit(void);
 
 /**
  * @brief Set up the timer interrupt to fire at a particular time
@@ -73,7 +73,7 @@ void esp_timer_impl_advance(int64_t time_us);
  * @brief Get time, in microseconds, since esp_timer_impl_init was called
  * @return timestamp in microseconds
  */
-uint64_t esp_timer_impl_get_time();
+uint64_t esp_timer_impl_get_time(void);
 
 /**
  * @brief Get minimal timer period, in microseconds
@@ -83,7 +83,7 @@ uint64_t esp_timer_impl_get_time();
  * callback, preventing other tasks from running.
  * @return minimal period of periodic timer, in microseconds
  */
-uint64_t esp_timer_impl_get_min_period_us();
+uint64_t esp_timer_impl_get_min_period_us(void);
 
 /**
  * @brief obtain internal critical section used esp_timer implementation
@@ -92,10 +92,10 @@ uint64_t esp_timer_impl_get_min_period_us();
  * the calls. Should be treated in the same way as a spinlock.
  * Call esp_timer_impl_unlock to release the lock
  */
-void esp_timer_impl_lock();
+void esp_timer_impl_lock(void);
 
 
 /**
  * @brief counterpart of esp_timer_impl_lock
  */
-void esp_timer_impl_unlock();
+void esp_timer_impl_unlock(void);

components/esp32/test/test_esp_timer.c

@@ -4,6 +4,7 @@
 #include <sys/time.h>
 #include <sys/param.h>
 #include "unity.h"
+#include "soc/frc_timer_reg.h"
 #include "esp_timer.h"
 #include "esp_heap_caps.h"
 #include "freertos/FreeRTOS.h"
@@ -594,3 +595,14 @@ TEST_CASE("after esp_timer_impl_advance, timers run when expected", "[esp_timer]
 
     ref_clock_deinit();
 }
+
+
+TEST_CASE("Test esp_timer_impl_set_alarm when the counter is near an overflow value", "[esp_timer]")
+{
+    for (int i = 0; i < 1024; ++i) {
+        uint32_t count_reg = 0xeffffe00 + i;
+        REG_WRITE(FRC_TIMER_LOAD_REG(1), count_reg);
+        printf("%d) count_reg = 0x%x\n", i, count_reg);
+        esp_timer_impl_set_alarm(1); // timestamp is expired
+    }
+}