[WIP] Handle APB frequency change #2250
Conversation
|
@stickbreaker @lbernstone @atanisoft @copercini guys please have a look and come up with suggestions and possible fails. The point is to be able to listen for APB change and to handle that properly in the enabled peripherals that depend on it. There are things that need to be added/fixed even now. Like handle micros better when clock changes, otherwise you can get negative time because the multiplier changed... I looked at IDF's PM source. It shows how to update FreeRTOS ref tick. Currently is wrong... While I looked at PM's code, I noticed that it has a callback only internally, so even is I enable power management in menuconfig for Arduino, we will have no way of knowing when frequency changes to adjust peripheral clocks... my plan is to discuss this with @igrr after the holidays and see if we can integrate something like that in IDF. |
|
No way to get an event generated when the frequency changes? Seems like tick rate is far more important to FreeRTOS than anything in arduino-esp32. |
|
there is code for adjusting tick rate in IDF's PM code, so that can be done. But no, there is no event from PM when it changes frequency (it does adjust tick rate but does not tell drivers to adjust their rates if APB changed) |
cores/esp32/esp32-hal-cpu.c
Outdated
| } | ||
| capb = calculateApb(&cconf); | ||
| apb = calculateApb(&conf); | ||
| log_i("%s: %u / %u = %u Mhz", (conf.source == RTC_CPU_FREQ_SRC_PLL)?"PLL":((conf.source == RTC_CPU_FREQ_SRC_APLL)?"APLL":((conf.source == RTC_CPU_FREQ_SRC_XTAL)?"XTAL":"8M")), conf.source_freq_mhz, conf.div, conf.freq_mhz); |
There was a problem hiding this comment.
this may be better at log_d instead of log_i but I'd defer to your judgement on specific level to log this at.
There was a problem hiding this comment.
will remove it all together but it's a good test point to ensure that UART flushes before cpu can be changed. nd I often keep log at info so I do not get all unnecessary debug :P
|
|
||
| bool addApbChangeCallback(void * arg, apb_change_cb_t cb){ | ||
| initApbChangeCallback(); | ||
| apb_change_t * c = (apb_change_t*)malloc(sizeof(apb_change_t)); |
There was a problem hiding this comment.
is there a template class that can be used for linked lists that is generic enough to plug in here, maybe even something from ESP-IDF that can be leveraged instead of managing the list here (and later in remove)?
There was a problem hiding this comment.
I don't know of one, but I have been thinking of such thing for some time. This is C though... so IDK
There was a problem hiding this comment.
is there a reason to stick with C only here, there are already a few C++ pieces here and there...
There was a problem hiding this comment.
you can also use something like this to mix C++ into C...
template<int signalGenerator>
void IRAM_ATTR signalGeneratorTimerISR(void)
it has C linkage but you can call it with different values to get different versions of it (this is part of how I do ISRs using one method but two diff data sources)
There was a problem hiding this comment.
the reason why it is C is so the whole HAL can be used in C-only libs and extensions. Not really sure if it's worth it, but it's how it started :)
|
I posted a couple comments on the files, not a whole lot to add other than having some sort of callback mechanism would be good for the IDF functions if the dev mixes Arduino and IDF APIs in their code. If they call only Arduino it is pretty straightforward to update the Arduino HAL and IDF PM/FreeRTOS but if they use IDF APIs the Arduino side won't be in sync. This may be a case where the dev may need to pick one or the other to use. Perhaps a dev note in docs (are there any docs for Arduino HAL?) |
|
well... I do not see IDF to be updating peripheral clocks when APB changes anyway... so they might benefit from such functionality and Arduino can benefit as well. But I can't just let PM lose here or we will get a lot of complaints about "garbage in serial" and more |
|
So I had a talk with Ivan last night on PM and clock changes. The reason why PM does not have callbacks is performance. That is quite understandable as waiting on peripherals to switch clock might take long time. One thing that we can do is switch UART and LEDC to use REF_TICK which is stable when APB/CPU clocks are changed, but it's currently hard defined as 1MHz which is not optimal for UART. I will investigate what the consequences are if it is changed and make a PR to IDF if possible and add it to menuconfig. |
|
For this PR it should be sufficient for now to change RTOS tick and esp_timer to behave properly and then we can go with implementing the callbacks in the peripherals (and test them). That way we can at least have a working manual CPU/APB switching. Another point is that for WiFi to work, APB must be at 80MHz and since APB can not be higher than CPU, min CPU for WiFi use must be 80MHz |
|
Seems to generate some trash on Serial during the change (from default clock to 80Mhz) =/
|
|
@copercini this looks like a different issue ;) it happens too early. maybe buffer overrun or something |
|
@me-no-dev it could be related to the change if there are still characters in the uart tx buffer, it may be a good idea to flush the buffers prior to change. @copercini can you add a Serial.flush() just after line 19 in printLocalTime() and test again? |
|
@atanisoft seems better, no more trash or truncated characters, but sometimes it just stuck after the change not printing more lines 😱 |
|
@copercini uart baudrate switching is not implemented (though in your case APB will not change so WTF?) |
|
@copercini you should try this latest code. I do not see bad chars in the serial anymore. void setup() {
Serial.begin(115200);
Serial.setDebugOutput(true);
Serial.println();
Serial.printf("CPU Freq: %uHz\n", getCpuFrequency() * 1000000);
Serial.printf("APB Freq: %uHz\n", getApbFrequency());
setCpuFrequency(3);
Serial.printf("CPU Freq: %uHz\n", getCpuFrequency() * 1000000);
Serial.printf("APB Freq: %uHz\n", getApbFrequency());
Serial.printf("1: %lu, %lu\n", micros(), millis());
delayMicroseconds(1000000);
Serial.printf("2: %lu, %lu\n", micros(), millis());
delay(1000);
Serial.printf("3: %lu, %lu\n", micros(), millis());
delay(10000);
Serial.printf("4: %lu, %lu\n", micros(), millis());
}Did not work below 3MHz though... |
|
@me-no-dev with |
|
@copercini with @me-no-dev latest code the Serial.flush() call may not be necessary. |
|
yup, no need to call flush :) |
|
ok, I saw the internal flush here https://github.com/espressif/arduino-esp32/blob/cpu-clock-integration/cores/esp32/esp32-hal-uart.c#L383 but |
|
@copercini this is bugging too early for the code to be at fault |
|
could it be something else? |
**esp32-hal-i2c.c** * add callback for cpu frequency changes * adjust fifo thresholds based on cpu frequency and i2c bus frequency * reduce i2c bus frequency if differential is too small **Wire.h** * version to 1.1.0
|
guys :) all peripheral support is added :) give it a shot and check for errors |
|
ok, let all hell loose :D |
|
Could we remove the Or is there a way to suppress the /cc @stickbreaker |
@cyberman54 from espressif#2250 recommended reducing unnecessary logging.
|
@cyberman54 Sure, There hasn't been any I2C problems since this last release, I'll move them behind the |
@cyberman54 from #2250 recommended reducing unnecessary logging.
No description provided.