Merge branch 'bugfix/jump_time_54_sec' into 'master'

esp_timer: Fix time jumps back ~ 54sec

Closes IDFGH-396

See merge request espressif/esp-idf!5943

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) {
+    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) {
+            timer_count_reload();
-                compare_val = ALARM_OVERFLOW_VAL;
+            s_time_base_us += s_timer_us_per_overflow;
-            }
         }
-        alarm_reg_val = (uint32_t) compare_val;
+        s_mask_overflow = false;
-    }
+        uint64_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);
+    } 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);
 }
 

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)

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;
 }