Common architecture for .enable() APIs

[bradleyharden]

@jbeaurivage, @vccggorski and @vcchtjader,

I (think?) I had some useful insights recently, while trying to solve #609. I wanted to lay out the problem and my proposed solution, to see if we can come up with a common approach that we all like. That would also help keep the APIs consistent across the HAL.

I'll start with the spi module. Right now, the two main structs look like this:

pub struct Config<P, M, Z>
where
    P: ValidPads,
    M: OpMode,
    Z: Size,
{
    // ...
}

pub struct Spi<C: ValidConfig> { ... }

Users create an spi::Config and use a builder API to configure it, before calling .enable(), which consumes the Config and places it inside the Spi. The AnyKind trait pattern is used (via ValidConfig) to reduce the repetition of type parameters on the Spi struct.

Right now, each call of a builder API function on Config corresponds to a register write. I liked this approach, because it gives users maximum freedom, and it means you don't need to store any of the configuration within Config; it is all stored directly in the registers.

On the other hand, @vccggorski preferred a different approach for the clock::v2 module. There, we define these structs.

pub struct Gclk<G, I>
where
    G: GclkId
    I: GclkSourceId
{
    // ....
}

pub type EnabledGclk<G, I, N> = Enabled<Gclk<G, I,>, N>;

In this API, users create instances of Gclk directly. Gclk also uses a builder API, but the configuration is stored within the struct. None of the builder-API functions actually write the registers. Only when calling .enable() are the registers actually written. Then, the Gclk is packed inside the Enabled struct, which provides compile-time counting of dependent clocks using the N type parameter.

@vccggorski and I have had a few debates about this approach. I don't really like that the configuration data is stored permanently within the Gclk. To me, it seems redundant and less compact than it could be.

However, I have to admit that, on at least one occasion, I have run into a problem with initialization order in the spi module. Moreover, I suspect #609 is also caused by initialization order. From that, I conclude that we should probably store the configuration parameters locally, and perform all the register writes in .enable(), so that we can guarantee the correct initialization order.

So I was left with a problem. I want to store all the configuration options within spi::Config and perform all register writes in the .enable() function, but I also don't want to bloat the size of spi::Spi. I think that is a fairly legitimate concern, too, because the full Config struct ends up looking like this:

pub struct Config<P, M, Z>
where
    P: ValidPads,
    M: OpMode,
    Z: Size,
{
    regs: Registers<P::Sercom>,
    pads: P,
    mode: PhantomData<M>,
    size: Z,
    freq: Hertz,
    cpol: Polarity,
    cpha: Phase,
    bit_order: BitOrder,
    baud: Hertz,
    ibon: bool,
    run_in_standby: bool,
}

You end up carrying around a minimum of something like 9 extra bytes for no real reason.

That's when I realized that the Config struct doesn't have to be stored within Spi. Instead, I can unpack the useful contents of Config in the .enable() call, and then drop the rest. With this approach, Spi ends up looking like this

pub struct Spi<P, M, Z>
where
    P: ValidPads,
    M: OpMode,
    Z: Size,
{
    // ...
}

where its fields are limited to keep it as compact as possible. The only downside is that the type parameters have to be repeated on Spi. But I think that actually ended up simplifying things in this particular case.

At this point, @jbeaurivage might ask how I handle reconfiguring an enabled Spi. The existing API has an Spi::reconfigure function that will temporarily disable the peripheral, call a user-provided closure, and then re-enable the peripheral. The user-provided closure effectively had the signature FnOnce(&mut Config). Users could then call methods of a non-builder API, with signatures like

impl Config {
    pub fn get_setting(&self) -> Setting {}
    pub fn set_setting(&mut self, setting: Setting) {}
}

With the new approach, the Config struct no longer has the non-builder API, because it is never used for anything other than constructing an Spi struct. Instead, I created a separate Reconfig struct that has get_ and set_ methods, and I changed the Spi::reconfigure function to take an FnOnce(Reconfig) closure.

I like this approach, because it provides the best of both worlds. It gives HAL authors complete control over creation and initialization, but it doesn't burden users with an unnecessarily bloated struct.

Moreover, I think the approach could be equally applicable to the clock::v2 API. There, we could provide a gclk::Config struct that acts like spi::Config. It would store all of the configuration options and perform all register writes in .enable(). The gclk::Gclk struct could then be stripped down to only the necessary fields.

At this point, I think @vccggorski might object, because he probably doesn't like the idea of calling gclk::Config::new to create a new Gclk. I also don't really like that, which is why I think we could provide

impl<G, I> Gclk<G, I>
where
    G: GclkId,
    I: GclkSourceId
{
    pub fn config<S>(token: GclkToken<G>, source: S) -> (Config<G, I>, S::Inc)
    where
        S: Source<Id = I>,
    {
    }
}

Existing calls to Gclk::new would instead become calls to Gclk::config. I actually think that's better, because it indicates to the user that the Gclk still needs configuration and is not yet enabled.

What do you all think of this approach? Do you see any problems or have any concerns? If it satisfies everyone, I would like to use this common approach in all of our code. I think that would give the HAL a very consistent feel.

Also, please see #613 for a draft of the new approach with the spi module. I think it's pretty much done, but I still need to update the documentation. I also don't want to go too far with it before I get buy-in from the rest of you.

[glaeqen]

@bradleyharden

I like the general idea but isn't this Reconfig struct reintroducing the initialization ordering problem? You have a Config that only has a local, logical state. Config::enable writes to registers according to spec order and returns Spi struct. Then, Spi::reconfigure allow you to write over registers in random order via Reconfig. Wouldn't it be better to make Reconfig struct similar to Config one but with the caveat of allowing register read operations? Thus, user could make conditional decisions based on the state of the peripheral HW (without storing the state in a struct itself) while the "finalization" of the reconfiguration process should also follow the in-spec order (just like Config::enable).

As for clock::v2, can one read from HW everything that is settable? Is there full symmetry?

[jbeaurivage]

@bradleyharden, I can see value in that approach. I like that all the register writes happen in only one place. I'll look at #613 and give my impressions over there.

[glaeqen]

@bradleyharden ping ping

[bradleyharden]

I like the general idea but isn't this Reconfig struct reintroducing the initialization ordering problem?

You're absolutely right on this. I forgot about that. I think I had in mind that most use cases for Reconfig would be to change a single setting, but that doesn't prevent you from doing the wrong thing accidentally. Let me rework Reconfig to prevent that. That might mean I completely remove Reconfig in favor of reusing Config.

[bradleyharden]

@vccggorski, I updated #613 to correct the Reconfig problem. It was a pretty minor change overall. The architecture didn't change at all.