Skip to content

Allows setting only one pin (rx or tx) in the first begin() #6394

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Allows setting only one pin (rx or tx) in the first begin() #6394

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions cores/esp32/HardwareSerial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,19 +261,25 @@ void HardwareSerial::begin(unsigned long baud, uint32_t config, int8_t rxPin, in
if (!uartIsDriverInstalled(_uart)) {
switch (_uart_nr) {
case UART_NUM_0:
rxPin = rxPin < 0 ? SOC_RX0 : rxPin;
txPin = txPin < 0 ? SOC_TX0 : txPin;
if (rxPin < 0 && txPin < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't make sense to me. The previous code

rxPin = rxPin < 0 ? SOC_RX0 : rxPin;
txPin = txPin < 0 ? SOC_TX0 : txPin;

allowed changing pins individually. With this code if rxPin is set then txPin won't be set to default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mrengineer7777 See #6356 (reply in thread)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly. Previous code was forcing any undefined pin to go to the default IO Pin, only in the first Serial.begin() call.

Now it only forces default pins when no pin is defined.

When only defining rxPin, tx IOMUX will be unset.
Thus, Serial.write() would not have any effect, but it also doesn't crash ESP32.
The same for txPin alone -> Serial.read() will always return -1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't make sense to me. The previous code
allowed changing pins individually. With this code if rxPin is set then txPin won't be set to default.

Please note that this code only applies to the first time Serial.begin() is called.
Subsequent calls of Serial.begin() will change rxPin and/or TxPin individually.

For example:

Serial1.begin(9600, SERIAL_8N1, 12, 14);   // first time calling begin will set rxPin and txPin
Serial1.begin(115200);                     // doesn't change any pin (rx/tx), but only baudrate
Serial1.begin(115200, SERIAL_8N1, -1, 27); // only changes txPin to 27, keeping rxPin in 12
Serial1.end();                             // no seeting is kept from the setup above
Serial1.begin(115200);                     // new begin() from an end() will configure rxPin and txPin 
                                           // to DEFAULT pins (Serial1 9 and 10 on ESP32)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so this is a deliberate rollback to previous behavior. Looks like the crash was being caused by the TX pin defaulting to a pin that's not available on the user's hardware. One question: does passing TX=-1 to the underlying driver uartBegin() cause any issues? I'm guessing not based on this comment:

Thus, Serial.write() would not have any effect, but it also doesn't crash ESP32.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No issues with TX or RX as -1. uartBegin() will just not associate its digital pad to any IO_MUX signal.
The consequence is that the function related to the pin won't work (sending, receiving, etc).
Anyway, this feature must be used with caution.

rxPin = SOC_RX0;
txPin = SOC_TX0;
}
break;
#if SOC_UART_NUM > 1 // may save some flash bytes...
case UART_NUM_1:
rxPin = rxPin < 0 ? RX1 : rxPin;
txPin = txPin < 0 ? TX1 : txPin;
if (rxPin < 0 && txPin < 0) {
rxPin = RX1;
txPin = TX1;
}
break;
#endif
#if SOC_UART_NUM > 2 // may save some flash bytes...
case UART_NUM_2:
rxPin = rxPin < 0 ? RX2 : rxPin;
txPin = txPin < 0 ? TX2 : txPin;
if (rxPin < 0 && txPin < 0) {
rxPin = RX2;
txPin = TX2;
}
break;
#endif
default:
Expand Down