From 28ecd0e5c6304a91bae9840159d200ed8361e68a Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 1 Sep 2022 08:51:12 +0200 Subject: [PATCH 1/4] bugfix for SP parser --- src/fsfw/osal/common/TcpTmTcServer.cpp | 19 ++++----- src/fsfw/tmtcservices/SpacePacketParser.cpp | 45 +++++++++++---------- src/fsfw/tmtcservices/SpacePacketParser.h | 18 +++++++-- 3 files changed, 45 insertions(+), 37 deletions(-) diff --git a/src/fsfw/osal/common/TcpTmTcServer.cpp b/src/fsfw/osal/common/TcpTmTcServer.cpp index f9cb923e..f1174734 100644 --- a/src/fsfw/osal/common/TcpTmTcServer.cpp +++ b/src/fsfw/osal/common/TcpTmTcServer.cpp @@ -161,7 +161,7 @@ void TcpTmTcServer::handleServerOperation(socket_t& connSocket) { while (true) { ssize_t retval = recv(connSocket, reinterpret_cast(receptionBuffer.data()), - receptionBuffer.capacity(), tcpConfig.tcpFlags); + receptionBuffer.size(), tcpConfig.tcpFlags); if (retval == 0) { size_t availableReadData = ringBuffer.getAvailableReadData(); if (availableReadData > lastRingBufferSize) { @@ -335,31 +335,28 @@ ReturnValue_t TcpTmTcServer::handleTcRingBufferData(size_t availableReadData) { } ringBuffer.readData(receptionBuffer.data(), readAmount, true); const uint8_t* bufPtr = receptionBuffer.data(); - const uint8_t** bufPtrPtr = &bufPtr; - size_t startIdx = 0; - size_t foundSize = 0; - size_t readLen = 0; - while (readLen < readAmount) { + FoundPacketInfo info; + ParsingState parseState; + while (parseState.amountRead < readAmount) { if (spacePacketParser == nullptr) { return returnvalue::FAILED; } - result = - spacePacketParser->parseSpacePackets(bufPtrPtr, readAmount, startIdx, foundSize, readLen); + result = spacePacketParser->parseSpacePackets(&bufPtr, readAmount, info, parseState); switch (result) { case (SpacePacketParser::NO_PACKET_FOUND): case (SpacePacketParser::SPLIT_PACKET): { break; } case (returnvalue::OK): { - result = handleTcReception(receptionBuffer.data() + startIdx, foundSize); + result = handleTcReception(receptionBuffer.data() + info.startIdx, info.sizeFound); if (result != returnvalue::OK) { status = result; } } } - ringBuffer.deleteData(foundSize); + ringBuffer.deleteData(info.sizeFound); lastRingBufferSize = ringBuffer.getAvailableReadData(); - std::memset(receptionBuffer.data() + startIdx, 0, foundSize); + // std::memset(receptionBuffer.data() + startIdx, 0, foundSize); } return status; } diff --git a/src/fsfw/tmtcservices/SpacePacketParser.cpp b/src/fsfw/tmtcservices/SpacePacketParser.cpp index b8364138..16ee5dac 100644 --- a/src/fsfw/tmtcservices/SpacePacketParser.cpp +++ b/src/fsfw/tmtcservices/SpacePacketParser.cpp @@ -7,16 +7,16 @@ SpacePacketParser::SpacePacketParser(std::vector validPacketIds) : validPacketIds(validPacketIds) {} ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t* buffer, const size_t maxSize, - size_t& startIndex, size_t& foundSize) { + FoundPacketInfo& packetInfo, + ParsingState& parsingState) { const uint8_t** tempPtr = &buffer; - size_t readLen = 0; - return parseSpacePackets(tempPtr, maxSize, startIndex, foundSize, readLen); + return parseSpacePackets(tempPtr, maxSize, packetInfo, parsingState); } ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t** buffer, const size_t maxSize, - size_t& startIndex, size_t& foundSize, - size_t& readLen) { - if (buffer == nullptr or maxSize < 5) { + FoundPacketInfo& packetInfo, + ParsingState& parsingState) { + if (buffer == nullptr or parsingState.nextStartIdx > maxSize) { #if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "SpacePacketParser::parseSpacePackets: Frame invalid" << std::endl; #else @@ -26,35 +26,36 @@ ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t** buffer, const } const uint8_t* bufPtr = *buffer; - auto verifyLengthField = [&](size_t idx) { - uint16_t lengthField = bufPtr[idx + 4] << 8 | bufPtr[idx + 5]; + auto verifyLengthField = [&](size_t localIdx) { + uint16_t lengthField = (bufPtr[localIdx + 4] << 8) | bufPtr[localIdx + 5]; size_t packetSize = lengthField + 7; - startIndex = idx; ReturnValue_t result = returnvalue::OK; if (lengthField == 0) { // Skip whole header for now - foundSize = 6; + packetInfo.sizeFound = 6; result = NO_PACKET_FOUND; - } else if (packetSize + idx > maxSize) { + } else if (packetSize + localIdx + parsingState.amountRead > maxSize) { // Don't increment buffer and read length here, user has to decide what to do - foundSize = packetSize; + packetInfo.sizeFound = packetSize; return SPLIT_PACKET; } else { - foundSize = packetSize; + packetInfo.sizeFound = packetSize; } - *buffer += foundSize; - readLen += idx + foundSize; + *buffer += packetInfo.sizeFound; + packetInfo.startIdx = localIdx + parsingState.amountRead; + parsingState.nextStartIdx = localIdx + parsingState.amountRead + packetInfo.sizeFound; + parsingState.amountRead = parsingState.nextStartIdx; return result; }; size_t idx = 0; // Space packet ID as start marker if (validPacketIds.size() > 0) { - while (idx < maxSize - 5) { - uint16_t currentPacketId = bufPtr[idx] << 8 | bufPtr[idx + 1]; + while (idx + parsingState.amountRead < maxSize - 5) { + uint16_t currentPacketId = (bufPtr[idx] << 8) | bufPtr[idx + 1]; if (std::find(validPacketIds.begin(), validPacketIds.end(), currentPacketId) != validPacketIds.end()) { - if (idx + 5 >= maxSize) { + if (idx + parsingState.amountRead >= maxSize - 5) { return SPLIT_PACKET; } return verifyLengthField(idx); @@ -62,10 +63,10 @@ ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t** buffer, const idx++; } } - startIndex = 0; - foundSize = maxSize; - *buffer += foundSize; - readLen += foundSize; + parsingState.nextStartIdx = maxSize; + packetInfo.sizeFound = maxSize; + parsingState.amountRead = maxSize; + *buffer += maxSize; return NO_PACKET_FOUND; } // Assume that the user verified a valid start of a space packet diff --git a/src/fsfw/tmtcservices/SpacePacketParser.h b/src/fsfw/tmtcservices/SpacePacketParser.h index 93f90afe..394083b2 100644 --- a/src/fsfw/tmtcservices/SpacePacketParser.h +++ b/src/fsfw/tmtcservices/SpacePacketParser.h @@ -7,6 +7,16 @@ #include "fsfw/container/DynamicFIFO.h" #include "fsfw/returnvalues/FwClassIds.h" +struct FoundPacketInfo { + size_t startIdx = 0; + size_t sizeFound = 0; +}; + +struct ParsingState { + size_t nextStartIdx = 0; + size_t amountRead = 0; +}; + /** * @brief This small helper class scans a given buffer for space packets. * Can be used if space packets are serialized in a tightly packed frame. @@ -53,8 +63,8 @@ class SpacePacketParser { * will be assigned. * -@c returnvalue::OK if a packet was found */ - ReturnValue_t parseSpacePackets(const uint8_t** buffer, const size_t maxSize, size_t& startIndex, - size_t& foundSize, size_t& readLen); + ReturnValue_t parseSpacePackets(const uint8_t** buffer, const size_t maxSize, + FoundPacketInfo& packetInfo, ParsingState& parsingState); /** * Parse a given frame for space packets @@ -69,8 +79,8 @@ class SpacePacketParser { * detected packet * -@c returnvalue::OK if a packet was found */ - ReturnValue_t parseSpacePackets(const uint8_t* buffer, const size_t maxSize, size_t& startIndex, - size_t& foundSize); + ReturnValue_t parseSpacePackets(const uint8_t* buffer, const size_t maxSize, + FoundPacketInfo& packetInfo, ParsingState& parsingState); private: std::vector validPacketIds; From 01651f0521d851402426fd6dca4b449e103b3d4d Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 1 Sep 2022 10:51:09 +0200 Subject: [PATCH 2/4] more simplfications --- src/fsfw/osal/common/TcpTmTcServer.cpp | 15 +++-- src/fsfw/tmtcservices/SpacePacketParser.cpp | 32 ++++------ src/fsfw/tmtcservices/SpacePacketParser.h | 66 +++++++-------------- 3 files changed, 40 insertions(+), 73 deletions(-) diff --git a/src/fsfw/osal/common/TcpTmTcServer.cpp b/src/fsfw/osal/common/TcpTmTcServer.cpp index f1174734..dff959ba 100644 --- a/src/fsfw/osal/common/TcpTmTcServer.cpp +++ b/src/fsfw/osal/common/TcpTmTcServer.cpp @@ -335,13 +335,13 @@ ReturnValue_t TcpTmTcServer::handleTcRingBufferData(size_t availableReadData) { } ringBuffer.readData(receptionBuffer.data(), readAmount, true); const uint8_t* bufPtr = receptionBuffer.data(); - FoundPacketInfo info; - ParsingState parseState; - while (parseState.amountRead < readAmount) { - if (spacePacketParser == nullptr) { - return returnvalue::FAILED; - } - result = spacePacketParser->parseSpacePackets(&bufPtr, readAmount, info, parseState); + SpacePacketParser::FoundPacketInfo info; + if (spacePacketParser == nullptr) { + return returnvalue::FAILED; + } + spacePacketParser->reset(); + while (spacePacketParser->getAmountRead() < readAmount) { + result = spacePacketParser->parseSpacePackets(&bufPtr, readAmount, info); switch (result) { case (SpacePacketParser::NO_PACKET_FOUND): case (SpacePacketParser::SPLIT_PACKET): { @@ -356,7 +356,6 @@ ReturnValue_t TcpTmTcServer::handleTcRingBufferData(size_t availableReadData) { } ringBuffer.deleteData(info.sizeFound); lastRingBufferSize = ringBuffer.getAvailableReadData(); - // std::memset(receptionBuffer.data() + startIdx, 0, foundSize); } return status; } diff --git a/src/fsfw/tmtcservices/SpacePacketParser.cpp b/src/fsfw/tmtcservices/SpacePacketParser.cpp index 16ee5dac..75db7a4e 100644 --- a/src/fsfw/tmtcservices/SpacePacketParser.cpp +++ b/src/fsfw/tmtcservices/SpacePacketParser.cpp @@ -6,17 +6,9 @@ SpacePacketParser::SpacePacketParser(std::vector validPacketIds) : validPacketIds(validPacketIds) {} -ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t* buffer, const size_t maxSize, - FoundPacketInfo& packetInfo, - ParsingState& parsingState) { - const uint8_t** tempPtr = &buffer; - return parseSpacePackets(tempPtr, maxSize, packetInfo, parsingState); -} - ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t** buffer, const size_t maxSize, - FoundPacketInfo& packetInfo, - ParsingState& parsingState) { - if (buffer == nullptr or parsingState.nextStartIdx > maxSize) { + FoundPacketInfo& packetInfo) { + if (buffer == nullptr or nextStartIdx > maxSize) { #if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "SpacePacketParser::parseSpacePackets: Frame invalid" << std::endl; #else @@ -30,11 +22,7 @@ ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t** buffer, const uint16_t lengthField = (bufPtr[localIdx + 4] << 8) | bufPtr[localIdx + 5]; size_t packetSize = lengthField + 7; ReturnValue_t result = returnvalue::OK; - if (lengthField == 0) { - // Skip whole header for now - packetInfo.sizeFound = 6; - result = NO_PACKET_FOUND; - } else if (packetSize + localIdx + parsingState.amountRead > maxSize) { + if (packetSize + localIdx + amountRead > maxSize) { // Don't increment buffer and read length here, user has to decide what to do packetInfo.sizeFound = packetSize; return SPLIT_PACKET; @@ -42,20 +30,20 @@ ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t** buffer, const packetInfo.sizeFound = packetSize; } *buffer += packetInfo.sizeFound; - packetInfo.startIdx = localIdx + parsingState.amountRead; - parsingState.nextStartIdx = localIdx + parsingState.amountRead + packetInfo.sizeFound; - parsingState.amountRead = parsingState.nextStartIdx; + packetInfo.startIdx = localIdx + amountRead; + nextStartIdx = localIdx + amountRead + packetInfo.sizeFound; + amountRead = nextStartIdx; return result; }; size_t idx = 0; // Space packet ID as start marker if (validPacketIds.size() > 0) { - while (idx + parsingState.amountRead < maxSize - 5) { + while (idx + amountRead < maxSize - 5) { uint16_t currentPacketId = (bufPtr[idx] << 8) | bufPtr[idx + 1]; if (std::find(validPacketIds.begin(), validPacketIds.end(), currentPacketId) != validPacketIds.end()) { - if (idx + parsingState.amountRead >= maxSize - 5) { + if (idx + amountRead >= maxSize - 5) { return SPLIT_PACKET; } return verifyLengthField(idx); @@ -63,9 +51,9 @@ ReturnValue_t SpacePacketParser::parseSpacePackets(const uint8_t** buffer, const idx++; } } - parsingState.nextStartIdx = maxSize; + nextStartIdx = maxSize; packetInfo.sizeFound = maxSize; - parsingState.amountRead = maxSize; + amountRead = maxSize; *buffer += maxSize; return NO_PACKET_FOUND; } diff --git a/src/fsfw/tmtcservices/SpacePacketParser.h b/src/fsfw/tmtcservices/SpacePacketParser.h index 394083b2..3a1b4d16 100644 --- a/src/fsfw/tmtcservices/SpacePacketParser.h +++ b/src/fsfw/tmtcservices/SpacePacketParser.h @@ -7,16 +7,6 @@ #include "fsfw/container/DynamicFIFO.h" #include "fsfw/returnvalues/FwClassIds.h" -struct FoundPacketInfo { - size_t startIdx = 0; - size_t sizeFound = 0; -}; - -struct ParsingState { - size_t nextStartIdx = 0; - size_t amountRead = 0; -}; - /** * @brief This small helper class scans a given buffer for space packets. * Can be used if space packets are serialized in a tightly packed frame. @@ -27,9 +17,11 @@ struct ParsingState { */ class SpacePacketParser { public: - //! The first entry is the index inside the buffer while the second index - //! is the size of the PUS packet starting at that index. - using IndexSizePair = std::pair; + + struct FoundPacketInfo { + size_t startIdx = 0; + size_t sizeFound = 0; + }; static constexpr uint8_t INTERFACE_ID = CLASS_ID::SPACE_PACKET_PARSER; static constexpr ReturnValue_t NO_PACKET_FOUND = MAKE_RETURN_CODE(0x00); @@ -46,44 +38,32 @@ class SpacePacketParser { SpacePacketParser(std::vector validPacketIds); /** - * Parse a given frame for space packets but also increment the given buffer and assign the - * total number of bytes read so far + * Parse a given frame for space packets but also increments the given buffer. * @param buffer Parser will look for space packets in this buffer * @param maxSize Maximum size of the buffer - * @param startIndex Start index of a found space packet - * @param foundSize Found size of the space packet - * @param readLen Length read so far. This value is incremented by the number of parsed - * bytes which also includes the size of a found packet - * -@c NO_PACKET_FOUND if no packet was found in the given buffer or the length field is - * invalid. foundSize will be set to the size of the space packet header. buffer and - * readLen will be incremented accordingly. - * -@c SPLIT_PACKET if a packet was found but the detected size exceeds maxSize. foundSize - * will be set to the detected packet size and startIndex will be set to the start of the - * detected packet. buffer and read length will not be incremented but the found length - * will be assigned. - * -@c returnvalue::OK if a packet was found + * @param packetInfo Information about packets found. + * -@c NO_PACKET_FOUND if no packet was found in the given buffer + * -@c SPLIT_PACKET if a packet was found but the detected size exceeds maxSize. packetInfo + * will contain the detected packet size and start index. + * -@c returnvalue::OK if a packet was found. Packet size and start index will be set in + * packetInfo */ ReturnValue_t parseSpacePackets(const uint8_t** buffer, const size_t maxSize, - FoundPacketInfo& packetInfo, ParsingState& parsingState); + FoundPacketInfo& packetInfo); - /** - * Parse a given frame for space packets - * @param buffer Parser will look for space packets in this buffer - * @param maxSize Maximum size of the buffer - * @param startIndex Start index of a found space packet - * @param foundSize Found size of the space packet - * -@c NO_PACKET_FOUND if no packet was found in the given buffer or the length field is - * invalid. foundSize will be set to the size of the space packet header - * -@c SPLIT_PACKET if a packet was found but the detected size exceeds maxSize. foundSize - * will be set to the detected packet size and startIndex will be set to the start of the - * detected packet - * -@c returnvalue::OK if a packet was found - */ - ReturnValue_t parseSpacePackets(const uint8_t* buffer, const size_t maxSize, - FoundPacketInfo& packetInfo, ParsingState& parsingState); + size_t getAmountRead() { + return amountRead; + } + + void reset() { + nextStartIdx = 0; + amountRead = 0; + } private: std::vector validPacketIds; + size_t nextStartIdx = 0; + size_t amountRead = 0; }; #endif /* FRAMEWORK_TMTCSERVICES_PUSPARSER_H_ */ From ebc02673ddc269f1cfdb98e061dc72c33783d3a6 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 2 Sep 2022 08:50:39 +0200 Subject: [PATCH 3/4] provide a weak print char impl --- src/fsfw_hal/common/CMakeLists.txt | 2 ++ src/fsfw_hal/common/printChar.c | 10 ++++++++++ 2 files changed, 12 insertions(+) create mode 100644 src/fsfw_hal/common/printChar.c diff --git a/src/fsfw_hal/common/CMakeLists.txt b/src/fsfw_hal/common/CMakeLists.txt index f1cfec52..1cd9c678 100644 --- a/src/fsfw_hal/common/CMakeLists.txt +++ b/src/fsfw_hal/common/CMakeLists.txt @@ -1 +1,3 @@ add_subdirectory(gpio) + +target_sources(${LIB_FSFW_NAME} PRIVATE printChar.c) \ No newline at end of file diff --git a/src/fsfw_hal/common/printChar.c b/src/fsfw_hal/common/printChar.c new file mode 100644 index 00000000..6e02c1df --- /dev/null +++ b/src/fsfw_hal/common/printChar.c @@ -0,0 +1,10 @@ +#include +#include + +void __attribute__((weak)) printChar(const char* character, bool errStream) { + if (errStream) { + fprintf(stderr, "%c", *character); + } else { + printf("%c", *character); + } +} From 80355910ee6260496fdfc196230c603c0d654582 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 2 Sep 2022 09:05:10 +0200 Subject: [PATCH 4/4] better warning --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5351aeb4..7b00a708 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -360,7 +360,7 @@ if(NOT FSFW_CONFIG_PATH) if(NOT FSFW_BUILD_DOCS) message( WARNING - "${MSG_PREFIX} Flight Software Framework configuration path not set") + "${MSG_PREFIX} Flight Software Framework configuration path FSFW_CONFIG_PATH not set") message( WARNING "${MSG_PREFIX} Setting default configuration from ${DEF_CONF_PATH} ..")