Bug in Audio Library (AudioPlaySdWav) ?

[vindar]

Hi,

While investigating a bug report in my library, I think I found a small bug in the AudioPlaySdWav object of the Audio library.

Setup

• Teensy 4.1 with the Audio Shield (rev D2 in my case)
• an SD card inserted in the builtin SD slot of the T4.1 (not the SD slot of the audio shield !).
• A device connected on SPI0 (e.g. a display).

Problem/explanation

Using SPI0 can make the sound played from the builtin SD glitch. This is because the AudioPlaySdWav object calls AudioStartUsingSPI() which itself is hardcoded to SPI0 (by calling SPI.usingInterrupt()). This means that in between a `SPI.beginTransaction()` and a `SPI.endTransaction()`, the audio library "thinks" it cannot access the SD card and this causes trouble whenever the transaction is longer that the data buffered from the SD. Indeed, by commenting away the "SPI.usingInterrupt(IRQ_SOFTWARE)" lines in Audio/spi_interrupt.h the problem goes away. The solution should be to modify the AudioStartUsingSPI() function so that it always uses the correct SPI bus (and not always SPI0) but I do not know the inner working of the library well enough to suggest a fix (who else calls this function...).

Code to reproduce the bug (adapted from "Part_1_03_Playing_Music" of the Audio examples)

#include <Audio.h>
#include <Wire.h>
#include <SPI.h>
#include <SD.h>
#include <SerialFlash.h>

AudioPlaySdWav           playSdWav1;
AudioOutputI2S           i2s1;
AudioConnection          patchCord1(playSdWav1, 0, i2s1, 0);
AudioConnection          patchCord2(playSdWav1, 1, i2s1, 1);
AudioControlSGTL5000     sgtl5000_1;

// Use these with the Teensy 3.5 & 3.6 & 4.1 SD card
#define SDCARD_CS_PIN    BUILTIN_SDCARD
#define SDCARD_MOSI_PIN  11  // not actually used
#define SDCARD_SCK_PIN   13  // not actually used

void setup() {
  Serial.begin(9600);
  AudioMemory(8);
  sgtl5000_1.enable();
  sgtl5000_1.volume(0.5);
  if (!(SD.begin(SDCARD_CS_PIN))) {
    while (1) {
      Serial.println("Unable to access the SD card");
      delay(500);
    }
  }

  // we will use SPI0 which is free because the audio library will uses the SD builtin SPI bus... 
  // Normally, the audio library should never touch the SPI0 object... 
  SPI.begin(); 

  delay(1000);
}

void loop() 
  {
  if (playSdWav1.isPlaying() == false) 
    {
    Serial.println("Start playing");
    playSdWav1.play("SDTEST2.WAV");
    delay(10); // wait for library to parse WAV info
    }

  // send dummy data via SPI0
  SPI.beginTransaction(SPISettings(2000000, MSBFIRST, SPI_MODE1));    
  const int transfer_len = 2000;  /// *** Sound problems appears for larges values (e.g. > 1000)
  for(int i=0; i < transfer_len; i++)
    {
    SPI.transfer(0);
    }
  SPI.endTransaction();
}

 

[h4yn0nnym0u5e]

You make a good point. As you say, tricky to get this right - I took a quick look but a proper solution is going to take some excavation in the depths of the SD and SdFat libraries…

 

[PaulStoffregen]

Today I do not have a solution for this problem. I just want to confirm it is on my list of high priority issues to fix for 1.59 or 1.60.

 

[h4yn0nnym0u5e]

I've taken a good hard look at this and I think it's "impossible". As far as I can tell there's no way at the moment for the SD instance to reveal how .begin() was called - all the information has been abstracted away. Hence you can't tell at play() time which SPI bus (if any) to claim. It gets worse if you extend the AudioPlaySd(Wav|Raw) classes to take an optional filesystem parameter (as mooted on this thread), because then you could also have LittleFS or USBHost filesystems in play, and not the same one each time! Oh, and the SDIO hardware adds another wrinkle - it's unprotected at the moment, so barfs horribly if used from audio and user code at the same time.

The only approach I can see bearing fruit at the moment is to default to assuming SPI (for backward compatibility), but add a usesSPI(int SPInumber) method to the AudioPlaySd(Wav|Raw) classes which allows the user to change the protected SPI bus. 0, 1 up to n would protect SPI, SPI1 up to SPIn; conceptually BUILTIN_SDCARD could protect SDIO; -1 protects nothing, for filesystems that don't use SPI. I don't think SDIO caters for this at the moment, but we're no worse off. Then play(char* filename) gets optional parameters: play(char* filename, int SPInumber = -1, FS& filesystem = SD): if you nominate a filesystem you're forced also to (temporarily) nominate a SPI bus, but using -1 just uses the existing protected value.

This is just one approach, based on what I can see - quite possibly there's a better way, shout out if you think so.

I'm happy to put a bit of work into a candidate PR, if that'd be helpful.