Cleanup circular_buffer implementation

unit tests will follow

src/hal/avr/usart.cpp

@@ -8,7 +8,7 @@ USART usart1(USART1);
 
 uint8_t USART::Read() {
     uint8_t c = 0;
-    rx_buf.ConsumeFirst(c);
+    rx_buf.pop(c);
     return c;
 }
 
@@ -18,7 +18,7 @@ void USART::Write(uint8_t c) {
     // to the data register and be done. This shortcut helps
     // significantly improve the effective datarate at high (>
     // 500kbit/s) bitrates, where interrupt overhead becomes a slowdown.
-    if (tx_buf.IsEmpty() && (husart->UCSRxA & (1 << 5))) {
+    if (tx_buf.empty() && (husart->UCSRxA & (1 << 5))) {
         husart->UDRx = c;
         husart->UCSRxA |= (1 << 6);
         return;
@@ -26,7 +26,7 @@ void USART::Write(uint8_t c) {
 
     // If the output buffer is full, there's nothing for it other than to
     // wait for the interrupt handler to empty it a bit
-    while (!tx_buf.push_back_DontRewrite(c)) {
+    while (!tx_buf.push(c)) {
         if (bit_is_clear(SREG, SREG_I)) {
             // Interrupts are disabled, so we'll have to poll the data
             // register empty flag ourselves. If it is set, pretend an

src/hal/circle_buffer.hpp

@@ -1,98 +0,0 @@
-// circle_buffer.hpp
-#pragma once
-
-#include <stdint.h>
-#include <stddef.h>
-/*****************************************************************************/
-// general circular buffer
-// you can never use entire size
-// because write position (end) cannot be equal to begin
-// because begin == end == empty
-template <class T, size_t SIZE>
-class CircleBuffer {
-public:
-    using Elem = T;
-
-protected:
-    T data[SIZE];
-    volatile size_t begin; // position of first element
-    volatile size_t end; // position behind last element == write position
-    volatile size_t pushed;
-    static void incrementIndex(volatile size_t &index) { index = (index + 1) % SIZE; }
-    static void decrementIndex(volatile size_t &index) { index = (index + SIZE - 1) % SIZE; }
-
-public:
-    CircleBuffer()
-        : begin(0)
-        , end(0)
-        , pushed(0) {}
-
-    void push_back(T elem);
-    bool push_back_DontRewrite(T elem);
-    size_t Count() const { return (end + SIZE - begin) % SIZE; }
-    bool IsEmpty() const { return begin == end; }
-    bool CanPush() const {
-        size_t index = begin;
-        incrementIndex(index);
-        return (index != end);
-    }
-    size_t PushedCount() const { return pushed; }
-
-    constexpr size_t Size() const { return SIZE; }
-
-    bool ConsumeFirst(T &elem); // data must be processed before next push_back
-    bool ConsumeLast(T &elem); // data must be processed before next push_back
-    const T &GetFirstIfAble() const; // data must be processed before next push_back, must not be empty
-    const T &GetLastIfAble() const; // data must be processed before next push_back, must not be empty
-};
-
-template <class T, size_t SIZE>
-void CircleBuffer<T, SIZE>::push_back(T elem) {
-    data[end] = elem;
-    incrementIndex(end);
-    if (begin == end) { //begin just was erased, set new begin
-        incrementIndex(begin);
-    }
-    ++pushed;
-}
-
-template <class T, size_t SIZE>
-bool CircleBuffer<T, SIZE>::push_back_DontRewrite(T elem) {
-    size_t index = begin;
-    incrementIndex(index);
-    if (index != end) {
-        push_back(elem);
-        return true;
-    }
-    return false;
-}
-
-template <class T, size_t SIZE>
-bool CircleBuffer<T, SIZE>::ConsumeFirst(T &elem) {
-    if (IsEmpty())
-        return false;
-    elem = GetFirstIfAble();
-    incrementIndex(begin);
-    return true;
-}
-
-template <class T, size_t SIZE>
-bool CircleBuffer<T, SIZE>::ConsumeLast(T &elem) {
-    if (IsEmpty())
-        return false;
-    elem = GetLastIfAble();
-    decrementIndex(end);
-    return true;
-}
-
-template <class T, size_t SIZE>
-const T &CircleBuffer<T, SIZE>::GetFirstIfAble() const {
-    return data[begin];
-}
-
-template <class T, size_t SIZE>
-const T &CircleBuffer<T, SIZE>::GetLastIfAble() const {
-    size_t index = end;
-    decrementIndex(index);
-    return data[index];
-}

src/hal/circular_buffer.h

@@ -0,0 +1,63 @@
+#pragma once
+
+#include <stdint.h>
+#include <stddef.h>
+
+/// A generic circular buffer class
+/// Can hold up to (size-1) elements
+/// @param T data type of stored elements
+/// @param index_t data type of indices into array of elements
+///   (recommended to keep uint8_fast8_t as single byte operations are atomical on the AVR)
+/// @param size number of elements to store + 1.
+///   It is recommended to keep a power of 2 to allow for optimal code generation on the AVR (there is no HW modulo instruction)
+template <typename T = uint8_t, typename index_t = uint_fast8_t, size_t size = 16>
+class CircularBuffer {
+public:
+    constexpr inline CircularBuffer()
+        : tail(0)
+        , head(0) {}
+
+    inline bool empty() const {
+        return tail == head;
+    }
+
+    bool full() const {
+        return next(head) == tail;
+    }
+
+    /// Insert an element into the buffer.
+    /// Checks for empty spot for the element and does not change the buffer content
+    /// in case the buffer is full.
+    /// @returns true if the insertion was successful (i.e. there was an empty spot for the element)
+    bool push(T elem) {
+        if (full())
+            return false;
+        data[head] = elem;
+        head = next(head);
+        return true;
+    }
+
+    /// @returns peeks the current element to extract from the buffer, however the element is left in the buffer
+    /// Does not perform any range checks for performance reasons, should be preceeded by if(!empty()) in the user code
+    inline T front() const {
+        return data[tail];
+    }
+
+    /// Extracts the current element from the buffer
+    /// @returns true in case there was an element for extraction (i.e. the buffer was not empty)
+    bool pop(T &elem) {
+        if (empty())
+            return false;
+        elem = front();
+        tail = next(tail);
+        return true;
+    }
+
+protected:
+    T data[size]; ///< array of stored elements
+    index_t tail; ///< index of element to read (pop/extract) from the buffer
+    index_t head; ///< index of an empty spot or element insertion (write)
+
+    /// @returns next index wrapped past the end of the array of elements
+    static index_t next(index_t index) { return (index + 1) % size; }
+};

src/hal/usart.h

@@ -2,7 +2,8 @@
 #include <inttypes.h>
 #include <avr/io.h>
 #include "gpio.h"
-#include "circle_buffer.hpp"
+#include "circular_buffer.h"
+
 /// USART interface
 /// @@TODO decide, if this class will behave like a singleton, or there will be multiple classes
 /// for >1 USART interfaces
@@ -33,11 +34,11 @@ public:
 
     /// @returns current character from the UART without extracting it from the read buffer
     uint8_t Peek() const {
-        return rx_buf.GetFirstIfAble();
+        return rx_buf.front();
     }
     /// @returns true if there are no bytes to be read
     bool ReadEmpty() const {
-        return rx_buf.IsEmpty();
+        return rx_buf.empty();
     }
     /// @returns current character from the UART and extracts it from the read buffer
     uint8_t Read();
@@ -48,7 +49,7 @@ public:
     void puts(const char *str);
     /// @returns true if there is at least one byte free in the TX buffer (i.e. some space to add a character to be sent)
     bool CanWrite() const {
-        return tx_buf.CanPush();
+        return !tx_buf.full();
     }
     /// blocks until the TX buffer was successfully transmitted
     void Flush();
@@ -69,13 +70,13 @@ public:
         if (husart->UCSRxA & (1 << 4)) {
             (void)husart->UDRx;
         } else {
-            rx_buf.push_back_DontRewrite(husart->UDRx);
+            rx_buf.push((uint8_t)husart->UDRx);
         }
     }
     /// implementation of the transmit ISR's body
     __attribute__((always_inline)) inline void ISR_UDRE() {
         uint8_t c = 0;
-        tx_buf.ConsumeFirst(c);
+        tx_buf.pop(c);
         husart->UDRx = c;
 
         // clear the TXC bit -- "can be cleared by writing a one to its bit
@@ -83,7 +84,7 @@ public:
         // actually got written
         husart->UCSRxA |= (1 << 6);
 
-        if (tx_buf.IsEmpty())
+        if (tx_buf.empty())
             husart->UCSRxB &= ~(1 << 5); // disable UDRE interrupt
     }
 
@@ -94,8 +95,8 @@ private:
     USART_TypeDef *husart;
     bool _written;
 
-    CircleBuffer<uint8_t, 32> tx_buf;
+    CircularBuffer<uint8_t, uint_fast8_t, 32> tx_buf;
-    CircleBuffer<uint8_t, 32> rx_buf;
+    CircularBuffer<uint8_t, uint_fast8_t, 32> rx_buf;
 };
 
 /// beware - normally we'd make a singleton, but avr-gcc generates suboptimal code for them, therefore we only keep this extern variable