Skip to content

Conversation

Sh3Rm4n
Copy link
Member

@Sh3Rm4n Sh3Rm4n commented May 19, 2021

This is a MVP of using const generics, based on the refactoring of the GPIO module in #189

The current implementation leaves out the downgrade() method to get a generic pin type, which is a major loss.
This is because we can not be generic over Unsigned via min const generics.
min const generic does not allow any const generics in where clauses, and I'm not really sure, if this would be the right approach even if we had full const generics available.

Maybe there are some improvements to be made, which would allow not having to remove this method.

@Sh3Rm4n
Copy link
Member Author

Sh3Rm4n commented May 19, 2021

Hey @Piroro-hs, I would like to ask you for your help.

The initial motivation of the PR making use of const generics was the gnarly error message introduced by #189, like
this one https://github.com/stm32-rs/stm32f3xx-hal/runs/2623986440#step:4:131.

I stumbled upon these error messages, while implementing test cases in #212,
and I would rather not release a version until this issue is fixed.

I know that typeenum is a pretty powerful tool to be generic over integers, but I feel like the current error messages are a major loss, taking into consideration, that this HAL implementation is strongly typed partly because the error messages can give such a could hint about a mistake.

I don't know what to do though. Reverting #189 would be the easiest way, but your great improvement are worthwhile, and I would like to keep them.
I was thinking a lot about an elegant solution, taking the approach in this PR, but I have not come to a conclusion, maybe you have some insights.

The easiest way I was thinking about, is adding a new generic type PXx (which is distinct from the current generic Pin) type, which has no generic parameter about the pin number. This would result into many duplications though, as we have to implement many methods again for this pin (which would reintroduce some macro bulk which you have been getting rid of in your improvment PR)

Do you have any idea or suggestion @Piroro-hs ?

@Sh3Rm4n Sh3Rm4n added the help wanted Extra attention is needed label May 19, 2021
@Piroro-hs
Copy link
Contributor

Piroro-hs commented May 22, 2021

prototyping: https://github.com/Piroro-hs/stm32f3xx-hal/tree/untypenum

Main idea is that using this simple struct instead of typenum crate

pub struct U<const X: u8>;

Error message looks like this.

error[E0277]: the trait bound `stm32f3xx_hal::gpio::Pin<Gpiob, U<8_u8>, Alternate<OpenDrain, 4_u8>>: SdaPin<I2C1>` is not satisfied
  --> examples/i2c_scanner.rs:41:19
   |
41 |     let mut i2c = hal::i2c::I2c::new(
   |                   ^^^^^^^^^^^^^^^^^^ the trait `SdaPin<I2C1>` is not implemented for `stm32f3xx_hal::gpio::Pin<Gpiob, U<8_u8>, Alternate<OpenDrain, 4_u8>>`
   |
   = help: the following implementations were found:
             <stm32f3xx_hal::gpio::Pin<Gpioa, U<10_u8>, Alternate<OpenDrain, 4_u8>> as SdaPin<I2C2>>
             <stm32f3xx_hal::gpio::Pin<Gpioa, U<14_u8>, Alternate<OpenDrain, 4_u8>> as SdaPin<I2C1>>
             <stm32f3xx_hal::gpio::Pin<Gpiob, U<7_u8>, Alternate<OpenDrain, 4_u8>> as SdaPin<I2C1>>
             <stm32f3xx_hal::gpio::Pin<Gpiob, U<9_u8>, Alternate<OpenDrain, 4_u8>> as SdaPin<I2C1>>
             <stm32f3xx_hal::gpio::Pin<Gpiof, U<0_u8>, Alternate<OpenDrain, 4_u8>> as SdaPin<I2C2>>
   = note: required by `I2c::<I2C, (SCL, SDA)>::new`

@Sh3Rm4n
Copy link
Member Author

Sh3Rm4n commented May 25, 2021

Well this is a much simpler solution than mine 😀 And it solves the problem the most prominent problem of typeenum error types. typenum::Unsigned as a trait is still pretty useful, and I don't see, that it can be replaced with min-const-generics.

Let's go your route for now. Switching to const-generics would be a dream, but let's wait until that is more fleshed out.

~~What I still might suggest in incorporate 1ef350a with your (Piroro-hs@b34576b) change, so that we can switch to const-generics while the constraint still remains, that const generics have to be in the last position. ~~

EDIT:
Well should have gone through the whole diff before commenting.

So you were taking the other approach, not replacing all instances where Index is used to const generics, but change Index, so itself uses const generics (and U as well) I was briefly thinking about this approach as well. It seems, that it solves the problem more elegantly and is a simpler change.

This is a viable approch, I'm waiting for a PR 😋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants