Skip to content

Restore UART write timeout - #348

Merged
ethanmathias merged 3 commits into
mainfrom
194-uart-reinstate-write-timeout-fix-lost-after-the-race
Oct 28, 2026
Merged

ethanmathias merged 3 commits into
mainfrom
194-uart-reinstate-write-timeout-fix-lost-after-the-race

Conversation

@RyanS4

@RyanS4 RyanS4 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a 500 ms default UART write timeout, aborts timed-out transfers, and clears late completion notifications.

Testing

Related Issue

Closes #194

Notes

@RyanS4 RyanS4 linked an issue Oct 25, 2026 that may be closed by this pull request
4 tasks
Comment thread Common/Drivers/Src/uart.cpp Resolved
Comment thread Common/Drivers/Inc/UART.h Resolved
@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

No findings: the write timeout mirrors UART::read() (uart.cpp:90-105), uses the same -2 timeout code, and the default is documented. Scope matches #194.

Comment on lines +92 to +94
TickType_t timeoutTicks = (timeout_ms == portMAX_DELAY)
? portMAX_DELAY
: pdMS_TO_TICKS(timeout_ms);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unrelated reformat of the existing read() ternary. It is outside #194 and adds diff noise. Please revert it. The new write() copy can keep the original layout from read() (the layout this hunk replaces) so the two match:

        TickType_t timeoutTicks =
        (timeout_ms == portMAX_DELAY)
            ? portMAX_DELAY
            : pdMS_TO_TICKS(timeout_ms);

Comment on lines +135 to +137
// Discard a late completion so it cannot satisfy the next write's wait.
// NOTE: this clears the task's whole default notification value, so a
// task must not use notification index 0 for anything but this driver.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check how this compares to read()'s timeout branch (line ~99). Does it also drain a late notification? If not, the extra ulTaskNotifyTake(pdTRUE, 0) is a hardware-race fix that deserves its why in one line, which the first comment line already gives. The second "NOTE" about notification index 0 is a fair constraint, but please make sure it's true for this repo (grep other ulTaskNotifyTake users) rather than a speculative warning. If it's speculative, drop it.

@ethanmathias
ethanmathias merged commit 620b3d8 into main Oct 28, 2026
2 checks passed
@ethanmathias
ethanmathias deleted the 194-uart-reinstate-write-timeout-fix-lost-after-the-race branch October 28, 2026 20:31
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.

UART: reinstate write timeout fix lost after the race

2 participants