Add rtc module #93

smedellin90 · 2020-04-16T22:55:07Z

Added rtc module, with supporting helper functions in modules rcc.rs and time.rs.
It's my first type doing a PR, so apologies if the commits are a mess.

Sh3Rm4n · 2020-04-17T19:35:37Z

Hey! :) Thank you for this PR, I'll have a look in the following days

Sh3Rm4n

I would love to see an example like others in src/examples and a bit more documentation on the public functions.

Don't worry about the commits too much. If needed this can be rebased later :)

I always like to take inspirations of other implementations of HAL abstractions. Have you looked at the implementation from the stm32f1xx-hal

Thank's for the PR again. I hope I didn't overdo it with my comments. Feel free to ask. And take the review with a grain of salt, as I did not check, if this is working on hardware or if the configuration / initialization complies to the reference manual.

src/rtc.rs

src/time.rs

src/rtc.rs

src/rcc.rs

smedellin90 · 2020-04-28T02:00:19Z

@Sh3Rm4n ,
Much appreciate the feedback! Will be going down the list

Co-Authored-By: Fabian <[email protected]>

Sh3Rm4n · 2020-05-06T22:41:29Z

BTW I found this crate: https://crates.io/crates/rtcc at rust-embedded/embedded-hal#207. Maybe this is useful for you, I like the idea to implement these traits for a higher level rtc access.

Maybe it is possible to change set_time and get_time to be as raw as reasonable, and build upon these traits to give a higher functionality?

By the way, I submitted a PR which documents the rtc periphery. This could be useful for this PR or a following one for the next stm32f3 release (v0.12.0)

smedellin90 · 2020-05-26T21:37:13Z

@Sh3Rm4n , I've made changes and used the rtcc traits you previously mentioned. Is this what you had in mind? the rtc function within the rtc module is still incomplete. Still thinking about how the user should program clock speed depending on the clock source.

smedellin90 · 2020-06-05T20:26:37Z

I applied an cfgRTC struct similarly to what is done in stm32l4xx-hal.

David-OConnor · 2020-07-15T22:45:20Z

Thoughts on merging this now? We can clean up details later if they arise.

David-OConnor · 2020-07-15T23:52:36Z

Could you please provide an example? Eg, how would we modify this to make it work? Thank you.

    let mut rtc_ = rtc::Rtc::rtc(
        dp.RTC,
        &mut rcc.apb1,
        &mut dp.RCC.bdcr,
        &mut dp.RCC.cr,
        &mut dp.RCC.csr,
        rtc::cfgRTC::default(),
    );

teskje

I have looked mostly on high-level design and coding style.

I'll also look at the actual RTC functionality once I've had time to become more familiar with how it works. It would be helpful if you could add an example (see the examples folder) so users (and me) can see how this feature is supposed to be used.

teskje · 2020-07-19T10:59:33Z

Cargo.toml

@@ -24,6 +24,7 @@ cortex-m-rt = "0.6"
 embedded-hal = "0.2"
 nb = "0.1"
 stm32f3 = "0.11"
+rtcc = "0.2.0"


Since specifying versions in Cargo.toml defaults to the caret operator (https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html), this is the same as "^0.2.0", which actually allows any patch version (i.e. 0.2.x). Therefore, it is less confusing to just specify rtcc = "0.2" here. We use this pattern for the other dependencies too.

teskje · 2020-07-19T11:38:16Z

src/rtc.rs

