Implement RTC

[mattico]

The RTC is a bit strange due to the backup power domain. I tried varying amounts of integration with the standard RCC, REC, PWR methods, but the implementation and semantics conflicted. Instead I added Backup to manage the backup power domain. Right now it's just the RTC but hypothetically it could help manage the backup SRAM and the LSE.

I started out with a minimal Date/Time implementation but found myself implementing more and more functionality so I replaced it with the time crate. It has a no_std mode but always requires an allocator. This isn't an issue for me but I might re-introduce the simple Date/Time and maybe genericize functions that take one of those as an argument.

I moved to the chrono crate since it doesn't require allocation.

Interested in any feedback you have.

Blocked on:

• time from crates.io doesn't support no_std time-rs/time#280
• RTC HSE divider is configurable according to CubeMX
• stm32h7 (H7: Document RTC Registers stm32-rs#446, H7: Array RTC_BCKP Registers stm32-rs#445)

[hargoniX]

Since you are using a custom, self made enum to represent possible clock sources for the RTC:

Considering that many peripherals such as for example the ADC do allow multiple clock sources in theory (as of now we limited it to one, but the clock source is actually connected to a MUX we could configure if we so desired inside the chip) would it possibly make sense to have an enum (or a set of structs which implement some trait) which represents clocks we have available to us in general? Since I feel like if we were to actually fully expose the H7 as a Rust interface this is definitely a thing we need to have and reimplementing it over and over again wouldn't be beneficial.

An basic idea without thinking it too much through in depth:

1. Build structs that represent all of our clocks such as the LSI, LSE, HSI etc. etc. These structs would carry information such as the frequency but also more specialised things such as for example the bypass thing for LSI.

2. Possibly implement a way to represent divided clocks as structs, this could be easily done once const generics are a thing actually since we can then simply have a type Divided<CLOCK, DIV> with CLOCK being one of the clock structs mentioned from 1 and DIV being the divider as a const generic. However we don't actually have to wait for const generics since most clocks I've seen so far are usually divided by a multiple of 2 so we can probably get away with providing zero sized PhantomData structs for 2,4,8,16,32,64 and maybe 128.

3. For peripherals that do allow to configure which clock source is their input simply have a function argument of the type T where T is constrainted to implement a marker trait which marks a certain (possibly divided clock source) as usable for this peripheral.

This has the advantage of having a centralized representation of clocks we can use across the chip. Furthermore the Rust compiler as well as docs.rs can still easily tell the user which clocks are allowed for this peripheral:

docs.rs: Check who implementes the peripheral specific marker trait
rustc: If the type does not match, rustc will usually print implementations of the trait it has found for the user so they get docs as a compiler error so to speak.

The disadvantage is of course that we increase our type signature, function signatures etc. by a bit which is not at all an issue for me but I have talked to people that would rather prefer if we did not do that.

Any thoughts on this? Or am I just totally overthinking it at this point xD?

[mattico]

Since you didn't mention it, I'll point you to our existing facilities for changing peripheral clocks: https://docs.rs/stm32h7xx-hal/0.7.1/stm32h7xx_hal/rcc/rec/struct.Cec.html#method.kernel_clk_mux (I didn't know/remember this existed until I started working on the RTC). Looking at the implementing macro you can see that many peripherals have no clock mux, many have group clock muxes where they share a clock source, and some have individual clock muxes:

stm32h7xx-hal/src/rcc/rec.rs

Lines 313 to 404 in e6da7dc

	peripheral_reset_and_enable_control! {
	AHB1, "AMBA High-performance Bus (AHB1) peripherals" => [
	Eth1Mac, Dma2, Dma1,
	#[cfg(any(feature = "dualcore"))] Art,
	Adc12 [group clk: Adc(Variant) d3ccip "ADC"]
	];
	AHB2, "AMBA High-performance Bus (AHB2) peripherals" => [
	Hash, Crypt,
	Rng [kernel clk: Rng d2ccip2 "RNG"],
	Sdmmc2 [group clk: Sdmmc]
	];
	AHB3, "AMBA High-performance Bus (AHB3) peripherals" => [
	Sdmmc1 [group clk: Sdmmc d1ccip "SDMMC"],
	Qspi [kernel clk: Qspi d1ccip "QUADSPI"],
	Fmc [kernel clk: Fmc d1ccip "FMC"],
	Jpgdec, Dma2d, Mdma
	];
	AHB4, "AMBA High-performance Bus (AHB4) peripherals" => [
	Hsem, Bdma, Crc,
	Adc3 [group clk: Adc],
	Gpioa, Gpiob, Gpioc, Gpiod, Gpioe, Gpiof, Gpiog, Gpioh, Gpioi, Gpioj, Gpiok
	];
	APB1L, "Advanced Peripheral Bus 1L (APB1L) peripherals" => [
	Dac12,
	I2c1 [group clk: I2c123 d2ccip2 "I2C1/2/3"],
	I2c2 [group clk: I2c123],
	I2c3 [group clk: I2c123],
	Cec [kernel clk: Cec(Variant) d2ccip2 "CEC"],
	Lptim1 [kernel clk: Lptim1(Variant) d2ccip2 "LPTIM1"],
	Spi2 [group clk: Spi123],
	Spi3 [group clk: Spi123],
	Tim2, Tim3, Tim4, Tim5, Tim6, Tim7, Tim12, Tim13, Tim14,
	Usart2 [group clk: Usart234578(Variant) d2ccip2 "USART2/3/4/5/7/8"],
	Usart3 [group clk: Usart234578],
	Uart4 [group clk: Usart234578],
	Uart5 [group clk: Usart234578],
	Uart7 [group clk: Usart234578],
	Uart8 [group clk: Usart234578]
	];
	APB1H, "Advanced Peripheral Bus 1H (APB1H) peripherals" => [
	Fdcan [kernel clk: Fdcan(Variant) d2ccip1 "FDCAN"],
	Swp [kernel clk: Swp d2ccip1 "SWPMI"],
	Crs, Mdios, Opamp
	];
	APB2, "Advanced Peripheral Bus 2 (APB2) peripherals" => [
	Hrtim,
	Dfsdm1 [kernel clk: Dfsdm1 d2ccip1 "DFSDM1"],
	Sai1 [kernel clk: Sai1(Variant) d2ccip1 "SAI1"],
	Sai2 [group clk: Sai23(Variant) d2ccip1 "SAI2/3"],
	Sai3 [group clk: Sai23],
	Spi1 [group clk: Spi123(Variant) d2ccip1 "SPI1/2/3"],
	Spi4 [group clk: Spi45(Variant) d2ccip1 "SPI4/5"],
	Spi5 [group clk: Spi45],
	Tim1, Tim8, Tim15, Tim16, Tim17,
	Usart1 [group clk: Usart16(Variant) d2ccip2 "USART1/6"],
	Usart6 [group clk: Usart16]
	];
	APB3, "Advanced Peripheral Bus 3 (APB3) peripherals" => [
	Ltdc,
	#[cfg(any(feature = "dsi"))] Dsi
	];
	APB4, "Advanced Peripheral Bus 4 (APB4) peripherals" => [
	Vref, Comp12,
	Lptim2 [kernel clk: Lptim2(Variant) d3ccip "LPTIM2"],
	Lptim3 [group clk: Lptim345(Variant) d3ccip "LPTIM3/4/5"],
	Lptim4 [group clk: Lptim345],
	Lptim5 [group clk: Lptim345],
	I2c4 [kernel clk: I2c4 d3ccip "I2C4"],
	Spi6 [kernel clk: Spi6(Variant) d3ccip "SPI6"],
	Sai4 [kernel clk_a: Sai4A(Variant) d3ccip
	"Sub-Block A of SAI4"]
	[kernel clk_b: Sai4B(Variant) d3ccip
	"Sub-Block B of SAI4"]
	];
	}

I think it's a reasonable abstraction once you know it exists. I think it'd certainly be possible to improve with some typesystem things, maybe represent the tree-like structure better, but I'm a bit wary about the complication. I remember being a rust noob and finding such things very confusing, and rustdoc is not great at exposing them.

Back to the RTC, the reasons I didn't integrate with this existing clock system are:

1. The RTC RCC registers are completely different from every other peripheral. It took some ugly hacks to work which didn't seem worthwhile for just one peripheral, so I decided to split it out from the PeripheralRec struct.
2. The RCC initialization is designed around bringing up a blank-slate reset system. The LSE lives in the backup domain and thus isn't reset unless you reset the whole backup domain (LSE and RTC). There are also lots of specific requirements around configuring the LSE clock, backup domain write protection, and RTC, like you can only set the RTC clock mux one time per backup domain reset, and you can't enable LSECSS until after the RTC is using the LSE.
3. I didn't think that anything else used the LSE clock. Looking at the STM32CubeMX clock configuration tab (why doesn't the reference manual have a clock tree chart!?) now I see that MCO1, CEC, LPTIMs, LPUARTs, and UARTs all can use the LSE. However I think these use-cases are very rare (nobody has asked for them yet) and the RTC is a much more primary use-case for the LSE. I think it's reasonable to ignore that use-case for now given the implementation and interface complexities they'd introduce. There are many places in the HAL where we choose to support the common use-case at the expense of flexibility and implementability.

