Added I2S (native) output support for RP2040

[tomcombriat]

Hello and happy new year!

I have started fiddling with the RP2040 (what a nice chip!) and as it does possess I2S support, found a bit sad not to use it in order to interface it with some external DAC. Having a native I2S support allows not to have two buffers in row (Mozzi and the I2S output buffer). Also, it uses DMAs.

A few details:

• there was a IRQ conflict with the implementation of the fast ADC, I add to move the later to IRQ1 instead of IRQ0
• the buffers size are the default ones from the library, maybe it would be worth to have them editable? I am afraid that I'll led the users to input bad configurations actually.

As a draft for now, it seems to work but I want to test it a bit further.

Best,

[tfry-git]

Yeah, we really want to have this! Looks good to me on a first glance (no actual testing done). Found some minor details, and also don't forget about the documentation (readme and news).

(Also, the RP2040 is just crying for an implementation of bare chip PDM output via PIO+DMA. Just saying ;-) ).

[tomcombriat]

Hei!

Found some minor details, and also don't forget about the documentation (readme and news).

Thanks for you comments, the lack of doc and testing explain the draft state of this, I wanted to have comments before going to deep in!

I added a few comments where I would be happy to have an external point of view if you have time!

Also, the RP2040 is just crying for an implementation of bare chip PDM output via PIO+DMA. Just saying ;-) ).
By PDM do you mean PWM? Or real PDM :P ? I am not that good when it comes to DMA (I get the code when reading it, had to to find the conflict, but writing it is probably another matter ;)) I also already have my next target (StateVariable filter) in mind ;)

Edit: that being said, I see a very fresh update (three days ago) on the arduino-pico core that should allow to use the same structure for PWM output (via DMA and internal buffers), so it might be easy.

[tfry-git]

I added a few comments where I would be happy to have an external point of view if you have time!

I don't seem to see those. Am I overlooking them?

Also, the RP2040 is just crying for an implementation of bare chip PDM output via PIO+DMA. Just saying ;-) ).
By PDM do you mean PWM? Or real PDM :P ?

I mean both, actually. But in fact, I was skipping over some bits, here: The PIO mechanism does not allow enough programming to perform PD-modulation inside the state machine. However, the RP2040 certainly has enough processing power for the encoding. DMA transfer is an extra bit, really, but will certainly be useful for handling the required signal rates.

For PWM modulation, the PIO state machines should be quite capable of performing that all by itself, and using DMA would offload (essentially) all output handling to hardware.

I am not that good when it comes to DMA (I get the code when reading it, had to to find the conflict, but writing it is probably another matter ;))

Yes, there is a reason, why I did not use DMA in my initial RP2040-port...

I also already have my next target (StateVariable filter) in mind ;)

Edit: that being said, I see a very fresh update (three days ago) on the arduino-pico core that should allow to use the same structure for PWM output (via DMA and internal buffers), so it might be easy.

Ah, great, that will be a significant enhancement to the current solution in itself, then.

[tfry-git]

I added a few comments where I would be happy to have an external point of view if you have time!

I don't seem to see those. Am I overlooking them?

Oh, you probably mean these:

there was a IRQ conflict with the implementation of the fast ADC, I add to move the later to IRQ1 instead of IRQ0

Sounds reasonable to me. On a more general note, I guess we should have an option to skip over the mozziAnalogRead() setup (not just for RP2040), for conflicts such as this one, but also, because to save some resources, where analog reads are not needed.

the buffers size are the default ones from the library, maybe it would be worth to have them editable? I am afraid that I'll led the users to input bad configurations actually.

Having them configurable via a define sounds benign to me. Thinking of this, it may also be a good idea to initialize the buffer size, explicitly, in any case. The default used in the i2s implementation could possibly be subject to change, which might in turn lead to mysterious differences.

[tomcombriat]

I don't seem to see those. Am I overlooking them?

Damn, attaching a screenshot with them, if they are only displaying for me there is very little point of commenting the code :/

The principal point is that it seems that there are two different standard: I2S and Left justified. The dac I am using is of the latter type hence a few problem. I raised an issue on arduino-pico for that, maybe just fiddling with the bits is enough I have to test.

