stm32/usart: Remove BasicInstance generic

[honzasp]

In Embassy, all driver structs (such as UartRx or I2c) are generic in the underlying peripheral. This has some disadvantages:

1. All higher-level drivers that wrap an underlying peripheral (for example, a driver for a gyroscope chip that wraps an I2c driver) must be generic in T: BasicInstance, which leads to a lot of clutter.
2. Code for the drivers and all higher-level code that uses them is duplicated for every instance of the peripheral. For example, if my code needs to work with two gyroscopes on two different I2C buses, all the code in the I2C driver and in the gyroscope driver will be duplicated.
3. It's not possible to specify the peripheral at runtime, the exact usage of all pins and peripherals must be hard-coded. (This PR does not fully address this use case, the driver struct still needs to be initialized with statically-known peripheral and pins.)

This is an experimental PR that removes the T: BasicInstance generic from the USART drivers. I'm well aware that this is a large change in how embassy-stm32 is structured, so I started with just a single driver to gather feedback whether this is the right approach.

The implementation in this PR is directly inspired by #980. The UART driver stores &'static Vtable, where Vtable is a struct that contains all information that was previously accessed using the T type argument, such as a pointer to the register block (Regs) or a function to disable the peripheral in the RCC (T::disable()).

I tested this PR on my Nucleo H743ZI2 board and it works with the code in my project. I'm not sure how to test the changes better, I'd welcome any suggestions on how Embassy code should be tested.

Finally, thanks for all the work that went into Embassy! :-)

[honzasp]

Huh, I just found #2835, so maybe @Dirbaio is already working on this?

[honzasp]

I renamed "info" to more descriptive "vtable" and rebased to main

[Dirbaio]

Hey! thanks for the PR and sorry for the late reply, work on this from my side has stalled a bit becuase other priorities. I do agree we really should remove the instance generics from all drivers, it'll make the API much easier to use and help code size.

I'd prefer to avoid function pointers, because they hurt perf a bit (both due to the indirect jump and due to preventing compiler optimizations because no inlining), and because they're not very friendly to stack usage analysis tools.

so ideally we need find "not-function-pointer" replacements of everything in the vtable (I called the vtable "info" in #2835. vtable only makes sense if it's function pointers). These are:

• clock frequency: can call it on creation, store the frequency in self. We currently don't support dynamically changing clock freqs, so that's okay.
• Interrupt enable/disable/pend: these can be done by storing the interrupt number, as a value of the Interrupt enum.
• RCC enable/reset bit: this is the tricky one, see below:

In #2835 I tried to solve that by adding a ClockEnableBit struct that stores register offset + bit offset https://github.com/embassy-rs/embassy/pull/2835/files#diff-7b62f295d24627a593c57a6d16536ba394b84472a96646597f61488646babdefR144 . With that it's possible to store the bit in info and disable it on drop, without needing function pointers.

The problem is currently some RCC bits are "reference counted". The refcount logic is generated by build.rs, according to this logic:

• If multiple peripherals share the same RCC bit, it gets a refcount. I think it's just ADC on some chips that hits this.
• USART always gets a refcount. This is needed for split: new increases the refcount by 2, each drop of tx/rx decreases it by 1 so that the 1st drop doesn't disable the USART, only the 2nd drop does.

This refcounting is not compatible with the idea of ClockEnableBit as it is now. Possible solutions:

• Give up on ClockEnableBit, just use a function pointer for that.
• extend ClockEnableBit a bit so it can deal with refcounting. It'd need a pointer to the refcount variable, so it'd increase it from 2 bytes to 8 bytes...
• Remove refcounting from the RCC code. Make the individual drivers (ADC, USART) handle it. The nwe can use ClockEnableBit as-is. UART on embassy-nrf already does this, it's not that bad.

I'm sort of leaning towards option 3.

[Dirbaio]

okay i've merged #2835 #2974 which do the generic removal for spi and i2c respectively. I think the approach works, we're ready to do it for all embassy-stm32.

Could you do these changes for uart?

• Do the same as in the above PRs (same naming, use the peri_trait! macro, etc.
• put a refcount in State of the rx/tx halves, like embassy-nrf
• remove force refcounting for usart. (probably remove the force_refcount thing entirely, we're not going to use it if the plan is to move away from refcounting integrated into RCC).
• in tx/rx drop, when refcount reaches zero, disable the usart with the ClockEnableBit.

[honzasp]

Awesome, I will try to do it today :)

[honzasp]

I have a related question on your PR that introduced the ClockEnableBit: #2835 (comment)

[honzasp]

Looking at the refcount_statics, at least for STM32H7 they are needed for ADC, FDCAN and OPAMP. One way how to handle these peripherals with the ClockEnableBit structure would be to allocate the refcounts into an array, and store just an 8-bit index into this array inside ClockEnableBit, to avoid the need for storing a 32-bit pointer. (However, that's not needed for this PR)

[honzasp]

I reworked the PR, following the same approach as you used for SPI and I2C. Some comments:

• I could not use the peri_trait! and peri_trait_impl! macros, because there are two UART drivers with two different states (buffered and unbuffered). However, I tried to keep the traits similar to those generated by these macros.
• I dropped the FullInstance trait (which represented non-LP UART) and renamed BasicInstance to Instance. The FullInstance trait was not used anywhere, and the distinction between normal and low-power UART is stored in Info::kind.
• The tx and rx halves share a refcount stored in State (I model this on the embassy-nrf UART driver). It's stored as an AtomicU8, but then I found that not all targets support atomic subtractions, so I use a critical section to guard the decrement of the ref count. But perhaps there is a more elegant way?

[Dirbaio]

thanks!

[honzasp]

Looks like the CI failed, but the error doesn't seem related to the PR. Is it a spurious failure, or did I introduce some subtle breakage?

[Dirbaio]

spurious failure probably