From dc0f41868e9acce960eb8f91221a7bc5b33866cd Mon Sep 17 00:00:00 2001 From: AnHardt Date: Wed, 3 Feb 2016 21:20:34 +0100 Subject: [PATCH 1/2] Make rx tail/head atomic --- Marlin/MarlinSerial.cpp | 4 ++-- Marlin/MarlinSerial.h | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Marlin/MarlinSerial.cpp b/Marlin/MarlinSerial.cpp index ad23a0620..cdaaf4607 100644 --- a/Marlin/MarlinSerial.cpp +++ b/Marlin/MarlinSerial.cpp @@ -33,7 +33,7 @@ #endif FORCE_INLINE void store_char(unsigned char c) { - int i = (unsigned int)(rx_buffer.head + 1) % RX_BUFFER_SIZE; + uint8_t i = (uint8_t)(rx_buffer.head + 1) % RX_BUFFER_SIZE; // if we should be storing the received character into the location // just before the tail (meaning that the head would advance to the @@ -116,7 +116,7 @@ int MarlinSerial::read(void) { } else { unsigned char c = rx_buffer.buffer[rx_buffer.tail]; - rx_buffer.tail = (unsigned int)(rx_buffer.tail + 1) % RX_BUFFER_SIZE; + rx_buffer.tail = (uint8_t)(rx_buffer.tail + 1) % RX_BUFFER_SIZE; return c; } } diff --git a/Marlin/MarlinSerial.h b/Marlin/MarlinSerial.h index c59884a28..f30c675cb 100644 --- a/Marlin/MarlinSerial.h +++ b/Marlin/MarlinSerial.h @@ -69,13 +69,14 @@ // using a ring buffer (I think), in which rx_buffer_head is the index of the // location to which to write the next incoming character and rx_buffer_tail // is the index of the location from which to read. +// 256 is the max limit due to uint8_t head and tail. Thats needed to make them atomic. #define RX_BUFFER_SIZE 128 struct ring_buffer { unsigned char buffer[RX_BUFFER_SIZE]; - int head; - int tail; + volatile uint8_t head; + volatile uint8_t tail; }; #if UART_PRESENT(SERIAL_PORT) @@ -92,8 +93,8 @@ class MarlinSerial { //: public Stream int read(void); void flush(void); - FORCE_INLINE int available(void) { - return (unsigned int)(RX_BUFFER_SIZE + rx_buffer.head - rx_buffer.tail) % RX_BUFFER_SIZE; + FORCE_INLINE uint8_t available(void) { + return (uint8_t)(RX_BUFFER_SIZE + rx_buffer.head - rx_buffer.tail) % RX_BUFFER_SIZE; } FORCE_INLINE void write(uint8_t c) { @@ -105,7 +106,7 @@ class MarlinSerial { //: public Stream FORCE_INLINE void checkRx(void) { if (TEST(M_UCSRxA, M_RXCx)) { unsigned char c = M_UDRx; - int i = (unsigned int)(rx_buffer.head + 1) % RX_BUFFER_SIZE; + uint8_t i = (uint8_t)(rx_buffer.head + 1) % RX_BUFFER_SIZE; // if we should be storing the received character into the location // just before the tail (meaning that the head would advance to the From cb88fdd24297cb1ebf836ee06747bbbca63546d4 Mon Sep 17 00:00:00 2001 From: AnHardt Date: Wed, 24 Feb 2016 17:18:12 +0100 Subject: [PATCH 2/2] Protect MarlinSerial against interrupts Protect MarlinSerial against interrupts by shielding the CRITICAL_SECTIONs Now with a test if RX_BUFFER_SIZE is in the required range. Code in peek() and read() is now optimized for readability and showing the similarity between the two. Maybe a bit overprotected in checkRx() and store_char(), but now some days without detected errors from this source. --- Marlin/MarlinSerial.cpp | 53 +++++++++++++++++++++++++---------------- Marlin/MarlinSerial.h | 45 +++++++++++++++++++++++----------- 2 files changed, 64 insertions(+), 34 deletions(-) diff --git a/Marlin/MarlinSerial.cpp b/Marlin/MarlinSerial.cpp index cdaaf4607..3ef8062e7 100644 --- a/Marlin/MarlinSerial.cpp +++ b/Marlin/MarlinSerial.cpp @@ -33,16 +33,19 @@ #endif FORCE_INLINE void store_char(unsigned char c) { - uint8_t i = (uint8_t)(rx_buffer.head + 1) % RX_BUFFER_SIZE; - - // if we should be storing the received character into the location - // just before the tail (meaning that the head would advance to the - // current location of the tail), we're about to overflow the buffer - // and so we don't write the character or advance the head. - if (i != rx_buffer.tail) { - rx_buffer.buffer[rx_buffer.head] = c; - rx_buffer.head = i; - } + CRITICAL_SECTION_START; + uint8_t h = rx_buffer.head; + uint8_t i = (uint8_t)(h + 1) & (RX_BUFFER_SIZE - 1); + + // if we should be storing the received character into the location + // just before the tail (meaning that the head would advance to the + // current location of the tail), we're about to overflow the buffer + // and so we don't write the character or advance the head. + if (i != rx_buffer.tail) { + rx_buffer.buffer[h] = c; + rx_buffer.head = i; + } + CRITICAL_SECTION_END; } @@ -101,24 +104,32 @@ void MarlinSerial::end() { int MarlinSerial::peek(void) { - if (rx_buffer.head == rx_buffer.tail) { - return -1; + int v; + CRITICAL_SECTION_START; + uint8_t t = rx_buffer.tail; + if (rx_buffer.head == t) { + v = -1; } else { - return rx_buffer.buffer[rx_buffer.tail]; + v = rx_buffer.buffer[t]; } + CRITICAL_SECTION_END; + return v; } int MarlinSerial::read(void) { - // if the head isn't ahead of the tail, we don't have any characters - if (rx_buffer.head == rx_buffer.tail) { - return -1; + int v; + CRITICAL_SECTION_START; + uint8_t t = rx_buffer.tail; + if (rx_buffer.head == t) { + v = -1; } else { - unsigned char c = rx_buffer.buffer[rx_buffer.tail]; - rx_buffer.tail = (uint8_t)(rx_buffer.tail + 1) % RX_BUFFER_SIZE; - return c; + v = rx_buffer.buffer[t]; + rx_buffer.tail = (uint8_t)(t + 1) & (RX_BUFFER_SIZE - 1); } + CRITICAL_SECTION_END; + return v; } void MarlinSerial::flush() { @@ -131,7 +142,9 @@ void MarlinSerial::flush() { // the value to rx_buffer_tail; the previous value of rx_buffer_head // may be written to rx_buffer_tail, making it appear as if the buffer // were full, not empty. - rx_buffer.head = rx_buffer.tail; + CRITICAL_SECTION_START; + rx_buffer.head = rx_buffer.tail; + CRITICAL_SECTION_END; } diff --git a/Marlin/MarlinSerial.h b/Marlin/MarlinSerial.h index f30c675cb..1c4cf9ed6 100644 --- a/Marlin/MarlinSerial.h +++ b/Marlin/MarlinSerial.h @@ -23,6 +23,12 @@ #define MarlinSerial_h #include "Marlin.h" +#ifndef CRITICAL_SECTION_START + #define CRITICAL_SECTION_START unsigned char _sreg = SREG; cli(); + #define CRITICAL_SECTION_END SREG = _sreg; +#endif + + #ifndef SERIAL_PORT #define SERIAL_PORT 0 #endif @@ -69,9 +75,13 @@ // using a ring buffer (I think), in which rx_buffer_head is the index of the // location to which to write the next incoming character and rx_buffer_tail // is the index of the location from which to read. -// 256 is the max limit due to uint8_t head and tail. Thats needed to make them atomic. -#define RX_BUFFER_SIZE 128 - +// 256 is the max limit due to uint8_t head and tail. Use only powers of 2. (...,16,32,64,128,256) +#ifndef RX_BUFFER_SIZE + #define RX_BUFFER_SIZE 128 +#endif +#if !((RX_BUFFER_SIZE == 256) ||(RX_BUFFER_SIZE == 128) ||(RX_BUFFER_SIZE == 64) ||(RX_BUFFER_SIZE == 32) ||(RX_BUFFER_SIZE == 16) ||(RX_BUFFER_SIZE == 8) ||(RX_BUFFER_SIZE == 4) ||(RX_BUFFER_SIZE == 2)) + #error RX_BUFFER_SIZE has to be a power of 2 and >= 2 +#endif struct ring_buffer { unsigned char buffer[RX_BUFFER_SIZE]; @@ -94,7 +104,11 @@ class MarlinSerial { //: public Stream void flush(void); FORCE_INLINE uint8_t available(void) { - return (uint8_t)(RX_BUFFER_SIZE + rx_buffer.head - rx_buffer.tail) % RX_BUFFER_SIZE; + CRITICAL_SECTION_START; + uint8_t h = rx_buffer.head; + uint8_t t = rx_buffer.tail; + CRITICAL_SECTION_END; + return (uint8_t)(RX_BUFFER_SIZE + h - t) & (RX_BUFFER_SIZE - 1); } FORCE_INLINE void write(uint8_t c) { @@ -106,16 +120,19 @@ class MarlinSerial { //: public Stream FORCE_INLINE void checkRx(void) { if (TEST(M_UCSRxA, M_RXCx)) { unsigned char c = M_UDRx; - uint8_t i = (uint8_t)(rx_buffer.head + 1) % RX_BUFFER_SIZE; - - // if we should be storing the received character into the location - // just before the tail (meaning that the head would advance to the - // current location of the tail), we're about to overflow the buffer - // and so we don't write the character or advance the head. - if (i != rx_buffer.tail) { - rx_buffer.buffer[rx_buffer.head] = c; - rx_buffer.head = i; - } + CRITICAL_SECTION_START; + uint8_t h = rx_buffer.head; + uint8_t i = (uint8_t)(h + 1) & (RX_BUFFER_SIZE - 1); + + // if we should be storing the received character into the location + // just before the tail (meaning that the head would advance to the + // current location of the tail), we're about to overflow the buffer + // and so we don't write the character or advance the head. + if (i != rx_buffer.tail) { + rx_buffer.buffer[h] = c; + rx_buffer.head = i; + } + CRITICAL_SECTION_END; } }