For the PWM, I think this might turn useful: earlephilhower/arduino-pico@08d37de , the PDM might be on my list in the future.

Thanks for your comments!

[tomcombriat]

@tfry-git Actually meant the ones in the screenshot above. Will also allow configuration of the buffers/DMA via define when I get back to it!

[tfry-git]

I think you need to click "Submit review" near the bottom of the page.

Ah, the PT8211 issue. I actually had my share of that in a different context (esp8266/Arduino#6940), before. I suppose, the only proper fix would have to be one inside the i2s implementation itself (and there, it would have to be a new option). -- Oh, and looking at the sources to see, where it would be done, I see it has just been added a few hours ago: setLSBJFormat().

Adding AUDIO_BIAS as a workaround is not without drawbacks: It removes the sign-bit, but at the same time, of course, it halves the available output range (and I believe, we'd also need to shift out one bit, too, to avoid overflow).

Either way, on the Mozzi-side, yes, I think we'll need an option / define for that. It should be labeled something like I2S_LSB_FORMAT, with a comment right next to it, that it is needed for PT8211. Just how that support is implemented should not be something for the user to worry about.

[tfry-git]

As for an option to invert left and right, I guess that may be (minor) overkill, as it seems easy enough to just switch left and right in hardware.

If at all, though, I'd suggest to make it a global option right next to AUDIO_CHANNELS. The implementation could simply switch l() and r() in AudioOutput.h (

Mozzi/AudioOutput.h

Line 100 in baca7e0

AudioOutputStorage_t l() const { return _l; };

).

[tomcombriat]

Hi,

I see it has just been added a few hours ago: setLSBJFormat().

Yes, we can say that this library is changing fast, my issue did not stay long in there (the PR was submitted when I was opening the source file).
This works great now for both plain I2S and LSB format.
I took your suggestions into account and added an option in the config file to switch between the two modes. I need to finish testing and documenting and will commit again soon.

[tomcombriat]

All good here!
Mono/Stereo and mozziAnalogRead() tested with a PT8211, left and right channels being as they should (another recent addition to the arduino-pico core).
As this uses some very recent changes in arduino-pico, the github version of the core is probably needed to have access to the LSBJ format used by the PT8211.

Best,

[tfry-git]

if AUDIO_BITS > 16?

[tomcombriat]

I do not think so, at least the default values in the I2S libraries are only different for 32 bits, cf line 153 of I2S.cpp:
_bufferWords = 16 * (_bps == 32 ? 2 : 1);

I prefer to stick on what is implemented there as I think he has a better understanding of the hardware behind than me, even though I agree that it would be logical to have 24 and 32 in the same batch. So let me know what you prefer ;)

[tfry-git]

I suggest to put total buffer sample size, here (i.e. 256 samples), and move the calculation what that means in terms of words to the implementation.

[tomcombriat]

I am not sure because of the previous comment actually, the case of 24bits being ill-posed. This is the closest to the library for sure but I agree that would be more understandable.

[tomcombriat]

In the end I agree. I think the case of 24 samples is a small mistake in the core I was inspired by.

[tomcombriat]

(see latest commit)

[tfry-git]

Does the i2s-library claim the hardware alarms?

[tomcombriat]

It claims IRQ0, isn't that an hardware alarm :3 ?

[tomcombriat]

I do not think so, at least the default values in the I2S libraries are only different for 32 bits, cf line 153 of I2S.cpp:
_bufferWords = 16 * (_bps == 32 ? 2 : 1);

I prefer to stick on what is implemented there as I think he has a better understanding of the hardware behind than me, even though I agree that it would be logical to have 24 and 32 in the same batch. So let me know what you prefer ;)

[tomcombriat]

I am not sure because of the previous comment actually, the case of 24bits being ill-posed. This is the closest to the library for sure but I agree that would be more understandable.

[tomcombriat]

It claims IRQ0, isn't that an hardware alarm :3 ?

[tfry-git]

It claims IRQ0, isn't that an hardware alarm :3 ?

