esp32: Fix UART rxidle and machine.Timer on ESP32-C2,C6 #17844
Conversation
|
@dpgeorge While testing this I reproduced a hard fault which might be the same one that you were seeing. I think that's a pre-existing bug not a regression, so I'll submit in a second PR that could be left until after the 1.26 release. |
6d122b3 to
c10bcd2
Compare
|
Thanks for debugging and fixing this so quickly! I tested on ESP32-C6 and it does indeed fix the
How about calling |
c10bcd2 to
fcaa4d7
Compare
Yes, this is better - have changed (and re-tested on OG ESP32 and C6, same timer results.) I got a bit hung up on "this should be compile-time calculated!" 😆 Possibly caching it isn't even worth it, it's not a particularly expensive function call to make relative to the frequency of the timer ISR. (But, then again, caching it only costs 4 bytes of RAM so it's not a big trade-off either way.) |
fcaa4d7 to
28b171b
Compare
Uh, I looked again and it doesn't call this function from the Timer ISR anyway (in retrospect it's unclear why it would need to), so I've pulled out the caching as well. |
28b171b to
8cdbfd3
Compare
|
I retested this on ESP32 and ESP32-C6, and the UART IRQ tests pass. |
Otherwise the PLL is not enabled. These changes are adapted from `timer_legacy.h` in ESP-IDF. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
Also future-proofs this code for other chips. Apart form C6 and C2, all currently supported chips use APB clock for GPTIMER_CLK_SRC_DEFAULT. ESP32-C2 uses 40MHz PLL but APB_CLK_FREQ was 26MHz. ESP32-C6 uses 80MHz PLL but APB_CLK_FREQ was 40MHz. Implementation now gets the correct frequency from ESP-IDF. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
8cdbfd3 to
593ae04
Compare
|
Now merged. Thanks again for the fast turn around on this! |
Summary
tests/extmod_hardware/machine_uart_irq_rxidle.py.machine.Timerimplementation assumed the default timer source clock was the APB clock but it's a different clock on some chips, leading to incorrect timer periods (ESP32-C2 used 26MHz but should be 40MHz, ESP32-C6 used 40MHz but should be 80MHz).Testing
extmod_hardware/machine_uart_*.pytests on ESP32-C2,C3,C6,S2 and original. All passing now.machine.Timertest on ESP32-C2,C6 and S2. Verified incorrect tick period on C2 and C6 that is fixed with this PR. Will submit a proper unit test for this, later.(Note: Some of these tests were run on the first version of this PR. I re-ran the latest version on C2, C6 and original ESP32.)
Trade-offs and Alternatives
esp_clk_tree_src_get_freq_hz()to get a constant clock frequency seems inelegant compared to calculating it at compile time, but this isn't called very often (initially I thought it was being called from the ISR, which made it seem worse, but it's not!)This work was funded through GitHub sponsors