Use move based api for I2S

[AstrickHarren]

This is to fix #3285, as in #3612 @Dominaezzz laid out reasons for a move-based i2s API so I thought I may help a bit about #2269 . Haven't tested or anything, just to put the draft PR here to see if I'm in the right direction. Also I'm very aware of #3219 but it seems to be dormant for a bit. I really need to make i2s working for my little project with mic and speaker so let me know if anyone's already working on this and I'll be glad to close my PR.

• I2S async works with DmaTxStreamBuf similar to DmaRxStreamBuf, tested on an ESP32-S3 with MAX98357A to output sound
  • Design is similar to its RX counterpart as well, based on a rotating linked list instead of a circular list.
• Support for resuming I2S when DMA stops. Not sure how to detect DMA stopping. Looks like we don't listen to GDMA_OUT_TOTAL_EOF_CHn_INT, so the other ways around maybe
  1. force check_owner and listen to error
  2. if I find all the descriptors are owned by CPU, then DMA probably already stopped (stopped because reached end of linked list)

[Dominaezzz]

I'm very glad to see someone else getting the DMA move stuff to the finish line. (I was struggling to find the motivation to touch the I2S driver)

• Not sure how to detect DMA stopping.

There isn't a one size fits all solution for this one. Some peripherals will tell you when this happens, and other won't.
The DMA interrupts could be used but only if the buffer has been setup to trigger the right interrupt, like you said with the check_owner bit.

There's probably other DMA registers that can be polled to check for this condition but I haven't looked yet.

I'm leaning towards leaving this problem to the user, because regardless of whatever solution you choose here, it is still up to the user to restart the I2S/DMA transfer when it stops.

[AstrickHarren]

Think this may be ready.

Added the TxStreamBuf that basically works symmetrically with the RxStreamBuf implementation.

A few problems I couldn't get my heads around

1. I tried to utilize DescriptorEmpty interrupt as a sign for DMA rx completion but I couldn't get the interrupt when the descriptors are used up. Compared to TotalEof which I assume is the counterpart for tx and it works every time when descriptors are used up.
2. not sure how to detect DMA rx stopping if DescriptorEmpty is not triggered. One way is to listen for I2sInterrupt::RxDone which I'm doing right now, but I don't think it works on every esp device.
3. I'm not sure what exactly does rx_eof_num do. Looks like it triggers SUC_EOF, but it doesn't make sense to set the number if I2S is meant to be used continuously. But I can't set it to 0 or very small, otherwise dma will only put that many data in each of the descriptors and the linked list is used up every quickly. Also the number can't be a huge arbitrary number too, it seems dma will have the same problem as before, not using up the descriptors entirely. You'd think it will fulfill descriptors until it reaches rx_of_num of data and effectively leave the modulo to the last descriptor but experiments seem to show that it leaves modulo data to every descriptor for some reason. Therefore, I left that as an argument and recommend it to be set as the same size as the descriptors in the list.

I'll fix the docs and also changelog once I get a basic review.

[Dominaezzz]

1. I tried to utilize DescriptorEmpty interrupt as a sign for DMA rx completion but I couldn't get the interrupt when the descriptors are used up. Compared to TotalEof which I assume is the counterpart for tx and it works every time when descriptors are used up.

The RX DMA doesn't "complete". It just keep going until something goes wrong. DescriptorEmpty will come out when the RX data overflows the DMA buffer basically. You'd have to wait for a peripheral interrupt or SucEof if the peripheral emits it when you need it.

2. not sure how to detect DMA rx stopping if DescriptorEmpty is not triggered. One way is to listen for I2sInterrupt::RxDone which I'm doing right now, but I don't think it works on every esp device.

Yeah DMA RX doesn't stop. What you want is to figure out when the peripheral has stopped, then you can stop the DMA.
So this sounds correct to me.

Which esp device are you unsure of?

3. I'm not sure what exactly does rx_eof_num do. Looks like it triggers SUC_EOF, but it doesn't make sense to set the number if I2S is meant to be used continuously. But I can't set it to 0 or very small, otherwise dma will only put that many data in each of the descriptors and the linked list is used up every quickly. Also the number can't be a huge arbitrary number too, it seems dma will have the same problem as before, not using up the descriptors entirely. You'd think it will fulfill descriptors until it reaches rx_of_num of data and effectively leave the modulo to the last descriptor but experiments seem to show that it leaves modulo data to every descriptor for some reason. Therefore, I left that as an argument and recommend it to be set as the same size as the descriptors in the list.

You're on the right track. I2S will emit an EOF every rx_eof_num bytes.

rx_eof_num does have a max, so you can't just pick a huge number.

Leaving it as an argument is the right way to go imo.

I'll review your PR shortly

[Dominaezzz]

First batch of comments.

Overall looks good, I'm excited to land this

[Dominaezzz]

A small downside to this PR is we've lost support of u16, u32, etc. but that can be brought back later.

[AstrickHarren]

Thanks a lot for PR reviewing @Dominaezzz appreciated!

[AstrickHarren]

Which esp device are you unsure of?

I meant esp32c3 and similar devices in the sense that they don't have the interrupt

[Dominaezzz]

@AstrickHarren this is super close to the finish line, any plans on getting back to it?

[AstrickHarren]

was too busy tho, I'll be working on it soon

[AstrickHarren]

I think we already have DmaLoopBuf if I'm not mistaken. I'll fix the conflicts and add macros like the one for DmaRxStreamBuf

[bugadani]

The goal of the QA test is to figure out a path forward with #4135 . If it's still possible to starve out an I2S transfer (and why wouldn't it be, if we don't drain the data we read?), then the test should be modified to match the old behaviour.

[AstrickHarren]

Regarding the late error:

Currently there is no good way to check if an ongoing transfer is late (due to data overflow in rx or underflow in tx). This is because transfers are separated from their underlying DMA streaming buffer implementations. I attempted to add an I2sTxDmaTransfer::is_done by reading the corresponding register but it looks like esp32s2 and esp32 don't have such register.

For others:

esp-hal/esp-hal/src/i2s/master.rs

Lines 2185 to 2187 in 24478a4

	fn rx_done(&self) -> bool {
	self.regs().int_raw().read().rx_done().bit_is_set()
	}

For esp32s2 and esp32, it doesn't work since in_suc_eof will be triggered in the streaming process.

esp-hal/esp-hal/src/i2s/master.rs

Lines 1761 to 1764 in 24478a4

	fn rx_done(&self) -> bool {
	self.regs().int_raw().read().in_suc_eof().bit_is_set()
	}

The other way will be to report a DmaError::Late whenever the streaming buffers are full (for rx and empty for tx) so a push(pop) to the transfer will cause such an error. This will work on esp32s2.

Please let know if there is a third option. @Dominaezzz

[AstrickHarren]

Also, I just saw the test was counting late errors, which confuses me since if a DMA transfer is already late, all subsequent pop to the transfer should also be late right? I could be wrong.

[Dominaezzz]

On the RX side, you can tell if the transfer is late if the DMA buffer has no data and the peripheral is no longer running.

On the TX side, there's a few way and you'll need to DMA data structure to look a certain way.

[Dominaezzz]

Actually the same should be doable for TX. If the buffer is full and the peripheral is no longer running. See this for what I mean

Also, I just saw the test was counting late errors, which confuses me since if a DMA transfer is already late, all subsequent pop to the transfer should also be late right? I could be wrong.

Which test is this? (link pls)

[AstrickHarren]

Actually the same should be doable for TX. If the buffer is full and the peripheral is no longer running. See this for what I mean

I didn't find a good way to check if I2s is done on chips esp32 and esp32c2. Looks like they don't have such a register? On the other chips, Late can be consistently reported by checking the rx_done or tx_done register. I think the example also doesn't work for those chips right? (@Dominaezzz )

Also, I just saw the test was counting late errors, which confuses me since if a DMA transfer is already late, all subsequent pop to the transfer should also be late right? I could be wrong.

Which test is this? (link pls)

esp-hal/qa-test/src/bin/embassy_wifi_i2s.rs

Lines 147 to 151 in be30dda

	let mut total_drained: usize = 0;
	let mut late_errors: u32 = 0;
	let start = Instant::now();
	// Start continuous draining to prevent DmaError(Late)

[Dominaezzz]

I'm reading the trm

[Dominaezzz]

Right, so in the S1 and S2, like you said, the I2S doesn't tell you that it's done, you just have to stop it when you're done receiving or transmitting data.
Going this route is a little complicated to design so let's use the DMA instead to check.

On the RX side, you can check for the IN_DSCR_EMPTY interrupt.

On the TX side, we'll need a different buffer implementation (sorry 😅).
A DmaTxCircularBuf that basically does the same thing as your DmaTxStreamingBuf except that it actually makes a circle. By default all the descriptors will be owned by the CPU until explicitly filled, then ownership can be given to the DMA. Auto write back and check owner must be enabled. This will mean the DMA will automatically give ownership back to the CPU and will fire an interrupt when it encounters a descriptor still owned by the CPU. The OUT_DESC_ERR interrupt.
This is basically how the DescriptorChain struct works.

Let me know if this makes sense. I'm not very good at explaining this sort of thing.

[AstrickHarren]

I could be wrong but I think on the TX side, TotalEof will be the interrupt to listen to? Should it be sufficient to detect a late for dma transfer? @Dominaezzz
Somehow I was having trouble with IN_DESC_EMPTY, I'll try to look at it once more

[Dominaezzz]

TotalEof will be the interrupt to listen to?

Yes that would work but every descriptor would need to have it's SUC_EOF marked. (which shouldn't be an issue here I suppose)

[Token-Thinker]

work was kicking my but and need summer off. anything i could attempt to assist with here? getting back into the grove of things

[AstrickHarren]

TotalEof will be the interrupt to listen to?

Yes that would work but every descriptor would need to have it's SUC_EOF marked. (which shouldn't be an issue here I suppose)

Are you sure about that? I'm reading manual but I didn't find such instructions? Would you mind point me to it?

work was kicking my but and need summer off. anything i could attempt to assist with here? getting back into the grove of things

I think we are left with how to detect the end of an i2s transfer. You can get the idea of it from the previous discussions.

[AstrickHarren]

@Dominaezzz I don't think IN_DESC_EMPTY works for some reason and TOTAL_EOF works fine without manually set each SUC_EOF bit. For some reason, IN_DESC_EMPTY won't be triggered even when RxStreamBuf is filled up. Is there any step I'm missing or.

esp-hal/esp-hal/src/dma/mod.rs

Lines 2335 to 2363 in a9777d8

	fn poll(
	self: core::pin::Pin<&mut Self>,
	cx: &mut core::task::Context<'_>,
	) -> Poll<Self::Output> {
	if self
	.rx
	.pending_in_interrupts()
	.contains(DmaRxInterrupt::Done)
	{
	self.rx.clear_in(DmaRxInterrupt::Done);
	Poll::Ready(Ok(()))
	} else if !self.rx.pending_in_interrupts().is_disjoint(
	DmaRxInterrupt::DescriptorError
	| DmaRxInterrupt::DescriptorEmpty
	| DmaRxInterrupt::ErrorEof,
	) {
	self.rx.clear_interrupts();
	Poll::Ready(Err(DmaError::DescriptorError))
	} else {
	self.rx.waker().register(cx.waker());
	self.rx.listen_in(
	DmaRxInterrupt::Done
	| DmaRxInterrupt::DescriptorError
	| DmaRxInterrupt::DescriptorEmpty
	| DmaRxInterrupt::ErrorEof,
	);
	Poll::Pending
	}
	}

In particular, the future above will never be triggered by DmaRxInterrupt::DescriptorEmpty for some reason

[Dominaezzz]

In particular, the future above will never be triggered by DmaRxInterrupt::DescriptorEmpty for some reason

I'm not too sure what's going on there. You can try using check_owner instead I suppose