ST77xx memory corruption + mp3 sync using library + mutex for poll in stream() - release

Not accessible from userland...

README.md

@@ -257,7 +257,6 @@ Follow the instructions from https://docs.espressif.com/projects/esp-idf/en/v4.0
 If you want to use a more recent version of gcc and IDF (4.0 stable), move to cmake-master branch</strong>
 
 You can install IDF manually on Linux or Windows (using the Subsystem for Linux) following the instructions at: https://www.instructables.com/id/ESP32-Development-on-Windows-Subsystem-for-Linux/
-And then copying the i2s.c patch file from this repo over to the esp-idf folder
 You also need to use esp-dsp recent version or at least make sure you have this patch https://github.com/espressif/esp-dsp/pull/12/commits/8b082c1071497d49346ee6ed55351470c1cb4264
 >>>>>>> refs/remotes/origin/master
 

components/display/ST77xx.c

@@ -90,7 +90,7 @@ static void Update16( struct GDS_Device* Device ) {
 			for (int i = FirstRow; i <= LastRow; i++) {
 				memcpy(optr, Private->Shadowbuffer + (i * Device->Width + FirstCol) * 2, ChunkSize);
 				optr += ChunkSize;
-				if (optr - Private->iRAM < PAGE_BLOCK && i < LastRow) continue;
+				if (optr - Private->iRAM <= (PAGE_BLOCK - ChunkSize) && i < LastRow) continue;
 				Device->WriteData(Device, Private->iRAM, optr - Private->iRAM);
 				optr = Private->iRAM;
 			}
@@ -157,7 +157,7 @@ static void Update24( struct GDS_Device* Device ) {
 			for (int i = FirstRow; i <= LastRow; i++) {
 				memcpy(optr, Private->Shadowbuffer + (i * Device->Width + FirstCol) * 3, ChunkSize);
 				optr += ChunkSize;
-				if (optr - Private->iRAM < PAGE_BLOCK && i < LastRow) continue;
+				if (optr - Private->iRAM <= (PAGE_BLOCK - ChunkSize) && i < LastRow) continue;
 				Device->WriteData(Device, Private->iRAM, optr - Private->iRAM);
 				optr = Private->iRAM;
 			}	

components/services/CMakeLists.txt

@@ -1,6 +1,6 @@
 idf_component_register(SRC_DIRS .
-						INCLUDE_DIRS . 
-						REQUIRES json tools platform_config display
+						INCLUDE_DIRS . ${IDF_PATH}/components/driver
+						REQUIRES json tools platform_config display 
 
 )
 

components/services/i2s.c

@@ -28,7 +28,7 @@
 #include "driver/i2s.h"
 #include "driver/rtc_io.h"
 #include "driver/dac.h"
-#include "adc1_i2s_private.h"
+#include <adc1_i2s_private.h>
 
 #include "esp_intr_alloc.h"
 #include "esp_err.h"
@@ -96,7 +96,8 @@ typedef struct {
 } i2s_obj_t;
 
 static i2s_obj_t *p_i2s_obj[I2S_NUM_MAX] = {0};
-static i2s_dev_t* I2S[I2S_NUM_MAX] = {&I2S0, &I2S1};
+/* DRAM_ATTR is required to avoid I2S array placed in flash, due to accessed from ISR */
+static DRAM_ATTR i2s_dev_t* I2S[I2S_NUM_MAX] = {&I2S0, &I2S1};
 static portMUX_TYPE i2s_spinlock[I2S_NUM_MAX] = {portMUX_INITIALIZER_UNLOCKED, portMUX_INITIALIZER_UNLOCKED};
 static int _i2s_adc_unit = -1;
 static int _i2s_adc_channel = -1;
@@ -240,14 +241,14 @@ static float i2s_apll_get_fi2s(int bits_per_sample, int sdm0, int sdm1, int sdm2
  *
  * @return     ESP_ERR_INVALID_ARG or ESP_OK
  */
-static esp_err_t i2s_apll_calculate_fi2s(int rate, int bits_per_sample, int *sdm0, int *sdm1, int *sdm2, int *odir) 
+static esp_err_t i2s_apll_calculate_fi2s(int rate, int bits_per_sample, int *sdm0, int *sdm1, int *sdm2, int *odir)
  {
  	int _odir, _sdm0, _sdm1, _sdm2;
 	float r = rtc_clk_xtal_freq_get() * 1000000. / (rate * 2 * 2);
 	int _sdm2_max;
 	uint32_t prec = -1;
 	int o, s1, s0;
-	
+
 	if (rate/bits_per_sample/2/8 < APLL_I2S_MIN_RATE) {
         return ESP_ERR_INVALID_ARG;
     }
@@ -291,80 +292,11 @@ static esp_err_t i2s_apll_calculate_fi2s(int rate, int bits_per_sample, int *sdm
 			}
 		}
 	}
-	
+
 	if (*sdm2 + *sdm0 + *sdm0 + *odir) return ESP_OK;
 	else return ESP_ERR_INVALID_ARG;
- }	
+ }
 
