Use DmaBuffer API for I2S#5603
Conversation
| Buf: DmaTxBuffer, | ||
| { | ||
| /// Waits for the transfer to finish and returns the peripheral and buffer. | ||
| pub async fn wait_async(mut self) -> Result<(I2sTx<'d, Async>, Buf::Final), DmaError> { |
There was a problem hiding this comment.
wait_async's API isn't cancel friendly. There should be a separate method like this which can be cancelled without dropping the transfer object or peripheral/buffer.
| { | ||
| /// Waits for the transfer to finish and returns the peripheral and buffer. | ||
| pub async fn wait_async(mut self) -> Result<(I2sTx<'d, Async>, Buf::Final), DmaError> { | ||
| DmaTxFuture::new(&mut self.i2s_tx.tx_channel).await?; |
There was a problem hiding this comment.
For TX, just because the DMA is done, it doesn't the peripheral is. I think you still need a while !self.is_done() {} here anyway.
| self.i2s_rx.rx_channel.stop_transfer(); | ||
| self.i2s_rx.i2s.rx_stop(); |
There was a problem hiding this comment.
For RX, the peripheral should be stopped before the DMA.
| pub async fn wait_for_available_async(&mut self) -> Result<(), DmaError> { | ||
| DmaRxFuture::new_with_config( | ||
| &mut self.i2s_rx.rx_channel, | ||
| enum_set!(DmaRxInterrupt::SuccessfulEof | DmaRxInterrupt::Done), |
There was a problem hiding this comment.
| enum_set!(DmaRxInterrupt::SuccessfulEof | DmaRxInterrupt::Done), | |
| enum_set!(DmaRxInterrupt::Done), |
DmaRxInterrupt::SuccessfulEof will always be accompanied with a DmaRxInterrupt::Done anyway.
The ideal would be to have one method for DmaRxInterrupt::SuccessfulEof and another for DmaRxInterrupt::Done, but that doesn't have to be in this PR.
There was a problem hiding this comment.
+1 this driver should see more improvements in future (since it's getting more and more popular)
| self.i2s_rx.rx_channel.stop_transfer(); | ||
| self.i2s_rx.i2s.rx_stop(); |
There was a problem hiding this comment.
For RX, stop the peripheral before the DMA.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/hil full --tests i2s, misc_non_drivers |
|
Triggered full HIL run for #5603. Run: https://github.com/esp-rs/esp-hal/actions/runs/26517161789 Status update: ❌ HIL (full) run failed (conclusion: failure). |
| let (i2s_tx, buf) = self.release(); | ||
|
|
||
| if i2s_tx.tx_channel.has_error() { | ||
| Err(DmaError::DescriptorError) |
There was a problem hiding this comment.
The peripheral and buffer should still be returned when there is an error.
| pub async fn wait_for_available_async(&mut self) -> Result<(), DmaError> { | ||
| DmaTxFuture::new_with_config( | ||
| &mut self.i2s_tx.tx_channel, | ||
| enum_set!(DmaTxInterrupt::Eof), |
There was a problem hiding this comment.
It looks like this interrupt isn't cleared/consumed. So it's only async the first time it's called and subsequent calls immediately return.
| desc.next = next; | ||
| next = desc; | ||
|
|
||
| desc.set_owner(Owner::Cpu); |
There was a problem hiding this comment.
I think this should be Owner::Dma, otherwise available_bytes returns a value that is too high in the first pass.
If I'm understanding this buffer type correctly, it sets up the descriptors such that they are all linked together and the first half of the linked list contains the prefilled data and the second half contains empty descriptors.
When the user pushes more data after the transfer starts, it takes descriptors from the start of the linked list, instead of the start of the 2nd half.
The push function checks the descriptor ownership to see if it's available for filling.
Once the dma gets through the first half of the linked list, the push function is going to think the DMA has gotten through the second half as well, even if the DMA hasn't, due to these descriptors being marked as owned by the CPU.
Changelog
esp-hal
dma_rx_buffer! / dma_tx_buffer!.
Migration guide
esp-hal/I2S driver
I2S DMA now uses the DmaBuffer API
I2S no longer uses manually passed DMA descriptors or the generic DmaTransfer* types. Buffers are created with the DMA buffer macros, channels are built without descriptors, and transfers are started by consuming the channel.
Use dma_rx_buffer! / dma_tx_buffer! instead of the *_stream_buffer! macros when you need a finite, one-shot transfer rather than continuous streaming.
Starting transfers
read / write take ownership of the channel. For async code, call .into_async() on I2s before building the RX/TX channel.
On failure, read / write return Err((Error, I2sRx, BUF)) (or the TX equivalents), so you can recover both the channel and the buffer.
Transfer handles and streaming I/O
Generic DmaTransferRxCircular / DmaTransferTxCircular (and the old I2sReadDmaTransferAsync / I2sWriteDmaTransferAsync) are replaced by I2sRxDmaTransfer / I2sTxDmaTransfer. These deref to the buffer view, exposing available_bytes(), pop(), push(), and push_with().
loop { - transfer.wait_for_data().await?; - let avail = transfer.available()?; + transfer.wait_for_available_async().await?; + let avail = transaction.available_bytes(); if avail > 0 { - transfer.pop(&mut rcv[..avail])?; + transfer.pop(&mut rcv[..avail]); } }Finishing a one-shot transfer