From 486ed20d326905799d788d9e5b9ce7479a4ce81f Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Mon, 13 Jan 2020 14:36:38 +0800 Subject: [PATCH] esp_timer/esp32: Fix case when alarm_reg > counter_reg but FRC_TIMER_INT_STATUS is not set (fast forward) --- components/esp32/esp_timer_esp32.c | 103 ++++++++++++++----------- components/esp32/esp_timer_impl.h | 10 +-- components/esp32/test/test_esp_timer.c | 12 +++ 3 files changed, 74 insertions(+), 51 deletions(-) diff --git a/components/esp32/esp_timer_esp32.c b/components/esp32/esp_timer_esp32.c index b6315ca76..a85f01efd 100644 --- a/components/esp32/esp_timer_esp32.c +++ b/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; - if (compare_val < ALARM_OVERFLOW_VAL) { - if (compare_val < cur_count + offset) { - compare_val = cur_count + offset; - if (compare_val > ALARM_OVERFLOW_VAL) { - compare_val = ALARM_OVERFLOW_VAL; - } + int32_t offset = s_timer_ticks_per_us * 2; + do { + // Adjust current time if overflow has happened + if (timer_overflow_happened() || + ((REG_READ(FRC_TIMER_COUNT_REG(1)) > 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; - } - REG_WRITE(FRC_TIMER_ALARM_REG(1), alarm_reg_val); + s_mask_overflow = false; + int64_t cur_count = REG_READ(FRC_TIMER_COUNT_REG(1)); + // 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; } diff --git a/components/esp32/esp_timer_impl.h b/components/esp32/esp_timer_impl.h index 5871428a5..a19bde521 100644 --- a/components/esp32/esp_timer_impl.h +++ b/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); diff --git a/components/esp32/test/test_esp_timer.c b/components/esp32/test/test_esp_timer.c index 510bed461..180d36b12 100644 --- a/components/esp32/test/test_esp_timer.c +++ b/components/esp32/test/test_esp_timer.c @@ -4,6 +4,7 @@ #include #include #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 + } +} \ No newline at end of file