stm32_common/periph/rtc: current implementation broken/poor accuracy

[MichelRottleuthner]

Description

In the current implementation rtc_unlock() doesn't, like the name indicates, only unlocks the rtc but also enters init mode. Looking at the datasheet this is not a really good idea. Refering to "Calendar initialization and configuration":

1. Set INIT bit to 1 in the RTC_ISR register to enter initialization mode. In this mode, the
calendar counter is stopped and its value can be updated.
(...)
5. (...) Exit the initialization mode by clearing the INIT bit. The actual calendar counter value is
then automatically loaded and the counting restarts after 4 RTCCLK clock cycles.

So every operation that calls rtc_unlock() will delay the rtc by the time the operation itself takes, plus at minimum the 4 RTCCLK cycles. This is for example true for rtc_set_alarm(). So if you happen to use the alarm feature to wake up every few seconds your time reference will become borked up pretty quickly.

The next problem comes up when using the rtc in combination with pm. This is only visible when your application makes use of power modes that issue a system reset when exiting this mode (namely Shutdown mode in my case). On reset rtc_init(), which is only conditioned by MODULE_PERIPH_RTC, is always called if the rtc is available. Since this is part of periph_init() it also doesn't help to disable auto_init.

One thing that was really nasty to debug here, came from the fact that rtc_init() isn't actually resetting the RTC_DR and RTC_TR registers. So testing if the rtc works together with pm looks "working ok" if you only observe the system over a short period of time, because the rtc_init() will effectively only loose the SSR (sub second regiser).
What do you think about using the INITS flag in RTC_ISR to only init the rtc if it wasn't already initialized before?

The fact that the current tests/periph_rtc doesn't highlight any of the problems brings me to the conclusion that the test needs improvement.

I'd like to exchange opinions on the following thoughts:
IMO It's absolutely necessary to add some quantifiable output on the accuracy. For example sntp could be used as a reference here.
This should also be tested with pm&rtc - particularly since rtc is a feature that will often be combined with low power modes.
Another thing that came to my mind: is it intentional that the rtc api doesn't provide functions to perform calibration?

Steps to reproduce the issue

taking tests/periph_rtc/ as starting point:
add USEMODULE += pm_layered to tests/periph_rtc/Makefile
add #include "pm_layered.h" to main.c
replace main() with this:

int main(void)
{
    struct tm time;
    rtc_get_time(&time);

    print_time("\nwoke up at: ", &time);

    time_t time_sec = mktime(&time);
    time_sec += 1;
    struct tm *alarm_time = gmtime(&time_sec);

    rtc_set_alarm(alarm_time, cb, NULL);

    print_time("setting alarm to: ", alarm_time);

    pm_unblock(0);
    pm_unblock(1);
    pm_unblock(2);
    pm_unblock(3);

    return 0;
}

Expected results

Accuracy near the oscillator specifications (e.g. <20 ppm on nucleo-l476 in my case).

Actual results

Accuracy dependent on application beaviour (duty cycle etc.) and for some use cases in fact abysmal.
Watching pyterms time stamps for the above code gives an error of ~220ms per minute. This translates to 3666 ppm.
Fun fact: this is more than seven times greater than the maximum this chips calibration feature could compensate, if it were implemented ;)

Versions

current master

[vincent-d]

One thing that was really nasty to debug here, came from the fact that rtc_init() isn't actually resetting the RTC_DR and RTC_TR registers. (...) What do you think about using the INITS flag in RTC_ISR to only init the rtc if it wasn't already initialized before?

+1. Resetting RTC_DR and RTC_TR at boot time is not a good idea, but checking if the RTC has been initialized or not before going through rtc_init sounds like a gound idea. Though I don't know if it should be done inside rtc_init or outside (i.e. in cpu_init)

Did you already work on fixes for these issues?

[MichelRottleuthner]

Did you already work on fixes for these issues?

Currently I've fixed it by guarding everything in rtc_init with if(!(RTC->ISR & RTC_ISR_INITS)) exept for the EXTI configuration which is lost after shutdown. And additionally replacing rtc_unlock() in rtc_set_alarm with

stmclk_dbp_unlock();
RTC->WPR = WPK1;
RTC->WPR = WPK2;

There may be more (an improved test would show), but with this I can at least get ~12ppm, with calibration even below that.
But as you already said, I also thought that simply adding it to rtc_init() is a thing to discuss.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[smlng]

This sound related to a problem I currently have when using rtc_set_alarm, i.e. it does not fire anymore after a longer interval. In my setup I basically use an extended version of our examples/lorawan to periodically send data via LoRaWAN, for the interval I use rtc_set_alarm as in the example. However, after a couple of minutes or hours the application stops and no data is send anymore.

I added some LED toggling to see where my app is and according to this it hangs right in or after rtc_set_alarm. Same as in the example the alarm callback should send a message to the main thread, which never arrives.

@MichelRottleuthner do you plan to open a PR with your fixes, or how do we proceed - to me this seems a severe problem. Specifically for use-cases as you described, i.e. RTC with low power stuff.

[MichelRottleuthner]

I almost forgot this one, good to have that verbose description^^ Yeah we should just open a PR.
@smlng what is your opinion on how to handle the "don't init if already initialized"-case discussed above?

For handling it from within the init function I see the following points:

+no interface change needed (the current documentation is not violated)
+kind of what one would expect from an RTC peripheral
-doesn't allow you to force-init the RTC anymore (not sure why you would need that with our currently
very limited API though)