I2S driver fixes for IRQs, protocol, factoring #4574
Conversation
|
I've just got a USB 8-line logic analyzer and will try and capture the fixed I2S waveform tonight to verify the I2S protocol fix. Just comparing the output using the simple playback example in the issue sounds much better (since it is sending out full-swing samples it was clipping something fierce before), but if at all possible I'd like to catch the entire waveform since there's not really documentation on these registers. |
|
The change looks good. Here is the I2S sending 32767 (0x7FFF). The add'l bit shift is clearly visible and correct: |
|
For comparison, the wrong, unshifted version that's in the repo now:
|
All redundant ICACHE_FLASH_ATTR decorators were removed, we already do this by default for all routines, anyway, The actual ISR and its called function moved to to IRAM. Used to be in flash due to the decorator, which could lead to crashes. Use ets_memset to mute buffers in ISR. Fix the I2S on-the-wire protocol by enabling the transmit delay I2STMS because I2S is supposed to send the MSB one clock after LRCLK toggles. This was causing I2S to be twice as loud as intended in the best of cases, and causing garbage/noise output when the MSB was set since data was effectively shifted. Refactor the clock divider setting to be done in one function only, as there is no reason to do the same complicated bit setting in two spots.
earlephilhower
force-pushed
the
i2s_bitdelay_fix
branch
from
78eae1d to
bccad88
Compare
|
My one comment is similar as I mentioned before: could you add comments for the register definitions/operations? At least for any changes/additions. If you're not sure, then suspicions are ok, as long as you explain that. Suspicions on how/why something works is better than no explanation at all! |
Comment the known and unknown I2S register settings for posterity, using the ESP32 guide as a basis. Use optimistic_yield() instead of esp_wdt_disable/enable when busy waiting in blocking writes to ensure we don't hog the CPU completely. Move the constant IO pins to #defines for easier understanding.
…Arduino into i2s_bitdelay_fix
|
I've added what I can glean for the I2S block. Looks very close to the ESP32 so it's not so bad. SLC (DMA) is not so simple, but this patch isn't touching that. |
|
Hello! Temporary fix: change line 474 in https://github.com/esp8266/Arduino/blob/master/cores/esp8266/core_esp8266_i2s.cpp#L474 to: |
All redundant ICACHE_FLASH_ATTR decorators were removed, we already do
this by default for all routines, anyway,
The actual ISR and its called function moved to to IRAM. Used to be in flash
due to the decorator, which could lead to crashes.
Fix the I2S on-the-wire protocol by enabling the transmit delay I2STMS because
I2S is supposed to send the MSB one clock after LRCLK toggles. This was
causing I2S to be twice as loud as intended in the best of cases, and causing
garbage/noise output when the MSB was set since data was effectively shifted.
Refactor the clock divider setting to be done in one function only, as there
is no reason to do the same complicated bit setting in two spots.
Fixes #4571 and a few other things I've been noticing.