Allow specifying I2S buffer sizes and counts #3790
Conversation
Add a new i2s_begin_custom() signature that takes a buffer count and buffer size (in samples). This allows for specifying a larger or smaller amount of DMA data which can help cover periods when the ESP's internal WiFi processing takes longer than the few milliseconds of data currently allocated (i.e at 44.1KHZ, the original buffers would last for 512 samples which works out to only 11.6 milliseconds). This also frees up ~76 bytes of RAM in all applications that do not use I2S since it dynamically allocates pointer/counter arrays and only has a single static pointer defined.
| i2s_slc_queue_len = 0; | ||
| int x, y; | ||
|
|
||
| i2s_slc_queue = (uint32_t*)malloc((SLC_BUF_CNT-1) * sizeof(uint32_t)); |
There was a problem hiding this comment.
Is it correct that this malloc is 1 item smaller than the others? I realize this is how it is currently implemented, but I have to wonder.
There was a problem hiding this comment.
Before the malloc is done, the pointer should be checked against null, and free()d if not.
There was a problem hiding this comment.
SIze is -1 by design (see below, the original code only alloc'd buf_cnt-1). Good catch on 2nd comment. It's safe to free(NULL), so I'll always free before allocing.
However, in the case they are not free'd already that means we potentially have DMA running and the ISR hooked up. There's a race condition then where the DMA could touch the free'd buffer or the ISR could work with these arrays, only to find we'd NULL'd them. Bad stuff.
I'll do an i2s_end() which completely resets the i2s subsystem, clearing everything and stopping any dma, right at the beginning. In this case there will be no frees in begin(), as they're guaranteed free'd in the i2s_end().
There was a problem hiding this comment.
You probably don't want to free and malloc again if the call is done with the same size arg.
| i2s_slc_queue_len = 0; | ||
| int x, y; | ||
|
|
||
| i2s_slc_queue = (uint32_t*)malloc((SLC_BUF_CNT-1) * sizeof(uint32_t)); |
There was a problem hiding this comment.
Before the malloc is done, the pointer should be checked against null, and free()d if not.
cores/esp8266/core_esp8266_i2s.c
Outdated
| @@ -50,10 +53,10 @@ struct slc_queue_item { | |||
| uint32 next_link_ptr; | |||
| }; | |||
|
|
|||
| static uint32_t i2s_slc_queue[SLC_BUF_CNT-1]; | |||
| static uint32_t *i2s_slc_queue; | |||
There was a problem hiding this comment.
These ptrs should be inited to null.
There was a problem hiding this comment.
They are, actually. In ANSI-C, static variables are set to all 0 and live in BSS, even if you don't explicitly list it. I'm happy to add in the assignment, of course, to make it absolutely clear, of course.
cores/esp8266/core_esp8266_i2s.c
Outdated
| @@ -148,8 +157,14 @@ void ICACHE_FLASH_ATTR i2s_slc_end(){ | |||
| SLCRXL &= ~(SLCRXLAM << SLCRXLA); // clear RX descriptor address | |||
|
|
|||
| for (int x = 0; x<SLC_BUF_CNT; x++) { | |||
| free(i2s_slc_buf_pntr[x]); | |||
| free(i2s_slc_buf_pntr[x]); | |||
There was a problem hiding this comment.
The ptrs here should be nulled after free.
There was a problem hiding this comment.
Again, good point, will do!
There was a problem hiding this comment.
Actually, scratch that. i2s_slc_buf_pntr[] is a dynamically allocated array of pointers. I free each pointer i2s_slc_buf_pntr[0...n], then completely free and null i2s_slc_buf_pntr. So there is no way to get to those old pointers, ever, so no need to NULL.
There was a problem hiding this comment.
What I am guarding against is a second call to begin() without a call to end(), I.e. make sure no leaks. This seems to be common usage in ArduinoLand.
There was a problem hiding this comment.
Arduinoland is a dangerous place to live...
It'll still (randomly, making it even more fun) crash w/o the changes I mentioned on the i2s_begin() call due to ISR and DMA working w/the old addresses.
If the user calls i2s_begin() while DMA is already running, the ISR or DMA engine could fire and use the old addresses after they're freed or even while we're working on them. Avoid this by forcing a i2s_stop() before doing any work. These changes effectively allow is2_begin() and i2s_end() to be called any number of times, in any order, without crashing.
|
I'm going to punt on this as it looks like I2S may be undergoing a larger change related to supporting input. It's small enough to regenerate in the future if there is interest. |
I've been doing a lot of audio work but am finding that sometimes the ESP8266 internal WiFi code can go off and do something for quite a long while (relative to audio delays, that is) causing the internal DMA buffers to empty and hiccups in the output to occur.
The simplest way to avoid this is to increase the size and number of RAM buffers we can pre-fill to cover these WiFi stack times. Granted, with only ~40K available you're not going to add seconds worth of buffering, but every little bit can help.
Here's a backwards compatible patch to allow an app to specify a larger buffer size/count if necessary, while preserving the original behavior (and saving a few bytes in everyone else's RAM).
---snip---
Add a new i2s_begin_custom() signature that takes a buffer count and buffer
size (in samples). This allows for specifying a larger or smaller amount
of DMA data which can help cover periods when the ESP's internal WiFi
processing takes longer than the few milliseconds of data currently
allocated (i.e at 44.1KHZ, the original buffers would last for 512 samples
which works out to only 11.6 milliseconds).
This also frees up ~76 bytes of RAM in all applications that do not use
I2S since it dynamically allocates pointer/counter arrays and only has a
single static pointer defined.