From 580ef6d88f22478112417c4ab3c1ee50211167d2 Mon Sep 17 00:00:00 2001 From: Sergey Vlasov Date: Fri, 4 Feb 2022 21:08:50 +0300 Subject: [PATCH] ChibiOS timer fixes (#16017) * chibios/timer: Move the 16-bit timer handling into a separate function Extract the code which effectively makes a 32-bit tick counter from a possibly 16-bit ChibiOS system timer into a separate function. Does not really change the behavior of the timer API, but makes the actions done in `timer_clear()` and `timer_read32()` more obvious. * chibios/timer: Rename some variable to better reflect their role * chibios/timer: Fix 32-bit tick counter overflow handling The QMK timer API implementation for ChibiOS used a 32-bit tick counter (obtained from the ChibiOS system timer) and then converted the value to milliseconds to produce the timer value for QMK. However, the frequency of the ChibiOS timer is above 1000 Hz in most cases (values of 10000 Hz or even 100000 Hz are typically used), and therefore the 32-bit tick counter was overflowing and wrapping around much earlier than expected (after about 5 days for 10000 Hz, or about 12 hours for 100000 Hz). When this wraparound happened, the QMK timer value was jumping back to zero, which broke various code dealing with timers (e.g., deferred executors). Just making the tick counter 64-bit to avoid the overflow is not a good solution, because the ChibiOS code which performs the conversion from ticks to milliseconds may encounter overflows when handling a 64-bit value. Adjusting just the value converted to milliseconds to account for lost 2**32 ticks is also not possible, because 2**32 ticks may not correspond to an integer number of milliseconds. Therefore the tick counter overflow is handled as follows: - A reasonably large number of ticks (the highest multiple of the ChibiOS timer frequency that fits into uint32_t) is subtracted from the tick counter, so that its value is again brought below 2**32. The subtracted value is chosen so that it would correspond to an integer number of seconds, therefore it could be converted to milliseconds without any loss of precision. - The equivalent number of milliseconds is then added to the converted QMK timer value, so that the QMK timer continues to count milliseconds as it was before the tick counter overflow. * chibios/timer: Add a virtual timer to make 16-bit timer updates more reliable The code which extends the 16-bit ChibiOS system timer to a 32-bit tick counter requires that it is called at least once for every overflow of the system timer (otherwise the tick counter can skip one or more overflow periods). Normally this requirement is satisfied just from various parts of QMK code reading the current timer value; however, in some rare circumstances the QMK code may be blocked waiting for some event, and when this situation is combined with having a rather high timer frequency, this may result in improper timekeeping. Enhance the timer reliability by adding a ChibiOS virtual timer which invokes a callback every half of the timer overflow period. The virtual timer callback can be invoked even when the normal QMK code is blocked; the only requirement is that the timer interrupts are enabled, and the ChibiOS kernel is not locked for an excessive time (but the timer update will eventually work correctly if the virtual timer handling is not delayed by more than a half of the timer overflow period). Keeping a virtual timer always active also works around a ChibiOS bug that can manifest with a 16-bit system timer and a relatively high timer frequency: when all active virtual timers have delays longer than the timer overflow period, the handling of virtual timers stops completely. In QMK this bug can result in a `wait_ms()` call with a delay larger than the timer overflow period just hanging indefinitely. However, when the timer update code adds a virtual timer with a shorter delay, all other virtual timers are also handled properly. --- platforms/chibios/timer.c | 102 +++++++++++++++++++++++++++++--------- 1 file changed, 78 insertions(+), 24 deletions(-) diff --git a/platforms/chibios/timer.c b/platforms/chibios/timer.c index 9f664e1f798..74fdd7a6fce 100644 --- a/platforms/chibios/timer.c +++ b/platforms/chibios/timer.c @@ -2,44 +2,98 @@ #include "timer.h" -static uint32_t reset_point = 0; +static uint32_t ticks_offset = 0; +static uint32_t last_ticks = 0; +static uint32_t ms_offset = 0; #if CH_CFG_ST_RESOLUTION < 32 static uint32_t last_systime = 0; static uint32_t overflow = 0; #endif -void timer_init(void) { timer_clear(); } +// Get the current system time in ticks as a 32-bit number. +// This function must be called from within a system lock zone (so that it can safely use and update the static data). +static inline uint32_t get_system_time_ticks(void) { + uint32_t systime = (uint32_t)chVTGetSystemTimeX(); + +#if CH_CFG_ST_RESOLUTION < 32 + // If the real system timer resolution is less than 32 bits, provide the missing bits by checking for the counter + // overflow. For this to work, this function must be called at least once for every overflow of the system timer. + // In the 16-bit case, the corresponding times are: + // - CH_CFG_ST_FREQUENCY = 100000, overflow will occur every ~0.65 seconds + // - CH_CFG_ST_FREQUENCY = 10000, overflow will occur every ~6.5 seconds + // - CH_CFG_ST_FREQUENCY = 1000, overflow will occur every ~65 seconds + if (systime < last_systime) { + overflow += ((uint32_t)1) << CH_CFG_ST_RESOLUTION; + } + last_systime = systime; + systime += overflow; +#endif + + return systime; +} + +#if CH_CFG_ST_RESOLUTION < 32 +static virtual_timer_t update_timer; + +// Update the system tick counter every half of the timer overflow period; this should keep the tick counter correct +// even if something blocks timer interrupts for 1/2 of the timer overflow period. +# define UPDATE_INTERVAL (((sysinterval_t)1) << (CH_CFG_ST_RESOLUTION - 1)) + +// VT callback function to keep the overflow bits of the system tick counter updated. +static void update_fn(void *arg) { + (void)arg; + chSysLockFromISR(); + get_system_time_ticks(); + chVTSetI(&update_timer, UPDATE_INTERVAL, update_fn, NULL); + chSysUnlockFromISR(); +} +#endif + +// The highest multiple of CH_CFG_ST_FREQUENCY that fits into uint32_t. This number of ticks will necessarily +// correspond to some integer number of seconds. +#define OVERFLOW_ADJUST_TICKS ((uint32_t)((UINT32_MAX / CH_CFG_ST_FREQUENCY) * CH_CFG_ST_FREQUENCY)) + +// The time in milliseconds which corresponds to OVERFLOW_ADJUST_TICKS ticks (this is a precise conversion, because +// OVERFLOW_ADJUST_TICKS corresponds to an integer number of seconds). +#define OVERFLOW_ADJUST_MS (TIME_I2MS(OVERFLOW_ADJUST_TICKS)) + +void timer_init(void) { + timer_clear(); +#if CH_CFG_ST_RESOLUTION < 32 + chVTObjectInit(&update_timer); + chVTSet(&update_timer, UPDATE_INTERVAL, update_fn, NULL); +#endif +} void timer_clear(void) { - reset_point = (uint32_t)chVTGetSystemTime(); -#if CH_CFG_ST_RESOLUTION < 32 - last_systime = reset_point; - overflow = 0; -#endif + chSysLock(); + ticks_offset = get_system_time_ticks(); + last_ticks = 0; + ms_offset = 0; + chSysUnlock(); } uint16_t timer_read(void) { return (uint16_t)timer_read32(); } uint32_t timer_read32(void) { - uint32_t systime = (uint32_t)chVTGetSystemTime(); - -#if CH_CFG_ST_RESOLUTION < 32 - // If/when we need to support 64-bit chips, this may need to be modified to match the native bit-ness of the MCU. - // At this point, the only SysTick resolution allowed other than 32 is 16 bit. - // In the 16-bit case, at: - // - CH_CFG_ST_FREQUENCY = 100000, overflow will occur every ~0.65 seconds - // - CH_CFG_ST_FREQUENCY = 10000, overflow will occur every ~6.5 seconds - // - CH_CFG_ST_FREQUENCY = 1000, overflow will occur every ~65 seconds - // With this implementation, as long as we ensure a timer read happens at least once during the overflow period, timing should be accurate. - if (systime < last_systime) { - overflow += ((uint32_t)1) << CH_CFG_ST_RESOLUTION; + chSysLock(); + uint32_t ticks = get_system_time_ticks() - ticks_offset; + if (ticks < last_ticks) { + // The 32-bit tick counter overflowed and wrapped around. We cannot just extend the counter to 64 bits here, + // because TIME_I2MS() may encounter overflows when handling a 64-bit argument; therefore the solution here is + // to subtract a reasonably large number of ticks from the tick counter to bring its value below the 32-bit + // limit again, and then add the equivalent number of milliseconds to the converted value. (Adjusting just the + // converted value to account for 2**32 ticks is not possible in general, because 2**32 ticks may not correspond + // to an integer number of milliseconds). + ticks -= OVERFLOW_ADJUST_TICKS; + ticks_offset += OVERFLOW_ADJUST_TICKS; + ms_offset += OVERFLOW_ADJUST_MS; } + last_ticks = ticks; + uint32_t ms_offset_copy = ms_offset; // read while still holding the lock to ensure a consistent value + chSysUnlock(); - last_systime = systime; - return (uint32_t)TIME_I2MS(systime - reset_point + overflow); -#else - return (uint32_t)TIME_I2MS(systime - reset_point); -#endif + return (uint32_t)TIME_I2MS(ticks) + ms_offset_copy; } uint16_t timer_elapsed(uint16_t last) { return TIMER_DIFF_16(timer_read(), last); }