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

Use enumset for DMA interrupts instead of countless functions #2196

Merged
merged 8 commits into from
Sep 23, 2024

Conversation

Dominaezzz
Copy link
Collaborator

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

PR title says it all. Rather than have many listen_*, unlisten_*, etc methods for every interrupt bit, I've collapsed it down to just one method that takes an enumset.
I've also added documentation for the interrupts.

This is part of #1767.

To make it easy to review, I'm doing this in multiple steps.
The first step was to remove the methods from the pdma and gdma files.
Unfortunately I encountered #2193 and I'm choosing to fix it in this PR.

Testing

CI, I don't have any PDMA chips on me at the moment.

@bugadani
Copy link
Contributor

To make it easy to review, I'm doing this in multiple steps.

I'd rather if you sent everything in at once, if you're ready with the other parts. Just separating into small-ish commits is enough, and it's sometimes better if we have the complete picture up front.

@Dominaezzz
Copy link
Collaborator Author

To make it easy to review, I'm doing this in multiple steps.

I'd rather if you sent everything in at once, if you're ready with the other parts. Just separating into small-ish commits is enough, and it's sometimes better if we have the complete picture up front.

Noted. In this case, I don't have the other parts ready yet.
I sent this early so I cut my losses if I get a "Hell nahh" as review feedback 😄.

@Dominaezzz
Copy link
Collaborator Author

Looks like the HIL setup for the base ESP32 is broken. Almost like the usb cable is disconnected.

@bugadani
Copy link
Contributor

Looks like the HIL setup for the base ESP32 is broken. Almost like the usb cable is disconnected.

It's being looked at.

I sent this early so I cut my losses if I get a "Hell nahh" as review feedback 😄.

:) Given the +200 lines of diff... 😅

esp-hal/src/dma/gdma.rs Outdated Show resolved Hide resolved
@Dominaezzz Dominaezzz changed the title [1/x] Use enumset for DMA interrupts instead of countless functions Use enumset for DMA interrupts instead of countless functions Sep 19, 2024
@burrbull
Copy link
Contributor

Similar to https://github.com/stm32-rs/stm32f4xx-hal/pull/673/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759

@Dominaezzz
Copy link
Collaborator Author

it's sometimes better if we have the complete picture up front.

Done

esp-hal/src/dma/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/dma/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/dma/gdma.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 20, 2024

CI, I don't have any PDMA chips on me at the moment.

I can at least confirm the i2s_sound example still works on ESP32

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.

If you can make the flag enums' discriminants correspond to their actual register bits, there's a chance the compiler can see through the code and read the register into the enumset in one step.

I'll not ask you to start experimenting with this, but I'll try and look into it because there's a bit of efficiency we're leaving on the table.

Otherwise, thank you a lot, this looks awesome :)

@bugadani bugadani linked an issue Sep 22, 2024 that may be closed by this pull request
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.

Thanks

@bjoernQ bjoernQ added this pull request to the merge queue Sep 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 23, 2024
@Dominaezzz
Copy link
Collaborator Author

Looks like #2210 needs to go in first

@bugadani bugadani added this pull request to the merge queue Sep 23, 2024
Merged via the queue into esp-rs:main with commit 794cdb0 Sep 23, 2024
28 checks passed
@Dominaezzz Dominaezzz deleted the dma_int_bits_as_enum branch September 23, 2024 09:23
@Dominaezzz Dominaezzz mentioned this pull request Sep 28, 2024
6 tasks
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.

GDMA vs DMA interrupt bit discrepancies
4 participants