[RFC] Erase pin types from HAL driver types about stm32h7xx-hal HOT 30 CLOSED

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.

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

By consuming the pins: I2cPinsChecked in the constructor, they're still used up and can't be used elsewhere, just we don't even need to store them.

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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?

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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

from stm32h7xx-hal.

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

from stm32h7xx-hal.

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

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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.

from stm32h7xx-hal.

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).

from stm32h7xx-hal.

@ryan-summers That doesn't exactly make sense though, Option requires the struct to again have an associated PINS type, so in the case of not containing any pins it would be Option<((),())> because in the end Options have to contain a certain type right? So that doesn't exactly make sense, you'd need literally two different variants of the I2C struct in order to make this up, one I2CChecked and another I2CUnchecked which would be created by new or new_unchecked respectively with the I2CChecked having a free() -> (I2Cx, PINS) and I2CUnchecked having a free() -> I2Cx so we can still do stuff with the peripheral register singleton.

from stm32h7xx-hal.

@hargoniX I think the idea is we have an Option over a handful of type-erased pins, just for the purpose of retaining ownership. The HAL type therefore still wouldn't need any generic types. We wouldn't have a free() method at all.

from stm32h7xx-hal.

I'm not seeing the issue you're referring to. If we assume there's a generic Pin type for all pins owned by the HAL (since this RFC proposes erasing all specific pin types), I2C may look something like this:

struct I2cPins {
   scl: Pin,
   sda: Pin,
};

struct I2C<I2CX> {
   i2c: I2CX,
   pins: Option<I2cPins>,
}

// ... (Implementation details)
pub fn new(i2c: I2CX, pins: I2cPinsChecked) -> Self {
   I2C { i2c: i2c, pins: Some(pins.degrade()) }
}

pub fn new_unchecked(i2c: I2CX) -> Self {
   I2C {i2c: i2c, pins: None} 
}

 pub fn free(self) -> (I2CX, Option<I2cPins>) {
    (self.i2c, self.pins)
}

from stm32h7xx-hal.

But the free in this approach has no need to return the I2cPins anymore, they are just a Pin struct without any information, you can't do anything with them, it's pointless to want them back after the .degrade() call.

struct I2C<I2CX> {
   i2c: I2CX,
}

pub fn new(i2c: I2CX, pins: I2cPinsChecked) -> Self {
   I2C { i2c }
}

pub fn new_unchecked(i2c: I2CX) -> Self {
   I2C { i2c } 
}

pub fn free(self) -> I2Cx {
    self.i2c
}

this would already be enough.

from stm32h7xx-hal.

That would also be fine, although I believe the intent behind "owning" the pins is to ensure that they aren't used for anything else in applications where the pins are only used for the HAL implementation.

from stm32h7xx-hal.

new_unchecked doesn't ensure the "used for other stuff" point anyways in both approaches (in fact for Adam that's the precise use case). And we don't have to store the pins in our struct, once the HAL user gave us ownership of the pin they are effectively gone for him with no way to ever re-use them. Your approach would just store a Pin struct instead of PB11 for example, returning just a Pin struct in free() is useless, a Pin struct contains 0 information, it's just there.

With the approach of just taking ownership of I2CPinsChecked and dropping the struct afterwards we don't even need a fully downgraded pin struct.

from stm32h7xx-hal.

The only way to make the PINS part of our free() return type meaningful is to store the precise pin struct (e.g. PB11) inside our struct like we are doing it right now -> forcing us to keep the giant generic type. This is the trade off we gotta do really, either we have

1. Giant generic type but the result of free() is reusable
2. No giant generic type -> the result of free() is not reusable -> if people wanna reconfigure their stuff at runtime they will have to use new_unchecked in order to be allowed to keep their pin structs. This of course assumes the user knows what they are doing i.e. they are entering terrain in which we can't provide any other guarantee for them than "your stuff is in the exact state you desire it to be in, we can't tell you whether that state is a proper one though"

from stm32h7xx-hal.

Point taken - I have no issue dropping the pins from the structure, I just hadn't realized that the constructor would consume (take ownership) of the pin structure and we didn't need to store them. Dropping them entirely seems appropriate - I agree that they're pretty meaningless after a free()

from stm32h7xx-hal.

My preference is very much for @hargoniX 's implementation, as it avoids the runtime checks on Option and the complexity of that for a minority of use cases where new_unchecked will be just fine.

With our current implementation using trait Pins<I2C> {}, the compiler substitutes concrete types at compile time, and could indeed return then from within an Option at the end. But that misses the entire goal of erasing those concrete types.

from stm32h7xx-hal.

Alright, going along with @hargoniX's proposal (for I2C in this case), the new API would be:

pub trait I2cExt<I2C> {
    fn i2c<PINS>(self, pins: PINS) -> I2c<I2C>
    where 
        PINS: Pins<I2C>;

    fn i2c_unchecked(self) -> I2c<I2C>;
}

Any calls to directly instantiate would not require pins. E.g. the following is accepted:

// Instantiate clocks and device peripherals
stm32h7xx_hal::I2c::I2c(dp.I2C1, 100.khz(), &ccdr);

How does this sound?

from stm32h7xx-hal.

The last part where we have I2c twice looks rather weird to me though. I think if at all this method should be provided as i2c and even then there is potential to confuse it with i2c1, i2c2 and the likes.
As this API is aimed at users with rather special requirements for their hardware (i.e. runtime reconfiguration) I don't think the API does have to be so intuitively accessible either (and maybe shouldn't so novice users "intuitively" find the proper API).
Hence I believe we should expose

stm32h7xx_hal::I2c::i2c_unchecked(dp.I2C1, 100.khz(), &ccdr);

Instead, but maybe I'm just overthinking this, not sure.

from stm32h7xx-hal.

Are there more opinions on this or should someone just go ahead and implement it? The technical implementation seems straight forward ish to me.

from stm32h7xx-hal.

I'm happy for someone to go ahead and implement it as discussed above!

Once opened, I can review an in-progress PR if that is helpful

from stm32h7xx-hal.

Sorry for the radio silence on this, I've been pretty busy with life. I have an implementation that nearly complies with what we discussed here that I can get into a PR shortly.

from stm32h7xx-hal.