I can imagine a system where the LSE is moved into the Backup which is passed to RCC::freeze() which populates the lse_ck in CoreClocks so other peripherals can use it. The main issue is reset. LSE and RTC are reset together so they really want to be initialized together. If you can think of a good interface for this I'd love to hear it.

[richardeoin]

This is really useful! I haven't looked at it all yet, but looks reasonable so far.

I think implementing ResetEnable separately for the backup domain is fine, especially since the peripheral_reset_and_enable_control! macro is complicated enough already. The documentation already does a reasonable job at describing which clocks are possible for each peripheral, although it's certainly possible to improve it more.

Indeed it would be nice to share the LSE state with the CCDR struct somehow, but agreed that's not essential for this PR.

[mattico]

I changed this from self to &mut self to make Rtc::handle_lse_css() only need to take &mut self, but it seems safe to me?

[richardeoin]

Ok, that's reasonable (and certainly not "unsafe" from a memory safety point-of-view).

The reason for requiring self is it forces the parent to be mutated when the kernel clock is mutated. You could also argue that actually handle_lse_css() should also take self, and it should return some new type called RtcFromLsi or similar. Thinking in terms of a state machine, Rtc and RtcFromLsi are the states and handle_lse_css() is a state transition - this kind of thinking is common in languages like Haskell and Erlang.

