Improves UART reading performance #7525

SuGlider · 2022-11-27T20:16:55Z

Description of Change

Improves UART reading more than 1 bytes, using HardwareSerial::read(uint8_t *buff, size_t len)

Tests scenarios

Tested with ESP32, ESP32S2, ESP32S3, ESP32C3 using this sketch and a Python script runing on a computer.
See #7505

General Testing Sketch:

//Testing by sending "0123456789abcdefghijklmnopqrstuvwxyz" using Serial Monitor

void setup() {
  Serial.begin(115200);
  Serial.println("\nSend anything to Serial... testing read() and peek()\n");
}

void loop() {
  if (Serial.available()) {
    Serial.printf("Serial has %d bytes available.\n", Serial.available());
    Serial.printf("Serial PEEK is %c\n", Serial.peek());
    int i = Serial.read();
    Serial.printf("Serial Read 1 byte after Peek: %d ['%c']\n", i, i);
    i = Serial.read();
    Serial.printf("Serial Read again 1 byte after First Read: %d ['%c']\n", i, i);
    Serial.println("Serial Read in blocks of 10 bytes:");
    i = Serial.peek();
    Serial.printf("Serial PEEK before trying to read a chunk of data is %d ['%c']\n", i, i);
    uint8_t buf[11];
    size_t len = 0;
    while (len = Serial.read(buf, 10)) {
    //while (Serial.available()) {
      buf[len] = '\0';
      Serial.println((char*)buf);
      i = Serial.peek();
      Serial.printf("Serial PEEK is %d ['%c']\n", i, i);
    }
    Serial.println("===============================================");
  }
}

Expected OUPUT when sending 0123456789abcdefghijklmnopqrstuvwxyz

Serial has 36 bytes available.
Serial PEEK is 0
Serial Read 1 byte after Peek: 48 ['0']
Serial Read again 1 byte after First Read: 49 ['1']
Serial Read in blocks of 10 bytes:
Serial PEEK before trying to read a chunk of data is 50 ['2']
23456789ab
Serial PEEK is 99 ['c']
cdefghijkl
Serial PEEK is 109 ['m']
mnopqrstuv
Serial PEEK is 119 ['w']
wxyz
Serial PEEK is -1 ['⸮']
===============================================

Related links

Fixes #7505
Fixes #7474

May Fix #6644
May Fix #6727

cores/esp32/esp32-hal-uart.c

mrengineer7777

In esp32-hal-uart.c->uartRead(), int len = uart_read_bytes(uart->num, &c, 1, 20 / portTICK_RATE_MS); can return negative.

mrengineer7777

PR drops call to available(). Likely OK, but need to test both HardwareSerial::read(void) and HardwareSerial::read(uint8_t *buffer, size_t size) when no data available to see if there are any side effects with calling IDF function uart_read_bytes() with 0 timeout.

PR looks promising. I see no other issues.

SuGlider · 2022-11-28T14:57:31Z

PR drops call to available(). Likely OK, but need to test both HardwareSerial::read(void) and HardwareSerial::read(uint8_t *buffer, size_t size) when no data available to see if there are any side effects with calling IDF function uart_read_bytes() with 0 timeout.

PR looks promising. I see no other issues.

I tested it using the example sketch of this PR.
I sent just "0", "01", "012" and "0123".
All of them try to read or peek from empty RX buffer at different moments.
Everything works as expected.

SuGlider · 2022-11-28T15:04:32Z

PR drops call to available(). Likely OK, but need to test both HardwareSerial::read(void) and HardwareSerial::read(uint8_t *buffer, size_t size) when no data available to see if there are any side effects with calling IDF function uart_read_bytes() with 0 timeout.

Sketch Example has been updated to test return of read(buffer) when there is no data to be read.

P-R-O-C-H-Y · 2022-11-30T10:46:17Z

@SuGlider I am getting this output from the sketch on ESP32-S2 (did not test other chips yet).

Serial has 36 bytes available.
Serial PEEK is 0
Serial Read 1 byte after Peek: 48 ['0']
Serial Read again 1 byte after First Read: 49 ['1']
Serial Read in blocks of 10 bytes:
Serial PEEK before trying to read a chunk of data is 50 ['2']
Serial PEEK is 99 ['c']
23456789ab
Serial PEEK is 109 ['m']
cdefghijkl
Serial PEEK is 119 ['w']
mnopqrstuv
Serial PEEK is -1 ['⸮']
wxyz
===============================================

Is that right? If yes maybe update the expected OUTPUT :)

P-R-O-C-H-Y · 2022-11-30T10:57:42Z

Also did a quick test of the performance of the reading as the sketch is written (only Serial.prints removed).

Before changes reading 36 bytes took 316-325 us.
After changes reading 36 bytes took 118-126 us.

Seems a really nice speed improvement. Great PR @SuGlider

SuGlider · 2022-12-04T13:52:29Z

New commit e072302 overrides Stream::readBytes() to make it faster with IDF.

