From c64363d1a16576764d131c58d71b5507278a8dbe Mon Sep 17 00:00:00 2001 From: John Holman Date: Tue, 24 Oct 2017 22:18:38 +0100 Subject: [PATCH 1/4] Create macro to guard critical sections for large transmit buffers New macro TX_BUFFER_ATOMIC makes the following code block atomic only if the transmit buffer is larger than 256 bytes. SREG is restored on completion. The macro is then used to simplify code for availableForWrite() --- .../avr/cores/arduino/HardwareSerial.cpp | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp index 5cd89e5e664..75e079f93c9 100644 --- a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include "Arduino.h" #include "HardwareSerial.h" @@ -76,6 +77,13 @@ void serialEventRun(void) #endif } +// macro to guard critical sections when needed for large TX buffer sizes +#if (SERIAL_TX_BUFFER_SIZE>256) +#define TX_BUFFER_ATOMIC ATOMIC_BLOCK(ATOMIC_RESTORESTATE) +#else +#define TX_BUFFER_ATOMIC +#endif + // Actual interrupt handlers ////////////////////////////////////////////////////////////// void HardwareSerial::_tx_udr_empty_irq(void) @@ -177,15 +185,13 @@ int HardwareSerial::read(void) int HardwareSerial::availableForWrite(void) { -#if (SERIAL_TX_BUFFER_SIZE>256) - uint8_t oldSREG = SREG; - cli(); -#endif - tx_buffer_index_t head = _tx_buffer_head; - tx_buffer_index_t tail = _tx_buffer_tail; -#if (SERIAL_TX_BUFFER_SIZE>256) - SREG = oldSREG; -#endif + tx_buffer_index_t head; + tx_buffer_index_t tail; + + TX_BUFFER_ATOMIC { + head = _tx_buffer_head; + tail = _tx_buffer_tail; + } if (head >= tail) return SERIAL_TX_BUFFER_SIZE - 1 - head + tail; return tail - head - 1; } From 09cd0005e08e3dd6da683b060ca1df78f1a62f49 Mon Sep 17 00:00:00 2001 From: John Holman Date: Wed, 25 Oct 2017 10:06:37 +0100 Subject: [PATCH 2/4] Prevent buffer retransmission when transmit buffer is empty Moving the head buffer pointer and setting interrupt flag is now atomic in write(). Previously an intervening ISR could empty the buffer before the second ISR is triggered causing retransmission. Fixes: #3745 (original issue only) --- hardware/arduino/avr/cores/arduino/HardwareSerial.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp index 75e079f93c9..560ef3a38a5 100644 --- a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp @@ -246,9 +246,14 @@ size_t HardwareSerial::write(uint8_t c) } _tx_buffer[_tx_buffer_head] = c; - _tx_buffer_head = i; - - sbi(*_ucsrb, UDRIE0); + + // make atomic to prevent execution of ISR between setting the + // head pointer and setting the interrupt flag resulting in buffer + // retransmission + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + _tx_buffer_head = i; + sbi(*_ucsrb, UDRIE0); + } return 1; } From b44062d61c505c6e7b98d597e9b925745c08a24b Mon Sep 17 00:00:00 2001 From: John Holman Date: Wed, 25 Oct 2017 10:38:23 +0100 Subject: [PATCH 3/4] Improve how TXCn bit is cleared in USCRnA register Preserve values of configuration bits MPCMn and U2Xn. Avoid setting other read-only bits for datasheet conformance. See #3745 --- hardware/arduino/avr/cores/arduino/HardwareSerial.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp index 560ef3a38a5..e6ccdef0d90 100644 --- a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp @@ -97,8 +97,10 @@ void HardwareSerial::_tx_udr_empty_irq(void) // clear the TXC bit -- "can be cleared by writing a one to its bit // location". This makes sure flush() won't return until the bytes - // actually got written - sbi(*_ucsra, TXC0); + // actually got written. Other r/w bits are preserved, and zeroes + // written to the rest. + + *_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << MPCM0))) | (1 << TXC0); if (_tx_buffer_head == _tx_buffer_tail) { // Buffer empty, so disable interrupts @@ -225,7 +227,7 @@ size_t HardwareSerial::write(uint8_t c) // 500kbit/s) bitrates, where interrupt overhead becomes a slowdown. if (_tx_buffer_head == _tx_buffer_tail && bit_is_set(*_ucsra, UDRE0)) { *_udr = c; - sbi(*_ucsra, TXC0); + *_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << MPCM0))) | (1 << TXC0); return 1; } tx_buffer_index_t i = (_tx_buffer_head + 1) % SERIAL_TX_BUFFER_SIZE; From 5d53e3aab109be6f5dfc19b196724092aaf9a5da Mon Sep 17 00:00:00 2001 From: John Holman Date: Wed, 25 Oct 2017 11:07:35 +0100 Subject: [PATCH 4/4] Fix flush hanging issue Make write to UDR and clearing of TXC bit in flush() atomic to avoid race condition. Fixes #3745 (second different issue introduced later but discussed in the same issue) --- .../arduino/avr/cores/arduino/HardwareSerial.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp index e6ccdef0d90..ecd7918431c 100644 --- a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp @@ -226,8 +226,18 @@ size_t HardwareSerial::write(uint8_t c) // significantly improve the effective datarate at high (> // 500kbit/s) bitrates, where interrupt overhead becomes a slowdown. if (_tx_buffer_head == _tx_buffer_tail && bit_is_set(*_ucsra, UDRE0)) { - *_udr = c; - *_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << MPCM0))) | (1 << TXC0); + // If TXC is cleared before writing UDR and the previous byte + // completes before writing to UDR, TXC will be set but a byte + // is still being transmitted causing flush() to return too soon. + // So writing UDR must happen first. + // Writing UDR and clearing TC must be done atomically, otherwise + // interrupts might delay the TXC clear so the byte written to UDR + // is transmitted (setting TXC) before clearing TXC. Then TXC will + // be cleared when no bytes are left, causing flush() to hang + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + *_udr = c; + *_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << MPCM0))) | (1 << TXC0); + } return 1; } tx_buffer_index_t i = (_tx_buffer_head + 1) % SERIAL_TX_BUFFER_SIZE;