So don't think there's a right-or-wrong answer. Requiring self does make it more difficult to use, especially if you're thinking imperatively. But it makes the program easier to reason about if you're thinking in terms of states.

[mattico]

That makes a lot of sense. I can envison an Rtc parameterized by the clock source. The reset situation makes changing between clock states a bit tricky. I'd like to use this more to better understand what kind of interface would be helpful.

[mattico]

Thanks for the review! I'll leave this as draft until stm32h7 0.13 then I'll rebase.

[mattico]

Actually it may be a while until 0.13 so I'll revert to the uglier implementation that works with the undocumented registers for now

[richardeoin]

Could you add the new rtc feature gate to the CI please? It's here and here, see what #81 does for an example. Otherwise the example doesn't get built in CI unfortunately.

[richardeoin]

The example still doesn't work for me unfortunately. It actually needs to be

    loop {
        info!("Time: {}", rtc.time().unwrap());
        let date = rtc.date().unwrap(); // Added this line
        rtc.unpend(rtc::Event::Wakeup);
        asm::wfi();
    }

It appears to be due to RM0433 Rev 7. 46.3.9 Third Paragraph. The read to DR needs to come last. One solution could be a dummy read of DR after every read of SSR or TR (there are several functions with these reads).

[richardeoin]

The first two paragraphs also talk about the required frequency of pclk4. It would be nice to assert! if PCLK4 is less than seven times the RTC clock frequency (fRTCCLK). That could reasonably happen when using the HSE as a source.

