From e08429f9f32588a31d98d3cf52f885adcd362e1c Mon Sep 17 00:00:00 2001 From: chuck todd Date: Tue, 10 Dec 2019 10:55:09 -0700 Subject: [PATCH 1/4] Fix Semaphore Handle leak code was allocating a semaphore handle on every write access. --- cores/esp32/esp32-hal-ledc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/esp32/esp32-hal-ledc.c b/cores/esp32/esp32-hal-ledc.c index 336d1efbb6b..576cdeb2f2e 100644 --- a/cores/esp32/esp32-hal-ledc.c +++ b/cores/esp32/esp32-hal-ledc.c @@ -28,7 +28,7 @@ #else #define LEDC_MUTEX_LOCK() do {} while (xSemaphoreTake(_ledc_sys_lock, portMAX_DELAY) != pdPASS) #define LEDC_MUTEX_UNLOCK() xSemaphoreGive(_ledc_sys_lock) -xSemaphoreHandle _ledc_sys_lock; +xSemaphoreHandle _ledc_sys_lock = NULL; #endif /* @@ -91,7 +91,7 @@ static void _ledcSetupTimer(uint8_t chan, uint32_t div_num, uint8_t bit_num, boo DPORT_CLEAR_PERI_REG_MASK(DPORT_PERIP_RST_EN_REG, DPORT_LEDC_RST); LEDC.conf.apb_clk_sel = 1;//LS use apb clock #if !CONFIG_DISABLE_HAL_LOCKS - _ledc_sys_lock = xSemaphoreCreateMutex(); + if( _ledc_sys_lock == NULL) _ledc_sys_lock = xSemaphoreCreateMutex(); #endif } LEDC_MUTEX_LOCK(); From bac586777fcf27d021b5330f19222136f079b1ba Mon Sep 17 00:00:00 2001 From: chuck todd Date: Tue, 10 Dec 2019 15:34:08 -0700 Subject: [PATCH 2/4] Fix memory leak caused by multiple calls to addApbChangeCallback() Every time frequency was changed, an additional cpu frequency change callback was added. Code now adds ONE callback and uses a static variable to denote active channels requiring timer reconfig on base frequency change. --- cores/esp32/esp32-hal-ledc.c | 47 +++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/cores/esp32/esp32-hal-ledc.c b/cores/esp32/esp32-hal-ledc.c index 576cdeb2f2e..9aa4055dd4a 100644 --- a/cores/esp32/esp32-hal-ledc.c +++ b/cores/esp32/esp32-hal-ledc.c @@ -55,27 +55,32 @@ xSemaphoreHandle _ledc_sys_lock = NULL; static void _on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old_apb, uint32_t new_apb){ if(ev_type == APB_AFTER_CHANGE && old_apb != new_apb){ - uint32_t iarg = (uint32_t)arg; - uint8_t chan = iarg; - uint8_t group=(chan/8), timer=((chan/2)%4); + uint16_t iarg = (uint16_t*)arg; + uint8_t chan = 0; old_apb /= 1000000; new_apb /= 1000000; - if(LEDC_TIMER(group, timer).conf.tick_sel){ - LEDC_MUTEX_LOCK(); - uint32_t old_div = LEDC_TIMER(group, timer).conf.clock_divider; - uint32_t div_num = (new_apb * old_div) / old_apb; - if(div_num > LEDC_DIV_NUM_HSTIMER0_V){ - new_apb = REF_CLK_FREQ / 1000000; - div_num = (new_apb * old_div) / old_apb; - if(div_num > LEDC_DIV_NUM_HSTIMER0_V) { - div_num = LEDC_DIV_NUM_HSTIMER0_V;//lowest clock possible + while(iarg){ // run though all active channels, adjusting timing configurations + if(iarg & 1) {// this channel is active + uint8_t group=(chan/8), timer=((chan/2)%4); + if(LEDC_TIMER(group, timer).conf.tick_sel){ + LEDC_MUTEX_LOCK(); + uint32_t old_div = LEDC_TIMER(group, timer).conf.clock_divider; + uint32_t div_num = (new_apb * old_div) / old_apb; + if(div_num > LEDC_DIV_NUM_HSTIMER0_V){ + div_num = ((REF_CLK_FREQ /1000000) * old_div) / old_apb; + if(div_num > LEDC_DIV_NUM_HSTIMER0_V) { + div_num = LEDC_DIV_NUM_HSTIMER0_V;//lowest clock possible + } + LEDC_TIMER(group, timer).conf.tick_sel = 0; + } else if(div_num < 256) { + div_num = 256;//highest clock possible + } + LEDC_TIMER(group, timer).conf.clock_divider = div_num; + LEDC_MUTEX_UNLOCK(); } - LEDC_TIMER(group, timer).conf.tick_sel = 0; - } else if(div_num < 256) { - div_num = 256;//highest clock possible } - LEDC_TIMER(group, timer).conf.clock_divider = div_num; - LEDC_MUTEX_UNLOCK(); + iarg = iarg >> 1; + chan++; } } } @@ -85,13 +90,16 @@ static void _ledcSetupTimer(uint8_t chan, uint32_t div_num, uint8_t bit_num, boo { uint8_t group=(chan/8), timer=((chan/2)%4); static bool tHasStarted = false; + static uint16_t _activeChannels = 0; if(!tHasStarted) { tHasStarted = true; DPORT_SET_PERI_REG_MASK(DPORT_PERIP_CLK_EN_REG, DPORT_LEDC_CLK_EN); DPORT_CLEAR_PERI_REG_MASK(DPORT_PERIP_RST_EN_REG, DPORT_LEDC_RST); LEDC.conf.apb_clk_sel = 1;//LS use apb clock + addApbChangeCallback((void*)&_activeChannels, _on_apb_change); + #if !CONFIG_DISABLE_HAL_LOCKS - if( _ledc_sys_lock == NULL) _ledc_sys_lock = xSemaphoreCreateMutex(); + _ledc_sys_lock = xSemaphoreCreateMutex(); #endif } LEDC_MUTEX_LOCK(); @@ -105,8 +113,7 @@ static void _ledcSetupTimer(uint8_t chan, uint32_t div_num, uint8_t bit_num, boo LEDC_TIMER(group, timer).conf.rst = 1;//This bit is used to reset timer the counter will be 0 after reset. LEDC_TIMER(group, timer).conf.rst = 0; LEDC_MUTEX_UNLOCK(); - uint32_t iarg = chan; - addApbChangeCallback((void*)iarg, _on_apb_change); + _activeChannels |= (1 << chan); // mark as active for APB callback } //max div_num 0x3FFFF (262143) From e705fce06e728a452a550f322a6e885e7a277c68 Mon Sep 17 00:00:00 2001 From: stickbreaker Date: Wed, 11 Dec 2019 20:24:52 -0700 Subject: [PATCH 3/4] Fix APBChangeCallback logic in LedC. Change apbChangeCallback to double link list, roll cpu freq change newest->oldest then Oldest->Newest to minimize UART disruption. Fix duplicate addApbChangeCallback detection. Fix UART MUTEX deadlock when log_x calls occuring from apbCallbacks --- cores/esp32/esp32-hal-cpu.c | 67 +++++++++++++++++++++--------------- cores/esp32/esp32-hal-ledc.c | 5 ++- cores/esp32/esp32-hal-uart.c | 10 +++--- 3 files changed, 49 insertions(+), 33 deletions(-) diff --git a/cores/esp32/esp32-hal-cpu.c b/cores/esp32/esp32-hal-cpu.c index 1e30bcf71b1..db002858827 100644 --- a/cores/esp32/esp32-hal-cpu.c +++ b/cores/esp32/esp32-hal-cpu.c @@ -28,6 +28,7 @@ #include "esp32-hal-cpu.h" typedef struct apb_change_cb_s { + struct apb_change_cb_s * prev; struct apb_change_cb_s * next; void * arg; apb_change_cb_t cb; @@ -53,9 +54,19 @@ static void triggerApbChangeCallback(apb_change_ev_t ev_type, uint32_t old_apb, initApbChangeCallback(); xSemaphoreTake(apb_change_lock, portMAX_DELAY); apb_change_t * r = apb_change_callbacks; - while(r != NULL){ - r->cb(r->arg, ev_type, old_apb, new_apb); - r=r->next; + if( r != NULL ){ + if(ev_type == APB_BEFORE_CHANGE ) + while(r != NULL){ + r->cb(r->arg, ev_type, old_apb, new_apb); + r=r->next; + } + else { // run backwards through chain + while(r->next != NULL) r = r->next; // find first added + while( r != NULL){ + r->cb(r->arg, ev_type, old_apb, new_apb); + r=r->prev; + } + } } xSemaphoreGive(apb_change_lock); } @@ -68,6 +79,7 @@ bool addApbChangeCallback(void * arg, apb_change_cb_t cb){ return false; } c->next = NULL; + c->prev = NULL; c->arg = arg; c->cb = cb; xSemaphoreTake(apb_change_lock, portMAX_DELAY); @@ -75,18 +87,20 @@ bool addApbChangeCallback(void * arg, apb_change_cb_t cb){ apb_change_callbacks = c; } else { apb_change_t * r = apb_change_callbacks; - if(r->cb != cb || r->arg != arg){ - while(r->next){ - r = r->next; - if(r->cb == cb && r->arg == arg){ - free(c); - goto unlock_and_exit; - } - } - r->next = c; + // look for duplicate callbacks + while( (r != NULL ) && !((r->cb == cb) && ( r->arg == arg))) r = r->next; + if (r) { + log_e("duplicate func=%08X arg=%08X",c->cb,c->arg); + free(c); + xSemaphoreGive(apb_change_lock); + return false; + } + else { + c->next = apb_change_callbacks; + apb_change_callbacks-> prev = c; + apb_change_callbacks = c; } } -unlock_and_exit: xSemaphoreGive(apb_change_lock); return true; } @@ -95,24 +109,21 @@ bool removeApbChangeCallback(void * arg, apb_change_cb_t cb){ initApbChangeCallback(); xSemaphoreTake(apb_change_lock, portMAX_DELAY); apb_change_t * r = apb_change_callbacks; - if(r == NULL){ + // look for matching callback + while( (r != NULL ) && !((r->cb == cb) && ( r->arg == arg))) r = r->next; + if ( r == NULL ) { + log_e("not found func=%08X arg=%08X",cb,arg); xSemaphoreGive(apb_change_lock); return false; - } - if(r->cb == cb && r->arg == arg){ - apb_change_callbacks = r->next; - free(r); - } else { - while(r->next && (r->next->cb != cb || r->next->arg != arg)){ - r = r->next; } - if(r->next == NULL || r->next->cb != cb || r->next->arg != arg){ - xSemaphoreGive(apb_change_lock); - return false; + else { + // patch links + if(r->prev) r->prev->next = r->next; + else { // this is first link + apb_change_callbacks = r->next; } - apb_change_t * c = r->next; - r->next = c->next; - free(c); + if(r->next) r->next->prev = r->prev; + free(r); } xSemaphoreGive(apb_change_lock); return true; @@ -186,7 +197,7 @@ bool setCpuFrequencyMhz(uint32_t cpu_freq_mhz){ //Update REF_TICK (uncomment if REF_TICK is different than 1MHz) //if(conf.freq_mhz < 80){ // ESP_REG(APB_CTRL_XTAL_TICK_CONF_REG) = conf.freq_mhz / (REF_CLK_FREQ / MHZ) - 1; - //} + // } //Update APB Freq REG rtc_clk_apb_freq_update(apb); //Update esp_timer divisor diff --git a/cores/esp32/esp32-hal-ledc.c b/cores/esp32/esp32-hal-ledc.c index 9aa4055dd4a..4854483aba8 100644 --- a/cores/esp32/esp32-hal-ledc.c +++ b/cores/esp32/esp32-hal-ledc.c @@ -55,7 +55,7 @@ xSemaphoreHandle _ledc_sys_lock = NULL; static void _on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old_apb, uint32_t new_apb){ if(ev_type == APB_AFTER_CHANGE && old_apb != new_apb){ - uint16_t iarg = (uint16_t*)arg; + uint16_t iarg = *(uint16_t*)arg; uint8_t chan = 0; old_apb /= 1000000; new_apb /= 1000000; @@ -78,6 +78,9 @@ static void _on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old_apb LEDC_TIMER(group, timer).conf.clock_divider = div_num; LEDC_MUTEX_UNLOCK(); } + else { + log_e("timer not active chan=%d",chan); + } } iarg = iarg >> 1; chan++; diff --git a/cores/esp32/esp32-hal-uart.c b/cores/esp32/esp32-hal-uart.c index 8bb4b185566..90acf54f2b1 100644 --- a/cores/esp32/esp32-hal-uart.c +++ b/cores/esp32/esp32-hal-uart.c @@ -217,7 +217,6 @@ uart_t* uartBegin(uint8_t uart_nr, uint32_t baudrate, uint32_t config, int8_t rx if(txPin != -1) { uartAttachTx(uart, txPin, inverted); } - addApbChangeCallback(uart, uart_on_apb_change); return uart; } @@ -377,18 +376,21 @@ static void uart_on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old uart->dev->int_clr.val = 0xffffffff; // read RX fifo uint8_t c; - BaseType_t xHigherPriorityTaskWoken; + // BaseType_t xHigherPriorityTaskWoken; while(uart->dev->status.rxfifo_cnt != 0 || (uart->dev->mem_rx_status.wr_addr != uart->dev->mem_rx_status.rd_addr)) { c = uart->dev->fifo.rw_byte; - if(uart->queue != NULL && !xQueueIsQueueFullFromISR(uart->queue)) { - xQueueSendFromISR(uart->queue, &c, &xHigherPriorityTaskWoken); + if(uart->queue != NULL ) { + xQueueSend(uart->queue, &c, 1); //&xHigherPriorityTaskWoken); } } + UART_MUTEX_UNLOCK(); + // wait TX empty while(uart->dev->status.txfifo_cnt || uart->dev->status.st_utx_out); } else { //todo: // set baudrate + UART_MUTEX_LOCK(); uint32_t clk_div = (uart->dev->clk_div.div_int << 4) | (uart->dev->clk_div.div_frag & 0x0F); uint32_t baud_rate = ((old_apb<<4)/clk_div); clk_div = ((new_apb<<4)/baud_rate); From 60267449fa9dbb5abf5d083223446b8c3ed4e348 Mon Sep 17 00:00:00 2001 From: chuck todd Date: Mon, 16 Dec 2019 11:12:47 -0700 Subject: [PATCH 4/4] correct error message Fix incorrect error message. If timer clock source was REF_CLK, (slow speed 8mhz) ApbClockChange callback was incorrectly reporting that timer channel was inactive. --- cores/esp32/esp32-hal-ledc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp32/esp32-hal-ledc.c b/cores/esp32/esp32-hal-ledc.c index 4854483aba8..24bbf329404 100644 --- a/cores/esp32/esp32-hal-ledc.c +++ b/cores/esp32/esp32-hal-ledc.c @@ -79,7 +79,7 @@ static void _on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old_apb LEDC_MUTEX_UNLOCK(); } else { - log_e("timer not active chan=%d",chan); + log_d("using REF_CLK chan=%d",chan); } } iarg = iarg >> 1;