-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add kconfig options for rp2040 uart #6549
Add kconfig options for rp2040 uart #6549
Conversation
Thank you for submitting a PR, pleas refer to point 3 in "What to expect in a review" in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and provide a signed off by line. Thanks |
… combinations for RP2040 Signed-off-by:Hriday Keni <info@amken.us>
… combinations for RP2040 Signed-off-by:Hriday Keni <info@amken.us>
Signed-off-by:Hriday Keni <info@amken.us>
97cfc56
to
a1a8e8c
Compare
I have included the sign off line for each commit and rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. We can certainly add additional serial options for the rp2040. However, please layout the user facing menu (and backend Kconfig code) in the same style as the stm32 serial selection menu. That is, we want to have a single flat menu with all the meaningful user facing communication options. Also, like on stm32, it's probably a good idea to tie the less used serial options to CONFIG_LOW_LEVEL_OPTIONS.
I also have a minor comment on the implementation - see below.
-Kevin
src/rp2040/serial.c
Outdated
|
||
#if CONFIG_RP2040_SERIAL_UART0 | ||
enable_pclock(RESETS_RESET_UART0_BITS); | ||
uint32_t pclk = get_pclock_frequency(RESETS_RESET_UART0_BITS); | ||
#elif CONFIG_RP2040_SERIAL_UART1 | ||
enable_pclock(RESETS_RESET_UART1_BITS); | ||
uint32_t pclk = get_pclock_frequency(RESETS_RESET_UART1_BITS); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid #if
in the executable code - prefer if (CONFIG_RP2040_SERIAL_UART0) ...
Signed-off-by: hrken1 <info@amken.us>
Signed-off-by: hrken1 <info@amken.us>
@KevinOConnor, thank you for the feedback. I have made the changes you suggested. |
src/rp2040/serial.c
Outdated
@@ -3,21 +3,65 @@ | |||
// Copyright (C) 2021 Kevin O'Connor <kevin@koconnor.net> | |||
// | |||
// This file may be distributed under the terms of the GNU GPLv3 license. | |||
//Modified 04/04/2024 by Amken3d (info@amken.us) to add all UART pin combos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't add changelog information to the code - use the git commit messages for describing and documenting history. There's a brief note on this at https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review . Sorry - I missed this in the previous review. Otherwise the changes look fine to me.
Thanks,
-Kevin
Signed-off-by: hrken1 <info@amken.us>
Signed-off-by: hrken1 <info@amken.us>
Done |
Thanks. -Kevin |
…3d#243) Modified serial.c and Kconfig to dynamically select all possible UART combinations for RP2040 Signed-off-by: Hriday Keni <info@amken.us> Co-authored-by: Amken USA <166057890+amken3d@users.noreply.github.com>
Current implementation only supports UART0 on Pin0 and 1.
This change brings in all the possible UART 0 and 1 pin combinations for the RP2040.
Has been tested on a custom board with UART1 on 24,25