Refactor Pin and Pad implementation

[BroderickCarlin]

I wanted to open this issue to discuss the extent that this crate uses macros and possible alternatives. Macro's tend to be more difficult to read and reason about while also not being as well supported by Rust's documentation system. It is my belief that because of this it may be beneficial to explore moving to code generation using some external scripts (such as Python) to generate the valid Rust code but I am curious to hear others thoughts of if this has been attempted previously

[wez]

While it is true that macros can be hard to read, and that it is easy to write macros that don't play as nicely with rust's documentation system, it is also true that they can be made clear and made to pass through documentation attributes and generate useful, meaningful documentation.

Rather than having a philosophical discussion, I think it would be useful if you could enumerate concrete examples of problems with macro usage in this crate. That would give a good starting point for an objective evaluation of how well things are working out!

[blm768]

I'd say some of the illegibility of the macros stems from using the macro system to accomplish tasks that could be done using other language features, such as the trait system. It looks like several other HALs are using a combination of macros and traits, but they lean more toward traits than atsamd-hal does. The Serial struct in stm32l4xx_hal might be a good example of this.

[jessebraham]

I think a significant portion of the readability problem can be solved just by better formatting and structuring, plus the addition of some comments where necessary. I believe that a couple macros have become a bit unwieldy, and can probably be broken up into smaller constituents.

As stated by @blm768, there is definitely some restructuring we can do to improve legibility as well. Procedural Macros are probably worth looking in to. I personally would rather keep as much in Rust as possible.

Really it's just getting a handle on the macros initially, once you've wrapped your head around them they're much easier to read. I do think we need to lower the bar for entry though, with that said.

If we can identify some concrete examples of problematic macros, perhaps we can begin a concerted effort to address them. I think it's quite difficult to act on this without hashing out some details first.

[blm768]

Really it's just getting a handle on the macros initially, once you've wrapped your head around them they're much easier to read. I do think we need to lower the bar for entry though, with that said.

Yeah; that corresponds with my own experience.

If we can identify some concrete examples of problematic macros, perhaps we can begin a concerted effort to address them. I think it's quite difficult to act on this without hashing out some details first.

