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

Instructions: Migration to v2 GPIO API #363

Closed
jaredwolff opened this issue Dec 28, 2020 · 27 comments
Closed

Instructions: Migration to v2 GPIO API #363

jaredwolff opened this issue Dec 28, 2020 · 27 comments

Comments

@jaredwolff
Copy link

jaredwolff commented Dec 28, 2020

I've been working on converting to the new V2 GPIO API. It was confusing at first how to migrate so this may help some folks:

  1. Remove define_pins! macro from your lib.rs file
  2. Include this at the top of your example/main use hal::gpio::v2::Pins; (where halis extern crate <your board> as hal;
  3. Then access the pins as shown in the documentation
    let pins = Pins::new(peripherals.PORT);
    let mut red_led = pins.pa23.into_push_pull_output();

I could be wrong here so any additions would be great.

@jaredwolff
Copy link
Author

jaredwolff commented Dec 29, 2020

Another thing that I needed to get working is USB. I'm using an SAMD21 so /hal/src/samd21/usb/mod.rs needs to be tweaked:

//! USB Device support

#[cfg(not(feature = "unproven"))]
use crate::gpio;
#[cfg(feature = "unproven")]
use crate::gpio::v2::{AlternateG, Pin, PA23, PA24, PA25};

pub use usb_device;

mod bus;
pub use self::bus::UsbBus;

mod devicedesc;
use self::devicedesc::Descriptors;

/// Emit SOF at 1Khz on this pin when configured as function G
#[cfg(feature = "unproven")]
pub type SofPad = Pin<PA23, AlternateG>;
#[cfg(not(feature = "unproven"))]
pub type SofPad = gpio::Pa23<gpio::PfG>;

/// USB D- is connected here
#[cfg(feature = "unproven")]
pub type DmPad = Pin<PA24, AlternateG>;
#[cfg(not(feature = "unproven"))]
pub type DmPad = gpio::Pa24<gpio::PfG>;

/// USB D+ is connected here
#[cfg(feature = "unproven")]
pub type DpPad = Pin<PA25, AlternateG>;
#[cfg(not(feature = "unproven"))]
pub type DpPad = gpio::Pa25<gpio::PfG>;

This is merely a bandaid until all projects/examples can move to the v2 API. Similar to what's proposed in #362

In my BSP lib.rs I've changed the usb_allocator function to:

use hal::gpio::v2::{Floating, Input, Pin, PA24, PA25};

...

#[cfg(feature = "usb")]
pub fn usb_allocator(
    usb: pac::USB,
    clocks: &mut GenericClockController,
    pm: &mut pac::PM,
    dm: Pin<PA24, Input<Floating>>,
    dp: Pin<PA25, Input<Floating>>,
) -> UsbBusAllocator<UsbBus> {
    let gclk0 = clocks.gclk0();
    let usb_clock = &clocks.usb(&gclk0).unwrap();

    UsbBusAllocator::new(UsbBus::new(
        usb_clock,
        pm,
        dm.into_alternate(),
        dp.into_alternate(),
        usb,
    ))
}

Then your main.rs may look something like:

extern crate <your board> as hal;
extern crate cortex_m;
extern crate panic_halt;
extern crate usb_device;
extern crate usbd_serial;

use hal::clock::GenericClockController;
use hal::entry;
use hal::gpio::v2::*;
use hal::pac::{interrupt, CorePeripherals, Peripherals};
use hal::prelude::*;

use hal::usb::UsbBus;
use usb_device::bus::UsbBusAllocator;

use usb_device::prelude::*;
use usbd_serial::{SerialPort, USB_CLASS_CDC};

use cortex_m::asm::delay as cycle_delay;
use cortex_m::peripheral::NVIC;

#[entry]
fn main() -> ! {
    let mut peripherals = Peripherals::take().unwrap();
    let mut core = CorePeripherals::take().unwrap();
    let mut clocks = GenericClockController::with_internal_32kosc(
        peripherals.GCLK,
        &mut peripherals.PM,
        &mut peripherals.SYSCTRL,
        &mut peripherals.NVMCTRL,
    );
    let pins = Pins::new(peripherals.PORT);
    let dm = pins.pa24.into_floating_input();
    let dp = pins.pa25.into_floating_input();

    let bus_allocator = unsafe {
        USB_ALLOCATOR = Some(hal::usb_allocator(
            peripherals.USB,
            &mut clocks,
            &mut peripherals.PM,
            dm,
            dp,
        ));
        USB_ALLOCATOR.as_ref().unwrap()
    };

Hope this helps folks change things over so they can use DynPin etc.

@bradleyharden
Copy link
Contributor

@jaredwolff, thanks for this. I haven't gone through it all, but you point out something valid. We should definitely have migration instructions. Maybe we can add it to the docs.

Over the long weekend, I want to go through this and the work from @ryankurte. If either of you have any feedback on v2, please let me know. I don't think it's too late to make changes.

@jaredwolff
Copy link
Author

Maybe we can add it to the docs.

Happy to help where I can. :)

If either of you have any feedback on v2, please let me know. I don't think it's too late to make changes.

One thing I've noticed, thus far, is there's no open drain output as before. That would be handy to have especially if you only need to drive the N-side. I haven't looked at the data sheet so I'm not sure if this is a specific mode or its basically putting the pin back into floating input when not being driven.

After going through the above I finally was able to collect my pins and use them as DynPin the jury is still out on that. So far so good though.

Is it possible to read and write directly to the PORT as well? That's also immensely useful if you don't want to define a bunch of pins then end put put side by side in an array.

Also, as mentioned earlier, some way to alias the pins would be great. I was also thinking about somehow when creating the stand alone enums that there could be type definitions for all trait combinations? Right now I'm using a separate file where I'm defining the pin types for use across the firmware:

use hal::gpio::v2::{Output, Pin, PushPull, PA17, PA22, PA23, PB14};

pub type LedPassT = Pin<PA23, Output<PushPull>>;
pub type LedFailT = Pin<PA22, Output<PushPull>>;

pub type PsEnT = Pin<PB14, Output<PushPull>>;
pub type VbatEnT = Pin<PA17, Output<PushPull>>;

It's handy especially when using something like rtic. (Here's what my Resources looks like:

#[rtic::app(device = hal::pac, peripherals = true)]
const APP: () = {
    struct Resources {
        bus_allocator: &'static UsbBusAllocator<UsbBus>,
        usb_serial_port: SerialPort<'static, UsbBus>,
        usb_dev: UsbDevice<'static, UsbBus>,
        p: Producer<'static, Command, U8>,
        c: Consumer<'static, Command, U8>,
        led_pass: types::LedPassT,
        led_fail: types::LedFailT,
        ps_en: types::PsEnT,
        vbat_en: types::VbatEnT,
        test_pins: Vec<DynPin, U23>,
    }

I'll likely do a write up, at the very least, about using atsamd-hal with rtic. I'd love to add some rtic examples once a BSP solution has been sorted out. 👍

This is me spitballing though. I've been pouring through here for the past few days straight. Take what you want and ignore the rest. Thanks for all your hard work on this. Getting the blinky LEDs and USB serial working made my weekend.

@bradleyharden
Copy link
Contributor

The SAM chips don't actually have open drain outputs. That's something I realized when writing v2. In fact, if you look at the old v1 code, you can see that push pull and open drain use the same settings. I think the original version of gpio must have been copypasta from another chip.

I was also thinking about somehow when creating the stand alone enums that there could be type definitions for all trait combinations?

Sorry, I'm not sure what you mean by this. What enums? And what trait combinations?

@jaredwolff
Copy link
Author

I think the original version of gpio must have been copypasta from another chip.

Got it. I figured as much as well. If someone wants to drive it like an open drain they can always set it to floating input and then back to an output I suppose. I'm looking at how the GPIOs get converted here Probably not worth the extra effort to create a new trait or extra handling in the set_high()/set_low() methods. If I do find myself needing it more I'll tinker to see if I can come up with something.

Sorry, I'm not sure what you mean by this. What enums? And what trait combinations?

Sorry, let me explain better. When the define_pins macro is called it generates the PAXX enums. Maybe it's not the best place but having all the different types defined of a pin may be useful. Otherwise someone has to go in manually and create the types. (This is more painful when the board pins are aliased. It kinda negates the whole purpose of an alias)

For example:

I can create this guy and use it throughout my code. It's handy for making the pins static variables that can be modified anywhere in the code.

pub type LedPassT = Pin<PA15, Output<PushPull>>;

Then used later like:

struct Resources {
    led_pass: types::LedPassT,
}

This gets hairy when you have to go back and look at what the pin was defined as in the BSP (originally before the v2 changes):

pin led_pass = a15,

So later if I wanted to use Pins.led_pass I'd have to go back, look up led_pass and define it manually everywhere or create a type like LedPassT. This is particularly applicable to rtic but applies if you want to create/define any mutable static variable for controlling GPIOS. (Unless I'm missing something here)

I hope that made sense!

@bradleyharden
Copy link
Contributor

bradleyharden commented Jan 2, 2021

@jaredwolff, a few comments:

This is merely a bandaid until all projects/examples can move to the v2 API. Similar to what's proposed in #362

I just added a comment to #362. Edit: I added a comment to #365 about #362. Now that v0.12 has been released, there are implementations of From between v1 and v2 pins. If you want to support both versions, you can make the type generic with a bound like Into<Pin<PA24, AlternateH>>.

Is it possible to read and write directly to the PORT as well?

The v2::Pins struct has a port method that will give you a reference to the PORT. It's unsafe, but it should be useful as an escape hatch, if you need it.

You can also use the free method. However, I just now realized that it won't work the way I thought it would. When I wrote it, I thought you could move pins out of the Pins struct, then call free to get the PORT and use it to control the remaining pins. But I realized just now that moving some of the pins out of the Pins struct will mean you can't call free, because it takes ownership of the Pins struct, which has been partially moved. If you need an owned PORT, you probably want to steal the peripherals and take it from there.

If someone wants to drive it like an open drain they can always set it to floating input and then back to an output I suppose.

That's an interesting idea. I wouldn't really call that open drain. It's more like a software emulated tri-state buffer. But it still might be useful. I would probably design that as a feature built on top of the gpio module. You could create a struct that holds a pin and manages its state for you.

When the define_pins macro is called it generates the PAXX enums.

As mentioned in #364, I think the unfortunate naming coincidence is causing some confusion here. The v2 define_pins macro is not meant for users. It's purely an internal macro used to reduce code duplication.

It sounds like you want the BSP to define type aliases for each pin in the corresponding mode for its use, e.g. LedPass in your example. I'll try to create a new define_pins macro that also allows you to create type aliases like that. It probably won't save much typing for BSP authors, but at least it will be there as a way to encourage them to add the type aliases.

I think that addresses all of your questions. Did I miss anything?

@bradleyharden
Copy link
Contributor

One more thing. If you're using DynPins, you might want to note the following. The PinId and PinMode traits both have DYN associated constants. For each type that implements them (e.g. PA24 for PinId or AlternateH for PinMode), the DYN constant is equal to the corresponding DynPinId or DynPinMode. That might come in handy. I'm not sure. Unfortunately, to access those constants, you might need to use the fully specified syntax, e.g.

<PA24 as PinId>::DYN

You could also access it through the type aliases defined above, e.g.

<<LedPass as AnyPin>::Id as PinId>::DYN

I think the context would determine which associated types/constants would require the full syntax to access.

BSP authors could also add constants that act like the type aliases.

pub const LED_PASS_ID: DynPinId = DynPinId { group: DynGroup::A, num: 24 };
pub const LED_PASS_MODE: DynPinMode = DYN_ALTERNATE_H;
// Users can't create `DynPin`s directly, so a BSP can only define the IDs and modes

@bradleyharden
Copy link
Contributor

bradleyharden commented Jan 3, 2021

@jaredwolff and @sajattack, how does this look as an updated version of the v1 define_pins macro?

The macro would be called like this:

bsp_pins!(
    #[cfg(feature = "unproven")]
    PA24 {
        name: led_pass,
        aliases: {
            AlternateH: LedPass,
            #[cfg(feature = "usb")]
            AlternateM: UsbPin
        }
    }
);

And it would expand to this:

pub mod bsp_pins_mod {
    use atsamd_hal::target_device as pac;
    use atsamd_hal::gpio::v2::{
        self as gpio, Pin, PinId, PinMode, Reset, DynPinId, DynPinMode
    };

    pub struct Pins {
            #[cfg(feature = "unproven")]
            pub led_pass: Pin<gpio::PA24, Reset>,
    }

    impl Pins {
        pub fn new(port: pac::PORT) -> Self {
            let pins = gpio::Pins::new(port);
            Self {
                #[cfg(feature = "unproven")]
                led_pass: pins.pa24,
            }
        }
    }

    #[cfg(feature = "unproven")]
    pub type LedPass = Pin<gpio::PA24, gpio::AlternateH>;
    #[cfg(feature = "unproven")]
    pub const LED_PASS_ID: DynPinId = <gpio::PA24 as PinId>::DYN;
    #[cfg(feature = "unproven")]
    pub const LED_PASS_MODE: DynPinMode = <gpio::AlternateH as PinMode>::DYN;

    #[cfg(feature = "unproven")]
    #[cfg(feature = "usb")]
    pub type UsbPin = Pin<gpio::PA24, gpio::AlternateM>;
    #[cfg(feature = "unproven")]
    #[cfg(feature = "usb")]
    pub const USB_PIN_ID: DynPinId = <gpio::PA24 as PinId>::DYN;
    #[cfg(feature = "unproven")]
    #[cfg(feature = "usb")]
    pub const USB_PIN_MODE: DynPinMode = <gpio::AlternateM as PinMode>::DYN;
}
pub use bsp_pins_mod::*;

@jaredwolff
Copy link
Author

That's an interesting idea. I wouldn't really call that open drain. It's more like a software emulated tri-state buffer. But it still might be useful. I would probably design that as a feature built on top of the gpio module. You could create a struct that holds a pin and manages its state for you.

Oh yea Tri-state would definitely be a better way to refer it as.

@jaredwolff and @sajattack, how does this look as an updated version of the v1 define_pins macro?

On first glance, it seems like it covers all the pain points I described earlier. Happy to kick the tires when it's ready for testing.

@bradleyharden
Copy link
Contributor

Well, I was just about ready to make a PR for you to test, but I did a really stupid git reset --hard and lost the entire thing, macro and documentation. Now I'm frustrated and don't want to work on it anymore. Sorry about that. I'll try to redo it tomorrow.

@jaredwolff
Copy link
Author

Oh crap! Sorry about that I've been there.

Do you have anything running backups in the background? Backblaze and Time Machine have saved my butt on multiple occasions like this.

@bradleyharden
Copy link
Contributor

HA! VSCode saved the day. It had everything in the undo history. Give me a minute.

@jaredwolff
Copy link
Author

Yesssss. VSCode for the win.

@bradleyharden
Copy link
Contributor

Once the other two GPIO PRs are accepted, I'll go back and add a little more detail to the Migration section in the gpio module docs.

@bradleyharden
Copy link
Contributor

bradleyharden commented Feb 27, 2021

@jaredwolff, could you check out the content in #411 and see if it covers all the problems/questions you ran into with gpio::v2? I added a bunch to the top-level gpio module docs. If you wait until it's merged, you should be able to view it rendered here. If you still have things to address, I'll create another PR.

@jaredwolff
Copy link
Author

Nice I'll check it out @bradleyharden

@bradleyharden
Copy link
Contributor

@jaredwolff, did you ever get a chance to check out the updated documentation? I added some discussion to the base gpio module-level docs.

Has anyone else taken a look? Can we close this issue? Are there any further thoughts on improving the documentation?

@jaredwolff
Copy link
Author

Hey @bradleyharden not yet. I've been swamped with work. It's on my list. I don't know when I can get to it though. 😞

@bradleyharden
Copy link
Contributor

No worries. Just trying to see what issues can be closed.

@jaredwolff
Copy link
Author

@bradleyharden so far here's a few bits of feedback since I have not looked/played with this in a while:

  • It was hard to find info about the gpio module since it was buried in the overarching common module. I did get to it finally by simply searching gpio. This isn't necessarily your fault, but the main page of the documentation is less than inviting.
  • Once I got there, the documentation is thorough and provides some great examples of converting between different GPIO modes. (Well done!)
  • I wasn't sure what the purpose of an Optional pin is. Or the use case. That may need more explanation if it's worth it.

Short of having some actual examples, this is a great start. (more for folks who are starting from zero) I have implemented a few examples here for the board I've been using this with. I have noticed some of the boards in this repo have been converted/are using V2 which is awesome!

@bradleyharden
Copy link
Contributor

@jaredwolff, a few replies:

  • @vccggorski is leading an effort to address some of these issues in API cleanup #426. In fact, Flatten HAL and move common content to crate root #425 removes the common module and lifts all of its submodules, including gpio, to the top level of the HAL.
  • I recently decided to use the typelevel module as a place to document some of the higher-kinded type patterns used throughout the HAL. When I publish it, I'll be sure to include a section on OptionalKind traits and their uses. I can point to that from the OptionalPin documentation.

If you're interested, you can take a look at the sercom::v2::spi::Pads struct to see an example using OptionalPad. However, note that I'll soon be making some changes to simplify and generalize that struct. The changes will also support the analogous struct in any future v2::uart or v2::i2c module.

@bradleyharden
Copy link
Contributor

I see you're using of the changes I made to the USB modules to support v2::Pins. I should just bite the bullet and make the same changes to the rest of the HAL. With the AnyPin trait, it's really easy to support both v1::Pins and v2::Pins simultaneously. It just takes time to modify the code and all the BSPs.

@jaredwolff
Copy link
Author

@bradleyharden yea you could bite the bullet. Is there a future benefit of using an AnyPin trait since V1 will be going bye bye soon enough?

@bradleyharden
Copy link
Contributor

Sometimes, and sometimes not. It can help save a type parameter in some cases. But I'm not sure the v1 modules will be deprecated so quickly. A lot of code depends on them, both in the HAL and in the BSPs. There's a lot of refactoring to do.

@jaredwolff
Copy link
Author

Yea that's true! That is a big mountain to climb. 😅

@jaredwolff
Copy link
Author

@bradleyharden one thing I noticed is that there's no way to know how to utilize the pins once they're set. There's no clear path on how to write or read a pin depending on its mode. It's hard to find since it seems to be in a different location. May be good to add to the examples though.

@bradleyharden
Copy link
Contributor

@jaredwolff, that's a good point. The only way to do so currently is through the embedded-hal traits, but that is not communicated explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants