rtcGetTime(): returns uninitialized memory Topic is solved

[lilvinz]

Hi Giovanni,

i am tracking down an off-by-many bug with the hours field of fatfs timestamps.
In that process i discovered that in get_fattime() there seems to be a use of uninitialized memory bug.
In that function timespec is being allocated on stack and passed as is into rtcGetTime().
rtcGetTime() does not alter the timespec->dstflag field. Thus it is being used uninitialized for further calculations in rtcConvertDateTimeToFAT().
So either this is a bug by not clearing timespec before passing it into rtcGetTime() or it is a bug within rtcGetTime() not setting timespec->dstflag to a defined state.
IMHO it is the latter.

Cheers

Vinz

[Giovanni]

Hi,

Moving into bug reports, thanks.

Giovanni

[Giovanni]

Hi,

I agree, it should be the driver putting it in a known state.

The easiest way is to put it to zero, alternatively it should be stored in some non-volatile bit into RTC and get it from there.

I am open to suggestions.

Giovanni

[lilvinz]

I see a couple of ways with its own pros and cons each.
The following are my thoughts:

Currently the isdst flag is part of the HAL but at least the STM32 RTCv1 and RTCv2 do not have support for that.
If there are no other ports supporting this flag, it should be dropped. In that way the user sees immediately that it is his own responsibility to handle dst.
If there are other ports supporting isdst, the question rises how to handle it for ports which don't. This leads me to the question how missing features of certain ports are handled for other HAL drivers. The handling of missing features should be as consistent as possible among the HAL.

Another way would be to emulate the behaviour of missing isdst feature in software within the HAL. That leads to the question of what exactly the RTC should do with isdst. Is it only storing the flag or shall there be some automatic switching depending on time zone settings or something like that.
This route can become very complex and even might change over years because of regulatory changes. (IIRC russia dropped daylight savings time recently)

Altogether i would tend to drop isdst from the HAL completely as it is connected to time zone and localization which has to be done by the user anyway.

Cheers

Vinz

PS: "user" means user of the HAL

[steved]

Currently the isdst flag is part of the HAL but at least the STM32 RTCv1 and RTCv2 do not have support for that.

Not entirely - looking at RM0091 (DocID018940 Rev 7) page 572 (for 32F0x - RTC2) there is the BKP bit which is intended to memorise whether DST is set. So this bit could be copied into the isDst flag. For anyone concerned about DST I feel this is valuable support, since it keeps all the time-related information together and backed up.

I agree that managing the transition in and out of DST needs to be done outside the HAL, as with other locale-related information.

I'm sure some helper routines to manage the DST transitions would be useful for some.

Its also worth bearing in mind that in some applications the RTC always reflects UTC (or maybe a fixed offset from UTC) with DST and locale-related adjustments being applied when reading or displaying the time.

[Giovanni]

I agree with Steved, the driver should at least store the flag, handling would be responsibility of application. Automatic handling is impossible considering it is subject to political whims.

I will make the LLD export a macro RTC_SUPPORTS_DST if the bit is handled, if undefined then drivers will always return it to zero.

Giovanni

[Giovanni]

Bumped.

[Giovanni]

Hi,

Fixed as bug #799.

Now the DST flag is stored/retrieved from the BKP bit of the CR register.

Giovanni