Rework GPIO API and fix a few inconsistencies

[ithinuel]

This PR delivers an overhaul of the GPIO framework use in the rp2040-hal crate.
It brings in several aspects significant breaking changes (from extra type parameters to plain trait removal).
On the other hand, it resolves a number of long lasting issues and limitations and inconsistencies of the current APIs.

My Opinions

I don't really like how heavy the API is having all three type parameters to the Pin but I think this is a necessary evil as, unlike most platforms, the default state for pins is not floating input but some sort of inactive pull down (except for some pins in the qspi bank).

While very nice for how it appears in documentation and code around the function signature, the AnyPin (and related type-level magic) requires some awkward gymnastics at times.

Fixes

#140, #446, #481, #485, #556 (#557)

[thejpster]

I started reviewing this on my phone, but I'm going to need a real computer!

In general this looks great and thank you for doing all this work. I'm excited to see it land.

I have some minor nits around the ordering of types and impl blocks, and missing module-level docs to explain what each file is for to the casual observer (including the pub-crate modules for sealed traits). I may try and add these myself instead of leaving comments, but not right now.

[thejpster]

The ADC provides a u16, so does it make sense to be generic over any type WORD, where WORD can be created from a u16? e.g. if someone asked for a u32 from the read function, they'd get it, but it would only be scaled 0..65535 not 0..2^32. I think?

[ithinuel]

🤷 That's how it was when I found it 🤷 I don't have opinions about it.
An ADC output may be 12 or 14bit and return the result on a u16 as 0..(2^12) and as far as I can remember it's not usually scaled up to a u16 so I guess one should not expect to get is scaled to a u32 even if that's the type they expect.

[thejpster]

pub enums should have comments on each variant, perhaps with a link to the datasheet?

[ithinuel]

I couldn't find something to write on each that would add anything valuable.

This seems pretty redundant:

pub enum DynFunction {
    /// Value-level `variant` for the Xip [`Function`].
    Xip,
    /// Value-level `variant` for the Uart [`Function`].
    Uart,
    // […]
}

[thejpster]

Well perhaps we could write them in the case that people expect and expand the acronyms?

/// Describes the function currently assigned to a pin with a dynamic type.
///
/// A 'pin' on the RP2040 can be connected to different parts of the chip
/// internally - for example, it could be configured as a GPIO pin and connected
/// to the SIO block, or it could be configured as a UART pin and connected to
/// the UART block.
pub enum DynFunction {
    /// The 'XIP' (or Execute-in-place) function, which means talking to the QSPI Flash
    Xip,
    /// The 'UART' (or 'serial-port') function.
    Uart
}

[thejpster]

Sorry more scattered drive-by comments as they come to me.

[jannic]

We should also have a look at the generated code.
As a (very) quick check, I ran cargo bloat --release --example blinky on this branch and on the base commit (22121b5078ffe75d5f2c3b49cadc067dd5d9ecda):

This branch:

File  .text   Size      Crate Name
0.1%  12.4%   886B        std compiler_builtins::int::specialized_div_rem::u64_div_rem
0.1%  11.6%   828B        std core::str::count::do_count_chars
0.1%  10.2%   732B rp2040_hal rp2040_hal::clocks::ClocksManager::init_default
0.1%   9.5%   682B        std core::fmt::Formatter::pad_integral
0.1%   9.2%   656B        std core::fmt::Formatter::pad
0.1%   7.0%   500B     blinky blinky::__cortex_m_rt_main
0.0%   5.9%   420B rp2040_hal rp2040_hal::pll::setup_pll_blocking
0.0%   5.9%   420B rp2040_hal rp2040_hal::pll::setup_pll_blocking
0.0%   5.4%   388B rp2040_hal rp2040_hal::gpio::pin::set_function
0.0%   4.8%   344B rp2040_hal rp2040_hal::clocks::init_clocks_and_plls
0.0%   3.5%   248B        std core::fmt::num::imp::<impl core::fmt::Display for u32>::fmt
0.0%   2.9%   208B   cortex_m cortex_m::delay::Delay::delay_us
0.0%   1.5%   104B rp2040_hal rp2040_hal::gpio::bank0::Pins::new
0.0%   1.1%    78B  [Unknown] __aeabi_lmul
0.0%   0.9%    62B        std core::fmt::Formatter::pad_integral::write_prefix
0.0%   0.8%    60B        std core::panicking::panic_bounds_check
0.0%   0.8%    60B        std core::panicking::unreachable_display
0.0%   0.6%    44B  [Unknown] __aeabi_llsl
0.0%   0.6%    44B        std core::panicking::panic
0.0%   0.6%    40B        std core::panicking::panic_fmt
0.0%   3.0%   218B            And 23 smaller methods. Use -n N to show more.
0.8% 100.0% 7.0KiB            .text section size, the file size is 883.2KiB

Base commit:

File  .text   Size       Crate Name
0.1%  22.2%   886B         std compiler_builtins::int::specialized_div_rem::u64_div_rem
0.1%  18.3%   732B  rp2040_hal rp2040_hal::clocks::ClocksManager::init_default
0.0%  10.5%   420B  rp2040_hal rp2040_hal::pll::setup_pll_blocking
0.0%  10.5%   420B  rp2040_hal rp2040_hal::pll::setup_pll_blocking
0.0%   9.6%   384B      blinky blinky::__cortex_m_rt_main
0.0%   8.6%   344B  rp2040_hal rp2040_hal::clocks::init_clocks_and_plls
0.0%   5.2%   208B    cortex_m cortex_m::delay::Delay::delay_us
0.0%   2.2%    88B  rp2040_hal rp2040_hal::gpio::pin::bank0::Pins::new
0.0%   2.0%    78B   [Unknown] __aeabi_lmul
0.0%   1.1%    44B   [Unknown] __aeabi_llsl
0.0%   1.1%    44B         std core::panicking::panic
0.0%   1.0%    40B         std core::panicking::panic_fmt
0.0%   1.0%    40B cortex_m_rt Reset
0.0%   0.6%    22B   [Unknown] __aeabi_uldivmod
0.0%   0.5%    20B   [Unknown] HardFaultTrampoline
0.0%   0.5%    18B         std __udivmoddi4
0.0%   0.4%    16B         std <T as core::any::Any>::type_id
0.0%   0.3%    10B    cortex_m __delay
0.0%   0.3%    10B   [Unknown] main
0.0%   0.2%     6B    cortex_m __primask_r
0.0%   0.8%    30B             And 11 smaller methods. Use -n N to show more.
0.4% 100.0% 3.9KiB             .text section size, the file size is 877.4KiB

Most of the size change seems to come from some additional formatting code. Probably some new panic somewhere, which pulls in a lot of stuff from core. While not perfect, I don't think this is a big issue, as it won't significantly affect a larger binary. (If at all, because with some probability a larger binary would contain that formatting code anyway.)

But I think the change in size of blinky::__cortex_m_rt_main from 384B to 500B is a little bit worrying. When I find some time I'll have a look at the generated code to find out what changed there.

[ithinuel]

There was a few missed optimisation oportunities that a few inline fixed :)
That being said, the rp-hal workspace does not have the common recommended profile.

I get the best results with

[profile.release]
codegen-units = 1
debug = 2
debug-assertions = false
incremental = false
lto = 'fat'
opt-level = "s"
overflow-checks = false

and cargo bloat --release --example blinky --features 'cortex-m/inline-asm'

I get the exact same

File  .text   Size       Crate Name
0.3%  60.7%   872B      blinky blinky::__cortex_m_rt_main
0.0%  10.3%   148B    cortex_m cortex_m::delay::Delay::delay_us
0.0%   7.5%   108B  rp2040_hal rp2040_hal::clocks::fractional_div
0.0%   5.4%    78B   [Unknown] __aeabi_lmul
0.0%   2.8%    40B cortex_m_rt Reset
0.0%   1.4%    20B   [Unknown] HardFaultTrampoline
0.0%   0.7%    10B         std core::panicking::panic
0.0%   0.7%    10B         std core::panicking::panic_fmt
0.0%   0.7%    10B   [Unknown] main
0.0%   0.1%     2B cortex_m_rt HardFault_
0.0%   0.1%     2B  panic_halt rust_begin_unwind
0.0%   0.1%     2B cortex_m_rt DefaultPreInit
0.0%   0.1%     2B cortex_m_rt DefaultHandler_
0.0%   0.0%     0B             And 0 smaller methods. Use -n N to show more.
0.5% 100.0% 1.4KiB             .text section size, the file size is 297.6KiB

on both main and this branch on commit ea49143b899df4ee66ac71d7b50d23f26c8ed073

[jannic]

I stumbled upon this change while porting Neotron code to the new API.
It was not immediately obvious what into() needs to be replaced by. Perhaps there should be some migration guide?

At first, I thought that this is a bad change, as .into() is a common pattern and easily discoverable. But then, I wondered if it was the correct abstraction in the first place. .into() is for type conversion, and usually doesn't have any side effects. Is it an anti-pattern to use .into() like this? If yes, then we should also avoid names like .into_typestate(), and prefer something like .set_typestate().

If HAL users have to fix all calls to .into() anyway, and we already lose the nicely discoverable name, we could as well switch to some "more correct" name. That leaves the .into_...-pattern for pure type conversions without side effects.

[ithinuel]

into more than being a pattern is a full fledged (generic) trait Into<T>.
This method was conflicting with that trait so it had to be renamed.

I'm not convinced about the name myself. I picked it to remain in line with the other .into_<some_type_state>() which are common amongst HAL's GPIO APIs.

[jannic]

https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv lists as_, into_ and to_ as common prefixes for type conversion functions. There's no explicit rule that forbids to use those names for something else.

What do other HALs do?

• esp-idf-hal works differently: https://github.com/esp-rs/esp-idf-hal/blob/master/examples/button.rs#L18-L19
• stm32f1xx-hal uses the into_ prefix: https://github.com/stm32-rs/stm32f1xx-hal/blob/master/examples/blinky.rs#L40
• embassy works more like esp-idf-hal: https://github.com/embassy-rs/embassy/blob/master/examples/rp/src/bin/blinky.rs#LL15C1-L15C53
• nrf-hal uses the into_-prefix, https://github.com/nrf-rs/nrf-hal/blob/master/examples/blinky-button-demo/src/main.rs#L22-L23

So using into_ is widespread, but not universal.

[ithinuel]

According to the API guidelines, the prefix should probably be to_:

Conversions prefixed to_, on the other hand, typically stay at the same level of abstraction but do some work to change from one representation to another.

In our case it definitely stays the same level of abstraction. It's the same generic type with different parameters.

[jannic]

The three prefixes are meant for functions which do pure type conversions (which might still be expensive, for example when converting strings), but don't perform other actions.

But the GPIO methods also configure the hardware. And that's not just some detail, I'd argue it's the main point of the call. That it also returns a different data type is nice, because it allows to detect some errors at compile time. But it's not the reason the method is called in the first place.

Therefore, IMHO, the functions should have names that don't start with one of these prefixes.

BTW, .into_typestate() (and .set_typestate(), as I suggested above) don't reflect the intention of the user at all, regardless of the prefix. The actual function is more like .reconfigure_by_typestate() - but that's quite cumbersome.
What about .reconfigure(), .reconfigure_pull_type(), .reconfigure_function()?

Notably, .into_dyn_pin() is fine, or could be replaced by .as_dyn_pin(), as it really only changes the type, but doesn't change the pin itself.

Of course, we might still decide to keep the into_ prefix for consistency with several other HALs. Especially as many of the functions, like .into_pull_up_input() are quite good at discoverability and readability. But I really worry about .into_typestate(). It's both difficult to find if you do not yet know the name of the function, and difficult to understand if you read the code.

[ithinuel]

I wouldn't consider the pin's state separate from the type.
That's why we type state it and all the effort to guaranty the unicity of the pin IDs.

So we are effectively converting (with some reconfiguration cost) a pin from one state to another. This matches the type conversion of into_ for owned to owned (non-copy) types.

[ithinuel]

Leaving this here while I'm reading them:
https://willcrichton.net/rust-api-type-patterns/state_machines.html
https://docs.rust-embedded.org/book/static-guarantees/state-machines.html

