DLE Encoder Improvements #467
No reviewers
Labels
No Label
API Change
Breaking API Change
bug
build
cosmetics
Documentation
duplicate
feature
help wanted
hotfix
invalid
question
Refactor
Tests
wontfix
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: fsfw/fsfw#467
Loading…
Reference in New Issue
No description provided.
Delete Branch "KSat/fsfw:mueller/dle-improvements"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
How to update existing code
encode
anddecode
are member functions now. ADleEncoder
instance needs to be created before calling the member functionsChanges
encode
anddecode
functions are member functions nowDLE STX DATA0 DATA1 DATA2 DLE ETX
)DLE Encoder Improvementsto WIP: DLE Encoder ImprovementsHi,
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.
Regards, Leon
I still need to add unit tests. I might add that length check while implementing those
WIP: DLE Encoder Improvementsto DLE Encoder ImprovementsThe 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
Test on SOURCE done, everything is working fine.
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;
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
@ -15,1 +18,4 @@
if(minAllowedLen > maxDestLen) {
return STREAM_TOO_SHORT;
}
if (addStxEtx) {
Also, not a big fan of separating writing the start markers here instead of the individual function.
@ -17,1 +22,3 @@
++encodedIndex;
size_t currentIdx = 0;
if(not escapeStxEtx) {
destStream[currentIdx++] = DLE_CHAR;
That is, check here for min length of 2
@ -18,0 +23,4 @@
if(not escapeStxEtx) {
destStream[currentIdx++] = DLE_CHAR;
}
destStream[currentIdx] = STX_CHAR;
and here for minlngth of 1
@ -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;
If addStxEtx is false, this must be 0
@ -70,0 +82,4 @@
++sourceIndex;
}
if (sourceIndex == sourceLen and encodedIndex < maxDestLen) {
bug in the original implementation: If addStxEtx is false, encodedIndex==maxDestLen is valid.
@ -70,0 +121,4 @@
++sourceIndex;
}
if (sourceIndex == sourceLen and encodedIndex < maxDestLen) {
same as above, I think encoded IndexCheck can be ommited here as there is one before writing to dest stream below
@ -76,3 +143,1 @@
uint8_t nextByte;
if (*sourceStream != STX_CHAR) {
return DECODING_ERROR;
size_t encodedIndex = 0;
as with encode, I think it is cleaner if all the handling, including the start marker, is in the individual decode functions
@ -125,0 +174,4 @@
and (sourceStream[encodedIndex] != STX_CHAR)) {
if (sourceStream[encodedIndex] == DLE_CHAR) {
if(encodedIndex + 1 >= sourceStreamLen) {
return DECODING_ERROR;
Enhancement over the original implementation: readlen should be set here (to sourceStreamLen), so that the user can resume parsing after the incorrect data.
@ -125,0 +192,4 @@
destStream[decodedIndex] = nextByte - 0x40;
}
else {
return DECODING_ERROR;
readlen should be set here (to encodedIndex + 2), so that the user can resume parsing after the incorrect data.
@ -125,0 +229,4 @@
}
nextByte = sourceStream[encodedIndex + 1];
if(nextByte == STX_CHAR) {
*readLen = ++encodedIndex;
I think it would be better to return *readlen = encodedIndex, so that the presumably valid DLE STX combination is preserved.
@ -125,0 +245,4 @@
return RETURN_OK;
}
else {
return DECODING_ERROR;
readlen should be set here (to encodedIndex + 2), so that the user can resume parsing after the incorrect data.
@ -125,0 +254,4 @@
++encodedIndex;
++decodedIndex;
}
return DECODING_ERROR;
can the error be narrowed down? Then we could set readLen and maybe also report if the dest stream was too small.
I added one commit with missing readLen access, please verify I did not mess it up.
Otherwise, looking good
One more thing, could you please update the description to include neccessary changes in user code to follow this PR.