I2S output on RP2040 (earlephilhower/arduino-pico) is broken, and new features could be added

[LinusHeu]

Hi, I would still call myself kind of beginner (or intermediate) at Arduino. I've never dug this deep into any library, and this is (for) my first project using the Raspberry Pi Pico. Please take everything I write with an "if I understand correctly" flag. :D

My hardware and software Raspberry Pi Pico W earlephilhower/arduino-pico Platformio Adafruit (3006) I2S 3W Class D Amplifier Breakout - MAX98357A configured to play (Left+Right)/2

Reproduce the Issue: I'm very confused why it seems to work for #661. Or is it even working for him? No music is audible (only cracking) when AudioLogger is set to Info or Debug. I ran those examples first on Error/Warning to hear it and again with Debug to get the log. examples/examples-stream/streams-memory_raw-i2s Nothing audible. mono-AAT_original-I2S_original-Debug.log

same example but with a FormatConverterStream to convert StarWars30.h to stereo before outputting to I2S played back at 2x speed (not this libraries fault, bug in I2S) loud noise after track is played. I added digitalWrite(LED_BUILTIN, LOW); after i2s.end() in loop() but the LED stays on. And the log seems to stop abruptly. I think playback is never properly stopped. main_stereo-AAT_original-I2S_original-Debug_shortened.log ("only" first 1000 and last ~70 lines.)

