Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose peripheral registers when enabling the unstable-pac feature. #2948

Open
liarokapisv opened this issue May 16, 2024 · 1 comment
Open

Comments

@liarokapisv
Copy link
Contributor

liarokapisv commented May 16, 2024

Foremost, thanks a ton for your great work!

The HAL cannot cover all use-cases, and at some point users may need to access the registers directly.
This can currently be achieved by enabling the unstable-pac feature and accessing the registers.
Unfortunately, there is currently no way for the user to associate the HAL peripherals with their registers
meaning that they cannot abstract over the peripherals themselves, since the registers are exposed only through the Sealed traits.

I propose exposing the regs values from the Sealed traits when enabling the unstable-pac feature. Just the registers
should be enough, there is no reason to enforce any extra public API requirements like exposing the helper Sealed trait methods
(although I personally would find that very useful as long as the HAL does not make any promises about the compatibility between versions).

Note that this is already the case with some peripherals, like AnyPin that allow exposing their GPIO port through the block method. The best example is perhaps the timer traits and low_level driver.

This allows users to bypass the HAL locally when needed without having to change the external interface of the code.

As an example, I demonstrate one of my recent use cases:

I am using the following DACs through SPI: https://www.rohm.com/products/data-converter/d-a-converters/10bit-d-a/bu22210muv-product.
I need to run 3 SPI daisy-chains with x2 of them in each one and approach ~10khz update rate for each channel.

This is implemented on the STM32H745IIx by using DMA Streams triggered by a timer and chained DMAMUX requests to trigger SPI DR register writes and synchronized GPIO BSRR register writes for the CS handling.

The implementation, at a high-level, looks like this:

TIM CH1 -> DMA1_CH1 -> SPI1 DR (also triggers DMAMUX's GEN1)
TIM CH2 -> DMA1_CH2 -> SPI1_CS BSRR with proper PIN values to set/reset (also triggers DMAMUX's GEN2)

DMAMUX_GEN1 -> DMA1_CH3 -> SPI2 DR
DMAMUX_GEN1 -> DMA1_CH5 -> SPI3 DR

DMAMUX_GEN2 -> DMA1_CH4 -> SPI2_GPIO BSRR
DMAMUX_GEN2 -> DMA1_CH6 -> SPI3_GPIO BSRR.

This is set up to run in circular mode without interrupts, writing to the DACs is achieved by writing to proper buffer positions
associated with the SPI DR writes.

Assuming that the Dmamux peripheral and Dma requests are exposed through an appropriate driver module, (I am in the process of establishing the API to make an upstream PR), the initialization function could like so:

fn init_dacs<
    Spi1: spi::Instance,
    Spi1Sck: SckPin<Spi1>,
    Spi1Mosi: MosiPin<Spi1>,
    Spi2: spi::Instance,
    Spi2Sck: SckPin<Spi2>,
    Spi2Mosi: MosiPin<Spi2>,
    Spi3: spi::Instance,
    Spi3Sck: SckPin<Spi3>,
    Spi3Mosi: MosiPin<Spi3>,
    Tim: timer::CoreInstance + timer::GeneralInstance4Channel,
    DMAMUX: dmamux::Instance,
>(
    dmamux: DMAMUX,
    tim: Tim,
    chain_1: ChainInfo<impl gpio::Pin, Spi1, Spi1Sck, Spi1Mosi, impl timer::Ch1Dma<Tim>, impl timer::Ch2Dma<Tim>>,
    chain_2: ChainInfo<
        impl gpio::Pin,
        Spi2,
        Spi2Sck,
        Spi2Mosi,
        impl dmamux::GenDma<DMAMUX>,
        impl dmamux::GenDma<DMAMUX>,
    >,
    chain_3: ChainInfo<
        impl gpio::Pin,
        Spi3, 
        Spi3Sck,
        Spi3Mosi,
        impl dmamux::GenDma<DMAMUX>,
        impl dmamux::GenDma<DMAMUX>,
    >,
) ;

(taking liberties with the dmamux module)

Unfortunately, I don't use the DMA streams in a conventional manner. Streams driven by peripherals are used to
trigger writes to registers of different peripherals. It does not feel as if that could be cleanly encapsulated in the HAL Api.
The best I can do here, is to drop to the dma::Transfer level and start a circular transfer manually. The problem is that this requires specifying the peripheral register target and specifically that of the SPIs. Unfortunately, their registers are not exposed from the spi::Instance s.

I am forced to drop to the metapac level, but then I lose the type-safety of the above Api, needing to leak the metapac to the public interface of the function and restricting its use. I feel exposing the Sealed traits' registers would be a clean solution to this, easily generalized for other use-cases but still limiting the user so they are encouraged to contribute to the HAL instead.

@mattico
Copy link
Contributor

mattico commented May 17, 2024

I was just going to open an issue about this. I have a similar use-case, where I have an ADC which has a dual-SPI-like interface connected to an STM32 SPI in Receive-only-slave mode, plus some unusual DMA configuration.

Previously I was using stm32-rs/stm32h7xx-hal which has fairly comprehensive support for peripheral configuration, but still maybe 10% of my peripherals had some configuration which wasn't supported natively in the HAL. When this situation came up I had a few choices:

  1. Use the HAL peripheral, but after the HAL configures it change a few values in the registers directly.
  2. Copy the HAL peripheral implementation into my project and modify it to work as necessary.
  3. Add support for the feature I needed to upstream, usually after having tested the changes by doing option 1 or 2.

Adding upstream support was preferred but was often way more work, as I'd need to come up with an expansive new general interface for the feature and implement it for all of the supported microcontrollers - not just a specific hack for my specific microcontroller. Often it's not practical for an upstream HAL to support the full breadth of a peripheral's possible configurations, as the implementation and interface would be massive and complex.

Options 1 and 2 would often break when updating the HAL since they relied so much on HAL internals, but they were still very useful and convenient compared to having to create a whole new peripheral from scratch based on just the PAC. embassy-stm32 sealing away its internals makes life more difficult for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants