esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
760 stars 212 forks source link

Type erase DMA channels from peripheral drivers #2248

Closed Dominaezzz closed 1 month ago

Dominaezzz commented 1 month ago

The goal of this issue is to (optionally) erase the dma channel type information from drivers. i.e.

Achieving this will require the following changes.

  1. The RegisterAccess trait will have to be changed to take &self/&mut self rather than being static/global. This will allow it to hold some state, which will be the "channel number".
  2. Since the RX and TX parts of the channel are used in separate objects, ChannelRx<_> and ChannelTx<_>, the RegisterAccess trait will have to be split up into an RX half and TX half.
  3. The interrupts are currently accessed statically from the interrupt handlers and won't be too happy with self being added to the RegisterAccess trait, so it's easier if the interrupt methods go into a separate trait. So in total there's three traits, RX, TX, Interrupts.
  4. Now that the DMA traits take self, on the GDMA side a AnyDmaChannel can be implemented.
  5. On the PDMA side, a AnySpiChannel and AnyI2sChannel can be implemented. (Just in case, if you're wondering why Any is useful on the PDMA side, go look at the base ESP32's TRM's DMA section)
  6. After this the drivers that use DMA can be changed to look something like this.
#[gdma]
type AnyChannel = AnyDmaChannel; // Or `AnyGdmaChannel` ?
#[pdma]
type AnyChannel = AnySpiChannel;

pub struct SpiDma<'d, ..., CH: DmaChannel = DefaultChannel> { /* .... */ }

I've started with #2247 to make my life easier in RustRover.

bugadani commented 1 month ago

This is an idea I've been toying with for a while now and if you can make it work, that's awesome and enables quite a bit more, like giving the same treatment to peripheral instances. However, the benefits are a bit more nuanced than with GPIOs. The examples you cherry-picked are working because the DMA channel is the last type parameter, but it needs a bigger change for SpiDmaBus where it's the second to last one. (I'd like to avoid SpiDmaBus<'d, _, _, _, Async> for example).

On the PDMA side, a AnySpiChannel and AnyI2sChannel can be implemented.

We shouldn't need those I think. As long as the peripheral constructor makes sure to only take a suitable channel, we should be able to convert that to an AnyChannel, I hope.

bugadani commented 1 month ago

On the PDMA side, a AnySpiChannel and AnyI2sChannel can be implemented. (Just in case, if you're wondering why Any is useful on the PDMA side, go look at the base ESP32's TRM's DMA section)

Terms and conditions apply:

Eight peripherals on ESP32-S2 have DMA features. As shown in Figure 2-1, UART0 and UART1 share one Internal DMA; SPI3 and ADC Controller share one Internal DMA; AES Accelerator and SHA Accelerator share one EDMA (i.e. Crypto DMA); SPI2 and I2S0 have their individual EDMA. Besides, the CPU Peripheral module on ESP32-S2 also has one Copy DMA.

This might be a bit more hairy than ideal :)

Dominaezzz commented 1 month ago

Yeah we can put a pin on the PDMA side 😄

The examples you cherry-picked are working because the DMA channel is the last type parameter

Yeah I was being cheeky there, I might leave the messier ones as an exercise to other motivated developers.

Dominaezzz commented 1 month ago

The examples you cherry-picked are working because the DMA channel is the last type parameter, but it needs a bigger change for SpiDmaBus where it's the second to last one.

Turns out the Rust insists that any generic params with defaults must be last in the list of params, or rather there must be no non-default params to the right of it. So this will likely need a breaking change for most drivers, unless we also specify default for the other parameters on the right. I'm not too interested in this part tbh. I'll just do the Camera driver as an example and the rest can be done eventually.

MabezDev commented 1 month ago

On the PDMA side, a AnySpiChannel and AnyI2sChannel can be implemented. (Just in case, if you're wondering why Any is useful on the PDMA side, go look at the base ESP32's TRM's DMA section)

I think we can and should avoid PDMA specific stuff, and only have AnyDmaChannel for everything. We should do the type checking to ensure that a given PDMA channel is compatible with the peripheral on the peripherals constructor. Once its past that type checking, internally we can just turn it into AnyDmaChannel and use it like normal.

I.e

pub fn with_dma<C, DmaMode>(
    self,
    mut channel: Channel<'d, C, DmaMode>,
) -> SpiDma<'d, crate::peripherals::SPI2, C, M, DmaMode>
where
    C: DmaChannel,
    C::P: SpiPeripheral + Spi2Peripheral,
    DmaMode: Mode,
{
    channel.tx.init_channel(); // no need to call this for both, RX and TX

    SpiDma {
        spi: self.spi,
        channel: channel.into(), // convert the channel into `AnyDmaChannel` since we now know the channel can be used with this peripheral.
        _mode: PhantomData,
    }
}
Dominaezzz commented 1 month ago

and only have AnyDmaChannel for everything.

That's going to be tough, at least with the way I'm planning to implement erasure. I think doing what you're describing might need dyn or delegate to work. (Might be a skill issue on my part, I've only just started using rust recently)

We can discuss further when I make the erasure PR, as we'll have something more concrete to scrutinize.

MabezDev commented 1 month ago

We can discuss further when I make the erasure PR

Sounds good, if you have a sketch on how you'd impl it we can discuss it before the PR. I'd like to avoid dyn, but we have already used delegate quite a bit.

Dominaezzz commented 1 month ago

There's a write up in the PR description.

(I already had a draft yesterday (don't tell Daniel) which is why I made the PR instead of discussing/sketching it out first)