Function added to detect baudrate

[Jeroen88]

Functions uart_detect_baudrate(uart_nr), uart_start_detect_baudrate(int uart_nr), wrappers for HardwareSerial and an example usage SerialDetectBaudrate.ino added.

These function detect the baudrate of the Serial port automatically. Of course there must be incoming data on the port. The function HardwareSerial::detectBaudrate() without a parameter returns inmediately. It should be called repeatedly until a non-zero value, the detected baudrate, is returned. The function HardwareSerial::detectBaudrate(time_t timeoutMillis) tries to detect the baudrate for maximum timeoutMillis ms and then returns zero if no baudrate was detected, or the detected baudrate otherwise. The detectBaudrate() functions may be called before Serial.begin() is called, they do not need the Rx buffer nor the SerialConfig parameters.

The uart can not detect other parameters like number of start- or stopbits, number of data bits or parity.

The detection does not change the baudrate, after detection it should be set using Serial.begin(detectedBaudrate).

Detection is very fast, it takes only a few incoming bytes.

Example of usage (same as SerialDetectBaudrate.ino):

#define TIMEOUT (10000UL)     // Maximum time to wait for serial activity to start

void setup() {
  // put your setup code here, to run once:

  Serial.begin(115200);

  // Serial.detectBaudrate() may also be called before Serial.begin()
  // There must be activity on the serial port for the baudrate to be detected
  unsigned long detectedBaudrate = Serial.detectBaudrate(TIMEOUT);

  if (detectedBaudrate) {
    Serial.printf("\nDetected baudrate is %lu, switching to that baudrate now...\n", detectedBaudrate);

    // Wait for printf to finish
    while (Serial.availableForWrite() != UART_TX_FIFO_SIZE) {
      yield();
    }

    // Clear Tx buffer to avoid extra characters being printed
    Serial.flush();

    // After this, any writing to Serial will print gibberish on the serial monitor if the baudrate doesn't match
    Serial.begin(detectedBaudrate);
  } else {
    Serial.println("\nNothing detected");
  }
}

void loop() {
  // put your main code here, to run repeatedly:

}

If you do not want to wait for a device responding on the Serial you can also use:

void setup() {
  // put your setup code here, to run once:

  Serial.begin(115200);

  Serial.startDetectBaudrate();
}

void loop() {
  // put your main code here, to run repeatedly:

  // Serial.testBaudrate() may also be called before Serial.begin()
  // If there is activity on the serial port the baudrate will be detected
  unsigned long detectedBaudrate = Serial.testBaudrate();

  if (detectedBaudrate) {
    unsigned long savedBaudrate = Serial.baudRate();
    if(detectedBaudrate != savedBaudrate) {
      Serial.begin(detectedBaudrate);
        // ... Read some serial data (keep in mind that the first few characters are missing because they were needed for detection) 
      Serial.begin(savedBaudrate);  // Revert to the saved baudrate
      Serial.startDetectBaudrate();  // If wanted, start detecting again
    }
  } 

  // Do something else...

}

[devyte]

Congratulations on getting baud detection to work!
In addition to my review comments, please implement a method HardwareSerial::detectBaudrate() that wraps uart_detect_baudrate().

[devyte]

This doTrigger var is inited to true, then set to false on first run. In order to reset it to true, the detection must actually succeed in a subsequent call.
There is no way to abort the detection process, or reset a detection considered to have failed.

[devyte]

Off the top of my head, I see the following possibilities:

1. add an argument which resets doTrigger when true. The arg should default to false.
2. make doTrigger a global static, and implement a resetter function.

[Jeroen88]

This was done on purpose, to have only one function to do the detection. If no detection takes place between two subsequent calls to uart_detect_baudrate(), triggering is not necessary: the uart is still in detecting state, and the function returns 0. If (enough) data was presented to the uart, the correct baudrate is returned.
If you prefer a "reset", I suggest to make a void uart_start_detect_baudrate(int uart_nr), that does the triggering, and a int uart_detect_baudrate(uart_nr) that reads the baudrate.
What do you prefer?

[devyte]

That would have the same effect as the current code: it wouldn't be possible to abort the detection process. Consider the case where you want to retry a number of times, then give up and report an error. Say that the serial lines are multiplexed or shared in some way to communicate with multiple devices. Consider what happens if the detection of the last device fails. The triggering can't be disabled for communication with the devices that were detected correctly.

I suppose my solution 1 above has the same problem, which leaves only solution 2 as viable. It must be possible to clean up the triggering in case of failure.

Unless you can think of another solution?