I can see how one could read it that way, but it's not what I had meant to write. Perhaps change my "hardware alarm" to "timer interrupt" for more clarity. (Interrupts come in different flavors, and the ones used for hardware timers are not the same ones used for DMA).

That said, after thinking about this some more, I believe we can probably do better than hogging up one DMA IRQ exclusivley (out of only two that are available!). In short (I hope to get around to trying something myself in a day or two, but you may be faster):

Mozzi/MozziGuts_impl_RP2040.hpp

Line 105 in 5a4a17a

irq_set_exclusive_handler(DMA_IRQ_1, rp2040_adc_queue_handler);

Using irq_add_shared_handler() ought to be good enough. Also, while looking at it, rp2040_adc_dma_chan never seems to be initialized. That should be set to dma_chan right after

Mozzi/MozziGuts_impl_RP2040.hpp

Line 85 in 5a4a17a

uint dma_chan = dma_claim_unused_channel(true);

. It was probably "luck" that it worked in the first place, and would very likely break once the irq is shared with something else.

[tomcombriat]

I believe we can probably do better than hogging up one DMA IRQ exclusivley

Indeed, I did not see before this irq_add_shared_handler() actually used in the I2S library and naively switched the ADC to IRQ1 (I'm still beginning on this platform). But requiring an exclusive access for "only" three analog pins is probably overkill. It should also probably have a lower priority than the I2S.
I won't have time to try this today but will if you haven't done it in the meantime ;).

[tomcombriat]

Hei,
I implemented and tested these changes, works good.
As a side note, I naively at some point that IRQ0 could be shared between the I2S and the ADC, but no… I was mislead by irq_add_shared_handler() but if understand well now, that is just to shared between interrupts that have to be fired on the same events (I am obviously learning DMA logic ;) ). Please correct me if I am wrong.
So in the end, this is claiming both DMA_IRQ when using I2S, leaving nothing for the user… I think that in that regard, something in the idea of #45 would be very relevant.

[tfry-git]

I assumed the same about irq_add_shared_handler(). What symptoms do you see when you try to actually share DMA_IRQ_0?

I did not take the time to connect an I2S chip, but I compiled with EXTERNAL_DAC_VIA_I2S, changed the implementation code to DMA_IRQ_0 (lines 106 and 107), and tried the Volume_Knob example. I got good readings from mozziAnalogRead(), and there is at least something coming out of pins 20-22.

[tfry-git]

I guess we will also need to patch

 void rp2040_adc_queue_handler() {
-  dma_hw->ints0 = 1u << rp2040_adc_dma_chan;  // clear interrupt flag
+  if (!dma_channel_get_irq0_status(rp2040_adc_dma_chan)) return; // shared handler may get called on unrelated events
+  dma_channel_acknowledge_irq0(rp2040_adc_dma_chan); // clear interrupt flag

for shared irqs. That's still based on theory, alone, not testing, though.

[tomcombriat]

I assumed the same about irq_add_shared_handler(). What symptoms do you see when you try to actually share DMA_IRQ_0?

Ah, so my initial understanding was the same… When I tried it worked in a non-reliable way:

• Serial.printing the value works fine
• using the value as a gain without Serial works for a while, then, after a random amount of time (usually with the pot on low values), the audio becomes random for a while and everything stops.
• using the value as a gain and printing it on Serial did work okay (except the dropouts from the Serial).

Will try your patch and report back!

[tomcombriat]

I guess we will also need to patch

 void rp2040_adc_queue_handler() {
-  dma_hw->ints0 = 1u << rp2040_adc_dma_chan;  // clear interrupt flag
+  if (!dma_channel_get_irq0_status(rp2040_adc_dma_chan)) return; // shared handler may get called on unrelated events
+  dma_channel_acknowledge_irq0(rp2040_adc_dma_chan); // clear interrupt flag

for shared irqs. That's still based on theory, alone, not testing, though.

Good catch, works like a charm on IRQ0 now!

[tfry-git]

Great! I'll merge, then!

[tomcombriat]

The end of a long story!