Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTC: Rewritten #162

Merged
merged 1 commit into from
Dec 10, 2021
Merged

RTC: Rewritten #162

merged 1 commit into from
Dec 10, 2021

Conversation

systec-ms
Copy link
Contributor

@systec-ms systec-ms commented Dec 10, 2021

  • 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)
  • Removed disgusting 12-hour format support (Which wasn't implemented consistent anyway)

See #160 (comment) from @adamgreig.

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...

* 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>
Copy link
Contributor

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @systec-ms. Looks good to me!

One thing I'm not sure of is the removal of rtcc. No idea how widely it is used. But if anyone cares, support can be re-added later.

@hannobraun hannobraun merged commit d632a5c into stm32-rs:master Dec 10, 2021
@systec-ms
Copy link
Contributor Author

Thanks also, true, should be quite easy to re-add, if someone cares or rtcc changes the trait definition.

@systec-ms systec-ms deleted the rewrite_rtc branch December 10, 2021 11:58
@burrbull
Copy link
Contributor

@systec-ms Are you interested in porting improvements to related hals like f4, f3, l0?

@oyren
Copy link

oyren commented Jan 3, 2022

@burrbull, this is on my to do list, but I don't know when I'll have the time.

(This is @systec-ms private account).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants