uart: BW improvements (#4620)

* uart fixes and BW improvements

* uart: read_char straightly use hw buffer

* +attributes for functions called by ISR

* uart: BW improvements
read_char straightly use hw buffer (+ ~10%bw)
read by block (+ ~190%bw) (instead of generic Stream::readBytes)
attributes for functions called by ISR
remove overrun message
remove some ISR flags which were not honoured

* fix merge

* fix buffer overflow

* serial stress test sketch

* astyle

* serial stress example: interactive keyboard, stop reading, overrun

* serial device test: bandwidth & overrun

* update + HardwareSerial::hasError()

* interactive overrun in example

* astyle

* Test using @plerup's SoftwareSerial as submodule (tag 3.4.1)

* update upstream ref (fix warning)

* host mock uart/read(buf,size)

* reset style changes in submodules before style diff

* update build_boards_manager_package.sh for submodules

* trigger CI (removing space)

* cannot reproduce locally the CI issue, setting bash -x option to get live trace

* remove previously added (in this PR) 'set -e' in package builder (passes local tests, not real CI)
script-comment new recipe.hooks.core.prebuild.3 (along with already commented .1 and .2)
moved CI package test to be first on the test list
remove 'set -x', wish me luck

Author: d-a-v

Diff for: .gitmodules

@@ -4,3 +4,6 @@
 [submodule "tools/sdk/ssl/bearssl"]
 	path = tools/sdk/ssl/bearssl
 	url = https://github.com./earlephilhower/bearssl-esp8266
+[submodule "libraries/SoftwareSerial"]
+	path = libraries/SoftwareSerial
+	url = https://github.com./plerup/espsoftwareserial.git

Diff for: .travis.yml

@@ -8,6 +8,8 @@ cache:
 
 matrix:
   include:
+    - env:
+      - BUILD_TYPE=package
     - env:
       - BUILD_TYPE=build_even
     - env:
@@ -22,8 +24,6 @@ matrix:
       - BUILD_TYPE=platformio_odd
     - env:
       - BUILD_TYPE=docs
-    - env:
-      - BUILD_TYPE=package
     - env:
       - BUILD_TYPE=host_tests
     - env:

Diff for: cores/esp8266/HardwareSerial.h

@@ -88,6 +88,10 @@ class HardwareSerial: public Stream
     void end();
 
     size_t setRxBufferSize(size_t size);
+    size_t getRxBufferSize()
+    {
+        return uart_get_rx_buffer_size(_uart);
+    }
 
     void swap()
     {
@@ -120,14 +124,22 @@ class HardwareSerial: public Stream
 
     int peek(void) override
     {
-        // this may return -1, but that's okay
+        // return -1 when data is unvailable (arduino api)
         return uart_peek_char(_uart);
     }
     int read(void) override
     {
-        // this may return -1, but that's okay
+        // return -1 when data is unvailable (arduino api)
         return uart_read_char(_uart);
     }
+    size_t readBytes(char* buffer, size_t size) override
+    {
+        return uart_read(_uart, buffer, size);
+    }
+    size_t readBytes(uint8_t* buffer, size_t size) override
+    {
+        return uart_read(_uart, (char*)buffer, size);
+    }
     int availableForWrite(void)
     {
         return static_cast<int>(uart_tx_free(_uart));
@@ -184,6 +196,11 @@ class HardwareSerial: public Stream
         return uart_has_overrun(_uart);
     }
 
+    bool hasRxError(void)
+    {
+        return uart_has_rx_error(_uart);
+    }
+
     void startDetectBaudrate();
 
     unsigned long testBaudrate();

Diff for: cores/esp8266/uart.c

@@ -47,11 +47,11 @@
 #include "user_interface.h"
 #include "uart_register.h"
 
-const char overrun_str [] PROGMEM STORE_ATTR = "uart input full!\r\n";
+//const char overrun_str [] PROGMEM STORE_ATTR = "uart input full!\r\n";
 static int s_uart_debug_nr = UART0;
 
 
-struct uart_rx_buffer_ 
+struct uart_rx_buffer_
 {
     size_t size;
     size_t rpos;
@@ -65,7 +65,8 @@ struct uart_
     int baud_rate;
     bool rx_enabled;
     bool tx_enabled;
-    bool overrun;
+    bool rx_overrun;
+    bool rx_error;
     uint8_t rx_pin;
     uint8_t tx_pin;
     struct uart_rx_buffer_ * rx_buffer;
@@ -85,7 +86,8 @@ struct uart_
 
 
 
-inline size_t 
+// called by ISR
+inline size_t ICACHE_RAM_ATTR
 uart_rx_fifo_available(const int uart_nr) 
 {
     return (USS(uart_nr) >> USRXC) & 0xFF;
@@ -110,11 +112,11 @@ uart_rx_available_unsafe(uart_t* uart)
     return uart_rx_buffer_available_unsafe(uart->rx_buffer) + uart_rx_fifo_available(uart->uart_nr);
 }
 
-
 //#define UART_DISCARD_NEWEST
 
 // Copy all the rx fifo bytes that fit into the rx buffer
-inline void 
+// called by ISR
+inline void ICACHE_RAM_ATTR
 uart_rx_copy_fifo_to_buffer_unsafe(uart_t* uart) 
 {
     struct uart_rx_buffer_ *rx_buffer = uart->rx_buffer;
@@ -124,11 +126,10 @@ uart_rx_copy_fifo_to_buffer_unsafe(uart_t* uart)
         size_t nextPos = (rx_buffer->wpos + 1) % rx_buffer->size;
         if(nextPos == rx_buffer->rpos) 
         {
-
-            if (!uart->overrun) 
+            if (!uart->rx_overrun)
             {
-                uart->overrun = true;
-                os_printf_plus(overrun_str);
+                uart->rx_overrun = true;
+                //os_printf_plus(overrun_str);
             }
 
             // a choice has to be made here,
@@ -158,25 +159,27 @@ uart_peek_char_unsafe(uart_t* uart)
 
     //without the following if statement and body, there is a good chance of a fifo overrun
     if (uart_rx_buffer_available_unsafe(uart->rx_buffer) == 0)
+        // hw fifo can't be peeked, data need to be copied to sw
         uart_rx_copy_fifo_to_buffer_unsafe(uart);
 
     return uart->rx_buffer->buffer[uart->rx_buffer->rpos];
 }
 
-inline int 
+// taking data straight from hw fifo: loopback-test BW jumps by 19%
+inline int
 uart_read_char_unsafe(uart_t* uart)
 {
-    int data = uart_peek_char_unsafe(uart);
-    if(data != -1)
+    if (uart_rx_buffer_available_unsafe(uart->rx_buffer))
+    {
+        // take oldest sw data
+        int ret = uart->rx_buffer->buffer[uart->rx_buffer->rpos];
         uart->rx_buffer->rpos = (uart->rx_buffer->rpos + 1) % uart->rx_buffer->size;
-    return data;
+        return ret;
+    }
+    // unavailable
+    return -1;
 }
 
-
-/**********************************************************/
-
-
-
 size_t 
 uart_rx_available(uart_t* uart)
 {
@@ -204,14 +207,47 @@ uart_peek_char(uart_t* uart)
 
 int 
 uart_read_char(uart_t* uart)
+{
+    uint8_t ret;
+    return uart_read(uart, (char*)&ret, 1)? ret: -1;
+}
+
+// loopback-test BW jumps by 190%
+size_t
+uart_read(uart_t* uart, char* userbuffer, size_t usersize)
 {
     if(uart == NULL || !uart->rx_enabled)
-        return -1;
-    
+        return 0;
+
+    size_t ret = 0;
     ETS_UART_INTR_DISABLE();
-    int data = uart_read_char_unsafe(uart);
+
+    while (ret < usersize && uart_rx_available_unsafe(uart))
+    {
+        if (!uart_rx_buffer_available_unsafe(uart->rx_buffer))
+        {
+            // no more data in sw buffer, take them from hw fifo
+            while (ret < usersize && uart_rx_fifo_available(uart->uart_nr))
+                userbuffer[ret++] = USF(uart->uart_nr);
+
+	    // no more sw/hw data available
+            break;
+        }
+
+        // pour sw buffer to user's buffer
+        // get largest linear length from sw buffer
+        size_t chunk = uart->rx_buffer->rpos < uart->rx_buffer->wpos?
+                           uart->rx_buffer->wpos - uart->rx_buffer->rpos:
+                           uart->rx_buffer->size - uart->rx_buffer->rpos;
+        if (ret + chunk > usersize)
+            chunk = usersize - ret;
+        memcpy(userbuffer + ret, uart->rx_buffer->buffer + uart->rx_buffer->rpos, chunk);
+        uart->rx_buffer->rpos = (uart->rx_buffer->rpos + chunk) % uart->rx_buffer->size;
+        ret += chunk;
+    }
+
     ETS_UART_INTR_ENABLE();
-    return data;
+    return ret;
 }
 
 size_t 
@@ -231,6 +267,8 @@ uart_resize_rx_buffer(uart_t* uart, size_t new_size)
     ETS_UART_INTR_DISABLE();
     while(uart_rx_available_unsafe(uart) && new_wpos < new_size)
         new_buf[new_wpos++] = uart_read_char_unsafe(uart); //if uart_rx_available_unsafe() returns non-0, uart_read_char_unsafe() can't return -1
+    if (new_wpos == new_size)
+        new_wpos = 0;
 
     uint8_t * old_buf = uart->rx_buffer->buffer;
     uart->rx_buffer->rpos = 0;
@@ -242,22 +280,39 @@ uart_resize_rx_buffer(uart_t* uart, size_t new_size)
     return uart->rx_buffer->size;
 }
 
+size_t
+uart_get_rx_buffer_size(uart_t* uart)
+{
+    return uart && uart->rx_enabled? uart->rx_buffer->size: 0;
+}
 
 
 void ICACHE_RAM_ATTR 
 uart_isr(void * arg)
 {
     uart_t* uart = (uart_t*)arg;
+    uint32_t usis = USIS(uart->uart_nr);
+
     if(uart == NULL || !uart->rx_enabled) 
     {
-        USIC(uart->uart_nr) = USIS(uart->uart_nr);
+        USIC(uart->uart_nr) = usis;
         ETS_UART_INTR_DISABLE();
         return;
     }
-    if(USIS(uart->uart_nr) & ((1 << UIFF) | (1 << UITO)))
+
+    if(usis & (1 << UIFF))
         uart_rx_copy_fifo_to_buffer_unsafe(uart);
+
+    if((usis & (1 << UIOF)) && !uart->rx_overrun)
+    {
+        uart->rx_overrun = true;
+        //os_printf_plus(overrun_str);
+    }
 
-    USIC(uart->uart_nr) = USIS(uart->uart_nr);
+    if (usis & ((1 << UIFR) | (1 << UIPE) | (1 << UITO)))
+        uart->rx_error = true;
+
+    USIC(uart->uart_nr) = usis;
 }
 
 static void 
@@ -270,9 +325,22 @@ uart_start_isr(uart_t* uart)
     // triggers the IRS very often.  A value of 127 would not leave much time
     // for ISR to clear fifo before the next byte is dropped.  So pick a value
     // in the middle.
-    USC1(uart->uart_nr) = (100   << UCFFT) | (0x02 << UCTOT) | (1 <<UCTOE );
+    // update: loopback test @ 3Mbauds/8n1 (=2343Kibits/s):
+    // - 4..120 give > 2300Kibits/s
+    // - 1, 2, 3 are below
+    // was 100, use 16 to stay away from overrun
+    #define INTRIGG 16
+
+    //was:USC1(uart->uart_nr) = (INTRIGG << UCFFT) | (0x02 << UCTOT) | (1 <<UCTOE);
+    USC1(uart->uart_nr) = (INTRIGG << UCFFT);
     USIC(uart->uart_nr) = 0xffff;
-    USIE(uart->uart_nr) = (1 << UIFF) | (1 << UIFR) | (1 << UITO);
+    //was: USIE(uart->uart_nr) = (1 << UIFF) | (1 << UIFR) | (1 << UITO);
+    // UIFF: rx fifo full
+    // UIOF: rx fifo overflow (=overrun)
+    // UIFR: frame error
+    // UIPE: parity error
+    // UITO: rx fifo timeout
+    USIE(uart->uart_nr) = (1 << UIFF) | (1 << UIOF) | (1 << UIFR) | (1 << UIPE) | (1 << UITO);
     ETS_UART_INTR_ATTACH(uart_isr,  (void *)uart);
     ETS_UART_INTR_ENABLE();
 }
@@ -415,7 +483,8 @@ uart_init(int uart_nr, int baudrate, int config, int mode, int tx_pin, size_t rx
         return NULL;
 
     uart->uart_nr = uart_nr;
-    uart->overrun = false;
+    uart->rx_overrun = false;
+    uart->rx_error = false;
 
     switch(uart->uart_nr) 
     {
@@ -678,11 +747,22 @@ uart_rx_enabled(uart_t* uart)
 bool 
 uart_has_overrun (uart_t* uart)
 {
-    if (uart == NULL || !uart->overrun)
+    if (uart == NULL || !uart->rx_overrun)
+        return false;
+
+    // clear flag
+    uart->rx_overrun = false;
+    return true;
+}
+
+bool
+uart_has_rx_error (uart_t* uart)
+{
+    if (uart == NULL || !uart->rx_error)
         return false;
 
     // clear flag
-    uart->overrun = false;
+    uart->rx_error = false;
     return true;
 }
 

Diff for: cores/esp8266/uart.h

@@ -126,17 +126,20 @@ void uart_set_baudrate(uart_t* uart, int baud_rate);
 int uart_get_baudrate(uart_t* uart);
 
 size_t uart_resize_rx_buffer(uart_t* uart, size_t new_size);
+size_t uart_get_rx_buffer_size(uart_t* uart);
 
 size_t uart_write_char(uart_t* uart, char c);
 size_t uart_write(uart_t* uart, const char* buf, size_t size);
 int uart_read_char(uart_t* uart);
 int uart_peek_char(uart_t* uart);
+size_t uart_read(uart_t* uart, char* buffer, size_t size);
 size_t uart_rx_available(uart_t* uart);
 size_t uart_tx_free(uart_t* uart);
 void uart_wait_tx_empty(uart_t* uart);
 void uart_flush(uart_t* uart);
 
 bool uart_has_overrun (uart_t* uart); // returns then clear overrun flag
+bool uart_has_rx_error (uart_t* uart); // returns then clear rxerror flag
 
 void uart_set_debug(int uart_nr);
 int uart_get_debug();