One place we might start is the macros relating to SERCOM instances. The SPI and pin/pad macros took me a little while to wrap my head around. Given that the structure of our SPI/UART types seems to be a little unconventional (as compared to the two other HAL crates I've looked at so far), perhaps that would be a decent place to start. We could draw some inspiration from those other crates (although we'd need to deviate somewhat from those crates to account for the unusually flexible I/O support on the SAMD microcontrollers.

• https://docs.rs/stm32l4xx-hal/
• https://docs.rs/gd32vf103xx-hal/

[bradleyharden]

When adding support for a new chip (#243) this past weekend, I also struggled to wrap my head around what was going on with the SERCOM instances. I'm very new to Rust in general, so I wasn't sure if it was just me, or if the code could be better structured. It sounds like it's not just me.

It's all still very fuzzy to me, but it seems like the Pins trait is what we're missing compared to the stm3214xx HAL. On top of that, we also have to deal with the different IOSETs, which is what @blm768 is referring to. Did I get all of that right, or am I misunderstanding?

Before even seeing this thread, I was going to ask if it would make sense to introduce an IOSET trait. If I understand correctly, it's currently possible to mix pins from different IOSETs without the compiler complaining, right?

[keithduncan]

I added some of the macros in the sercom module to support more typestate programming at the sercom pad level. I agree they are complex, they took me a while to wrap my head around, and I’d be open to ideas to make them easier to understand while preserving the type guarantees currently supported 😄 if more traits or docs would help let me know.

---

As an example I have the following code in a board support crate I’ve made for the Arduino MKR NB1500:

// SERCOM4 => UART => Sara

pub extern crate sara_r4;

pub type SaraRx  = mcu::gpio::Pa13<mcu::gpio::PfD>;
pub type SaraTx  = mcu::gpio::Pa12<mcu::gpio::PfD>;
pub type SaraRts = mcu::gpio::Pa14<mcu::gpio::PfD>;
pub type SaraCts = mcu::gpio::Pa15<mcu::gpio::PfD>;

pub type SaraR410M = mcu::sercom::UART4<
    mcu::sercom::Sercom4Pad1<SaraRx>,
    mcu::sercom::Sercom4Pad0<SaraTx>,
    mcu::sercom::Sercom4Pad2<SaraRts>,
    mcu::sercom::Sercom4Pad3<SaraCts>,
>;

pub fn sara_r4(
        clocks: &mut mcu::clock::GenericClockController,
        sercom: mcu::pac::SERCOM4,
        pm: &mut mcu::pac::PM,
        interrupts: mcu::sercom::UartInterrupts,
        port: &mut mcu::gpio::Port,
        rx: mcu::gpio::Pa13<mcu::gpio::Input<mcu::gpio::Floating>>,
        tx: mcu::gpio::Pa12<mcu::gpio::Input<mcu::gpio::Floating>>,
        rts: mcu::gpio::Pa14<mcu::gpio::Input<mcu::gpio::Floating>>,
        cts: mcu::gpio::Pa15<mcu::gpio::Input<mcu::gpio::Floating>>,
    ) -> SaraR410M {
    let gclk0 = clocks.gclk0();
    mcu::sercom::UART4::new(
        &clocks.sercom4_core(&gclk0).expect("sercom4 clock"),
        115_200.hz(),
        sercom,
        pm,
        interrupts,
        (
            rx.into_pad(port),
            tx.into_pad(port),
            rts.into_pad(port),
            cts.into_pad(port),
        )
    )
}

Personally I like the specificity of seeing which pad configuration is used for board devices and how the pads are multiplexed but that might just be me 😅

[jacobrosenthal]

Discussion of macro avoidance guidelines in nrf-hal https://github.com/nrf-rs/nrf-hal/issues/215

[bradleyharden]

Based on the guidelines in @jacobrosenthal's post, I've created a proposed refactoring of the gpio module. So far, I've only done it for thumbv7em, but I can do it for thumbv6m as well.

In this post, I'll review the structure of the new code and my motivations for each choice. There is probably a way to make all of these changes non-breaking, but I wouldn't recommend it. However, exactly how much you're willing to break is up for debate. Currently, it is written with absolutely no thought toward backwards compatibility, but that might not be palatable.

As mentioned in @jacobrosenthal's post, I start by defining a local, private module and a Sealed trait. I use this trait throughout the gpio module, but I don't export it. This allows me to make all the gpio traits public, while still letting me limit the implementations to only those defined in the module.

mod sealed { pub trait Sealed {} }
use sealed::Sealed;

Next, I define traits representing input, output, and peripheral function "configurations". For example,

pub trait InputConfig: Sealed {}

pub struct Floating;
impl Sealed for Floating {}
impl InputConfig for Floating {}
// ...

/// Represents a pin configured as input
/// The CFG type is one of `Floating`, `PullDown` or `PullUp`
pub struct Input<CFG: InputConfig> {
    cfg: PhantomData<CFG>,
}

Currently, Input and Output are generic over a type, but all 14 peripheral functions are defined as distinct types on the same level as Input and Output. I decided to change that. Now, it looks like this

/// Trait representing peripheral configurations
pub trait PeripheralConfig: Sealed {
    /// Value written to the PMUX register for the given peripheral function
    const NUM: u8;
}

pub struct A;
impl Sealed for A {}
impl PeripheralConfig for A {const NUM: u8 = 00;}
// ...

pub struct PFunc<CFG: PeripheralConfig> {
    cfg: PhantomData<CFG>,
}

Now, you can change a pin from Input<Floating> to PFunc<D>. I really like the symmetry of this approach.

The types Input, Output and PFunc make up the available implementations of the PinMode trait.

/// Trait representing different pin modes
/// The available modes are `Input`, `Output` and `PFunc`
pub trait PinMode: Sealed {}

impl<CFG: InputConfig> PinMode for Input<CFG> {}
impl<CFG: OutputConfig> PinMode for Output<CFG> {}
impl<CFG: PeripheralConfig> PinMode for PFunc<CFG> {}

Next, I define a Group trait. This trait exists to act as a zero-sized reference to the corresponding GROUP registers.

/// Trait used to implement zero-sized references to the GROUP register blocks
pub trait Group: Sealed {
    fn group() -> &'static GROUP;
}

pub struct Group0;
impl Sealed for Group0 {}
impl Group for Group0 {
    #[inline(always)]
    fn group() -> &'static GROUP {
        unsafe { &(*PORT::ptr()).group0 }
}

To represent the identity of each pin, i.e. PA00 vs PC05, I define a PinId trait.

/// Trait representing the ID of a pin
pub trait PinId: Sealed
{
    /// Associated type used as a zero-sized reference to the correct GROUP registers
    type Group: Group;
    /// Pin number, e.g. 05 in PA05
    const NUM: usize;
}

pub struct PB04;
impl Sealed for PB04 {}
impl PinId for PB04 {
    type Group = Group1;
    const NUM: usize = 04;
}

Implementing the ID as a trait allows us to create a blanket implementation for Pin that is generic over the particular PinId. The PinId trait contains everything needed to modify the corresponding PORT registers.

Finally, we can implement the Pin itself.

/// Represents a generic pin
/// The ID type identifies the corresponding actual pin, and
/// the MODE type represents the pin mode and configuration
pub struct Pin<ID, MODE> where
    ID: PinId,
    MODE: PinMode,
{
    id: PhantomData<ID>,
    mode: PhantomData<MODE>,
}

Note that, although all of the traits and structs in this module are public, it's not possible to implement any of them from user code. The traits rely on the Sealed trait, which is private. And the structs use private members, so they can't be created directly by users.

Now, we can provide a blanket implementation that is generic over the PinId. I think this is a much better approach than the existing solution, where each pin has its own implementation, created by a macro.

impl<ID, MODE> Pin<ID, MODE> where
    ID: PinId,
    MODE: PinMode,
{
    pub fn into_input_floating(self) -> Pin<ID, Input<Floating>> { }
    pub fn into_input_pull_down(self) -> Pin<ID, Input<PullDown>> { }
    // ...
    pub fn into_pfunc<CFG: PeripheralConfig>(self) -> Pin<ID, PFunc<CFG>> { }

In each function, the corresponding GROUP for each pin is accessed like this.

let group = <ID::Group as Group>::group();

The rest of the module is fairly straightforward. The Parts structure no longer needs a port field, because the Pin functions can access the corresponding GROUP through the ID type. That also means that none of the functions need to take &mut Port, as you can see in the function definitions above.

Finally, note that I also made a lot of potentially "bikeshedding" changes. Personally, I think these changes make things more consistent, but I realize there is a compatibility price to be payed.

I changed the Pin function names from, e.g. into_floating_input to into_input_floating. The current name reads more like natural English, but the new name matches the structure of the MODE generics, e.g. Input<Floating>. The new name also serves to present the information from least to most specific, i.e. you're turning the pin into an input that is configured as floating.

I changed the name from into_function to into_pfunc. I found the name into_function to be overly generic. When reading code, it's not immediately obvious what function refers to. You have know that the object is a Pin, and you have to know that Atmel/Microchip calls them "peripheral functions" before it makes sense. To me, changing it to pfunc makes it more immediately obvious what function you're referring to.

I changed the pin type names and Parts field names. I chose to name the PinId implementations PA00 so that all of them are the same character width. I purposefully decided against PascalCase for the names, because I thought Pa00 was less visually distinct, and PA00 exactly matches the datasheet. I used fully lowercase names (pa00) in the Parts fields to be consistent with snake_case conventions there.

I chose to name the PeripheralConfig implementations bare letters, e.g. A, B, etc., because I didn't like the existing naming scheme. For example, PfA is visually similar to Pa4, but they represent completely different things. Now, you can use PFunc<A>.

You can see the full implementation of gpio here. You can verify that it compiles by checking out the gpio_refactor branch of my fork.

Overall, I think the refactor was a big success. I was able to eliminate about 200 lines of code, and I think the result is much more readable. Let me know what you think.

[bradleyharden]

Here's a quick prototype using the same approach to clean up the SERCOM pad definitions. I also added restrictions to statically check IOSET.

I got the prototype to compile, so I'm done for the night. I'll fill it out more later this week. I probably won't be able to work on it much tomorrow. Let me know what you all think.

use core::marker::PhantomData;

use crate::gpio_new::*;

mod sealed { pub trait Sealed {} }
use sealed::Sealed;

trait Sercom: Sealed {}
trait Pad: Sealed {}
trait IoSet: Sealed {}

macro_rules! instance {
    ($Trait:ident, $Instance:ident) => {
        struct $Instance;
        impl Sealed for $Instance {}
        impl $Trait for $Instance {}
    };
}

instance!(Sercom, Sercom0);
instance!(Sercom, Sercom1);
instance!(Sercom, Sercom2);
instance!(Sercom, Sercom3);
instance!(Sercom, Sercom4);
instance!(Sercom, Sercom5);
instance!(Sercom, Sercom6);
instance!(Sercom, Sercom7);

instance!(Pad, Pad0);
instance!(Pad, Pad1);
instance!(Pad, Pad2);
instance!(Pad, Pad3);

instance!(IoSet, IoSet1);
instance!(IoSet, IoSet2);
instance!(IoSet, IoSet3);
instance!(IoSet, IoSet4);
instance!(IoSet, IoSet5);
instance!(IoSet, IoSet6);

struct SercomPad<SERCOM, PAD, IOSET> where
    SERCOM: Sercom,
    PAD: Pad,
    IOSET: IoSet,
{
    sercom: PhantomData<SERCOM>,
    pad: PhantomData<PAD>,
    ioset: PhantomData<IOSET>,
}

impl<SERCOM, PAD, IOSET> Sealed for SercomPad<SERCOM, PAD, IOSET> where
    SERCOM: Sercom,
    PAD: Pad,
    IOSET: IoSet,
{}

impl<ID, MODE> Sealed for Pin<ID, MODE> where
    ID: PinId,
    MODE: PinMode,
{}

trait IntoPad<SERCOM, PAD, IOSET>: Sealed + Sized where 
    SERCOM: Sercom,
    PAD: Pad,
    IOSET: IoSet,
{
    fn into_pad(self) -> SercomPad<SERCOM, PAD, IOSET> {
        SercomPad { sercom: PhantomData, pad: PhantomData, ioset: PhantomData }
    }
}

trait IntoPin<ID, CFG>: Sealed + Sized where 
    ID: PinId,
    CFG: PeripheralConfig,
{
    fn into_pin(self) -> Pin<ID, PFunc<CFG>> {
        Pin { id: PhantomData, mode: PhantomData }
    }
}

impl IntoPad<Sercom0, Pad0, IoSet1> for Pin<PA08, PFunc<C>> {}
impl IntoPad<Sercom0, Pad0, IoSet2> for Pin<PB24, PFunc<C>> {}
impl IntoPad<Sercom0, Pad0, IoSet3> for Pin<PA04, PFunc<D>> {}
impl IntoPad<Sercom0, Pad0, IoSet4> for Pin<PC17, PFunc<D>> {}

impl IntoPin<PA08, C> for SercomPad<Sercom0, Pad0, IoSet1> {}
impl IntoPin<PB24, C> for SercomPad<Sercom0, Pad0, IoSet2> {}
impl IntoPin<PA04, D> for SercomPad<Sercom0, Pad0, IoSet3> {}
impl IntoPin<PC17, D> for SercomPad<Sercom0, Pad0, IoSet4> {}

[blm768]

I like the overall structure of your SERCOM example. I haven't fully thought this through, but it seems like it should be possible to eliminate the need to manually implement IntoPin for each pin/pad pairing. Perhaps there should be some kind of PadPin trait, like so:

pub trait PadPin<SERCOM: Sercom, PAD: Pad> {
    type IoSet: IoSet;
}

struct SercomPad<SERCOM, PAD, PIN> where
    PIN: PadPin<SERCOM, PAD>
{
    // PhantomData as necessary
    pin: PIN,
}

impl<SERCOM, PAD, PIN> SercomPad<PIN> where
    PIN: PadPin<SERCOM, PAD>
{
    pub fn new(pin: PIN) -> Self {
        Self { pin } // Plus PhantomData stuff
    }

    pub fn release(self) -> Pin {
        self.pin
    }
}

// Appropriate std::convert::From impls here...

impl PadPin<Sercom0, Pad0> for Pin<PA08, PFunc<C>> {
    type IoSet = IoSet1;  // Are we actually using this for something?
}
// etc...

[bradleyharden]

Hmm. Let me think about it more.

I think it's important that we label each SERCOM Pad by its IOSET. That allows downstream functions to restrict themselves to Pads using a single IOSET. For example, in the current code, the following example compiles, even though it will be non-functional. PA09 and PA11 are in IOSET 1, but PB24 is in IOSET 2.

let pins = peripherals.PORT.split();
let mut port = pins.port;

type SPIMaster = SPIMaster0<
    Sercom0Pad0<Pb24<PfC>>,
    Sercom0Pad3<Pa11<PfC>>,
    Sercom0Pad1<Pa9<PfC>>,
>;

let _spi = SPIMaster::new(
    &sercom_clock,
    MegaHertz(1),
    MODE_0,
    peripherals.SERCOM0,
    &mut peripherals.MCLK,
    (
        pins.pb24.into_pad(&mut port),
        pins.pa11.into_pad(&mut port),
        pins.pa9.into_pad(&mut port),
    ),
);

I need to think about it more, but I'm not sure if your approach will allow downstream functions to guarantee a single IOSET.

[blm768]

Hmm; perhaps if we parameterized on IOSETs instead...

pub trait PadPin<SERCOM: Sercom, IOSET: IoSet> {
    type Pad: Pad;
}

struct SercomPad<SERCOM, IOSET, PIN> where
    PIN: PadPin<SERCOM, IOSET>
{
    // PhantomData as necessary
    pin: PIN,
}

impl<SERCOM, IOSET, PIN> SercomPad<PIN> where
    PIN: PadPin<SERCOM, IOSET>
{
    pub fn new(pin: PIN) -> Self {
        Self { pin } // Plus PhantomData stuff
    }

    pub fn release(self) -> Pin {
        self.pin
    }
}

// Appropriate std::convert::From impls here...

impl PadPin<Sercom0, IoSet1> for Pin<PA08, PFunc<C>> {
    type Pad = Pad0;
}
// etc...

pub struct PadPins<SERCOM, IOSET, P0, P1, P2, P3> where
    P0: PadPin<SERCOM, IOSET, Pad=Pad0>,
    P1: PadPin<SERCOM, IOSET, Pad=Pad1>,
    // ...
{
     pad0: P0,
     pad1: P1,
     // ...
}

Then again, if I understand correctly, its seems like there's only one possible valid type for P0, P1, etc. for any given combination of SERCOM and IOSET. Seems like we should be able to avoid having to specify those, although I haven't invested enough time to figure out exactly how yet.

[bradleyharden]

@blm768, I'm playing with your idea now.

Yes, for a given SERCOM, Pad, and IOSET, there is a unique mapping to a particular pin. Basically, I'm trying to encode the tables in Section 6.2.8.1 of the SAMD5x/E5x datasheet. I don't see how there's any way around specifying all of those, though. They are already specified in the current code, but without any notion of IOSET, so users are free to mix IOSETs, which is a problem.

[bradleyharden]

@blm768, I worked on this a bit more last night. I have a solution that's pretty close to what you were talking about. I also think I can explain why your versions won't work.

Note, in the following code, I changed the type names slightly, compared to my initial prototype. Sercom still represents a particular SERCOM. But now, PadNum represents the pad number (Pad0 - Pad3), and Pad is the name of the full specification type, parameterized by both Sercom and PadNum. If we put Pad in the sercom module namespace, then you can refer to sercom::Pad, making the name SercomPad redundant.

At the bare minimum, Pad should be parameterized by both Sercom and PadNum. Downstream code will need to know both in order to use a particular Pad.

You parameterized Pad over (Sercom, PadNum and Pin) in your first example and (Sercom, IoSet and Pin) in your second. I don't think either of those is the most natural parameterization, though. In the first case, if you want to verify that each Pad uses the same IoSet, you have "look through" the Pin, i.e. <Pad.pin as PadPin>::IoSet. In the second case, you know the IoSet of each Pad, but you have to "look through" the Pin again to figure out the PadNum (<Pad.pin as PadPin>::PadNum).

I argue that Pad should be parameterized by (Sercom, PadNum and IoSet). Those are the most relevant parameters for downstream code. And on top of that, this parameterization can (almost) be completely exhaustive, i.e. every combination of (Sercom, PadNum and IoSet) is valid. FOOTNOTE: This isn't quite true, because IoSet5 and IoSet6 don't exist for every Sercom.

With this parameterization, you can define a trait to map from (Sercom, PadNum and IoSet) to the correct values of PinId and PeripheralConfig.

/// Trait mapping each (`Sercom`, `PadNum`, `IoSet`) triple to its corresponding
/// `PinId` and `PeripheralConfig`. This trait is used to limit transformations
/// to and from `Pad` and `Pin` to the valid possibilities
pub trait Map: Instance
{
    type ID: PinId;
    type CFG: PeripheralConfig;
}

Then, you can implement Map for each Pad<SERCOM, PADNUM, IOSET>,

impl Map for Pad<Sercom0, Pad0, IoSet1> { type ID = PA08; type CFG = C; }
impl Map for Pad<Sercom0, Pad0, IoSet2> { type ID = PB24; type CFG = C; }
impl Map for Pad<Sercom0, Pad0, IoSet3> { type ID = PA04; type CFG = D; }
impl Map for Pad<Sercom0, Pad0, IoSet4> { type ID = PC17; type CFG = D; }
// ...

Given a particular Pad you can view it as a Map to recover the Pin information,

<PAD as Map>::ID
<PAD as Map>::CFG

This is everything you need to implement core::convert::From. While I think the approach is elegant, the syntax is somewhat turbofish soup.

/// Convert from `Pad` to `Pin`. This transformation is only defined for valid
/// combinations of `Sercom`, `PadNum`, `IoSet` and `PinId`.
impl<PAD> From<PAD> for Pin<<PAD as Map>::ID, PFunc<<PAD as Map>::CFG>>
where
    PAD: Map,
{
    fn from(_: PAD) -> Pin<<PAD as Map>::ID, PFunc<<PAD as Map>::CFG>>
    {
        Pin::new()
    }
}

/// Convert from `Pin` to `Pad`. This transformation is only defined for valid
/// combinations of `Sercom`, `PadNum`, `IoSet` and `PinId`.
impl<SERCOM, PADNUM, IOSET, MODE> From<Pin<<Self as Map>::ID, MODE>>
for Pad<SERCOM, PADNUM, IOSET>
where
    SERCOM: Sercom,
    PADNUM: PadNum,
    IOSET: IoSet,
    Self: Map,
    MODE: PinMode,
{
    fn from(pin: Pin<<Self as Map>::ID, MODE>) -> Self {
        let _ = pin.into_pfunc::<<Self as Map>::CFG>();
        Pad::new()
    }
}

With this approach, you can also implement the structs you were imagining.

pub struct Pads<SERCOM, IOSET> 
(
    Pad<SERCOM, Pad0, IOSET>,
    Pad<SERCOM, Pad1, IOSET>,
    Pad<SERCOM, Pad2, IOSET>,
    Pad<SERCOM, Pad3, IOSET>,
)
where
    SERCOM: Sercom,
    IOSET: IoSet,
;

pub struct PadPins<SERCOM, IOSET, MODE0, MODE1, MODE2, MODE3> 
(
    Pin<<Pad<SERCOM, Pad0, IOSET> as Map>::ID, MODE0>,
    Pin<<Pad<SERCOM, Pad1, IOSET> as Map>::ID, MODE1>,
    Pin<<Pad<SERCOM, Pad2, IOSET> as Map>::ID, MODE2>,
    Pin<<Pad<SERCOM, Pad3, IOSET> as Map>::ID, MODE3>,
)
where
    SERCOM: Sercom,
    IOSET: IoSet,
    MODE0: PinMode,
    MODE1: PinMode,
    MODE2: PinMode,
    MODE3: PinMode,
    Pad<SERCOM, Pad0, IOSET>: Map,
    Pad<SERCOM, Pad1, IOSET>: Map,
    Pad<SERCOM, Pad2, IOSET>: Map,
    Pad<SERCOM, Pad3, IOSET>: Map,
;

pub struct ConfiguredPadPins<SERCOM, IOSET> 
(
    Pin<<Pad<SERCOM, Pad0, IOSET> as Map>::ID, PFunc<<Pad<SERCOM, Pad0, IOSET> as Map>::CFG>>,
    Pin<<Pad<SERCOM, Pad1, IOSET> as Map>::ID, PFunc<<Pad<SERCOM, Pad1, IOSET> as Map>::CFG>>,
    Pin<<Pad<SERCOM, Pad2, IOSET> as Map>::ID, PFunc<<Pad<SERCOM, Pad2, IOSET> as Map>::CFG>>,
    Pin<<Pad<SERCOM, Pad3, IOSET> as Map>::ID, PFunc<<Pad<SERCOM, Pad3, IOSET> as Map>::CFG>>,
)
where
    SERCOM: Sercom,
    IOSET: IoSet,
    Pad<SERCOM, Pad0, IOSET>: Map,
    Pad<SERCOM, Pad1, IOSET>: Map,
    Pad<SERCOM, Pad2, IOSET>: Map,
    Pad<SERCOM, Pad3, IOSET>: Map,
;

Then we can use these structs to transform groups of Pad and Pin.

pub fn get_pads<S, I, M0, M1, M2, M3>(pins: PadPins<S, I, M0, M1, M2, M3>) -> Pads<S, I>
where
    S: Sercom,
    I: IoSet,
    M0: PinMode,
    M1: PinMode,
    M2: PinMode,
    M3: PinMode,
    Pad<S, Pad0, I>: Map,
    Pad<S, Pad1, I>: Map,
    Pad<S, Pad2, I>: Map,
    Pad<S, Pad3, I>: Map,
{
    Pads(pins.0.into(), pins.1.into(), pins.2.into(), pins.3.into())
}

pub fn get_pins<S, I>(pads: Pads<S, I>) -> ConfiguredPadPins<S, I>
where
    S: Sercom,
    I: IoSet,
    Pad<S, Pad0, I>: Map,
    Pad<S, Pad1, I>: Map,
    Pad<S, Pad2, I>: Map,
    Pad<S, Pad3, I>: Map,
{
    ConfiguredPadPins(pads.0.into(), pads.1.into(), pads.2.into(), pads.3.into())
}

Finally, we would use the code like this.

let pins = PORT.split()

let pad0: Pad<Sercom0, Pad0, IoSet1> = pins.pa08.into()
let pad1: Pad<Sercom0, Pad1, IoSet1> = pins.pa09.into()
let pad2: Pad<Sercom0, Pad2, IoSet1> = pins.pa10.into()
let pad3: Pad<Sercom0, Pad3, IoSet1> = pins.pa11.into()
// OR
let pad_pins = PadPins(pins.pa08, pins.pa09, pins.pa10, pins.pa11);
let pads = get_pads::<Sercom0, IoSet1>(pad_pins);
let Pads(pad0, pad1, pad2, pad3) = pads;

// Then later ...

let pa08 = pad0.into()
let pa09 = pad1.into()
let pa10 = pad2.into()
let pa11 = pad3.into()
// OR
let pads = Pads(pad0, pad1, pad2, pad3);
let pad_pins = get_pins(pads);
let ConfiguredPadPins(pa08, pa09, pa10, pa11) = pad_pins;

I think this is the best way to do it. You can see the full prototype here. Let me know what you think.

[blm768]

I haven't looked through the full prototype, but I like what you've described so far. I have to agree that your parameterization is more natural.

I'm not sure what the advantage of having separate PadPins and ConfiguredPadPins is; does it enable a use case that wouldn't be possible otherwise?

[bradleyharden]

I actually wasn't sure how to handle that. When converting from Pin to Pad, I wanted to allow each Pin to be in an arbitrary PinMode. The conversion function would configure each Pin for the correct peripheral function, and then convert it to a Pad. But when you go the other way, from Pad to Pin, you know the PinMode for each Pin.

Originally, I didn't actually have those data structures. I only had function signatures for get_pads and get_pins. Then I re-read your post and realized that the function signatures would be way easier to read if I introduced Pads and PadPins data structures. I basically copied and pasted the definitions from the signatures into three separate data structures. I called the third one ConfiguredPadPins, because each Pin is configured for its corresponding Pad.

Now that I think about it more, I'm not sure if you need separate data structures. PadPins is strictly more general than ConfiguredPadPins, so it should be possible for get_pins to return it. I just might have to modify get_pins to tell the compiler what PinMode to use when converting from Pad to Pin.

I'm working now on converting the SPI module to use the new Pad. At some point, I'll create a tracking issue laying out the tasks required to make these changes. We can add that to a 1.0 milestone.

[bradleyharden]

I just had an idea. We could keep ConfiguredPadPins as a private data structure, and then I could add one extra line to get_pins to convert the ConfiguredPadPins to a regular PadPins. That keeps the turbofish soup definition of ConfiguredPadPins in one place.

[bradleyharden]

Another side note: I converted all my code to use single letter type parameters, except in a few cases to increase readability. The where clauses tells you what each letter represents, and I found it easier to read. I generally default to being verbose rather than terse, since I think it helps readability when you're unfamiliar with the code. But in this case I think my instincts failed me.

[blm768]

I just had an idea. We could keep ConfiguredPadPins as a private data structure, and then I could add one extra line to get_pins to convert the ConfiguredPadPins to a regular PadPins. That keeps the turbofish soup definition of ConfiguredPadPins in one place.

I think I'd actually favor going the other way, making ConfiguredPadPins the public struct (and ideally, the only multi-pin struct), under the name PadPins (or perhaps Pads, or ConfiguredPads, or…). It seems like Pad::new could handle the conversion into a Pin<Pad<SERCOM, PAD, IOSET> as Map>::ID, PFunc<<Pad<SERCOM, PAD, IOSET> as Map>::CFG>>, much like into_pad does now, and Pad could store the converted pin directly instead of relying on PhantomData. This would produce more turbofish soup, but we could clean that up with a couple of type aliases:

type PinForPad<SERCOM, PAD, IOSET, MODE> = Pin<Pad<SERCOM, PAD, IOSET> as Map>::ID, MODE>;
type ConfiguredPinForPad<SERCOM, PAD, IOSET> = PinForPad<SERCOM, PAD, IOSET, PFunc<<Pad<SERCOM, PAD, IOSET> as Map>::CFG>>;

[bradleyharden]

I have a few points.

First, what is the utility of putting the Pin inside the Pad? It makes it easier to convert from Pad back to Pin, but otherwise, I don't see any other advantages. Am I missing something?

If we want to put the Pin in the Pad, then we have to change things around a little bit. One approach would be to allow any pin to be stored in the Pad. We could do that by expanding the type parameters of Pad

pub struct Pad<S, P, I, Id, Mode>
where
    S: Sercom,
    P: PadNum,
    I: IoSet,
    Id: PinId,
    Mode: PinMode
{
    sercom: PhantomData<S>,
    padnum: PhantomData<P>,
    ioset: PhantomData<I>,
    pin: Pin<Id, Mode>,
}

or by using the gpio::Instance trait, which labels all instances of Pin.

pub struct Pad<S, P, I, Pin>
where
    S: Sercom,
    P: PadNum,
    I: IoSet,
    Pin: gpio::Instance,
{
    sercom: PhantomData<S>,
    padnum: PhantomData<P>,
    ioset: PhantomData<I>,
    pin: Pin,
}

Neither of these feels particularly natural to me, though. The valid combinations of Sercom, PadNum and IoSet are densely packed. There aren't many invalid combinations. But introducing any of the Pin traits makes it much sparser. Most of the combinations are now invalid.

Alternatively, we could restrict the stored Pin to the properly configured Pin. I think this is what you were imagining. However, to do that, we need to access the corresponding PinId and PeripheralConfig through the Map trait. But right now Map is defined on instances of Pad. You can't use a type's traits within the definition of the type. That doesn't really make sense.

We can get around that by defining Map on instances of IoSet instead. This was actually the original way I structured it. I moved away from that, because it required that Map take a Sercom and PadNum as type parameters, i.e. Map<S, P>. At the time, that felt redundant to me; we already had a something that took a Sercom and PadNum as type parameters, so why create another one? But I still have the instinct that Map really ought to be a trait on IoSet, since that is the crux of the pin mapping problem.

At the time, I also didn't realize you could create type aliases like that. I thought those type aliases would require trait bounds, and I knew you couldn't use trait bounds in the definition of a type alias. Now that I know it works, its much easier to make this all readable.

Here's what I came up with.

pub trait Map<S, P>: IoSet
where
    S: Sercom,
    P: PadNum,
{
    type Id: PinId;
    type Cfg: PeripheralConfig;
}

type MatchingId<S, P, I> = <I as Map<S, P>>::Id;

type MatchingCfg<S, P, I> = <I as Map<S, P>>::Cfg;

type MatchingMode<S, P, I> = PFunc<MatchingCfg<S, P, I>>;

type MatchingPin<S, P, I, M> = Pin<MatchingId<S, P, I>, M>;

type ConfiguredPin<S, P, I> = Pin<MatchingId<S, P, I>, MatchingMode<S, P, I>>;

Now, we can define Pad like this.

pub struct Pad<S, P, I>
where
    S: Sercom,
    P: PadNum,
    I: IoSet + Map<S, P>,
{
    sercom: PhantomData<S>,
    padnum: PhantomData<P>,
    ioset: PhantomData<I>,
    pin: ConfiguredPin<S, P, I>,
}

The implementations of From change slightly.

impl<S, P, I> From<Pad<S, P, I>> for ConfiguredPin<S, P, I>
where
    S: Sercom,
    P: PadNum,
    I: IoSet + Map<S, P>,
{
    fn from(pad: Pad<S, P, I>) -> Self
    {
        pad.pin
    }
}

impl<S, P, I, M> From<MatchingPin<S, P, I, M>> for Pad<S, P, I>
where
    S: Sercom,
    P: PadNum,
    I: IoSet + Map<S, P>,
    M: PinMode,
{
    fn from(pin: MatchingPin<S, P, I, M>) -> Self {
        let pin = pin.into_pfunc::<MatchingCfg<S, P, I>>();
        Pad::new(pin)
    }
}

I chose to implement MatchingPins and ConfiguredPins as type aliases of a tuple, rather than as tuple structs. I have a few reasons for this.

type MatchingPins<S, I, M0, M1, M2, M3> =
(
    MatchingPin<S, Pad0, I, M0>,
    MatchingPin<S, Pad1, I, M1>,
    MatchingPin<S, Pad2, I, M2>,
    MatchingPin<S, Pad3, I, M3>
);

type ConfiguredPins<S, I> =
(
    ConfiguredPin<S, Pad0, I>,
    ConfiguredPin<S, Pad1, I>,
    ConfiguredPin<S, Pad2, I>,
    ConfiguredPin<S, Pad3, I>,
);

The more I think about it, the less I like exposing either of these data structures outside the module. Both of these represent some sort of intermediate stage in configuration. They aren't just a collection of four Pins in a tuple. Each has trait bounds that progressively restrict them on their way from Pin to Pad. But none of the intermediate steps are particularly useful. I can't really see a use case for MatchingPins or ConfiguredPins outside of this module, except as an intermediate step before destructuring.

By making them local type aliases to tuples, users will never see these as well-defined data structures. Instead, they will either give a tuple of Pins to get_pads or get a tuple of Pins from get_pins. To me, that feels more aligned with what users would expect.

On the other hand, I think we should keep Pads defined as a tuple struct. I think there are use cases for a collection of all four Pads outside of this module. I defined it like this.

pub struct Pads<S, I> (
    Pad<S, Pad0, I>,
    Pad<S, Pad1, I>,
    Pad<S, Pad2, I>,
    Pad<S, Pad3, I>,
)
where
    S: Sercom,
    I: IoSet + Map<S, Pad0> + Map<S, Pad1> + Map<S, Pad2> + Map<S, Pad3>,
;

The get_pads and get_pins functions become

pub fn get_pads<S, I, M0, M1, M2, M3>(pins: MatchingPins<S, I, M0, M1, M2, M3>) -> Pads<S, I>
where
    S: Sercom,
    I: IoSet + Map<S, Pad0> + Map<S, Pad1> + Map<S, Pad2> + Map<S, Pad3>,
    M0: PinMode,
    M1: PinMode,
    M2: PinMode,
    M3: PinMode,
{
    Pads(pins.0.into(), pins.1.into(), pins.2.into(), pins.3.into())
}

pub fn get_pins<S, I>(pads: Pads<S, I>) -> ConfiguredPins<S, I>
where
    S: Sercom,
    I: IoSet + Map<S, Pad0> + Map<S, Pad1> + Map<S, Pad2> + Map<S, Pad3>,
{
    (pads.0.pin, pads.1.pin, pads.2.pin, pads.3.pin)
}

[blm768]

First, what is the utility of putting the Pin inside the Pad? It makes it easier to convert from Pad back to Pin, but otherwise, I don't see any other advantages. Am I missing something?

It also makes it slightly clearer that the Pad is taking ownership. Pin should be a zero-sized type, and it can be created "from the ether" when we take it back out of the Pad, but doing so should be an unsafe operation (so users can't create two independent Pin structs pointing at the same registers without using unsafe), and taking ownership explicitly would avoid the need for an unsafe block.

It's a minor point, though, so if the design doesn't allow it, we should be fine as long as we uphold the proper logical invariants.

Alternatively, we could restrict the stored Pin to the properly configured Pin. I think this is what you were imagining. However, to do that, we need to access the corresponding PinId and PeripheralConfig through the Map trait. But right now Map is defined on instances of Pad. You can't use a type's traits within the definition of the type. That doesn't really make sense.

We can get around that by defining Map on instances of IoSet instead. This was actually the original way I structured it. I moved away from that, because it required that Map take a Sercom and PadNum as type parameters, i.e. Map<S, P>. At the time, that felt redundant to me; we already had a something that took a Sercom and PadNum as type parameters, so why create another one? But I still have the instinct that Map really ought to be a trait on IoSet, since that is the crux of the pin mapping problem.

We might also be able to work around that by defining an auxiliary, private helper struct and implementing Map on that. One could argue that the mapping doesn't really belong to any one of Sercom, Pad, or IoSet, but is shared between them, and that type would be the "mediator". If one of those did "own" the mapping, though, IoSet seems reasonable.

The more I think about it, the less I like exposing either of these data structures [MatchingPins and ConfiguredPins] outside the module. Both of these represent some sort of intermediate stage in configuration.

By making them local type aliases to tuples, users will never see these as well-defined data structures. Instead, they will either give a tuple of Pins to get_pads or get a tuple of Pins from get_pins. To me, that feels more aligned with what users would expect.

That seems perfectly reasonable.

On the other hand, I think we should keep Pads defined as a tuple struct. I think there are use cases for a collection of all four Pads outside of this module.

I think I like that idea. Once we've got type-safety nailed down for Pad, we may even be able to eliminate the need for get_pins and get_pads entirely by making the fields of the struct pub. It seems like it should be impossible to abuse the API (without unsafe shenanigans, at least). Then users could just do something like this:

let pads: Pads<Sercom0, IoSet0> = Pads(pin0.into(), pin1.into(), pin2.into(), pin3.into()); // Might need .into().into()
let Pads(pin0, pin1, pin2, pin3) = pads;

For convenience, a Pads::new method that takes the four pins and automatically handles the into() stuff (so basically just get_pads) would be nice on top of that. I'm not sure yet whether it should take a tuple or just take the pins individually. I suppose that depends on whether it's possible to configure a Sercom with "missing" pins/pads (and whether we want to use a shorter tuple vs. a fixed-size, four-element tuple of Option<Pad<_, _>>).

[bradleyharden]

It also makes it slightly clearer that the Pad is taking ownership. Pin should be a zero-sized type, and it can be created "from the ether" when we take it back out of the Pad, but doing so should be an unsafe operation (so users can't create two independent Pin structs pointing at the same registers without using unsafe), and taking ownership explicitly would avoid the need for an unsafe block.

I was imagining that you would use pin = pad.into() to get back the Pin. No unsafe is required there. I agree, though, that keeping the Pin inside the Pad does make the ownership a bit more explicit.

It's a minor point, though, so if the design doesn't allow it, we should be fine as long as we uphold the proper logical invariants.

Moving Map to IoSet was enough to allow it. The code in my last post compiles just fine.

Once we've got type-safety nailed down for Pad, we may even be able to eliminate the need for get_pins and get_pads entirely by making the fields of the struct pub. It seems like it should be impossible to abuse the API (without unsafe shenanigans, at least). Then users could just do something like this

Actually, that's already possible. I just tried it. Without realizing it, I had completely obviated the need for get_pads. And now that the Pin is owned by the Pad, this also works:

let pins = (pads.0.pin, pads.1.pin, pads.2.pin, pads.3.pin);

I'll go ahead and remove get_pads and get_pins.

I suppose that depends on whether it's possible to configure a Sercom with "missing" pins/pads

I'm mostly interested in UART and SPI, but in both of those cases, using fewer than four pads is the norm, not an exception. I'm not sure how best to handle that. If I understand correctly, using any sort of enum, like Option, creates a non-zero-sized type, right? How does the compiler handle that? If it knows the correct enum variant at compile time, will it still optimize everything away? Or do we have to pay some run-time price for using Option? It might be worth it for clarity and simplicity. I'm just trying to understand the tradeoffs.

[bradleyharden]

Here's a random, mostly unrelated idea. Would it make sense to implement From for conversions between PinModes? For example,

impl<I, M> From<Pin<I, M>> for Pin<I, Output<PushPull>>
where
    I: PinId,
    M: PinMode,
{
    fn from(pin: Pin<I, M>) -> Self {
        pin.into_push_pull_output()
    }
}

I'm imagining use cases where the target PinMode is already constrained (like the case for Pad), and it would be easier to just use pin.into() rather than typing out pin.into_push_pull_output().

[blm768]

I was imagining that you would use pin = pad.into() to get back the Pin. No unsafe is required there. I agree, though, that keeping the Pin inside the Pad does make the ownership a bit more explicit.

Hmm. It seems like Pin::new should be unsafe. Speaking of which, I never found an implementation of Pin::new in your branch. Does that not exist yet, or am I just bad at reading code?

If I understand correctly, using any sort of enum, like Option, creates a non-zero-sized type, right?

Yes, that's true. I belived we'd have a one-byte overhead for each Option. That might not be enough to really worry about, but if we can avoid it without complicating the design, that seems worthwhile.

Thinking about this again, the existing SPI and UART types already sort of handle this. Rather than taking a full Pads struct, they take tuples of individual pads up to the number of items they care about, where the order of the items defines the mapping between pads and actual input/output lines. SpiMasterX, for instance, takes only three pads, which are always ordered as MISO, MOSI, and SCK. It seems like the solution to this will probably happen at the level of those types. Perhaps we won't actually need Pads in practice because the individual types of peripheral will define structs that better suit their own purposes.

In any case, allowing for optional pads is technically a separate issue (#58), so perhaps we should punt on that until we've got the basics ironed out and the rest of the HAL updated to work with the new design.

[blm768]

Here's a random, mostly unrelated idea. Would it make sense to implement From for conversions between PinModes?

I don't see why not. There would be cases where the type inference wouldn't be constrained enough to use those impls, but that's a common thing with From anyway. It would be at least situationally convenient.

[bradleyharden]

Hmm. It seems like Pin::new should be unsafe. Speaking of which, I never found an implementation of Pin::new in your branch. Does that not exist yet, or am I just bad at reading code?

Right now, it's pub(crate). But now that I'm storing the Pin in the Pad, my only usage outside of gpio is gone. I'll turn it back into a private function.

I posted links to the gpio and pads modules in my gpio_refactor branch somewhere in the posts above. I'll repeat them here, for convenience: gpio & pads.

Rather than taking a full Pads struct, they take tuples of individual pads up to the number of items they care about ... It seems like the solution to this will probably happen at the level of those types.

I agree.

Perhaps we won't actually need Pads in practice because the individual types of peripheral will define structs that better suit their own purposes.

I was initially hesitant to create the Pads struct for exactly that reason. But I think there's at least a possibility it could find use. If nothing else, it's an easy way to convert multiple pads simultaneously. You can just ignore the unused pads.

In any case, allowing for optional pads is technically a separate issue (#58), so perhaps we should punt on that until we've got the basics ironed out and the rest of the HAL updated to work with the new design.

I've actually been working on revamping the SPI module, and I can already see opportunities to simplify things based on the new structure. I'm going to try to come up with my own way to give optional Pads within the new framework. I'll let you know how it comes along.

[bradleyharden]

Back to the issue of converting Pins between different PinModes. I think I have a cleaner way to implement all of that.

Right now, we need individually named methods to convert Pins between modes. I realized that this approach is somewhat akin to the original macro approach. It uses brute force (a new method name) rather than traits and the type system to accomplish its goals.

Instead of method names, I wanted a way to do this.

// Don't do this
let pin = pin.into_push_pull_output()

// Do this
type PushPullOutput = Output<PushPull>;
let pin = pin.into::<PushPullOutput>()

At first, I wasn't sure how to implement that. But then it hit me. Converting a pin from one PinMode into another is an action associated with a particular PinMode. Naturally, the way to do that is with a trait on the PinMode. I was able to accomplish the above with the following code.

pub trait IntoMode: PinMode + Sized {
    fn into_mode<I: PinId, M: PinMode>(pin: Pin<I, M>) -> Pin<I, Self>;
}

impl IntoMode for PushPullOutput {
    fn into_mode<Id: PinId, Mode: PinMode>(pin: Pin<Id, Mode>) -> Pin<Id, Self> {
        let group = <Id::Group as Group>::group();
        // Set register values
        Pin::new()
    }
}

impl<Cfg> IntoMode for PFunc<Cfg>
where
    Cfg: PeripheralConfig,
{
    fn into_mode<Id: PinId, Mode: PinMode>(pin: Pin<Id, Mode>) -> Pin<Id, Self> {
        let group = <Id::Group as Group>::group();
        // Set register values using Cfg::NUM
        Pin::new()
    }
}

Each PinMode implements its own conversion as a trait. Then, you can call the corresponding conversion from a Pin method, with the desired PinMode as a type parameter on the function.

impl<Id, Mode> Pin<Id, Mode>
where
    Id: PinId,
    Mode: PinMode,
{
    pub fn into<NewMode: PinMode + IntoMode>(self) -> Pin<Id, NewMode> {
        <NewMode as IntoMode>::into_mode(self)
    }
}

I really like this approach. It feels more natural than the method name approach. What do you think?

[jessebraham]

Thanks to everybody for pushing forward on this! I'm really liking what I see so far. 🎉

I'm going to focus only on the gpio bits for now. I will address the changes to sercom in a later comment once I get a little more caught up. This is going to mostly be a brain-dump, I'll try my best to keep it coherent 😉

---

I have read through @bradleyharden's gpio_new.rs implementation, and I think it brings a lot of benefits to the table.

This new version is easier to read and more concise, I don't think anybody will really argue that. It's also easily extensible, which is always nice to have in a design. I like that it uses macros sparingly, and the macros defined tend to be quite small and thus relatively easy to grok. It also gives us a generic Pin type which is something we should have and that has been requested, so that's a bonus.

There were some less-good things (small things!) I feel should be addressed as well. Please keep in mind that these are probably approaching the realm of nitpicking.

My main issue with the current version is with the invocation of the pfunc and pins macros. They just seem a little unwieldy, and I think we can probably make some adjustments to make them a bit more friendly. For example, when calling pfunc we currently have:

pfunc!([A, 00, B, 01, C, 02, D, 03, E, 04, F, 05, G, 06,
        H, 07, I, 08, J, 09, K, 10, L, 11, M, 12, N, 13,]);

It's not immediately clear that there's any relationship between $Letter and $num; passing them in as tuples for example makes this relationship explicit:

pfunc!([
    (A, 00),
    (B, 01),
    ...
]);

This could just be a personal preference, but I feel explicitly stating relationships like these makes it more accessible to people unfamiliar with the code base. I'd like to hear others' opinions about this.

I'm not really sure what I'd do different with the pins macro honestly. Maybe we can try to be clever and construct the pin $X_ids automatically in the macro or something using paste. I'll need some time to think on that one probably, or maybe it's just fine. I'm going to poke around with this a bit over the weekend most likely.

Based on what I've seen, this should be fairly easy to adapt to the thumbv6m targets. They don't have gpio groups in the same way the thumbv7em targets do, but that's primarily an internal implementation detail.

---

Regarding the From impls, I don't really have any strong feelings regarding that honestly. Might be useful, might not. I'd like to hear what some others think of it.

I think that IntoMode trait is really neat! My only concern is that it is diverging from how most HALs I've seen tend to operate, but that's not in itself a bad thing. I'm not sure we really have an "official policy" regarding this. More discussion/experimentation is likely needed.

---

Sorry if I've missed it above, but has this module been tested on hardware yet?

[bradleyharden]

My main issue with the current version is with the invocation of the pfunc and pins macros.

@jessebraham, you found one of the times I got lazy... I agree the pfunc macro could be better. I wrote it quickly and didn't want to eat up 14 lines of screen real estate. But your approach is more explicit and better, especially for new readers of the code.

I'm not really sure what I'd do different with the pins macro honestly. Maybe we can try to be clever and construct the pin $X_ids automatically in the macro or something using paste.

This is a tough one. I thought a lot about how I could generate the identifiers. Unfortunately paste can only combine existing identifiers. There's no way to turn a literal into an identifier. I did see that paste is able to turn identifiers into strings for doc attributes, so I think it must be possible to do with procedural macros. I considered opening an issue to discuss this use case, but I never got around to it. It would also be really nice if we could provide a range, rather than specify every option from 0..=31. That's another possible feature for paste.

I think that IntoMode trait is really neat! My only concern is that it is diverging from how most HALs I've seen tend to operate

Yeah, I understand the perspective. There seems to be some discussion around this topic in other HALs, so maybe this is an opportunity to influence how other HALs approach the problem.

Sorry if I've missed it above, but has this module been tested on hardware yet?

No, it hasn't. I only have my custom board to work with, and I haven't tried it yet. I also think it would be pretty hard to test. Wouldn't I have to modify the rest of the HAL to be compatible with the new gpio and pads modules? I didn't want to do that until we had settled on the final architecture.

There are some other nitpicky things I want to discuss as well, particularly naming. For example, I'm not sold on PFunc. It was the best I could come up with at the time, but I'm not in love with it. Maybe Alternate would be better and more consistent with other HALs? Anyway, that's just an example. I was planning to create a tracking issue at some point to discuss these types of things before merging. For now, I think it probably makes sense to keep the discussion to the general architecture.

[bradleyharden]

If we're considering diverging from existing HALs (slightly), we might want to consider the content of this RFC. I thought there were some good points in it.

In general, it seems like embedded-hal might be better served to act in a reactionary manner. Let the individual HALs experiment with different architectures, then cherry pick the best pieces as guidelines. That makes for a slightly more fractured ecosystem, but it also allows more freedom for progress.

[blm768]

Once we reach the point where we want to test this on hardware, I've got a board that I could mess around with it on. It doesn't expose all the GPIOs, and my main UART is kinda broken, but I can at least make sure we can still blink an LED and run an SPI display.

I think I'd favor calling Pin's mode conversion into_mode instead of just into so it doesn't clash with std::convert::Into.

[bradleyharden]

I think I'd favor calling Pin's mode conversion into_mode instead of just into so it doesn't clash with std::convert::Into.

I actually chose it specifically to act as a replacement of std::convert::Into. If the target type is fully known, std::convert::Into is really nice.

\\ Suppose the Pad fully constrains the allowed Pin assignments
let pad.pin = pin.into();

But if the target type is unknown, std::convert::Into is really ugly.

let pin: Pin<PB24, PushPullOutput> = pin.into();

Methods like into_push_pull_output can clean this up.

let pin = pin.into_push_pull_output();

But with IntoMode, you get the best of both worlds. If the target type is unknown, you fall back to something that looks like the current method call.

let pin = pin.into::<PushPullOutput>();

But if the target type is known, then the compiler can infer the type parameter, and you can shorten it to

\\ Suppose the Pad fully constrains the allowed Pin assignments
let pad.pin = pin.into();

I would argue that we don't need or want to implement std::convert::Into, because our implementation is strictly better for our use case. And if we don't implement std::convert::Into, then there's no potential for confusion.

Is there some benefit to implementing std::convert::Into that I'm not seeing?

[bradleyharden]

I think I have a way to create optional Pads without requiring the run-time cost of an enum. It's getting late, and I'm headed to bed. But before I do, I wanted to share a preview, since I think it's a cool idea.

I got the idea by searching for "mutually exclusive traits". I found this StackOverflow answer which pointed to this Master's thesis where he gives an example of how to implement it on page 37 & 38.

This example compiles on the Playground.

use std::marker::PhantomData;

struct SomeT;
struct NoneT;

trait OptionT {}

impl OptionT for SomeT {}
impl OptionT for NoneT {}

struct Pad0;
struct Pad1;

trait PadNum {
    type Option: OptionT;
}

impl PadNum for Pad0 { type Option = SomeT; }
impl PadNum for Pad1 { type Option = SomeT; }
impl PadNum for NoneT { type Option = NoneT; }

struct Pad<P: PadNum> {
    padnum: PhantomData<P>
}

impl<P: PadNum> Pad<P> {

    fn can_always_run() -> Self {
        Pad { padnum: PhantomData }
    }

    fn can_only_run_with_valid_pad() -> Self
    where
        P: PadNum<Option = SomeT>
    {
        Pad { padnum: PhantomData }
    }

}

[blm768]

I suppose that if the compiler is able to resolve the ambiguity between the different versions of into without requiring something like <the_pin as Into>::into, that approach wouldn't be too bad.

Hmm. It seems like Pin::new should be unsafe.

Right now, it's pub(crate). But now that I'm storing the Pin in the Pad, my only usage outside of gpio is gone. I'll turn it back into a private function.

By typical convention, even private functions that could produce undefined behavior if called incorrectly should be marked unsafe.

[bradleyharden]

I suppose that if the compiler is able to resolve the ambiguity between the different versions of into without requiring something like <the_pin as Into>::into, that approach wouldn't be too bad.

Looks like your intuition was right. There is a problem with naming it into, but it's not the problem I was expecting. There wouldn't be type ambiguity in the case I mentioned, but there's another problem. We usually convert Pins to Pads with std::convert::Into. When I defined my new version of into in an inherent implementation, it began to override calls to the trait implementation of into. To transform a Pin to a Pad, I would have to use <pin as Into>::into(). I think your alternative is the best solution. We can name it into_mode instead, so we don't conflict with std::convert::Into.

By typical convention, even private functions that could produce undefined behavior if called incorrectly should be marked unsafe.

Hmm. I get the reasoning, but that doesn't seem to hold in practice across the code base. For instance, GenericClockController has a private constructor new that isn't marked as unsafe. I don't have a strong opinion either way, but if that's the philosphical stance we want to take, we should be consistent.

And by the way, I have a nearly complete overhaul of the spi module. It is based on the new Pad implementation, and it uses the OptionalT trick above to make individual Pads optional. I also parameterized the Spi struct by several of the configuration options that change the character size and register width, since they affect the type used in FullDuplex. I'll do a write-up on it soon.

[bradleyharden]

Here's the new SPI module. It doesn't require a configured clock yet, but that's a pretty minor piece. I wasn't able to use the current clock library for my purposes, since it has pretty limited options. I'm thinking about fleshing it out more, to cover at least a few of the possible configuration options.

I would write more about the SPI module and optional Pads, but it's pretty late. If you're curious, have a look at it. The Tx and Rx traits are probably the most interesting. That's where everything is fully constrained. You can trace pieces back from there to understand how it all works.

[blm768]

At least in broad strokes, I think I like the way the SPI stuff is going. I haven't read through the whole thing yet, but I've had a few thoughts so far:

• I think I'd favor combining ChSize and MSSEn, at least as far as the public API is concerned, so we have a single type parameter on Spi that takes one of three DataSize types rather than directly exposing the implementation detail of how the configuration registers map to those sizes.
• I'm not sure yet whether the enabled/disabled state should be a type parameter or a totally separate struct, but having two almost entirely disjoint sets of methods suggests that at least the public interface should use two differently named structs for the enabled and disabled states.

[bradleyharden]

@blm768, thanks for taking a look.

I think I'd favor combining ChSize and MSSEn, at least as far as the public API is concerned, so we have a single type parameter on Spi that takes one of three DataSize types rather than directly exposing the implementation detail of how the configuration registers map to those sizes.

I think you mean ChSize and Data32B. MSSEn is for the hardware control of SS. I see your point, and I think I probably agree, but I'm not sure what the best approach is. It's a little more complicated than three different DataSizes.

Enabling Data32B allows 32-bit reads and writes of the DATA register, but it doesn't require 32-bit transactions. You can use the LENGTH counter to create SPI transactions from 1-255 bytes. If we want to abstract away the implementation details, we should probably allow the user to specify the SPI transaction length, and then choose the appropriate settings. However, if we do that, I'm not sure it's worth supporting 9-bit transactions, since that is the only allowable transaction length that isn't a multiple of 8-bits. I can't imagine 9-bit transactions are common; I've certainly never seen it. And it would complicate the interface. If we allow users to specify the number of bits in a transaction, then most of the values are invalid. But if we restrict it to the number of bytes, then all u8 values are valid.

That actually brings up another issue. Right now, we only use the DRE flag when transmitting. But if you're using the LENGTH counter, I think you need to wait for a TXC flag before you start the next transaction. The wording of the datasheet isn't clear to me, so I'm not sure of the exact behavior here. I'll probably find out soon enough, because I plan to use it for 24-bit transactions. Do we want to handle that in the tx function? We would probably need to keep a local byte counter, so we know when to look for TXC instead of DRE.

There's also a lot of different behavior to consider in Slave mode, but I'm intentionally neglecting it, because I don't see it being used very often.

I'm not sure yet whether the enabled/disabled state should be a type parameter or a totally separate struct, but having two almost entirely disjoint sets of methods suggests that at least the public interface should use two differently named structs for the enabled and disabled states.

Hmm. Yeah, I guess that makes sense. I don't really have a strong opinion, at least not initially.

[blm768]

I think you mean ChSize and Data32B.

Oh, derp. Yeah, that's what I meant to write.

I think it's about time I actually read the datasheet.

[bradleyharden]

When you get to it, I'd like to hear how you interpret the text about the DRE and TXC flags.

[bradleyharden]

I'm not sure yet whether the enabled/disabled state should be a type parameter or a totally separate struct, but having two almost entirely disjoint sets of methods suggests that at least the public interface should use two differently named structs for the enabled and disabled states.

I just had a realization. If you make the enabled state a completely separate struct, you still have to parameterize it by all of the same type parameters, otherwise you can't implement the Tx and Rx traits correctly. And on top of that, you need to keep them so you can recover the correct type when returning to the disabled struct. To me, that indicates tight coupling, so I would probably argue to keep them the same struct. Unless you have a way to separate them more cleanly.

[blm768]

OK, looking at the datasheet, according to 35.6.3.8, paragraph 4, only 8-bit characters are supported with DATA32B set. Looking at the register descriptions section, it would appear that CHSIZE is just ignored in that case.

When you get to it, I'd like to hear how you interpret the text about the DRE and TXC flags.

It looks like they're both set by the hardware when the data register is emptied. My best guess is that before the first transaction, DRE is set and TXC is unset. After the first transaction, both flags would probably be set.

A non-interrupt implementation would probably poll them continuously until they were set so it could avoid clobbering the data while a transmission is in progress. Such an implementation might just query DRE before writing and ignore TXC so it doesn't have to wait for the transmission to finish before executing non-SPI-related instructions. An interrupt-based implementation would likely use them to determine which interrupt had fired. In either case, the implementation would clear them after checking them, either by writing directly to the flags or by refilling the DATA register.

I just had a realization. If you make the enabled state a completely separate struct, you still have to parameterize it by all of the same type parameters, otherwise you can't implement the Tx and Rx traits correctly. And on top of that, you need to keep them so you can recover the correct type when returning to the disabled struct. To me, that indicates tight coupling, so I would probably argue to keep them the same struct. Unless you have a way to separate them more cleanly.

They're tightly coupled, but they present very different interfaces. My main concern is with the Rustdoc output. Methods that are implemented conditionally on a type parameter are slightly harder to read (in my opinion) than separate types. Compiler error messages might be another concern. The two variants could share a common data struct internally to reduce friction in the implementation, but I think they'd be more discoverable to new users if their public interfaces were separated.

That's just my opinion, though, so take that with a grain of salt.

[bradleyharden]

I still owe you some thoughts on the SPI module.

But in the meantime, I couldn't sleep, and I had a cool idea. Previously, I had tried to implement core::covert::From between different PinModes, but I kept overlapping with the identity implementation in core, i.e. impl<T> From<T> for T. I couldn't figure out a way to restrict the Mode1 and Mode2 generics to only those cases where Mode1 is not Mode2. I concluded that I would need specialization to implement it with generics.

However, tonight I realized that where generics failed, macros can succeed. I remembered reading about recursive macros, and I realized this is a perfect case.

// Use a recursive macro to implement core::covert::From for each pair of PinModes
macro_rules! impl_core_from {
    ( $Mode1:ident, ) => {};
    ( $Mode1:ident, $($Mode2:ident,)* ) => {
        $(
            impl<I: PinId> From<Pin<I, $Mode1>> for Pin<I, $Mode2> {
                fn from(pin: Pin<I, $Mode1>) -> Self {
                    pin.into_mode()
                }
            }
            impl<I: PinId> From<Pin<I, $Mode2>> for Pin<I, $Mode1> {
                fn from(pin: Pin<I, $Mode2>) -> Self {
                    pin.into_mode()
                }
            }
        )*
        impl_core_from!($($Mode2,)*);
    };
}

impl_core_from!(
    Reset,
    FloatingInput,
    PullDownInput,
    PullUpInput,
    PushPullOutput,
    ReadableOutput,
    AlternateA,
    AlternateB,
    AlternateC,
    AlternateD,
    AlternateE,
    AlternateF,
    AlternateG,
    AlternateH,
    AlternateI,
    AlternateJ,
    AlternateK,
    AlternateL,
    AlternateM,
    AlternateN,
);

Now every possible conversion between modes can be accomplished using pin.into(). And if the type isn't already constrained, you can fall back to pin.into_mode::<FloatingInput>() or one of the literal versions, like pin.into_floating_input().

On top of that, you can now use Into<Pin<I, M>> as a trait bound, which covers a few more cases than previous alternatives. Right now, you would have to accept Pin<I, M> and call pin.into_mode(), but with Into<Pin<I, M>> as a trait bound, you could also accept a SERCOM Pad that can be converted back into a Pin. It's not huge, but I think it's a nice touch.

[jacobrosenthal]

If you're following this discussion, its getting to last chance to weigh in on https://github.com/atsamd-rs/atsamd/pull/302

[bradleyharden]

The first step of this refactor has been merged in #302.

[bradleyharden]

Part 2 of this refactor is proposed in #327.