SuGlider · 2022-12-04T14:01:03Z

@SuGlider I am getting this output from the sketch on ESP32-S2 (did not test other chips yet).

Serial has 36 bytes available.
Serial PEEK is 0
Serial Read 1 byte after Peek: 48 ['0']
Serial Read again 1 byte after First Read: 49 ['1']
Serial Read in blocks of 10 bytes:
Serial PEEK before trying to read a chunk of data is 50 ['2']
Serial PEEK is 99 ['c']
23456789ab
Serial PEEK is 109 ['m']
cdefghijkl
Serial PEEK is 119 ['w']
mnopqrstuv
Serial PEEK is -1 ['⸮']
wxyz
===============================================

Is that right? If yes maybe update the expected OUTPUT :)

Thanks. Example and Output are now updated.

mrengineer7777 · 2022-12-05T22:53:07Z

@SuGlider See my review comments above, especially on readBytes().

SuGlider · 2022-12-06T12:56:43Z

@SuGlider See my review comments above, especially on readBytes().

This one #7525 (review)?

SuGlider · 2022-12-06T12:59:51Z

In esp32-hal-uart.c->uartRead(), int len = uart_read_bytes(uart->num, &c, 1, 20 / portTICK_RATE_MS); can return negative.

The code already treats negative return:

if (size > 0) {
       int len = uart_read_bytes(uart->num, buffer, size, pdMS_TO_TICKS(timeout_ms));
       if (len < 0) len = 0;  // error reading UART   <<<=======================Here
       bytes_read += len;
    }

mrengineer7777 · 2022-12-06T15:10:20Z

@SuGlider Apologies, Github is being weird about my review notes and is showing review as pending (but not posted?). I'll cancel the review, look over the current code and just comment here.

mrengineer7777 · 2022-12-06T15:24:26Z

On cores/esp32/HardwareSerial.h readBytes(char *buffer, size_t length), should this be uint8_t instead of char? recommend: return readBytes((uint8_t *)buffer, length);

