Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: separate pad configurations from the HAL #73

Merged
merged 6 commits into from
Jul 13, 2020
Merged

Conversation

mciantyre
Copy link
Member

@mciantyre mciantyre commented Jul 8, 2020

RFC: separate pad configurations from the HAL

The PR introduces the imxrt-iomuxc crate family. The goal is to de-couple pad definitions from the HAL, supporting a split HAL (#56). Additional, the crates introduce a new pin muxing interface, reaching the ideal described in #26. Finally, the pad configurations and interfaces are re-usable without requiring any HAL, which supports out-of-tree driver development, and also encourages alternative HAL implementations, such as fully-asyc HALs.

I'm looking for feedback on the approach. If you'd like to try this out today, you can use the iomuxc-integration branch of the teensy4-rs project. I've updated and tested all (non-RTIC) examples with the new interface.

Proposal

  • Remove the HAL's iomuxc module.
  • Introduce imxrt-iomuxc, which provides
    • pin traits
    • pin configuration
    • helper code, targeted towards developers who implement pin crates
  • Introduce a pin implementation crate, called imxrt106x-iomuxc. The crate provides the pad implementations previously available in the HAL's iomuxc module.
  • Introduce imxrt-iomuxc-build, which is used by pin crate implementers. The crate lets us auto-generate Rust pad definitions at build time.
  • Integrate the imxrt-iomuxc interfaces into the HAL.
    • Until we have a split HAL, re-export imxrt106x-iomuxc from the current HAL.
  • As we consider a split HAL, we add new pin implementation crates, similar to imxrt106x-iomuxc.
  • Support the imxrt-iomuxc crate, and pin implementation crates, for HAL and driver designers.

Walkthrough

We introduce imxrt-iomuxc, the top-level crate for the IOMUXC crate family. In the spirit of the IOMUXC hardware peripheral, which provides pad configuration and multiplexing, we name these crates to include iomuxc.

imxrt-iomuxc defines pin traits. An example Pin trait for a UART-compatible pin resembles

/// A UART pin
pub trait Pin: super::IOMUX {
    /// The alternate value for the UART pin
    const ALT: u32;
    /// The daisy register which will select the pad
    const DAISY: Option<super::Daisy>;
    /// Pin direction
    type Direction: Direction;
    /// UART module; `U3` for `UART3`
    type Module: super::consts::Unsigned;
}

where

  • ALT is the alternate value.
  • DAISY is the daisy register.
  • Direction is a type tag for UART signals; currently either TX or RX.
  • Module is a typenum constant.

The alternate value is an associated constant. In today's iomuxc module, it's a type state. The type state leaks into user code, code that doesn't care about this implementation detail. The detail also leaks into our pad definition macros, which needs to know what alternate values a pad may take (see below). The associated constant lets us mark it once, then never worry about it again.

We favor typenum type-level constants. These replace the hand-rolled type constants that the HAL was using and exposing.

We have similar traits in today's iomuxc module. The only difference from today is in dependency management. We can use these traits independent of the HAL, which is not possible today.

A driver implementer uses these traits in their interfaces:

impl UART {
    pub fn new<T, R>(mut tx: T, mut rx: R, /* ... */) -> UART
    where
        T: Pin<Direction = TX>,
        R: Pin<Direction = RX, Module = <T as Pin>::Module>,
    {
        // Peripheral implementer resposible for calling unsafe functions
        unsafe {
            imxrt_iomuxc::uart::prepare(&mut tx);
            imxrt_iomuxc::uart::prepare(&mut rx);
        }
        // ...
    }

    pub unsafe fn new_unchecked(tx: ErasedPad, rx: ErasedPad) -> UART {
        // ...
    }
}

Notice the calls to prepare(). imxrt-iomuxc provides all functions for configuring pads. These functions are designed to a common trait, such that they're usable with strongly-typed pads or type-erased pads. The imxrt-iomuxc's interface is typically unsafe, which puts the onus on driver and HAL designers to guarantee safety. The unsafe interface also signals that these functions might not be suitable for normal user code. See the crate documentation for justification on the unsafe interfaces.

Also note the ErasedPad types. The new crates provide type-erased pads, which users may prefer over strongly-typed pads (at the cost of memory overhead). The documentation provides guidance on how to design interfaces to strongly-typed and type-erased pads.

We introduce the imxrt106x-iomuxc crate. The crate

  1. implements imxrt-iomuxc pin trats
  2. defines all of the available 106x pads

An example of 1 is

use imxrt_iomuxc::uart::{Pin, TX, RX};
use imxrt_iomuxc::consts::*;

impl Pin for AD_B1_03 {
    const ALT: u32 = 2;
    const DAISY: Option<Daisy> = Some(DAISY_LPUART2_RX_AD_B1_03);
    type Direction = RX;
    type Module = U2;
}

impl Pin for AD_B1_02 {
    const ALT: u32 = 2;
    const DAISY: Option<Daisy> = Some(DAISY_LPUART2_TX_AD_B1_02);
    type Direction = TX;
    type Module = U2;
}

Unlike today's iomuxc module, which heavily relies on macros, we can simply implement all Pin traits with associated constants and types from the imxrt-iomuxc crate. The constants prefixed with DAISY_* may be written by hand, or auto-generated from an i.MX RT SVD file. We introduce a script that generates these constants.

Note the AD_B1_03 pad type. This leads to 2: defining the processor pads. We generate pads at build-time using the imxrt-iomuxc-build crate:

// ~~ build.rs ~~
use imxrt_iomuxc_build as build;

let ad_b1 = build::PadRange::new("AD_B1", 0..16);
let b0 = build::PadRange::new("B0", 0..16);
// ...

let mut pads_rs = fs::File::create(out_dir.join("pads.rs"))?;

build::write_pads(
    &mut pads_rs,
    vec![&ad_b1, &b0, /* ... */],
)?;

We introduce the imxrt-iomuxc-build crate. Processor-specific crates like imxrt106x-iomuxc use build scripts to define the pads. The build scripts depend on imxrt-iomuxc-build. The build scripts generate

  • all processor pad types, organized by modules.
  • an implementation of the GPIO Pin traits, since all pads may be used as GPIOs.
  • a collection of Pads structs, which can be used to acquire pad objects.

Auto-generating the pads lets us quickly support other i.MX RT processor variants. We prefer to add pad definitions and pin implementations in separate crates (like imxrt101x-iomuxc), but we could provide definitions in the processor-specific HALs.

This PR includes all of these new crates, and it integrates the crates through the HAL. The crates have other details that aren't covered here, but are commented in source. Also see the documentation in the new crates.

This is a breaking change. The ways users reference and pass pad objects to HAL code is different. Additionally, there's a new GPIO driver which takes advantage of the new pad objects.

Discussion

The updated documentation, as well as the integration in the teensy4-rs project, shows that this achieves the goals described by #26. Do we think this is a sufficient approach for pin configuration?

What are our thoughts on using this as the foundation for split HALs, as part of #56? Pad definitions are one of the most processor-specific features we need to support, and this approach should let us de-couple the pad definitions from more general peripheral drivers.

I favor separating the pad definitions from the HAL. Specifically, I favor having a imxrt106x-iomuxc crate that's used by a imxrt106x-hal, rather than incorporating the contents directly into the HAL crate. These pin traits and pad implementations could have value for others, and keeping them separate from any HAL implementation could help others with prototyping. Do we think that's a worthy goal? Or, are there reasons that pad definitions should live in processor-specific HALs?

I went back and forth on how much Rust code we should auto-generate. At one point, I was writing a custom SVD -> Rust translator that would auto-generate all of the Pin implementations based on the SVD definitions. I eventually settled on this middle-ground, where we auto-generate the pad definitions, then manually implement Pin traits. I thought a fully-automated approach would be overkill, particularly since the number of processors we need to support is bounded (I have six different reference manuals; there's probably six crates we'd need to implement). The proposed approach does require us to manually check against the reference manuals. Should we consider more, or less, automation?

Any preference on maintaining these new crates in this repo? Or, should they live in a separate repo? This PR adds the new crates to this repo, but my first plan was to maintain them in a separate repo (the crates were all developed outside of the HAL).

TODOs

  • Ensure that the pad configuration interface is consistent with the previous iomux::pin_config module.
  • Use enum with no variants, instead of struct, for symbols that are intended as type-level tags.

TODO the GPIO module is gone. It needs to be re-implemented.
TODO need to document how a user should configure pads. The default
pad configurations are gone, so users who want to stay compatible
will need to provide the configurations themselves.
Much fewer macros, much easier to read, and nearly the same interface.
I'm adding these to the repo so that it's easier to review. We'll
talk about if we should continue maintaining in this repo, or in
a separate repo.

For my reference: the imxrt-iomuxc files track to commit d886fa4
in my local development repo.
The build-hal job will catch all formatting and lint errors when
checking the HAL. We just need to run the tests separately.
@mciantyre mciantyre added enhancement New feature or request imxrt-hal HAL related issue labels Jul 8, 2020
@mciantyre mciantyre requested a review from teburd July 8, 2020 23:41
@mciantyre mciantyre marked this pull request as ready for review July 8, 2020 23:50
Copy link
Member

@teburd teburd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this change set a lot, for many reasons.

First and foremost I think the doc comments (haven't checked) will be much much nicer. Which itself is a mega win.

I really like that now to add devices for each HAL is perhaps a common driver that takes iomux pins that impl the appropriate type. impl i2c::Pin for AD_B0 { ... } is amazingly simple, and will make porting things to other chips easier.

I really think this is one of the best change sets yet to this crate ecosystem. I'm hoping to find some time before the end of summer to make a 102x hal and evk crate, but we shall see. You might beat me to it at this rate!

I think splitting the crates up as you have here makes a lot of sense. Its sensible to have common drivers in their own crates in this repo. Keeping things in a mono repo is easier in my opinion, so I appreciate this setup.

@teburd teburd merged commit 1815db6 into master Jul 13, 2020
@mciantyre mciantyre deleted the iomuxc-integration branch July 21, 2020 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request imxrt-hal HAL related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants