[RFC] Erase pin types from HAL driver types

[ryan-summers]

When implementing an application using RTFM and storing peripheral resources in the Resource structure, the type of the peripheral can be quite onerous to carry around as it contains generic parameters for the pins and peripheral instance.

Example:

let spi = hal::spi::Spi1::new(dp.SP2, (sck, miso, mosi), ...);

The spi variable has the following type:

hal::spi::Spi<
    hal::stm32::SPI2, (
        hal::gpio::gpiob::PB10<hal::gpio::Alternate<hal::gpio::AF5>>, 
        hal::gpio::gpiob::PB14<hal::gpio::Alternate<hal::gpio::AF5>>,
        hal::spi::NoMosi)
>

This type then changes based on which pins are used. This type is also required to be specified in the RTFM resource structure declaration, which results in a (fairly messy) structure definition that's non-intuitive.

Because the pin types are used only as a compile-time guarantee that they implement the necessary SPI alternate functions, I don't believe there's a need for the SPI module to carry around this type information after instantiation (although the SPI peripheral needs to maintain ownership of the pins).

I propose that we update the SPI constructor to downgrade the pins to a generic "pin" type since they are never directly controlled by the SPI peripheral. This would remove pin types from the type signature and simplify it to something like hal::spi::Spi<hal::stm32::SPI2>, which is much more manageable (and we could potentially remove the SPI2 type as well with other work, although that's beyond scope here).

[richardeoin]

I throughly support that, the current types are quite unwieldy and I see no real use case for them.

Honestly this seems like an oversight in the design of the current spi implemenation (and i2c, uart, qei..). The current pwm implementation does erase the pin types already.

[hargoniX]

I support the idea as well, when wanting to iterate over a set of Pins from different GPIO blocks you gotta fall back to arrays of &dyn mut OutputPin, this could be a lot prettier if we just had a basic type we could downgrade our pins into.

The only use case is as richard mentioned for the static verification in our peripheral implementations as it's used for lot's of traits there, I believe that this is a good reason to keep them as well as it stops people that don't know everything about this chip from the top of their head from for example using the wrong I2C peripheral, i.e. IC21 while their pins could only be used with I2C2.

[hargoniX]

Thinking about it, we could keep our static verifications if, e.g. the I2C struct would take the structs with all their generics etc, then downgrade them internally and store downgraded Pin structs instead of the pins structs with generics it currently gets, this would allow us to keep the API fully consistent + keep our verification + the resulting type would be I2c without any of the pins as generics.

I see only one issue with this approach, the free method which is supposed to return you your precious pins so you can reconfigure them again later on. As this free method would then return only our downgraded pins we would have no chance of using our current GPIO machinery to configure them further I think (?). If we had a solution for that we could keep static verification and make our types smaller so everyone is happy \o/

Anyone got an idea for the free() problem?

Edit: Do we actually need the free() method? It's only purpose would be to reclaim pins from a Peripheral in order to reuse it for other stuff, is that actually a thing to use the same pins for different protocols? Do people do that?

[ryan-summers]

There are some (uncommon) use cases for freeing pins, such as if the output pins flow into a MUX to various on-board devices, but I've never seen it in my embedded work and it would introduce a lot of overhead into the application at runtime (e.g. would have to reinstantiate peripherals whenever you free them).

Indeed, the current thought is to keep the pin trait implementations for the new() functions, but downgrade them internally so the type returned by new() doesn't contain the pin types itself (e.g. still get the compile-time pin checks).

I was also thinking about the free() issue - we could potentially store run-time metadata in the generic Pins structure about it's state and re-cast the pin into the correct time upon free(), although that's not the most elegant solution. I would personally try to figure out if the free() api is something we need.

[richardeoin]

On balance, I'm happy to loose the free() functionality here in return for the simplified types.

[adamgreig]

This kicked off a huge off-topic rant from me about type state programming on the Matrix (sorry!!) but I think did help uncover some interesting use cases for this PR, including around free():

• If this RFC went ahead, such that HAL types erased pin states after construction, it would be possible to add a new alternative constructor which didn't take pins at all, and only set up the HAL peripheral itself, without touching GPIO
• Users would be responsible for ensuring at least some pins were in the relevant AF state for use with the peripheral (perhaps the new constructor is marked 'unsafe' to drive this point home)
• Such a constructor would allow users to hold on to the pins for doing runtime reconfiguration, replacing the use case for free()
• Additionally this allows more niche uses of the peripheral, such as using a UART with only TX by not setting any pins to the RX AF

[nickray]

For the original issue, why not just use type defs/aliases? and perhaps a board support crate :)

[ryan-summers]

This kicked off a huge off-topic rant from me about type state programming on the Matrix (sorry!!) but I think did help uncover some interesting use cases for this PR, including around free():

• If this RFC went ahead, such that HAL types erased pin states after construction, it would be possible to add a new alternative constructor which didn't take pins at all, and only set up the HAL peripheral itself, without touching GPIO
• Users would be responsible for ensuring at least some pins were in the relevant AF state for use with the peripheral (perhaps the new constructor is marked 'unsafe' to drive this point home)
• Such a constructor would allow users to hold on to the pins for doing runtime reconfiguration, replacing the use case for free()
• Additionally this allows more niche uses of the peripheral, such as using a UART with only TX by not setting any pins to the RX AF

Using the peripheral without guarantees on pin ownership would definitely be an API that we would want to mark unsafe. We have no concept as to whether or not we could actually perform requested transactions..

For the original issue, why not just use type defs/aliases? and perhaps a board support crate :)

I do currently use typedef aliases, it's just pretty hefty to have to carry around that type information when it's not necessary and (in my opinion) obfuscates the code. Additionally, it duplicates the pin definitions, since the pin type is defined when instantiating the pin, but it's now also defined in the RTFM resources structure. This means there's two places to update the pin if it ever changes, which doesn't really provide benefit.

My main concern is that the current implementation is not intuitive. I had to look at some examples of other RTFM applications to figure out just what the type signature should look like.

[adamgreig]

Using the peripheral without guarantees on pin ownership would definitely be an API that we would want to mark unsafe. We have no concept as to whether or not we could actually perform requested transactions..

But it's not actually 'unsafe', right? There's no memory safety or UB implication, just the SPI wouldn't work if you didn't configure some pins, the same way it wouldn't work if you safely gave it the wrong pins for your application. I see it more as the user choosing to opt-out of some helpful static pin assignment checks. Not a hill I'm willing to die on at all, I'd much much rather have such a constructor but it be unsafe than not have it.

[ryan-summers]

I believe a HAL implementation without pin ownership would be inherently unsafe because there is no way to guarantee the input/output of data. Take the following hypothetical scenario:

• There is a SPI peripheral that talks to some external memory
• The pins used for the SPI peripheral are muxed to some LEDs
• There are two tasks running asynchronously with different priorities:
  • 1. LED toggles (high priority) - The pins are switched to GPIO outputs to signal some LED indications momentarily
  • 2. Memory read (low priority) - The pins are switched to SPI to read memory from an external EEPROM

If the first task (LED toggle) interrupts the second task (EEPROM memory read), the results of the memory read may become invalid because the SPI pins were configured differently for part of the transaction.

[adamgreig]

I see your point, but I think you can't guarantee the input/output of data even with pin ownership, e.g. if the user provides pins that are not physically connected to an EEPROM. More to the point, the SPI HAL only returns [u8] anyway; it doesn't memory map the contents or cast to types with invalid representations, so you might receive the 'wrong' data but it's still valid data; it hasn't actually caused UB.

The story might be more complicated with a memory mapped peripheral like F[S]MC or QSPI where changing pins at runtime really could screw with memory in the same way DMA can, so I guess the HAL for those peripherals would have to take extra precautions. I think the edge cases that require runtime reconfiguration are less likely for such peripherals, at least.

[jordens]

That's invalid, dangerous and data corrupting. But not unsafe in the rust sense. The safe/unsafe notion stops at the processor core/memory/cache. I would not extend it to configurations and usage patterns of peripherals and external hardware.
How do you know that setting a gpio high leads to well defined behavior? It's certainly a safe API, but it may well power down some chip causing all kinds of problems including undefined behavior.

[ryan-summers]

That's invalid, dangerous and data corrupting. But not unsafe in the rust sense. The safe/unsafe notion stops at the processor core/memory/cache. I would not extend it to configurations and usage patterns of peripherals and external hardware.
How do you know that setting a gpio high leads to well defined behavior? It's certainly a safe API, but it may well power down some chip causing all kinds of problems including undefined behavior.

I see your point, but I think you can't guarantee the input/output of data even with pin ownership, e.g. if the user provides pins that are not physically connected to an EEPROM. More to the point, the SPI HAL only returns [u8] anyway; it doesn't memory map the contents or cast to types with invalid representations, so you might receive the 'wrong' data but it's still valid data; it hasn't actually caused UB.

The story might be more complicated with a memory mapped peripheral like F[S]MC or QSPI where changing pins at runtime really could screw with memory in the same way DMA can, so I guess the HAL for those peripherals would have to take extra precautions. I think the edge cases that require runtime reconfiguration are less likely for such peripherals, at least.

Agreed that I don't believe this could result in unsafe behavior from a rust standpoint (with the exception of memory-mapped peripherals, as previously mentioned).

I am not opposed to removing the pins from the HAL implementations (although this may make the HAL slightly less user-friendly).

[adamgreig]

I am not opposed to removing the pins from the HAL implementations (although this may make the HAL slightly less user-friendly).

I think a lot of people like having the pins in the constructor enough to make it worth keeping; my suggestion here is that with this RFC it would additionally be possible to make an alternative constructor (new_unchecked?) which could be used to bypass those static checks without losing any other functionality. I hope that would have a minimal impact on user friendliness for users who benefit from the checks, while making more niche uses cases possible with the HAL.

[ryan-summers]

Ah, I understand you now and I think that's appropriate. We could then update the driver to contain an Option<PINS> where it retains ownership if they were provided in the constructor.

That's a nice solution to the lack of ability to free() the pins if we degrade the type for the users that need the pins for multiple use cases.