drv_rtc_init can trigger interrupts before setting p_instance, causing a hardfault (fix included)

[Triffid Hunter]

Program received signal SIGTRAP, Trace/breakpoint trap.
HardFault_Handler () at ../../../../HAL/NRF52/HAL.cpp:122
122 __debugbreak();
(gdb) bt
#0 HardFault_Handler () at ../../../../HAL/NRF52/HAL.cpp:122
#1 <signal handler called>
#2 nrf_rtc_event_pending (event=event@entry=NRF_RTC_EVENT_OVERFLOW, p_reg=0x20010000) at /home/triffid/Projects/NRF/nRF5_SDK/modules/nrfx/hal/nrf_rtc.h:361
#3 evt_pending (event=event@entry=NRF_RTC_EVENT_OVERFLOW, p_instance=<optimized out>, p_instance=<optimized out>) at /home/triffid/Projects/NRF/nRF5_SDK/components/libraries/timer/drv_rtc.c:187
#4 0x00003180 in drv_rtc_overflow_pending (p_instance=p_instance@entry=0x0 <nrf_atfifo_get_free>) at /home/triffid/Projects/NRF/nRF5_SDK/components/libraries/timer/drv_rtc.c:289
#5 0x00002996 in rtc_irq (p_instance=0x0 <nrf_atfifo_get_free>) at /home/triffid/Projects/NRF/nRF5_SDK/components/libraries/timer/app_timer2.c:459
#6 <signal handler called>
#7 0x00002fe4 in drv_rtc_init (p_instance=p_instance@entry=0x20000010 <m_rtc_inst>, p_config=p_config@entry=0x2000ffe4, handler=handler@entry=0x298d <rtc_irq>) at /home/triffid/Projects/NRF/nRF5_SDK/components/libraries/timer/drv_rtc.c:104
#8 0x000028e6 in app_timer_init () at /home/triffid/Projects/NRF/nRF5_SDK/components/libraries/timer/app_timer2.c:538
#9 0x00001406 in init () at ../../../../HAL/NRF52/HAL.cpp:94

If I edit drv_rtc_init@drv_rtc.c:102 and move NRFX_IRQ_ENABLE a few lines down below m_cb[p_instance->instance_id].p_instance = p_instance;, the hardfault no longer occurs and my code works normally.

Why the interrupt is triggering immediately after being enabled I've no idea.

Fwiw, I'm using nRF5_SDK_17.0.2_d674dde which I believe is the latest release.

[ovrebekk]

Hi

Is there any chance the the RTC is being enabled before you initialize the app_timer library?

You have made sure the RTC in use is assigned to the app_timer, and nothing else?

I will forward your report to the software team, and see if they have some input on the matter. 

Best regards
Torbjørn

[Triffid Hunter]

My code should call app_timer_init before any RTC things, although it does use NRF_RTC1 for some other tasks but should set it up after app_timer_init is called - how do I discern which RTC the app timers are using?

I can check the call order with a debugger tomorrow if you like.

Is it ok to call app_timer_create before app_timer_init?

Either way, the suggested fix seems like sensible defensive programming that shouldn't break anything even if I am doing something weird that causes the relevant interrupt flag to be set before app_timer_init.

PS: Despite the presence of C++ in my project, I'm well aware of the C++ static initialization order fiasco and have taken several steps to control precisely when and in which order constructors are run.

[ovrebekk]

Hi

The app_timer module is set up to use RTC1, and if you use NRF_RTC1 for anything else this could lead to problems. You should avoid using this module when the app_timer module is in use. 

In general hardware peripherals such as the RTC or serial modules should be used either exclusively through the driver interface, or exclusively through direct register access. Combining the two methods should not be done, since the internal state of the driver might not match the state of the peripheral. 

Triffid Hunter said:

Is it ok to call app_timer_create before app_timer_init?

No, this should also be avoided.  It won't have any ill effect, but app_timer_create(..) will return NRF_ERROR_INVALID_STATE if you call it before the app_timer is initialized, and no timers will be created. 

Triffid Hunter said:

Either way, the suggested fix seems like sensible defensive programming that shouldn't break anything even if I am doing something weird that causes the relevant interrupt flag to be set before app_timer_init.

I can discuss it with the team, but it is still good to know why the problem occurs in your case. 

If there are some issues with the order of initialization, or there is some conflict in the way RTC1 is accessed, this could lead to other issues down the line, so it would make sense to look into this as well. 

Best regards
Torbjørn

[Triffid Hunter]

ovrebekk said:

The app_timer module is set up to use RTC1, and if you use NRF_RTC1 for anything else this could lead to problems. You should avoid using this module when the app_timer module is in use. 

I'll switch it over to another one then - although my RTC init code should run after app_timer_init, and my timers seem to work fine…

ovrebekk said:

No, this should also be avoided.  It won't have any ill effect, but app_timer_create(..) will return NRF_ERROR_INVALID_STATE if you call it before the app_timer is initialized, and no timers will be created. 

How curious, I'm calling app_timer_create from C++ constructors of static singletons via what I presume is __libc_init_array with an assert in case they fail (they don't and the timers work fine), yet app_timer_init is called from main() - perhaps g++ is organising a late construct for me or something? I'll have to investigate further.

ovrebekk said:

I can discuss it with the team, but it is still good to know why the problem occurs in your case. 

If there are some issues with the order of initialization, or there is some conflict in the way RTC1 is accessed, this could lead to other issues down the line, so it would make sense to look into this as well.

Cool, sounds like you can fix your end and I can fix mine, and everyone will be happier 

[Triffid Hunter]

I checked my project, and it turns out I had a 128× correction factor in my timer library to correct for my RTC library overriding the app_timer RTC settings.

Changing my RTC driver to RTC0 allowed me to remove that correction factor and everything's working great.

Also, g++ is doing a late construct on my singletons, but perhaps I'll move some things around to ensure that app_timer_create is always called after app_timer_init.

[ovrebekk]

Good to hear you got it working well

I will consider the case resolved then. 

[Triffid Hunter]

Yep lovely, do you already have the fix for the race condition in your internal NRF5 SDK release candidate?

Might be worth checking if a similar race exists in other drivers?

[ovrebekk]

Hi

I have registered the issue internally, but it is not up to me to decide if or when it will be fixed in the SDK. 

As we are moving more and more focus over to the new nRF Connect SDK we are trying to scale down work on the nRF5 SDK. 

One advantage of the new platform is that it is all based on public github repositories, so everyone can propose improvements and bug fixes

Best regards
Torbjørn

[Triffid Hunter]

nRF Connect SDK? Is that the nightmare mess I had to use when trialing the NRF91 LTE thingy?

Public git repos I can send pull requests to sounds lovely though!

[ovrebekk]

Hi 

Triffid Hunter said:

nRF Connect SDK? Is that the nightmare mess I had to use when trialing the NRF91 LTE thingy?

The very same, yes ;) 

This will be the shared platform for all our devices going forward, and we are working hard to ensure a smoother experience for anyone getting started with it. 

It is a very different beast from the nRF5 SDK though, so there is definitely a bit of a hurdle when migrating from one to the other. 

For the nRF52 series you can choose between the nRF5 SDK or nRF Connect SDK (NCS), but the nRF91 and nRF53 series is only supported in NCS. 

Best regards
Torbjørn