Clocking API V2 (for thumbv7em)

[glaeqen]

• Authors: @bradleyharden, @vccggorski, @vcchtjader

Summary

New clocking API allowing to build a typesafe chain of dependent clock sources, generic clock generators and peripheral channels.

Motivation

Current clocking API assumes the following workflow:

let mut clocks = GenericClockController::with_{external,internal}_32kosc(...PAC...);
let gclk9 = clocks.configure_gclk_divider_and_source(ClockGenId::GCLK9, 1, ClockSource::DFLL48M, false);
let adc0_clock: Adc0Clock = clocks.adc0(&gclk9);

This API has the following limitations.

1. By default the GenericClockController constructor configures and provides an opinionated set of clock sources and generic clock generators that cannot be changed afterwards. Following clock configuration is set for thumbv7em.
   • GCLK0 (Generic Clock Generator 0) is driven by DFLL48M (48MHz) through GCLK5 (divider: 24 -> 2MHz) through Pclk1 (Peripheral Channel 1) through DPLL0 (multiplier: 60 -> 120MHz)
   • GCLK1 is driven either by XOSC32K (use_external_32kosc) or OSCULP32K (use_internal_32kosc)
2. It is not possible to set up additional clock sources within the framework of current clocking API (without hacking around PACs).
3. Once you setup the Clock Source - GCLK pair, it is impossible to change it later.
4. Once you setup the GCLK - PeripheralChannel pair, it is impossible to change it later.

Main clock locked to 120MHz by HAL, even though being acceptable in basic scenarios, is a serious design flaw that severely diminishes flexibility of a HAL and might either encourage users to hack unsafe solutions on top of existing abstractions or discourage them from using the HAL altogether.

Having these points in mind and also the fact that current clocking API implementation would benefit from refactoring anyway, striving for improvement seems to be fully motivated and justified.

Detailed design

Introduction

ATSAMD clocking system assumes 3 types of major building blocks:

• Clock sources (referred to further on by ClkSrc)
• Generic Clock Generators (referred to further on by Gclk)
• Peripheral Channels (referred to further on by Pclk)

Properties of ATSAMD clocking system:

• Gclks depend on ClkSrcs in a N:1 relationship
• Pclks depend on Gclks in a M:1 relationship
• Some ClkSrcs can depend on some Pclks Specifically:
  • For thumbv7em-based MCUs
    • Pclk0 can serve as a reference clock provider for ClkSrc:Dfll48M
    • Pclk1 can serve as a reference clock provider for ClkSrc:Dpll0
    • Pclk2 can serve as a reference clock provider for ClkSrc:Dpll1
  • For thumbv6m-based MCUs
    • Pclk0 (on thumbv6m Peripheral Channels are called GCLK Multiplexers) can serve as a reference for ClkSrc:Dfll48M
• Some ClkSrc can depend on some other ClkSrcs Specifically
  • For thumbv7em-based MCUs
    • 32Khz signal of ClkSrc:Xosc32K can serve as a clock source for ClkSrc:Dpll{0, 1}
    • ClkSrc:Xosc{0, 1} can serve as a clock source for ClkSrc:Dpll{0, 1}

Mapping HW model to Rust

In order to model one-to-many type of relationship between dependencies a few additional concepts/abstractions were introduced.

Enabled type and its helper types/traits

Enabled type wrapper represents a clocking component in its enabled state while also holding an information about current amount of dependencies (usually 0 upon construction). This amount of dependencies is embedded into the type via second generic parameter leveraging typenum::{UInt, UTerm} types.

pub trait Counter {} /* implemented for every `typenum::Unsigned` */

pub trait Increment: Counter {
    /* implemented for every `typenum::Unsigned` and `Enabled` */
    type Inc: Counter;
    fn inc(self) -> Self::Inc;
}

pub trait Decrement: Counter {
    /* implemented for every `typenum::Unsigned` and `Enabled` */
    type Dec: Counter;
    fn dec(self) -> Self::Dec;
}

pub struct Enabled<T, N: Counter>(pub(crate) T, PhantomData<N>);

Via specialized implementation blocks for this type (like for Enabled<Gclk<G, H, U0>) it is possible to introduce special behaviour; e.g. fn disable will only exist for clocking component having U0 current users.