-#if 0
-static esp_err_t i2s_apll_calculate_fi2s(int rate, int bits_per_sample, int *sdm0, int *sdm1, int *sdm2, int *odir)
-{
-    int _odir, _sdm0, _sdm1, _sdm2;
-    float avg;
-    float min_rate, max_rate, min_diff;
-    if (rate/bits_per_sample/2/8 < APLL_I2S_MIN_RATE) {
-        return ESP_ERR_INVALID_ARG;
-    }
-
-    *sdm0 = 0;
-    *sdm1 = 0;
-    *sdm2 = 0;
-    *odir = 0;
-    min_diff = APLL_MAX_FREQ;
-
-    for (_sdm2 = 4; _sdm2 < 9; _sdm2 ++) {
-        max_rate = i2s_apll_get_fi2s(bits_per_sample, 255, 255, _sdm2, 0);
-        min_rate = i2s_apll_get_fi2s(bits_per_sample, 0, 0, _sdm2, 31);
-        avg = (max_rate + min_rate)/2;
-        if (abs(avg - rate) < min_diff) {
-            min_diff = abs(avg - rate);
-            *sdm2 = _sdm2;
-        }
-    }
-    min_diff = APLL_MAX_FREQ;
-    for (_odir = 0; _odir < 32; _odir ++) {
-        max_rate = i2s_apll_get_fi2s(bits_per_sample, 255, 255, *sdm2, _odir);
-        min_rate = i2s_apll_get_fi2s(bits_per_sample, 0, 0, *sdm2, _odir);
-        avg = (max_rate + min_rate)/2;
-        if (abs(avg - rate) < min_diff) {
-            min_diff = abs(avg - rate);
-            *odir = _odir;
-        }
-    }
-    min_diff = APLL_MAX_FREQ;
-    for (_sdm2 = 4; _sdm2 < 9; _sdm2 ++) {
-        max_rate = i2s_apll_get_fi2s(bits_per_sample, 255, 255, _sdm2, *odir);
-        min_rate = i2s_apll_get_fi2s(bits_per_sample, 0, 0, _sdm2, *odir);
-        avg = (max_rate + min_rate)/2;
-        if (abs(avg - rate) < min_diff) {
-            min_diff = abs(avg - rate);
-            *sdm2 = _sdm2;
-        }
-    }
-
-    min_diff = APLL_MAX_FREQ;
-    for (_sdm1 = 0; _sdm1 < 256; _sdm1 ++) {
-        max_rate = i2s_apll_get_fi2s(bits_per_sample, 255, _sdm1, *sdm2, *odir);
-        min_rate = i2s_apll_get_fi2s(bits_per_sample, 0, _sdm1, *sdm2, *odir);
-        avg = (max_rate + min_rate)/2;
-        if (abs(avg - rate) < min_diff) {
-            min_diff = abs(avg - rate);
-            *sdm1 = _sdm1;
-        }
-    }
-
-    min_diff = APLL_MAX_FREQ;
-    for (_sdm0 = 0; _sdm0 < 256; _sdm0 ++) {
-        avg = i2s_apll_get_fi2s(bits_per_sample, _sdm0, *sdm1, *sdm2, *odir);
-        if (abs(avg - rate) < min_diff) {
-            min_diff = abs(avg - rate);
-            *sdm0 = _sdm0;
-        }
-    }
-
-    return ESP_OK;
-}
-#endif
 esp_err_t i2s_set_clk(i2s_port_t i2s_num, uint32_t rate, i2s_bits_per_sample_t bits, i2s_channel_t ch)
 {
     int factor = (256%bits)? 384 : 256; // According to hardware codec requirement(supported 256fs or 384fs)
@@ -797,7 +729,7 @@ esp_err_t i2s_set_dac_mode(i2s_dac_mode_t dac_mode)
     return ESP_OK;
 }
 
