Merge pull request #12938 from kjbracey-arm/chrono-sleep-correct

Correct core RTOS sleep routine timing

platform/source/mbed_os_timer.cpp

@@ -200,16 +200,18 @@ OsClock::time_point do_timed_sleep_absolute(OsClock::time_point wake_time, bool
 #if MBED_CONF_RTOS_PRESENT
 /* The 32-bit limit is part of the API - we will always wake within 2^32 ticks */
 /* This version is tuned for RTOS use, where the RTOS needs to know the time spent sleeping */
-OsClock::duration_u32 do_timed_sleep_relative(OsClock::duration_u32 wake_delay, bool (*wake_predicate)(void *), void *wake_predicate_handle)
+/* Note that unlike do_timed_sleep_relative_or_forever it does not do a tick update on entry; */
+/* it assumes the caller has been using the ticker, and is passing a delay relative to the */
+/* time point of the ticks it has acknowledged. */
+OsClock::duration_u32 do_timed_sleep_relative_to_acknowledged_ticks(OsClock::duration_u32 wake_delay, bool (*wake_predicate)(void *), void *wake_predicate_handle)
 {
-    OsClock::time_point sleep_start = OsClock::now();
     // When running with RTOS, the requested delay will be based on the kernel's tick count.
-    // If it missed a tick as entering idle, we should reflect that by moving the
+    // If it missed a tick as entering idle, we should reflect that by basing the start time
-    // start time back to reflect its current idea of time.
+    // on its current idea of time - OsClock::acknowledged_ticks().
-    // Example: OS tick count = 100, our tick count = 101, requested delay = 50
+    // Example: OS acknowledged tick count = 100, our reported tick count = 101, requested delay = 50
     // We need to schedule wake for tick 150, report 50 ticks back to our caller, and
     // clear the unacknowledged tick count.
-    sleep_start -= os_timer->unacknowledged_ticks();
+    OsClock::time_point sleep_start = OsClock::acknowledged_ticks();
 
     OsClock::time_point sleep_finish = do_timed_sleep_absolute(sleep_start + wake_delay, wake_predicate, wake_predicate_handle);
 
@@ -226,7 +228,8 @@ void do_untimed_sleep(bool (*wake_predicate)(void *), void *wake_predicate_handl
 }
 
 /* max() delay is treated as "wait forever" */
-/* This version is tuned for non-RTOS use, where we don't need to return sleep time, and waiting forever is possible */
+/* This version is tuned for non-RTOS use, where we must update the tick count, */
+/* don't need to return sleep time, and waiting forever is possible */
 void do_timed_sleep_relative_or_forever(OsClock::duration_u32 wake_delay, bool (*wake_predicate)(void *), void *wake_predicate_handle)
 {
     // Special-case 0 delay, to save multiple callers having to do it. Just call the predicate once.

platform/source/mbed_os_timer.h

@@ -42,18 +42,42 @@ extern OsTimer *os_timer;
 OsTimer *init_os_timer();
 
 /** A C++11 chrono TrivialClock for os_timer
+ *
+ * Due to the nature of OsTimer/SysTimer, this does not have a single `now` method, but has
+ * multiple ways to report the current state:
+ *
+ *      High-res timeline -------------------------------------------------------------
+ *      Ticks               |  a  |  b  |  b  |  b  |  c  |  c  |  c  |  c  |  c  | d ^
+ *                                ^                 ^                             ^   os_timer->get_time()
+ *                         acknowledged_ticks()   reported_ticks()               now()
+ *
+ * (a) is time read from hardware by OsTimer, reported to the user of OsTimer, and acknowledged by that user.
+ * (b) is time read from hardware by OsTimer, reported to the user of OsTimer, but not yet acknowledged.
+ * (c) is time already elapsed in the hardware but yet to be read and processed as ticks by OsTimer.
+ * (d) is time already elapsed in the hardware that doesn't yet form a tick.
+ *
+ * Time is "reported" either by:
+ *   * calls to the OsTimer's handler following start_tick - these must be acknowledged
+ *   * the result of OsTimer::update_and_get_tick() / OsClock::now() - calling this implies acknowledgment.
+ *
+ * As such `now()` is used when the ticker is not in use - it processes ticks that would have been
+ * processed by the tick handler. If the ticker is in uses `reported_ticks` or `acknowleged_ticks` must be used.
  *
  * @note To fit better into the chrono framework, OsClock uses
  *       chrono::milliseconds as its representation, which makes it signed
- * and at least 45 bits (so it will be int64_t or equivalent).
+ *       and at least 45 bits, so it will be int64_t or equivalent, unlike
+ *       OsTimer which uses uint64_t rep.
  */
 struct OsClock {
     /* Standard TrivialClock fields */
-    using duration = std::chrono::milliseconds;
+    using period = OsTimer::period;
-    using rep = duration::rep;
+    using rep = std::chrono::milliseconds::rep;
-    using period = duration::period;
+    using duration = std::chrono::duration<rep, period>; // == std::chrono::milliseconds, if period is std::milli
     using time_point = std::chrono::time_point<OsClock, duration>;
     static constexpr bool is_steady = true;
+    // Read the hardware, and return the updated time_point.
+    // Initialize the timing system if necessary - this could be the first call.
+    // See SysTimer::update_and_get_tick for more details.
     static time_point now()
     {
         // We are a real Clock with a well-defined epoch. As such we distinguish ourselves
@@ -61,6 +85,18 @@ struct OsClock {
         // are not convertible, so need to fiddle here.
         return time_point(init_os_timer()->update_and_get_tick().time_since_epoch());
     }
+    // Return the current reported tick count, without update.
+    // Assumes timer has already been initialized, as ticker should have been in use to
+    // keep that tick count up-to-date. See SysTimer::get_tick for more details.
+    static time_point reported_ticks()
+    {
+        return time_point(os_timer->get_tick().time_since_epoch());
+    }
+    // Return the acknowledged tick count.
+    static time_point acknowledged_ticks()
+    {
+        return reported_ticks() - os_timer->unacknowledged_ticks();
+    }
     // Slightly-optimised variant of OsClock::now() that assumes os_timer is initialised.
     static time_point now_with_init_done()
     {
@@ -81,7 +117,7 @@ OsClock::time_point do_timed_sleep_absolute(OsClock::time_point wake_time, bool
 #if MBED_CONF_RTOS_PRESENT
 /* Maximum sleep time is 2^32-1 ticks; timer is always set to achieve this */
 /* Assumes that ticker has been in use prior to call, so restricted to RTOS use */
-OsClock::duration_u32 do_timed_sleep_relative(OsClock::duration_u32 wake_delay, bool (*wake_predicate)(void *) = NULL, void *wake_predicate_handle = NULL);
+OsClock::duration_u32 do_timed_sleep_relative_to_acknowledged_ticks(OsClock::duration_u32 wake_delay, bool (*wake_predicate)(void *) = NULL, void *wake_predicate_handle = NULL);
 #else
 
 void do_untimed_sleep(bool (*wake_predicate)(void *), void *wake_predicate_handle = NULL);

rtos/source/TARGET_CORTEX/mbed_rtx_idle.cpp

@@ -139,7 +139,7 @@ extern "C" {
         rtos::Kernel::Clock::duration_u32 ticks_to_sleep{osKernelSuspend()};
         // osKernelSuspend will call OS_Tick_Disable, cancelling the tick, which frees
         // up the os timer for the timed sleep
-        rtos::Kernel::Clock::duration_u32 ticks_slept = mbed::internal::do_timed_sleep_relative(ticks_to_sleep, rtos_event_pending);
+        rtos::Kernel::Clock::duration_u32 ticks_slept = mbed::internal::do_timed_sleep_relative_to_acknowledged_ticks(ticks_to_sleep, rtos_event_pending);
         MBED_ASSERT(ticks_slept < rtos::Kernel::wait_for_u32_max);
         osKernelResume(ticks_slept.count());
     }