Merge pull request #1320 from alltheblinkythings/SerialInterframeFixes

Fixes for poor HardwareSerial performance/crashes exacerbated by SDK1.5

cores/esp8266/HardwareSerial.cpp

@@ -101,7 +101,8 @@ void uart_disarm_tx_interrupt(uart_t* uart);
 void uart_set_baudrate(uart_t* uart, int baud_rate);
 int uart_get_baudrate(uart_t* uart);
 
-uart_t* uart_init(int uart_nr, int baudrate, byte config);
+uart_t* uart_start_init(int uart_nr, int baudrate, byte config);
+void uart_finish_init(uart_t* uart);
 void uart_uninit(uart_t* uart);
 void uart_swap(uart_t* uart);
 
@@ -116,6 +117,13 @@ int uart_get_debug();
 // ####################################################################################################
 // ####################################################################################################
 
+// These function internals can be used from interrupt handlers to ensure they
+// are in instruction RAM, or anywhere that the uart_nr has been validated.
+#define UART_GET_TX_FIFO_ROOM(uart_nr)     (UART_TX_FIFO_SIZE - ((USS(uart_nr) >> USTXC) & 0xff))
+#define UART_TRANSMIT_CHAR(uart_nr, c)     do { USF(uart_nr) = (c); } while(0)
+#define UART_ARM_TX_INTERRUPT(uart_nr)     do { USIE(uart_nr) |= (1 << UIFE); } while(0)
+#define UART_DISARM_TX_INTERRUPT(uart_nr)  do { USIE(uart_nr) &= ~(1 << UIFE); } while(0)
+
 void ICACHE_RAM_ATTR uart_interrupt_handler(uart_t* uart) {
 
     // -------------- UART 0 --------------
@@ -133,13 +141,7 @@ void ICACHE_RAM_ATTR uart_interrupt_handler(uart_t* uart) {
     }
 
     // -------------- UART 1 --------------
-
+    // Note: only TX is supported on UART 1.
-    if(Serial1.isRxEnabled()) {
-        while(U1IS & (1 << UIFF)) {
-            Serial1._rx_complete_irq((char) (U1F & 0xff));
-            U1IC = (1 << UIFF);
-        }
-    }
     if(Serial1.isTxEnabled()) {
         if(U1IS & (1 << UIFE)) {
             U1IC = (1 << UIFE);
@@ -166,7 +168,7 @@ size_t uart_get_tx_fifo_room(uart_t* uart) {
     if(uart == 0)
         return 0;
     if(uart->txEnabled) {
-        return UART_TX_FIFO_SIZE - ((USS(uart->uart_nr) >> USTXC) & 0xff);
+        return UART_GET_TX_FIFO_ROOM(uart->uart_nr);
     }
     return 0;
 }
@@ -183,7 +185,7 @@ void uart_transmit_char(uart_t* uart, char c) {
     if(uart == 0)
         return;
     if(uart->txEnabled) {
-        USF(uart->uart_nr) = c;
+        UART_TRANSMIT_CHAR(uart->uart_nr, c);
     }
 }
 
@@ -247,7 +249,7 @@ void uart_arm_tx_interrupt(uart_t* uart) {
     if(uart == 0)
         return;
     if(uart->txEnabled) {
-        USIE(uart->uart_nr) |= (1 << UIFE);
+        UART_ARM_TX_INTERRUPT(uart->uart_nr);
     }
 }
 
@@ -255,7 +257,7 @@ void uart_disarm_tx_interrupt(uart_t* uart) {
     if(uart == 0)
         return;
     if(uart->txEnabled) {
-        USIE(uart->uart_nr) &= ~(1 << UIFE);
+        UART_DISARM_TX_INTERRUPT(uart->uart_nr);
     }
 }
 
@@ -272,9 +274,8 @@ int uart_get_baudrate(uart_t* uart) {
     return uart->baud_rate;
 }
 
-uart_t* uart_init(int uart_nr, int baudrate, byte config, byte mode) {
+uart_t* uart_start_init(int uart_nr, int baudrate, byte config, byte mode) {
 
-    uint32_t conf1 = 0x00000000;
     uart_t* uart = (uart_t*) os_malloc(sizeof(uart_t));
 
     if(uart == 0) {
@@ -294,6 +295,7 @@ uart_t* uart_init(int uart_nr, int baudrate, byte config, byte mode) {
             IOSWAP &= ~(1 << IOSWAPU0);
             break;
         case UART1:
+            // Note: uart_interrupt_handler does not support RX on UART 1.
             uart->rxEnabled = false;
             uart->txEnabled = (mode != SERIAL_RX_ONLY);
             uart->rxPin = 255;
@@ -309,6 +311,12 @@ uart_t* uart_init(int uart_nr, int baudrate, byte config, byte mode) {
     uart_set_baudrate(uart, baudrate);
     USC0(uart->uart_nr) = config;
 
+    return uart;
+}
+
+void uart_finish_init(uart_t* uart) {
+    uint32_t conf1 = 0x00000000;
+
     uart_flush(uart);
     uart_interrupt_enable(uart);
 
@@ -321,8 +329,6 @@ uart_t* uart_init(int uart_nr, int baudrate, byte config, byte mode) {
     }
 
     USC1(uart->uart_nr) = conf1;
-
-    return uart;
 }
 
 void uart_uninit(uart_t* uart) {
@@ -479,35 +485,48 @@ int uart_get_debug() {
 // ####################################################################################################
 
 HardwareSerial::HardwareSerial(int uart_nr) :
-        _uart_nr(uart_nr), _uart(0), _tx_buffer(0), _rx_buffer(0), _written(false) {
+        _uart_nr(uart_nr), _uart(0), _tx_buffer(0), _rx_buffer(0) {
 }
 
 void HardwareSerial::begin(unsigned long baud, byte config, byte mode) {
+    InterruptLock il;
 
     // disable debug for this interface
     if(uart_get_debug() == _uart_nr) {
         uart_set_debug(UART_NO);
     }
 
-    _uart = uart_init(_uart_nr, baud, config, mode);
+    if (_uart) {
+        os_free(_uart);
+    }
+    _uart = uart_start_init(_uart_nr, baud, config, mode);
 
     if(_uart == 0) {
         return;
     }
 
-    if(_uart->rxEnabled) {
+    // Disable the RX and/or TX functions if we fail to allocate circular buffers.
-        if(!_rx_buffer)
+    // The user can confirm they are enabled with isRxEnabled() and isTxEnabled().
-            _rx_buffer = new cbuf(SERIAL_RX_BUFFER_SIZE);
+    if(_uart->rxEnabled && !_rx_buffer) {
+        _rx_buffer = new cbuf(SERIAL_RX_BUFFER_SIZE);
+        if(!_rx_buffer) {
+            _uart->rxEnabled = false;
+        }
     }
-    if(_uart->txEnabled) {
+    if(_uart->txEnabled && !_tx_buffer) {
-        if(!_tx_buffer)
+        _tx_buffer = new cbuf(SERIAL_TX_BUFFER_SIZE);
-            _tx_buffer = new cbuf(SERIAL_TX_BUFFER_SIZE);
+        if(!_tx_buffer) {
+            _uart->txEnabled = false;
+        }
     }
-    _written = false;
     delay(1);
+
+    uart_finish_init(_uart);
 }
 
 void HardwareSerial::end() {
+    InterruptLock il;
+
     if(uart_get_debug() == _uart_nr) {
         uart_set_debug(UART_NO);
     }
@@ -541,13 +560,13 @@ void HardwareSerial::setDebugOutput(bool en) {
     }
 }
 
-bool HardwareSerial::isTxEnabled(void) {
+bool ICACHE_RAM_ATTR HardwareSerial::isTxEnabled(void) {
     if(_uart == 0)
         return false;
     return _uart->txEnabled;
 }
 
-bool HardwareSerial::isRxEnabled(void) {
+bool ICACHE_RAM_ATTR HardwareSerial::isRxEnabled(void) {
     if(_uart == 0)
         return false;
     return _uart->rxEnabled;
@@ -606,44 +625,52 @@ void HardwareSerial::flush() {
         return;
     if(!_uart->txEnabled)
         return;
-    if(!_written)
-        return;
 
+    const int uart_nr = _uart->uart_nr;
     while(true) {
         {
             InterruptLock il;
             if(_tx_buffer->getSize() == 0 &&
-               uart_get_tx_fifo_room(_uart) >= UART_TX_FIFO_SIZE) {
+               UART_GET_TX_FIFO_ROOM(uart_nr) >= UART_TX_FIFO_SIZE) {
                 break;
+            } else if(il.savedInterruptLevel() > 0) {
+                _tx_empty_irq();
+                continue;
             }
         }
         yield();
     }
-    _written = false;
 }
 
 size_t HardwareSerial::write(uint8_t c) {
     if(_uart == 0 || !_uart->txEnabled)
         return 0;
-    _written = true;
 
+    bool tx_now = false;
+    const int uart_nr = _uart->uart_nr;
     while(true) {
         {
             InterruptLock il;
             if(_tx_buffer->empty()) {
-                if(uart_get_tx_fifo_room(_uart) > 0) {
+                if(UART_GET_TX_FIFO_ROOM(uart_nr) > 0) {
-                    uart_transmit_char(_uart, c);
+                    tx_now = true;
                 } else {
                     _tx_buffer->write(c);
-                    uart_arm_tx_interrupt(_uart);
+                    UART_ARM_TX_INTERRUPT(uart_nr);
                 }
                 break;
             } else if(_tx_buffer->write(c)) {
                 break;
+            } else if(il.savedInterruptLevel() > 0) {
+                _tx_empty_irq();
+                continue;
             }
         }
         yield();
     }
+    if (tx_now) {
+        UART_TRANSMIT_CHAR(uart_nr, c);
+    }
     return 1;
 }
 
@@ -651,26 +678,21 @@ HardwareSerial::operator bool() const {
     return _uart != 0;
 }
 
-void HardwareSerial::_rx_complete_irq(char c) {
+void ICACHE_RAM_ATTR HardwareSerial::_rx_complete_irq(char c) {
-    if(_rx_buffer) {
+    _rx_buffer->write(c);
-        _rx_buffer->write(c);
-    }
 }
 
-void HardwareSerial::_tx_empty_irq(void) {
+void ICACHE_RAM_ATTR HardwareSerial::_tx_empty_irq(void) {
-    if(_uart == 0)
+    const int uart_nr = _uart->uart_nr;
-        return;
-    if(_tx_buffer == 0)
-        return;
     size_t queued = _tx_buffer->getSize();
     if(!queued) {
-        uart_disarm_tx_interrupt(_uart);
+        UART_DISARM_TX_INTERRUPT(uart_nr);
         return;
     }
 
-    size_t room = uart_get_tx_fifo_room(_uart);
+    size_t room = UART_GET_TX_FIFO_ROOM(uart_nr);
     int n = static_cast<int>((queued < room) ? queued : room);
     while(n--) {
-        uart_transmit_char(_uart, _tx_buffer->read());
+        UART_TRANSMIT_CHAR(uart_nr, _tx_buffer->read());
     }
 }

cores/esp8266/HardwareSerial.h

@@ -117,7 +117,6 @@ class HardwareSerial: public Stream {
         uart_t* _uart;
         cbuf* _tx_buffer;
         cbuf* _rx_buffer;
-        bool _written;
 };
 
 extern HardwareSerial Serial;

cores/esp8266/cbuf.cpp

@@ -0,0 +1,45 @@
+/* 
+ cbuf.cpp - Circular buffer implementation
+ Copyright (c) 2014 Ivan Grokhotkov. All rights reserved.
+ This file is part of the esp8266 core for Arduino environment.
+ 
+ This library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ This library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with this library; if not, write to the Free Software
+ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include "cbuf.h"
+#include "c_types.h"
+
+size_t ICACHE_RAM_ATTR cbuf::getSize() const {
+    if(_end >= _begin) {
+        return _end - _begin;
+    }
+    return _size - (_begin - _end);
+}
+
+int ICACHE_RAM_ATTR cbuf::read() {
+    if(empty()) return -1;
+
+    char result = *_begin;
+    _begin = wrap_if_bufend(_begin + 1);
+    return static_cast<int>(result);
+}
+
+size_t ICACHE_RAM_ATTR cbuf::write(char c) {
+     if(full()) return 0;
+
+     *_end = c;
+     _end = wrap_if_bufend(_end + 1);
+     return 1;
+}

cores/esp8266/cbuf.h

@@ -21,7 +21,10 @@
 #ifndef __cbuf_h
 #define __cbuf_h
 
+#include <stddef.h>
 #include <stdint.h>
+#include <string.h>
+
 class cbuf {
     public:
         cbuf(size_t size) :
@@ -32,11 +35,7 @@ class cbuf {
             delete[] _buf;
         }
 
-        size_t getSize() const {
+        size_t getSize() const;
-            if(_end >= _begin) return _end - _begin;
-
-            return _size - (_begin - _end);
-        }
 
         size_t room() const {
             if(_end >= _begin) {
@@ -45,23 +44,21 @@ class cbuf {
             return _begin - _end - 1;
         }
 
-        bool empty() const {
+        inline bool empty() const {
             return _begin == _end;
         }
 
+        inline bool full() const {
+            return wrap_if_bufend(_end + 1) == _begin;
+        }
+
         int peek() {
-            if(_end == _begin) return -1;
+            if(empty()) return -1;
 
             return static_cast<int>(*_begin);
         }
 
-        int read() {
+        int read();
-            if(getSize() == 0) return -1;
-
-            char result = *_begin;
-            _begin = wrap_if_bufend(_begin + 1);
-            return static_cast<int>(result);
-        }
 
         size_t read(char* dst, size_t size) {
             size_t bytes_available = getSize();
@@ -79,13 +76,7 @@ class cbuf {
             return size_read;
         }
 
-        size_t write(char c) {
+        size_t write(char c);
-            if(room() == 0) return 0;
-
-            *_end = c;
-            _end = wrap_if_bufend(_end + 1);
-            return 1;
-        }
 
         size_t write(const char* src, size_t size) {
             size_t bytes_available = room();
@@ -109,7 +100,7 @@ class cbuf {
         }
 
     private:
-        inline char* wrap_if_bufend(char* ptr) {
+        inline char* wrap_if_bufend(char* ptr) const {
             return (ptr == _bufend) ? _buf : ptr;
         }
 

cores/esp8266/interrupts.h

@@ -31,6 +31,10 @@ public:
         xt_wsr_ps(_state);
     }
 
+    uint32_t savedInterruptLevel() const {
+        return _state & 0x0f;
+    }
+
 protected:
     uint32_t _state;
 };

libraries/ESP8266WiFi/src/WiFiClient.cpp

@@ -38,7 +38,6 @@ extern "C"
 #include "lwip/tcp.h"
 #include "lwip/inet.h"
 #include "lwip/netif.h"
-#include "cbuf.h"
 #include "include/ClientContext.h"
 #include "c_types.h"
 

libraries/ESP8266WiFi/src/WiFiClientSecure.cpp

@@ -37,7 +37,6 @@ extern "C"
 #include "lwip/tcp.h"
 #include "lwip/inet.h"
 #include "lwip/netif.h"
-#include "cbuf.h"
 #include "include/ClientContext.h"
 #include "c_types.h"
 

libraries/ESP8266WiFi/src/WiFiServer.cpp

@@ -35,7 +35,6 @@ extern "C" {
 #include "lwip/opt.h"
 #include "lwip/tcp.h"
 #include "lwip/inet.h"
-#include "cbuf.h"
 #include "include/ClientContext.h"
 
 WiFiServer::WiFiServer(IPAddress addr, uint16_t port)