From d80fae2c88dedafbc5e9080a80bcad58712fcbea Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Thu, 5 Sep 2019 20:15:37 +0800 Subject: [PATCH 1/2] esp32: Add UTs to check the System time does not jump back --- components/esp32/test/test_esp_timer.c | 130 +++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/components/esp32/test/test_esp_timer.c b/components/esp32/test/test_esp_timer.c index 52265fb82..4512f42b8 100644 --- a/components/esp32/test/test_esp_timer.c +++ b/components/esp32/test/test_esp_timer.c @@ -649,3 +649,133 @@ TEST_CASE("after esp_timer_impl_advance, timers run when expected", "[esp_timer] ref_clock_deinit(); } + +#if !defined(CONFIG_FREERTOS_UNICORE) && defined(CONFIG_ESP32_DPORT_WORKAROUND) + +#include "soc/dport_reg.h" +#include "soc/frc_timer_reg.h" +#include "esp_ipc.h" +static bool task_stop; +static bool time_jumped; + +static void task_check_time(void *p) +{ + int64_t t1 = 0, t2 = 0; + while (task_stop == false) { + + t1 = t2; + t2 = esp_timer_get_time(); + if (t1 > t2) { + int64_t shift_us = t2 - t1; + time_jumped = true; + printf("System clock jumps back: %lli us\n", shift_us); + } + + vTaskDelay(1); + } + vTaskDelete(NULL); +} + +static void timer_callback(void* arg) +{ + +} + +static void dport_task(void *pvParameters) +{ + while (task_stop == false) { + DPORT_STALL_OTHER_CPU_START(); + ets_delay_us(3); + DPORT_STALL_OTHER_CPU_END(); + } + vTaskDelete(NULL); +} + +TEST_CASE("esp_timer_impl_set_alarm does not set an alarm below the current time", "[esp_timer][timeout=62]") +{ + const int max_timers = 2; + time_jumped = false; + task_stop = false; + + xTaskCreatePinnedToCore(task_check_time, "task_check_time", 4096, NULL, 5, NULL, 0); + // dport_task is used here to interrupt the esp_timer_impl_set_alarm function. + // To interrupt it we can use an interrupt with 4 or 5 levels which will run on CPU0. + // Instead, an interrupt we use the dport workaround which has 4 interrupt level for stall CPU0. + xTaskCreatePinnedToCore(dport_task, "dport_task", 4096, NULL, 5, NULL, 1); + + const esp_timer_create_args_t periodic_timer_args = { + .callback = &timer_callback, + }; + + esp_timer_handle_t periodic_timer[max_timers]; + printf("timers created\n"); + + esp_timer_create(&periodic_timer_args, &periodic_timer[0]); + esp_timer_start_periodic(periodic_timer[0], 9000); + + esp_timer_create(&periodic_timer_args, &periodic_timer[1]); + esp_timer_start_periodic(periodic_timer[1], 9000); + + vTaskDelay(60 * 1000 / portTICK_PERIOD_MS); + task_stop = true; + + esp_timer_stop(periodic_timer[0]); + esp_timer_delete(periodic_timer[0]); + esp_timer_stop(periodic_timer[1]); + esp_timer_delete(periodic_timer[1]); + printf("timers deleted\n"); + + vTaskDelay(1000 / portTICK_PERIOD_MS); + + TEST_ASSERT(time_jumped == false); +} + + +static esp_timer_handle_t oneshot_timer; + +static void oneshot_timer_callback(void* arg) +{ + esp_timer_start_once(oneshot_timer, 5000); +} + +static const esp_timer_create_args_t oneshot_timer_args = { + .callback = &oneshot_timer_callback, +}; + +TEST_CASE("esp_timer_impl_set_alarm and using start_once do not lead that the System time jumps back", "[esp_timer][timeout=62]") +{ + time_jumped = false; + task_stop = false; + + xTaskCreatePinnedToCore(task_check_time, "task_check_time", 4096, NULL, 5, NULL, 0); + // dport_task is used here to interrupt the esp_timer_impl_set_alarm function. + // To interrupt it we can use an interrupt with 4 or 5 levels which will run on CPU0. + // Instead, an interrupt we use the dport workaround which has 4 interrupt level for stall CPU0. + xTaskCreatePinnedToCore(dport_task, "dport_task", 4096, NULL, 5, NULL, 1); + + const esp_timer_create_args_t periodic_timer_args = { + .callback = &timer_callback, + }; + + esp_timer_handle_t periodic_timer; + + esp_timer_create(&periodic_timer_args, &periodic_timer); + esp_timer_start_periodic(periodic_timer, 5000); + + esp_timer_create(&oneshot_timer_args, &oneshot_timer); + esp_timer_start_once(oneshot_timer, 9990); + printf("timers created\n"); + + vTaskDelay(60 * 1000 / portTICK_PERIOD_MS); + task_stop = true; + + esp_timer_stop(oneshot_timer); + esp_timer_delete(oneshot_timer); + printf("timers deleted\n"); + + vTaskDelay(1000 / portTICK_PERIOD_MS); + + TEST_ASSERT(time_jumped == false); +} + +#endif // !defined(CONFIG_FREERTOS_UNICORE) && defined(CONFIG_ESP32_DPORT_WORKAROUND) From 007d01c10a1971058af3679578d0e9d6d7a97450 Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Fri, 6 Sep 2019 20:17:25 +0800 Subject: [PATCH 2/2] esp_timer: Fix System time jumps back ~54secs Closes: https://github.com/espressif/esp-idf/issues/2513 --- components/esp32/esp_timer_esp32.c | 56 ++++++++++----------------- components/esp_common/src/esp_timer.c | 16 +++++--- 2 files changed, 31 insertions(+), 41 deletions(-) diff --git a/components/esp32/esp_timer_esp32.c b/components/esp32/esp_timer_esp32.c index 541ddc740..37981a2e4 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" @@ -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 @@ -152,10 +146,6 @@ portMUX_TYPE s_time_update_lock = portMUX_INITIALIZER_UNLOCKED; // Check if timer overflow has happened (but was not handled by ISR yet) 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) || (!TIMER_IS_AFTER_OVERFLOW(REG_READ(FRC_TIMER_ALARM_REG(1))) && TIMER_IS_AFTER_OVERFLOW(REG_READ(FRC_TIMER_COUNT_REG(1)))))); @@ -222,35 +212,31 @@ uint64_t IRAM_ATTR esp_timer_impl_get_time(void) 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; - } + do { + // Adjust current time if overflow has happened + if (timer_overflow_happened()) { + 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; + uint64_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); + } while (REG_READ(FRC_TIMER_ALARM_REG(1)) <= REG_READ(FRC_TIMER_COUNT_REG(1))); portEXIT_CRITICAL(&s_time_update_lock); } @@ -261,7 +247,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 @@ -336,7 +321,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); } diff --git a/components/esp_common/src/esp_timer.c b/components/esp_common/src/esp_timer.c index 718b8100d..8a343e34d 100644 --- a/components/esp_common/src/esp_timer.c +++ b/components/esp_common/src/esp_timer.c @@ -129,12 +129,15 @@ esp_err_t IRAM_ATTR esp_timer_start_once(esp_timer_handle_t timer, uint64_t time if (!is_initialized() || timer_armed(timer)) { return ESP_ERR_INVALID_STATE; } + timer_list_lock(); timer->alarm = esp_timer_get_time() + timeout_us; timer->period = 0; #if WITH_PROFILING timer->times_armed++; #endif - return timer_insert(timer); + esp_err_t err = timer_insert(timer); + timer_list_unlock(); + return err; } esp_err_t IRAM_ATTR esp_timer_start_periodic(esp_timer_handle_t timer, uint64_t period_us) @@ -145,13 +148,16 @@ esp_err_t IRAM_ATTR esp_timer_start_periodic(esp_timer_handle_t timer, uint64_t if (!is_initialized() || timer_armed(timer)) { return ESP_ERR_INVALID_STATE; } + timer_list_lock(); period_us = MAX(period_us, esp_timer_impl_get_min_period_us()); timer->alarm = esp_timer_get_time() + period_us; timer->period = period_us; #if WITH_PROFILING timer->times_armed++; #endif - return timer_insert(timer); + esp_err_t err = timer_insert(timer); + timer_list_unlock(); + return err; } esp_err_t IRAM_ATTR esp_timer_stop(esp_timer_handle_t timer) @@ -173,16 +179,17 @@ esp_err_t esp_timer_delete(esp_timer_handle_t timer) if (timer_armed(timer)) { return ESP_ERR_INVALID_STATE; } + timer_list_lock(); timer->event_id = EVENT_ID_DELETE_TIMER; - timer->alarm = esp_timer_get_time() + 50; + timer->alarm = esp_timer_get_time(); timer->period = 0; timer_insert(timer); + timer_list_unlock(); return ESP_OK; } static IRAM_ATTR esp_err_t timer_insert(esp_timer_handle_t timer) { - timer_list_lock(); #if WITH_PROFILING timer_remove_inactive(timer); #endif @@ -205,7 +212,6 @@ static IRAM_ATTR esp_err_t timer_insert(esp_timer_handle_t timer) if (timer == LIST_FIRST(&s_timers)) { esp_timer_impl_set_alarm(timer->alarm); } - timer_list_unlock(); return ESP_OK; }