examples/examples-stream/streams-generator-i2s works (Haven't verified if the frequency is truly correct, but it makes a continuous beep)

What's broken & why: I think that the big update to arduino-pico I2S changed the API, but this library wasn't updated, so compatibility is broken. For example, it looks to me like some return values have changed.

Some issues: https://github.com/pschatzmann/arduino-audio-tools/blob/fdaf00e5ffd7f24fbd75f041b19da2f149c47610/src/AudioI2S/I2SRP2040.h#L105-L108 This method already returns the amount of bytes written, now. It should be /2 for mono and not multiplied by anything for stereo. And it only works if the amount of bytes is a multiple of 4. I can't find any place where this is ensured all the time. With the FormatConverterStream or the SoundGeneratorStream it always writes multiple of 4 bytes according to the log. With the streams-memory_raw-i2s example, the third write is not a multiple of 2 (so not a multiple of 4 after the mono-stereo conversion that I2SDriverRP2040::writeBytes() does), causing write to fail. Maybe I2SDriverRP2040 should have a buffer where up to 3 bytes could be buffered between calls to size_t I2SDriverRP2040::writeBytes(const void *src, size_t size_bytes). Having int I2SDriverRP2040::availableForWrite() always return a multiple of 4 is not enough, I think, because the amount of bytes available at the data source might be fewer and not a multiple of 4.

https://github.com/pschatzmann/arduino-audio-tools/blob/fdaf00e5ffd7f24fbd75f041b19da2f149c47610/src/AudioI2S/I2SRP2040.h#L127 I2S.availableForWrite() returns amount of 32-bit words. int I2SDriverRP2040::availableForWrite() is supposed to return bytes, so it needs to be multiplied by 4 instead of /4*4.

https://github.com/pschatzmann/arduino-audio-tools/blob/fdaf00e5ffd7f24fbd75f041b19da2f149c47610/src/AudioI2S/I2SRP2040.h#L70-L72 This seems unnecessary/wrong now.

Also see: https://github.com/earlephilhower/arduino-pico/discussions/1450 Relevant documentation: https://arduino-pico.readthedocs.io/en/latest/i2s.html

The good news The update to I2S added a bunch of great new features!

• 8,16,24,32 bits per sample
• I2S Input
• Optionally swap the pins of BCLK and WCLK
• LSB-J format. Is this called I2S_LSB_FORMAT or I2S_LEFT_JUSTIFIED_FORMAT in I2SConfig?

I have already started out updating I2SRP2040.h but I ran into this weird issue: https://github.com/earlephilhower/arduino-pico/discussions/1450#discussioncomment-5932003 Will continue investigating and trying different things out in the next few days. (Haven't had time yet to try increasing buffers as earlephilhower suggested. But the speed difference is particularly weird to me.) I would appreciate any ideas, tips, etc. I will let you know if I need help with anything specific.

[pschatzmann]

I agree with your conclusions. Unfortunately I don't currently have any easy way to test this on a RP2040 as I am mostly working with ESP32 based systems. I think with the updated functionality we should also add 8bit, 24 bit and 32bit support both for output and input and we should clean up the unused protected methods.

Are you willing to help me to make things work ?

The best way to work is to make sure that the tests with a stereo and then mono signal are working properly first, before testing any other functionality.

In the Arduino Stream API availableForWrite() is always returning bytes. Are you sure that the I2S API is violating this convention ?

ps. It is normal that you don't get any proper audio in Debug mode.

[LinusHeu]

I think with the updated functionality we should also add 8bit, 24 bit and 32bit support both for output and input and we should clean up the unused protected methods.

I agree

Are you willing to help me to make things work ?

Of course! I only need (clear, cracking-free) I2S Output of files from SD-Card for my current project, so that would be my priority right now. But might as well do it right and add input while we're at it. I have a second Adafruit Amplifier (for stereo), an I2S Microphone, and an SD-breakout that I could use for testing after basic output works.

In the Arduino Stream API availableForWrite() is always returning bytes. Are you sure that the I2S API is violating this convention ?

Yes. https://github.com/earlephilhower/arduino-pico/discussions/1450#discussioncomment-5921912

https://github.com/earlephilhower/arduino-pico/blob/b57ac66815ccf5f42be20deb621044b1912e762c/libraries/I2S/src/I2S.cpp#L397-L401

https://github.com/earlephilhower/arduino-pico/blob/b57ac66815ccf5f42be20deb621044b1912e762c/libraries/AudioBufferManager/src/AudioBufferManager.cpp#L217-L233

[pschatzmann]

Just looked at the documentation. We should also call setBuffers() and extend I2SConfig to support the buffer settings not only for the esp32 and stm32. That might also help with the cracking...

[LinusHeu]

For inspiration I looked at how the other I2SDriver implementations handle mono.

https://github.com/pschatzmann/arduino-audio-tools/blob/b26f41983278a5d569fb820e81185e90c8515179/src/AudioI2S/I2SESP8266.h#L88-L103 What if I2SDriverESP8266::writeBytes(const void* src, size_t size_bytes) gets called with a size_bytes that is not a multiple of 2 while in 8 bit per sample stereo mode? Example with writeBytes(src, 3) to keep it short:

First iteration of for(): j = 0 frame[0] = data[0] 256 frame[1] = data[1] 256

Second iteration of for(): j = 2 frame[0] = data[2] 256 frame[1] = data[3] 256 <- tries to access one byte too much from src

Is this a bug? Did I miss something? Or is it ensured that writeBytes(src,size_t) (of any I2SDriver) always gets called with full L+R samples? Or with a size_bytes that's a multiple of 2? I didn't find any place where that would be ensured. And, then why does my first log from the RP2040 issue show it trying to write 49 bytes?

I'm not sure if the I2SDriverRP2040 has to handle receiving any amount of bytes. I think it does (from looking at the log and StreamCopy) but now I'm not sure.

[pschatzmann]

It is assumed that the driver can process any amount of bytes. The availableForWrite() can be used to drive the processing to only use some valid byte count. If you use the AudioCopy class the copy size is by default 1024 bytes, if it is not restricted by availableForWrite()

If only full frames are supported by writeBytes() it would be necessary just to process the supported amount of bytes and return the processed byte count as result, so that nothing gets lost.

[LinusHeu]

The availableForWrite() can be used to drive the processing to only use some valid byte count.

But this is not always guaranteed, right? If my availableForWrite() returns a multiple of 4, but only fewer bytes can be read from the source, it might still try to write a number of bytes that is not a multiple of 4. For example availableForWrite() returns 1024 but the source returns 1023 bytes.

If only full frames are supported by writeBytes() it would be necessary just to process the supported amount of bytes and return the processed byte count as result, so that nothing gets lost.

Won't size_t StreamCopyT<T>::write(size_t len, size_t &delayCount ) just try again and again until it fails? Because of that, I thought buffering invalid byte amounts would be better. For example

1. writeBytes(src, 15): Return 15. Actually write 12 bytes to I2S and 3 to buffer
2. writeBytes(src, 21): Return 21. Actually write the 3 buffered bytes and 22 bytes from src to I2S

The I2S Library does something a bit similar when using the size_t I2S::write(int8_t s) methods.

[pschatzmann]

No, writeBytes(src, 15) would need to return 12 if you can only process full frames! So that the user knows that he has to resend the last 3 unprocessed bytes. To avoid this complexity, the AudioCopy is automatially taking care of this.

[LinusHeu]

That makes the RP2040Driver easier! :D

But I'm still not sure if I understand correctly. Just re-sending the unprocessed bytes would not be enough. They would need to be sent together with the next bunch of bytes.

1. writeBytes(src,15): Write 12 to I2S and Return 12.
2. writeBytes(src,3): would just return and write 0 because that's not a multiple of 4.

Instead, the user/StreamCopy would have to do this:

1. writeBytes(src,15) return 12 user stores those 3 bytes somewhere
2. writeBytes(src{3 unprocessed frames from before + new data}, 24)

Do I understand that correctly?

I can't find where StreamCopy does that right now, but maybe I'm just looking at the wrong place.

[pschatzmann]

Yes, that is what's happening. The sender will just need to send the next 15 bytes of unprocessed data, assuming that you consistently write 15 bytes...

[LinusHeu]

Thank you so much for your help.

https://github.com/LinusHeu/arduino-audio-tools/blob/develop/src/AudioI2S/I2SRP2040.h

I have a first version that I think should work, there are still todos, questions and some things I would to change. I wanted to just make a pull request once I had it definitely working but I need some more help.

Mono=>I2S as well as Mono=>FormatconverterStream-to-stereo=>I2S at16bit per sample plays at (I think) the correct speed and you definitely hear what song its supposed to be. But I can't get rid of the cracking.

Even with I2S_BUFFER_SIZE 1024 I2S_BUFFER_COUNT 40 arduino-pico I2S uses BUFFER_SIZE = amount of uint32_t, so 80 kilobyte of buffer seems kind of excessive to me.

Generally, it seems like increasing the I2S_Buffer increases the time between cracks. (Still always multiple per second.)

Additionally, some combinations of I2S_BUFFER_COUNT and I2S_BUFFER_SIZE cause copy to fail. Haven't really looked into that yet. I2S_BUFFER_SIZE 1024 I2S_BUFFER_COUNT 20 => works but cracking I2S_BUFFER_COUNT 5 or 10 => [E] StreamCopy.h : 289 - write to target has failed! (1024 bytes) device-monitor-230525-152534_bufferCount5.log Buffer should have been half full at the time the write failed.

I2S_BUFFER_SIZE 16 I2S_BUFFER_COUNT 8 => works, just huge amount of cracking because the buffer is definitely way too small.

I don't really have any good ideas right now. Maybe I made a mistake somewhere in I2SDriverRP2040? If the RP2040 is fast enough for decoding mp3, just filling the buffers with raw data should be absolutely no problem, right? Maybe there is some performance bottleneck on the RP2040? But I have no clue where/what that could be. The audio-buffer manager of arduino-pico has a method to check if an underflow occured. I think I will try that next to see if there actually are underflows or if something else causes the cracking. Or I could continue trying out more buffer sizes and counts. Could too large/too many buffers be a problem? Do you have any other ideas?

A big issue I have is that I have almost zero confirmed working code.

My main test sketch:

#include "AudioTools.h"
#include "StarWars30.h"
AudioInfo info(22050, 1, 16);
I2SStream i2s;  // Output to I2S
MemoryStream music(StarWars30_raw, StarWars30_raw_len);
StreamCopy copier(i2s, music); // copies sound into i2s
void setup(){
  pinMode(LED_BUILTIN, OUTPUT);
  digitalWrite(LED_BUILTIN,LOW);
    Serial.begin(115200);
    while(!Serial);
    delay(5000);
    digitalWrite(LED_BUILTIN,HIGH);

    AudioLogger::instance().begin(Serial, AudioLogger::Error);

    auto config = i2s.defaultConfig(TX_MODE);
    config.copyFrom(info);
    if(!i2s.begin(config)){
      while(1){
        Serial.println("FAIL");
      }
    }

}
void loop(){
    if (!copier.copy()){
      i2s.end();
      digitalWrite(LED_BUILTIN, LOW);
      stop();
    }
}

[LinusHeu]

I couldn't get any completely cracking-free audio with the ESP8266Audio library either. (Before I found this library.) https://github.com/earlephilhower/ESP8266Audio/discussions/603

Buffer size 2048 and count 5 was almost good with ESP8266Audio. But with Arduino-Audio-Tools and my current I2SDriverRP2040 that causes the "write to target has failed!" error as well.

Because both of these audio libraries have cracking, I'm not completely sure if arduino-pico/I2S isn't the issue. However, I tested: Playing StarWars.h directly to I2S without using any Arduino-audio-tools: Works with the default buffer count (8) and size (16) of arduino-pico/I2S. Just at 2x speed because Starwars30.h is mono and I didn't do any mono->stereo conversion for that test. libraries/I2S/examples/SimpleTone/SimpleTone.ino from arduino-pico: Works perfectly.

[pschatzmann]

I suggest that you commit your changes to your fork, so that I can review your current status and maybe do some tests myself

[LinusHeu]

https://github.com/LinusHeu/arduino-audio-tools/tree/develop This is my current version

[LinusHeu]

Tiny update: This minimal sketch, which uses no arduino-audio-tools, produces clear sound with zero cracking.

#include <Arduino.h>
#include <I2S.h>
#include <StarWars30.h>
I2S philhowerI2S = I2S(OUTPUT);

const unsigned char *ptr = StarWars30_raw;
size_t toPlay = StarWars30_raw_len;
long sampleRate = 22050;

void setup() {
  pinMode(LED_BUILTIN,OUTPUT);
  digitalWrite(LED_BUILTIN,LOW);
  philhowerI2S.setBCLK(26); //WS = 27
  philhowerI2S.setDATA(28);
  philhowerI2S.setBitsPerSample(16);
  if(philhowerI2S.begin(sampleRate)){
    digitalWrite(LED_BUILTIN,HIGH);
  }
}
void loop() {
  while(toPlay){
    int16_t frame[2];
    frame[0] = *(int16_t*)ptr;
    frame[1] = *(int16_t*)ptr;

    while(!philhowerI2S.write((uint8_t*)frame,4)){
      //putting a delay(1) here causes cracking
    }
    //philhowerI2S.write(*(int32_t*)frame,true);//both of these ways to write the audio data work

    toPlay -= 2;
    ptr += 2;
  }
  digitalWrite(LED_BUILTIN,LOW);
}

[LinusHeu]

I just got cracking-free playback! 🥳

The "solution"/workaround doesn't seem that great, because I don't think it really fits the conventions of Arduino streams or your library. There could possibly be an underlying bug, that should get fixed instead of using a workaround.

Commit "works hopefully": Cracking no matter how much I crank the buffers. As described in my message 5 days ago. For now, I'm only testing without formatConverterStream. Log: device-monitor-230530-205606--commit-works_hopefully.log

Log Excerpt

``` [D] StreamCopy.h : 97 - size_t audio_tools::StreamCopyT::copy() [with T = unsigned char; size_t = unsigned int] [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [I] StreamCopy.h : 295 - try write - 2 (open 1024 bytes) [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [I] StreamCopy.h : 295 - try write - 3 (open 1024 bytes) [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [I] StreamCopy.h : 295 - try write - 4 (open 1024 bytes) [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [I] StreamCopy.h : 295 - try write - 5 (open 1024 bytes) [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [I] StreamCopy.h : 295 - try write - 6 (open 1024 bytes) [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [I] StreamCopy.h : 295 - try write - 7 (open 1024 bytes) [D] I2SStream.h : 92 - I2SStream::write: 1024 [D] I2SRP2040.h : 123 - size_t audio_tools::I2SDriverRP2040::writeBytes(const void*, size_t) [I] StreamCopy.h : 295 - try write - 8 (open 0 bytes) [I] StreamCopy.h : 139 - StreamCopy::copy 1024 -> 1024 -> 1024 bytes - in 8 hops ```

I noticed this weird behaviour in the log. It would often need multiple attempts to write 1024 bytes. It writes 0 bytes and then writes all 1024 at once in the (mostly) 7 to 8th attempt.

Possible explanation / wild guess: StreamCopy thinks it can write 1024 bytes, when actually 0 bytes can be written. Then, at the 7-8th attempt, one buffer is completely played and can get filled. Possibly something is wrong with my I2SDriverRP2040::availableForWrite() or with I2S::availableForWrite(). However, I don't see any reason why this would cause cracking. So this is explanation is just a guess. Maybe it has something to do with //putting a delay(1) here causes cracking) in the sketch in my previous message. In that sketch with bare arduino-pico adding a delay(1) after a write failed (because the buffer was full) causes cracking. And in StreamCopy::write() there is a delay(5) after each failed write. Again, I absolutely don't understand why that would cause cracking because the buffers should be (almost) full at that point. (And 20 buffers with 1024*4 bytes each should be large enough to handle 1ms delay.)

Workaround: Make I2SDriverRP2040::writeBytes() blocking. To test this workaround, I only made it blocking when mono 16bps audio is played. Because that's what I'm testing with right now (StarWars.h) and the change was quick. It still may not always write everything, as it still only writes a multiple of 4 bytes. Audio-Quality: Great. Noticed zero cracking. Log: device-monitor-230530-211114--try-make-i2srp2040-writeexpandchannel-blocking.log

I will try to investigate further.

[LinusHeu]

I can reliably reproduce 2 issues using only arduino-pico/I2S:

1. cracking and noise (even though no underflow occurs) when there is a delay() anywhere. Even if there is only a delay when all buffers are completely full.
2. very slight and infrequent popping when running I2S on core0. On core1, it sounds perfect. (As long as there is no delay() or similar function on core1.)

https://github.com/earlephilhower/arduino-pico/discussions/1491

I swear, I WILL finish this. But I hope it's alright, that this is taking so long. :D

[LinusHeu]

In my opinion, I2S output is done. 🥳 🎵 Unless you think the issues below are actually caused by I2SRP2040. If you have any ideas for improvements, notice bugs, etc, feel free to correct me. I might need some time until I get to I2S input, so I think merging output now would be good. Hopefully other people can test it as well. Unless someone else does it first, I will add input later. During testing, I noticed a few issues, but I think they have nothing to do with I2SRP2040 in particular.

Testing I did: Hardware: Pico W, Adafruit MAX98357A Software: Platformio

1. examples/examples-stream/streams-memory_raw-i2s works great

2. examples/examples-stream/streams-generator-i2s works great

3. examples/examples-stream/streams-memory_mp3_short-i2s/streams-memory_mp3_short-i2s.ino cracking. I think from the mp3 decoder or too small buffers.

4. examples/examples-stream/streams-generator-i2s but with 8 or 32 bits per sample completely silent.

5. examples/examples-stream/streams-memory_raw-i2s but with a FormatConverterStream to stereo works but crashes at the end of the track.

6. examples/examples-stream/streams-memory_raw-i2s but with a FormatConverterStream to 32 or 24 bit per sample works but crashes at the end of the track. Based on how often [I] StreamCopy.h : 139 - StreamCopy::copy 1024 -> 1024 -> 1024 bytes - in 1 hops is in the log, I think it crashes when trying to read the last 320 bytes. I bet it's the assert in here: https://github.com/pschatzmann/arduino-audio-tools/blob/59e204d460d4cc3ccfe83872147a879f8981db17/src/AudioTools/ResampleStream.h#L38-L57

My main.cpp code for 4:

```ino //examples/examples-stream/streams-generator-i2s/streams-generator-i2s.ino /** * @file streams-generator-i2s.ino * @author Phil Schatzmann * @brief see https://github.com/pschatzmann/arduino-audio-tools/blob/main/examples/examples-stream/streams-generator-i2s/README.md * @copyright GPLv3 */ #include "AudioTools.h" uint16_t sample_rate=44100; uint8_t channels = 2; // The stream will have 2 channels SineWaveGenerator sineWave(32000); // subclass of SoundGenerator with max amplitude of 32000 GeneratedSoundStream sound(sineWave); // Stream generated from sine wave I2SStream out; StreamCopy copier(out, sound); // copies sound into i2s // Arduino Setup void setup(void) { // Open Serial Serial1.begin(115200); //while(!Serial); delay(1000); AudioLogger::instance().begin(Serial1, AudioLogger::Error); // start I2S Serial1.println("starting I2S..."); auto config = out.defaultConfig(TX_MODE); config.sample_rate = sample_rate; config.channels = channels; config.bits_per_sample = 32; out.begin(config); // Setup sine wave sineWave.begin(channels, sample_rate, N_B4); Serial1.println("started..."); } // Arduino loop - copy sound to out void loop() { copier.copy(); } ```

main and log for 5 and 6

```ino #include #include "StarWars30.h" AudioInfo infoIn(22050, 1, 16); AudioInfo infoOut(22050,1,24); I2SStream i2s; // Output to I2S MemoryStream music(StarWars30_raw, StarWars30_raw_len); FormatConverterStream converter(music); StreamCopy copier(i2s, converter); // copies sound into i2s void setup(){ pinMode(LED_BUILTIN, OUTPUT); digitalWrite(LED_BUILTIN,LOW); Serial.begin(115200); while(!Serial); delay(5000); digitalWrite(LED_BUILTIN,HIGH); AudioLogger::instance().begin(Serial, AudioLogger::Error); converter.begin(infoIn,infoOut); auto config = i2s.defaultConfig(TX_MODE); config.copyFrom(infoOut); if(!i2s.begin(config)){ while(1){ } } } void loop(){ if (!copier.copy()){ i2s.end(); digitalWrite(LED_BUILTIN, LOW);//this LED never turns off stop(); } } ``` [device-monitor-230604-131901_CrashAtEndOfTrack_formatconverter-main_stereo_Shortened.log](https://github.com/pschatzmann/arduino-audio-tools/files/11645581/device-monitor-230604-131901_CrashAtEndOfTrack_formatconverter-main_stereo_Shortened.log)

P.S: Sorry, I haven't really used git before so I don't know how I could fix or prevent the merge conflict of the pull request.

[pschatzmann]

Today I took some time to have a closer look and do some extensive testing:

• pt 3: was actually coming from the writeExpandChannel(). I changed the implementation to use blocking writes and the issue has gone. Initially I thought that it was an issue with the helix decoder, but MAD was giving exactly the same issue.
• pt 4: the 32 bit example was working w/o issue, just remove the max amplitude value. 8 bits seems to not work in any case, but I am not aware of any i2s api that would support 8 bits. So this is acceptable
• pt 5&6 were working w/o issues

I was also restoring the rp2040 mbed implementation that was broken by this change.

I plan to have a look at the reading side next

[pschatzmann]

Reading support has been added

[LinusHeu]

@pschatzmann Thank you very much :D I got kinda busy with other things... Among others, there was another I2S bug in the Arduino-Pico core that I fixed. Now it should work well and reliably.

Yes, 5 and 6 were #950 #946