@RobinMcCorkell, I haven't looked at this PR at all, but it sounds like it will overlap with #450, which, believe it or not, is still in the works. We're actually pretty close to publishing it, but I've been short on time at the moment.

In general, though, I would suggest starting a discussion topic about any big breaking changes

That's quite likely, I'm happy to rebase this on top of that branch if it's close to completion.

Are discussion topics different from GitHub Issues? There's been no traction on #642 since it was created, so I thought I'd publish some code and see what folks think. It was a useful learning exercise regardless.

Yes, sorry for the delay on your issue. Most of the maintainers have been pretty busy lately.

I think #450 will solve your issue. We have an RtcOsc type to represent the RTC oscillator. And we have plans to integrate it with the rest of the HAL. However, it's only for thumbv7em chips right now. What board are you working on?

I'm on a thumbv6m chip, but I'm happy to help port the new clock code over once #450 is merged. The core of this PR (using a typed token to initialize the RTC/SERCOM instead of a raw frequency) is separate.

I've done a port of the clocking v2 stuff here to thumb6m if you're interested in taking a look. I haven't brought in any of the recent updates from here though, but it's working for us quite well so far.

@sakian and @RobinMcCorkell, #450 has now been merged. If you have a port of it for thumbv6m, we would love to see a PR.

I tried to integrate the new v2 clocking API for thumbv7m with the existing clock API for thumbv6m, but I've hit a roadblock:

• thumbv6m has a RtcClock type that represents the configured RTC clock
• The new v2 API for thumbv7m does not have a similar type; it has a RtcOsc generic type (type constructor) that takes a source clock ID type token instead

I don't know how the design for v2 was established, but I don't see the utility of a source clock token in the type signature of the RTC clock. On both these platforms, the RTC clock is a singleton, there is only ever one RTC clock (which can be configured in one of several ways). As a user of this API, I would like to refer to this singleton RTC clock with a single type, which leans towards the current thumbv6m API rather than the new v2 API.

This PR is blocked until there is a way to reconcile these two APIs.

I have a rough port to thumbv6, but haven't had time to update it since I last posted. I do plan to clean it up and submit a PR.

@RobinMcCorkell I believe I ran into that issue as well and fixed in my branch. I'll take a look at how I implemented it when I get back infront of my computer.

@RobinMcCorkell, in general, the SourceId types are an important, if slightly annoying, requirement for enforcing clock safety. Without storing some SourceId type, it would be possible to configure the RTC and then disable or otherwise change its clock source. One solution is to say "just don't do that". But we wanted to go further and make it completely impossible.

A potential "solution" for a given application is to simply define a type alias

type RtcOsc = rtcosc::RtcOsc<Xosc32kId>;

Would that be sufficient for your use case?

Also note that the RtcOsc type was not meant to be the final API for the RTC. I haven't read much about the RTC, and I didn't want to tackle it as part of #450, so we created the rtcosc module as a temporary fix. Eventually, we should refactor the rtc module to either accept the RtcOsc type or integrate it more closely with the rest of the clock::v2 module.

@sakian, I just skimmed the D21 datasheet. Unfortunately, the GCLK peripheral is significantly different from the D5x/E5x version. It looks like our gclk and pclk modules will have to change significantly in the thumbv6m version, because everything is configured through shared registers. I think that will break memory safety, which will require a modified approach, like the ahb and apb modules. How did you tackle that?

@RobinMcCorkell, for more information on the design of clock::v2 and the SourceId types, checkout the section of the docs on Clock relationships, specifically the part about Acting as a clock Source.

The SourceId type is sort of acting as a shared, Rc reference, but at the type level. It prevents the corresponding source clock from being modified or disabled.

As per the documentation of "1:1 clocks" which I believe applies here:

One-to-one relationships are easily modelled in Rust using move semantics. For example, an enabled peripheral channel clock is represented as a Pclk object. The respective peripheral API can move the Pclk and take ownership of it. In that case, the Pclk acts as proof that the peripheral clock is enabled, and the transfer of ownership prevents users from modifying or disabling the Pclk while it is in use by the peripheral.

One-to-one clocks generally have little to no configuration. They are typically converted directly from disabled Token types to fully enabled clock types. For example, the Pclk type has only two methods, Pclk::enable and Pclk::disable, which convert PclkTokens to Pclks and vice versa.

This implies I should expect e.g. an RtcClk to configure the RTC peripheral, but as stated above that is incorrect. Instead I need to use a type from the type constructor RtcOsc<I: RtcSourceId.

A peripheral doesn't really care about the source clock, so let's abstract away from the RtcSourceId. Two ways I can see this working in the framework of the v2 API:

• RtcOsc<I> implements some Source<Id=RtcOscId>, then a peripheral would require C where C: Source<Id=RtcOscId>
• RtcOsc<I> implements trait RtcClk, then a peripheral would require C where C: RtcClk

