From f0df7fadfa4eed76df43e2b18ae5be1183360375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20Bj=C3=B8rn=20Nedergaard?= Date: Thu, 7 Jul 2022 08:16:50 +0200 Subject: [PATCH] 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 | 2 +- include/Hdlcpp.hpp | 140 ++++++++++++++++++++++++++++++---------- python/CMakeLists.txt | 5 ++ python/Phdlcpp.cpp | 16 +++-- python/hdlcpp.py | 4 +- python/hdlcppserial.py | 4 +- test/src/TestHdlcpp.cpp | 81 ++++++++++++++++++++--- 7 files changed, 197 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 0a3578e..0c72b3e 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ protocol = std::make_shared( ## 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=` 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 index 975278c..9e23f75 100644 --- a/include/Hdlcpp.hpp +++ b/include/Hdlcpp.hpp @@ -35,27 +35,73 @@ namespace Hdlcpp { using TransportRead = std::function buffer)>; using TransportWrite = std::function buffer)>; +template +class Buffer { + using Container = std::array; + +public: + [[nodiscard]] bool empty() const { + return std::span(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 dataSpan() { + return {m_head, m_tail}; + } + + std::span 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 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 class Hdlcpp { + static_assert(Capacity > 0, "HDLCPP requires a buffer size larger than 0"); + using Container = std::array; 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,18 +231,41 @@ class Hdlcpp { ControlTypeSelectiveReject, }; - int encode(Frame &frame, uint8_t &sequenceNumber, const std::span source, std::vector &destination) + template + struct span { + constexpr span(std::span 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 m_span; + typename std::span::iterator itr; + }; + + int encode(Frame &frame, uint8_t &sequenceNumber, const std::span source, Hdlcpp::span 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()) @@ -205,7 +273,8 @@ class Hdlcpp { 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 source, std::span destination, uint16_t &discardBytes) + int decode(Frame &frame, uint8_t &sequenceNumber, const std::span source, std::span 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 &destination) + int escape(uint8_t value, Hdlcpp::span &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 transportReadBuffer; - std::vector readBuffer; - std::vector writeBuffer; + Buffer readBuffer; + Container writeBuffer; Frame readFrame; uint16_t writeTimeout; uint8_t writeRetries; diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index aca943a..e03cddb 100644 --- a/python/CMakeLists.txt +++ b/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) diff --git a/python/Phdlcpp.cpp b/python/Phdlcpp.cpp index acf2a44..b93411e 100644 --- a/python/Phdlcpp.cpp +++ b/python/Phdlcpp.cpp @@ -1,16 +1,18 @@ #include #include +#include #include "../include/Hdlcpp.hpp" using Pybind11Read = std::function; using Pybind11Write = std::function; +using Hdlcpp_t = Hdlcpp::Hdlcpp; PYBIND11_MODULE(phdlcpp, m) { - pybind11::class_(m, "Hdlcpp") - .def(pybind11::init([](Pybind11Read read, Pybind11Write write, uint16_t bufferSize, + pybind11::class_(m, "Hdlcpp") + .def(pybind11::init([](Pybind11Read read, Pybind11Write write, uint16_t writeTimeout, uint8_t writeRetries) { - return std::unique_ptr(new Hdlcpp::Hdlcpp( + return std::make_unique( [read](std::span 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 buffer) { return write(pybind11::bytes(reinterpret_cast(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(data), bytes); }, pybind11::call_guard()) - .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(data), length}); }, pybind11::call_guard()) - .def("close", [](Hdlcpp::Hdlcpp *hdlcpp) { + .def("close", [](Hdlcpp_t *hdlcpp) { hdlcpp->close(); }, pybind11::call_guard()); } diff --git a/python/hdlcpp.py b/python/hdlcpp.py index ac52d3e..baeb3ee 100644 --- a/python/hdlcpp.py +++ b/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) diff --git a/python/hdlcppserial.py b/python/hdlcppserial.py index b3ca7b2..421da77 100644 --- 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, 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() diff --git a/test/src/TestHdlcpp.cpp b/test/src/TestHdlcpp.cpp index f9ee804..9510cf2 100644 --- a/test/src/TestHdlcpp.cpp +++ b/test/src/TestHdlcpp.cpp @@ -5,13 +5,14 @@ #include "Hdlcpp.hpp" class HdlcppFixture { + static constexpr uint16_t bufferSize = 64; public: + using Hdlcpp_t = Hdlcpp::Hdlcpp; HdlcppFixture() { - hdlcpp = std::make_shared( + hdlcpp = std::make_shared( [this](std::span buffer) { return transportRead(buffer); }, [this](const std::span buffer) { return transportWrite(buffer); }, - bufferSize, 1 // Use a 1 ms timeout to speed up tests ); @@ -31,14 +32,13 @@ class HdlcppFixture { return writeBuffer.size(); } - const uint16_t bufferSize = 64; const uint8_t frameAck[6] = { 0x7e, 0xff, 0x41, 0x0a, 0xa3, 0x7e }; const uint8_t frameNack[6] = { 0x7e, 0xff, 0x29, 0x44, 0x4c, 0x7e }; const uint8_t frameData[7] = { 0x7e, 0xff, 0x12, 0x55, 0x36, 0xa3, 0x7e }; 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; + std::shared_ptr hdlcpp; std::vector readBuffer; std::vector writeBuffer; uint8_t dataBuffer[10]; @@ -165,7 +165,7 @@ 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::Hdlcpp::FrameAck); + CHECK(hdlcpp->writeResult == Hdlcpp_t::FrameAck); } SECTION("Test read of nack frame") @@ -173,23 +173,73 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") hdlcpp->writeSequenceNumber = 1; readBuffer.assign(frameNack, frameNack + sizeof(frameNack)); CHECK(hdlcpp->read(dataBuffer) == 0); - CHECK(hdlcpp->writeResult == Hdlcpp::Hdlcpp::FrameNack); + CHECK(hdlcpp->writeResult == Hdlcpp_t::FrameNack); } SECTION("Test encode/decode functions with 1 byte data") { - std::vector data; + std::array data{}; uint16_t discardBytes = 0; uint8_t dataValue = 0x55, encodeSequenceNumber = 3, decodeSequenceNumber = 0; - Hdlcpp::Hdlcpp::Frame encodeFrame = Hdlcpp::Hdlcpp::FrameData, decodeFrame = Hdlcpp::Hdlcpp::FrameNack; + Hdlcpp_t::Frame encodeFrame = Hdlcpp_t::FrameData, decodeFrame = Hdlcpp_t::FrameNack; - CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {&dataValue, sizeof(dataValue)}, data) > 0); + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {&dataValue, sizeof(dataValue)}, {data}) > 0); CHECK(hdlcpp->decode(decodeFrame, decodeSequenceNumber, data, dataBuffer, discardBytes) > 0); CHECK(encodeFrame == decodeFrame); CHECK(encodeSequenceNumber == decodeSequenceNumber); CHECK(dataBuffer[0] == dataValue); } + SECTION("Test encode buffer too small") + { + // Slowly increasing buffer size to traverse down Hdlcpp::encode function. + Hdlcpp_t::Frame encodeFrame = Hdlcpp_t::FrameData; + uint8_t dataValue = 0x55, encodeSequenceNumber = 3; + + { + std::array buffer{}; + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {}, {buffer}) == -EINVAL); + } + + { + std::array buffer{}; + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {}, {buffer}) == -EINVAL); + } + + { + std::array buffer{}; + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {}, {buffer}) == -EINVAL); + } + + { + std::array buffer{}; + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {}, {buffer}) == -EINVAL); + } + + { + std::array buffer{}; + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {&dataValue, sizeof(dataValue)}, {buffer}) == -EINVAL); + } + + { + std::array buffer{}; + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {&dataValue, sizeof(dataValue)}, {buffer}) == -EINVAL); + } + + { + std::array buffer{}; + CHECK(hdlcpp->encode(encodeFrame, encodeSequenceNumber, {&dataValue, sizeof(dataValue)}, {buffer}) == -EINVAL); + } + } + + SECTION("Test escape in a too small buffer") + { + std::array buffer{}; + Hdlcpp_t::span span(buffer); + const auto value {GENERATE(0x7e, 0x7d)}; + CHECK(hdlcpp->escape(value, span) == -EINVAL); + } + SECTION("Test close function") { hdlcpp->close(); @@ -225,4 +275,17 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") readBuffer.assign(frame1, frame1 + sizeof(frame1)); CHECK(hdlcpp->read(dataBuffer) == 2); } + + SECTION("Test push_back on full buffer") + { + std::array buffer{}; + + Hdlcpp::Hdlcpp<1>::span span(buffer); + + CHECK(span.push_back(1)); + CHECK_FALSE(span.push_back(2)); + CHECK(buffer.back() == 1); + } + + }