-static esp_err_t _i2s_adc_mode_recover(void)
+static esp_err_t _i2s_adc_mode_recover()
 {
     I2S_CHECK(((_i2s_adc_unit != -1) && (_i2s_adc_channel != -1)), "i2s ADC recover error, not initialized...", ESP_ERR_INVALID_ARG);
     return adc_i2s_mode_init(_i2s_adc_unit, _i2s_adc_channel);
@@ -943,12 +875,6 @@ static esp_err_t i2s_param_config(i2s_port_t i2s_num, const i2s_config_t *i2s_co
     I2S_CHECK(!((i2s_config->mode & I2S_MODE_DAC_BUILT_IN) && (i2s_num != I2S_NUM_0)), "I2S DAC built-in only support on I2S0", ESP_ERR_INVALID_ARG);
     I2S_CHECK(!((i2s_config->mode & I2S_MODE_PDM) && (i2s_num != I2S_NUM_0)), "I2S DAC PDM only support on I2S0", ESP_ERR_INVALID_ARG);
 
-    if (i2s_num == I2S_NUM_1) {
-        periph_module_enable(PERIPH_I2S1_MODULE);
-    } else {
-        periph_module_enable(PERIPH_I2S0_MODULE);
-    }
-
     if(i2s_config->mode & I2S_MODE_ADC_BUILT_IN) {
         //in ADC built-in mode, we need to call i2s_set_adc_mode to
         //initialize the specific ADC channel.
@@ -1001,6 +927,7 @@ static esp_err_t i2s_param_config(i2s_port_t i2s_num, const i2s_config_t *i2s_co
     I2S[i2s_num]->conf.rx_start = 0;
 
     if (i2s_config->mode & I2S_MODE_TX) {
+		// PATCH
         I2S[i2s_num]->conf.tx_msb_right = 1;
         I2S[i2s_num]->conf.tx_right_first = 0;
 
@@ -1013,7 +940,8 @@ static esp_err_t i2s_param_config(i2s_port_t i2s_num, const i2s_config_t *i2s_co
     }
 
     if (i2s_config->mode & I2S_MODE_RX) {
-        I2S[i2s_num]->conf.rx_msb_right = 1;
+		// PATCH
+        I2S[i2s_num]->conf.rx_msb_right = 1;	
         I2S[i2s_num]->conf.rx_right_first = 0;
         I2S[i2s_num]->conf.rx_slave_mod = 0; // Master
         I2S[i2s_num]->fifo_conf.rx_fifo_mod_force_en = 1;
@@ -1156,8 +1084,10 @@ esp_err_t i2s_driver_install(i2s_port_t i2s_num, const i2s_config_t *i2s_config,
 
         //To make sure hardware is enabled before any hardware register operations.
         if (i2s_num == I2S_NUM_1) {
+            periph_module_reset(PERIPH_I2S1_MODULE);
             periph_module_enable(PERIPH_I2S1_MODULE);
         } else {
+            periph_module_reset(PERIPH_I2S0_MODULE);
             periph_module_enable(PERIPH_I2S0_MODULE);
         }
 
@@ -1241,18 +1171,6 @@ esp_err_t i2s_driver_uninstall(i2s_port_t i2s_num)
     return ESP_OK;
 }
 
-int i2s_write_bytes(i2s_port_t i2s_num, const void *src, size_t size, TickType_t ticks_to_wait)
-{
-    size_t bytes_written = 0;
-    int res = 0;
-    res = i2s_write(i2s_num, src, size, &bytes_written, ticks_to_wait);
-    if (res != ESP_OK) {
-        return ESP_FAIL;
-    } else {
-        return bytes_written;
-    }
-}
-
 esp_err_t i2s_write(i2s_port_t i2s_num, const void *src, size_t size, size_t *bytes_written, TickType_t ticks_to_wait)
 {
     char *data_ptr, *src_byte;
@@ -1378,18 +1296,6 @@ esp_err_t i2s_write_expand(i2s_port_t i2s_num, const void *src, size_t size, siz
     return ESP_OK;
 }
 
-int i2s_read_bytes(i2s_port_t i2s_num, void *dest, size_t size, TickType_t ticks_to_wait)
-{
-    size_t bytes_read = 0;
-    int res = 0;
-    res = i2s_read(i2s_num, dest, size, &bytes_read, ticks_to_wait);
-    if (res != ESP_OK) {
-        return ESP_FAIL;
-    } else {
-        return bytes_read;
-    }
-}
-
 esp_err_t i2s_read(i2s_port_t i2s_num, void *dest, size_t size, size_t *bytes_read, TickType_t ticks_to_wait)
 {
     char *data_ptr, *dest_byte;
@@ -1428,29 +1334,3 @@ esp_err_t i2s_read(i2s_port_t i2s_num, void *dest, size_t size, size_t *bytes_re
     xSemaphoreGive(p_i2s_obj[i2s_num]->rx->mux);
     return ESP_OK;
 }
-
-int i2s_push_sample(i2s_port_t i2s_num, const void *sample, TickType_t ticks_to_wait)
-{
-    size_t bytes_push = 0;
-    int res = 0;
-    I2S_CHECK((i2s_num < I2S_NUM_MAX), "i2s_num error", ESP_FAIL);
-    res = i2s_write(i2s_num, sample, p_i2s_obj[i2s_num]->bytes_per_sample, &bytes_push, ticks_to_wait);
-    if (res != ESP_OK) {
-        return ESP_FAIL;
-    } else {
-        return bytes_push;
-    }
-}
-
-int i2s_pop_sample(i2s_port_t i2s_num, void *sample, TickType_t ticks_to_wait)
-{
-    size_t bytes_pop = 0;
-    int res = 0;
-    I2S_CHECK((i2s_num < I2S_NUM_MAX), "i2s_num error", ESP_FAIL);
-    res = i2s_read(i2s_num, sample, p_i2s_obj[i2s_num]->bytes_per_sample, &bytes_pop, ticks_to_wait);
-    if (res != ESP_OK) {
-        return ESP_FAIL;
-    } else {
-        return bytes_pop;
-    }
-}

components/squeezelite/display.c

@@ -628,8 +628,9 @@ static void grfe_handler( u8_t *data, int len) {
 	
 	scroller.active = false;
 	
-	// visu has priority when full screen on small screens
-	if ((visu.mode & VISU_ESP32) && !visu.col && visu.row < displayer.height) {
+	// full screen artwork or for small screen, full screen visu has priority
+	if (((visu.mode & VISU_ESP32) && !visu.col && visu.row < displayer.height) ||
+		(artwork.enable && artwork.x == 0 && artwork.y == 0)) {
 		xSemaphoreGive(displayer.mutex);
 		return;
 	}	
@@ -751,8 +752,11 @@ static void grfg_handler(u8_t *data, int len) {
 	
 	LOG_DEBUG("gfrg s:%hu w:%hu (len:%u)", htons(pkt->screen), htons(pkt->width), len);
 
-	// on small screen, visu has priority when full screen	
-	if ((visu.mode & VISU_ESP32) && !visu.col && visu.row < displayer.height) return;
+	// full screen artwork or for small screen, visu has priority when full screen	
+	if (((visu.mode & VISU_ESP32) && !visu.col && visu.row < displayer.height) || 
+		(artwork.enable && artwork.x == 0 && artwork.y == 0)) {
+		return;
+	}	
 	
 	xSemaphoreTake(displayer.mutex, portMAX_DELAY);
 		
@@ -795,7 +799,7 @@ static void grfa_handler(u8_t *data, int len) {
 
 	// when using full screen visualizer on small screen there is a brief overlay	
 	artwork.enable = (length != 0);
-
+	
 	// just a config or an actual artwork	
 	if (length < 32) {
 		if (artwork.enable) {
@@ -840,8 +844,10 @@ static void grfa_handler(u8_t *data, int len) {
  * Update visualization bars
  */
 static void visu_update(void) {
-	// no need to protect against no woning the display as we are playing	
-	if (pthread_mutex_trylock(&visu_export.mutex)) return;
+	// no update when artwork is full screen (but no need to protect against not owning the display as we are playing	
+	if ((artwork.enable && artwork.x == 0 && artwork.y == 0) || pthread_mutex_trylock(&visu_export.mutex)) {
+		return;
+	}	
 	
 	int mode = visu.mode & ~VISU_ESP32;
 				

components/squeezelite/mad.c

@@ -417,3 +417,4 @@ struct codec *register_mad(void) {
 	LOG_INFO("using mad to decode mp3");
 	return &ret;
 }
+

components/squeezelite/output_i2s.c

@@ -234,6 +234,11 @@ void output_init_i2s(log_level level, char *device, unsigned output_buf_size, ch
 	set_i2s_pin(spdif_config, &i2s_spdif_pin);										
 	set_i2s_pin(dac_config, &i2s_dac_pin);										
 
+	/* BEWARE: i2s. must be patched to set tx_msb_right/rx_msb_right to 1
+	 * or SPDIF will not work. These settings are not accessible from
+	 * userland and I don't know why
+	 */
+	 
 	// common I2S initialization
 	i2s_config.mode = I2S_MODE_MASTER | I2S_MODE_TX;
 	i2s_config.channel_format = I2S_CHANNEL_FMT_RIGHT_LEFT;

components/squeezelite/stream.c

@@ -42,8 +42,30 @@ static log_level loglevel;
 static struct buffer buf;
 struct buffer *streambuf = &buf;
 
-#define LOCK   mutex_lock(streambuf->mutex)
-#define UNLOCK mutex_unlock(streambuf->mutex)
+#define LOCK     mutex_lock(streambuf->mutex)
+#define UNLOCK   mutex_unlock(streambuf->mutex)
+
+/* 
+After a lot of hesitation, I've added that "poll mutex" to prevent
+socket from being allocated while we are still in poll(). The issue 
+happens is we have a close quickly followed by an open, we might still
+be in the poll() and simple OS fail as they re-allocate the same socket
+on which a thread is still waiting. 
+Ideally, you want to set the lock in the disconnect() but that would mean
+very often we'd have to always wait for the end of the poll(), i.e. up to
+100ms for nothing most of the time where if it is in the open(), it is 
+less elegant as closing a socket on which there is a poll() is not good 
+but it's more efficient as it is very rare that you'd have an open() less 
+then 100ms after a close()
+*/
+#if EMBEDDED
+static mutex_type poll_mutex;
+#define LOCK_L   mutex_lock(poll_mutex)
+#define UNLOCK_L mutex_unlock(poll_mutex)
+#else
+#define LOCK_L   
+#define UNLOCK_L 
+#endif
 
 static sockfd fd;
 
@@ -187,6 +209,7 @@ static void *stream_thread() {
 
 		} else {
 
+			LOCK_L;
 			pollinfo.fd = fd;
 			pollinfo.events = POLLIN;
 			if (stream.state == SEND_HEADERS) {
@@ -195,9 +218,10 @@ static void *stream_thread() {
 		}
 
 		UNLOCK;
-
+		
 		if (_poll(ssl, &pollinfo, 100)) {
 
+			UNLOCK_L;	
 			LOCK;
 
 			// check socket has not been closed while in poll
@@ -350,7 +374,7 @@ static void *stream_thread() {
 			UNLOCK;
 			
 		} else {
-			
+			UNLOCK_L;
 			LOG_SDEBUG("poll timeout");
 		}
 	}
@@ -403,6 +427,9 @@ void stream_init(log_level level, unsigned stream_buf_size) {
 	*stream.header = '\0';
 
 	fd = -1;
+#if EMBEDDED	
+	mutex_create_p(poll_mutex);
+#endif	
 
 #if LINUX || FREEBSD
 	touch_memory(streambuf->buf, streambuf->size);
@@ -432,13 +459,16 @@ void stream_close(void) {
 #endif
 	free(stream.header);
 	buf_destroy(streambuf);
+#if EMBEDDED	
+	mutex_destroy(poll_mutex);
+#endif	
 }
 
 void stream_file(const char *header, size_t header_len, unsigned threshold) {
 	buf_flush(streambuf);
 
 	LOCK;
-
+	
 	stream.header_len = header_len;
 	memcpy(stream.header, header, header_len);
 	*(stream.header+header_len) = '\0';
@@ -473,7 +503,9 @@ void stream_file(const char *header, size_t header_len, unsigned threshold) {
 void stream_sock(u32_t ip, u16_t port, const char *header, size_t header_len, unsigned threshold, bool cont_wait) {
 	struct sockaddr_in addr;
 
+	LOCK_L;
 	int sock = socket(AF_INET, SOCK_STREAM, 0);
+	UNLOCK_L;
 
 	if (sock < 0) {
 		LOG_ERROR("failed to create socket");