From 7c7a8a5df7510e9a35f6bd6f88d21cad54454548 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 11 Sep 2021 19:18:18 +0200 Subject: [PATCH 1/3] added improvements from code review --- src/fsfw/globalfunctions/DleEncoder.cpp | 93 ++++++++++++------- src/fsfw/globalfunctions/DleEncoder.h | 5 + .../unit/globalfunctions/testDleEncoder.cpp | 2 +- 3 files changed, 65 insertions(+), 35 deletions(-) diff --git a/src/fsfw/globalfunctions/DleEncoder.cpp b/src/fsfw/globalfunctions/DleEncoder.cpp index 0db557b6..35ba84d5 100644 --- a/src/fsfw/globalfunctions/DleEncoder.cpp +++ b/src/fsfw/globalfunctions/DleEncoder.cpp @@ -8,22 +8,12 @@ DleEncoder::~DleEncoder() {} ReturnValue_t DleEncoder::encode(const uint8_t* sourceStream, size_t sourceLen, uint8_t* destStream, size_t maxDestLen, size_t* encodedLen, bool addStxEtx) { - size_t minAllowedLen = 0; - if(escapeStxEtx) { - minAllowedLen = 1; - } - else { - minAllowedLen = 2; - } - if(minAllowedLen > maxDestLen) { - return STREAM_TOO_SHORT; - } if (addStxEtx) { size_t currentIdx = 0; if(not escapeStxEtx) { destStream[currentIdx++] = DLE_CHAR; } - destStream[currentIdx] = STX_CHAR; + } if(escapeStxEtx) { @@ -40,9 +30,15 @@ ReturnValue_t DleEncoder::encode(const uint8_t* sourceStream, ReturnValue_t DleEncoder::encodeStreamEscaped(const uint8_t *sourceStream, size_t sourceLen, uint8_t *destStream, size_t maxDestLen, size_t *encodedLen, bool addStxEtx) { - size_t encodedIndex = 1; + size_t encodedIndex = 0; size_t sourceIndex = 0; uint8_t nextByte = 0; + if(addStxEtx) { + if(maxDestLen < 1) { + return STREAM_TOO_SHORT; + } + destStream[encodedIndex++] = STX_CHAR; + } while (encodedIndex < maxDestLen and sourceIndex < sourceLen) { nextByte = sourceStream[sourceIndex]; // STX, ETX and CR characters in the stream need to be escaped with DLE @@ -82,8 +78,11 @@ ReturnValue_t DleEncoder::encodeStreamEscaped(const uint8_t *sourceStream, size_ ++sourceIndex; } - if (sourceIndex == sourceLen and encodedIndex < maxDestLen) { + if (sourceIndex == sourceLen) { if (addStxEtx) { + if(encodedIndex + 1 >= maxDestLen) { + return STREAM_TOO_SHORT; + } destStream[encodedIndex] = ETX_CHAR; ++encodedIndex; } @@ -98,9 +97,16 @@ ReturnValue_t DleEncoder::encodeStreamEscaped(const uint8_t *sourceStream, size_ ReturnValue_t DleEncoder::encodeStreamNonEscaped(const uint8_t *sourceStream, size_t sourceLen, uint8_t *destStream, size_t maxDestLen, size_t *encodedLen, bool addStxEtx) { - size_t encodedIndex = 2; + size_t encodedIndex = 0; size_t sourceIndex = 0; uint8_t nextByte = 0; + if(addStxEtx) { + if(maxDestLen < 2) { + return STREAM_TOO_SHORT; + } + destStream[encodedIndex++] = DLE_CHAR; + destStream[encodedIndex++] = STX_CHAR; + } while (encodedIndex < maxDestLen and sourceIndex < sourceLen) { nextByte = sourceStream[sourceIndex]; // DLE characters are simply escaped with DLE. @@ -121,7 +127,7 @@ ReturnValue_t DleEncoder::encodeStreamNonEscaped(const uint8_t *sourceStream, si ++sourceIndex; } - if (sourceIndex == sourceLen and encodedIndex < maxDestLen) { + if (sourceIndex == sourceLen) { if (addStxEtx) { if(encodedIndex + 2 >= maxDestLen) { return STREAM_TOO_SHORT; @@ -140,17 +146,6 @@ ReturnValue_t DleEncoder::encodeStreamNonEscaped(const uint8_t *sourceStream, si ReturnValue_t DleEncoder::decode(const uint8_t *sourceStream, size_t sourceStreamLen, size_t *readLen, uint8_t *destStream, size_t maxDestStreamlen, size_t *decodedLen) { - size_t encodedIndex = 0; - if(not escapeStxEtx) { - if (*sourceStream != DLE_CHAR) { - return DECODING_ERROR; - } - ++encodedIndex; - } - if (sourceStream[encodedIndex] != STX_CHAR) { - return DECODING_ERROR; - } - if(escapeStxEtx) { return decodeStreamEscaped(sourceStream, sourceStreamLen, readLen, destStream, maxDestStreamlen, decodedLen); @@ -164,10 +159,15 @@ ReturnValue_t DleEncoder::decode(const uint8_t *sourceStream, ReturnValue_t DleEncoder::decodeStreamEscaped(const uint8_t *sourceStream, size_t sourceStreamLen, size_t *readLen, uint8_t *destStream, size_t maxDestStreamlen, size_t *decodedLen) { - // Skip start marker, was already checked - size_t encodedIndex = 1; + size_t encodedIndex = 0; size_t decodedIndex = 0; uint8_t nextByte; + if(maxDestStreamlen < 1) { + return STREAM_TOO_SHORT; + } + if (sourceStream[encodedIndex++] != STX_CHAR) { + return DECODING_ERROR; + } while ((encodedIndex < sourceStreamLen) and (decodedIndex < maxDestStreamlen) and (sourceStream[encodedIndex] != ETX_CHAR) @@ -192,6 +192,8 @@ ReturnValue_t DleEncoder::decodeStreamEscaped(const uint8_t *sourceStream, size_ destStream[decodedIndex] = nextByte - 0x40; } else { + // Set readLen so user can resume parsing after incorrect data + *readLen = encodedIndex + 2; return DECODING_ERROR; } } @@ -205,8 +207,13 @@ ReturnValue_t DleEncoder::decodeStreamEscaped(const uint8_t *sourceStream, size_ ++decodedIndex; } if (sourceStream[encodedIndex] != ETX_CHAR) { - *readLen = ++encodedIndex; - return DECODING_ERROR; + if(decodedIndex == maxDestStreamlen) { + return STREAM_TOO_SHORT; + } + else { + *readLen = ++encodedIndex; + return DECODING_ERROR; + } } else { *readLen = ++encodedIndex; @@ -218,18 +225,29 @@ ReturnValue_t DleEncoder::decodeStreamEscaped(const uint8_t *sourceStream, size_ ReturnValue_t DleEncoder::decodeStreamNonEscaped(const uint8_t *sourceStream, size_t sourceStreamLen, size_t *readLen, uint8_t *destStream, size_t maxDestStreamlen, size_t *decodedLen) { - // Skip start marker, was already checked - size_t encodedIndex = 2; + size_t encodedIndex = 0; size_t decodedIndex = 0; uint8_t nextByte; + if(maxDestStreamlen < 2) { + return STREAM_TOO_SHORT; + } + if (sourceStream[encodedIndex++] != DLE_CHAR) { + return DECODING_ERROR; + } + if (sourceStream[encodedIndex++] != STX_CHAR) { + return DECODING_ERROR; + } while ((encodedIndex < sourceStreamLen) && (decodedIndex < maxDestStreamlen)) { if (sourceStream[encodedIndex] == DLE_CHAR) { if(encodedIndex + 1 >= sourceStreamLen) { + *readLen = encodedIndex; return DECODING_ERROR; } nextByte = sourceStream[encodedIndex + 1]; if(nextByte == STX_CHAR) { - *readLen = ++encodedIndex; + // Set readLen so the DLE/STX char combination is preserved. Could be start of + // another frame + *readLen = encodedIndex; return DECODING_ERROR; } else if(nextByte == DLE_CHAR) { @@ -245,6 +263,7 @@ ReturnValue_t DleEncoder::decodeStreamNonEscaped(const uint8_t *sourceStream, return RETURN_OK; } else { + *readLen = encodedIndex; return DECODING_ERROR; } } @@ -254,7 +273,13 @@ ReturnValue_t DleEncoder::decodeStreamNonEscaped(const uint8_t *sourceStream, ++encodedIndex; ++decodedIndex; } - return DECODING_ERROR; + *readLen = encodedIndex; + if(decodedIndex == maxDestStreamlen) { + return STREAM_TOO_SHORT; + } + else { + return DECODING_ERROR; + } } void DleEncoder::setEscapeMode(bool escapeStxEtx) { diff --git a/src/fsfw/globalfunctions/DleEncoder.h b/src/fsfw/globalfunctions/DleEncoder.h index e4871bf0..09fa2726 100644 --- a/src/fsfw/globalfunctions/DleEncoder.h +++ b/src/fsfw/globalfunctions/DleEncoder.h @@ -71,6 +71,8 @@ public: * @param addStxEtx Adding STX start marker and ETX end marker can be omitted, * if they are added manually * @return + * - RETURN_OK for successful encoding operation + * - STREAM_TOO_SHORT if the destination stream is too short */ ReturnValue_t encode(const uint8_t *sourceStream, size_t sourceLen, uint8_t *destStream, size_t maxDestLen, size_t *encodedLen, @@ -85,6 +87,9 @@ public: * @param maxDestStreamlen * @param decodedLen * @return + * - RETURN_OK for successful decode operation + * - DECODE_ERROR if the source stream is invalid + * - STREAM_TOO_SHORT if the destination stream is too short */ ReturnValue_t decode(const uint8_t *sourceStream, size_t sourceStreamLen, size_t *readLen, uint8_t *destStream, diff --git a/tests/src/fsfw_tests/unit/globalfunctions/testDleEncoder.cpp b/tests/src/fsfw_tests/unit/globalfunctions/testDleEncoder.cpp index 4cc3326e..a82ac73a 100644 --- a/tests/src/fsfw_tests/unit/globalfunctions/testDleEncoder.cpp +++ b/tests/src/fsfw_tests/unit/globalfunctions/testDleEncoder.cpp @@ -165,7 +165,7 @@ TEST_CASE("DleEncoder" , "[DleEncoder]") { result = dleEncoder.decode(vecToDecode.data(), vecToDecode.size(), &readLen, buffer.data(), faultyDestSizes, &decodedLen); - REQUIRE(result == static_cast(DleEncoder::DECODING_ERROR)); + REQUIRE(result == static_cast(DleEncoder::STREAM_TOO_SHORT)); } }; From d36d849e6994356c045e17924a60a00b13ae75e3 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 11 Sep 2021 19:21:21 +0200 Subject: [PATCH 2/3] removed part which is now not necessary anymore --- src/fsfw/globalfunctions/DleEncoder.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/fsfw/globalfunctions/DleEncoder.cpp b/src/fsfw/globalfunctions/DleEncoder.cpp index 35ba84d5..04e4985c 100644 --- a/src/fsfw/globalfunctions/DleEncoder.cpp +++ b/src/fsfw/globalfunctions/DleEncoder.cpp @@ -8,14 +8,6 @@ DleEncoder::~DleEncoder() {} ReturnValue_t DleEncoder::encode(const uint8_t* sourceStream, size_t sourceLen, uint8_t* destStream, size_t maxDestLen, size_t* encodedLen, bool addStxEtx) { - if (addStxEtx) { - size_t currentIdx = 0; - if(not escapeStxEtx) { - destStream[currentIdx++] = DLE_CHAR; - } - - } - if(escapeStxEtx) { return encodeStreamEscaped(sourceStream, sourceLen, destStream, maxDestLen, encodedLen, addStxEtx); From dae27a8e105a5d0e9c0f8bd5bda6b6d2826441b3 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 11 Sep 2021 19:22:51 +0200 Subject: [PATCH 3/3] indentation --- src/fsfw/globalfunctions/DleEncoder.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/fsfw/globalfunctions/DleEncoder.cpp b/src/fsfw/globalfunctions/DleEncoder.cpp index 04e4985c..df9a909e 100644 --- a/src/fsfw/globalfunctions/DleEncoder.cpp +++ b/src/fsfw/globalfunctions/DleEncoder.cpp @@ -1,13 +1,13 @@ #include "fsfw/globalfunctions/DleEncoder.h" -DleEncoder::DleEncoder(bool escapeStxEtx, bool escapeCr): escapeStxEtx(escapeStxEtx), - escapeCr(escapeCr) {} +DleEncoder::DleEncoder(bool escapeStxEtx, bool escapeCr): + escapeStxEtx(escapeStxEtx), escapeCr(escapeCr) {} DleEncoder::~DleEncoder() {} ReturnValue_t DleEncoder::encode(const uint8_t* sourceStream, - size_t sourceLen, uint8_t* destStream, size_t maxDestLen, - size_t* encodedLen, bool addStxEtx) { + size_t sourceLen, uint8_t* destStream, size_t maxDestLen, + size_t* encodedLen, bool addStxEtx) { if(escapeStxEtx) { return encodeStreamEscaped(sourceStream, sourceLen, destStream, maxDestLen, encodedLen, addStxEtx); @@ -136,16 +136,16 @@ ReturnValue_t DleEncoder::encodeStreamNonEscaped(const uint8_t *sourceStream, si } ReturnValue_t DleEncoder::decode(const uint8_t *sourceStream, - size_t sourceStreamLen, size_t *readLen, uint8_t *destStream, - size_t maxDestStreamlen, size_t *decodedLen) { - if(escapeStxEtx) { - return decodeStreamEscaped(sourceStream, sourceStreamLen, - readLen, destStream, maxDestStreamlen, decodedLen); - } - else { + size_t sourceStreamLen, size_t *readLen, uint8_t *destStream, + size_t maxDestStreamlen, size_t *decodedLen) { + if(escapeStxEtx) { + return decodeStreamEscaped(sourceStream, sourceStreamLen, + readLen, destStream, maxDestStreamlen, decodedLen); + } + else { return decodeStreamNonEscaped(sourceStream, sourceStreamLen, readLen, destStream, maxDestStreamlen, decodedLen); - } + } } ReturnValue_t DleEncoder::decodeStreamEscaped(const uint8_t *sourceStream, size_t sourceStreamLen,