Fix Memory leak in addApbChangeCallback() #3560
Conversation
code was allocating a semaphore handle on every write access.
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.
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
Fix incorrect error message. If timer clock source was REF_CLK, (slow speed 8mhz) ApbClockChange callback was incorrectly reporting that timer channel was inactive.
stickbreaker
changed the title
|
@stickbreaker if i run my code on current head of this repository, i suddenly get this error: |
|
@cyberman54 add this debug to About line 99: bool addApbChangeCallback(void * arg, apb_change_cb_t cb){
// snipped
else {
log_d("adding apb callback func=%08X arg=%08X",c->cb,c->arg); // << this line
c->next = apb_change_callbacks;
apb_change_callbacks-> prev = c;
apb_change_callbacks = c;
}
}
xSemaphoreGive(apb_change_lock);
return true;
}and rerun your code with CORE_DEBUG set to DEBUG. The error message is reporting that the specified callback with the specified argument did not exist in the list of clock change callbacks. Either delete is being called twice for the same callback, or the callback function address is different or the arg value does not match the callback function address. Grab the serial log, lets see what the problem is. Chuck. |
|
@stickbreaker Thanks, i will prepare this soon. My code doesn't do anything to CPU clock cycle. Thus i need to check which library causes this error. I suspect it's the LMIC lorawanstack. |
|
@stickbreaker I now see this in the serial log: Looks like the remove happenes before the add... ? |
|
add a call to
That will give you a stack dump and you can decode it to find where the out of sequence The error is outside of Chuck. |
|
@cyberman54 you could add a condition compile: log_e("something bad happend");
#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG)
abort();
#endifSo it would only abort when compiled for debugging. Chuck. |
|
@stickbreaker thanks for you help. I inserted an Some Context: I am using the library MCCI LMIC which provides functions to communicate with a LORA RF chip via SPI. Stack trace looks like there goes something wrong with the SPI communication.
|
|
Peeked into this some deeper: Looks like the problem is caused by |
|
@cyberman54 the LMIC code doesn't use any thing special in the ESP32 SPI; it doesn't use callbacks at all. There was a change (mcci-catena/arduino-lmic#190) specifically done for ESP32 by @manuelbl, but it just uses standard higher-level SPI functions. I don't think this is an LMIC problem per se. Possibilities to consider:
Based on the stack backtrace, I'd investigate (5) further. |
|
@cyberman54 the problem is that It is a problem in Chuck. |
|
as a temp fix you can either ignore the error, and/or remove the |
|
@terrillmoore @stickbreaker thanks for your efforts to analyze this. So it's a bug in arduino-esp32 which is now revealed. |
|
@cyberman54 the fix is in #3675 |
ledcWriteTone()added aapbcallback()evertime the tone value was non zero.addApbChangeCallback()did not detect duplicate callbacks.beforethen oldest -> newest after the clock change. This made the UART debug log output have minimal gibberish during the clock change.apbchangeCallback()executed alog_x()a deadlock would occur.This fixes #3555