Bugs in GPIO open drain, etc.

[arjanmels]

Current implementation does not actually set pins to open drain. Bit pad_driver of pinX register needs to be set.

• [x] Fix setting of open drain mode #36
• [x] For convenience modification to esp32 crate is desired to rename the pinX_pad_driver field to just pad_driver. #36
• [x] This needs a lot of lines in yaml files however, unless svdtools is modified to allow pattern in _strip fields. https://github.com/stm32-rs/svdtools/pull/28
• [x] Also we should allow "simultaneous" input and output pins. (e.g. for open drain, but also for peripherals like i2c which do direction switching themselves). #36
• [x] Change gpio api for improved ergonomics. #36
• [x] Support alternate functions in peripherals. #36

[MabezDev]

Current implementation does not actually set pins to open drain. Bit pad_driver of pinX register needs to be set.

Good spot!

For convenience modification to esp32 crate is desired to rename the pinX_pad_driver field to just pad_driver.

I added bitfield stripping in https://github.com/stm32-rs/svdtools/blob/master/CHANGELOG.md#v004-2020-01-12 Which should help!

Also we should allow "simultaneous" input and output pins. (e.g. for open drain, but also for peripherals like i2c which do direction switching themselves).

I agree I think we should limit the into_X methods to the following:

• into_output
• into_input
• into_analog
• into_alternate (I'm guessing alternate mode should both enable input and output mode and let the peripheral handle it?)

Then have extra methods on these types to do the other stuff, like setting open_drain mode, pullups etc. For example stm32 land, I typically use something like this configure the i2c pins:

let scl = gpiob
            .pb8
            .into_alternate_af4()
            .set_open_drain()
            .internal_pull_up(true)
            .set_speed(Speed::VeryHigh);

Which I think would be very applicable to this HAL. Thoughts?

[arjanmels]

I added bitfield stripping in https://github.com/stm32-rs/svdtools/blob/master/CHANGELOG.md#v004-2020-01-12 Which should help!

Yep, busy expanding this with pattern matching to not have to have separete lines for all 40 gpios

Which I think would be very applicable to this HAL. Thoughts?

I agree that we need to limit the amount of into_xxx. Still think we may also want into_io (or into_inouput or into_input_output), this is quite a common case and bit clumsy to have to handle via alternate mode 3.

And we need to implement functions on the gpios to handle the multiplexer routing (something like gpio.connect_output_to(PERIPHERAL), gpio.connect_input_to(PERIPHERAL).

[arjanmels]

@MabezDev and @chocol4te I reviewed various stm32 and nrf51 gpio implementations.

They all use into_open_drain. None has concept of IO (Input/Output). I'll indeed add helper functions for advanced config (like pullup/down, strength and enable input on an output).

@chocol4te Why do you prefer the nrf51 implementation. I think the syntax with split is nicer. Also they still use macros, so I don't see the key difference.

[fmckeogh]

@arjanmels Mainly it was the ergonomics of making peripherals with generic constraints on GPIO types, e.g. Pin<AF3> as opposed to PINS where PINS: I2cPins and impl I2cPins for (Gpio4, Gpio5) (pseudocode). Generally I think expressing requirements using the type system is cleaner than generating the impls with macros.

The pattern used in STM32 makes sense because those peripherals tend to only work on a low number of pin groups, and similarly for NRF where nearly every peripheral can use any pin. ESP32 seems to be a middle ground where some peripherals require specific pins and others work on any.

I don’t feel strongly either way though, I can see the merits of both, so please do whatever you feel is best :)

[arjanmels]

@chocol4te Ok, I'll take another look at that aspect.

I will start including changes into PR #36.

[MabezDev]

I'll indeed add helper functions for advanced config (like pullup/down, strength and enable input on an output

The stm32-f4xx-hal does something like this which is where I was going with my example https://github.com/stm32-rs/stm32f4xx-hal/blob/master/src/gpio.rs#L587-L655

We could have many helper into_X functions for common cases like you said. Then for alternate modes with could have methods like set_open_drain, enable_input, enable_output etc, for more advanced configuration.

I was looking at the esp-idf implementation and the peripheral driver configures the pins, it would be really nice if we could just pass the pins in the correct alternate function mode and let the peripheral driver do the configuration.

Very rough idea and not how I'd actually implement it, but the basic idea:

pub trait AlternateFunction { // implemented on every pin
  fn enable_input(&mut self) -> &mut Self;
  fn enable_output(&mut self) -> &mut Self;
  // etc etc
}

// i2c driver

impl I2c {
  // can generically accept any valid i2c pins, and correctly configure them
  // in a the real impl, there would be constraints for the specific pins configurations (see stm32 and nrf hals for examples)
  pub fn new<I2C, SCL: AlternateFunction, SDA: AlternateFunction>(i2c: I2C, scl: SCL, sda: SDA) {
    sda
        .enable_input()
        .enable_output(); 
   // etc, basically the i2c driver knows how the pin is meant to be setup, so let it configure it.
  }
}

Thoughts on that? I would be happy to try and impl this for a more indepth example

[arjanmels]

Agree with your thinking on into functions versus helpers. The into functions are now simplified and corrected in #36. Helper functions are easy to add.

Regarding alternate functions: I agree that the peripherals should be responsible for the configuration.

Note that there are 2 types of alternate function on esp32:

• Flexibily routed via gpio matrix (this will additionally need helper functions connect_input_to_function , connect_function_to_output). Here no separete pin type is needed, but function can just use most appropriate either Input or Output (or general non-typed).
• Fixed routing via iomux (for high speed peripherals). Here only fixed pins can be used and the AF1-7 settings are used.

I am not yet sure how to pass in either the general or the dedicated AF via the same parameter. This needs some more thought.

[arjanmels]

I made another round over gpio. I arrayed the registers in the esp32 crate, this makes for a cleaner implementation (and was really needed to make the connect_input_to_peripheral possible.

I also improved the ergonomics via extra traits. See #36 and example usage in https://github.com/arjanmels/esp32-hal/blob/feature-i2c-am/src/i2c.rs

Still working on the implementation for the alternate functions

@MabezDev and @chocol4te: what do you think?

Also I got an idea to use the into and from traits to improve the ergonomics further. Idea is that you can declare functions/structs as e.g. new(sda: Into<Output<OpenDrain>>, sda: Into<Output<OpenDrain>>) and new_via_iomux(sda: Into<Alternate<AF1>>. scl: Into<Alternate<AF1>>). Then you can pass any pin which can be transformed into these properties into the peripheral and you can call sda.into() to get pin properly configured and with the proper type.

[fmckeogh]

Looks really great, the automatic configuration with a call to .into() is just awesome :D

Edit: I do get quite a few build errors when I tried to test the branch, assuming this is expected as its a WIP?

[arjanmels]

@chocol4te The branch should build, but you do require the latest github version of esp32, which needs version 0.1.6 of svdtools (pip3 install --upgrade svdtools)

[jessebraham]

... which needs version 1.6 of svdtools

Typo here, this should be version 0.1.6.

[arjanmels]

... which needs version 1.6 of svdtools

Typo here, this should be version 0.1.6.

Thanks, I corrected it.

[arjanmels]

Closed by #36