RTC timestamp, was not read atomic, which can lead to wrong timestamps. #160
Conversation
RTC timestamp, was not read atomic, which can lead to wrong timestamps. Signed-off-by: Moritz Scheuren <moritz.scheuren@systec-electronic.com>
|
I saw this PR just after writing a bunch of RTC handling code for another project so maybe it will be useful: Instead of returning an error, maybe it would be nicer to perform a second read and so always return consistent data? The RM says (in RM0385 29.3.8 Reading the calendar) that if the APB clock is >= 7x the RTC clock (which is very likely since RTC clock is usually 32kHz) then it's OK to just read TR and then DR: the hardware locks DR after you read TR to ensure you get consistent data. In fact, it's important to always read TR and then DR, otherwise the later read of TR will lock an old value of DR. But, even if the APB clock is extremely slow, you can always read TR, then DR, then TR, then DR, and use the first values if the two TRs match, or the second values if the TRs don't match (as also described in the RM). Seems friendlier than returning an error to the user who has no choice but to now re-read in a loop until it doesn't error. It looks like the driver should also wait for RSF to be set before taking values in case the user re-reads very soon afterwards too, and clear it after each read. Maybe it would be best to have a single |
Probably yes, I did it that way since the error type was already there.
You are right.
I agree, but I am unsure which one is better. Returning always a full consistent datetime would conflict a bit with the rtcc trait. Nevertheless, I currently think this would be the best option, and we rebuild the whole thing on the chrono crate. Also, that every get function sets the calendar clock to 24 hr format with |
|
It might also be worth checking the (revamped) time crate which works fine on no_std now and seems more currently maintained, though I'm not sure what the general backstory is between them. |
|
Thank you for your insight, @adamgreig. I like the idea of only returning a full date/time to ensure consistency. I don't know how whether it's worth diverging from the In any case, I don't have time to implement any of the suggestions (as usual), but I'm happy to review and merge pull requests, in case anyone wants to make further improvements here. |
* Removed rtcc in favor of time * Removed get functions in favor of a single get_datetime function * Takes into account registers synchronization flag * Always returns a valid time (or panics) See stm32-rs#160 (comment) > Instead of returning an error, maybe it would be nicer to perform a second read and so always return consistent data? > > The RM says (in RM0385 29.3.8 Reading the calendar) that if the APB clock is >= 7x the RTC clock (which is very likely since RTC clock is usually 32kHz) then it's OK to just read TR and then DR: the hardware locks DR after you read TR to ensure you get consistent data. In fact, it's important to always read TR and then DR, otherwise the later read of TR will lock an old value of DR. But, even if the APB clock is extremely slow, you can always read TR, then DR, then TR, then DR, and use the first values if the two TRs match, or the second values if the TRs don't match (as also described in the RM). Seems friendlier than returning an error to the user who has no choice but to now re-read in a loop until it doesn't error. > > It looks like the driver should also wait for RSF to be set before taking values in case the user re-reads very soon afterwards too, and clear it after each read. Maybe it would be best to have a single get_datetime() method that does the correct wait-for-RSF, read TR, read DR, clear RSF sequence, and parses the fields, and then convenience methods for reading just seconds/minutes/etc could call that and return the relevant bit. Or just always give the user a full consistent datetime to make sure they don't try and read each part separately and end up with inconsistent data that way anyway... Signed-off-by: Moritz Scheuren <moritz.scheuren@systec-electronic.com>
|
See #162. |
* Removed rtcc in favor of time * Removed get functions in favor of a single get_datetime function * Takes into account registers synchronization flag * Always returns a valid time (or panics) See stm32-rs#160 (comment) > Instead of returning an error, maybe it would be nicer to perform a second read and so always return consistent data? > > The RM says (in RM0385 29.3.8 Reading the calendar) that if the APB clock is >= 7x the RTC clock (which is very likely since RTC clock is usually 32kHz) then it's OK to just read TR and then DR: the hardware locks DR after you read TR to ensure you get consistent data. In fact, it's important to always read TR and then DR, otherwise the later read of TR will lock an old value of DR. But, even if the APB clock is extremely slow, you can always read TR, then DR, then TR, then DR, and use the first values if the two TRs match, or the second values if the TRs don't match (as also described in the RM). Seems friendlier than returning an error to the user who has no choice but to now re-read in a loop until it doesn't error. > > It looks like the driver should also wait for RSF to be set before taking values in case the user re-reads very soon afterwards too, and clear it after each read. Maybe it would be best to have a single get_datetime() method that does the correct wait-for-RSF, read TR, read DR, clear RSF sequence, and parses the fields, and then convenience methods for reading just seconds/minutes/etc could call that and return the relevant bit. Or just always give the user a full consistent datetime to make sure they don't try and read each part separately and end up with inconsistent data that way anyway... Signed-off-by: Moritz Scheuren <moritz.scheuren@systec-electronic.com>
No description provided.