SourceMarker trait and its subtraits

This marker trait unifies family of more specific traits. These ones are essential during a construction fn ::{new, enable} and deconstruction fn ::{free, disable} of clocking components as they provide information to the constructed/deconstructed type what its source is (shown in the example later) and which variant of source (associated constant) is applicable while performing a HW register write.

pub trait SourceMarker {}

pub trait GclkSourceMarker: SourceMarker {
    const GCLK_SRC: crate::pac::gclk::genctrl::SRC_A /* GclkSourceEnum */;
}

pub trait PclkSourceMarker: GenNum + SourceMarker {
    const PCLK_SRC: crate::pac::gclk::pchctrl::GEN_A /* PclkSourceEnum */;
}

pub trait DpllSourceMarker: SourceMarker {
    const DPLL_SRC: crate::pac::oscctrl::dpll::dpllctrlb::REFCLK_A /* DpllSrc */;
}

pub trait GclkOutSourceMarker: GenNum + SourceMarker {}

These are implemented by marker types corresponding to existing clocking abstractions e.g.:

pub enum Pll1 {}
impl GclkSourceMarker for Pll1 {
    const GCLK_SRC: GclkSourceEnum = GclkSourceEnum::DPLL1;
}
// or
pub enum Dfll {}
impl GclkSourceMarker for Dfll {
    const GCLK_SRC: GclkSourceEnum = GclkSourceEnum::DFLL;
}

Source trait and its subtraits

This trait represents a source of clocking signal while subtraits its more specialized flavours (source of signal for Dpll, Pclk, Gclk, etc.).

pub trait Source {
    fn freq(&self) -> Hertz;
}

pub trait GclkSource<G: GenNum>: Source {
    type Type: GclkSourceMarker;
}

pub trait PclkSource: Source {
    type Type: PclkSourceMarker;
}

pub trait DpllSource: Source {
    type Type: DpllSourceMarker;
}

pub trait PrivateGclkOutSource: Source {
    fn enable_gclk_out(&mut self, polarity: bool);
    fn disable_gclk_out(&mut self);
}

pub trait GclkOutSource: PrivateGclkOutSource {
    type Type: GclkOutSourceMarker;
}

These are implemented by corresponding specialized types of Enabled e.g.:

impl<G, D, M, N> GclkSource<G> for Enabled<Dpll<D, M>, N>
where
    G: GenNum,
    D: DpllNum + GclkSourceMarker,
    M: SrcMode,
    N: Counter,
{
    type Type = D;
}

Regardless of how complicated it might seem to look, it can be roughly understood as: Enabled Dpll peripheral can be a source of signal for any Gclk.

*Token types

Unfortunately, PAC::Peripherals granularity is too low for them to be useful when spliting clocking system into such small, semi-independent pieces. In order to solve this problem, we consume PAC at the very beginning and return a set of tokens that internally use appropriate RegisterBlock directly, retrieved from a raw pointer . It is safe because register regions managed by different tokens do not overlap. Tokens cannot be created by a user; they are provided during initialization and do not expose any public API. Memory accesses are read/write-synchronized (Chapter 13.3; p. 138).

Source/SourceMarker traits usage in an API

This is a slightly simplified example of how more less every clocking component that relies on one-to-many depenedency relationships is implemented

impl<G, T> Gclk<G, T>
where
    // `GenNum` is a generalization of a Gclk compile time parameters
    // (e.g. ordering number via associated constant)
    G: GenNum,
    // Practical application of `SourceMarker`; it makes a connection between
    // `Source` and a `Gclk` used by it
    T: GclkSourceMarker,
{
    pub fn new<S>(token: GclkToken<G>, source: S) -> (Self, S::Inc)
    where
        S: GclkSource<G, Type = T> + Increment,
    {
        // .. implementation details ..
        let gclk = Gclk {
            token,
            /* ... */
        };
        (gclk, source.inc())
    }

    pub fn free<S>(self, source: S) -> (GclkToken<G>, S::Dec)
    where
        S: GclkSource<G, Type = T> + Decrement,
    {
        (self.token, source.dec())
    }

    pub fn enable(mut self) -> Enabled<Self, U0> {
        // HW register writes
        Enabled::new(self)
    }
    // Other functions operating on a disabled `Gclk<G, T>`
}
impl<G, T> Enabled<Gclk<G, T>, U0>
where
    G: GenNum,
    T: GclkSourceMarker,
{
    fn disable(mut self) -> Gclk<G, T> {
        // HW register writes
        self.0
    }
}

