Multiple fixes for core_esp8266_i2s

[gadgetmind]

Hardware

Hardware: Wemos D1 Mini
Core Version: 2.3.0

Description

I'm very new to ESP8266 and I've been playing with I2S output for a week or so with this project as a starting point.

https://github.com/bbx10/SFX-I2S-web-trigger

Initially it was all rather delicate with frequent crashes and audio corruption but I've now got it solid. I'd generate a pull request but TBH I'm no expert (understatement!) with these and would prefer it if others could check my changes and see how best to incorporate them.

Issues:

1. Memory leak.
   i2s_slc_begin allocates an array of buffers but i2s_slc_end didn't free them.
   I've added -
   for (x=0; x<SLC_BUF_CNT; x++) free(i2s_slc_buf_pntr[x]);
   to the end of i2s_slc_end.

2. Crashes due to ISR not being in RAM.
   i2s_slc_isr and i2s_slc_queue_next_item both need the ICACHE_RAM_ATTR attribute. Adding these fixed the instability. I think the ETS_SLC_INTR_DISABLE(); (calls ets_isr_unmask) is OK but this is all new to me.

3. Audio issues due to the way the clock is updated in i2s_set_rate.
   When changing the sample rate, the I2S output would get into a strange state with lots of audio distortion. Fixing this took hours of experimentation as I wasn't sure what they problem was. I finally found that holding the transmitter in reset during the update fixed it. So the "guts" of i2s_set_rate are now -
   // Need to hold transmitter in reset while changing clock bits as it can otherwise get into a weird state.
   // We also need to use a temporary variable to ensure we don't do a lot of read/modify/write operations.
   uint32_t i2sc_temp = I2SC;
   i2sc_temp |= (I2STXR); // Hold transmitter in reset
   I2SC = i2sc_temp;
   // Clear out and then rewrite clock bits
   i2sc_temp &= ~((I2SBDM << I2SBD) | (I2SCDM << I2SCD));
   i2sc_temp |= ((i2s_bck_div-1) << I2SBD) | ((i2s_clock_div-1) << I2SCD) ;
   I2SC = i2sc_temp;
   i2sc_temp &= ~(I2STXR); // Release reset
   I2SC = i2sc_temp;

Setting I2STXR at the same time as writing the new clock bits didn't work for me and the above sequence is the shortest that I found that's (so far!) 100% reliable.

Note that some of the bits that were configured here now aren't. I'm considering letting the user set these without editing this code so I've moved them to near the end of I2s_begin just before the call to i2s_set_rate.

// This initialisation moved here rather than in i2s_set_rate
//!trans master, !bits mod, rece slave mod, rece msb shift, right first, msb right
I2SC &= ~(I2STSM | (I2SBMM << I2SBM) | (I2SBDM << I2SBD) | (I2SCDM << I2SCD));
I2SC |= I2SRF | I2SMR | I2SRSM | I2SRMS;

I'm tempted to use a temporary here too as it will be in register and so more efficient than the read-modify-write of a volatile, but it's working for me now so I'm disinclined to touch it!

I think making changes (1) and (2) should be uncontentious but someone might want to play samples at different rates to check (3). Note that the web trigger code calls i2s_begin and i2s_end for each sample so sees the memory leak pretty quickly. I changed it to only call i2s_begin as startup and change the rate for each sample, so we seeing the audio corruption more often. With my fixes, both approaches should work just fine.

GM

[techman83]

@gadgetmind your web trigger gave me an excellent start for my own project! A pr would be super useful, as 1 has been implemented, but 2 + 3 haven't. I've made an attempt to implement those changes based on the description, but so far I get sound that is quite loud and very distorted regardless of making those changes or not.

[gadgetmind]

I suspect the sound issue is down to a misconfiguration between the I2S output and your DAC. There are a lot of things to get right! I'm happy to share my code but I haven't touched it for months! I'm also not a user of git so a proper pr would take some time.

[gadgetmind]

