From 314df7a02105b29852b9e1d5f6393a17770526c0 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 23 Nov 2023 16:48:40 +0100 Subject: [PATCH 1/5] bugfix for virt channel: clear invalid state --- mission/com/LiveTmTask.cpp | 9 +++++---- mission/com/TmStoreTaskBase.cpp | 7 ++++--- mission/com/VirtualChannel.cpp | 4 +++- mission/com/VirtualChannelWithQueue.cpp | 2 +- mission/tmtc/DirectTmSinkIF.h | 1 + 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/mission/com/LiveTmTask.cpp b/mission/com/LiveTmTask.cpp index 56fe5a51..c671090c 100644 --- a/mission/com/LiveTmTask.cpp +++ b/mission/com/LiveTmTask.cpp @@ -173,15 +173,16 @@ ReturnValue_t LiveTmTask::handleGenericTmQueue(MessageQueueIF& queue, bool isCfd size_t writtenSize = 0; result = channel.write(data, size, writtenSize); if (result == DirectTmSinkIF::PARTIALLY_WRITTEN) { - result = channel.handleWriteCompletionSynchronously(writtenSize, 200); + result = channel.handleWriteCompletionSynchronously(writtenSize, 400); if (result != returnvalue::OK) { // TODO: Event? Might lead to dangerous spam though.. sif::warning << "LiveTmTask: Synchronous write of last segment failed with code 0x" - << std::setw(4) << std::hex << result << std::dec << std::endl; + << std::setfill('0') << std::setw(4) << std::hex << result << std::dec + << std::endl; } } else if (result != returnvalue::OK) { - sif::error << "LiveTmTask: Channel write failed with code 0x" << std::hex << std::setw(4) - << result << std::dec << std::endl; + sif::error << "LiveTmTask: Channel write failed with code 0x" << std::setfill('0') << std::hex + << std::setw(4) << result << std::dec << std::endl; } } // Try delete in any case, ignore failures (which should not happen), it is more important to diff --git a/mission/com/TmStoreTaskBase.cpp b/mission/com/TmStoreTaskBase.cpp index 215cef08..9e86ba08 100644 --- a/mission/com/TmStoreTaskBase.cpp +++ b/mission/com/TmStoreTaskBase.cpp @@ -141,11 +141,12 @@ ReturnValue_t TmStoreTaskBase::performDump(PersistentTmStoreWithTmQueue& store, size_t writtenSize = 0; result = channel.write(tmReader.getFullData(), dumpedLen, writtenSize); if (result == VirtualChannelIF::PARTIALLY_WRITTEN) { - result = channel.handleWriteCompletionSynchronously(writtenSize, 200); + result = channel.handleWriteCompletionSynchronously(writtenSize, 400); if (result != returnvalue::OK) { // TODO: Event? Might lead to dangerous spam though.. - sif::warning << "PersistentTmStore: Synchronous write of last segment failed with code 0x" - << std::setw(4) << std::hex << result << std::dec << std::endl; + sif::warning << "LiveTmTask: Synchronous write of last segment failed with code 0x" + << std::setfill('0') << std::setw(4) << std::hex << result << std::dec + << std::endl; } } else if (result == DirectTmSinkIF::IS_BUSY) { sif::warning << "PersistentTmStore: Unexpected VC channel busy" << std::endl; diff --git a/mission/com/VirtualChannel.cpp b/mission/com/VirtualChannel.cpp index e531727e..a75959e6 100644 --- a/mission/com/VirtualChannel.cpp +++ b/mission/com/VirtualChannel.cpp @@ -74,5 +74,7 @@ ReturnValue_t VirtualChannel::handleWriteCompletionSynchronously(size_t& written return result; } } - return returnvalue::FAILED; + // Timeout. Cancel the transfer + cancelTransfer(); + return TIMEOUT; } diff --git a/mission/com/VirtualChannelWithQueue.cpp b/mission/com/VirtualChannelWithQueue.cpp index 0d9e4d11..fb3375dc 100644 --- a/mission/com/VirtualChannelWithQueue.cpp +++ b/mission/com/VirtualChannelWithQueue.cpp @@ -41,7 +41,7 @@ ReturnValue_t VirtualChannelWithQueue::handleNextTm(bool performWriteOp) { if (performWriteOp) { result = write(data, size, writtenSize); if (result == PARTIALLY_WRITTEN) { - result = handleWriteCompletionSynchronously(writtenSize, 200); + result = handleWriteCompletionSynchronously(writtenSize, 400); if (result != returnvalue::OK) { // TODO: Event? Might lead to dangerous spam though.. sif::warning diff --git a/mission/tmtc/DirectTmSinkIF.h b/mission/tmtc/DirectTmSinkIF.h index ff8ab6fe..546b293e 100644 --- a/mission/tmtc/DirectTmSinkIF.h +++ b/mission/tmtc/DirectTmSinkIF.h @@ -15,6 +15,7 @@ class DirectTmSinkIF { static constexpr ReturnValue_t IS_BUSY = returnvalue::makeCode(CLASS_ID, 0); static constexpr ReturnValue_t PARTIALLY_WRITTEN = returnvalue::makeCode(CLASS_ID, 1); static constexpr ReturnValue_t NO_WRITE_ACTIVE = returnvalue::makeCode(CLASS_ID, 2); + static constexpr ReturnValue_t TIMEOUT = returnvalue::makeCode(CLASS_ID, 3); /** * @brief Implements the functionality to write to a TM sink directly. From 352043cb51de23b5c598632e48e2fad23b0ea370 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 23 Nov 2023 17:20:55 +0100 Subject: [PATCH 2/5] printout improvements --- linux/ipcore/PapbVcInterface.cpp | 1 + mission/com/LiveTmTask.cpp | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/linux/ipcore/PapbVcInterface.cpp b/linux/ipcore/PapbVcInterface.cpp index c02d0935..a176c3af 100644 --- a/linux/ipcore/PapbVcInterface.cpp +++ b/linux/ipcore/PapbVcInterface.cpp @@ -30,6 +30,7 @@ ReturnValue_t PapbVcInterface::initialize() { ReturnValue_t PapbVcInterface::write(const uint8_t* data, size_t size, size_t& writtenSize) { // There are no packets smaller than 4, this is considered a configuration error. if (size < 4) { + sif::warning << "PapbVcInterface::write: Passed packet smaller than 4 bytes" << std::endl; return returnvalue::FAILED; } // The user must call advance until completion before starting a new packet transfer. diff --git a/mission/com/LiveTmTask.cpp b/mission/com/LiveTmTask.cpp index c671090c..33b9322a 100644 --- a/mission/com/LiveTmTask.cpp +++ b/mission/com/LiveTmTask.cpp @@ -54,8 +54,13 @@ ReturnValue_t LiveTmTask::performOperation(uint8_t opCode) { } } } else if (result != MessageQueueIF::EMPTY) { - sif::warning << "LiveTmTask: TM queue failure, returncode 0x" << std::hex << std::setw(4) - << result << std::dec << std::endl; + const char* contextStr = "Regular TM queue"; + if (isCfdp) { + contextStr = "CFDP TM queue"; + } + sif::warning << "LiveTmTask: " << contextStr << " handling failure, returncode 0x" + << std::setfill('0') << std::hex << std::setw(4) << result << std::dec + << std::endl; } } From ef948af5f33a4aea98ecb6ec973b1f019e1dd2ca Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 23 Nov 2023 17:22:36 +0100 Subject: [PATCH 3/5] small possible fix --- linux/ipcore/PapbVcInterface.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/linux/ipcore/PapbVcInterface.cpp b/linux/ipcore/PapbVcInterface.cpp index a176c3af..e4414363 100644 --- a/linux/ipcore/PapbVcInterface.cpp +++ b/linux/ipcore/PapbVcInterface.cpp @@ -84,6 +84,9 @@ ReturnValue_t PapbVcInterface::advanceWrite(size_t& writtenSize) { writtenSize++; } if (not pollReadyForOctet(MAX_BUSY_POLLS)) { + if (not pollReadyForPacket()) { + return PARTIALLY_WRITTEN; + } abortPacketTransfer(); return returnvalue::FAILED; } From 9e31c065632807e6c3b0eaf3c6add65c467af110 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 23 Nov 2023 17:47:44 +0100 Subject: [PATCH 4/5] changelog update --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74bd77e3..7369c89f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,13 @@ will consitute of a breaking change warranting a new major release: # [unreleased] +## Fixed + +- Increase allowed time for PTME writers to finish partial transfers. A duration of 200 ms was + not sufficient for cases where 3 writers write concurrently. +- Fixed state issue for PTME writer object where the writer was not reset properly after a timeout + of a partial transfer. This bloked the whole VC. + # [v7.3.0] 2023-11-07 ## Changed From 14e545618c8a602e5cf8d23a633cd0809a6ae6a7 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 28 Nov 2023 15:42:36 +0100 Subject: [PATCH 5/5] typo --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7369c89f..19159f5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ will consitute of a breaking change warranting a new major release: - Increase allowed time for PTME writers to finish partial transfers. A duration of 200 ms was not sufficient for cases where 3 writers write concurrently. - Fixed state issue for PTME writer object where the writer was not reset properly after a timeout - of a partial transfer. This bloked the whole VC. + of a partial transfer. This was a major bug blocking the whole VC if it occured. # [v7.3.0] 2023-11-07