👋Hello gonzabrusco, we appreciate your contribution to this project!

Generated by 🚫dangerJS against3e78c46

Test Results

 76 files   76 suites   12m 47s ⏱️
 38 tests  38 ✅ 0 💤 0 ❌
241 runs  241 ✅ 0 💤 0 ❌

Results for commit3e78c46.

♻️ This comment has been updated with latest results.

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.

@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 intoHardwareSerial::begin(...) (last parameter) that will allow the application to set any UART Clock Source based on the possible SoC options.

KConfig is not used by most Arduino Users.
It makes a lot of sense when building an application using Arduino as IDF Component.

May I add the extra parameter for defining the UART Clock Source on top of your PR?
Let me know. Otherwise I can open a new one.

@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 intoHardwareSerial::begin(...) (last parameter) that will allow the application to set any UART Clock Source based on the possible SoC options.

KConfig is not used by most Arduino Users. It makes a lot of sense when building an application using Arduino as IDF Component.

May I add the extra parameter for defining the UART Clock Source on top of your PR? Let me know. Otherwise I can open a new one.

@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.

@gonzabrusco - After discussing this change internally, we have decided not to add an extra parameter to the already full of parametersbegin(...).
Therefore, I'm adding a new method calledbool HardwareSerial::setClockSource(SerialClkSrc clkSrc) that must be called beforeHardwareSerial::begin() in order to apply choosen UART Clock Source.

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.

@gonzabrusco - After discussing this change internally, we have decided not to add an extra parameter to the already full of parametersbegin(...). Therefore, I'm adding a new method calledbool HardwareSerial::setClockSource(SerialClkSrc clkSrc) that must be called beforeHardwareSerial::begin() in order to apply choosen UART Clock Source.

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

@SuGlider I was checking the code and everything seems fine except for my description of the KConfig option that, because of the changes, it didn't age well hehe.

Note: This configuration is not applicable to the low-power (LP) UART controller.

I think this note should be removed since you made that option apply to LP UART too!

I have changed the kconfig description. I think that I should changeHardwareSerial::setClockSource() to checkARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE and display a warning (log_w()) when it is enabled, becuase this function won't have any effect.

Another solution is to just allow it be there whenARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE is off, something similar to a#if 0 to remove it completly from the code, in caseARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE is on

Other option would be just leaveHardwareSerial::setClockSource() and remove theARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE kconfig option.

What do think that makes more sense?

@SuGlider I was checking the code and everything seems fine except for my description of the KConfig option that, because of the changes, it didn't age well hehe.

Note: This configuration is not applicable to the low-power (LP) UART controller.

I think this note should be removed since you made that option apply to LP UART too!

I have changed the kconfig description. I think that I should changeHardwareSerial::setClockSource() to checkARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE and display a warning (log_w()) when it is enabled, becuase this function won't have any effect.

Another solution is to just allow it be there whenARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE is off, something similar to a#if 0 to remove it completly from the code, in caseARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE is on

Other option would be just leaveHardwareSerial::setClockSource() and remove theARDUINO_SERIAL_FORCE_IDF_DEFAULT_CLOCK_SOURCE kconfig option.

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.

@me-no-dev
Changed status to work on removing kconfig option and just leaving the newHardwareSerial::setClockSource() method.
It will get back to review in a couple days.

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.

Thanks! I agree to you. I have changed the code to remove kconfig setting.
Please go for a final review and then we will move forward for merging.