From 69a8bfafe9e999151315d2ce2f94d4ad50402949 Mon Sep 17 00:00:00 2001 From: robert rozee Date: Thu, 24 Sep 2015 02:50:52 +1200 Subject: [PATCH 1/2] make serial.available, peek, read atomic when SERIAL_RX_BUFFER_SIZE is defined as greater than 256, the current HardwareSerial.cpp code almost works correctly. but not quite. the function serial.available may return an incorrect number of characters waiting (though when treated purely as a boolean it seems not to fault).serial.peek has a similar issue detecting if a character is waiting, while serial.read may also leave _rx_buffer_tail corrupt to an interrupt occurring mid-update (this corruption may cause the ISR to be mistaken about how much free is available in the Rx buffer). the proposed changes to these three functions add cli/sti pairings around the critical pieces of code. i have tested and verified the change made to serial.available as correcting the problem, while the change to serial.peek follows the exact same pattern. the changes to serial.read have been confirmed as not breaking the function, but i have not been in a position to test the failure of the original (non-atomic) version to make comparisons. multiple testing was conducted at 115,200 baud and 500,000 baud using data streams of 1,000,000 characters sent over a 2 minute interval. any error causing loss of character would have resulted in a catastrophic (ie, very obvious) failure. cheers, rob :-) --- .../avr/cores/arduino/HardwareSerial.cpp | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp index 5cd89e5e664..18692a2e69a 100644 --- a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp @@ -151,12 +151,22 @@ void HardwareSerial::end() int HardwareSerial::available(void) { - return ((unsigned int)(SERIAL_RX_BUFFER_SIZE + _rx_buffer_head - _rx_buffer_tail)) % SERIAL_RX_BUFFER_SIZE; + uint8_t SaveSREG = SREG; // save interrupt flag + cli(); // disable interrupts + int HmT = _rx_buffer_head - _rx_buffer_tail; // access the shared data + SREG = SaveSREG; // restore the interrupt flag + + return ((unsigned int)(SERIAL_RX_BUFFER_SIZE + HmT)) % SERIAL_RX_BUFFER_SIZE; } int HardwareSerial::peek(void) { - if (_rx_buffer_head == _rx_buffer_tail) { + uint8_t SaveSREG = SREG; // save interrupt flag + cli(); // disable interrupts + int HmT = _rx_buffer_head - _rx_buffer_tail; // access the shared data + SREG = SaveSREG; // restore the interrupt flag + + if (HmT == 0) { return -1; } else { return _rx_buffer[_rx_buffer_tail]; @@ -166,11 +176,21 @@ int HardwareSerial::peek(void) int HardwareSerial::read(void) { // if the head isn't ahead of the tail, we don't have any characters - if (_rx_buffer_head == _rx_buffer_tail) { + uint8_t SaveSREG = SREG; // save interrupt flag + cli(); // disable interrupts + int HmT = _rx_buffer_head - _rx_buffer_tail; // access the shared data + SREG = SaveSREG; // restore the interrupt flag + + if (HmT == 0) { return -1; } else { unsigned char c = _rx_buffer[_rx_buffer_tail]; - _rx_buffer_tail = (rx_buffer_index_t)(_rx_buffer_tail + 1) % SERIAL_RX_BUFFER_SIZE; + rx_buffer_index_t NewTail = (_rx_buffer_tail + 1) % SERIAL_RX_BUFFER_SIZE; + + cli(); + _rx_buffer_tail = NewTail; // access the shared data + SREG = SaveSREG; + return c; } } From 91d817aa9b2a8dd53f0954ac7e5af3119f547fe5 Mon Sep 17 00:00:00 2001 From: robert rozee Date: Fri, 25 Sep 2015 01:20:47 +1200 Subject: [PATCH 2/2] Update HardwareSerial.cpp --- .../avr/cores/arduino/HardwareSerial.cpp | 59 ++++++++++++------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp index 18692a2e69a..3c85e077a98 100644 --- a/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp @@ -151,46 +151,61 @@ void HardwareSerial::end() int HardwareSerial::available(void) { - uint8_t SaveSREG = SREG; // save interrupt flag +#if (SERIAL_RX_BUFFER_SIZE>256) + uint8_t oldSREG = SREG; // save interrupt flag cli(); // disable interrupts - int HmT = _rx_buffer_head - _rx_buffer_tail; // access the shared data - SREG = SaveSREG; // restore the interrupt flag - - return ((unsigned int)(SERIAL_RX_BUFFER_SIZE + HmT)) % SERIAL_RX_BUFFER_SIZE; +#endif + rx_buffer_index_t head = _rx_buffer_head; // retrieve Rx head index + rx_buffer_index_t tail = _rx_buffer_tail; // retrieve Rx tail index +#if (SERIAL_RX_BUFFER_SIZE>256) + SREG = oldSREG; // restore the interrupt flag +#endif + return ((unsigned int)(SERIAL_RX_BUFFER_SIZE + head - tail)) % SERIAL_RX_BUFFER_SIZE; } int HardwareSerial::peek(void) { - uint8_t SaveSREG = SREG; // save interrupt flag +#if (SERIAL_RX_BUFFER_SIZE>256) + uint8_t oldSREG = SREG; // save interrupt flag cli(); // disable interrupts - int HmT = _rx_buffer_head - _rx_buffer_tail; // access the shared data - SREG = SaveSREG; // restore the interrupt flag - - if (HmT == 0) { +#endif + rx_buffer_index_t head = _rx_buffer_head; // retrieve Rx head index + rx_buffer_index_t tail = _rx_buffer_tail; // retrieve Rx tail index +#if (SERIAL_RX_BUFFER_SIZE>256) + SREG = oldSREG; // restore the interrupt flag +#endif + if (head == tail) { return -1; } else { - return _rx_buffer[_rx_buffer_tail]; + return _rx_buffer[tail]; } } int HardwareSerial::read(void) { - // if the head isn't ahead of the tail, we don't have any characters - uint8_t SaveSREG = SREG; // save interrupt flag +#if (SERIAL_RX_BUFFER_SIZE>256) + uint8_t oldSREG = SREG; // save interrupt flag cli(); // disable interrupts - int HmT = _rx_buffer_head - _rx_buffer_tail; // access the shared data - SREG = SaveSREG; // restore the interrupt flag +#endif + rx_buffer_index_t head = _rx_buffer_head; // retrieve Rx head index + rx_buffer_index_t tail = _rx_buffer_tail; // retrieve Rx tail index +#if (SERIAL_RX_BUFFER_SIZE>256) + SREG = oldSREG; // restore the interrupt flag +#endif - if (HmT == 0) { + if (head == tail) { return -1; } else { - unsigned char c = _rx_buffer[_rx_buffer_tail]; - rx_buffer_index_t NewTail = (_rx_buffer_tail + 1) % SERIAL_RX_BUFFER_SIZE; - - cli(); - _rx_buffer_tail = NewTail; // access the shared data - SREG = SaveSREG; + unsigned char c = _rx_buffer[tail]; +#if (SERIAL_RX_BUFFER_SIZE>256) + uint8_t oldSREG = SREG; // save interrupt flag + cli(); // disable interrupts +#endif + _rx_buffer_tail = (tail + 1) % SERIAL_RX_BUFFER_SIZE; +#if (SERIAL_RX_BUFFER_SIZE>256) + SREG = oldSREG; // restore the interrupt flag +#endif return c; } }