From 4b1b18fafdf7373d8300d0dd97bd63a408f36afc Mon Sep 17 00:00:00 2001 From: Peter Harper Date: Thu, 5 Dec 2024 15:38:55 +0000 Subject: [PATCH 1/2] ta_set_timeout can fail to set an alarm If alarm_pool_irq_handler takes <1us between handling an alarm and calling ta_set_timeout then no alarms will be set as it will appear as if an earlier alarm is already armedi before the target time. Make sure ta_set_timeout always leaves with an alarm set by checking the armed status. Fixes #2118 --- .../include/pico/time_adapter.h | 3 +- test/pico_time_test/pico_time_test.c | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h b/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h index 59308b4b9..63dc39e30 100644 --- a/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h +++ b/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h @@ -48,7 +48,8 @@ static inline void ta_set_timeout(alarm_pool_timer_t *timer, uint alarm_num, int // 2. If we just passed the target time, then time_til_target will be high, meaning we will // likely not do the update, but this is OK since the caller who has the full 64 bits // must check if the target time has passed when we return anyway to avoid races. - if (time_til_target < time_til_alarm) { + // 3. We should never leave here without an alarm being armed + if (time_til_target < time_til_alarm || (timer_hw_from_timer(timer)->armed & (1 << alarm_num)) == 0) { timer_hw_from_timer(timer)->alarm[alarm_num] = (uint32_t) target; } } diff --git a/test/pico_time_test/pico_time_test.c b/test/pico_time_test/pico_time_test.c index 362187ddb..d73867122 100644 --- a/test/pico_time_test/pico_time_test.c +++ b/test/pico_time_test/pico_time_test.c @@ -8,6 +8,7 @@ #include #include #include +#include "hardware/clocks.h" #include "pico/stdlib.h" #include "pico/test.h" // Include sys/types.h before inttypes.h to work around issue with @@ -74,6 +75,7 @@ static bool repeating_timer_callback(struct repeating_timer *t) { int issue_195_test(void); int issue_1812_test(void); int issue_1953_test(void); +int issue_2118_test(void); int main() { setup_default_uart(); @@ -246,6 +248,8 @@ int main() { issue_1953_test(); + issue_2118_test(); + PICOTEST_END_TEST(); } @@ -325,3 +329,38 @@ int issue_1953_test(void) { PICOTEST_END_SECTION(); return 0; } + +static int counter_2118; +static bool timer_callback_issue_2118(repeating_timer_t *rt) { + counter_2118++; + return true; +} + +int issue_2118_test(void) { + PICOTEST_START_SECTION("Issue #2118 defect - failure to set an alarm"); + + // this problem only happens when running the clock fast as it requires the time between + // alarm_pool_irq_handler handling an alarm and setting the next alarm to be <1us + set_sys_clock_hz(200 * MHZ, true); + setup_default_uart(); + + alarm_pool_t *pool = alarm_pool_create(2, 1); + repeating_timer_t timer; + alarm_pool_add_repeating_timer_ms(pool, -20, timer_callback_issue_2118, NULL, &timer); + + int iterations = 0; + while(iterations < 100) { + iterations++; + sleep_ms(20); + } + PICOTEST_CHECK(counter_2118 >= 100, "Repeating timer failure"); + + cancel_repeating_timer(&timer); + alarm_pool_destroy(pool); + + set_sys_clock_hz(SYS_CLK_HZ, true); + setup_default_uart(); + + PICOTEST_END_SECTION(); + return 0; +} From 2282b2b19340070366a8d572762ef82be84484d4 Mon Sep 17 00:00:00 2001 From: Peter Harper Date: Fri, 6 Dec 2024 11:25:59 +0000 Subject: [PATCH 2/2] ta_disable_irq_handler should unarm its timer --- src/rp2_common/pico_time_adapter/include/pico/time_adapter.h | 1 + test/pico_time_test/pico_time_test.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h b/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h index 63dc39e30..bc9caf1ad 100644 --- a/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h +++ b/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h @@ -76,6 +76,7 @@ static inline void ta_enable_irq_handler(alarm_pool_timer_t *timer, uint alarm_n static inline void ta_disable_irq_handler(alarm_pool_timer_t *timer, uint alarm_num, irq_handler_t irq_handler) { uint irq_num = timer_hardware_alarm_get_irq_num(timer, alarm_num); + timer_hw_from_timer(timer)->armed = 1u << alarm_num; // disarm the timer hw_clear_bits(&timer_hw_from_timer(timer)->inte, 1u << alarm_num); irq_set_enabled(irq_num, true); irq_remove_handler(irq_num, irq_handler); diff --git a/test/pico_time_test/pico_time_test.c b/test/pico_time_test/pico_time_test.c index d73867122..8092abbef 100644 --- a/test/pico_time_test/pico_time_test.c +++ b/test/pico_time_test/pico_time_test.c @@ -355,8 +355,8 @@ int issue_2118_test(void) { } PICOTEST_CHECK(counter_2118 >= 100, "Repeating timer failure"); - cancel_repeating_timer(&timer); alarm_pool_destroy(pool); + hard_assert(timer_hw->armed == 0); // check destroying the pool unarms its timer set_sys_clock_hz(SYS_CLK_HZ, true); setup_default_uart();