Changed from internal allocation of buffers to external for better me…

…mory control (#35) * pass buffers as spans to allow for external placement of buffers and allow difference read and write buffer sizes * removing cmake default python buffer size * updated readme * moved using declarations out of Hdlcpp class and into Hdlcpp namespace replaced std::span with using declarations in unit test, testing with both std::array and std::vector as buffers
bang-olufsen · Jul 8, 2022 · eb97c1c · eb97c1c
1 parent f0df7fa
commit eb97c1c
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 54 deletions.
diff --git a/README.md b/README.md
@@ -5,21 +5,21 @@ Hdlcpp is a header-only C++11 framing protocol optimized for embedded communicat
 
 ## Usage
 
-Hdlcpp requires that a transport read and write function is supplied as e.g. a [lambda expression](https://en.cppreference.com/w/cpp/language/lambda) for the actual data transport. Hereby Hdlcpp can easily be integrated e.g. with different UART implementations. It is also possible to increase the buffer size for encoding/decoding data, write timeout and number of write retries when constructing the instance.
+Hdlcpp requires that a transport read and write function is supplied as e.g. a [lambda expression](https://en.cppreference.com/w/cpp/language/lambda) for the actual data transport. Hereby Hdlcpp can easily be integrated e.g. with different UART implementations. It required a read and write buffer. The buffer type must to compatible with [std::span](https://en.cppreference.com/w/cpp/container/span), basically any contiguous sequence containers. The buffers can be size independent for encoding/decoding data, write timeout and number of write retries are also configurable when constructing the instance.
 
 ```cpp
 hdlcpp = std::make_shared<Hdlcpp::Hdlcpp>(
     [this](std::span<uint8_t> buffer) { return serial->read(buffer); },
     [this](const std::span<const uint8_t> buffer) { return serial->write(buffer); },
-    bufferSize, writeTimeout, writeRetries);
+    readBuffer, writeBuffer, writeTimeout, writeRetries);
 ```
 
 In the case where the underlying transport layer does not support `std::span`, the pointer to the first element and the size can be extracted from the span like so.
 ```cpp
 hdlcpp = std::make_shared<Hdlcpp::Hdlcpp>(
     [this](std::span<uint8_t> buffer) { return serial->read(buffer.data(), buffer.size()); },
     [this](const std::span<const uint8_t> buffer) { return serial->write(buffer.data(), buffer.size()); },
-    bufferSize, writeTimeout, writeRetries);
+    readBuffer, writeBuffer, writeTimeout, writeRetries);
 ```
 
 To read and write data using Hdlcpp the read and write functions are used. These could again e.g. be used as lambdas expressions to a protocol implementation (layered architecture). The protocol could e.g. be [nanopb](https://github.com/nanopb/nanopb).
@@ -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.

diff --git a/include/Hdlcpp.hpp b/include/Hdlcpp.hpp
@@ -32,20 +32,20 @@
 
 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>
+template<typename T>
 class Buffer {
-    using Container = std::array<T, Capacity>;
+    using Container = std::span<T>;
 
 public:
+    Buffer(Container buffer)
+    : m_buffer(buffer) {}
+
     [[nodiscard]] bool empty() const {
         return std::span<typename Container::value_type>(m_head, m_tail).empty();
     }
 
     constexpr size_t capacity() {
-        return Capacity;
+        return m_buffer.size();
     }
 
     typename Container::iterator begin() {
@@ -56,11 +56,11 @@ class Buffer {
         return m_tail;
     }
 
-    std::span<uint8_t> dataSpan() {
+    Container dataSpan() {
         return {m_head, m_tail};
     }
 
-    std::span<uint8_t> unusedSpan() {
+    Container unusedSpan() {
         return {m_tail, m_buffer.end()};
     }
 
@@ -70,7 +70,7 @@ class Buffer {
 
     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);
+            Container toBeMoved(last, m_tail);
             for (const auto &byte: toBeMoved) {
                 *first++ = byte;
             }
@@ -85,20 +85,35 @@ class Buffer {
     typename Container::iterator m_tail{ m_buffer.begin() };
 };
 
-//! @param Capacity The buffer size to be allocated for encoding/decoding frames
+using Container = std::span<uint8_t>;
+using ConstContainer = const std::span<const Container::element_type>;
+using value_type = Container::value_type;
+
+template<size_t Capacity>
+using StaticBuffer = std::array<Container::value_type, Capacity>;
+
 template<size_t Capacity>
+struct Calculate {
+    // For details see: https://en.wikipedia.org/wiki/High-Level_Data_Link_Control#Structure
+    static constexpr size_t WithOverhead{ Capacity * 2 + 8 };
+};
+
+using TransportRead = std::function<int(Container buffer)>;
+using TransportWrite = std::function<int(ConstContainer buffer)>;
+
+//! @param Capacity The buffer size to be allocated for encoding/decoding frames
 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 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 writeTimeout = 100, uint8_t writeRetries = 1)
+    Hdlcpp(TransportRead read, TransportWrite write, Container readBuffer, Container writeBuffer, uint16_t writeTimeout = 100, uint8_t writeRetries = 1)
         : transportRead(std::move(read))
         , transportWrite(std::move(write))
+        , readBuffer(readBuffer)
+        , writeBuffer(writeBuffer)
         , readFrame(FrameNack)
         , writeTimeout(writeTimeout)
         , writeRetries(writeRetries) { }
@@ -110,7 +125,7 @@ class Hdlcpp {
     //! @param data A pointer to an allocated buffer (should be bigger than max frame length)
     //! @param length The length of the allocated buffer
     //! @return The number of bytes received if positive or an error code from <cerrno>
-    virtual int read(std::span<uint8_t> buffer)
+    virtual int read(Container buffer)
     {
         int result;
         uint16_t discardBytes;
@@ -164,7 +179,7 @@ class Hdlcpp {
     //! @param data A pointer to the data to be sent
     //! @param length The length of the data to be sent
     //! @return The number of bytes sent if positive or an error code from <cerrno>
-    virtual int write(const std::span<const uint8_t> buffer)
+    virtual int write(ConstContainer buffer)
     {
         int result;
 
@@ -251,7 +266,7 @@ class Hdlcpp {
         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)
+    int encode(Frame &frame, uint8_t &sequenceNumber, ConstContainer source, Hdlcpp::span<uint8_t> destination)
     {
         uint8_t value = 0;
         uint16_t i, fcs16Value = Fcs16InitValue;
@@ -293,7 +308,7 @@ class Hdlcpp {
         return destination.size();
     }
 
-    int decode(Frame &frame, uint8_t &sequenceNumber, const std::span<uint8_t> source, std::span<uint8_t> destination, uint16_t &discardBytes) const
+    int decode(Frame &frame, uint8_t &sequenceNumber, const Container source, Container destination, uint16_t &discardBytes) const
     {
         uint8_t value = 0;
         bool controlEscape = false;
@@ -365,7 +380,7 @@ class Hdlcpp {
         return result;
     }
 
-    int writeFrame(Frame frame, uint8_t sequenceNumber, const std::span<const uint8_t> data)
+    int writeFrame(Frame frame, uint8_t sequenceNumber, ConstContainer data)
     {
         int result;
 
@@ -481,7 +496,7 @@ class Hdlcpp {
     std::mutex writeMutex;
     TransportRead transportRead;
     TransportWrite transportWrite;
-    Buffer<uint8_t, Capacity> readBuffer;
+    Buffer<uint8_t> readBuffer;
     Container writeBuffer;
     Frame readFrame;
     uint16_t writeTimeout;

diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt
@@ -1,9 +1,5 @@
 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)
diff --git a/python/Phdlcpp.cpp b/python/Phdlcpp.cpp
@@ -5,27 +5,29 @@
 
 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_t>(m, "Hdlcpp")
-        .def(pybind11::init([](Pybind11Read read, Pybind11Write write,
+    pybind11::class_<Hdlcpp::Hdlcpp>(m, "Hdlcpp")
+        .def(pybind11::init([](Pybind11Read read, Pybind11Write write, size_t bufferSize,
             uint16_t writeTimeout, uint8_t writeRetries) {
-            return std::make_unique<Hdlcpp_t>(
-                [read](std::span<uint8_t> buffer) {
+            std::vector<Hdlcpp::value_type> readBuffer(bufferSize), writeBuffer(bufferSize);
+            return std::make_unique<Hdlcpp::Hdlcpp>(
+                [read](Hdlcpp::Container buffer) {
                     // Read a single byte as the python serial read will wait for all bytes to be present
                     uint8_t length = 1;
                     buffer[0] = static_cast<uint8_t>(read(length));
 
                     return length;
                 },
-                [write](const std::span<const uint8_t> buffer) {
+                [write](Hdlcpp::ConstContainer buffer) {
                     return write(pybind11::bytes(reinterpret_cast<const char *>(buffer.data()), buffer.size()));
                 },
+                readBuffer,
+                writeBuffer,
                 writeTimeout, writeRetries);
         }))
-        .def("read", [](Hdlcpp_t *hdlcpp, uint16_t length) {
+        .def("read", [](Hdlcpp::Hdlcpp *hdlcpp, uint16_t length) {
             int bytes;
             uint8_t data[length];
 
@@ -34,10 +36,10 @@ PYBIND11_MODULE(phdlcpp, m)
 
             return pybind11::bytes(reinterpret_cast<char *>(data), bytes);
         }, pybind11::call_guard<pybind11::gil_scoped_release>())
-        .def("write", [](Hdlcpp_t *hdlcpp, char *data, uint16_t length) {
+        .def("write", [](Hdlcpp::Hdlcpp *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_t *hdlcpp) {
+        .def("close", [](Hdlcpp::Hdlcpp *hdlcpp) {
             hdlcpp->close();
         }, pybind11::call_guard<pybind11::gil_scoped_release>());
 }
diff --git a/python/hdlcpp.py b/python/hdlcpp.py
@@ -2,8 +2,8 @@
 import phdlcpp
 
 class Hdlcpp:
-    def __init__(self, transportRead, transportWrite, writeTimeout=100, writeRetries=1):
-        self.hdlcpp = phdlcpp.Hdlcpp(transportRead, transportWrite, writeTimeout, writeRetries)
+    def __init__(self, transportRead, transportWrite, bufferze = 256, writeTimeout=100, writeRetries=1):
+        self.hdlcpp = phdlcpp.Hdlcpp(transportRead, transportWrite, bufferze, writeTimeout, writeRetries)
 
     def read(self, length):
         return self.hdlcpp.read(length)

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

diff --git a/test/src/TestHdlcpp.cpp b/test/src/TestHdlcpp.cpp
@@ -7,26 +7,27 @@
 class HdlcppFixture {
     static constexpr uint16_t bufferSize = 64;
 public:
-    using Hdlcpp_t = Hdlcpp::Hdlcpp<bufferSize>;
-    HdlcppFixture()
+    HdlcppFixture() : hdlcpp_writeBuffer(bufferSize)
     {
-        hdlcpp = std::make_shared<Hdlcpp_t>(
-            [this](std::span<uint8_t> buffer) { return transportRead(buffer); },
-            [this](const std::span<const uint8_t> buffer) { return transportWrite(buffer); },
+        hdlcpp = std::make_shared<Hdlcpp::Hdlcpp>(
+            [this](Hdlcpp::Container buffer) { return transportRead(buffer); },
+            [this](Hdlcpp::ConstContainer buffer) { return transportWrite(buffer); },
+            hdlcpp_readBuffer,
+            hdlcpp_writeBuffer,
             1 // Use a 1 ms timeout to speed up tests
             );
 
         // For testing only execute a single iteration instead of blocking
         hdlcpp->stopped = true;
     }
 
-    size_t transportRead(std::span<uint8_t> buffer)
+    size_t transportRead(Hdlcpp::Container buffer)
     {
         std::memcpy(buffer.data(), readBuffer.data(), readBuffer.size());
         return readBuffer.size();
     }
 
-    size_t transportWrite(const std::span<const uint8_t> buffer)
+    size_t transportWrite(Hdlcpp::ConstContainer buffer)
     {
         writeBuffer.assign(buffer.begin(), buffer.end());
         return writeBuffer.size();
@@ -38,7 +39,9 @@ class HdlcppFixture {
     const uint8_t frameDataInvalid[7] = { 0x7e, 0xff, 0x12, 0x33, 0x67, 0xf8, 0x7e };
     const uint8_t frameDataDoubleFlagSequence[9] = { 0x7e, 0x7e, 0xff, 0x12, 0x55, 0x36, 0xa3, 0x7e, 0x7e };
 
-    std::shared_ptr<Hdlcpp_t> hdlcpp;
+    std::shared_ptr<Hdlcpp::Hdlcpp> hdlcpp;
+    Hdlcpp::StaticBuffer<Hdlcpp::Calculate<bufferSize>::WithOverhead> hdlcpp_readBuffer{};
+    std::vector<Hdlcpp::value_type> hdlcpp_writeBuffer{};
     std::vector<uint8_t> readBuffer;
     std::vector<uint8_t> writeBuffer;
     uint8_t dataBuffer[10];
@@ -165,23 +168,23 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]")
         hdlcpp->writeSequenceNumber = 1;
         readBuffer.assign(frameAck, frameAck + sizeof(frameAck));
         CHECK(hdlcpp->read(dataBuffer) == 0);
-        CHECK(hdlcpp->writeResult == Hdlcpp_t::FrameAck);
+        CHECK(hdlcpp->writeResult == Hdlcpp::Hdlcpp::FrameAck);
     }
 
     SECTION("Test read of nack frame")
     {
         hdlcpp->writeSequenceNumber = 1;
         readBuffer.assign(frameNack, frameNack + sizeof(frameNack));
         CHECK(hdlcpp->read(dataBuffer) == 0);
-        CHECK(hdlcpp->writeResult == Hdlcpp_t::FrameNack);
+        CHECK(hdlcpp->writeResult == Hdlcpp::Hdlcpp::FrameNack);
     }
 
     SECTION("Test encode/decode functions with 1 byte data")
     {
         std::array<uint8_t, 256> data{};
         uint16_t discardBytes = 0;
         uint8_t dataValue = 0x55, encodeSequenceNumber = 3, decodeSequenceNumber = 0;
-        Hdlcpp_t::Frame encodeFrame = Hdlcpp_t::FrameData, decodeFrame = Hdlcpp_t::FrameNack;
+        Hdlcpp::Hdlcpp::Frame encodeFrame = Hdlcpp::Hdlcpp::FrameData, decodeFrame = Hdlcpp::Hdlcpp::FrameNack;
 
         CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {&dataValue, sizeof(dataValue)}, {data}) > 0);
         CHECK(hdlcpp->decode(decodeFrame, decodeSequenceNumber, data, dataBuffer, discardBytes) > 0);
@@ -193,7 +196,7 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]")
     SECTION("Test encode buffer too small")
     {
         // Slowly increasing buffer size to traverse down Hdlcpp::encode function.
-        Hdlcpp_t::Frame encodeFrame = Hdlcpp_t::FrameData;
+        Hdlcpp::Hdlcpp::Frame encodeFrame = Hdlcpp::Hdlcpp::FrameData;
         uint8_t dataValue = 0x55, encodeSequenceNumber = 3;
 
         {
@@ -235,7 +238,7 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]")
     SECTION("Test escape in a too small buffer")
     {
         std::array<uint8_t, 0> buffer{};
-        Hdlcpp_t::span<uint8_t> span(buffer);
+        Hdlcpp::Hdlcpp::span<uint8_t> span(buffer);
         const auto value {GENERATE(0x7e, 0x7d)};
         CHECK(hdlcpp->escape(value, span) == -EINVAL);
     }
@@ -280,7 +283,7 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]")
     {
         std::array<uint8_t, 1> buffer{};
 
-        Hdlcpp::Hdlcpp<1>::span<uint8_t> span(buffer);
+        Hdlcpp::Hdlcpp::span<uint8_t> span(buffer);
 
         CHECK(span.push_back(1));
         CHECK_FALSE(span.push_back(2));