[mbed] Replace Nordic's appTimer with a driver for RTC1 (#932)
Rohit Grover
This requires careful review and testing. I've done a fair bit, but much more is needed.
This has been very tricky code owing mostly to:
- RTC counter being 24bit instead of 32bit.
- us_ticker API receiving time in microseconds, whereas RTC counting in units of 1/32Khz.
- trickiness involved in manipulating lists from re-entrant functions in interrupt context. Please review and test. I have a suspicion extensive testing may reveal more hardware related quirks which haven't been taken care of.
You can view, comment on, or merge this pull request online at:
https://github.com/mbedmicro/mbed/pull/932
Commit Summary
- attempting to replace appTimer
- second round of changes at replacing app_timer with RTC
- rtc1_stop() doesn't need to be static.
- MICROSECONDS_TO_RTC_UNITS() should round-up
- remove app_timer.c
- remove m_rtc1_running; made redundant by us_ticker_inited.
- white space diffs.
- us_ticker_set_interrupt() should set an interrupt for the given timestamp even if there's a pending interrupt.
- reset us_ticker_callbackPending when disabling interrupts.
- set_interrupt: short-circuit the setting of the same interrupt.
- set_interrupt: if callbackTime is NOW, invoke handler.
- minor white space diff
- add a comment block for us_ticker_set_interrupt()
- check for instantaneous callback before checking for repeat callback.
- rename callbackTime to newCallbackTime
- add a helper method: INVOKE_CALLBACK()
- If set_interrupt() is used to setup an interrupt for a time in the past, then the callback is invoked right-away.
- minor improvement to the wording for the comment header for set_interrupt()
- minor white-space and comment improvements.
- introduce FUZZY_RTC_TICKS for comparsions
- updating the comment header for set_interrupt() with a note.
- rearranging some code so that state is tidied up before calling us_ticker_set_interrtupt()
- INVOKE_CALLBACK() is now a static inline instead of a macro.
- white space diff.
- fix for #832 Merge branch 'replaceAppTimer'
File Changes
- M libraries/mbed/common/us_ticker_api.c (6)
- D libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/Lib/app_common/app_timer.c (1160)
- M libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c (227)
Patch Links:
—
Reply to this email directly or view it on GitHub.
Martin Kojtal
Please remove the project header file, see Travis log : error for #include "projectconfig.h"
Martin Kojtal
We should inform users who reported problems with previous implementation about this update, they can test this.
Rohit Grover
@0xc0170 could you please review the code? Either you or Bogdan?
thanks.
I'll also connect with the previous reporters.
Martin Kojtal
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_COMPARE0_Msk;
> +}
> +
> +static __INLINE void rtc1_enableOverflowInterrupt(void)
> +{
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + NRF_RTC1->INTENSET = RTC_INTENSET_OVRFLW_Msk;
> +}
> +
> +static __INLINE void rtc1_disableOverflowInterrupt(void)
> +{
> + NRF_RTC1->INTENCLR = RTC_INTENSET_OVRFLW_Msk;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> +}
> +
> +static __INLINE void INVOKE_CALLBACK(void)
Why is the function in capital letters? This can clash with macros
Martin Kojtal
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + NRF_RTC1->INTENSET = RTC_INTENSET_COMPARE0_Msk;
> +}
> +
> +static __INLINE void rtc1_disableCompareInterrupt(void)
> +{
> + NRF_RTC1->INTENCLR = RTC_INTENSET_COMPARE0_Msk;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_COMPARE0_Msk;
> +}
> +
> +static __INLINE void rtc1_enableOverflowInterrupt(void)
> +{
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + NRF_RTC1->INTENSET = RTC_INTENSET_OVRFLW_Msk;
> +}
> +
> +static __INLINE void rtc1_disableOverflowInterrupt(void)
Please use c99 inline keyword rather than a macro
Martin Kojtal
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> +static __INLINE uint32_t rtc1_getCounter(void)
> +{
> + if (NRF_RTC1->EVENTS_OVRFLW) {
> + overflowBits += (1 << 24);
> + NRF_RTC1->EVENTS_OVRFLW = 0;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + }
> + return overflowBits | NRF_RTC1->COUNTER;
> +}
> +
> +/**
> + * @brief Function for handling the RTC1 interrupt.
> + *
> + * @details Checks for timeouts, and executes timeout handlers for expired timers.
> + */
> +void RTC1_IRQHandler(void)
Can we use NVIC_SetVector(), rather than redefining weak IRQ Handler? I have noticed this is used only for NRF HAL implementation, other targerts are using mbed provided vectors functionality.
Rohit Grover
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> +static __INLINE uint32_t rtc1_getCounter(void)
> +{
> + if (NRF_RTC1->EVENTS_OVRFLW) {
> + overflowBits += (1 << 24);
> + NRF_RTC1->EVENTS_OVRFLW = 0;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + }
> + return overflowBits | NRF_RTC1->COUNTER;
> +}
> +
> +/**
> + * @brief Function for handling the RTC1 interrupt.
> + *
> + * @details Checks for timeouts, and executes timeout handlers for expired timers.
> + */
> +void RTC1_IRQHandler(void)
Not with this change. There's a lot going on already.
Martin Kojtal
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + * boundary for the shelf life of nRF products when used with mbed; please
> + * accept my two to the power 56 apologies for that. But then life often
> + * forces one to live within constraints. What's the meaning of all this
> + * anyway?
> + *
> + * System uptime on an nRF is maintained using RTC's counter. We track the
> + * overflow count to extend the 24-bit hardware counter by an additional 32
> + * bits. This then forms the basis for system time.
> + * RTC_UNITS_TO_MICROSECONDS() converts this into microsecond units (in
> + * 64-bits). We then shave off the lower 32-bits of it and add in the
> + * incoming timestamp to get a mapping into a virtual 64-bit timestamp.
> + * There's one additional check to handle the case of wraparound for the
> + * 32-bit timestamp.
> + */
> + uint64_t timestamp64 = (RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter()) & ~(uint64_t)0xFFFFFFFF) + timestamp;
> + if ((us_ticker_read() > 0xF0000000) && (timestamp < 0x10000000)) {
Can you describe this check ?
Martin Kojtal
IS there a reason to call 2x us ticker , using 2 different invokes (RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter() and us_ticker_read() ) ?
Rohit Grover
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + * boundary for the shelf life of nRF products when used with mbed; please
> + * accept my two to the power 56 apologies for that. But then life often
> + * forces one to live within constraints. What's the meaning of all this
> + * anyway?
> + *
> + * System uptime on an nRF is maintained using RTC's counter. We track the
> + * overflow count to extend the 24-bit hardware counter by an additional 32
> + * bits. This then forms the basis for system time.
> + * RTC_UNITS_TO_MICROSECONDS() converts this into microsecond units (in
> + * 64-bits). We then shave off the lower 32-bits of it and add in the
> + * incoming timestamp to get a mapping into a virtual 64-bit timestamp.
> + * There's one additional check to handle the case of wraparound for the
> + * 32-bit timestamp.
> + */
> + uint64_t timestamp64 = (RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter()) & ~(uint64_t)0xFFFFFFFF) + timestamp;
> + if ((us_ticker_read() > 0xF0000000) && (timestamp < 0x10000000)) {
you're correct. I'm duplicating work here. I could reuse the value from RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter()) instead of calling us_ticker_read(). thanks.
Rohit Grover
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + * boundary for the shelf life of nRF products when used with mbed; please
> + * accept my two to the power 56 apologies for that. But then life often
> + * forces one to live within constraints. What's the meaning of all this
> + * anyway?
> + *
> + * System uptime on an nRF is maintained using RTC's counter. We track the
> + * overflow count to extend the 24-bit hardware counter by an additional 32
> + * bits. This then forms the basis for system time.
> + * RTC_UNITS_TO_MICROSECONDS() converts this into microsecond units (in
> + * 64-bits). We then shave off the lower 32-bits of it and add in the
> + * incoming timestamp to get a mapping into a virtual 64-bit timestamp.
> + * There's one additional check to handle the case of wraparound for the
> + * 32-bit timestamp.
> + */
> + uint64_t timestamp64 = (RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter()) & ~(uint64_t)0xFFFFFFFF) + timestamp;
> + if ((us_ticker_read() > 0xF0000000) && (timestamp < 0x10000000)) {
if ((us_ticker_read() > 0xF0000000) && (timestamp < 0x10000000)) {
the above check detects an attempt to set a timeout for a value which has wrapped around, but the attempt is being made at a time when the current RTC value when translated into microseconds hasn't wrapped around.
Martin Kojtal
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + }
> + return (overflowCount << 24) | NRF_RTC1->COUNTER;
> +}
> +
> +/**
> + * @brief Function for handling the RTC1 interrupt.
> + *
> + * @details Checks for timeouts, and executes timeout handlers for expired timers.
> + */
> +void RTC1_IRQHandler(void)
> +{
> + if (NRF_RTC1->EVENTS_OVRFLW) {
> + overflowCount++;
> + NRF_RTC1->EVENTS_OVRFLW = 0;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + }
Is this duplication? The overflow is handled in rtc1_getCounter(), which is invoked meeting some conditions below. Can this be reorganized?
counter = rtc1_getCounter();
if (NRF_RTC1->EVENTS_COMPARE[0] && us_ticker_callbackPending && ((int)(us_ticker_callbackTimestamp - counter) <= 0))
Martin Kojtal
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + nrf_delay_us(MAX_RTC_TASKS_DELAY);
> +}
> +
> +/**
> + * @brief Function for returning the current value of the RTC1 counter.
> + *
> + * @return Current RTC1 counter as a 32-bit value (even though the underlying counter is 24-bit)
> + */
> +static __INLINE uint32_t rtc1_getCounter(void)
> +{
> + if (NRF_RTC1->EVENTS_OVRFLW) {
> + overflowCount++;
> + NRF_RTC1->EVENTS_OVRFLW = 0;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + }
> + return (overflowCount << 24) | NRF_RTC1->COUNTER;
Shouldn't be overflowCount 8bit?
Rohit Grover
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + nrf_delay_us(MAX_RTC_TASKS_DELAY);
> +}
> +
> +/**
> + * @brief Function for returning the current value of the RTC1 counter.
> + *
> + * @return Current RTC1 counter as a 32-bit value (even though the underlying counter is 24-bit)
> + */
> +static __INLINE uint32_t rtc1_getCounter(void)
> +{
> + if (NRF_RTC1->EVENTS_OVRFLW) {
> + overflowCount++;
> + NRF_RTC1->EVENTS_OVRFLW = 0;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + }
> + return (overflowCount << 24) | NRF_RTC1->COUNTER;
overflowCount could be 8-bit, but keeping it 32-bit allows for a 56-bit virtual RTC clock, and protects us from time wraparound.
Rohit Grover
I'm using the following main.cpp to test my ticker for long periods of time.
#include "mbed.h" #include "iBeaconService.h" BLEDevice ble; Ticker ticker1; Ticker ticker2; DigitalOut led1(LED1); DigitalOut led2(LED2); void periodicCallback1(void) { led1 = !led1; /* Do blinky on LED1 while we're waiting for BLE events */ } void periodicCallback2(void) { led2 = !led2; /* Do blinky on LED2 while we're waiting for BLE events */ } const uint8_t uuid[] = {0xE2, 0x0A, 0x39, 0xF4, 0x73, 0xF5, 0x4B, 0xC4, 0xA1, 0x2F, 0x17, 0xD1, 0xAD, 0x07, 0xA9, 0x61 }; uint16_t majorNumber = 1122; uint16_t minorNumber = 3344; uint16_t txPower = 0xC8; int main(void) { ble.init(); ticker1.attach(periodicCallback1, 0.5); ticker2.attach(periodicCallback2, 0.25); iBeaconService ibeacon(ble, uuid, majorNumber, minorNumber, txPower); ble.setAdvertisingInterval(Gap::MSEC_TO_ADVERTISEMENT_DURATION_UNITS(1000)); /* 1000ms. */ ble.startAdvertising(); while(1) { ble.waitForEvent(); // allows or low power operation } }
Sissors
In libraries/mbed/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/us_ticker.c:
> + }
> + return (overflowCount << 24) | NRF_RTC1->COUNTER;
> +}
> +
> +/**
> + * @brief Function for handling the RTC1 interrupt.
> + *
> + * @details Checks for timeouts, and executes timeout handlers for expired timers.
> + */
> +void RTC1_IRQHandler(void)
> +{
> + if (NRF_RTC1->EVENTS_OVRFLW) {
> + overflowCount++;
> + NRF_RTC1->EVENTS_OVRFLW = 0;
> + NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk;
> + }
I think it does give a (small) chance on errors with really unlucky timing. When no interrupts can be handled (for example because it is in the Ticker interrupt routine), and between the check on the overflow flag and the reading of the timer it overflows it will use the new RTC value with the old overflow value.
As random example what I did once: https://github.com/mbedmicro/mbed/blob/master/libraries/mbed/targets/hal/TARGET_Freescale/TARGET_K20XX/us_ticker.c#L74. There are probably other options too.
Rohit Grover
Merged #932.