Gclk::new consumes, upon construction, a GclkSource provided to it and returns it with a type of Enabled<_, N++> (as mentioned previously, specializations of Enabled implement Source based traits). Analogically, Gclk::free consumes a GclkSource passed in and returns it with a new type of Enabled<_, N-->. By design it is impossible to go below 0, because there is always less or equal amount of users than a counter value.

Gclk0 case

Amount of users might be less than a value of a counter in case of special types like Gclk<Gen0> which always has an implicit single user -- synchronous clocking domain. Minimal amount of users for it is 1, making it impossible to disable and therefore consistent with its documented HW characteristics.

It also makes it impossible to change a configuration of a Gclk0 as a Enabled<Gclk0, _> cannot be deconstructed. Therefore, Enabled<Gclk0, U1> exposes additional methods that are usually available only for disabled Gclks.

Usage from the HAL perspective

Peripheral Y associated with Pclk<Y, _> will just consume it upon construction. Thus, clocking tree that is relevant for HAL peripheral X is locked and has to be released step by step. It is worth noting, that only that part of clocking tree is locked so the granularity is quite fine.

V1 clocking API compatibility

In order to support v1 clocking compatible peripherals, every clock::v1::*Clock object implements a From<Pclk<_, _>> trait. Therefore, it is feasible to use existing peripheral implementations with new clocking API.

To make a migration simplier, there're going to be standalone functions returning available clocking presets that were available in v1 clocking.

Handling of default clocking system state

fn crate::clock::v2::retrieve_clocks returns a tuple of following type

(
    Enabled<Gclk0<marker::Dfll>, U1>,
    Enabled<Dfll<OpenLoop>, U1>,
    Enabled<OscUlp32k, U0>,
    Tokens,
)

which is supposed represent a default state of clocking system after reset (Chapter 13.7; p. 141). As mentioned before Gclk0 is owned by virtual single user representing CPU and synchronous clocking domain and count of 1 cannot be decreased further.

What's still missing?

• Module root
  • [x] Documentation
• ahb
  • [x] Documentation
  • [x] Make sure there's no HW interactions in fn ::{new,free,<setter>,<getter>} kind of methods; only in fn ::{enable,disable}
• apb
  • [x] Documentation
  • [x] Make sure there's no HW interactions outside of fn ::{enable,disable}
• dfll
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable}
  • [x] Look through some TODOs
• dpll
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable}
  • [x] Assertions: prevent possible duplications + refine them if necessary
• gclkio
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable}
  • Revisit GclkOutSource trait approach
    • @bradleyharden Can we just leave it?
• gclk
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable}
• osculp32k
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable,activate_*}
    • set_calibration is fine?; called on a Enabled type > @vcchtjader <
• pclk
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable}
• xosc32k
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable,activate_*}
• xosc
  • [x] Documentation
  • [x] Make sure there's not HW interactions outside of fn ::{enable,disable}
  • [x] Assertions: prevent possible duplications + refine them if necessary (single panic case here)
    • @vcchtjader Do we want to move this code from Xosc::from_crystal to enable stage somehow or do we leave it as it is?
• rtc
  • [x] Make it safer to setup clock signal source
• Runnable examples
  • Do we want to have a separate example for that? I've added examples in documentation (it compiles with cargo test --doc). Maybe we should postpone it until we have BSC-less examples crate figured out? @bradleyharden @vcchtjader
• [x] Consider normalizing naming conventions for types implementing Source and SourceMarker traits
• [x] Make sure #[inline] is applied where it makes sense
• [x] Standalone ~functions~ macros replicating clocking presets available in v1 clocking
• [x] Get rid of / refactor re-exports in clock/v2.rs module
• [x] hal/src/typelevel.rs - 2x TODOs left
• Unsoundness if default state does not match the one expected after reset (BL jumping to APP case; both setup clocks) - proper solution? @bradleyharden @vcchtjader
  • I guess we can consider it unsupported, at least for now. It might become feasible to come up with a sane solution as soon as we have runtime based, symetrical API for clocking.

References

