From 7d71e01c746eb5be041569210377bb9e4c6c9631 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20Bj=C3=B8rn=20Nedergaard?= Date: Thu, 23 Jun 2022 09:01:31 +0200 Subject: [PATCH] Removed raw pointers and using std::span as interface (#32) * std::span as interface * updated phdlcpp to use std::span * review comments on removing size_t as return type using move on lambda expressions/std::function made some functions static --- README.md | 16 ++++-- include/Hdlcpp.hpp | 63 ++++++++++++------------ python/Phdlcpp.cpp | 14 +++--- test/src/TestHdlcpp.cpp | 105 ++++++++++++++++++++-------------------- 4 files changed, 103 insertions(+), 95 deletions(-) diff --git a/README.md b/README.md index c50860f..0a3578e 100644 --- a/README.md +++ b/README.md @@ -9,8 +9,16 @@ Hdlcpp requires that a transport read and write function is supplied as e.g. a [ ```cpp hdlcpp = std::make_shared( - [this](uint8_t *data, uint16_t length) { return serial->read(data, length); }, - [this](const uint8_t *data, uint16_t length) { return serial->write(data, length); }, + [this](std::span buffer) { return serial->read(buffer); }, + [this](const std::span buffer) { return serial->write(buffer); }, + bufferSize, 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( + [this](std::span buffer) { return serial->read(buffer.data(), buffer.size()); }, + [this](const std::span buffer) { return serial->write(buffer.data(), buffer.size()); }, bufferSize, writeTimeout, writeRetries); ``` @@ -18,8 +26,8 @@ To read and write data using Hdlcpp the read and write functions are used. These ```cpp protocol = std::make_shared( - [this](uint8_t *data, uint16_t length) { return hdlcpp->read(data, length); }, - [this](const uint8_t *data, uint16_t length) { return hdlcpp->write(data, length); }); + [this](std::span buffer) { return hdlcpp->read(buffer); }, + [this](const std::span buffer) { return hdlcpp->write(buffer); }); ``` ## Python binding diff --git a/include/Hdlcpp.hpp b/include/Hdlcpp.hpp index 7488511..975278c 100644 --- a/include/Hdlcpp.hpp +++ b/include/Hdlcpp.hpp @@ -26,13 +26,14 @@ #include #include #include -#include #include +#include +#include namespace Hdlcpp { -using TransportRead = std::function; -using TransportWrite = std::function; +using TransportRead = std::function buffer)>; +using TransportWrite = std::function buffer)>; class Hdlcpp { public: @@ -44,8 +45,8 @@ class Hdlcpp { //! @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) - : transportRead(read) - , transportWrite(write) + : transportRead(std::move(read)) + , transportWrite(std::move(write)) , transportReadBuffer(bufferSize) , readFrame(FrameNack) , writeTimeout(writeTimeout) @@ -63,31 +64,31 @@ 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 - virtual int read(uint8_t *data, uint16_t length) + virtual int read(std::span buffer) { int result; uint16_t discardBytes; - if (!data || !length || (length > transportReadBuffer.size())) + if (!buffer.data() || buffer.empty() || (buffer.size() > transportReadBuffer.size())) 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, data, length, discardBytes); + result = decode(readFrame, readSequenceNumber, readBuffer, buffer, discardBytes); if (result >= 0) { doTransportRead = false; } } if (doTransportRead) { - if ((result = transportRead(transportReadBuffer.data(), length)) <= 0) + if ((result = transportRead(transportReadBuffer)) <= 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, data, length, discardBytes); + result = decode(readFrame, readSequenceNumber, readBuffer, buffer, discardBytes); } if (discardBytes > 0) { @@ -99,7 +100,7 @@ class Hdlcpp { case FrameData: if (++readSequenceNumber > 7) readSequenceNumber = 0; - writeFrame(FrameAck, readSequenceNumber, nullptr, 0); + writeFrame(FrameAck, readSequenceNumber, {}); return result; case FrameAck: case FrameNack: @@ -107,7 +108,7 @@ class Hdlcpp { break; } } else if ((result == -EIO) && (readFrame == FrameData)) { - writeFrame(FrameNack, readSequenceNumber, nullptr, 0); + writeFrame(FrameNack, readSequenceNumber, {}); } } while (!stopped); @@ -118,11 +119,11 @@ 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 - virtual int write(const uint8_t *data, uint16_t length) + virtual int write(const std::span buffer) { int result; - if (!data || !length) + if (!buffer.data() || buffer.empty()) return -EINVAL; std::lock_guard writeLock(writeMutex); @@ -133,7 +134,7 @@ class Hdlcpp { for (uint8_t tries = 0; tries <= writeRetries; tries++) { writeResult = -1; - if ((result = writeFrame(FrameData, writeSequenceNumber, data, length)) <= 0) + if ((result = writeFrame(FrameData, writeSequenceNumber, buffer)) <= 0) break; if (writeTimeout == 0) @@ -145,7 +146,7 @@ class Hdlcpp { if (result == FrameNack) break; - return length; + return buffer.size(); } std::this_thread::sleep_for(std::chrono::milliseconds(1)); @@ -185,7 +186,7 @@ class Hdlcpp { ControlTypeSelectiveReject, }; - int encode(Frame &frame, uint8_t &sequenceNumber, const uint8_t *source, uint16_t sourceLength, std::vector &destination) + int encode(Frame &frame, uint8_t &sequenceNumber, const std::span source, std::vector &destination) { uint8_t value = 0; uint16_t i, fcs16Value = Fcs16InitValue; @@ -199,12 +200,12 @@ class Hdlcpp { escape(value, destination); if (frame == FrameData) { - if (!source || !sourceLength) + if (!source.data() || source.empty()) return -EINVAL; - for (i = 0; i < sourceLength; i++) { - fcs16Value = fcs16(fcs16Value, source[i]); - escape(source[i], destination); + for (const auto &byte : source) { + fcs16Value = fcs16(fcs16Value, byte); + escape(byte, destination); } } @@ -221,14 +222,14 @@ class Hdlcpp { return destination.size(); } - int decode(Frame &frame, uint8_t &sequenceNumber, const std::vector &source, uint8_t *destination, uint16_t destinationLength, uint16_t &discardBytes) + int decode(Frame &frame, uint8_t &sequenceNumber, const std::span source, std::span destination, uint16_t &discardBytes) { uint8_t value = 0; bool controlEscape = false; uint16_t i, fcs16Value = Fcs16InitValue, sourceSize = source.size(); int result = -1, frameStartIndex = -1, frameStopIndex = -1, destinationIndex = 0; - if (!destination || !destinationLength) + if (!destination.data() || destination.empty()) return -EINVAL; for (i = 0; i < sourceSize; i++) { @@ -270,7 +271,7 @@ class Hdlcpp { decodeControlByte(value, frame, sequenceNumber); } else if (i > controlByteIndex) { destination[destinationIndex++] = value; - } + } } } } @@ -293,19 +294,19 @@ class Hdlcpp { return result; } - int writeFrame(Frame frame, uint8_t sequenceNumber, const uint8_t *data, uint16_t length) + int writeFrame(Frame frame, uint8_t sequenceNumber, const std::span data) { int result; writeBuffer.clear(); - if ((result = encode(frame, sequenceNumber, data, length, writeBuffer)) < 0) + if ((result = encode(frame, sequenceNumber, data, writeBuffer)) < 0) return result; - return transportWrite(writeBuffer.data(), writeBuffer.size()); + return transportWrite(writeBuffer); } - void escape(char value, std::vector &destination) + void escape(uint8_t value, std::vector &destination) { if ((value == FlagSequence) || (value == ControlEscape)) { destination.push_back(ControlEscape); @@ -315,7 +316,7 @@ class Hdlcpp { destination.push_back(value); } - uint8_t encodeControlByte(Frame frame, uint8_t sequenceNumber) + static uint8_t encodeControlByte(Frame frame, uint8_t sequenceNumber) { uint8_t value = 0; @@ -342,7 +343,7 @@ class Hdlcpp { return value; } - void decodeControlByte(uint8_t value, Frame &frame, uint8_t &sequenceNumber) + static void decodeControlByte(uint8_t value, Frame &frame, uint8_t &sequenceNumber) { // Check if the frame is a S-frame if ((value >> ControlSFrameBit) & 0x1) { @@ -363,7 +364,7 @@ class Hdlcpp { } } - uint16_t fcs16(uint16_t fcs16Value, uint8_t value) + static uint16_t fcs16(uint16_t fcs16Value, uint8_t value) { static const uint16_t fcs16ValueTable[256] = { 0x0000, 0x1189, 0x2312, 0x329b, 0x4624, 0x57ad, 0x6536, 0x74bf, 0x8c48, 0x9dc1, 0xaf5a, 0xbed3, 0xca6c, diff --git a/python/Phdlcpp.cpp b/python/Phdlcpp.cpp index f51fa63..acf2a44 100644 --- a/python/Phdlcpp.cpp +++ b/python/Phdlcpp.cpp @@ -11,15 +11,15 @@ PYBIND11_MODULE(phdlcpp, m) .def(pybind11::init([](Pybind11Read read, Pybind11Write write, uint16_t bufferSize, uint16_t writeTimeout, uint8_t writeRetries) { return std::unique_ptr(new Hdlcpp::Hdlcpp( - [read](uint8_t *data, uint16_t length) { + [read](std::span buffer) { // Read a single byte as the python serial read will wait for all bytes to be present - length = 1; - data[0] = static_cast(read(length)); + uint8_t length = 1; + buffer[0] = static_cast(read(length)); return length; }, - [write](const uint8_t *data, uint16_t length) { - return write(pybind11::bytes(reinterpret_cast(data), length)); + [write](const std::span buffer) { + return write(pybind11::bytes(reinterpret_cast(buffer.data()), buffer.size())); }, bufferSize, writeTimeout, writeRetries)); })) @@ -27,13 +27,13 @@ PYBIND11_MODULE(phdlcpp, m) int bytes; uint8_t data[length]; - if ((bytes = hdlcpp->read(data, length)) < 0) + if ((bytes = hdlcpp->read({data, length})) < 0) bytes = 0; return pybind11::bytes(reinterpret_cast(data), bytes); }, pybind11::call_guard()) .def("write", [](Hdlcpp::Hdlcpp *hdlcpp, char *data, uint16_t length) { - return hdlcpp->write(reinterpret_cast(data), length); + return hdlcpp->write({reinterpret_cast(data), length}); }, pybind11::call_guard()) .def("close", [](Hdlcpp::Hdlcpp *hdlcpp) { hdlcpp->close(); diff --git a/test/src/TestHdlcpp.cpp b/test/src/TestHdlcpp.cpp index 12690f3..f9ee804 100644 --- a/test/src/TestHdlcpp.cpp +++ b/test/src/TestHdlcpp.cpp @@ -9,8 +9,8 @@ class HdlcppFixture { HdlcppFixture() { hdlcpp = std::make_shared( - [this](uint8_t *data, uint16_t length) { return transportRead(data, length); }, - [this](const uint8_t *data, uint16_t length) { return transportWrite(data, length); }, + [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 ); @@ -19,17 +19,16 @@ class HdlcppFixture { hdlcpp->stopped = true; } - int transportRead(uint8_t *data, uint16_t length) + size_t transportRead(std::span buffer) { - (void)length; - std::memcpy(data, readBuffer.data(), readBuffer.size()); + std::memcpy(buffer.data(), readBuffer.data(), readBuffer.size()); return readBuffer.size(); } - int transportWrite(const uint8_t *data, uint16_t length) + size_t transportWrite(const std::span buffer) { - writeBuffer.assign((char *)data, (char *)data + length); - return length; + writeBuffer.assign(buffer.begin(), buffer.end()); + return writeBuffer.size(); } const uint16_t bufferSize = 64; @@ -42,68 +41,68 @@ class HdlcppFixture { std::shared_ptr hdlcpp; std::vector readBuffer; std::vector writeBuffer; - uint8_t buffer[10]; + uint8_t dataBuffer[10]; }; TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") { SECTION("Test write with invalid input") { - CHECK(hdlcpp->write(buffer, 0) == -EINVAL); - CHECK(hdlcpp->write(nullptr, sizeof(buffer)) == -EINVAL); + CHECK(hdlcpp->write({dataBuffer, 0}) == -EINVAL); + CHECK(hdlcpp->write({}) == -EINVAL); } SECTION("Test write with valid 1 byte data input") { - hdlcpp->write(&frameData[3], 1); + hdlcpp->write({&frameData[3], 1}); CHECK(std::memcmp(frameData, writeBuffer.data(), sizeof(frameData)) == 0); } SECTION("Test write/read with FlagSequence as data input") { - hdlcpp->write(&hdlcpp->FlagSequence, 1); + hdlcpp->write({&hdlcpp->FlagSequence, 1}); // Size should be 1 byte more compared to a 1 byte data frame due to escape of the value CHECK(writeBuffer.size() == (sizeof(frameData) + 1)); readBuffer = writeBuffer; - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == hdlcpp->FlagSequence); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == hdlcpp->FlagSequence); } SECTION("Test write/read with ControlEscape as data input") { - hdlcpp->write(&hdlcpp->ControlEscape, 1); + hdlcpp->write({&hdlcpp->ControlEscape, 1}); // Size should be 1 byte more compared to a 1 byte data frame due to escape of the value CHECK(writeBuffer.size() == (sizeof(frameData) + 1)); readBuffer = writeBuffer; - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == hdlcpp->ControlEscape); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == hdlcpp->ControlEscape); } SECTION("Test read with invalid input") { - CHECK(hdlcpp->read(buffer, 0) == -EINVAL); - CHECK(hdlcpp->read(nullptr, sizeof(buffer)) == -EINVAL); + CHECK(hdlcpp->read({dataBuffer, 0}) == -EINVAL); + CHECK(hdlcpp->read({}) == -EINVAL); // This should fail as the buffer size is configured to less - CHECK(hdlcpp->read(buffer, 256) == -EINVAL); + CHECK(hdlcpp->read({dataBuffer, 256}) == -EINVAL); readBuffer.assign(frameDataInvalid, frameDataInvalid + sizeof(frameDataInvalid)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == -EIO); + CHECK(hdlcpp->read(dataBuffer) == -EIO); CHECK(std::memcmp(frameNack, writeBuffer.data(), sizeof(frameNack)) == 0); } SECTION("Test read of two valid 1 byte data frames") { readBuffer.assign(frameData, frameData + sizeof(frameData)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); } @@ -111,21 +110,21 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") { // Add the first 3 bytes to be read readBuffer.assign(frameData, frameData + 3); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == -ENOMSG); + CHECK(hdlcpp->read(dataBuffer) == -ENOMSG); CHECK(writeBuffer.empty()); // Now add the remaining bytes to complete the data frame readBuffer.assign(frameData + 3, frameData + sizeof(frameData)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); } SECTION("Test read of valid 1 byte data frame with double flag sequence") { readBuffer.assign(frameDataDoubleFlagSequence, frameDataDoubleFlagSequence + sizeof(frameDataDoubleFlagSequence)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); } @@ -133,13 +132,13 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") { readBuffer.assign(frameData, frameData + sizeof(frameData)); // One complete frame readBuffer.insert(readBuffer.end(), frameData, frameData + 3); // A partial frame - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); readBuffer.assign(frameData + 3, frameData + sizeof(frameData)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); } @@ -149,15 +148,15 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") readBuffer.insert(readBuffer.end(), frameData, frameData + sizeof(frameData)); // Read first frame - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); readBuffer.clear(); - + // This must read the second frame without reading from transport layer - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 1); - CHECK(buffer[0] == frameData[3]); + CHECK(hdlcpp->read(dataBuffer) == 1); + CHECK(dataBuffer[0] == frameData[3]); CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); } @@ -165,7 +164,7 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") { hdlcpp->writeSequenceNumber = 1; readBuffer.assign(frameAck, frameAck + sizeof(frameAck)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 0); + CHECK(hdlcpp->read(dataBuffer) == 0); CHECK(hdlcpp->writeResult == Hdlcpp::Hdlcpp::FrameAck); } @@ -173,7 +172,7 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") { hdlcpp->writeSequenceNumber = 1; readBuffer.assign(frameNack, frameNack + sizeof(frameNack)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 0); + CHECK(hdlcpp->read(dataBuffer) == 0); CHECK(hdlcpp->writeResult == Hdlcpp::Hdlcpp::FrameNack); } @@ -184,11 +183,11 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") uint8_t dataValue = 0x55, encodeSequenceNumber = 3, decodeSequenceNumber = 0; 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, buffer, sizeof(buffer), discardBytes) > 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(buffer[0] == dataValue); + CHECK(dataBuffer[0] == dataValue); } SECTION("Test close function") @@ -209,21 +208,21 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") const uint8_t frame7[] = { 0x7e, 0xff, 0x21, 0x0c, 0xc0, 0x7e }; readBuffer.assign(frame1, frame1 + sizeof(frame1)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 2); + CHECK(hdlcpp->read(dataBuffer) == 2); readBuffer.assign(frame2, frame2 + sizeof(frame2)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == -EIO); + CHECK(hdlcpp->read(dataBuffer) == -EIO); readBuffer.assign(frame3, frame3 + sizeof(frame3)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 9); + CHECK(hdlcpp->read(dataBuffer) == 9); readBuffer.assign(frame4, frame4 + sizeof(frame4)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == -ENOMSG); + CHECK(hdlcpp->read(dataBuffer) == -ENOMSG); readBuffer.assign(frame5, frame5 + sizeof(frame5)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == -ENOMSG); + CHECK(hdlcpp->read(dataBuffer) == -ENOMSG); readBuffer.assign(frame6, frame6 + sizeof(frame6)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 9); + CHECK(hdlcpp->read(dataBuffer) == 9); readBuffer.assign(frame7, frame7 + sizeof(frame7)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 0); + CHECK(hdlcpp->read(dataBuffer) == 0); // Check that the initial frame can still be decoded successfully readBuffer.assign(frame1, frame1 + sizeof(frame1)); - CHECK(hdlcpp->read(buffer, sizeof(buffer)) == 2); + CHECK(hdlcpp->read(dataBuffer) == 2); } }