Skip to content

Fix buffer retransmission and flush hang problems in hardware serial support #6855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Fix buffer retransmission and flush hang problems in hardware serial support #6855

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c64363d
Create macro to guard critical sections for large transmit buffers
johnholman Oct 24, 2017
09cd000
Prevent buffer retransmission when transmit buffer is empty
johnholman Oct 25, 2017
b44062d
Improve how TXCn bit is cleared in USCRnA register
johnholman Oct 25, 2017
5d53e3a
Fix flush hanging issue
johnholman Oct 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 39 additions & 16 deletions hardware/arduino/avr/cores/arduino/HardwareSerial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <stdio.h>
#include <string.h>
#include <inttypes.h>
#include <util/atomic.h>
#include "Arduino.h"

#include "HardwareSerial.h"
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down