Skip to content
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

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

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Mar 8, 2022

Summary

This PR allow users to start HardwareSerial setting just one pin (Rx or Tx) or both of them.
It allows these possible initial configurations:

// In this case, Serial will configure rxPin and only be able to receive data
Serial1.begin(9600, SERIAL_8N1, rxPin);
// In this case, Serial will configure txPin and only be able to send data
Serial1.begin(9600, SERIAL_8N1, -1, txPin);
// In this case, Serial will configure rxPin and txPin with default pins for the chip (as in HardwareSerial.cpp)
Serial1.begin(9600, SERIAL_8N1);
// In this case, Serial will configure rxPin and txPin as defined by the user
Serial1.begin(9600, SERIAL_8N1, rxPin, txPin);

Impact

None

Related links

This PR solves a discussion at #6356
Fixes #6395

@SuGlider SuGlider added the Area: Peripherals API Relates to peripheral's APIs. label Mar 8, 2022
@SuGlider SuGlider added this to the 2.0.3 milestone Mar 8, 2022
@SuGlider SuGlider requested a review from me-no-dev March 8, 2022 19:23
@SuGlider SuGlider self-assigned this Mar 8, 2022
@@ -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.

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.

Jason2866 added a commit to Jason2866/Tasmota that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Peripherals API Relates to peripheral's APIs.
Projects
Development

Successfully merging this pull request may close these issues.

Tasmota crashes on ESP32-PICO-V3 with recent core
4 participants