If there's a reference to an external document with a specified page, it is implicit reference to SAM D5x/E5x Family Data Sheet (DS60001507F) in a context of thumbv7em based MCUs. For thumbv6m-based MCU, it is a reference to SAM D21/DA1 Family Data Sheet (DS40001882F).

Related issues

Closes #114

[Sympatron]

Unsoundness if default state does not match the one expected after reset (BL jumping to APP case; both setup clocks) - proper solution?

Is it possible to reset all clocks? Otherwise this is quite problematic I think, because most(?) bootloaders will setup the clocks in some way.

[glaeqen]

Unsoundness if default state does not match the one expected after reset (BL jumping to APP case; both setup clocks) - proper solution?

Is it possible to reset all clocks? Otherwise this is quite problematic I think, because most(?) bootloaders will setup the clocks in some way.

Yes and no. Gclks expose HW register for SWRST. The rest is hit or miss. We had a conversation about this with @bradleyharden , solutions heavily depend on circumstances. If one knows a clock state before reaching retrieve_clocks, one could recreate a clock tree with types while using some unsafe without any HW writes. If one does not know the clock state before one's application starts, then it's very problematic. Solution for the former scenario is not really that bulletproof either, because, let's say, if firmware is upgraded partially, some invariants regarding clock tree expectations might get violated - in the grand scheme of things, I can imagine it's not really that hard to mess this up.

I came up with this custom ghetto solution that tries its best to restore default state in a correct order. It is impossible to restore everything to the default state, as there are some register that are populated by MCU with factory settings (like fine and coarse params for some oscillators and whatnot). In my approach I kinda ignore that and assume that user didn't mess with these too much.

//! Module containing clock system related utilities that go beyond of
//! [`atsamd_hal::clock::v2`] scope
use atsamd_hal::pac::{GCLK, MCLK, OSC32KCTRL, OSCCTRL};

/// Function implementing HW clocking reset (with some caveats).
///
/// In most cases this step is redundant. However, [`retrieve_clocks`] might be
/// unsound in scenarios when there is no full software reset between
/// application runs (eg. bootloader jumping to application or to secondary
/// bootloader). Enforcing a default state of a clocking system improves overall
/// soundness of an API.
///
/// Notes:
/// - If `Wrtlock` And Peripheral Access Controller is engaged, this procedure
///   might not be enough.
/// - Some parameters cannot be restored, eg. fine and coarse values for `Dfll`
///   running in an open mode. They are set to factory values after reset; if
///   user has changed them there is no way to restore them (is that for
///   certain?).
/// - Resetting Rtc source (to 1kHz UlpOsc32k) might corrupt Rtc operation if it
///   uses a source of different frequency AND it is supposed operate
///   continuously regardless of a switch from BL to APP/SBL.
pub unsafe fn hard_reset_clock_system(
    oscctrl: &mut OSCCTRL,
    osc32kctrl: &mut OSC32KCTRL,
    gclk: &mut GCLK,
    mclk: &mut MCLK,
) {
    // Reset main clock
    mclk.cpudiv.reset();
    mclk.ahbmask.reset();
    mclk.apbamask.reset();
    mclk.apbbmask.reset();
    mclk.apbcmask.reset();
    mclk.apbdmask.reset();
    mclk.intenclr.reset();
    mclk.intenset.reset();
    mclk.intflag.reset();

    // Reset Dfll
    oscctrl.dfllctrla.reset();
    while oscctrl.dfllsync.read().enable().bit_is_set() {}
    oscctrl.dfllctrlb.reset();
    while oscctrl.dfllsync.read().dfllctrlb().bit_is_set() {}
    //// Note:
    //// DFLLVAL contains FINE and COURSE values that come from factory
    //// If user managed to set these in previous application run, there's no easy
    //// way of resetting them
    // oscctrl.dfllval.reset();
    // while oscctrl.dfllsync.read().dfllval().bit_is_set() {}
    oscctrl.dfllmul.reset();
    while oscctrl.dfllsync.read().dfllmul().bit_is_set() {}

    // Reset Gclks and Pclks
    gclk.ctrla.write(|w| w.swrst().set_bit());
    while gclk.ctrla.read().swrst().bit_is_set() || gclk.syncbusy.read().swrst().bit_is_set() {}

    // Reset Dplls
    oscctrl.dpll.iter().for_each(|dpll| {
        dpll.dpllctrla.reset();
        while dpll.dpllsyncbusy.read().enable().bit_is_set() {}
        dpll.dpllratio.reset();
        while dpll.dpllsyncbusy.read().dpllratio().bit_is_set() {}
        dpll.dpllctrlb.reset();
    });

    // Reset Xoscs
    oscctrl.xoscctrl.iter().for_each(|xosc| {
        xosc.reset();
    });

    // Interrupts and miscellaneous
    oscctrl.evctrl.reset();
    oscctrl.intenclr.reset();
    oscctrl.intenset.reset();
    oscctrl.intflag.reset();

    // Reset Xosc32ks
    osc32kctrl.xosc32k.reset();
    // Cannot just reset, CALIB[5:0] is non zero
    osc32kctrl
        .osculp32k
        .write(|w| w.en1k().set_bit().en32k().set_bit());
    osc32kctrl.evctrl.reset();
    osc32kctrl.cfdctrl.reset();
    osc32kctrl.rtcctrl.reset();
    osc32kctrl.intenclr.reset();
    osc32kctrl.intenset.reset();
    osc32kctrl.intflag.reset();
}

