STM32: RTC No Longer Enabled by Reading the Time

[mattbrown015]

Description

I think this #8777 causes a problem! :-(

@jeromecoutant tried to sneak in a cheeky optimisation that, AFAICT, isn't important for the PR i.e. STM32 RTC : skip rtc_write if possible.

This change means the RTC doesn't get enabled when previously it did.

time in mbed_rtc_time.cpp does this:

time_t time(time_t *timer)
{
    _mutex->lock();
    if (_rtc_isenabled != NULL) {
        if (!(_rtc_isenabled())) {
            set_time(0);
        }
    }
    ...
}

But, if the RTC isn't enabled its value is probably 0 and set_time(0) will just return early without doing anything!

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug

[jeromecoutant]

OK
You propose to revert 9da5e48 ?

[mattbrown015]

I'm not sure, initially I just wanted to see if others agree it is a problem.

I quite like the optimisation and was wondering if time could do set_time(1) instead. But that's not STM code and therefore is probably much harder to change.

How about something like:

....
if (t == rtc_read() && rtc_enabled()) {
    return;
}
...

But now it's not so simple. :-(

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-351

[kjbracey]

I don't particularly like the "optimisation" - it's costing code space, and runtime for the sensible case.

If any application thinks they might be making a "null adjustment", and want to avoid doing it in case it disrupts the tick phase, they should be doing their own check, as this behaviour isn't specified by the HAL API or set_time itself, so is not portable. And for that you might even want a bit of hysteresis - some code I worked on this week avoided adjustments unless by at least 2 seconds.

The HAL API requires that rtc_write starts the timer counting. If you do really want to have this "optimisation", then I think check should be if (rtc_isenabled() && t == rtc_read()) { return; }

(Oops, @mattbrown015 already said that...)

[mattbrown015]

I think I've started leaning towards just reverting.

rtc_write is a pretty heavyweight function and it seems a waste if it isn't really changing the time but, on the other hand, no one expects dealing with time to be lightweight!

In our app we would only set the time in response to a command from an operator and handling these commands is not part of main control flow (if you see what I mean).

[kjbracey]

But presumably someone must set the time at some time, so the rtc_write is always going to be in the build, if it's ever adjustable. Unless the setup is factory-only or in a bootloader?

And once it is set and running, that time() call won't actually call it, because rtc_isenabled() should be saying true.

[mattbrown015]

We get the time using time in the main control flow. As we know the first call to time after a cold boot will call set_time and enable the RTC.

With no further intervention the device will just run and all the reported times will be circa 1970.

If the operator wants to send a command to make the time more reasonable they can.

The RTC remains enabled during a warm boot so in our case the RTC will only go back to 0 if the battery is removed.

So set_time is only called after a cold boot or operator intervention.