From 496dac89e4c094e03578f40d8651485f7f378cb2 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 31 Aug 2022 22:47:58 +0200 Subject: [PATCH] important bugfix for TCP TMTC server --- src/fsfw/osal/common/TcpTmTcServer.cpp | 19 ++++---- .../pus/Service11TelecommandScheduling.tpp | 2 +- src/fsfw/tcdistribution/PusDistributor.cpp | 4 +- src/fsfw/tmtcservices/SpacePacketParser.cpp | 45 ++++++++++--------- src/fsfw/tmtcservices/SpacePacketParser.h | 18 ++++++-- src/fsfw/util/dataWrapper.h | 11 ++--- unittests/util/testDataWrapper.cpp | 12 ++--- 7 files changed, 56 insertions(+), 55 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/pus/Service11TelecommandScheduling.tpp b/src/fsfw/pus/Service11TelecommandScheduling.tpp index 315d8950..9a3cea57 100644 --- a/src/fsfw/pus/Service11TelecommandScheduling.tpp +++ b/src/fsfw/pus/Service11TelecommandScheduling.tpp @@ -564,7 +564,7 @@ inline ReturnValue_t Service11TelecommandScheduling::getMapFilterFr if (result != returnvalue::OK) { return result; } - if(fromTimestamp > toTimestamp) { + if (fromTimestamp > toTimestamp) { return INVALID_TIME_WINDOW; } itBegin = telecommandMap.begin(); diff --git a/src/fsfw/tcdistribution/PusDistributor.cpp b/src/fsfw/tcdistribution/PusDistributor.cpp index c94d5736..37e09d56 100644 --- a/src/fsfw/tcdistribution/PusDistributor.cpp +++ b/src/fsfw/tcdistribution/PusDistributor.cpp @@ -146,7 +146,7 @@ ReturnValue_t PusDistributor::initialize() { void PusDistributor::checkerFailurePrinter() const { #if FSFW_VERBOSE_LEVEL >= 1 - const char* keyword = "unnamed error"; + const char* keyword = "unnamed"; if (tcStatus == tcdistrib::INCORRECT_CHECKSUM) { keyword = "checksum"; } else if (tcStatus == tcdistrib::INCORRECT_PRIMARY_HEADER) { @@ -157,6 +157,8 @@ void PusDistributor::checkerFailurePrinter() const { keyword = "incorrect secondary header"; } else if (tcStatus == tcdistrib::INCOMPLETE_PACKET) { keyword = "incomplete packet"; + } else if (tcStatus == tcdistrib::INVALID_SEC_HEADER_FIELD) { + keyword = "invalid secondary header field"; } #if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "PUSDistributor::handlePacket: Packet format invalid, " << keyword << " error" 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; diff --git a/src/fsfw/util/dataWrapper.h b/src/fsfw/util/dataWrapper.h index 84a9814f..f2620556 100644 --- a/src/fsfw/util/dataWrapper.h +++ b/src/fsfw/util/dataWrapper.h @@ -25,18 +25,13 @@ union DataUnion { }; struct DataWrapper { - DataWrapper() = default; - DataWrapper(const uint8_t* data, size_t size): type(DataTypes::RAW) { - setRawData({data, size}); - } + DataWrapper(const uint8_t* data, size_t size) : type(DataTypes::RAW) { setRawData({data, size}); } - explicit DataWrapper(BufPair raw): type(DataTypes::RAW) { - setRawData(raw); - } + explicit DataWrapper(BufPair raw) : type(DataTypes::RAW) { setRawData(raw); } - explicit DataWrapper(SerializeIF& serializable): type(DataTypes::SERIALIZABLE) { + explicit DataWrapper(SerializeIF& serializable) : type(DataTypes::SERIALIZABLE) { setSerializable(serializable); } diff --git a/unittests/util/testDataWrapper.cpp b/unittests/util/testDataWrapper.cpp index 72931d90..ef381a62 100644 --- a/unittests/util/testDataWrapper.cpp +++ b/unittests/util/testDataWrapper.cpp @@ -16,9 +16,7 @@ TEST_CASE("Data Wrapper", "[util]") { bool deleteInst = false; REQUIRE(wrapper.isNull()); std::array data = {1, 2, 3, 4}; - SECTION("Setter") { - wrapper.setRawData({data.data(), data.size()}); - } + SECTION("Setter") { wrapper.setRawData({data.data(), data.size()}); } SECTION("Direct Construction Pair") { instance = new util::DataWrapper(util::BufPair(data.data(), data.size())); deleteInst = true; @@ -31,7 +29,7 @@ TEST_CASE("Data Wrapper", "[util]") { REQUIRE(instance->type == util::DataTypes::RAW); REQUIRE(instance->dataUnion.raw.data == data.data()); REQUIRE(instance->dataUnion.raw.len == data.size()); - if(deleteInst) { + if (deleteInst) { delete instance; } } @@ -41,9 +39,7 @@ TEST_CASE("Data Wrapper", "[util]") { bool deleteInst = false; REQUIRE(instance->isNull()); SimpleSerializable serializable; - SECTION("Setter") { - wrapper.setSerializable(serializable); - } + SECTION("Setter") { wrapper.setSerializable(serializable); } SECTION("Direct Construction") { instance = new util::DataWrapper(serializable); deleteInst = true; @@ -52,7 +48,7 @@ TEST_CASE("Data Wrapper", "[util]") { REQUIRE(not instance->isNull()); REQUIRE(instance->type == util::DataTypes::SERIALIZABLE); REQUIRE(instance->dataUnion.serializable == &serializable); - if(deleteInst) { + if (deleteInst) { delete instance; } }