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

Erase DMA channel type from Camera and AesDma drivers #2258

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

Dominaezzz
Copy link
Collaborator

@Dominaezzz Dominaezzz commented Sep 30, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

This PR gives users the ability to degrade a DMA channel into a less specific type.
On GDMA chips this is a transformation from DmaChannel0/DmaChannel1/DmaChannel2/etc to AnyDmaChannel.
On PDMA chips this does nothing (For now).

I've added a default generic parameter to Camera and AesDma.
i.e. pub struct Camera<'d, CH: DmaChannel = AnyDmaChannel>

So now users can do this.

let dma = Dma::new(peripherals.DMA);
let channel = dma.channel0;

let channel = channel.configure(false, DmaPriority::Priority0).degrade(); // <---- DEGRADE!!!
let rx_channel = channel.rx;

// Users can also do this.
// let channel = channel.configure(false, DmaPriority::Priority0);
// let rx_channel = channel.rx.degrade(); <--- DEGRADE!!!!

let mut camera = Camera::new(
    lcd_cam.cam,
    rx_channel,
    rx_descriptors,
    cam_data_pins,
    20u32.MHz(),
);

I've only done Camera and AesDma as those two didn't require breaking changes to add.
I8080 for example has this type signature struct I8080<'d, CH: DmaChannel, DM: Mode>.
To add the erased DMA channel type I have to do reorder the generics like so struct I8080<'d, DM: Mode, CH: DmaChannel = AnyDmaChannel>, which is a breaking change. There's the option of adding a default mode as well but I'm not interested in making this decision nor carrying out the admin for the breaking changes.

For the PDMA side, having an "Any" type isn't as important because there is always a single answer to the question "What DMA channel do I need to use for this peripheral?" (Except on the ESP32 SPI DMA but we won't get into that here).
In this case I think it's reasonable to use a conditional type alias, like what I did for Aes.

#[cfg(gdma)]
type DefaultChannel = crate::dma::AnyDmaChannel;
#[cfg(pdma)]
type DefaultChannel = (); // Replace with PDMA channel once support is added.

pub struct AesDma<'d, C = DefaultChannel> { /* ... */ }

The nice this about this is that PDMA chips always have "type erasure" in the sense that you'll never have to name the channel, even if you don't call degrade().

For SPI and I2S that have multiple instances, an associated type can be used.

trait Instance {
    type DefaultChannel: DmaChannel;

    // .... write, read, etc.
}

impl Instance for SPI2 {
    #[cfg(gdma)]
    type DefaultChannel = crate::dma::AnyDmaChannel;
    #[cfg(pdma)]
    type DefaultChannel = Spi2DmaChannel;
}

impl Instance for SPI3 {
    #[cfg(gdma)]
    type DefaultChannel = crate::dma::AnyDmaChannel;
    #[cfg(pdma)]
    type DefaultChannel = Spi3DmaChannel;
}

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

If you don't like my proposal for the PDMA, you can land this PR since it implements erasure on the GDMA side and build PDMA erasure on top of it I guess.
If you choose to take this path of further erasure, just beware of the ESP32-P4 because the GDMA isn't as universal as the current chips. Half of the channels work on some peripherals and the other half on a different set of peripherals. (The difference is PSRAM support)

This is the finish line for me, I've done the heavy lifting. Someone else can pick up the admin of updating all the other drivers and creating migration guides 😄 .

Testing

It builds. I was also able to name Camera without specifying the channel type after calling degrade().

@bugadani
Copy link
Contributor

I appreciate your work and I'm also happy to take over.

I've taken the liberty to remove the "Fixes" from the PR description, as I'd prefer not closing the tracking issue at a halfway point.

@Dominaezzz
Copy link
Collaborator Author

I've taken the liberty to remove the "Fixes" from the PR description

Ah true, thanks! I forgot to remove that after I wrote the last paragraph haha

Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this PR doesn't seem to bring in any user-facing breaking changes, I'm happy to get this in as is. Thanks!

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bjoernQ bjoernQ added this pull request to the merge queue Oct 1, 2024
Merged via the queue into esp-rs:main with commit 8e9f6b5 Oct 1, 2024
28 checks passed
@Dominaezzz Dominaezzz deleted the erase_dma_channel branch October 1, 2024 08:50
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

Successfully merging this pull request may close these issues.

3 participants