Skip to content

Conversation

sjlongland
Copy link

Before calling uart_poll_in, check we have space to store the character in the ring buffer beforehand. If we do, then poll for the character.

That way we don't miss out on serial traffic when our ring buffer is full unless we fill our hardware ring buffer too.

Fixes issue #97235.

Copy link

github-actions bot commented Oct 9, 2025

Hello @sjlongland, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@zephyrbot zephyrbot added the area: Shell Shell subsystem label Oct 9, 2025
@sjlongland sjlongland force-pushed the feature/CORE-5613-shell-check-space branch from 6e88425 to c5843ce Compare October 9, 2025 02:54
jakub-uC
jakub-uC previously approved these changes Oct 9, 2025
@jakub-uC jakub-uC added the bug The issue is a bug, or the PR is fixing a bug label Oct 9, 2025
while (uart_poll_in(sh_uart->common.dev, &c) == 0) {
while ((ring_buf_space_get(&sh_uart->rx_ringbuf) > 0) &&
(uart_poll_in(sh_uart->common.dev, &c) == 0)) {
if (ring_buf_put(&sh_uart->rx_ringbuf, &c, 1) == 0U) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the if statement still make sense to have it?

Copy link
Author

Choose a reason for hiding this comment

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

I left it there just in case there was some possibility of concurrency in the shell thread… I'm still getting my head around the architecture that is Zephyr, but I can remove it if you think that'd be better.

Copy link
Author

Choose a reason for hiding this comment

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

Removed in c14fdd5. :-)

@henrikbrixandersen
Copy link
Member

"CORE-5615 / GHI-97235" - what does this refer to?

@sjlongland
Copy link
Author

"CORE-5615 / GHI-97235" - what does this refer to?

One's an internal Jira tracking number (which I can remove).

GHI stands for GitHub Issue. i.e. Git Hub Issue #97235.

@nashif
Copy link
Member

nashif commented Oct 9, 2025

"CORE-5615 / GHI-97235" - what does this refer to?

at leasr one of them is obvious #97235 :)

@nashif nashif changed the title CORE-5615 / GHI-97235: shell_uart: Stop polling if buffer is full shell_uart: Stop polling if buffer is full Oct 9, 2025
@pdgendt
Copy link
Contributor

pdgendt commented Oct 9, 2025

"CORE-5615 / GHI-97235" - what does this refer to?

One's an internal Jira tracking number (which I can remove).

GHI stands for GitHub Issue. i.e. Git Hub Issue #97235.

Please remove it from the commit subject, if needed put a full link in the commit body.

Before calling `uart_poll_in`, check we have space to store the character
in the ring buffer beforehand.  If we do, *then* poll for the character.

That way we don't miss out on serial traffic when our ring buffer is full
unless we fill our hardware ring buffer too.

Signed-off-by: Stuart Longland <[email protected]>
@sjlongland sjlongland force-pushed the feature/CORE-5613-shell-check-space branch from c5843ce to fdb36c9 Compare October 10, 2025 10:55
We're now checking if there is space prior to executing the loop body,
therefore guaranteeing this condition is never going to fire in a
single-threaded context.

Signed-off-by: Stuart Longland <[email protected]>
@sjlongland
Copy link
Author

Please remove it from the commit subject, if needed put a full link in the commit body.

Yep, I've removed it from the commit message.

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Shell Shell subsystem bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants