From 139ace9909ef09d40e8ef208b156213188a6c5f6 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 12 Mar 2022 11:21:35 -0800 Subject: [PATCH 1/2] 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. --- cores/rp2040/SerialUART.cpp | 38 ++++++++++++++++++++++++++++++++++++- cores/rp2040/SerialUART.h | 6 ++++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/cores/rp2040/SerialUART.cpp b/cores/rp2040/SerialUART.cpp index 8cf61c50..56abb9ff 100644 --- a/cores/rp2040/SerialUART.cpp +++ b/cores/rp2040/SerialUART.cpp @@ -127,7 +127,12 @@ static void _uart0IRQ(); static void _uart1IRQ(); void SerialUART::begin(unsigned long baud, uint16_t config) { + if (_running) { + return; // Already going, must stop to change anything + } _queue = new uint8_t[_fifoSize]; + mutex_init(&_mutex); + mutex_init(&_fifoMutex); _baud = baud; uart_init(_uart, baud); int bits, stop; @@ -210,6 +215,20 @@ void SerialUART::end() { _running = false; } +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() { CoreMutex m(&_mutex); if (!_running || !m) { @@ -217,6 +236,8 @@ int SerialUART::peek() { } if (_polling) { _handleIRQ(); + } else { + _pumpFIFO(); } if (_writer != _reader) { return _queue[_reader]; @@ -231,6 +252,8 @@ int SerialUART::read() { } if (_polling) { _handleIRQ(); + } else { + _pumpFIFO(); } if (_writer != _reader) { auto ret = _queue[_reader]; @@ -250,6 +273,8 @@ int SerialUART::available() { } if (_polling) { _handleIRQ(); + } else { + _pumpFIFO(); } return (_writer - _reader) % _fifoSize; } @@ -325,7 +350,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 +376,9 @@ void __not_in_flash_func(SerialUART::_handleIRQ)() { // TODO: Overflow } } + if (inIRQ) { + mutex_exit(&_fifoMutex); + } } static void __not_in_flash_func(_uart0IRQ)() { diff --git a/cores/rp2040/SerialUART.h b/cores/rp2040/SerialUART.h index 6fb7785e..b785bcad 100644 --- a/cores/rp2040/SerialUART.h +++ b/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 -- 2.54.0 From 9bc3a22fa497a28079c335d02d15ba7f61026d99 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 15 Mar 2022 13:41:27 -0700 Subject: [PATCH 2/2] Clean up mutexes, allow multiple begin()s 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 | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/cores/rp2040/SerialUART.cpp b/cores/rp2040/SerialUART.cpp index 56abb9ff..56196b88 100644 --- a/cores/rp2040/SerialUART.cpp +++ b/cores/rp2040/SerialUART.cpp @@ -121,6 +121,7 @@ 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(); @@ -128,11 +129,9 @@ static void _uart1IRQ(); void SerialUART::begin(unsigned long baud, uint16_t config) { if (_running) { - return; // Already going, must stop to change anything + end(); } _queue = new uint8_t[_fifoSize]; - mutex_init(&_mutex); - mutex_init(&_fifoMutex); _baud = baud; uart_init(_uart, baud); int bits, stop; @@ -203,6 +202,7 @@ void SerialUART::end() { if (!_running) { return; } + _running = false; if (!_polling) { if (_uart == uart0) { irq_set_enabled(UART0_IRQ, false); @@ -210,9 +210,14 @@ 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() { @@ -235,7 +240,7 @@ int SerialUART::peek() { return -1; } if (_polling) { - _handleIRQ(); + _handleIRQ(false); } else { _pumpFIFO(); } @@ -251,7 +256,7 @@ int SerialUART::read() { return -1; } if (_polling) { - _handleIRQ(); + _handleIRQ(false); } else { _pumpFIFO(); } @@ -272,7 +277,7 @@ int SerialUART::available() { return 0; } if (_polling) { - _handleIRQ(); + _handleIRQ(false); } else { _pumpFIFO(); } @@ -285,7 +290,7 @@ int SerialUART::availableForWrite() { return 0; } if (_polling) { - _handleIRQ(); + _handleIRQ(false); } return (uart_is_writable(_uart)) ? 1 : 0; } @@ -296,7 +301,7 @@ void SerialUART::flush() { return; } if (_polling) { - _handleIRQ(); + _handleIRQ(false); } uart_tx_wait_blocking(_uart); } @@ -307,7 +312,7 @@ size_t SerialUART::write(uint8_t c) { return 0; } if (_polling) { - _handleIRQ(); + _handleIRQ(false); } uart_putc_raw(_uart, c); return 1; @@ -319,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) { -- 2.54.0