Avoid "chunkiness" of UART FIFO availability (#511)

* Avoid "chunkiness" of UART FIFO availability

The UART FIFO will generate an IRQ to transfer data into the SerialUART
FIFOs either every 4 received bytes, or every 4 idle byte times.  This
causes the ::available count to report "0" until either of those two
cases happen, causing a potentially delay in data becoming available to
the app.

Change the code to pull data from the HW FIFO on a read/available/peek.
Use a non-blocking mutex and IRQ disabling to safely empty the FIFO from
user space.  The mutex added to the IRQ is non-blocking and will be
a single CAS the vast majority of the time, so it should not impact the
Serial performance.

Fixes #464 and others where `setPollingMode()` was needed as a workaround.

Make sure we have all mutexes locked before we disable the port and free
the queue to avoid evil cases.

Only init the mutexes once, on object creation.

In polled mode, don't bother acquiring/releasing the fifo mutex.

When begin() is called on an already running port, call end() to clean
up the old data/etc. before making a new queue/config.  This avoids a
memory leak and potential write-after-free case.

cores/rp2040/SerialUART.cpp

@@ -121,12 +121,16 @@ SerialUART::SerialUART(uart_inst_t *uart, pin_size_t tx, pin_size_t rx) {
     _rts = UART_PIN_NOT_DEFINED;
     _cts = UART_PIN_NOT_DEFINED;
     mutex_init(&_mutex);
+    mutex_init(&_fifoMutex);
 }
 
 static void _uart0IRQ();
 static void _uart1IRQ();
 
 void SerialUART::begin(unsigned long baud, uint16_t config) {
+    if (_running) {
+        end();
+    }
     _queue = new uint8_t[_fifoSize];
     _baud = baud;
     uart_init(_uart, baud);
@@ -198,6 +202,7 @@ void SerialUART::end() {
     if (!_running) {
         return;
     }
+    _running = false;
     if (!_polling) {
         if (_uart == uart0) {
             irq_set_enabled(UART0_IRQ, false);
@@ -205,9 +210,28 @@ void SerialUART::end() {
             irq_set_enabled(UART1_IRQ, false);
         }
     }
+    // Paranoia - ensure nobody else is using anything here at the same time
+    mutex_enter_blocking(&_mutex);
+    mutex_enter_blocking(&_fifoMutex);
     uart_deinit(_uart);
     delete[] _queue;
-    _running = false;
+    // Reset the mutexes once all is off/cleaned up
+    mutex_exit(&_fifoMutex);
+    mutex_exit(&_mutex);
+}
+
+void SerialUART::_pumpFIFO() {
+    // Use the _fifoMutex to guard against the other core potentially
+    // running the IRQ (since we can't disable their IRQ handler).
+    // We guard against this core by disabling the IRQ handler and
+    // re-enabling if it was previously enabled at the end.
+    auto irqno = (_uart == uart0) ? UART0_IRQ : UART1_IRQ;
+    bool enabled = irq_is_enabled(irqno);
+    irq_set_enabled(irqno, false);
+    mutex_enter_blocking(&_fifoMutex);
+    _handleIRQ(false);
+    mutex_exit(&_fifoMutex);
+    irq_set_enabled(irqno, enabled);
 }
 
 int SerialUART::peek() {
@@ -216,7 +240,9 @@ int SerialUART::peek() {
         return -1;
     }
     if (_polling) {
-        _handleIRQ();
+        _handleIRQ(false);
+    } else {
+        _pumpFIFO();
     }
     if (_writer != _reader) {
         return _queue[_reader];
@@ -230,7 +256,9 @@ int SerialUART::read() {
         return -1;
     }
     if (_polling) {
-        _handleIRQ();
+        _handleIRQ(false);
+    } else {
+        _pumpFIFO();
     }
     if (_writer != _reader) {
         auto ret = _queue[_reader];
@@ -249,7 +277,9 @@ int SerialUART::available() {
         return 0;
     }
     if (_polling) {
-        _handleIRQ();
+        _handleIRQ(false);
+    } else {
+        _pumpFIFO();
     }
     return (_writer - _reader) % _fifoSize;
 }
@@ -260,7 +290,7 @@ int SerialUART::availableForWrite() {
         return 0;
     }
     if (_polling) {
-        _handleIRQ();
+        _handleIRQ(false);
     }
     return (uart_is_writable(_uart)) ? 1 : 0;
 }
@@ -271,7 +301,7 @@ void SerialUART::flush() {
         return;
     }
     if (_polling) {
-        _handleIRQ();
+        _handleIRQ(false);
     }
     uart_tx_wait_blocking(_uart);
 }
@@ -282,7 +312,7 @@ size_t SerialUART::write(uint8_t c) {
         return 0;
     }
     if (_polling) {
-        _handleIRQ();
+        _handleIRQ(false);
     }
     uart_putc_raw(_uart, c);
     return 1;
@@ -294,7 +324,7 @@ size_t SerialUART::write(const uint8_t *p, size_t len) {
         return 0;
     }
     if (_polling) {
-        _handleIRQ();
+        _handleIRQ(false);
     }
     size_t cnt = len;
     while (cnt) {
@@ -325,7 +355,15 @@ void arduino::serialEvent2Run(void) {
 }
 
 // IRQ handler, called when FIFO > 1/8 full or when it had held unread data for >32 bit times
-void __not_in_flash_func(SerialUART::_handleIRQ)() {
+void __not_in_flash_func(SerialUART::_handleIRQ)(bool inIRQ) {
+    if (inIRQ) {
+        uint32_t owner;
+        if (!mutex_try_enter(&_fifoMutex, &owner)) {
+            // Main app on the other core has the mutex so it is
+            // in the process of pulling data out of the HW FIFO
+            return;
+        }
+    }
     // ICR is write-to-clear
     uart_get_hw(_uart)->icr = UART_UARTICR_RTIC_BITS | UART_UARTICR_RXIC_BITS;
     while (uart_is_readable(_uart)) {
@@ -343,6 +381,9 @@ void __not_in_flash_func(SerialUART::_handleIRQ)() {
             // TODO: Overflow
         }
     }
+    if (inIRQ) {
+        mutex_exit(&_fifoMutex);
+    }
 }
 
 static void __not_in_flash_func(_uart0IRQ)() {

cores/rp2040/SerialUART.h

@@ -63,7 +63,7 @@ public:
     operator bool() override;
 
     // Not to be called by users, only from the IRQ handler.  In public so that the C-language IQR callback can access it
-    void _handleIRQ();
+    void _handleIRQ(bool inIRQ = true);
 
 private:
     bool _running = false;
@@ -78,7 +78,9 @@ private:
     uint32_t _writer;
     uint32_t _reader;
     size_t   _fifoSize = 32;
-    uint8_t  *_queue;
+    uint8_t *_queue;
+    mutex_t  _fifoMutex; // Only needed when non-IRQ updates _writer
+    void _pumpFIFO(); // User space FIFO transfer
 };
 
 extern SerialUART Serial1; // HW UART 0