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

Type erase DMA channels from peripheral drivers #2248

Closed
Dominaezzz opened this issue Sep 28, 2024 · 8 comments · Fixed by #2261
Closed

Type erase DMA channels from peripheral drivers #2248

Dominaezzz opened this issue Sep 28, 2024 · 8 comments · Fixed by #2261
Assignees
Labels
peripheral:dma DMA Peripheral
Milestone

Comments

@Dominaezzz
Copy link
Collaborator

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.

  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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@bugadani bugadani added peripheral:dma DMA Peripheral and removed status:needs-attention This should be prioritized labels Sep 30, 2024
@MabezDev
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

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)

@MabezDev MabezDev added this to the 0.21.0 milestone Oct 1, 2024
@bugadani bugadani mentioned this issue Oct 1, 2024
6 tasks
@tom-borcin tom-borcin modified the milestones: 0.21.0, 0.22.0 Oct 7, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
peripheral:dma DMA Peripheral
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants