DLE Encoder Improvements #467

Merged
muellerr merged 22 commits from KSat/fsfw:mueller/dle-improvements into development 2021-09-13 15:25:02 +02:00
Owner

How to update existing code

  • The encode and decode are member functions now. A DleEncoder instance needs to be created before calling the member functions

Changes

  • The encode and decode functions are member functions now
  • DLE encoder is configured via ctor
  • There is a escaped and non-escaped mode now. This makes it more flexible because there are some DLE implementation which work non-escaped (e.g. DLE STX DATA0 DATA1 DATA2 DLE ETX)
/**
 * This encoder can be used to achieve a basic transport layer when using
 * char based transmission systems. There are two implemented variants:
 *
 * 1. Escaped variant
 *
 * The encoded stream starts with a STX marker and ends with an ETX marker.
 * STX and ETX occurrences in the stream are escaped and internally encoded as well so the
 * receiver side can simply check for STX and ETX markers as frame delimiters. When using a
 * strictly char based reception of packets encoded with DLE,
 * STX can be used to notify a reader that actual data will start to arrive
 * while ETX can be used to notify the reader that the data has ended.
 *
 * 2. Non-escaped variant
 *
 * The encoded stream starts with DLE STX and ends with DLE ETX. All DLE occurrences in the stream
 * are escaped with DLE. If the receiver detects a DLE char, it needs to read the next char
 * to determine whether a start (STX) or end (ETX) of a frame has been detected.
 */
## How to update existing code - The `encode` and `decode` are member functions now. A `DleEncoder` instance needs to be created before calling the member functions ## Changes - The `encode` and `decode` functions are member functions now - DLE encoder is configured via ctor - There is a escaped and non-escaped mode now. This makes it more flexible because there are some DLE implementation which work non-escaped (e.g. `DLE STX DATA0 DATA1 DATA2 DLE ETX`) ```cpp /** * This encoder can be used to achieve a basic transport layer when using * char based transmission systems. There are two implemented variants: * * 1. Escaped variant * * The encoded stream starts with a STX marker and ends with an ETX marker. * STX and ETX occurrences in the stream are escaped and internally encoded as well so the * receiver side can simply check for STX and ETX markers as frame delimiters. When using a * strictly char based reception of packets encoded with DLE, * STX can be used to notify a reader that actual data will start to arrive * while ETX can be used to notify the reader that the data has ended. * * 2. Non-escaped variant * * The encoded stream starts with DLE STX and ends with DLE ETX. All DLE occurrences in the stream * are escaped with DLE. If the receiver detects a DLE char, it needs to read the next char * to determine whether a start (STX) or end (ETX) of a frame has been detected. */ ``` - Fixes #466
muellerr added 2 commits 2021-08-17 15:16:28 +02:00
muellerr added the
feature
label 2021-08-17 15:16:39 +02:00
muellerr added 2 commits 2021-08-17 15:41:07 +02:00
muellerr added 1 commit 2021-08-17 16:00:58 +02:00
muellerr changed title from DLE Encoder Improvements to WIP: DLE Encoder Improvements 2021-08-17 16:02:37 +02:00
muellerr added the
Breaking API Change
label 2021-08-17 16:02:43 +02:00
muellerr added 1 commit 2021-08-17 16:03:10 +02:00
muellerr added 1 commit 2021-08-18 13:18:55 +02:00
muellerr added 1 commit 2021-08-18 13:33:40 +02:00
First-time contributor

Hi,

I believe there is a potential seg fault in decodeStreamEscaped and decodeStreamNonEscaped.
The fault should only show up in invalid messages if DLE_CHAR is not followed by anything, but this could still be problematic with malfunctioning partners.

    while ((encodedIndex < sourceStreamLen) && (decodedIndex < maxDestStreamlen)
            && (sourceStream[encodedIndex] != ETX_CHAR)
            && (sourceStream[encodedIndex] != STX_CHAR)) {
        if (sourceStream[encodedIndex] == DLE_CHAR) {
            nextByte = sourceStream[encodedIndex + 1];  <<< HERE
            // The next byte is a DLE character that was escaped by another
            // DLE character, so we can write it to the destination stream.

Regards, Leon

Hi, I believe there is a potential seg fault in decodeStreamEscaped and decodeStreamNonEscaped. The fault should only show up in invalid messages if DLE_CHAR is not followed by anything, but this could still be problematic with malfunctioning partners. ``` while ((encodedIndex < sourceStreamLen) && (decodedIndex < maxDestStreamlen) && (sourceStream[encodedIndex] != ETX_CHAR) && (sourceStream[encodedIndex] != STX_CHAR)) { if (sourceStream[encodedIndex] == DLE_CHAR) { nextByte = sourceStream[encodedIndex + 1]; <<< HERE // The next byte is a DLE character that was escaped by another // DLE character, so we can write it to the destination stream. ``` Regards, Leon
Author
Owner

I still need to add unit tests. I might add that length check while implementing those

I still need to add unit tests. I might add that length check while implementing those
muellerr added 6 commits 2021-09-09 10:48:38 +02:00
muellerr added 1 commit 2021-09-09 11:12:53 +02:00
muellerr changed title from WIP: DLE Encoder Improvements to DLE Encoder Improvements 2021-09-09 11:13:51 +02:00
Author
Owner

The size checks were implemented and the unit tests were added, thanks @lteichtroeb for the hint. I am doing some integrations tests with the SOURCE project as well

The size checks were implemented and the unit tests were added, thanks @lteichtroeb for the hint. I am doing some integrations tests with the SOURCE project as well
muellerr requested review from mohr 2021-09-09 11:14:35 +02:00
Author
Owner

Test on SOURCE done, everything is working fine.

Test on SOURCE done, everything is working fine.
mohr requested changes 2021-09-10 11:46:57 +02:00
mohr left a comment
Owner

As I do not have the time right now to run the code, this is a "read only" review.
So, please forgive me possible misinterpretations...

As I do not have the time right now to run the code, this is a "read only" review. So, please forgive me possible misinterpretations...
mohr requested changes 2021-09-10 12:22:19 +02:00
mohr left a comment
Owner

As I do not have the time right now to run the code, this is a "read only" review.
So, please forgive me possible misinterpretations...

As I do not have the time right now to run the code, this is a "read only" review. So, please forgive me possible misinterpretations...
@ -12,3 +11,1 @@
}
size_t encodedIndex = 0, sourceIndex = 0;
uint8_t nextByte;
size_t minAllowedLen = 0;
Owner

addStxEtx comes into play here, too. If it is false, 1 or even 0 might be valid. I think STREAM_TOO_SHORT should be caught directely when writing to the dest stream

addStxEtx comes into play here, too. If it is false, 1 or even 0 might be valid. I think STREAM_TOO_SHORT should be caught directely when writing to the dest stream
mohr marked this conversation as resolved
@ -15,1 +18,4 @@
if(minAllowedLen > maxDestLen) {
return STREAM_TOO_SHORT;
}
if (addStxEtx) {
Owner

Also, not a big fan of separating writing the start markers here instead of the individual function.

Also, not a big fan of separating writing the start markers here instead of the individual function.
mohr marked this conversation as resolved
@ -17,1 +22,3 @@
++encodedIndex;
size_t currentIdx = 0;
if(not escapeStxEtx) {
destStream[currentIdx++] = DLE_CHAR;
Owner

That is, check here for min length of 2

That is, check here for min length of 2
mohr marked this conversation as resolved
@ -18,0 +23,4 @@
if(not escapeStxEtx) {
destStream[currentIdx++] = DLE_CHAR;
}
destStream[currentIdx] = STX_CHAR;
Owner

and here for minlngth of 1

and here for minlngth of 1
mohr marked this conversation as resolved
@ -70,0 +40,4 @@
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;
Owner

If addStxEtx is false, this must be 0

If addStxEtx is false, this must be 0
mohr marked this conversation as resolved
@ -70,0 +82,4 @@
++sourceIndex;
}
if (sourceIndex == sourceLen and encodedIndex < maxDestLen) {
Owner

bug in the original implementation: If addStxEtx is false, encodedIndex==maxDestLen is valid.

bug in the original implementation: If addStxEtx is false, encodedIndex==maxDestLen is valid.
mohr marked this conversation as resolved
@ -70,0 +121,4 @@
++sourceIndex;
}
if (sourceIndex == sourceLen and encodedIndex < maxDestLen) {
Owner

same as above, I think encoded IndexCheck can be ommited here as there is one before writing to dest stream below

same as above, I think encoded IndexCheck can be ommited here as there is one before writing to dest stream below
mohr marked this conversation as resolved
@ -76,3 +143,1 @@
uint8_t nextByte;
if (*sourceStream != STX_CHAR) {
return DECODING_ERROR;
size_t encodedIndex = 0;
Owner

as with encode, I think it is cleaner if all the handling, including the start marker, is in the individual decode functions

as with encode, I think it is cleaner if all the handling, including the start marker, is in the individual decode functions
mohr marked this conversation as resolved
@ -125,0 +174,4 @@
and (sourceStream[encodedIndex] != STX_CHAR)) {
if (sourceStream[encodedIndex] == DLE_CHAR) {
if(encodedIndex + 1 >= sourceStreamLen) {
return DECODING_ERROR;
Owner

Enhancement over the original implementation: readlen should be set here (to sourceStreamLen), so that the user can resume parsing after the incorrect data.

Enhancement over the original implementation: readlen should be set here (to sourceStreamLen), so that the user can resume parsing after the incorrect data.
mohr marked this conversation as resolved
@ -125,0 +192,4 @@
destStream[decodedIndex] = nextByte - 0x40;
}
else {
return DECODING_ERROR;
Owner

readlen should be set here (to encodedIndex + 2), so that the user can resume parsing after the incorrect data.

readlen should be set here (to encodedIndex + 2), so that the user can resume parsing after the incorrect data.
mohr marked this conversation as resolved
@ -125,0 +229,4 @@
}
nextByte = sourceStream[encodedIndex + 1];
if(nextByte == STX_CHAR) {
*readLen = ++encodedIndex;
Owner

I think it would be better to return *readlen = encodedIndex, so that the presumably valid DLE STX combination is preserved.

I think it would be better to return \*readlen = encodedIndex, so that the presumably valid DLE STX combination is preserved.
mohr marked this conversation as resolved
@ -125,0 +245,4 @@
return RETURN_OK;
}
else {
return DECODING_ERROR;
Owner

readlen should be set here (to encodedIndex + 2), so that the user can resume parsing after the incorrect data.

readlen should be set here (to encodedIndex + 2), so that the user can resume parsing after the incorrect data.
mohr marked this conversation as resolved
@ -125,0 +254,4 @@
++encodedIndex;
++decodedIndex;
}
return DECODING_ERROR;
Owner

can the error be narrowed down? Then we could set readLen and maybe also report if the dest stream was too small.

can the error be narrowed down? Then we could set readLen and maybe also report if the dest stream was too small.
mohr marked this conversation as resolved
muellerr added 1 commit 2021-09-11 19:18:53 +02:00
muellerr requested review from mohr 2021-09-11 19:20:18 +02:00
muellerr added 1 commit 2021-09-11 19:21:32 +02:00
muellerr added 1 commit 2021-09-11 19:22:58 +02:00
mohr added 1 commit 2021-09-13 10:38:39 +02:00
mohr approved these changes 2021-09-13 10:40:15 +02:00
mohr left a comment
Owner

I added one commit with missing readLen access, please verify I did not mess it up.
Otherwise, looking good

I added one commit with missing readLen access, please verify I did not mess it up. Otherwise, looking good
Owner

One more thing, could you please update the description to include neccessary changes in user code to follow this PR.

One more thing, could you please update the description to include neccessary changes in user code to follow this PR.
muellerr added 1 commit 2021-09-13 14:37:35 +02:00
muellerr added 1 commit 2021-09-13 15:18:25 +02:00
muellerr added 1 commit 2021-09-13 15:24:55 +02:00
muellerr merged commit 23c562bb67 into development 2021-09-13 15:25:02 +02:00
muellerr deleted branch mueller/dle-improvements 2021-09-13 15:25:09 +02:00
gaisser added this to the v2.0.0 milestone 2021-09-27 15:03:30 +02:00
Sign in to join this conversation.
No description provided.