From cb88fdd24297cb1ebf836ee06747bbbca63546d4 Mon Sep 17 00:00:00 2001
From: AnHardt <github@kitelab.de>
Date: Wed, 24 Feb 2016 17:18:12 +0100
Subject: [PATCH] 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 cdaaf4607f..3ef8062e74 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 f30c675cb2..1c4cf9ed68 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;
}
}
--
GitLab