[jannic]

While there should indeed be a 1:1 mapping between the type and the state of the pin (after all it's called typestate), I'm not convinced that having type conversion functions with implicit side effects is a good choice.
They may be equivalent in a mathematical sense, that doesn't make it right to add side effects into type conversion functions. But that may be a red herring. Let me try to avoid arguing what's "right", and instead concentrate on what's easy to learn:

Just from observing myself while I fixed this compile error:

error[E0277]: the trait bound `rp_pico::rp2040_hal::gpio::Pin<Gpio27, FunctionPio1, PullNone>: From<rp_pico::rp2040_hal::gpio::Pin<Gpio27, FunctionNull, PullDown>>` is not satisfied
   --> src/main.rs:712:37
    |
712 |                     i2s_bit_clock: hal_pins.gpio27.into(),
    |                                                    ^^^^ the trait `From<rp_pico::rp2040_hal::gpio::Pin<Gpio27, FunctionNull, PullDown>>` is not implemented for `rp_pico::rp2040_hal::gpio::Pin<Gpio27, FunctionPio1, PullNone>`
    |
    = note: required for `rp_pico::rp2040_hal::gpio::Pin<Gpio27, FunctionNull, PullDown>` to implement `Into<rp_pico::rp2040_hal::gpio::Pin<Gpio27, FunctionPio1, PullNone>>`

It's quite obvious that rust doesn't know how convert hal_pins.gpio27 into the correct type automatically, as the From trait (or the Into trait) are not implemented. So far so good. I also see from the types that I need to configure the pin function and turn off pull-down. But at that point I don't know what function to call to do that. And it really took me a while to find out that .into_typestate() is the right call. In fact, I only found it by looking at the recent changes you did on rp2040-hal. But that was only possible because I knew that the same code did work a week ago, so it must have been a recent change. How could somebody new to embedded rust find that function?

And I'd argue that a vast majority of software developers don't think about type states when they configure a GPIO pin. Typestates are a tool to improve safety, but not a goal in itself. They work best if you don't have to think about them at all. Therefore, when looking for a way to configure a pin, it's difficult to find a function named into_typestate().

Again, I'm much less worried about names like .into_pull_up_input(). They may not match the pure idea of a type conversion function. But they are obvious. Easy to find, easy to understand. Seeing let button = gpio17.into_pull_up_input(), I immediately understand what happens. Compare that to let button = gpio17.into_typestate(). What does that even mean? And then, consider let button = gpio17.reconfigure(). That's readable again. There's some magic, because the kind of reconfiguration is implicit, but the idea of what should happen is clear. Add a doc string to explain where the target mode comes from:

    /// Configure the pin to match the new state given by the type parameters F2, P2.
    pub fn reconfigure<F2, P2>(self) -> Pin<I, F2, P2>
    where
        F2: func::Function,
        P2: PullType,
        I: func::ValidFunction<F2>,

Note that for other functions, you also chose the wording "Configure the pin ..." and not "Convert the pin ...":

    /// Configure the pin to operate as a floating input
    #[inline]
    pub fn into_floating_input(self) -> Pin<I, FunctionSio<SioInput>, PullNone>

BTW, I'm not claiming that reconfigure is the best function name, it's just what came to my mind immediately.

[ithinuel]

I understand your point now. I think marking into as deprecated and using configure<F, P>() (or configure_as::<F, p>() to hint there's more to the function than using the current state to configure the pin.) would work well with the doc:

    /// Configure the pin to match the new state given by the type parameters F2, P2.
    pub fn reconfigure<F2, P2>(self) -> Pin<I, F2, P2>
    where
        F2: func::Function,
        P2: PullType,
        I: func::ValidFunction<F2>

EDIT: removed previous obsolete message

[thejpster]

I just wanted to jump in and say: I enjoyed reading the discussion above. Congratulations to all for Being Nice On The Internet :)

[jannic]

As mentioned on matrix: I didn't do a full review, but I read through most changes (multiple times), ported some code to the changed API, and didn't see any obvious issues. AFAIK others did the same. So in sum, this change should be sufficiently reviewed to be merged.