Add an option to force IDF's default UART clock source #11191
Conversation
👋 Hello gonzabrusco, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 76 files 76 suites 12m 47s ⏱️ Results for commit 3e78c46. ♻️ This comment has been updated with latest results. |
me-no-dev
added
Peripheral: UART
Status: Review needed
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gonzabrusco - Thank you for the PR and for the consistent collaboration! This PR is, certainly, a way for solving it. After the discussion in #11132, I have been thinking about it. I was considering adding an extra parameter directly into KConfig is not used by most Arduino Users. May I add the extra parameter for defining the UART Clock Source on top of your PR? |
@SuGlider Wow, I was just thinking exactly the same. I was about to propose that. Yes, the alternative to this is to have a last parameter in HardwareSerial::begin(...) that has a default value of -1 (Arduino current automatic clock selection). That way you don't break current sketches, and you also provide clock selection for each UART controller individually. I think that is a better approach to my original pull request. If you want me to help you with that, just let me know. It's a pity this didn't make it into Arduino 3.2.0. PS: Yes, you can add that on top of this PR. No problem. |
SuGlider
added
Status: In Progress ⚠️
SuGlider
moved this from Todo
to Selected for Development
in Arduino ESP32 Core Project Roadmap
|
@gonzabrusco - After discussing this change internally, we have decided not to add an extra parameter to the already full of parameters In case that none is set, it will follow the current algorithm (REF_TICK for ESP32/S2 when baud rate < 250,000 - or APB otherwise || XTAL for all other SOC). It will also keep the option from kConfig as defined in your PR. Let me know if you agree and/or have another suggestion. |
That makes sense! LGTM |
I have changed the kconfig description. I think that I should change Another solution is to just allow it be there when Other option would be just leave What do think that makes more sense? |
To be completely honest, after all your hard work, I think the KConfig option has become virtually unnecessary. Although it was the initial trigger for this pull request, it has now evolved into something else, something better. So feel free to remove the KConfig option; it will also make the code more readable. |
SuGlider
added
Status: In Progress ⚠️
|
@me-no-dev |
Thanks! I agree to you. I have changed the code to remove kconfig setting. |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (4)
- cores/esp32/HardwareSerial.cpp: Language not supported
- cores/esp32/HardwareSerial.h: Language not supported
- cores/esp32/esp32-hal-uart.c: Language not supported
- cores/esp32/esp32-hal-uart.h: Language not supported
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (4)
- cores/esp32/HardwareSerial.cpp: Language not supported
- cores/esp32/HardwareSerial.h: Language not supported
- cores/esp32/esp32-hal-uart.c: Language not supported
- cores/esp32/esp32-hal-uart.h: Language not supported
SuGlider
added
Status: Review needed
|
@me-no-dev @P-R-O-C-H-Y @lucasssvaz - All set. Ready for review. |
SuGlider
added
Status: Pending Merge
github-project-automation
bot
moved this from In Review
to Done
in Arduino ESP32 Core Project Roadmap
Description of Change
Using the ESP32 and baud rates lower than 250000, Arduino 3.X forces you to use
REF_TICKas clock source for the UART (instead of the APB as Arduino 2.X). Only if the baud rate is higher than 250000, it switches toAPB. This restricts on the RX timeout you can configure with thesetRxTimeoutmethod ofSerial. You can only set a maximum of 1 symbol timeout while usingREF_TICK. For using longer timeouts, you need to useAPBas clock source, but Arduino was not giving me the possibility. So I created this option that overrides this automatic selection.This pull request introduces changes to the UART clock source configuration for the ESP32 platform, adding new options and improving flexibility. The key changes include adding a new configuration option in the Kconfig file, defining new clock source options, and updating the UART initialization and configuration methods to support these new options.
Configuration Updates:
Kconfig.projbuild: Added a new configuration optionARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCEto force the default UART clock source from IDF, allowing users to override Arduino's automatic clock selection.** This was the initial approach and motivation, but it was replaced by the new
HardwareSerial::setClockSource()API.**Code Enhancements:
cores/esp32/HardwareSerial.cpp: Implemented a new methodsetClockSourceto set the UART clock source before starting UART usingbegin(). This method supports various clock sources based on the SoC.cores/esp32/HardwareSerial.h: AddedSerialClkSrcenum to define different UART clock sources and updated theHardwareSerialclass to include thesetClockSourcemethod. [1] [2]Structural Changes:
cores/esp32/esp32-hal-uart.c: Modified theuart_struct_tstructure to include_uart_clock_sourceand updated the UART initialization (uartBegin) and baud rate setting (uartSetBaudRate) methods to utilize the new clock source configuration. [1] [2] [3]cores/esp32/esp32-hal-uart.h: Added theuartSetClockSourcefunction to set the UART clock source mode, ensuring it is called beforeuartBegin().Tests scenarios
setRxTimeout. You will see the following IDF error in the console:E (215) uart: tout_thresh = 2 > maximum value = 1Related links
#11132