On cores/esp32/esp32-hal-uart.c uint8_t uartRead(uart_t* uart) (not currently modified by PR) uart_read_bytes() can return negative. Recommend changing if (len == 0) { to if(len <= 0) {. uartPeek() may have the same problem.

On cores/esp32/esp32-hal-uart.c uint8_t uartRead(uart_t* uart) I recommend the following comments on the function:

//Reads one byte from uart.
//Check uartAvailable() > 0 before calling uartRead().

Also consider marking uartRead() as //Deprecated. Use uartReadBytes() instead.

SuGlider · 2022-12-06T19:36:10Z

On cores/esp32/HardwareSerial.h readBytes(char *buffer, size_t length), should this be uint8_t instead of char? recommend: return readBytes((uint8_t *)buffer, length);

On cores/esp32/esp32-hal-uart.c uint8_t uartRead(uart_t* uart) (not currently modified by PR) uart_read_bytes() can return negative. Recommend changing if (len == 0) { to if(len <= 0) {. uartPeek() may have the same problem.

On cores/esp32/esp32-hal-uart.c uint8_t uartRead(uart_t* uart) I recommend the following comments on the function:
//Reads one byte from uart.
//Check uartAvailable() > 0 before calling uartRead().
Also consider marking uartRead() as //Deprecated. Use uartReadBytes() instead.

It is a bit confusing to review code this way. Do you mind using the traditional way as below:

When reviewing it within GitHub, you can use 2 buttons:

The Red Arrow will insert the review immediately.
The Yellow Arrow, adds the review, but it will be published only when all the review is submitted, by pressing the Yellow marked button below (in the top of the GH web page):

SuGlider · 2022-12-06T19:57:51Z

Thanks @mrengineer7777 for the review.

On cores/esp32/HardwareSerial.h readBytes(char *buffer, size_t length), should this be uint8_t instead of char? recommend: return readBytes((uint8_t *)buffer, length);

Yes, correct. Fixed now.

On cores/esp32/esp32-hal-uart.c uint8_t uartRead(uart_t* uart) (not currently modified by PR) uart_read_bytes() can return negative. Recommend changing if (len == 0) { to if(len <= 0) {. uartPeek() may have the same problem.

Yes, noted. Both functions are now fixed.

On cores/esp32/esp32-hal-uart.c uint8_t uartRead(uart_t* uart) I recommend the following comments on the function:
//Reads one byte from uart.
//Check uartAvailable() > 0 before calling uartRead().
Also consider marking uartRead() as //Deprecated. Use uartReadBytes() instead.

Actually the functions exposed in esp32-hal-uart.c shall not be used by the application.
This is a middleware to connect IDF to Arduino API.

mrengineer7777 · 2022-12-07T15:25:46Z

@SuGlider Apologies for not replying sooner. Hectic day at work yesterday. Your explanation on Github review buttons is very helpful.

Just read through all the code changes again. With your latest commits PR looks good to me.

SuGlider · 2022-12-07T17:19:20Z

@SuGlider Apologies for not replying sooner. Hectic day at work yesterday. Your explanation on Github review buttons is very helpful.

Just read through all the code changes again. With your latest commits PR looks good to me.

@mrengineer7777 - Thank you for the code review!

Improves UART reading performance

8701c88

SuGlider added Area: Performance Issue related to performance problems and improvements Area: Peripherals API Relates to peripheral's APIs. Peripheral: UART labels Nov 27, 2022

SuGlider added this to the 2.0.6 milestone Nov 27, 2022

SuGlider requested review from me-no-dev, PilnyTomas and P-R-O-C-H-Y November 27, 2022 20:16

SuGlider self-assigned this Nov 27, 2022

This was referenced Nov 27, 2022

Possible regression of serial communication performance since version 2.x #7505

Closed

Possible performance improvement to HardwareSerial::Read #7474

Closed

TD-er reviewed Nov 28, 2022

View reviewed changes

cores/esp32/esp32-hal-uart.c Show resolved Hide resolved

mrengineer7777 suggested changes Nov 28, 2022

View reviewed changes

mrengineer7777 reviewed Nov 28, 2022

View reviewed changes

awawa-dev mentioned this pull request Nov 28, 2022

Ensure serial communication compatibility with the upcoming Arduino 2.0.6 awawa-dev/HyperSerialESP32#7

Merged

P-R-O-C-H-Y approved these changes Nov 30, 2022

View reviewed changes

overrides Stream::readBytes()

e072302

This was referenced Dec 4, 2022

error in updating Firmware OTA on esp32 using LTE module #6644

Closed

Problem with receiving data from Serial to update ESP32 OTA #6727

Closed

Merge branch 'master' into uart_read_faster

08f668d

SuGlider added 2 commits December 6, 2022 16:42

fixes override signature

6ef09b6

adds some IDF error return conditions

d37a199

SuGlider added 2 commits December 8, 2022 09:13

Merge branch 'master' into uart_read_faster

638d0ff

Merge branch 'master' into uart_read_faster

d00d4c3

me-no-dev approved these changes Dec 13, 2022

View reviewed changes

me-no-dev merged commit 7cbd037 into espressif:master Dec 13, 2022

mrengineer7777 mentioned this pull request Apr 13, 2023

Why is there a large drop in performance between 2.0.5 and 2.0.6? #8019

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves UART reading performance #7525

Improves UART reading performance #7525

SuGlider commented Nov 27, 2022 •

edited

Loading

mrengineer7777 left a comment

mrengineer7777 left a comment

SuGlider commented Nov 28, 2022

SuGlider commented Nov 28, 2022

P-R-O-C-H-Y commented Nov 30, 2022

P-R-O-C-H-Y commented Nov 30, 2022

SuGlider commented Dec 4, 2022

SuGlider commented Dec 4, 2022

mrengineer7777 commented Dec 5, 2022

SuGlider commented Dec 6, 2022 •

edited

Loading

SuGlider commented Dec 6, 2022

mrengineer7777 commented Dec 6, 2022 •

edited

Loading

mrengineer7777 commented Dec 6, 2022 •

edited

Loading

SuGlider commented Dec 6, 2022 •

edited

Loading

SuGlider commented Dec 6, 2022 •

edited

Loading

mrengineer7777 commented Dec 7, 2022

SuGlider commented Dec 7, 2022

Improves UART reading performance #7525

Improves UART reading performance #7525

Conversation

SuGlider commented Nov 27, 2022 • edited Loading

Description of Change

Tests scenarios

Related links

mrengineer7777 left a comment

Choose a reason for hiding this comment

mrengineer7777 left a comment

Choose a reason for hiding this comment

SuGlider commented Nov 28, 2022

SuGlider commented Nov 28, 2022

P-R-O-C-H-Y commented Nov 30, 2022

P-R-O-C-H-Y commented Nov 30, 2022

SuGlider commented Dec 4, 2022

SuGlider commented Dec 4, 2022

mrengineer7777 commented Dec 5, 2022

SuGlider commented Dec 6, 2022 • edited Loading

SuGlider commented Dec 6, 2022

mrengineer7777 commented Dec 6, 2022 • edited Loading

mrengineer7777 commented Dec 6, 2022 • edited Loading

SuGlider commented Dec 6, 2022 • edited Loading

SuGlider commented Dec 6, 2022 • edited Loading

mrengineer7777 commented Dec 7, 2022

SuGlider commented Dec 7, 2022

SuGlider commented Nov 27, 2022 •

edited

Loading

SuGlider commented Dec 6, 2022 •

edited

Loading

mrengineer7777 commented Dec 6, 2022 •

edited

Loading

mrengineer7777 commented Dec 6, 2022 •

edited

Loading

SuGlider commented Dec 6, 2022 •

edited

Loading

SuGlider commented Dec 6, 2022 •

edited

Loading