-
Notifications
You must be signed in to change notification settings - Fork 7.8k
fix(uart): terminates uart driver whenever both rx and tx pins are detached #12080
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
Conversation
👋 Hello SuGlider, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 76 files 76 suites 15m 52s ⏱️ Results for commit fec4a82. ♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Updated UART test configurations to handle I2C interactions correctly by detaching TX pin and restarting the UART driver when both RX and TX are detached.
Updated UART test configuration to detach only one pin instead of both, ensuring the UART driver continues to function. Removed redundant code related to UART driver restart when both pins are detached.
Updated UART test configuration to detach TX pin while keeping RX pin active. Adjusted assertions to reflect the new pin settings.
Added note about driver behavior when RX and TX pins are detached.
|
@me-no-dev - Not sure if this change would be considered a Breaking Change - stopping the UART after both RX/TX pins are detached. |
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.
Pull request overview
This PR implements automatic UART driver termination when both RX and TX pins are detached from a UART peripheral, which can occur when another peripheral (like I2C) claims those pins. The fix includes safeguards to prevent unwanted driver termination during legitimate pin reconfiguration operations.
Key Changes
- Added logic to terminate UART driver when both RX and TX pins are detached
- Implemented callback muting mechanism in
uartSetPinsto prevent driver termination during pin changes - Moved debug UART reset from
HardwareSerial::end()touartEnd()for centralized cleanup - Updated tests to avoid detaching both pins simultaneously, which now triggers driver termination
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cores/esp32/esp32-hal-uart.c | Core UART driver changes: added pin detachment detection logic, callback muting during pin changes, and debug UART cleanup in uartEnd() |
| cores/esp32/HardwareSerial.cpp | Removed debug UART reset from end() method (now handled in uartEnd()) |
| cores/esp32/chip-debug-report.cpp | Simplified Serial.begin() call by removing explicit 0 baud rate parameter |
| tests/validation/uart/uart.ino | Modified tests to detach only one pin at a time, avoiding driver termination |
| docs/en/api/serial.rst | Added documentation note explaining the new behavior when both pins are detached |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
pedrominatel
left a comment
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.
Just a minor change on the .rst file.
Thanks!
|
Description of Change
This PR fixes Paripheral Manager issue related to detaching rx and tx in the same or different execution time which shall lead to stoping the IDF UART driver and shuting down the Arduino log interface if enabled.
Test Scenarios
PPP using UART0 rx and tx pins for communication.
UART CI Test Sketch.
related verbose ouput:
Related links
none.