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

Simplify dma usage #1238

Merged
merged 6 commits into from
Mar 13, 2024
Merged

Simplify dma usage #1238

merged 6 commits into from
Mar 13, 2024

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Mar 4, 2024

We used to move the driver and the buffers into the DMA transfer and have a way to get them back. That's to prevent the user from initiating another transfer or tamper with the buffers while the transfer is in progress.

However, this makes code using the API look quite clumsy and cumbersome.

We can avoid that by taking buffers and self as &mut (with appropriate life-times)

e.g.

before

        let transfer = spi.dma_transfer(send, receive).unwrap();
        (receive, send, spi) = transfer.wait().unwrap();

now

        let transfer = spi.dma_transfer(&mut send, &mut receive).unwrap();
        transfer.wait().unwrap();

Basically, the same thing we did to the passing of GPIOs into drivers (where we previously moved the GPIOs into the driver and had a free function to get them back - now we can pass &mut pins and "get them back" once the driver is dropped)

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

MabezDev commented Mar 5, 2024

Sorry for these comments trickling in one by one :D, but should SpiDma also have a HalfDuplexReadWrite implementation? I guess it can't really because the buffer requirements for DMA usage are stronger ('static lifetime required) than the trait.

Maybe longer term we could think about relaxing this 'static requirement, and instead document that drivers should never be core::mem::forget'ed 🤔.

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Mar 6, 2024

Yeah, currently it can't but t.b.h. I wasn't sure if embedded-dma isn't too strict while doing the initial implementation but it's used a lot in other HALs so I just used it.

While requiring 'static is probably the safest thing to do, it's also somewhat inconvenient. Lifting that requirement is probably a good next step

@MabezDev
Copy link
Member

MabezDev commented Mar 6, 2024

While requiring 'static is probably the safest thing to do, it's also somewhat inconvenient. Lifting that requirement is probably a good next step

I'm glad we're on the same page, I'll make an issue to track/discuss that elsewhere. This PR LGTM now, just waiting on the next release. For my QSPI display use case, I can pump out an entire 356 * 400 * 2 frame buffer in around 7ms with DMA assistance 😎.

@MabezDev
Copy link
Member

MabezDev commented Mar 6, 2024

I opened #1245 with my thoughts.

@MabezDev
Copy link
Member

I think because this affects the public API I guess we need a change log entry - after that, this looks good to go!

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Mar 11, 2024

I think because this affects the public API I guess we need a change log entry - after that, this looks good to go!

Oh yes - I just was a bit lazy since this shouldn't get into 0.16.0 or a patch release for that 😄 will add it before marking this as ready

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Once CHANGELOG.md gets updated I think this LGTM. Once we've published our patch release tomorrow and @MabezDev has given his final stamp of approval, we can merge this I think.

Thanks for this contribution, this looks much nicer now!

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM!

@jessebraham jessebraham marked this pull request as ready for review March 12, 2024 15:38
@jessebraham
Copy link
Member

I think we're ready to move forward with this! If you don't mind making the last couple changes here, that'd be appreciated!

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.

LGTM, thanks!

@MabezDev MabezDev added this pull request to the merge queue Mar 13, 2024
Merged via the queue into esp-rs:main with commit c283563 Mar 13, 2024
18 checks passed
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.

4 participants