Unable to use peripherals with generics

[petersonev]

Most of the implementations for peripherals are created using macros which results in a number of specialized impls that don't indicate they have shared functionality. Because there are no traits for these impls the functions defined inside cannot be used with generics. By just adding the impls inside the macros to a trait instead it would allow for peripherals to be used with generics.

For example right now you can't do something like this because all of the impls are specialized:

fn serial_stuff<USART>(serial: serial::Serial<USART>) {
    serial.configure(...);
    ...
}

However if the following:

macro_rules! usart {
    ...
    impl Serial<$USARTX> {
        ...

was given a trait like:

macro_rules! usart {
    ...
    impl SerialTrait for Serial<$USARTX> {
        ...

it would be very convenient to use with generics using a where serial::Serial<USART>: SerialTrait

Not sure how common of a use case this is but at least for my use case it would be very helpful as it is communicating very similar data between multiples of the same peripheral on different buses.

I'd be happy to add these traits to the relevant modules if this is a good/useful solution.

[mlamoore]

The embedded-hal crate contains traits that are meant to provide a generic interface to many different microcontrollers. Libraries, drivers for external devices, and applications should be able to generically code behavior against these embedded-hal traits and have that code run on any device. These traits only specify how peripherals are used, not how they are configured, since configuration varies so much from device to device. Here's the embedded-hal serial module that contains two traits, Read and Write, for the two halves of a serial interface. https://docs.rs/embedded-hal/0.2.7/embedded_hal/serial/index.html In stm32h7xx-hal, Serial implements Read<u8> and Write<u8>, while the split serial Rx and Tx structs each implement the corresponding trait. https://docs.rs/stm32h7xx-hal/0.12.1/stm32h7xx_hal/serial/struct.Serial.html#impl-Read%3Cu8%3E If you can generically do what you want in terms of the embedded-hal traits, that's generally best, but if you think there's missing functionality that should be covered by a trait, we could potentially have our own behavioral traits, or we could try to design new traits that could be added to embedded-hal (which requires something that can be implemented by many different microcontrollers, not directly exposing chip specific features).

…

On Tue, May 3, 2022 at 5:21 AM Evan Peterson ***@***.***> wrote: Most of the implementations for peripherals are created using macros which results in a number of specialized impls that don't indicate they have shared functionality. Because there are no traits for these impls the functions defined inside cannot be used with generics. By just adding the impls inside the macros to a trait instead it would allow for peripherals to be used with generics. For example right now you can't do something like this because all of the impls are specialized: fn serial_stuff<USART>(serial: serial::Serial<USART>) { serial.configure(...); ... } However if the following: macro_rules! usart { ... impl Serial<$USARTX> { ... was given a trait like: macro_rules! usart { ... impl SerialTrait for Serial<$USARTX> { ... it would be very convenient to use with generics using a where serial::Serial<USART>: SerialTrait Not sure how common of a use case this is but at least for my use case it would be very helpful as it is communicating very similar data between multiples of the same peripheral on different buses. I'd be happy to add these traits to the relevant modules if this is a good/useful solution. — Reply to this email directly, view it on GitHub <#362>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUS5FP56WZJVB4B27YHZJDVID4ZLANCNFSM5U6PI4NA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[petersonev]

Yeah, and where possible using the Read and Write traits makes sense. At least for my use case though it's communicating with multiples of the same device and setting up DMA for all of those which would be a lot of repeated configuration and uses multiple functions that are not part of any embedded-hal trait.

I think ideally the peripherals would have been set up similar to the stm32f4-hal crate where instead of having a macro for the impl of Serial type, the macro is on the impl for an Instance trait which is implemented for each of the pac::USART types. Then the Serial impl just has a where USART: Instance. This allows having the Serial implementation outside of a macro (which is always nice), reusable code (even if it is only within that STM32 device type), and nicer generated docs (the Serial/Tx/Rx struct docs don't have 8x copies of every function because a macro made a copy for each USART). (Also this is applicable for most of the peripherals, I'm just using Serial as an example)

I could try and see if it's possible to change it similar to how the stm32f4-hal does it without making a breaking change (cause this is definitely doesn't seem important enough to cause a breaking change), but just adding a trait inside of the existing macros would at least solve not being able to use non embedded-hal functions within generics.

I know the one of the main points of this library is to implement the embedded_hal functions and that's very useful for a lot of things, but this library is used to write code for the STM32H7 so even if there are traits only useful for the H7 it seems useful to allow for as much generic/reusable code as possible.

[mlamoore]

That makes a lot of sense to me; then we would have both the embedded-hal traits that are portable and our own traits that are specific to the configuration of the H7 chips that allow for easier repeated configuration. I agree that this should be possible as a non-breaking change, except I think you might have to have the new traits in the prelude or in a use statement to make them usable. Does anyone else have any opinions on whether that's OK?

…

On Fri, May 6, 2022, 10:34 AM Evan Peterson ***@***.***> wrote: Yeah, and where possible using the Read and Write traits makes sense. At least for my use case though it's communicating with multiples of the same device and setting up DMA for all of those which would be a lot of repeated configuration and uses multiple functions that are not part of any embedded-hal trait. I think ideally the peripherals would have been set up similar to the stm32f4-hal crate where instead of having a macro for the impl of Serial type, the macro is on the impl for an Instance trait which is implemented for each of the pac::USART types. Then the Serial impl just has a where USART: Instance. This allows having the Serial implementation outside of a macro (which is always nice), reusable code (even if it is only within that STM32 device type), and nicer generated docs (the Serial/Tx/Rx struct docs don't have 8x copies of every function because a macro made a copy for each USART). (Also this is applicable for most of the peripherals, I'm just using Serial as an example) I could try and see if it's possible to change it similar to how the stm32f4-hal does it without making a breaking change (cause this is definitely doesn't seem important enough to cause a breaking change), but just adding a trait inside of the existing macros would at least solve not being able to use non embedded-hal functions within generics. I know the one of the main points of this library is to implement the embedded_hal functions and that's very useful for a lot of things, but this library is used to write code for the STM32H7 so even if there are traits only useful for the H7 it seems useful to allow for as much generic/reusable code as possible. — Reply to this email directly, view it on GitHub <#362 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUS5FPSTHMMZNCE4UZTXXLVIU3YFANCNFSM5U6PI4NA> . You are receiving this because you commented.Message ID: ***@***.***>

[richardeoin]

For simple use cases the embedded-hal traits are preferred, but indeed there's a use case for having traits that cover more functionality as @petersonev suggests. The spi module already has this after #260 and #282, and I've not seen any issues.

My comments would be

• Favour simplicity over including all functionality in the trait (although in most cases I hope possible to include all functionality in the trait)
• Ideally backwards-compatible and similar to other Rust HALs (such as stm32f4xx-hal - already mentioned)

[petersonev]

Created a branch to try this out with serial here.

It moves all implementations outside of the macro except for SerialExt (which is still needed to limit the generics to USART and set the ptr location). In this case SerialExt is acting the same as Instance in other stm32 hal implementations like for the f4 or f7 (I think ideally the name for SerialExt would be changed to `Instance as well if that's an okay breaking change)

Due to moving outside of generics a couple of breaking changes were made:

• $usartX functions were changed to new (this is the same as other hals, these could be added back to macro as deprecated then just have them call the new function)
• The reconfigure function always has a synchronous parameter instead of only existing for USART then this parameter is ignored for UART (similar to how fields in config are ignored and I would argue this is better practice anyway, however due to lack of function overloading wouldn't be able to deprecate the UART reconfigure)

These changes allow for generics like:

pub struct Device<USART> {
    serial: serial::Serial<USART>,
}

impl<USART: serial::SerialExt> Device<USART> {
    pub fn new<PINS: serial::Pins<USART>>(
        serial: USART,
        pins: PINS,
        prec: USART::Rec,
        clocks: &rcc::CoreClocks,
    ) -> Device<USART> {
    // Configure usart, call funcs including those outside of embedded-hal
    ...
}

It also cleans up the docs a lot so the structs don't show 8x of every function

If desired I could open a PR for this with just serial or I could try to make this same change for the other applicable peripherals.