-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(uart): HardwareSerial begin() causes Infinite Loop #2785
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
base: main
Are you sure you want to change the base?
Conversation
…ction added. Signed-off-by: Bartu Özcan <[email protected]>
Signed-off-by: Bartu Özcan <[email protected]>
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.
Hi @baftii
Thanks for this PR.
I've made a first round review with some proposal.
I will review the getPCLK deeply later.
// Default Value | ||
uint32_t clock_prescaler = UART_PRESCALER_DIV1; | ||
|
||
uint32_t pclk = uart_getPCLK(huart); | ||
clock_prescaler = calculatePresc(pclk, baudrate, huart->Init.OverSampling); | ||
huart->Init.ClockPrescaler = clock_prescaler; |
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.
I suggest to simplify by calling uart_getPCLK
inside the calculatePresc
, so simply pass the huart
, then no more need the oversampling.
// Default Value | |
uint32_t clock_prescaler = UART_PRESCALER_DIV1; | |
uint32_t pclk = uart_getPCLK(huart); | |
clock_prescaler = calculatePresc(pclk, baudrate, huart->Init.OverSampling); | |
huart->Init.ClockPrescaler = clock_prescaler; | |
huart->Init.ClockPrescaler = uart_compute_prescaler(huart, baudrate); |
libraries/SrcWrapper/inc/uart.h
Outdated
#if defined(UART_PRESCALER_DIV1) | ||
uint32_t calculatePresc(uint32_t pclk, uint32_t baudrate, uint32_t oversampling); | ||
#endif | ||
|
||
uint32_t uart_getPCLK(UART_HandleTypeDef *huart); | ||
|
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.
I suggest to:
- change the naming to be homogenous with other functions.
- simply give the
huart
as arguments - avoid add
uart_get_clock_source_freq
when no prescaler needed as it is only used by it.
#if defined(UART_PRESCALER_DIV1) | |
uint32_t calculatePresc(uint32_t pclk, uint32_t baudrate, uint32_t oversampling); | |
#endif | |
uint32_t uart_getPCLK(UART_HandleTypeDef *huart); | |
#if defined(UART_PRESCALER_DIV1) | |
uint32_t uart_compute_prescaler(UART_HandleTypeDef * huart, uint32_t baudrate) | |
uint32_t uart_get_clock_source_freq(UART_HandleTypeDef *huart); | |
#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.
I kept uart_get_clock_source_freq
outside of the guard on purpose. Although the function is used only for the prescaler configuration, the UART's source clock frequency is not directly related to the prescaler, so I thought someone can want to use it for development or experimental usages in works that nonrelated with prescaler. However, if you think keeping it within the guard would be more beneficial, I have no problem with that.
* @retval uint32_t clock prescaler | ||
*/ | ||
#if defined(UART_PRESCALER_DIV1) | ||
uint32_t calculatePresc(uint32_t pclk, uint32_t baudrate, uint32_t oversampling) |
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.
uint32_t calculatePresc(uint32_t pclk, uint32_t baudrate, uint32_t oversampling) | |
uint32_t uart_compute_prescaler(UART_HandleTypeDef * huart, uint32_t baudrate) |
static const uint16_t presc_div[12] = {1, 2, 4, 6, 8, 10, 12, 16, 32, 64, 128, 256}; | ||
|
||
uint32_t condition = 0; | ||
if (oversampling == UART_OVERSAMPLING_16) { |
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.
if (oversampling == UART_OVERSAMPLING_16) { | |
if (huart->Init.OverSampling== UART_OVERSAMPLING_16) { |
for (uint32_t idx = 0; idx < 8; idx++) { | ||
uint32_t uartclk = pclk / presc_div[idx]; | ||
uint32_t brr = 0; | ||
if (oversampling == UART_OVERSAMPLING_16) { |
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.
if (oversampling == UART_OVERSAMPLING_16) { | |
if (huart->Init.OverSampling== UART_OVERSAMPLING_16) { |
condition = 8U; | ||
} | ||
|
||
for (uint32_t idx = 0; idx < 8; idx++) { |
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.
why it is not 12 instead of 8?
for (uint32_t idx = 0; idx < 8; idx++) { | |
for (uint32_t idx = 0; idx < 12; idx++) { |
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.
Its my bad. Firstly I wrote presc_div until 16 and after that I added values between 16 and 256. However, I forgot to change in here.
|
||
/** | ||
* @brief Function called to set the uart clock prescaler | ||
* @param pclk : supplied clock rate to related uart |
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.
To be reviewed accordingly to other change.
* @param huart : uart handle structure | ||
* @retval uint32_t clock source frequency | ||
*/ | ||
uint32_t uart_getPCLK(UART_HandleTypeDef *huart) |
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.
uint32_t uart_getPCLK(UART_HandleTypeDef *huart) | |
uint32_t uart_get_clock_source_freq(UART_HandleTypeDef *huart) |
} | ||
return UART_PRESCALER_DIV1; | ||
} | ||
#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.
like the uart_getPCLK
is only used to compute the prescaler, I would add his guard after it.
#endif |
|
||
return 0; | ||
} | ||
|
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.
#endif /* UART_PRESCALER_DIV1 */ |
Hi, I looked at your proposals for the first review and commented on a few. However, I'm a bit new to PR, so I don't know if I need to do anything right now. Do I need to batch commit your proposed changes or call a resolve conversation for proposals? Or is that all handled by you? |
Simply update your code, commit and push on your branch then it will be updated here. |
Signed-off-by: baftii <[email protected]>
I found an issue while using HardwareSerial. Sometimes, when calling begin() function, the ClockPrescaler field inside huart->Init contains a garbage value. This wrong value is then passed to HAL_UART_Init(), and later used in UART_SetConfig() → UART_DIV_SAMPLING16() macro. When this happens, the MCU triggers an interrupt and get stuck in an infinite loop.
I didn't see this problem with every HardwareSerial instance. Although I couldn't exactly find what it depends on probably it is happening when the HardwareSerial object is a member of another class, and begin() is called from that class’s constructor.
I am using PlatformIO, but I replaced framework by Arduino Core STM32 version with the latest one from GitHub. I encountered this issue while using STM32H750VBT chip.
To solve this issue I added some functions and code blocks in uart.c and uart.h files.
Added clockPrescaler setter in uart_init() function for devices which support clockPrescaler for UART. However, while setting I needed value of connected Peripharel clock and clockPrescaler finder function.
Therefore, Added a function to calculate the correct ClockPrescaler based on the UART’s PCLK frequency.
Added a helper function to get the PCLK frequency. I inspected all the datasheets while making this function but I am newbie so that this function still needs more review and testing for different STM32 series.
Tried to make the solution generic. Changes passed from CI tool but review gonna be good.
Testing