Removed usage of std::vector and replaced with std::array (#33)

* Removed usage of std::vector and replaced with std::array
added helper class `Buffer` to mimic the erase function of std::vector
and move tailing data to start of array

* review comments

* fixed python binding, currently settings buffer size fixed to 256, as it was the default before

* const/constexpr method and rename buffer to m_buffer

* added cmake defined hdlcpp buffer size for python module.
added return result to push_back function to allow exit on full internal buffer

* unit test

* rename using type of Hdlcpp template

* rewording

* function renaming

README.md

@@ -33,7 +33,7 @@ protocol = std::make_shared<Protocol>(
 ## Python binding
 
 A python binding made using [pybind11](https://github.com/pybind/pybind11) can be found under the [python](https://github.com/bang-olufsen/hdlcpp/tree/master/python) folder which can be used e.g. for automated testing.
-
+Buffer size is determined on Python module build, the default buffer size is 256 bytes, to override this for the python binding add `-DPYTHON_HDLCPP_BUFFER_SIZE=<buffer size>` to the CMake build command.
 ## HDLC implementation
 
 The supported HDLC frames are limited to DATA (I-frame with Poll bit), ACK (S-frame Receive Ready with Final bit) and NACK (S-frame Reject with Final bit). All DATA frames are acknowledged or negative acknowledged. The Address and Control fields uses the 8-bit format which means that the highest sequence number is 7. The FCS field is 16-bit.

include/Hdlcpp.hpp

@@ -35,27 +35,73 @@ namespace Hdlcpp {
 using TransportRead = std::function<int(std::span<uint8_t> buffer)>;
 using TransportWrite = std::function<int(const std::span<const uint8_t> buffer)>;
 
+template<typename T, std::size_t Capacity>
+class Buffer {
+    using Container = std::array<T, Capacity>;
+
+public:
+    [[nodiscard]] bool empty() const {
+        return std::span<typename Container::value_type>(m_head, m_tail).empty();
+    }
+
+    constexpr size_t capacity() {
+        return Capacity;
+    }
+
+    typename Container::iterator begin() {
+        return m_head;
+    }
+
+    typename Container::iterator end() {
+        return m_tail;
+    }
+
+    std::span<uint8_t> dataSpan() {
+        return {m_head, m_tail};
+    }
+
+    std::span<uint8_t> unusedSpan() {
+        return {m_tail, m_buffer.end()};
+    }
+
+    void appendToTail(size_t tail) {
+        m_tail += tail;
+    }
+
+    constexpr typename Container::iterator erase(typename Container::iterator first, typename Container::iterator last) {
+        if (last < m_tail) {
+            std::span<uint8_t> toBeMoved(last, m_tail);
+            for (const auto &byte: toBeMoved) {
+                *first++ = byte;
+            }
+            m_tail = first;
+        }
+        return m_tail;
+    }
+
+private:
+    Container m_buffer{};
+    typename Container::iterator m_head{ m_buffer.begin() };
+    typename Container::iterator m_tail{ m_buffer.begin() };
+};
+
+//! @param Capacity The buffer size to be allocated for encoding/decoding frames
+template<size_t Capacity>
 class Hdlcpp {
+    static_assert(Capacity > 0, "HDLCPP requires a buffer size larger than 0");
+    using Container = std::array<uint8_t, Capacity>;
 public:
     //! @brief Constructs the Hdlcpp instance
     //! @param read A std::function for reading from the transport layer (e.g. UART)
     //! @param write A std::function for writing to the transport layer (e.g. UART)
-    //! @param bufferSize The buffer size to be allocated for encoding/decoding frames
     //! @param writeTimeout The write timeout in milliseconds to wait for an ack/nack
     //! @param writeRetries The number of write retries in case of timeout
-    Hdlcpp(TransportRead read, TransportWrite write, uint16_t bufferSize = 256,
-        uint16_t writeTimeout = 100, uint8_t writeRetries = 1)
+    Hdlcpp(TransportRead read, TransportWrite write, uint16_t writeTimeout = 100, uint8_t writeRetries = 1)
         : transportRead(std::move(read))
         , transportWrite(std::move(write))
-        , transportReadBuffer(bufferSize)
         , readFrame(FrameNack)
         , writeTimeout(writeTimeout)
-        , writeRetries(writeRetries)
-    {
-        // Reserve the specified buffer size to avoid increasing the vector in small chunks (performance)
-        readBuffer.reserve(bufferSize);
-        writeBuffer.reserve(bufferSize);
-    }
+        , writeRetries(writeRetries) { }
 
     //! @brief Destructs the Hdlcpp instance
     virtual ~Hdlcpp() = default;
@@ -69,26 +115,25 @@ class Hdlcpp {
         int result;
         uint16_t discardBytes;
 
-        if (!buffer.data() || buffer.empty() || (buffer.size() > transportReadBuffer.size()))
+        if (!buffer.data() || buffer.empty() || (buffer.size() > readBuffer.capacity()))
             return -EINVAL;
 
         do {
             bool doTransportRead{true};
             if (!readBuffer.empty()) {
                 // Try to decode the readBuffer before potentially blocking in the transportRead
-                result = decode(readFrame, readSequenceNumber, readBuffer, buffer, discardBytes);
+                result = decode(readFrame, readSequenceNumber, readBuffer.dataSpan(), buffer, discardBytes);
                 if (result >= 0) {
                     doTransportRead = false;
                 }
             }
 
             if (doTransportRead) {
-                if ((result = transportRead(transportReadBuffer)) <= 0)
+                if ((result = transportRead(readBuffer.unusedSpan())) <= 0)
                     return result;
 
-                // Insert the read data into the readBuffer for easier manipulation (e.g. erase)
-                readBuffer.insert(readBuffer.end(), transportReadBuffer.begin(), transportReadBuffer.begin() + result);
-                result = decode(readFrame, readSequenceNumber, readBuffer, buffer, discardBytes);
+                readBuffer.appendToTail(result);
+                result = decode(readFrame, readSequenceNumber, readBuffer.dataSpan(), buffer, discardBytes);
             }
 
             if (discardBytes > 0) {
@@ -186,26 +231,50 @@ class Hdlcpp {
         ControlTypeSelectiveReject,
     };
 
-    int encode(Frame &frame, uint8_t &sequenceNumber, const std::span<const uint8_t> source, std::vector<uint8_t> &destination)
+    template<typename T>
+    struct span {
+        constexpr span(std::span<T> span) : m_span(span), itr(span.begin()) {}
+
+        constexpr bool push_back(const T &value) {
+            if (itr < m_span.end()) {
+                *itr++ = value;
+                return true;
+            }
+            return false;
+        }
+
+        constexpr size_t size() {
+            return std::distance(m_span.begin(), itr);
+        }
+
+        std::span<T> m_span;
+        typename std::span<T>::iterator itr;
+    };
+
+    int encode(Frame &frame, uint8_t &sequenceNumber, const std::span<const uint8_t> source, Hdlcpp::span<uint8_t> destination)
     {
         uint8_t value = 0;
         uint16_t i, fcs16Value = Fcs16InitValue;
 
-        destination.push_back(FlagSequence);
+        if(!destination.push_back(FlagSequence))
+            return -EINVAL;
         fcs16Value = fcs16(fcs16Value, AllStationAddress);
-        escape(AllStationAddress, destination);
+        if(escape(AllStationAddress, destination) < 0)
+            return -EINVAL;
 
         value = encodeControlByte(frame, sequenceNumber);
         fcs16Value = fcs16(fcs16Value, value);
-        escape(value, destination);
+        if(escape(value, destination) < 0)
+            return -EINVAL;
 
         if (frame == FrameData) {
             if (!source.data() || source.empty())
                 return -EINVAL;
 
             for (const auto &byte : source) {
                 fcs16Value = fcs16(fcs16Value, byte);
-                escape(byte, destination);
+                if(escape(byte, destination) < 0)
+                    return -EINVAL;
             }
         }
 
@@ -214,15 +283,17 @@ class Hdlcpp {
 
         for (i = 0; i < sizeof(fcs16Value); i++) {
             value = ((fcs16Value >> (8 * i)) & 0xFF);
-            escape(value, destination);
+            if(escape(value, destination) < 0)
+                return -EINVAL;
         }
 
-        destination.push_back(FlagSequence);
+        if(!destination.push_back(FlagSequence))
+            return -EINVAL;
 
         return destination.size();
     }
 
-    int decode(Frame &frame, uint8_t &sequenceNumber, const std::span<uint8_t> source, std::span<uint8_t> destination, uint16_t &discardBytes)
+    int decode(Frame &frame, uint8_t &sequenceNumber, const std::span<uint8_t> source, std::span<uint8_t> destination, uint16_t &discardBytes) const
     {
         uint8_t value = 0;
         bool controlEscape = false;
@@ -298,22 +369,24 @@ class Hdlcpp {
     {
         int result;
 
-        writeBuffer.clear();
-
-        if ((result = encode(frame, sequenceNumber, data, writeBuffer)) < 0)
+        if ((result = encode(frame, sequenceNumber, data, {writeBuffer})) < 0)
             return result;
 
-        return transportWrite(writeBuffer);
+        return transportWrite(std::span(writeBuffer).first(result));
     }
 
-    void escape(uint8_t value, std::vector<uint8_t> &destination)
+    int escape(uint8_t value, Hdlcpp::span<uint8_t> &destination) const
     {
         if ((value == FlagSequence) || (value == ControlEscape)) {
-            destination.push_back(ControlEscape);
+            if(!destination.push_back(ControlEscape))
+                return -EINVAL;
             value ^= 0x20;
         }
 
-        destination.push_back(value);
+        if(!destination.push_back(value))
+            return -EINVAL;
+
+        return 0;
     }
 
     static uint8_t encodeControlByte(Frame frame, uint8_t sequenceNumber)
@@ -408,9 +481,8 @@ class Hdlcpp {
     std::mutex writeMutex;
     TransportRead transportRead;
     TransportWrite transportWrite;
-    std::vector<uint8_t> transportReadBuffer;
-    std::vector<uint8_t> readBuffer;
-    std::vector<uint8_t> writeBuffer;
+    Buffer<uint8_t, Capacity> readBuffer;
+    Container writeBuffer;
     Frame readFrame;
     uint16_t writeTimeout;
     uint8_t writeRetries;

python/CMakeLists.txt

@@ -1,4 +1,9 @@
 set(MODULE_NAME phdlcpp)
 
+if(NOT DEFINED PYTHON_HDLCPP_BUFFER_SIZE)
+    set(PYTHON_HDLCPP_BUFFER_SIZE 256)
+endif()
+
 pybind11_add_module(${MODULE_NAME} Phdlcpp.cpp)
+target_compile_definitions(${MODULE_NAME} PRIVATE -DPYTHON_HDLCPP_BUFFER_SIZE=${PYTHON_HDLCPP_BUFFER_SIZE})
 install(TARGETS ${MODULE_NAME} LIBRARY DESTINATION /usr/lib)

python/Phdlcpp.cpp

@@ -1,16 +1,18 @@
 #include <pybind11/pybind11.h>
 #include <pybind11/functional.h>
+#include <memory>
 #include "../include/Hdlcpp.hpp"
 
 using Pybind11Read = std::function<char(int)>;
 using Pybind11Write = std::function<int(pybind11::bytes)>;
+using Hdlcpp_t = Hdlcpp::Hdlcpp<PYTHON_HDLCPP_BUFFER_SIZE>;
 
 PYBIND11_MODULE(phdlcpp, m)
 {
-    pybind11::class_<Hdlcpp::Hdlcpp>(m, "Hdlcpp")
-        .def(pybind11::init([](Pybind11Read read, Pybind11Write write, uint16_t bufferSize,
+    pybind11::class_<Hdlcpp_t>(m, "Hdlcpp")
+        .def(pybind11::init([](Pybind11Read read, Pybind11Write write,
             uint16_t writeTimeout, uint8_t writeRetries) {
-            return std::unique_ptr<Hdlcpp::Hdlcpp>(new Hdlcpp::Hdlcpp(
+            return std::make_unique<Hdlcpp_t>(
                 [read](std::span<uint8_t> buffer) {
                     // Read a single byte as the python serial read will wait for all bytes to be present
                     uint8_t length = 1;
@@ -21,9 +23,9 @@ PYBIND11_MODULE(phdlcpp, m)
                 [write](const std::span<const uint8_t> buffer) {
                     return write(pybind11::bytes(reinterpret_cast<const char *>(buffer.data()), buffer.size()));
                 },
-                bufferSize, writeTimeout, writeRetries));
+                writeTimeout, writeRetries);
         }))
-        .def("read", [](Hdlcpp::Hdlcpp *hdlcpp, uint16_t length) {
+        .def("read", [](Hdlcpp_t *hdlcpp, uint16_t length) {
             int bytes;
             uint8_t data[length];
 
@@ -32,10 +34,10 @@ PYBIND11_MODULE(phdlcpp, m)
 
             return pybind11::bytes(reinterpret_cast<char *>(data), bytes);
         }, pybind11::call_guard<pybind11::gil_scoped_release>())
-        .def("write", [](Hdlcpp::Hdlcpp *hdlcpp, char *data, uint16_t length) {
+        .def("write", [](Hdlcpp_t *hdlcpp, char *data, uint16_t length) {
             return hdlcpp->write({reinterpret_cast<uint8_t *>(data), length});
         }, pybind11::call_guard<pybind11::gil_scoped_release>())
-        .def("close", [](Hdlcpp::Hdlcpp *hdlcpp) {
+        .def("close", [](Hdlcpp_t *hdlcpp) {
             hdlcpp->close();
         }, pybind11::call_guard<pybind11::gil_scoped_release>());
 }

python/hdlcpp.py

@@ -2,8 +2,8 @@
 import phdlcpp
 
 class Hdlcpp:
-    def __init__(self, transportRead, transportWrite, buffersize=256, writeTimeout=100, writeRetries=1):
-        self.hdlcpp = phdlcpp.Hdlcpp(transportRead, transportWrite, buffersize, writeTimeout, writeRetries)
+    def __init__(self, transportRead, transportWrite, writeTimeout=100, writeRetries=1):
+        self.hdlcpp = phdlcpp.Hdlcpp(transportRead, transportWrite, writeTimeout, writeRetries)
 
     def read(self, length):
         return self.hdlcpp.read(length)

python/hdlcppserial.py

@@ -3,9 +3,9 @@
 from hdlcpp import Hdlcpp
 
 class HdlcppSerial(Hdlcpp):
-    def __init__(self, port, baudrate=115200, bufferSize=256, writeTimeout=100, writeRetries=1):
+    def __init__(self, port, baudrate=115200, writeTimeout=100, writeRetries=1):
         self.serial = Serial(port, baudrate)
-        super().__init__(self._transportRead, self._transportWrite, bufferSize, writeTimeout, writeRetries)
+        super().__init__(self._transportRead, self._transportWrite, writeTimeout, writeRetries)
 
     def stop(self):
         self.serial.cancel_read()