Not sure if we want something like this in a HAL though. Maybe you or others have better idea how to tackle this. Probably the best option would be to introduce this dynamic clock layer to equation (DynPin like mechanism) that would allow switching back and forth from typed clocking API to runtime one. And retrieve_clock could also have a more dynamic sibling that would actually retrieve a clock state from HW. At least user could choose. BUT, I think, this is orthogonal to this PR, and could be introduced at any point in time later by people interested in developing it. @bradleyharden mentioned that he might at some point play with it and develop something like this.

[Sympatron]

Do you intent to put this (or something similar) in the HAL?

[glaeqen]

Didn't plan to because I considered it a hackish, half-assed solution to the complex problem which definitely does not work in all cases. I just assumed that this caveat should be considered officially unsupported and should be managed by hacks in user code if desired (here be dragons style). Putting it in the HAL, even as unsafe, means that this is something we are supporting. I don't know though.

[Sympatron]

You are probably right about that...

Did you test what happens, when you run your clock setup code after the bootloader initialized the clocks? Since a lot of ATSAMD boards have the Adafruit bootloader or something similar by default this must not fail.

[glaeqen]

You are probably right about that...

Did you test what happens, when you run your clock setup code after the bootloader initialized the clocks? Since a lot of ATSAMD boards have the Adafruit bootloader or something similar by default this must not fail.

It's sorta a HW-wise UB. In our case on "second" initialization of DPLL in application it hanged on getting lock, AFAIR.

[bradleyharden]

@vccggorski, I spent some time with the code this morning for the first time in a very long time. I think it helped give me fresh eyes on some of the problems. I think I had a few insights.

I was only able to get through the gclk, gclkio, pclk and dpll modules so far, but I already have quite a few comments. I think I'll stop for now and let you digest. This might be a long post, but I think the changes to the code would be pretty minor.

One of the problems I see with the codebase is just how many traits there are. Moreover, many have similar names. Unless you're already familiar with the trait structure, I think it would be very difficult to follow. I've worked on this code a lot, and even I had forgotten the meaning of some of the traits.

One big obstacle I see is that we extremely overload the word "source". It's used for all manner of purposes and ends up losing meaning. I think the most significant distinction lost can be illustrated with the GclkSource trait.

pub trait GclkSource<G: GenNum>: Source {
    type Type: GclkSourceMarker;
}

The problem is that we use the word "source" to talk about a relationship from two different perspectives. A clock can act as a source to another clock, or it can have a clock source of its own. The GclkSource trait shows both directions. In the forward direction, implementing types act as sources for the generic GCLK G. And in the reverse direction, a GCLK G has a source of Type.

I think we should try to find a consistent way to make the distinction between perspectives. For that, I would like to suggest we use the term "driver" for the forward relationship, when a type acts as a source for some downstream clock. For the reverse relationship, I think we can stick with the term "source".

I also think it would be nice if we could cut down on the total number of traits in the module. Fortunately, I think I have a proposal that can accomplish both goals. But before I explain the proposal, I'd like to point out a realization I had recently.

