diff --git a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp index 5cd89e5e664..ecd7918431c 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) @@ -89,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 @@ -177,15 +187,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; } @@ -218,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; - sbi(*_ucsra, 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; @@ -240,9 +258,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; }