espressif/arduino-esp32

Don't use begin() method to just change baudrate

Closed this issue · 6 comments

GitHubLionel commented

Ok, in that case, we need a warning for the begin() method to say that if you just want to change the Baudrate you must use updateBaudRate() method even if uartBegin() seems to allow this change.

Originally posted by @GitHubLionel in #10314

GitHubLionel commented

But I think that it will be better to correct the code in esp32-hal-uart.c since uartBegin() method should be do that.

Jason2866 commented

There is nothing to correct when using wrong command for changing the UART speed.
As shown in example https://github.com/espressif/arduino-esp32/blob/master/libraries/ESP32/examples/Serial/Serial_All_CPU_Freqs/Serial_All_CPU_Freqs.ino

SuGlider commented

Up on the Arduino specification, Serial.begin(baud_rate) must correctly change the baud rate.
Therefore, I consider it as a bug.

I'll get a PR to fix it.

  • 👍1
SuGlider commented

@GitHubLionel - please test PR #11122 and let me know.

GitHubLionel commented

Hi,
No, not work because MUTEX is misplaced.
You should replace this code:

      UART_MUTEX_LOCK();
      //User may just want to change some parameters, such as baudrate, data length, parity, stop bits or pins
      if (uart->_baudrate != baudrate) {
        if (!uartSetBaudRate(uart, baudrate)) {
          log_e("UART%d changing baudrate failed.", uart_nr);
          retCode = false;
        } else {
          log_v("UART%d changed baudrate to %d", uart_nr, baudrate);
          uart->_baudrate = baudrate;
        }
      }

By this one:

      //User may just want to change some parameters, such as baudrate, data length, parity, stop bits or pins
      if (uart->_baudrate != baudrate) {
      	retCode = uartSetBaudRate(uart, baudrate);
      }
      UART_MUTEX_LOCK();

Another thing, if you could use %ld in log message in uartSetBaudRate() for baud_rate to avoid compiler warning.
log_v("Setting UART%d baud rate to %ld.", uart->num, baud_rate);
log_e("Setting UART%d baud rate to %ld has failed.", uart->num, baud_rate);

  • 👍1
SuGlider commented

@GitHubLionel - Thanks for testing and reviewing. All good now. This issue shall be closed when the PR is merged.