I saw a series of posts starting here. He argues that it is often better to leave off trait bounds from the definition of a struct. Instead, you should apply bounds only at the points where they are required to enforce correctness. Even if you include the bound on the struct, you still need to repeat the bound anyway, so it doesn't save you any typing. But it does require you to repeat the bound in places where it doesn't matter.

I sort of recognized this fact before, but not quite as explicitly. I chose to put the bounds on structs anyway, because it makes sure you don't accidentally miss a required bound. But there may be cases where the opposite apporach is preferred.

Coming back to clock::v2, I think this is actually one of those cases. The XSource traits are all nearly identical. For the most part, they differ only in the trait bound applied to Type. But that trait bound is already repeated at almost every usage site, in the form S: XSource<Type = T> where T: XSourceMarker. I would like to propose that we unify all of these traits into a single trait.

pub trait Driver<Target = ()> {
    type Source;
    fn freq(&self) -> Hertz;
}

The existing, general Source trait simply becomes Driver, where the Target is unspecified.

impl<D, ..., N> Driver for Enabled<Dpll<D, ...>, N>
where
    D: DpllNum,
    ...
    N: Counter,
{
    type Source = D;
}

Then, when you need to limit the targets of a Driver, like the GclkSource<G> trait does, you can specify a particular Target type.

impl<G, D, ..., N> Driver<G> for Enabled<Dpll<D, ...>, N>
where
    G: GenNum,
    D: DpllNum + GclkSource,
    ...
    N: Counter,
{
    type Source = D;
}

Consumers of a Driver are responsible for applying the appropriate trait bounds to the Source type.

impl<G, S> Gclk<G, S>
where
    G: GenNum,
    S: GclkSource,
{
    pub fn new<D>(token: GclkToken<G>, driver: D) -> (Gclk<G, S>, D::Inc)
    where
        D: Driver<G, Source = S> + Increment,
    {
        \\ ...
    }
}

One thing to note: With this approach, you can have an implementation of Driver<()> and exactly one generic implementation of Driver<T: SomeBound>, but you can't have two generic implementations. I think that's ok, though, because GclkSource<G> is the only version that takes a type parameter right now, and you can always implement as many as you need manually.

Notice also that we can now rename the XSourceMarker traits to simply XSource. Driver represents the forward direction, while XSource represents the reverse direction.

What do you think?

Here are a few other suggestions I'd like to discuss:

• If we rename the XSourceMarker traits to simply XSource, then I think we should also rename the XSourceEnum types to DynXSource. It fits nicely with other HAL conventions. If XSource is a type-level enum with multiple variant types, then DynXSource is the value-level equivalent.
• Shouldn't Pclk, GclkIn and GclkOut use functions named new rather than enable to create an instance from a Token. I thought that was the way you prefer?
• I don't think GclkIn::disable should exist. The code should be moved to Enabled<GclkIn<...>, U0>. Similar for GclkOut.
• I think we should move the Increment, Decrement, Counter and associated traits to the typelevel module. However, we should keep the Enabled type in the clock module. The traits are generic type-level tools, but I agree that the Enabled type is particularly tied to the clock module.
• GclkDivider::get_inner is not very descriptive. Maybe get_variant_field instead?
• If both implementors of GclkDivider implement Default, should it be a super trait?
• I think we might want to rearrange some of the trait implementations. I think I originally wanted to keep trait implementations in the same module as the implementing type. But now I think that's the wrong approach. The best example is the gclkio::NotGclkIn trait. Right now, it has zero implementations in the gclkio module. They are all spread out. But that makes it hard to tell what it's for, which types implement it, and whether the set is complete or not. Since NotGclkIn is really a type-level enum, I think it might make more sense to have all of its implementations next to it in the gclkio module.
• I don't really like the names PclkId and PclkType. I did a bad job there.
• I think PclkId might be better named PclkIndex, because it represents the index into the array of Pclk registers. We should probably add repr(usize) to its definition.
• pclk::PclkType is analogous to gclk::GenNum and dpll::DpllNum. Maybe we should come up with a consistent naming scheme here?
  • Since they are all identifiers used to select one of several clocks, maybe they should be named GclkId, PclkId and DpllId, similar to PinId?
• We should probably also be consistent with naming between the corresponding variant types, e.g. Gen0 and Pll0.
  • Maybe they should be GclkId0 and DpllId0? I think I like that better than marker::Gclk0. Thoughts?
• We might want to restructure the dpll::SrcMode trait. In fact, I think I see a problem with the current implementation.

Right now, Dpll::from_xosc32k takes reference_clk: S, where S: DpllSource<Type = T>, but there is no restriction on T guaranteeing that it represents Xosc32k. The Dpll::from_xosc function has the same bounds as Dpll::from_xosc32k, so I believe it would be possible to mix up the two.

There's also an inconsistency when specifying the Dpll type parameters. Normally, clock types take an ID type followed by a source type, e.g. Pclk<Sercom1, GclkId2>. But right now, the Dpll types require an extra wrapper type, e.g. Dpll<Pll0, PclkDriven<Pll0, GclkId2>>.

I think we can get rid of that wrapper by doing some type-level mapping. In the following, I assume the new naming schemes developed above.

pub trait DpllSource {
    const DPLL_SRC: DynDpllSource;
    type Payload;
    fn predivider(payload: &Self::Payload) -> u16;
}

impl<D, S> DpllSource for Pclk<D, S>
where
    D: DpllId + PclkId,
    S: PclkSource,
{
    const DPLL_SRC: DynDpllSource = DynDpllSource::GCLK;
    type Payload = Self;
    fn predivider(payload: &Self::Payload) -> u16 {
        1
    }
}

impl DpllSource for Osc0 {
    const DPLL_SRC: DynDpllSource = DynDpllSource::XOSC0;
    type Payload = u16;
    fn predivider(payload: &Self::Payload) -> u16 {
        2 * (1 + *payload)
    }
}

impl DpllSource for Osc32k {
    const DPLL_SRC: DynDpllSource = DynDpllSource::XOSC32;
    type Payload = ();
    fn predivider(payload: &Self::Payload) -> u16 {
        1
    }
}

pub struct Dpll<D, S>
where
    D: DpllId,
    S: DpllSource,
{
    token: DpllToken<D>,
    src_freq: Hertz,
    mult: u16,
    frac: u8,
    payload: S::Payload,
}

The Dpll::from_* functions each know how to construct their respective payloads, just like the current implementation. And Dpll<D, S> can still call predivider generically, using S::predivider(&self.payload).

As I said, I'll stop here for now and let you digest.

[bradleyharden]

I've been working on the code to implement some of these changes. I have a few small updates.

1. I was able to get rid of the Target type parameter for Driver. I realized it was only there to prevent Gclk1 from being used as its own clock source. But the way you solved that problem for GclkIn was better. I added a new trait, just like NotGclkIn, and I removed the Target type from Driver. Now all uses of Driver are identical. The only difference is the trait bound applied to the Source associated type.
   • There was only one small casualty here. It slightly complicates how you create Gclks, because it messes with type inference a bit. The workaround is to explicitly use Gclk5::new, for example, rather than Gclk::new with type inference. Personally, I think that's a small price to pay in ergonomics, with a large benefit in simplicity of implementation.
2. I implemented the Payload approach described above for the dpll module. I also did the same for the xosc module. In both cases, it helps to increase consistency and reduce redundancy when naming the complete type.
3. I found a way to remove the use of PhantomData within the Enabled type. Now it uses the Unsigned types directly. I think it's cleaner that way.
4. I went through and normalized some of the trait, type parameter, and type names, to increase consistency. Here are a few examples:
   • I renamed all of the former marker::* types. I gave them all the Id suffix. Like the PinId types, I think it helps to communicate that it represents the identity of the clock type, but not the clock itself. It also breaks the naming collision without adding a ton of length to the name.
   • I realized that the relationship between the PinId and DynPinId types was mirrored in several places throughout the clock module. It just wasn't obvious, because the names weren't consistent. For example, the former GclkSourceMarker and GclkSourceEnum types have the exact same relationship. I renamed them to GclkSourceId and DynGclkSourceId. There are similar examples in the dpll, pclk, ahb, and apb modules. This can form the basis of a Dyn interface later.

Overall, I'm really happy with how the whole module looks right now. It is astronomically simpler and easier to understand than it used to be. It takes time to find all of the patterns and assign consistent naming schemes for them. But once you do, everything becomes much clearer.