Right now, the v2 API requires consumers care about the source clock ID, which IMHO is not optimal. We need a layer of traits to abstract that away (assuming the actual clock struct is going to continue to be generic, in order to support the type-level clock hierarchy as per the docs).

I've pushed a new commit that demos how this might work with the new clocking API. Looking for comments on the approach @bradleyharden @sakian

Instead of referencing the actual clock type in the peripheral, it now takes an additional Clock type parameter that implements an unsafe trait RtcClock. The various clock types implement this marker.

This is kind of like the AnyKind pattern described by the typelevel module, and I'm unsure whether an unsafe marker trait or use of Sealed and Is would be preferred. It's not clear to me how the latter would work.

@RobinMcCorkell,

As per the documentation of "1:1 clocks" which I believe applies here

I probably I need to update the clock::v2 docs, because I don't think it's clear enough.

When we speak of 1:1 and 1:N clocks, we're talking about the relationship between a clock and it's consumers. You are correct that the RTC clock has a 1:1 relationship with the RTC peripheral. Because of that, the RTC peripheral can simply take ownership of the RTC clock for the duration of its existence.

However, in D51 and E5x chips, the potential clock sources for the RTC clock are 1:N clocks. Specifically, they are the XOSC32K and OSCULP32K clocks, which can both be used in several places. To ensure correctness, all consumers of 1:N clocks must track the identity of their corresponding producers. That is why RtcOsc needs the RtcSourceId type parameter.

Now that I've done some work on the D21 clocking system, I know that things are a bit different for those chips. In your case, you have to enable a "generic clock" for the RTC, and you have to specify which generic clock generator to use. On D51 chips, "generic clocks" are called "peripheral channel clocks", so we call them Pclks in clock::v2.

On D21 chips, there is no need for an RtcOsc type. Instead, you would just use a Pclk<RtcId, G> type, where G: GclkId. But note that we still have the same fundamental problem. Because the Gclks are 1:N, all of their consumers must track the corresponding GclkId. No matter what we do, the Rtc type will need an additional type parameter to track the identity of the clock source feeding the RTC. Exactly what that looks like, though, will vary between D21 and D51 chips.

I think I know how to model this correctly in both cases, but I don't think we should make that change until #450 is ported to D21 chips. I've been working on that lately, so I don't think it will be too much longer.

That makes sense, and aligns with this marker trait idea. Notice the trait implementations:

unsafe impl<I: RtcSourceId> RtcClockMarker for RtcOsc<I> {
    fn freq(&self) -> Hertz {
        self.freq()
    }
}

I imagine that the D21 equivalent would look like:

unsafe impl<G: GclkId> RtcClockMarker for Pclk<RtcId, G> {
    fn freq(&self) -> Hertz {
        self.freq()
    }
}

This is of course assuming that the RTC clock (as an RtcOsc or Pclk) is always 1:1 with the actual RTC, and cannot be reused with other peripherals.

Can you verify that what I'm saying is correct? And then, whether the unsafe trait model is good, or whether a safe trait Sealed/Is model would be better?

@RobinMcCorkell, the problem with your approach is that the Rtc types will be more complicated than necessary. You define the Rtc type as

struct Rtc<M, C>
where
    M: RtcMode,
    C: RtcClock,

With this definition, a concrete instance of the Rtc type on D21 chips might be Rtc<ClockMode, Pclk<RtcId, Gclk3Id>>. And on D51 chips, an example would be Rtc<ClockMode, RtcOsc<Xosc32kId>>. If we inspect these types, we can see that there is redundant information. On D21 chips, we know that the RtcClock type must be a Pclk<RtcId, G>, yet we have to repeat both Pclk and RtcId every time we reference a concrete Rtc type. Similarly, for D51 chips, the RtcClock type must be RtcOsc<I>, yet we have to repeat RtcOsc.

The better way would be to pass on the corresponding Id type to the Rtc type, so that you get something like Rtc<ClockMode, Gclk3Id> or Rtc<ClockMode, Xosc32kId>. That eliminates redundant information and keeps the concrete types as concise as possible. Exactly how that is implemented will depend on the port of #450 to D21 chips.

To answer your other question, Sealed traits should be used when you want to completely prevent users from implementing a trait. Traits are normally open, but Sealed traits are completely closed and can never be extended. That's what we're trying to accomplish here, so that's the appropriate tool. An unsafe trait is intended to be implemented by users, but it requires knowledge that the compiler can't verify to prevent undefined behavior. In this particular case, we don't want users to implement the trait, nor would those implementations cause UB, so unsafe would not be an appropriate choice here.

I don't think the AnyKind pattern really applies here. That pattern is most useful when you have a type with multiple type parameters, and you want to "compress" the information into a single type parameter. For instance, if you need to store a Pin, you need to know both the PinId and the PinMode. AnyPin can be used to abstract over both with a single type parameter.

AnyKind isn't needed here, because we don't actually need to abstract over all Pclk types. We know that it must be Pclk<RtcId, G>, so we can just directly specify that. There's only one unknown type parameter, so there's nothing to be gained by introducing an abstraction.