+        match &cfg_rtc.src {
+            RTCSrc::LSI => Rtc::enable_lsi(csr),
+            RTCSrc::HSE => Rtc::enable_hse(cr, false),
+            RTCSrc::LSE => Rtc::enable_lse(bdcr, false),


So this is the only reason we need to expose the CSR and CR registers from RCC in the first place, right? Like I described in my other comment, at least CR is problematic. Exposing CSR seems fine, but I'd still like to avoid it if not necessary.

That said, how important is it that we support all the clock sources for the RTC? Do users really care about them all? Or would it be enough to only support one of them? We need BDCR anyway for other configurations, so I would suggest always using the LSE and not supporting the other clock sources. Then we wouldn't need access to CR and CSR at all.

That's also the approach used by the stm32f1xx-hal: https://github.com/stm32-rs/stm32f1xx-hal/blob/master/src/rtc.rs#L54

That is correct, the reason for exposing CSR and CR is to allow clock source selection. The LSE is not included on the STM32F303 Discovery board, so I initially wanted to make the rtc module flexible and allow the user to choose. That being said, if I had to choose one for support, it would be the LSE. I agree.

teskje · 2020-07-19T11:43:41Z

src/rtc.rs

+                w.prediv_s()
+                    .bits(cfg_rtc.prediv_s)
+                    .prediv_a()
+                    .bits(cfg_rtc.prediv_a)


This formatting is very unreadable. It looks like you do three modifications on prediv_s, but actually you do only one and then another one on prediv_a. I know rustfmt isn't helpful here, but you can restructure your code a bit to fix this:

regs.prer.write(|w| unsafe { w.prediv_s().bits(cfg_rtc.prediv_s); w.prediv_a().bits(cfg_rtc.prediv_a) });

This also applies to the other instances of register access chaining below.

teskje · 2020-07-19T11:48:25Z

src/rtc.rs

+
+    fn enable_lsi(csr: &mut CSR) {
+        csr.csr().write(|w| w.lsion().set_bit());
+        while csr.csr().read().lsirdy().bit_is_clear() {}


Most of the PAC registers come with more descriptive methods than the generic ones ({set,clear}_bit, bit_is_{set,clear}. In this case those would be:

csr.csr().write(|w| w.lsion().on()); while csr.csr().read().lsirdy().is_not_ready() {}

Apart from improving readability, they also sometimes prevent bugs (e.g. sometimes you clear an interrupt flag by setting a bit). So I would suggest using them everywhere possible.

teskje · 2020-07-19T11:51:29Z

src/rtc.rs

+    }
+
+    fn enable_lsi(csr: &mut CSR) {
+        csr.csr().write(|w| w.lsion().set_bit());


I'm pretty sure you should use modify here instead of write. write sets all register contents except the ones you set in the closure to their reset values. Which means if you have already configured part of a register and don't want to lose this configuration, you should always use modify.

There are a couple other instances of write use in this file. Please check if they all work as intended.

My limited knowledge points to the same. The calls to write followed by unsafe blocks are probably correct, but the others should be modify. (?)

Thank you for catching this. I'll look through the code to see if I confused it anywhere else.

teskje · 2020-07-19T12:50:56Z

src/rtc.rs

+use rtcc::{Datelike, Hours, NaiveDate, NaiveDateTime, NaiveTime, Rtcc, Timelike};
+
+/// Invalid input error
+// #[derive(Clone, Copy)]


Did you forget to remove this?

teskje · 2020-07-19T12:51:18Z

src/rtc.rs

+
+/// RTC clock input source
+#[derive(Copy, Clone, Debug, PartialEq)]
+pub enum RTCSrc {


Check out this RFC for naming conventions in Rust: https://github.com/rust-lang/rfcs/blob/master/text/0430-finalizing-naming-conventions.md

Notably acronyms count as one word for the CamelCase notation, so did should be RtcSrc. But there is no need to obfuscate the name just to save three characters here, so RtcSource would be what I prefer.

teskje · 2020-07-19T12:51:35Z

src/rtc.rs

+pub enum RTCSrc {
+    LSE = 0b01,
+    LSI = 0b10,
+    HSE = 0b11,


I don't think to use these bit patterns anywhere? If so, I would just leave them out.

teskje · 2020-07-19T12:51:56Z

src/rtc.rs

+
+/// RTC configuration struct
+#[derive(Copy, Clone, Debug, PartialEq)]
+pub struct cfgRTC {


This name also doesn't follow the naming conventions. How about RtcConfig?

Will not be a problem with only LSE support

teskje · 2020-07-19T12:52:24Z

src/rtc.rs

+        cfgRTC {
+            src: RTCSrc::LSI,
+            prediv_s: 311,
+            prediv_a: 127,


You discussed this with @Sh3Rm4n before. Like him I think there should be a reference to the relevant manual and section you got these magic numbers from documented.

teskje · 2020-07-19T13:02:30Z

Thoughts on merging this now? We can clean up details later if they arise.

I would rather have a clean initial implementation, since anything else will fall back on us by making this crate harder to maintain. Hoping that someone will clean up later doesn't work out in my experience, because everyone prefers to work on new features instead ;)

If something should be merged faster, then I think the way to go is by merging a (good) MVP first and adding more features later.

David-OConnor · 2020-07-19T13:11:39Z

I'm working off a variant of this and tweaking as I go. I'll let you know what I come up with. Excited to get this cleaned up and merged.

David-OConnor · 2020-07-19T13:17:15Z

Do you think RTC wakeup code like this should go in this RTC module, or in an interrupt module? Note that the artc var there is dp.RTC, ie what this module calls the regs field.

It's not working yet, but I think I'm almost there, since I've done everything I can find in the ref man and rtc man.

Thoughts on making reg public? ie using the Rtc struct then blocks you from (mutably borrowing) the RTC peripheral elsewhere.

teskje · 2020-07-19T14:57:04Z

I can't say much to RTC specifics because I've not read up to that yet.

What I can say is that usually how it works is that a HAL crate provides abstractions over peripherals. When you create an instance of such an abstraction, you have to give it ownership over the peripheral registers. The abstraction basically wraps these registers and provides a safe API for them. Peripheral abstractions interact with one another, but they don't access the registers of another peripheral directly, only the safe methods.

So in this case, the relevant peripheral abstraction would be Rtc. When instantiating it, you have to give it the RTC register, at which point Rtc has exclusive access over it. Rtc is responsible for managing that peripheral now. It won't give out the RTC register for other code to modify, since that would make it impossible (or at least very hard) to provide a safe API. But that's okay, since other components shouldn't have a need to access the register directly, they can always call methods on the Rtc instead.

So no, I don't think we should provide public access to RTC. What we could provide is an Rtc::free method that consumes the Rtc and gives its owned registers back in the process. I think this is good practice to have for all peripheral abstractions (see https://docs.rust-embedded.org/book/design-patterns/hal/interoperability.html#c-free) but I don't think we're doing that consistently yet.

As a final note, it is always possible to get a register using unsafe code, e.g.: let rtc = unsafe { &*RTC::ptr() };. So in a pinch you can add functionality missing from the HAL that way. Of course, the long-term solution should be adding the missing functionality to the HAL.

David-OConnor · 2020-07-19T15:06:00Z

That's a 2-for-1 response: The interrupt (wakeup, alarm, tamper etc) functionality needs to be baked into this module, if it's going to control that periph. Eg the code I linked above makes heavy use of it.

Btw, that's a great pointer, on the RTC::ptr() example! Do you know how to modify this line to use raw pointers? Required to enable wakeup from standby. I'm using a custom fork to make that field pub: apb1.enr().modify(|_, w| w.pwren().set_bit());

David-OConnor · 2020-07-19T18:32:35Z

Btw, I'm glad this uses the rtcc module. It was a recent addition to the HAL ecosystem that fits the role of abstracting RTC to any architecture. I think it should be officially in embedded-hal.

Thoughts on adding the setup_rtc_wakeup fn in my code above directly to this PR, since it uses the RTC periph this owns?

Sh3Rm4n · 2020-07-19T20:36:27Z

Btw, I'm glad this uses the rtcc module. It was a recent addition to the HAL ecosystem that fits the role of abstracting RTC to any architecture. I think it should be officially in embedded-hal.

Off-topic: Regarding rtcc as a potential candidate for the embedded-hal. I wonder if we should feature gate these crates until there is official support in the embedded-hal to avoid breaking changes or should we just go on and if we switch the implementation back to embedded-hal, if the trait is up streamed and denote it as a breaking change via semver.
This is what I had in mind, when suggesting it for the CAN #100 PR, implemented here

eldruin · 2020-07-20T08:12:08Z

Hi, nice to see rtcc being used for an MCU HAL implementation as well :). Please let me know if it fits well and if there are things that could be improved.

I do not want to hold it back but I do not think it will be merged into embedded-hal any time soon if at all so I would not hold my breath on that. I think it would be interesting to see adoption among HAL implementations for several MCUs grow first to iron out any difficulties and see of how it fits.

Anyway feel free to express your opinion at rust-embedded/embedded-hal#207 as well.

smedellin90 · 2020-08-15T07:58:03Z

Do you think RTC wakeup code like this should go in this RTC module, or in an interrupt module? Note that the artc var there is dp.RTC, ie what this module calls the regs field.

It's not working yet, but I think I'm almost there, since I've done everything I can find in the ref man and rtc man.

Thoughts on making reg public? ie using the Rtc struct then blocks you from (mutably borrowing) the RTC peripheral elsewhere.

@David-OConnor , I believe it makes the most sense to apply the RTC wakeup code in a separate EXTI module, as it is done here .

David-OConnor · 2020-08-15T09:12:56Z

Note that most people in the related issue on this repo favor interrupt code to be built into the pins as methods.

teskje · 2020-09-08T17:32:23Z

Closing in favor of #136.

Scott Medellin added 12 commits April 14, 2020 16:51

Added bkp struct to Rcc struct

c113526

Added Rcc helper functions for Rtc support

5a7723a

Rcc helper functions for Rtc

b24c4f9

Add rcc helper functions for rtc module

9175483

Add rcc helper functions for future rtc module

7cf3829

Fix format

476a92c

Fix more formatting

44fe6fc

ran 'cargo fmt' to format file

fb505ec

Added rtc module. Added functions to time module

1422683

Removed unused 'perform_write' function from rtc module

98c3f24

Added lib.rs to changes

c629df9

Removed unused dependency 'core::fmt' from time.rs

1533749

smedellin90 changed the title ~~Rcc helper functions for rtc~~ Add rtc module Apr 17, 2020

Sh3Rm4n self-requested a review April 17, 2020 19:34

Sh3Rm4n requested changes Apr 27, 2020

View reviewed changes

smedellin90 and others added 2 commits April 27, 2020 19:01

Update src/rtc.rs

144588e

Co-Authored-By: Fabian <[email protected]>

Update src/rtc.rs

4c6d8d6

Co-Authored-By: Fabian <[email protected]>

implement rtcc traits

91f831b

smedellin90 and others added 3 commits May 26, 2020 19:29

Merge branch 'master' into Rcc-helper-functions-for-Rtc

4777f6d

move rtc initialization functions from rcc.rs to rtc.rs

158632a

move rtc initialization functions from rcc.rs to rtc.rs

ad355d6

smedellin90 requested a review from Sh3Rm4n June 5, 2020 20:27

teskje requested changes Jul 19, 2020

View reviewed changes

Merge branch 'master' into Rcc-helper-functions-for-Rtc

348e4a9

This was referenced Sep 3, 2020

Added DAC with basic features #133

Closed

Rtc v2 #136

Merged

teskje closed this Sep 8, 2020

David-OConnor mentioned this pull request Sep 21, 2020

Add clock HSE bypass #142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rtc module #93

Add rtc module #93

smedellin90 commented Apr 16, 2020 •

edited

Loading

Sh3Rm4n commented Apr 17, 2020

Sh3Rm4n left a comment

smedellin90 commented Apr 28, 2020 •

edited

Loading

Sh3Rm4n commented May 6, 2020 •

edited

Loading

smedellin90 commented May 26, 2020

smedellin90 commented Jun 5, 2020

David-OConnor commented Jul 15, 2020 •

edited

Loading

David-OConnor commented Jul 15, 2020 •

edited

Loading

teskje left a comment

teskje Jul 19, 2020

teskje Jul 19, 2020

smedellin90 Aug 15, 2020 •

edited

Loading

teskje Jul 19, 2020

teskje Jul 19, 2020

teskje Jul 19, 2020

David-OConnor Jul 19, 2020

smedellin90 Aug 15, 2020

teskje Jul 19, 2020

teskje Jul 19, 2020

teskje Jul 19, 2020

teskje Jul 19, 2020

smedellin90 Aug 15, 2020

teskje Jul 19, 2020

teskje commented Jul 19, 2020

David-OConnor commented Jul 19, 2020

David-OConnor commented Jul 19, 2020 •

edited

Loading

teskje commented Jul 19, 2020

David-OConnor commented Jul 19, 2020 •

edited

Loading

David-OConnor commented Jul 19, 2020

Sh3Rm4n commented Jul 19, 2020 •

edited

Loading

eldruin commented Jul 20, 2020

smedellin90 commented Aug 15, 2020

David-OConnor commented Aug 15, 2020 •

edited

Loading

teskje commented Sep 8, 2020

Add rtc module #93

Add rtc module #93

Conversation

smedellin90 commented Apr 16, 2020 • edited Loading

Sh3Rm4n commented Apr 17, 2020

Sh3Rm4n left a comment

Choose a reason for hiding this comment

smedellin90 commented Apr 28, 2020 • edited Loading

Sh3Rm4n commented May 6, 2020 • edited Loading

smedellin90 commented May 26, 2020

smedellin90 commented Jun 5, 2020

David-OConnor commented Jul 15, 2020 • edited Loading

David-OConnor commented Jul 15, 2020 • edited Loading

teskje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smedellin90 Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teskje commented Jul 19, 2020

David-OConnor commented Jul 19, 2020

David-OConnor commented Jul 19, 2020 • edited Loading

teskje commented Jul 19, 2020

David-OConnor commented Jul 19, 2020 • edited Loading

David-OConnor commented Jul 19, 2020

Sh3Rm4n commented Jul 19, 2020 • edited Loading

eldruin commented Jul 20, 2020

smedellin90 commented Aug 15, 2020

David-OConnor commented Aug 15, 2020 • edited Loading

teskje commented Sep 8, 2020

smedellin90 commented Apr 16, 2020 •

edited

Loading

smedellin90 commented Apr 28, 2020 •

edited

Loading

Sh3Rm4n commented May 6, 2020 •

edited

Loading

David-OConnor commented Jul 15, 2020 •

edited

Loading

David-OConnor commented Jul 15, 2020 •

edited

Loading

smedellin90 Aug 15, 2020 •

edited

Loading

David-OConnor commented Jul 19, 2020 •

edited

Loading

David-OConnor commented Jul 19, 2020 •

edited

Loading

Sh3Rm4n commented Jul 19, 2020 •

edited

Loading

David-OConnor commented Aug 15, 2020 •

edited

Loading