This post is older than 2 years and might not be relevant anymore
More Info: Consider searching for newer posts

[bugreport] app_timer_stop() resets RTC1 counter when APP_TIMER_KEEPS_RTC_ACTIVE is set

My setup is as follows: nRF5 SDK 13.1 with s132 5.1.0 nRF52832_AA Keil 5.25pre

I use RTC1 counter for timestamping along with app_timer timers. My get_timestamp() function retrieves it's time value by calling app_timer_cnt_get() and then uses app_timer_cnt_diff_compute() to check how much time passed since it was called. Result is used to increment monotonous timer tick counter which is then processed to obtain timestamp:

  uint64_t get_timestamp()
  {
    static uint32_t last_ticks=0;
	static uint64_t timestamp_ticks=0;
	uint32_t curr_ticks=app_timer_cnt_get();

	timestamp_ticks+=app_timer_cnt_diff_compute(curr_ticks, last_ticks);
	last_ticks=curr_ticks;

	return timestamp_ticks;
  }

It is working well in few applications but I've noticed that when I use app_timer_stop() timestamp jumps by large value. It turns out that calling this function on last running timer resets RTC1 counter by calling timer_list_remove() which contains following code snippet:

// Timer is the first in the list
    if (p_previous == p_current)
    {
        mp_timer_id_head = mp_timer_id_head->next;

        // No more timers in the list. Reset RTC1 in case Start timer operations are present in the queue.
        if (mp_timer_id_head == NULL)
        {
            NRF_RTC1->TASKS_CLEAR = 1;
            m_ticks_latest        = 0;
            m_rtc1_reset          = true;
            nrf_delay_us(MAX_RTC_TASKS_DELAY);
        }
    }

IMHO this is a bug because setting APP_TIMER_KEEPS_RTC_ACTIVE=1 in sdk_config.h according to infocenter documentation should result in always running RTC1 which can be used 'for timestamping'. Introducing large and unpredictable jumps in counter values contradicts this.

As a workaround I've started always running recurring timer which does nothing. This works but it's not ideal since nRF will wake from sleep to process timeout events for this timer.

In attempt to have more reliable fix I've patched snippet above so it looks like this:

    // Timer is the first in the list
    if (p_previous == p_current)
    {
        mp_timer_id_head = mp_timer_id_head->next;

        // No more timers in the list. Reset RTC1 in case Start timer operations are present in the queue.
        if (mp_timer_id_head == NULL)
        {
//do not reset RTC counter when it's used for timestamp
#if (APP_TIMER_KEEPS_RTC_ACTIVE == 0)
            NRF_RTC1->TASKS_CLEAR = 1;
            m_ticks_latest        = 0;
            m_rtc1_reset          = true;
            nrf_delay_us(MAX_RTC_TASKS_DELAY);
#else
			m_rtc1_reset          = false;
#endif

        }
    }

I'd appreciate If someone from Nordic could take a look and check if this won't break something or propose different solution.

Parents
  • Hi,

    The SDK team is aware of the vulnerability, but they haven't looked thoroughly into it because they have been working on an entirely new implementation of the library. The new implementation is scheduled to be included in SDK 15.

  • As a temporary workaround it's fine but for the release I'm afraid it won't do.

    Per safety standards we must be compatible with such timer is considered a 'dead code' and should be removed. The software module which uses my timestamp function for debouncing and tracking time between interrupt events is used as a library outside of our development team.

    It's very likely that upstream developers will simply omit this timer either by mistake or thinking that it shouldn't be there since it does nothing. SDK patch which makes app_timer_cnt_get() robust would be a much better solution.

    I understand that is's holiday time but I'd really appreciate if someone could take a look at my patch when Christmas break ends.

Reply
  • As a temporary workaround it's fine but for the release I'm afraid it won't do.

    Per safety standards we must be compatible with such timer is considered a 'dead code' and should be removed. The software module which uses my timestamp function for debouncing and tracking time between interrupt events is used as a library outside of our development team.

    It's very likely that upstream developers will simply omit this timer either by mistake or thinking that it shouldn't be there since it does nothing. SDK patch which makes app_timer_cnt_get() robust would be a much better solution.

    I understand that is's holiday time but I'd really appreciate if someone could take a look at my patch when Christmas break ends.

Children
No Data
Related