From 027147b233a921d1b364ba328d032ce744a9823a Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Mon, 15 Apr 2024 19:35:51 +0200 Subject: [PATCH 1/4] drivers/periph_timer: add timer_get_closest_freq Add an API to search for the frequency supported by a timer that is closest to the given target frequency. This is in fact non-trivial to get right, as pre-scaler registers can be 16 bit or even 32 bit in size, making a naive loop over all possible pre-scalers too expensive (computationally). --- drivers/include/periph/timer.h | 18 ++++++++++++ drivers/periph_common/timer.c | 51 +++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/drivers/include/periph/timer.h b/drivers/include/periph/timer.h index 1253cd347a87..66587b5a6a91 100644 --- a/drivers/include/periph/timer.h +++ b/drivers/include/periph/timer.h @@ -296,6 +296,24 @@ uword_t timer_query_channel_numof(tim_t dev); */ uint32_t timer_query_freqs(tim_t dev, uword_t index); +/** + * @brief Search the frequency supported by the timer that is closest to + * a given target frequency, efficiently + * + * @param dev Timer to get the closest supported frequency for + * @param target Ideal frequency to match + * @return The frequency supported by the timer @p dev that is + * closest to @p target + * + * @note Add `FEATURES_REQUIRED += periph_timer_query_freqs` to your + * `Makefile`. + * + * @details This will use binary search internally to have an O(log(n)) + * runtime. This can be relevant on hardware with 16 bit or 32 bit + * prescaler registers. + */ +uint32_t timer_get_closest_freq(tim_t dev, uint32_t target); + #if defined(DOXYGEN) /** * @brief Check whether a compare channel has matched diff --git a/drivers/periph_common/timer.c b/drivers/periph_common/timer.c index 624bbf003f61..385bd958c47b 100644 --- a/drivers/periph_common/timer.c +++ b/drivers/periph_common/timer.c @@ -32,10 +32,59 @@ int timer_set(tim_t dev, int channel, unsigned int timeout) #endif #ifdef MODULE_PERIPH_TIMER_QUERY_FREQS -__attribute__((weak)) +__attribute__((weak, pure)) uword_t timer_query_channel_numof(tim_t dev) { (void)dev; return TIMER_CHANNEL_NUMOF; } + +uint32_t timer_get_closest_freq(tim_t dev, uint32_t target) +{ + uint32_t freq; + uword_t idx_max = timer_query_freqs_numof(dev) - 1; + + /* use binary search to find one of the three possibilities: + * 1. If an exact match is possible: The exact match is found + * 2. Otherwise: + * a) the highest frequency below the target, or + * b) the lowest frequency above the target + */ + uword_t idx_lower = 0; + uword_t idx_upper = idx_max; + while (idx_lower != idx_upper) { + uword_t idx_mid = (idx_lower + idx_upper) >> 1; + freq = timer_query_freqs(dev, idx_mid); + if (freq > target) { + idx_lower = idx_mid + 1; + } + else { + idx_upper = idx_mid; + } + } + + freq = timer_query_freqs(dev, idx_lower); + if ((freq < target) && (idx_lower > 0)) { + /* binary search yielded the closest frequency below the target. But + * maybe the closest frequency above the target is actually better. */ + uint32_t diff = target - freq; + uint32_t alternative = timer_query_freqs(dev, idx_lower - 1); + if (target + diff > alternative) { + /* the alternative is better */ + return alternative; + } + } + else if ((freq > target) && (idx_lower < idx_max)) { + /* Got a frequency above the target, maybe the one below would be + * a closer match */ + uint32_t diff = target - freq; + uint32_t alternative = timer_query_freqs(dev, idx_lower + 1); + if (target < alternative + diff) { + return alternative; + } + } + + /* either we got an exact match, or the other candidate was no closer */ + return freq; +} #endif From 555ab6a71ec3f6692a9fa3927f8f0553057004d5 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Mon, 15 Apr 2024 20:09:08 +0200 Subject: [PATCH 2/4] tests/periph/timer: test timer_get_closest_freq() --- tests/periph/timer/main.c | 40 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/tests/periph/timer/main.c b/tests/periph/timer/main.c index 0071119a2840..7a5e559110a8 100644 --- a/tests/periph/timer/main.c +++ b/tests/periph/timer/main.c @@ -202,7 +202,7 @@ static int test_timer(unsigned num, uint32_t timer_freq) return 0; } - printf(" OK (no spurious IRQs)\n"); + printf(" [OK] (no spurious IRQs)\n"); return 1; } @@ -249,8 +249,38 @@ static void print_supported_frequencies(tim_t dev) " %u: %" PRIu32 "\n", (unsigned)(end - 1), timer_query_freqs(dev, end - 1)); } +} + +static void test_querying(tim_t dev) +{ + uword_t last = query_freq_numof(dev) - 1; + + puts("Testing timer_get_closest_freq()..."); + for (uword_t i = 0; i <= MIN(last, 255); i++) { + uint32_t closest; + uint32_t freq = timer_query_freqs(dev, i); + + /* Searching for a supported freq should yield the supported freq: */ + expect(freq == timer_get_closest_freq(dev, freq)); + + /* Testing for 1 Hz below freq. + * + * Note that freq - 1 is even closer to + * freq - 1 than freq, and freq - 2 is equally close. Either may + * be supported, so we allow everything with [freq: freq -2]. */ + closest = timer_get_closest_freq(dev, freq - 1); + expect((freq >= closest) && closest >= freq - 2); + + /* Testing for 1 Hz above freq, with the same corner case + * handling as above. */ + closest = timer_get_closest_freq(dev, freq + 1); + expect((freq <= closest) && closest <= freq + 2); + } - expect(timer_query_freqs(dev, end) == 0); + puts("[OK]\n" + "Testing timer_query_freqs() for out of bound index..."); + expect(timer_query_freqs(dev, last + 1) == 0); + puts("[OK]"); } int main(void) @@ -265,6 +295,12 @@ int main(void) printf("\nTIMER %u\n" "=======\n\n", i); print_supported_frequencies(TIMER_DEV(i)); + + /* test querying of frequencies, but only if supported by the driver */ + if (IS_USED(MODULE_PERIPH_TIMER_QUERY_FREQS)) { + test_querying(TIMER_DEV(i)); + } + uword_t end = query_freq_numof(TIMER_DEV(i)); /* Test only up to three frequencies and only the fastest once. From 889c261fa318b3bb106a6df2fc60fc723f3d7482 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Mon, 15 Apr 2024 20:27:37 +0200 Subject: [PATCH 3/4] sys/ztimer: make use of periph_timer_query_freqs This makes use of the `periph_timer_query_freqs` feature: 1. It does choose the closest frequency supported before calling timer_init() in the ztimer_periph_timer backend. 2. It does make use of the actually chosen frequency when using `ztimer_convert_frac`. 3. It does `assert()` the frequency is within 5% of the specified when no frequency conversion is performed or `ztimer_convert_shift_up` is used. --- sys/include/ztimer/periph_timer.h | 6 ++++-- sys/ztimer/Makefile.dep | 1 + sys/ztimer/init.c | 17 +++++++++++++---- sys/ztimer/periph_timer.c | 12 ++++++++++-- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/sys/include/ztimer/periph_timer.h b/sys/include/ztimer/periph_timer.h index 21b8277a19a5..5e1a7ed634f8 100644 --- a/sys/include/ztimer/periph_timer.h +++ b/sys/include/ztimer/periph_timer.h @@ -52,9 +52,11 @@ typedef struct { * @param[in] dev periph timer to use * @param[in] freq frequency to configure * @param[in] max_val maximum value this timer supports + * + * @return The actual frequency the timer has been configured to */ -void ztimer_periph_timer_init(ztimer_periph_timer_t *clock, tim_t dev, - uint32_t freq, uint32_t max_val); +uint32_t ztimer_periph_timer_init(ztimer_periph_timer_t *clock, tim_t dev, + uint32_t freq, uint32_t max_val); #ifdef __cplusplus } diff --git a/sys/ztimer/Makefile.dep b/sys/ztimer/Makefile.dep index e20ba7e2a7b0..02c31c1d4a5e 100644 --- a/sys/ztimer/Makefile.dep +++ b/sys/ztimer/Makefile.dep @@ -39,6 +39,7 @@ endif ifneq (,$(filter ztimer_periph_timer,$(USEMODULE))) FEATURES_REQUIRED += periph_timer + FEATURES_OPTIONAL += periph_timer_query_freqs endif ifneq (,$(filter ztimer_periph_rtc,$(USEMODULE))) diff --git a/sys/ztimer/init.c b/sys/ztimer/init.c index 08aa1b4f056f..bd24ebb89ee9 100644 --- a/sys/ztimer/init.c +++ b/sys/ztimer/init.c @@ -39,7 +39,9 @@ #include "kernel_defines.h" +#include "assert.h" #include "board.h" +#include "compiler_hints.h" #include "ztimer.h" #include "ztimer/convert_frac.h" #include "ztimer/convert_shift.h" @@ -290,8 +292,11 @@ void ztimer_init(void) "ztimer_init(): ZTIMER_TIMER using periph timer %u, freq %lu, width %u\n", CONFIG_ZTIMER_USEC_DEV, ZTIMER_TIMER_FREQ, CONFIG_ZTIMER_USEC_WIDTH); - ztimer_periph_timer_init(&ZTIMER_TIMER, CONFIG_ZTIMER_USEC_DEV, - ZTIMER_TIMER_FREQ, WIDTH_TO_MAXVAL(CONFIG_ZTIMER_USEC_WIDTH)); + MAYBE_UNUSED + uint32_t periph_timer_freq = + ztimer_periph_timer_init(&ZTIMER_TIMER, CONFIG_ZTIMER_USEC_DEV, + ZTIMER_TIMER_FREQ, + WIDTH_TO_MAXVAL(CONFIG_ZTIMER_USEC_WIDTH)); # if MODULE_PM_LAYERED && !MODULE_ZTIMER_ONDEMAND LOG_DEBUG("ztimer_init(): ZTIMER_TIMER setting block_pm_mode to %i\n", CONFIG_ZTIMER_TIMER_BLOCK_PM_MODE); @@ -337,17 +342,21 @@ void ztimer_init(void) #if MODULE_ZTIMER_USEC # if ZTIMER_TIMER_FREQ != FREQ_1MHZ # if ZTIMER_TIMER_FREQ == FREQ_250KHZ + /* 250 kHz ± 5% */ + assert((periph_timer_freq > 237500) && (periph_timer_freq < 262500)); LOG_DEBUG("ztimer_init(): ZTIMER_USEC convert_shift %lu to 1000000\n", - ZTIMER_TIMER_FREQ); + periph_timer_freq); ztimer_convert_shift_up_init(&_ztimer_convert_shift_usec, ZTIMER_USEC_BASE, 2); # else LOG_DEBUG("ztimer_init(): ZTIMER_USEC convert_frac %lu to 1000000\n", ZTIMER_TIMER_FREQ); ztimer_convert_frac_init(&_ztimer_convert_frac_usec, ZTIMER_USEC_BASE, - FREQ_1MHZ, ZTIMER_TIMER_FREQ); + FREQ_1MHZ, periph_timer_freq); # endif # else + /* 1 MHz ± 5% */ + assert((periph_timer_freq > 950000) && (periph_timer_freq < 1050000)); LOG_DEBUG("ztimer_init(): ZTIMER_USEC without conversion\n"); # endif diff --git a/sys/ztimer/periph_timer.c b/sys/ztimer/periph_timer.c index 6b00bdce718b..f52b7aa66b23 100644 --- a/sys/ztimer/periph_timer.c +++ b/sys/ztimer/periph_timer.c @@ -21,7 +21,9 @@ */ #include "assert.h" +#include "compiler_hints.h" #include "irq.h" +#include "modules.h" #include "ztimer/periph_timer.h" #ifndef ZTIMER_PERIPH_TIMER_OFFSET @@ -104,12 +106,16 @@ static const ztimer_ops_t _ztimer_periph_timer_ops = { #endif }; -void ztimer_periph_timer_init(ztimer_periph_timer_t *clock, tim_t dev, - uint32_t freq, uint32_t max_val) +uint32_t ztimer_periph_timer_init(ztimer_periph_timer_t *clock, tim_t dev, + uint32_t freq, uint32_t max_val) { clock->dev = dev; clock->super.ops = &_ztimer_periph_timer_ops; clock->super.max_value = max_val; + + if (IS_USED(MODULE_PERIPH_TIMER_QUERY_FREQS)) { + freq = timer_get_closest_freq(dev, freq); + } int ret = timer_init(dev, freq, _ztimer_periph_timer_callback, clock); (void)ret; @@ -127,4 +133,6 @@ void ztimer_periph_timer_init(ztimer_periph_timer_t *clock, tim_t dev, * the first ztimer_acquire() call starts the peripheral */ timer_stop(dev); #endif + + return freq; } From 4933b9b6a19169f379fa6ab1d874649a724d94a6 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Mon, 15 Apr 2024 21:04:26 +0200 Subject: [PATCH 4/4] boards/olimex-msp430-h1611: fix ztimer config --- boards/olimex-msp430-h1611/include/board.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/boards/olimex-msp430-h1611/include/board.h b/boards/olimex-msp430-h1611/include/board.h index 7eb7833bf109..d0443113a312 100644 --- a/boards/olimex-msp430-h1611/include/board.h +++ b/boards/olimex-msp430-h1611/include/board.h @@ -36,11 +36,18 @@ extern "C" { #endif /** - * @name Xtimer configuration + * @name ztimer configuration * @{ */ -#define XTIMER_WIDTH (16) -#define XTIMER_BACKOFF (40) +#define CONFIG_ZTIMER_USEC_WIDTH 16 /**< running on a 16-bit timer */ +/** + * @brief Configure clock to ~1 MHz. + * + * HACK: Do not use exactly 1 MHz to force use of ztimer_convert_frac, as the + * internal oscillator is not precise enough for time keeping and compensation + * based on the actual measured CPU frequency is needed. + */ +#define CONFIG_ZTIMER_USEC_BASE_FREQ (MHZ(1) + 1) /** @} */ #ifdef __cplusplus