Skip to content

Commit 9ad8607

Browse files
stickbreakerme-no-dev
stickbreaker
authored and
me-no-dev
committed
Fix Memory leak in addApbChangeCallback() (#3560)
* `ledcWriteTone()` added a `apbcallback()` evertime the tone value was non zero. * `addApbChangeCallback()` did not detect duplicate callbacks. * changed the apbcallback list to a double link to support roll forward, roll back execution. This made the sequences of clock change callback start with the newest registered -> to oldest on the `before` then oldest -> newest after the clock change. This made the UART debug log output have minimal gibberish during the clock change. * change how the UART callback handled the MUTEX because if any `apbchangeCallback()` executed a `log_x()` a deadlock would occur. This fixes #3555
1 parent cec3fca commit 9ad8607

File tree

3 files changed

+75
-52
lines changed

3 files changed

+75
-52
lines changed

‎cores/esp32/esp32-hal-cpu.c‎

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "esp32-hal-cpu.h"
2929

3030
typedef struct apb_change_cb_s {
31+
struct apb_change_cb_s * prev;
3132
struct apb_change_cb_s * next;
3233
void * arg;
3334
apb_change_cb_t cb;
@@ -53,9 +54,19 @@ static void triggerApbChangeCallback(apb_change_ev_t ev_type, uint32_t old_apb,
5354
initApbChangeCallback();
5455
xSemaphoreTake(apb_change_lock, portMAX_DELAY);
5556
apb_change_t * r = apb_change_callbacks;
56-
while(r != NULL){
57-
r->cb(r->arg, ev_type, old_apb, new_apb);
58-
r=r->next;
57+
if( r != NULL ){
58+
if(ev_type == APB_BEFORE_CHANGE )
59+
while(r != NULL){
60+
r->cb(r->arg, ev_type, old_apb, new_apb);
61+
r=r->next;
62+
}
63+
else { // run backwards through chain
64+
while(r->next != NULL) r = r->next; // find first added
65+
while( r != NULL){
66+
r->cb(r->arg, ev_type, old_apb, new_apb);
67+
r=r->prev;
68+
}
69+
}
5970
}
6071
xSemaphoreGive(apb_change_lock);
6172
}
@@ -68,25 +79,28 @@ bool addApbChangeCallback(void * arg, apb_change_cb_t cb){
6879
return false;
6980
}
7081
c->next = NULL;
82+
c->prev = NULL;
7183
c->arg = arg;
7284
c->cb = cb;
7385
xSemaphoreTake(apb_change_lock, portMAX_DELAY);
7486
if(apb_change_callbacks == NULL){
7587
apb_change_callbacks = c;
7688
} else {
7789
apb_change_t * r = apb_change_callbacks;
78-
if(r->cb != cb || r->arg != arg){
79-
while(r->next){
80-
r = r->next;
81-
if(r->cb == cb && r->arg == arg){
82-
free(c);
83-
goto unlock_and_exit;
84-
}
85-
}
86-
r->next = c;
90+
// look for duplicate callbacks
91+
while( (r != NULL ) && !((r->cb == cb) && ( r->arg == arg))) r = r->next;
92+
if (r) {
93+
log_e("duplicate func=%08X arg=%08X",c->cb,c->arg);
94+
free(c);
95+
xSemaphoreGive(apb_change_lock);
96+
return false;
97+
}
98+
else {
99+
c->next = apb_change_callbacks;
100+
apb_change_callbacks-> prev = c;
101+
apb_change_callbacks = c;
87102
}
88103
}
89-
unlock_and_exit:
90104
xSemaphoreGive(apb_change_lock);
91105
return true;
92106
}
@@ -95,24 +109,21 @@ bool removeApbChangeCallback(void * arg, apb_change_cb_t cb){
95109
initApbChangeCallback();
96110
xSemaphoreTake(apb_change_lock, portMAX_DELAY);
97111
apb_change_t * r = apb_change_callbacks;
98-
if(r == NULL){
112+
// look for matching callback
113+
while( (r != NULL ) && !((r->cb == cb) && ( r->arg == arg))) r = r->next;
114+
if ( r == NULL ) {
115+
log_e("not found func=%08X arg=%08X",cb,arg);
99116
xSemaphoreGive(apb_change_lock);
100117
return false;
101-
}
102-
if(r->cb == cb && r->arg == arg){
103-
apb_change_callbacks = r->next;
104-
free(r);
105-
} else {
106-
while(r->next && (r->next->cb != cb || r->next->arg != arg)){
107-
r = r->next;
108118
}
109-
if(r->next == NULL || r->next->cb != cb || r->next->arg != arg){
110-
xSemaphoreGive(apb_change_lock);
111-
return false;
119+
else {
120+
// patch links
121+
if(r->prev) r->prev->next = r->next;
122+
else { // this is first link
123+
apb_change_callbacks = r->next;
112124
}
113-
apb_change_t * c = r->next;
114-
r->next = c->next;
115-
free(c);
125+
if(r->next) r->next->prev = r->prev;
126+
free(r);
116127
}
117128
xSemaphoreGive(apb_change_lock);
118129
return true;
@@ -186,7 +197,7 @@ bool setCpuFrequencyMhz(uint32_t cpu_freq_mhz){
186197
//Update REF_TICK (uncomment if REF_TICK is different than 1MHz)
187198
//if(conf.freq_mhz < 80){
188199
// ESP_REG(APB_CTRL_XTAL_TICK_CONF_REG) = conf.freq_mhz / (REF_CLK_FREQ / MHZ) - 1;
189-
//}
200+
// }
190201
//Update APB Freq REG
191202
rtc_clk_apb_freq_update(apb);
192203
//Update esp_timer divisor

‎cores/esp32/esp32-hal-ledc.c‎

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
#else
2929
#define LEDC_MUTEX_LOCK() do {} while (xSemaphoreTake(_ledc_sys_lock, portMAX_DELAY) != pdPASS)
3030
#define LEDC_MUTEX_UNLOCK() xSemaphoreGive(_ledc_sys_lock)
31-
xSemaphoreHandle _ledc_sys_lock;
31+
xSemaphoreHandle _ledc_sys_lock = NULL;
3232
#endif
3333

3434
/*
@@ -55,27 +55,35 @@ xSemaphoreHandle _ledc_sys_lock;
5555

5656
static void _on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old_apb, uint32_t new_apb){
5757
if(ev_type == APB_AFTER_CHANGE && old_apb != new_apb){
58-
uint32_t iarg = (uint32_t)arg;
59-
uint8_t chan = iarg;
60-
uint8_t group=(chan/8), timer=((chan/2)%4);
58+
uint16_t iarg = *(uint16_t*)arg;
59+
uint8_t chan = 0;
6160
old_apb /= 1000000;
6261
new_apb /= 1000000;
63-
if(LEDC_TIMER(group, timer).conf.tick_sel){
64-
LEDC_MUTEX_LOCK();
65-
uint32_t old_div = LEDC_TIMER(group, timer).conf.clock_divider;
66-
uint32_t div_num = (new_apb * old_div) / old_apb;
67-
if(div_num > LEDC_DIV_NUM_HSTIMER0_V){
68-
new_apb = REF_CLK_FREQ / 1000000;
69-
div_num = (new_apb * old_div) / old_apb;
70-
if(div_num > LEDC_DIV_NUM_HSTIMER0_V) {
71-
div_num = LEDC_DIV_NUM_HSTIMER0_V;//lowest clock possible
62+
while(iarg){ // run though all active channels, adjusting timing configurations
63+
if(iarg & 1) {// this channel is active
64+
uint8_t group=(chan/8), timer=((chan/2)%4);
65+
if(LEDC_TIMER(group, timer).conf.tick_sel){
66+
LEDC_MUTEX_LOCK();
67+
uint32_t old_div = LEDC_TIMER(group, timer).conf.clock_divider;
68+
uint32_t div_num = (new_apb * old_div) / old_apb;
69+
if(div_num > LEDC_DIV_NUM_HSTIMER0_V){
70+
div_num = ((REF_CLK_FREQ /1000000) * old_div) / old_apb;
71+
if(div_num > LEDC_DIV_NUM_HSTIMER0_V) {
72+
div_num = LEDC_DIV_NUM_HSTIMER0_V;//lowest clock possible
73+
}
74+
LEDC_TIMER(group, timer).conf.tick_sel = 0;
75+
} else if(div_num < 256) {
76+
div_num = 256;//highest clock possible
77+
}
78+
LEDC_TIMER(group, timer).conf.clock_divider = div_num;
79+
LEDC_MUTEX_UNLOCK();
80+
}
81+
else {
82+
log_d("using REF_CLK chan=%d",chan);
7283
}
73-
LEDC_TIMER(group, timer).conf.tick_sel = 0;
74-
} else if(div_num < 256) {
75-
div_num = 256;//highest clock possible
7684
}
77-
LEDC_TIMER(group, timer).conf.clock_divider = div_num;
78-
LEDC_MUTEX_UNLOCK();
85+
iarg = iarg >> 1;
86+
chan++;
7987
}
8088
}
8189
}
@@ -85,11 +93,14 @@ static void _ledcSetupTimer(uint8_t chan, uint32_t div_num, uint8_t bit_num, boo
8593
{
8694
uint8_t group=(chan/8), timer=((chan/2)%4);
8795
static bool tHasStarted = false;
96+
static uint16_t _activeChannels = 0;
8897
if(!tHasStarted) {
8998
tHasStarted = true;
9099
DPORT_SET_PERI_REG_MASK(DPORT_PERIP_CLK_EN_REG, DPORT_LEDC_CLK_EN);
91100
DPORT_CLEAR_PERI_REG_MASK(DPORT_PERIP_RST_EN_REG, DPORT_LEDC_RST);
92101
LEDC.conf.apb_clk_sel = 1;//LS use apb clock
102+
addApbChangeCallback((void*)&_activeChannels, _on_apb_change);
103+
93104
#if !CONFIG_DISABLE_HAL_LOCKS
94105
_ledc_sys_lock = xSemaphoreCreateMutex();
95106
#endif
@@ -105,8 +116,7 @@ static void _ledcSetupTimer(uint8_t chan, uint32_t div_num, uint8_t bit_num, boo
105116
LEDC_TIMER(group, timer).conf.rst = 1;//This bit is used to reset timer the counter will be 0 after reset.
106117
LEDC_TIMER(group, timer).conf.rst = 0;
107118
LEDC_MUTEX_UNLOCK();
108-
uint32_t iarg = chan;
109-
addApbChangeCallback((void*)iarg, _on_apb_change);
119+
_activeChannels |= (1 << chan); // mark as active for APB callback
110120
}
111121

112122
//max div_num 0x3FFFF (262143)

‎cores/esp32/esp32-hal-uart.c‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,6 @@ uart_t* uartBegin(uint8_t uart_nr, uint32_t baudrate, uint32_t config, int8_t rx
217217
if(txPin != -1) {
218218
uartAttachTx(uart, txPin, inverted);
219219
}
220-
221220
addApbChangeCallback(uart, uart_on_apb_change);
222221
return uart;
223222
}
@@ -377,18 +376,21 @@ static void uart_on_apb_change(void * arg, apb_change_ev_t ev_type, uint32_t old
377376
uart->dev->int_clr.val = 0xffffffff;
378377
// read RX fifo
379378
uint8_t c;
380-
BaseType_t xHigherPriorityTaskWoken;
379+
// BaseType_t xHigherPriorityTaskWoken;
381380
while(uart->dev->status.rxfifo_cnt != 0 || (uart->dev->mem_rx_status.wr_addr != uart->dev->mem_rx_status.rd_addr)) {
382381
c = uart->dev->fifo.rw_byte;
383-
if(uart->queue != NULL && !xQueueIsQueueFullFromISR(uart->queue)) {
384-
xQueueSendFromISR(uart->queue, &c, &xHigherPriorityTaskWoken);
382+
if(uart->queue != NULL ) {
383+
xQueueSend(uart->queue, &c, 1); //&xHigherPriorityTaskWoken);
385384
}
386385
}
386+
UART_MUTEX_UNLOCK();
387+
387388
// wait TX empty
388389
while(uart->dev->status.txfifo_cnt || uart->dev->status.st_utx_out);
389390
} else {
390391
//todo:
391392
// set baudrate
393+
UART_MUTEX_LOCK();
392394
uint32_t clk_div = (uart->dev->clk_div.div_int << 4) | (uart->dev->clk_div.div_frag & 0x0F);
393395
uint32_t baud_rate = ((old_apb<<4)/clk_div);
394396
clk_div = ((new_apb<<4)/baud_rate);

0 commit comments

Comments
 (0)