Type erase DMA channels from peripheral drivers
Closed this issue ยท 8 comments
The goal of this issue is to (optionally) erase the dma channel type information from drivers.
i.e.
I8080<'d, DmaChannel0>
->I8080<'d>
Camera<'d, DmaChannel2>
->Camera<'d>
SpiDma<'d, ...., SpiDmaChannel>
->SpiDma<'d, ....>
- etc
Achieving this will require the following changes.
- 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". - Since the RX and TX parts of the channel are used in separate objects,
ChannelRx<_>
andChannelTx<_>
, theRegisterAccess
trait will have to be split up into an RX half and TX half. - The interrupts are currently accessed statically from the interrupt handlers and won't be too happy with
self
being added to theRegisterAccess
trait, so it's easier if the interrupt methods go into a separate trait. So in total there's three traits, RX, TX, Interrupts. - Now that the DMA traits take
self
, on the GDMA side aAnyDmaChannel
can be implemented. - On the PDMA side, a
AnySpiChannel
andAnyI2sChannel
can be implemented. (Just in case, if you're wondering whyAny
is useful on the PDMA side, go look at the base ESP32's TRM's DMA section) - 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.
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.
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 :)
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.
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.
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,
}
}
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.
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.
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)