Skip to content

Fixes uartAvailable() in case UART_READ_RX_FIFO is set #5541

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

Closed
wants to merge 0 commits into from
Closed

Fixes uartAvailable() in case UART_READ_RX_FIFO is set #5541

wants to merge 0 commits into from

Conversation

SuGlider
Copy link
Collaborator

Summary

Fixes #5512
In version 2.0.0, in case UART_READ_RX_FIFO is set, a race condition may happen causing the result of uartAvailable() to fail.
If two tasks read the same UART, uartAvailable() may be preempted right when the sum is being executed, generating a sporadic failed result.

Impact

Fixes the specific condition detailed in issue #5512

@SuGlider SuGlider requested a review from me-no-dev August 13, 2021 22:21
@rob-deutsch
Copy link

Thank you for the PR, @SuGlider.

Do interrupts also have to be turned off? For example, what happens if _uart_isr() is called right after uxQueueMessagesWaiting() but before uart->dev->status.rxfifo_cnt is fetched?

return (uxQueueMessagesWaiting(uart->queue) + uart->dev->status.rxfifo_cnt) ;
UART_MUTEX_UNLOCK();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line will never be called and the lock will potentially be left locked. You will need to store the value in a temporary variable and unlock prior to return.

Copy link
Collaborator Author

@SuGlider SuGlider Aug 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely right!
esp32-hal-uart.c is being refactored.
For the matter UART_READ_RX_FIFO is unset by default, thus this part of the code isn't used in 2.0.0.
Closing this PR in favor of the UART code refactoring.

@SuGlider SuGlider closed this Aug 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uartAvailable() has race condition
3 participants