-
Notifications
You must be signed in to change notification settings - Fork 214
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
Migrate Camera to a move based API #2242
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "circular if you can process it fast enough" approach is interesting.
Let's start moving these buffer types out to submodules, the main dma module is big enough without these as it is.
2195702
to
84ed346
Compare
I'll do this one in a separate PR, I'm scared of merge conflicts from doing it here. |
577bdbd
to
be113ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now. I also have an ESP32S3-EYE now and can confirm the example is still working
# Conflicts: # esp-hal/src/lcd_cam/cam.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
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 📝
cargo xtask fmt-packages
command to ensure that all changed code is formatted correctly.CHANGELOG.md
in the proper section.Extra:
Pull Request Details 📖
Description
Similar to #2191, this PR migrates the
Camera
driver to move based API for the same reasons.Camera
now accepts aDmaRxBuffer
rather than a plain slice, providing flexibility with DMA.This PR depends on #2213 and #2191 (I need to refactor it to expose the newView
type).This PR introduces a new
DmaRxBuffer
implementation calledDmaRxStreamBuf
. It's effectively the "circular transfer" version ofDmaRxBuf
but with an important difference, it doesn't create a circle of descriptors, which avoids the problem in #2021. It also provides a zero copy API and exposes EOFs (See #2033). There's more info on how it works in the doc of the struct.To support this new
DmaRxBuffer
implementation, I've added an associated typeView
toDmaRxBuffer
(andDmaTxBuffer
) to allow limited but safe access to the buffer during the DMA transfer. This is what lets users read from the buffer during the transfer.Now the choice of "one shot" or "circular" (or any other transfer type) is based on the dma buffer implementation rather than the driver, effectively achieving half of #2035 .
I haven't provided a
with_buffer
/SpiDmaBus
equivalent here because most people using this driver would typically just start one transfer and endlessly read from it for the rest of their application.Users can easily create a wrapper if they have a more specific use case.
Testing
Ran the modified example on my devkit and it's even more reliable than before.
cc @MabezDev for running the example on your ESP32-S3-EYE. (I don't think anyone else on the team has a ESP32-S3 + OV2640 devkit).