Oh, note that web trigger isn't mine. I used it to get going, decided I needed to get things solid before going too far, and once I did sort of lost interest in an ESP8266-based lightsaber. :-)

[techman83]

Cool, yeah if you get a chance chuck a gist up or stick it in code tags. I'm using the same adafruit one that the web trigger uses.

Ahh, of course, should have been able to tell from the url. Lightsaber sound cool! My aim is the Imperial March for my doorbell :-)

[techman83]

Turns out I misread the pin mappings and had attached LRC to the wrong pin. Still a little crackly, but not the clipping distorted mess it was before! https://www.instagram.com/p/BU37XbjFJHF/

To add to this, it actually is fine on initial boot. But after 30 seconds to a minute it'll become crackly. I really need to invest in an oscilloscope!

[gadgetmind]

Glad it's better. I found the I2S very subject to noise and I got different results based on whether powering with USB or via a PSU. I intended to try some impedance matching but never got around to it. Fix 3 above also prevented the I2S getting into a noisy mode when changing clock rate.

[techman83]

Interesting! I did a lot of messing around and no changes to the code made any difference. Oddly the changes in 3 seemed to make it crash.

You have a point on power, I decided it was good enough for the purpose and hooked it up to my door. It doesn't crackle at all now. I did read that it can draw 600 - 700mA on a bigger speaker, which will push past the 500mA non negotiated max of a USB port.

[devyte]

(1) is already implemented in latest git.
(2) is valid, the attribute is not present in the ISRs.
(3) may be valid, assuming the code proposed here is ok.

[romaca]

One of two distortion issues I have noticed I believe I have fixed - please see my post on another thread bbx10/SFX-I2S-web-trigger#3.
The second issue (LRCLK/BCLK spuriously flipping out of phase with each other) I explain in the post may be resolved by the i2s_set_rate mod above. I shall be trying it out. Thanks!

[dagnall53]

I solved my crash issues with this code (just needed an int x; as well as the line romaca added. Perhaps this could be changed at the next update?

void ICACHE_FLASH_ATTR i2s_slc_end(){
  int x;               // part of edit below
  ETS_SLC_INTR_DISABLE();
  SLCIC = 0xFFFFFFFF;
  SLCIE = 0;
  SLCTXL &= ~(SLCTXLAM << SLCTXLA); // clear TX descriptor address
  SLCRXL &= ~(SLCRXLAM << SLCRXLA); // clear RX descriptor address
                       //     https://github.com/esp8266/Arduino/issues/2590
         for (x=0; x<SLC_BUF_CNT; x++) free(i2s_slc_buf_pntr[x]);
                       // end edit
}

[d-a-v]

@dagnall53 what about making a pull-request with your changes?

[dagnall53]

This update with a small change is included already in the latest RC2 version, and will presumably be part of the next update.

[earlephilhower]

I the three issues mentioned at the start are all cleared up. (1) the mem is freed in i2s_end(), (2) ISR is in RAM, and (3) we've redone the i2s clock rate selection to be much more correct thanks to the work of @Crypter. Closing.

[geoffday67]

I've recently been using the ESP8266Audio library for I2S output and I still had this occasional distortion problem. Although the OP's suggested code doesn't apply directly to the current source, if I did as they suggested and held transmit reset while changing the rate in i2s_set_dividers I got no more distortion. So it seems like the issue does still exist. Here's the modded code I used (comments removed for brevity):

  uint32_t i2sc_temp = I2SC;
  i2sc_temp |= (I2STXR); // Hold transmitter in reset
  I2SC = i2sc_temp;

  i2sc_temp &= ~(I2STSM | I2SRSM | (I2SBMM << I2SBM) | (I2SBDM << I2SBD) | (I2SCDM << I2SCD));
  i2sc_temp |= I2SRF | I2SMR | I2SRMS | I2STMS | (div1 << I2SBD) | (div2 << I2SCD);

  I2SC = i2sc_temp;
  i2sc_temp &= ~(I2STXR); // Release reset
  I2SC = i2sc_temp;

[earlephilhower]

@geoffday67 could you submit a PR for this?