[bradleyharden]

Well.... looks like most of my work this weekend has been wasted. I thought I was up-to-date, but it looks like I didn't properly git fetch, and the branch I've been reviewing and editing is a whopping 78 commits behind the tip.

I think many of the insights I had will probably still apply. But I think I will need to start the review completely over, since so much has changed.

[bradleyharden]

@vccggorski, @vcchtjader and @sakian, I just rebased onto master

[glaeqen]

@bradleyharden Assuming this PR will get at some point integrated, I'd really like to see doc-tests being exercised, so to speak, in CIs for PR gating and whatnot. I fixed them just lately. I'll try to do general review of existing documentation in the nearest future (TM).

Also, considering #614, do we want to include this HW-register-only-stored-state as a part of this PR? IMHO, one could just create clock::v3 or rather after current v1 gets completely phased out, v2 will be become "just" clock module for thumbv7 and then one could have a duplicated clock::nightly or another clock::v2 module with duplicated functionality that needs to be duplicated in order to expand it and rest is just dependent on clock. And over time it could just overwrite original abstractions as part of the breaking change. I think it would be nice to finally get a functional API for clocks in mainline HAL.

[bradleyharden]

I'd really like to see doc-tests being exercised, so to speak, in CIs for PR gating and whatnot

Agreed. I think this is something we should move toward.

I think it would be nice to finally get a functional API for clocks in mainline HAL.

Yes, I think we should move to get this PR pushed out. I think it's close enough to it's final state that further modifications will be minor. Those changes could still be breaking, but I don't think they would be at the level of the v1 vs. v2 split.

However, I would like to take one last look through with fresh eyes, both to update the documentation and to spot any potentially fatal flaws (like the Apb Sync issue was). What I don't want is to release v2 and start the migration process, only to require another, major rework. The migration from gpio::v1 to gpio::v2 has been difficult, so I'd prefer to keep similar levels of change to a minimum.

[sakian]

I've been using this branch for atsamd51 chip for several months now without issues. Is there any sense on how to make this available for atsamd21 chips (thumbv6)? Seems to be a subset of the features, but wasn't sure if any thought has been put into this.

[sajattack]

Very well done documentation. Better than Microchip's maybe. :laughing: Although I wonder if some of the internals docs might be going overboard for consumers of the library. Maybe some of it could be moved into the private modules or we could find a better way to split things up. Idk. Overall this looks great though :smiley: :+1:

[jbeaurivage]

Very well done! I know there's been a great deal of work t going into this effort.

My only question so far is regarding how we want to roll this out. Do we still want to make v1 available and mark it as deprecated? Do we want to remove it on the next breaking semver bump? Or do we want to remove immediately? Or never remove it?

I'd also like to see this ported to thumbv6m chips, how much work would that realistically be? I think it's better suited for a later PR though.

Again, thank you to everyone involved, impressive work.

[bradleyharden]

My only question so far is regarding how we want to roll this out. Do we still want to make v1 available and mark it as deprecated? Do we want to remove it on the next breaking semver bump? Or do we want to remove immediately? Or never remove it?

For the moment, I don't think we should do anything other than merge v2. No other parts of the HAL make use of it, so it definitely doesn't make sense to deprecate or remove v1 yet. Over time, we can migrate parts of the HAL to accept the v2 types. At that point, we can deprecate v1 and consider removing it in the future.

I'd also like to see this ported to thumbv6m chips, how much work would that realistically be? I think it's better suited for a later PR though.

Agreed. Before we deprecate or remove anything, I think we need to port it to thumbv6m. I have no idea how much work that would be, though. I haven't looked at the datasheet in any depth.

[bradleyharden]

It has been over two years since my initial issue and over a year and a half since @glaeqen's issue. Moreover, this PR has been open for well over a year, and several people are working directly from this branch (including myself, @glaeqen, @sakian and probably others).

With two maintainer approvals and a green CI, I think it's time we finally merged.

🚀

[glaeqen]

Shout out to @AfoHT aka @vcchtjader as well, considering his invaluable contribution at multiple stages of this PR. Great job everyone!

[glaeqen]

I think we are doing a squash merge, right? That means we should have a reasonable commit message.

[bradleyharden]

I'm writing it as we speak.