Skip to content

Commit

Permalink
Removed raw pointers and using std::span<uint8_t> as interface (#32)
Browse files Browse the repository at this point in the history 
* std::span<uint8_t> 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
  • Loading branch information
@anedergaard
anedergaard authored Jun 23, 2022
1 parent 37e5449 commit 7d71e01
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 95 deletions.
16 changes: 12 additions & 4 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,25 @@ Hdlcpp requires that a transport read and write function is supplied as e.g. a [

```cpp
hdlcpp = std::make_shared<Hdlcpp::Hdlcpp>(
[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<uint8_t> buffer) { return serial->read(buffer); },
[this](const std::span<const uint8_t> 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<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);
```

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).

```cpp
protocol = std::make_shared<Protocol>(
[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<uint8_t> buffer) { return hdlcpp->read(buffer); },
[this](const std::span<const uint8_t> buffer) { return hdlcpp->write(buffer); });
```
## Python binding
Expand Down
63 changes: 32 additions & 31 deletions include/Hdlcpp.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,14 @@
#include <thread>
#include <atomic>
#include <cerrno>
#include <vector>
#include <functional>
#include <array>
#include <span>

namespace Hdlcpp {

using TransportRead = std::function<int(uint8_t *, uint16_t)>;
using TransportWrite = std::function<int(const uint8_t *, uint16_t)>;
using TransportRead = std::function<int(std::span<uint8_t> buffer)>;
using TransportWrite = std::function<int(const std::span<const uint8_t> buffer)>;

class Hdlcpp {
public:
Expand All @@ -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)
Expand All @@ -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 <cerrno>
virtual int read(uint8_t *data, uint16_t length)
virtual int read(std::span<uint8_t> 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) {
Expand All @@ -99,15 +100,15 @@ class Hdlcpp {
case FrameData:
if (++readSequenceNumber > 7)
readSequenceNumber = 0;
writeFrame(FrameAck, readSequenceNumber, nullptr, 0);
writeFrame(FrameAck, readSequenceNumber, {});
return result;
case FrameAck:
case FrameNack:
writeResult = readFrame;
break;
}
} else if ((result == -EIO) && (readFrame == FrameData)) {
writeFrame(FrameNack, readSequenceNumber, nullptr, 0);
writeFrame(FrameNack, readSequenceNumber, {});
}
} while (!stopped);

Expand All @@ -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 <cerrno>
virtual int write(const uint8_t *data, uint16_t length)
virtual int write(const std::span<const uint8_t> buffer)
{
int result;

if (!data || !length)
if (!buffer.data() || buffer.empty())
return -EINVAL;

std::lock_guard<std::mutex> writeLock(writeMutex);
Expand All @@ -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)
Expand All @@ -145,7 +146,7 @@ class Hdlcpp {
if (result == FrameNack)
break;

return length;
return buffer.size();
}

std::this_thread::sleep_for(std::chrono::milliseconds(1));
Expand Down Expand Up @@ -185,7 +186,7 @@ class Hdlcpp {
ControlTypeSelectiveReject,
};

int encode(Frame &frame, uint8_t &sequenceNumber, const uint8_t *source, uint16_t sourceLength, std::vector<uint8_t> &destination)
int encode(Frame &frame, uint8_t &sequenceNumber, const std::span<const uint8_t> source, std::vector<uint8_t> &destination)
{
uint8_t value = 0;
uint16_t i, fcs16Value = Fcs16InitValue;
Expand All @@ -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);
}
}

Expand All @@ -221,14 +222,14 @@ class Hdlcpp {
return destination.size();
}

int decode(Frame &frame, uint8_t &sequenceNumber, const std::vector<uint8_t> &source, uint8_t *destination, uint16_t destinationLength, uint16_t &discardBytes)
int decode(Frame &frame, uint8_t &sequenceNumber, const std::span<uint8_t> source, std::span<uint8_t> 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++) {
Expand Down Expand Up @@ -270,7 +271,7 @@ class Hdlcpp {
decodeControlByte(value, frame, sequenceNumber);
} else if (i > controlByteIndex) {
destination[destinationIndex++] = value;
}
}
}
}
}
Expand All @@ -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<const uint8_t> 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<uint8_t> &destination)
void escape(uint8_t value, std::vector<uint8_t> &destination)
{
if ((value == FlagSequence) || (value == ControlEscape)) {
destination.push_back(ControlEscape);
Expand All @@ -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;

Expand All @@ -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) {
Expand All @@ -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,
Expand Down
14 changes: 7 additions & 7 deletions python/Phdlcpp.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,29 @@ PYBIND11_MODULE(phdlcpp, m)
.def(pybind11::init([](Pybind11Read read, Pybind11Write write, uint16_t bufferSize,
uint16_t writeTimeout, uint8_t writeRetries) {
return std::unique_ptr<Hdlcpp::Hdlcpp>(new Hdlcpp::Hdlcpp(
[read](uint8_t *data, uint16_t length) {
[read](std::span<uint8_t> buffer) {
// Read a single byte as the python serial read will wait for all bytes to be present
length = 1;
data[0] = static_cast<uint8_t>(read(length));
uint8_t length = 1;
buffer[0] = static_cast<uint8_t>(read(length));

return length;
},
[write](const uint8_t *data, uint16_t length) {
return write(pybind11::bytes(reinterpret_cast<const char *>(data), length));
[write](const std::span<const uint8_t> buffer) {
return write(pybind11::bytes(reinterpret_cast<const char *>(buffer.data()), buffer.size()));
},
bufferSize, writeTimeout, writeRetries));
}))
.def("read", [](Hdlcpp::Hdlcpp *hdlcpp, uint16_t length) {
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<char *>(data), bytes);
}, pybind11::call_guard<pybind11::gil_scoped_release>())
.def("write", [](Hdlcpp::Hdlcpp *hdlcpp, char *data, uint16_t length) {
return hdlcpp->write(reinterpret_cast<uint8_t *>(data), length);
return hdlcpp->write({reinterpret_cast<uint8_t *>(data), length});
}, pybind11::call_guard<pybind11::gil_scoped_release>())
.def("close", [](Hdlcpp::Hdlcpp *hdlcpp) {
hdlcpp->close();
Expand Down
Loading

0 comments on commit 7d71e01

Please sign in to comment.