De-duplicate DMA transfer implementations #1550
Conversation
| /// Wait until the transfer is done. | ||
| /// Depending on the peripheral this might include checking the DMA | ||
| /// channel. | ||
| fn peripheral_wait_dma(&mut self, is_tx: bool, is_rx: bool); |
There was a problem hiding this comment.
Does this just wait for the DMA to empty it's data into the peripheral's FIFO? Or also wait for the peripheral to empty it's FIFO as well?
There was a problem hiding this comment.
This is impl specific I think, for SPI the peripheral FIFO is checked, for others its DMA & peripheral registers. I guess it doesn't matter what the impl does, as long as it does the expected thing which is returns when all bytes are sent/recv'd by the peripheral.
| where | ||
| I: dma_private::DmaSupportTx, | ||
| { | ||
| instance: &'a mut I, |
There was a problem hiding this comment.
In a future PR, it might be interesting to use BorrowMut/DerefMut here instead of a plain mut ref to solve the #1512.
There was a problem hiding this comment.
I have ideas how to avoid duplicating everything when introducing a Move-API but would need to explore that in more detail - feel free to do the same on top of this (I'm not yet too happy with my own ideas)
| I: dma_private::DmaSupportTx, | ||
| { | ||
| fn drop(&mut self) { | ||
| self.instance.peripheral_wait_dma(true, false); |
There was a problem hiding this comment.
Some peripherals like LCD_CAM support cancellation, where you can just stop the transfer on the spot. There should probably be a cancel_and_wait method with a default that just waits like this.
There was a problem hiding this comment.
Though, since this shouldn't be introducing new behavior, maybe this can be saved for a different PR.
There was a problem hiding this comment.
Makes sense! As you said probably not in this PR
There was a problem hiding this comment.
LGTM, thanks! I tested related examples (including i8080, @Dominaezzz ) and they work fine for me.
There was a problem hiding this comment.
Overall looks good to me! I think @Dominaezzz brought up some good points which we should think about before merging.
Agreed we definitely shouldn't forget about those points. My original idea for this PR was to have one place for the implementation and explore the next steps in another PR. (and keep this PR a "naive" refactoring) |
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 📝
CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Previously each DMA capable peripheral had its own implementation of DMA transaction. This creates common implementations in the DMA module and the peripherals now use the shared implementation.
Testing
All DMA-related examples should work as before.
I cannot test lcd_cam examples (lacking hardware) - the i8080 example still outputs "something" on the pins at least