added the possibility to add None gpio pins

[alexandruradovici]

Pull Request Overview

Following the discussion for #1675, this pull request adds the possibility to add None gpio pins, basically preserving pin index numbers when some of the gpio pins are not exported.

I make a change to:

• the gpio capsule, taking Option<&InterruptValuePin>
• the gpio_component_helper macro, so that boards may declare None pins

The format is a little strange, boards need to declare

Some => pin
// or
None => any_expression_here

Is there any way to check inside a macro if the value supplied is None (textual None) or an expression?

Any feedback is welcome.

[bradjc]

The one bit of weirdness doing this adds is the return value for command 0, aka the number of pins on the board. I'm not sure if that should return the number of array items or the number of actual pins. Perhaps array items is right for now, and if we add more return values in 2.0 we could return both numbers.

In some sense this change goes farther from making apps easily portable. However, I don't believe that any of the gpio/adc/spi/etc. capsules will ever be conducive to portable apps. In that case, I like this change because it gives more flexibility to board authors to make an intuitive interface for the low-level hardware resources they want to provide to userspace. For example, imix has no pins labeled "D0" or "D1" (but has D2-D8). Should the board author make pin "D2" index 0 in the userspace code? Or should it be index 2? I argue the author should be able to decide what is a better representation of the hardware for their users, and this change would let them decide.

[ppannuto]

I quite like this. Since there's some lingering issues with the macro already, I might offer a different thought on its design: Should we maybe be more explicit about the indices, in a way that would make lookup easier and the intent of the Nones clearer, perhaps:

    let gpio = GpioComponent::new(board_kernel).finalize(components::gpio_component_array!(
        0: stm32f4xx::gpio::PIN[6][9].as_ref().unwrap(), //D0
        1: stm32f4xx::gpio::PIN[6][14].as_ref().unwrap(), //D1
        @gap,
        5: stm32f4xx::gpio::PIN[4][11].as_ref().unwrap(), //D5
        6: stm32f4xx::gpio::PIN[4][9].as_ref().unwrap(), //D6
        @end
    )

This borrows a bit from the registers interface, that only requires you to specify the hardware that is present and lets the macro fill in the gaps automatically. It also makes it clearer that this list is actually an array intended to be indexed.

Thoughts?

[alexandruradovici]

As @ppannuto suggested, I modified the macro to have the following form:

let gpio = components::gpio::GpioComponent::new(board_kernel).finalize(
components::gpio_component_helper!(
     // pin number => pin reference
     2 => &nrf52840::gpio::PORT[GPIO_D2],
     3 => &nrf52840::gpio::PORT[GPIO_D3],
     4 => &nrf52840::gpio::PORT[GPIO_D4],
     5 => &nrf52840::gpio::PORT[GPIO_D5],
     6 => &nrf52840::gpio::PORT[GPIO_D6],
     7 => &nrf52840::gpio::PORT[GPIO_D7],
     8 => &nrf52840::gpio::PORT[GPIO_D8],
     9 => &nrf52840::gpio::PORT[GPIO_D9],
     10 => &nrf52840::gpio::PORT[GPIO_D10]
));

The @gap was not very useful, as gaps can be computed from the values of the pin number. Any thoughts about this?

[ppannuto]

I think this looks great! And yeah, the @gap @end stuff doesn't really make sense for this use case.

It looks like that travis error is real, but otherwise I think this is good to go.

[alexandruradovici]

I'll make the changes for all the platform.

[alexandruradovici]

I have a question related to the pin assignment for:

hail I assigned the pins according to their Dx, so instead of being 0,1,2,3 they will be 0, 1, None ..., 6, 7. Is this ok?

0 => &sam4l::gpio::PB[14], // DO
1 => &sam4l::gpio::PB[15], // D1
6 => &sam4l::gpio::PB[11], // D6
7 => &sam4l::gpio::PB[12]  // D7

nrf52dk As all the pins are on P0 and seem to have printed numbers on the PCB, I assigned them like this:

// Bottom right header on DK board
3 => &nrf52832::gpio::PORT[Pin::P0_03],
4 => &nrf52832::gpio::PORT[Pin::P0_04],
28 => &nrf52832::gpio::PORT[Pin::P0_28],
29 => &nrf52832::gpio::PORT[Pin::P0_29],
30 => &nrf52832::gpio::PORT[Pin::P0_30],
31 => &nrf52832::gpio::PORT[Pin::P0_31],
// Top mid header on DK board
12 => &nrf52832::gpio::PORT[Pin::P0_12],
11 => &nrf52832::gpio::PORT[Pin::P0_11],
// Top left header on DK board
27 => &nrf52832::gpio::PORT[Pin::P0_27],
26 => &nrf52832::gpio::PORT[Pin::P0_26],
2 => &nrf52832::gpio::PORT[Pin::P0_02],
25 => &nrf52832::gpio::PORT[Pin::P0_25]

Is this ok, or should they be assigned in sequence?

[ppannuto]

In both cases I think this is the correct choice. Having the software id match the number that is printed on the board makes a lot of sense to me.

[ppannuto]

Tiny nits, but then I think this is probably good to go post-1.5 release

[ppannuto]

This is awesome!

Blocked pending 1.5 release

[bradjc]

I like the flexibility, but I worry about the breaking changes to boards. It's probably better to not change boards like opentitan and hail.

[ppannuto]

We made an argument during the "should GPIO be a stable driver" discussion that GPIO is too low-level for that and if you're using GPIO directly then you should understand the risk of being tied to board implementations.

From a quick git grep gpio over libtock-c, I believe this would break only the board specific tests (i.e. the hail, imix, and nrf52840-dk test apps), the rf233-on-imix example, and the gpio test apps.

Given that we just did a 1.5 release, this actually seems like a very reasonable time to make a change like this.

[bradjc]

We made an argument during the "should GPIO be a stable driver" discussion that GPIO is too low-level for that and if you're using GPIO directly then you should understand the risk of being tied to board implementations.

From a quick git grep gpio over libtock-c, I believe this would break only the board specific tests (i.e. the hail, imix, and nrf52840-dk test apps), the rf233-on-imix example, and the gpio test apps.

Given that we just did a 1.5 release, this actually seems like a very reasonable time to make a change like this.

Well, of code that we wrote in libtock-c. Hail, for example, we made available and I don't think it is worth changing the GPIO interface from userland right now. I know I would be annoyed if someone changed it on me. Maybe 2.0 would be a time to change it, otherwise I see risks with minimal upside.

[alexandruradovici]

I can change back imix and hail so that we can merge this without any breaking changes. Once you decide that pin numbering may be changed for these ones, I submit a new PR.

[bradjc]

Quick doc update and not changing existing boards for me this is good to go.

[alexandruradovici]

I reverted the pins to their original values, there should be no breaking changes now.

[bradjc]

bors r+

[bors]

Build succeeded:

• ci-build (macos-latest)
• ci-build (ubuntu-latest)
• ci-format (macos-latest)
• ci-format (ubuntu-latest)
• ci-tests (macos-latest)
• ci-tests (ubuntu-latest)
• continuous-integration/travis-ci/push
• emulation-check