[mattico]

I really should've tested the example after I made those changes, but I left my nucleo board at home... I'll get it working. Thanks for the review!

[richardeoin]

I think it's nearly there!

After the example is working, I'll finish reviewing it 👍

[mattico]

Reading through that section of the RM again I see a few options:

1. Fix the order of the shadow register reads and clear the RSF after every read like the RM says. This limits you to one read every RTC clock cycle which is probably 32KHz. I think that could be annoying or surprising for some applications that are trying to read the RTC often for timestamps, etc. and suddenly all their CPU is used waiting around for the RTC. Two solutions to that:
   A. Return nb::Result and let the user do work while the RTC is not ready to read. This pushes the complexity to the user, and how many people are set-up to handle async operations properly right now?
   B. Read the whole time/date in each member function that reads any component, and store the result. Subsequent reads will check the RSF and if it's not set use the memoized value.
2. Disable the shadow register and read directly from the RTC TR/DR/SSR. You now have to read twice to make sure the registers didn't change during your read. "While BYPSHAD=1, instructions which read the calendar registers require one extra APB
   cycle to complete." So that effectively quadruples the read time since we need to read twice and it takes twice (?) as long, not that it really matters.

It looks like both 1.B and 2 would work, but there's one more thing:

After waking up from low-power mode (Stop or Standby), RSF must be cleared by software.
The software must then wait until it is set again before reading the RTC_SSR, RTC_TR and
RTC_DR registers.

We don't support low-power mode in the HAL, though it's pretty common to use wfi(). We could implement some sort of callback system that tells the RTC when we wake up from sleep/standby; maybe there's a register we can read somewhere that will tell us.

But it would be so much easier to just disable the shadow register. I can't see any reason not to. RSF is convenient to wait for shift and initialization to complete but we can check those flags manually if we have to.

[mattico]

There were two additional issues which caused the example to not work:

1. RTC interrupts are routed through EXTI to NVIC, presumably so they can wakeup the core or D3. You need to enable the interrupts in EXTI as well. Thanks ST for not mentioning this anywhere at all.
2. The RTC_WKUP interrupt handler didn't unpend the interrupt so it just ran forever. 🤦‍♂️

@richardeoin this should be ready for review again when you have the time.

[richardeoin]

Great, thanks for fixing up the example.

I think that disabling the shadow registers and reading directly (BYPSHAD=1) is a good solution. It does mean every read takes a little longer but it's worth it. Both to avoid blocking waiting for RSF to go high, and needing to do things on wakeup. (BTW for the example in release mode, I measure about 500 cycles for the rtc.data_time().unwrap() call in the interrupt. That's fine)

I just have one comment about enable_wakeup, otherwise this is ready to merge.

[richardeoin]

bors r+

[richardeoin]

@mattico What do you think of the rustfmt check in CI? Having consistent formatting is nice, and it means there should never be any formatting noise in diffs. However it's quite annoying when submitting a PR, unless you have rustfmt-on-save in your editor (a personable choice imo).

Could it be an optional check? Even better would be that bors runs rustfmt when merging the PR, but I don't know if bors supports that.

[bors]

Build succeeded:

• ci (1.43.0, stm32h743)
• ci (1.43.0, stm32h743v)
• ci (1.43.0, stm32h747cm7)
• ci (1.43.0, stm32h753)
• ci (1.43.0, stm32h753v)
• ci (stable, stm32h743)
• ci (stable, stm32h743v)
• ci (stable, stm32h747cm7)
• ci (stable, stm32h753)
• ci (stable, stm32h753v)
• Rustfmt

[mattico]

@richardeoin I like having the code consistently formatted and I think it's good to have a PR check.

The reason I have trouble with it is that I primarily develop stm32h7hh-hal in a submodule of my firmware project which uses nightly. The nightly rustfmt formats differently from the stable rustfmt that CI uses so I don't use format-on-save and need to remember to cargo fmt. I am not good at remembering :)