If you're worried about usage, I think that is addressed by wrapping the whold thing in a HardwareSerial::detectBaudrate() method. In fact, you could even implement a timed loop with yield and max retries in there and avoid having the user do the whole logic inside loop(). Trivial usage :)

[Jeroen88]

I don't know if I get you right... The uart starts detecting by clearing and setting the UART_AUTOBAUD_EN bit. I do not know if it stops detecting by only clearing the bit. But why do you want to stop the uart detecting? It is not consuming any processor time. Retrying is just: call uart_detect_baudrate() again. Giving up is just stop calling that function. The detection of the baudrate does not change it. In the case of multiple devices, if you want to detect some device D after another device B was detected uart_detect_baudrate() will return the rate of device B and reinit. If you don't need the device B baudrate, just call uart_detect_baudrate once, ignore the return value, and start detecting in a loop until device D comes up. Am I not getting you???

I will implement a hard restart, in uart.h and uart.c, just in case.

About your last suggestion, that would block the processor in waiting for the uart. What about two wrappers? A non blocking detectBaudrate() and a blocking detectBaudrate(time_t timeoutMillis)?

[devyte]

Are all of these rates valid for both 80MHz and 160MHz CPU speed?
Or rather, has the detection process been tested with both CPU speeds and different baudrates?

[Jeroen88]

These baud rates are common rates. Yes it has been tested with both 80MHz and 160 MHz, and with two devices, one at 9600 baud and one at 115200 baud. I did not test changing the crystal frequency, because I do not know the purpose of changing the crystal frequency.

[devyte]

One last thing: please add a minimalistic example sketch to show usage.

[Jeroen88]

Thanks for the congrats @devyte! There is already a minimalistic example at the top of this PR, is that what you're looking for? Because there is no /examples subdirectory in Arduino/cores/esp8266, I did not include the example in the code.
I will implement the wrapper to HardwareSerial.

[devyte]

Yes, more or less, although I would rather the example be done with the HardwareSerial wrapper method instead of the functions in uart.h.
The example should probably be added here.

[Jeroen88]

I will add a HardwareSerial example at the suggested location.

[devyte]

This should probably be renamed to detectBaudrateOnce() or similar. the usage semantic is very different from the overload with the timeout.
Personally, I would prefer to not expose this method and just make it private. I can't think of a usage example where some other time-critical process has to be ongoing at the same time as baudrate detection.
If you really need this to be public, please add another example with its usage from the loop().

[Jeroen88]

I will rename it to testBaudrate().
I can imagine that you have some kind of hot swappable serial device, and want to test every now and then if something has been connected. I prefer to keep it public.
I will add an example as a comment on this PR, not in the examples directory which seems overkill for me

[devyte]

@Jeroen88 some more comments:
-Your implementation of ::detectBaudrate() is exactly what I had in mind. It's not true that the CPU will be busy while detection is ongoing. In each yield, the SYS context will continue to operate, including servicing of the wifi stack and user callbacks. What won't continue to operate until done is the CONT context (aka the loop), and that's exactly what we want.
-Are you up to adding an entry in the docs in this file section Serial ?
-baudrate detection should probably be limited to uart0, because uart1 is TX only
-My question about resetting the register bit in case of failure remains. Despite you saying that leaving the bit enabled in case of failure would have no effect, I would prefer to clean it up, at least for general use. If you implement a simple resetter function, it could be called at the end of ::detectBaudrate() in case of failure, and that would be enough to let me sleep at night :)
-Good job on this!

[Jeroen88]

@devyte

• I think we share the same insight that the CPU is not busy detecting the baudrate, because the UART is doing that and that it is busy polling in the loop
• I will add an entry in the docs
• I know that UART1 is Tx only (unfortunately!). I pass the uart_nr parameter to be consistent with the other library functions (like inline size_t uart_rx_fifo_available(const int uart_nr)) and to not have to create exception code for UART1 / Serial1.
• I'd rather let you sleep at night, but I am affraid you have to stay awake ;), if I add the resetter, I would have to introduce a third function, I would have to declare the static bool doTrigger outside the function to set it to true in the resetter. An alternative for the latter would be to check if the UART_AUTOBAUD_EN is cleared, making uart_detect_baudrate() less transparent. Only for an extra function that serves no purpose at all.
• Thnx!

[devyte]

Sorry, I missed the single line statements in my previous review.
Other than that, it looks ok to me to merge. If you fix the formatting today, this will make it into 2.4.2.

[devyte]

break; on a new line please

[devyte]

return 0; on a new line please

[devyte]

i-- on a new line please

[Jeroen88]

Done! Would be nice if it is merged into 2.4.2 :)