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 type params #2261

Merged
merged 36 commits into from
Oct 8, 2024
Merged

Erase DMA type params #2261

merged 36 commits into from
Oct 8, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 1, 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 implements DMA type erasure. In the current system, the peripherals prescribe the erased DMA channel type. This allows erase-using-ZST-types on PDMA devices, and will allow handling P4's different DMAs without a ton of peripheral-specific #[cfg] gates. Pretty good idea, thanks @Dominaezzz even if this was terribly tricky to implement 😅

There is no AnyDmaChannel type. The goal of this exercise was not to introduce one, but to remove the type parameter from peripherals, and the current implementation is a bit more information-rich with (barring fn degrade) the same user-facing API.

Fixes #2248

@bugadani bugadani changed the title Erase DMA type params Erase DMA type params (maybe, eventually) Oct 1, 2024
esp-hal/src/dma/pdma.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Dominaezzz Dominaezzz left a comment

Choose a reason for hiding this comment

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

A problem I forsee is, what happens when users call .degrade() on a PDMA channel before creating the driver with it?

EDIT: I suppose this would easily happen on multi chip libraries/applications.

esp-hal/src/lcd_cam/cam.rs Outdated Show resolved Hide resolved
@Dominaezzz
Copy link
Collaborator

should we keep DMA type parameters optionally

Yes please. I'd like to be able to get my channel back from the driver, ideally in the same condition (type) that I gave to it.

This is particularly important on chips with a very limited number of channels.

esp-hal/src/dma/pdma.rs Outdated Show resolved Hide resolved
@MabezDev
Copy link
Member

MabezDev commented Oct 2, 2024

I think full erasure of the type is the correct path. I don't think having DMA register access as a ZST vs a runtime access makes any real difference. Most of the time spent on DMA setup is with descriptors and or the buffer preparation. It being fully erased means we can apply this to any driver without having to rely on DMA being the last generic param on a driver to avoid inference issues.

@Dominaezzz
Copy link
Collaborator

I was hoping to be able to statically tell if a DMA channel supported EDMA or not. I'm guessing you envision this sort of thing to be a runtime check now?

@bugadani bugadani changed the title Erase DMA type params (maybe, eventually) Erase DMA type params Oct 2, 2024
@bugadani bugadani force-pushed the erase-dma branch 20 times, most recently from dab86f9 to 2fcf862 Compare October 7, 2024 13:44
@bugadani
Copy link
Contributor Author

bugadani commented Oct 8, 2024

Latest push is just a rebase.

hil-test/tests/qspi.rs Outdated Show resolved Hide resolved
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Very pleased to see these type removed!

I'm quite happy with the legibility of the compiler errors too:

error[E0277]: the trait bound `I2s1DmaChannel: DmaChannelConvert<Spi2DmaChannel>` is not satisfied
   --> src/bin/embassy_spi.rs:63:19
    |
63  |         .with_dma(dma_channel.configure_for_async(false, DmaPriority::Priority0))
    |          -------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `DmaChannelConvert<Spi2DmaChannel>` is not implemented for `I2s1DmaChannel`
    |          |
    |          required by a bound introduced by this call
    |

It's fairly good, which is not always the case :D.

@bugadani bugadani added this pull request to the merge queue Oct 8, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 8, 2024

Very pleased to see these type removed!

I'm quite happy with the legibility of the compiler errors too:

error[E0277]: the trait bound `I2s1DmaChannel: DmaChannelConvert<Spi2DmaChannel>` is not satisfied
   --> src/bin/embassy_spi.rs:63:19
    |
63  |         .with_dma(dma_channel.configure_for_async(false, DmaPriority::Priority0))
    |          -------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `DmaChannelConvert<Spi2DmaChannel>` is not implemented for `I2s1DmaChannel`
    |          |
    |          required by a bound introduced by this call
    |

It's fairly good, which is not always the case :D.

Could we improve it with rust-lang/rust#119888 ?

We are already on MSRV 1.79 and I think it was introduced in 1.78

Merged via the queue into esp-rs:main with commit 7ca1b43 Oct 8, 2024
28 checks passed
@bugadani bugadani deleted the erase-dma branch October 8, 2024 14:36
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.

Type erase DMA channels from peripheral drivers
4 participants