From ceba3be8a2eaabffc92fd028c1ed25c65751896b Mon Sep 17 00:00:00 2001 From: David J Kessler Date: Wed, 30 Oct 2024 10:40:31 -0700 Subject: [PATCH 1/7] Added duplicated packet event/warning --- Svc/FileUplink/Events.fppi | 13 +++++++++++-- Svc/FileUplink/FileUplink.hpp | 5 +++++ Svc/FileUplink/Warnings.cpp | 11 +++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/Svc/FileUplink/Events.fppi b/Svc/FileUplink/Events.fppi index 3949d90fb5..357b2b381f 100644 --- a/Svc/FileUplink/Events.fppi +++ b/Svc/FileUplink/Events.fppi @@ -63,10 +63,19 @@ event PacketOutOfOrder( format "Received packet {} after packet {}" \ throttle 20 +@ The File Uplink component encountered a duplicate packet during file receipt +event PacketDuplicate( + packetIndex: U32 @< The sequence index of the duplicate packet + ) \ + severity warning high \ + id 7 \ + format "Received a duplicate of packet {}" \ + throttle 20 + @ The File Uplink component received a CANCEL packet event UplinkCanceled \ severity activity high \ - id 7 \ + id 8 \ format "Received CANCEL packet" @ Error decoding file packet @@ -74,5 +83,5 @@ event DecodeError( status: I32 @< The sequence index of the out-of-order packet ) \ severity warning high \ - id 8 \ + id 9 \ format "Unable to decode file packet. Status: {}" diff --git a/Svc/FileUplink/FileUplink.hpp b/Svc/FileUplink/FileUplink.hpp index ce3b6793b5..46273ba300 100644 --- a/Svc/FileUplink/FileUplink.hpp +++ b/Svc/FileUplink/FileUplink.hpp @@ -161,6 +161,11 @@ namespace Svc { const U32 lastSequenceIndex ); + //! Record a Duplicate Packet warning + void packetDuplicate( + const U32 sequenceIndex + ); + //! Record a File Write warning void fileWrite(Fw::LogStringArg& fileName); diff --git a/Svc/FileUplink/Warnings.cpp b/Svc/FileUplink/Warnings.cpp index da1ee4831c..fbbbb9ce19 100644 --- a/Svc/FileUplink/Warnings.cpp +++ b/Svc/FileUplink/Warnings.cpp @@ -57,6 +57,17 @@ namespace Svc { this->warning(); } + void FileUplink::Warnings :: + packetDuplicate( + const U32 sequenceIndex + ) + { + this->m_fileUplink->log_WARNING_HI_PacketDuplicate( + sequenceIndex + ); + this->warning(); + } + void FileUplink::Warnings :: fileWrite(Fw::LogStringArg& fileName) { From cd94c63e104c9a61293c8dc81ac226f030e22abc Mon Sep 17 00:00:00 2001 From: David J Kessler Date: Wed, 30 Oct 2024 10:48:26 -0700 Subject: [PATCH 2/7] Added method to check for duplicated packet --- Svc/FileUplink/FileUplink.cpp | 12 ++++++++++++ Svc/FileUplink/FileUplink.hpp | 3 +++ 2 files changed, 15 insertions(+) diff --git a/Svc/FileUplink/FileUplink.cpp b/Svc/FileUplink/FileUplink.cpp index 2eea06d78a..162b902877 100644 --- a/Svc/FileUplink/FileUplink.cpp +++ b/Svc/FileUplink/FileUplink.cpp @@ -174,6 +174,18 @@ namespace Svc { this->m_lastSequenceIndex = sequenceIndex; } + bool FileUplink :: + checkDuplicatedPacket(const U32 sequenceIndex) + { + // check for duplicate packet + if (sequenceIndex == this->m_lastSequenceIndex) { + this->m_warnings.packetDuplicate(sequenceIndex); + return true; + } + + return false; + } + void FileUplink :: compareChecksums(const Fw::FilePacket::EndPacket& endPacket) { diff --git a/Svc/FileUplink/FileUplink.hpp b/Svc/FileUplink/FileUplink.hpp index 46273ba300..f42acb1777 100644 --- a/Svc/FileUplink/FileUplink.hpp +++ b/Svc/FileUplink/FileUplink.hpp @@ -251,6 +251,9 @@ namespace Svc { //! Check sequence index void checkSequenceIndex(const U32 sequenceIndex); + //! Check if a received packet is a duplicate + bool checkDuplicatedPacket(const U32 sequenceIndex); + //! Compare checksums void compareChecksums(const Fw::FilePacket::EndPacket& endPacket); From 427247d0a0d5d0959d47b179f6351bdd60795a25 Mon Sep 17 00:00:00 2001 From: David J Kessler Date: Wed, 30 Oct 2024 10:52:06 -0700 Subject: [PATCH 3/7] Added last packet write status member variable --- Svc/FileUplink/FileUplink.cpp | 3 +++ Svc/FileUplink/FileUplink.hpp | 3 +++ 2 files changed, 6 insertions(+) diff --git a/Svc/FileUplink/FileUplink.cpp b/Svc/FileUplink/FileUplink.cpp index 162b902877..2d73ee965a 100644 --- a/Svc/FileUplink/FileUplink.cpp +++ b/Svc/FileUplink/FileUplink.cpp @@ -25,6 +25,7 @@ namespace Svc { FileUplinkComponentBase(name), m_receiveMode(START), m_lastSequenceIndex(0), + m_lastPacketWriteStatus(Os::File::OP_OK), m_filesReceived(this), m_packetsReceived(this), m_warnings(this) @@ -136,6 +137,8 @@ namespace Svc { if (status != Os::File::OP_OK) { this->m_warnings.fileWrite(this->m_file.name); } + + this->m_lastPacketWriteStatus = status; } void FileUplink :: diff --git a/Svc/FileUplink/FileUplink.hpp b/Svc/FileUplink/FileUplink.hpp index f42acb1777..ca626bc2c5 100644 --- a/Svc/FileUplink/FileUplink.hpp +++ b/Svc/FileUplink/FileUplink.hpp @@ -275,6 +275,9 @@ namespace Svc { //! The sequence index of the last packet received U32 m_lastSequenceIndex; + //! The write status of the last packet received + Os::File::Status m_lastPacketWriteStatus; + //! The file being assembled File m_file; From a0b7c0a9c9765ef8e5879a3d1ea32be8b4038e2a Mon Sep 17 00:00:00 2001 From: David J Kessler Date: Wed, 30 Oct 2024 10:53:25 -0700 Subject: [PATCH 4/7] Duplicate file packets are now skipped --- Svc/FileUplink/FileUplink.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Svc/FileUplink/FileUplink.cpp b/Svc/FileUplink/FileUplink.cpp index 2d73ee965a..fa13bf89f8 100644 --- a/Svc/FileUplink/FileUplink.cpp +++ b/Svc/FileUplink/FileUplink.cpp @@ -121,7 +121,15 @@ namespace Svc { this->m_warnings.invalidReceiveMode(Fw::FilePacket::T_DATA); return; } + const U32 sequenceIndex = dataPacket.asHeader().getSequenceIndex(); + + // skip this packet if it is a duplicate and it has already been written + if (this->m_lastPacketWriteStatus == Os::File::OP_OK && + this->checkDuplicatedPacket(sequenceIndex)) { + return; + } + this->checkSequenceIndex(sequenceIndex); const U32 byteOffset = dataPacket.getByteOffset(); const U32 dataSize = dataPacket.getDataSize(); From f97c010d95166b456577f92f58530637cdd7c943 Mon Sep 17 00:00:00 2001 From: David J Kessler Date: Wed, 30 Oct 2024 10:54:53 -0700 Subject: [PATCH 5/7] Added duplicate file packet UT --- Svc/FileUplink/test/ut/FileUplinkMain.cpp | 5 +++ Svc/FileUplink/test/ut/FileUplinkTester.cpp | 42 +++++++++++++++++++++ Svc/FileUplink/test/ut/FileUplinkTester.hpp | 4 ++ 3 files changed, 51 insertions(+) diff --git a/Svc/FileUplink/test/ut/FileUplinkMain.cpp b/Svc/FileUplink/test/ut/FileUplinkMain.cpp index 46feb643a5..fd0f288aa9 100644 --- a/Svc/FileUplink/test/ut/FileUplinkMain.cpp +++ b/Svc/FileUplink/test/ut/FileUplinkMain.cpp @@ -49,6 +49,11 @@ TEST(FileUplink, PacketOutOfOrder) { tester.packetOutOfOrder(); } +TEST(FileUplink, PacketDuplicated) { + Svc::FileUplinkTester tester; + tester.packetDuplicated(); +} + TEST(FileUplink, CancelPacketInStartMode) { Svc::FileUplinkTester tester; tester.cancelPacketInStartMode(); diff --git a/Svc/FileUplink/test/ut/FileUplinkTester.cpp b/Svc/FileUplink/test/ut/FileUplinkTester.cpp index 1163807e7f..5fcbe02be9 100644 --- a/Svc/FileUplink/test/ut/FileUplinkTester.cpp +++ b/Svc/FileUplink/test/ut/FileUplinkTester.cpp @@ -377,6 +377,48 @@ namespace Svc { } + void FileUplinkTester :: + packetDuplicated() + { + const char *const sourcePath = "source.bin"; + const char *const destPath = "dest.bin"; + U8 packetData[] = { 5, 6, 7, 8, 9 }; + const size_t fileSize = 2 * PACKET_SIZE; + + // Send the start packet (packet 0) + this->sendStartPacket(sourcePath, destPath, fileSize); + ASSERT_TLM_SIZE(1); + ASSERT_TLM_PacketsReceived( + 0, + ++this->expectedPacketsReceived + ); + ASSERT_EVENTS_SIZE(0); + + // Simulate duplication of packet 0 + --this->sequenceIndex; + + // capture the checksum after sending the first packet + const ::CFDP::Checksum expectedChecksum(component.m_file.m_checksum); + + // Send the data packet (packet 0) again + const size_t byteOffset = 0; + this->sendDataPacket(byteOffset, packetData); + ASSERT_TLM_SIZE(2); + ASSERT_TLM_PacketsReceived( + 0, + ++this->expectedPacketsReceived + ); + ASSERT_TLM_Warnings(0, 1); + + ASSERT_EVENTS_SIZE(1); + ASSERT_EVENTS_PacketDuplicate(0, component.m_lastSequenceIndex); + + // verify that the checksum hasn't changed + ASSERT_EQ(expectedChecksum, component.m_file.m_checksum); + + this->removeFile("test.bin"); + } + void FileUplinkTester :: cancelPacketInStartMode() { diff --git a/Svc/FileUplink/test/ut/FileUplinkTester.hpp b/Svc/FileUplink/test/ut/FileUplinkTester.hpp index 21acb06a47..19703c4770 100644 --- a/Svc/FileUplink/test/ut/FileUplinkTester.hpp +++ b/Svc/FileUplink/test/ut/FileUplinkTester.hpp @@ -80,6 +80,10 @@ namespace Svc { //! void packetOutOfOrder(); + //! Send a file with an duplicated packet + //! + void packetDuplicated(); + //! Send a CANCEL packet in START mode //! void cancelPacketInStartMode(); From 1915264889496fe29a47736ad6de48f1141336a6 Mon Sep 17 00:00:00 2001 From: David J Kessler Date: Thu, 7 Nov 2024 12:45:46 -0800 Subject: [PATCH 6/7] Fixed `m_lastPacketWriteStatus` initialization. --- Svc/FileUplink/FileUplink.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Svc/FileUplink/FileUplink.cpp b/Svc/FileUplink/FileUplink.cpp index fa13bf89f8..6c19af7a2e 100644 --- a/Svc/FileUplink/FileUplink.cpp +++ b/Svc/FileUplink/FileUplink.cpp @@ -25,7 +25,7 @@ namespace Svc { FileUplinkComponentBase(name), m_receiveMode(START), m_lastSequenceIndex(0), - m_lastPacketWriteStatus(Os::File::OP_OK), + m_lastPacketWriteStatus(Os::File::MAX_STATUS), m_filesReceived(this), m_packetsReceived(this), m_warnings(this) @@ -217,6 +217,7 @@ namespace Svc { this->m_file.osFile.close(); this->m_receiveMode = START; this->m_lastSequenceIndex = 0; + this->m_lastPacketWriteStatus = Os::File::MAX_STATUS; } void FileUplink :: @@ -224,6 +225,7 @@ namespace Svc { { this->m_receiveMode = DATA; this->m_lastSequenceIndex = 0; + this->m_lastPacketWriteStatus = Os::File::MAX_STATUS; } } From 23fce4362aa17ae6350fb64f1ef07d982e04b222 Mon Sep 17 00:00:00 2001 From: David J Kessler Date: Thu, 7 Nov 2024 12:46:48 -0800 Subject: [PATCH 7/7] Updated UT to test that the first packet isn't erroneously marked as a duplicate --- Svc/FileUplink/test/ut/FileUplinkTester.cpp | 24 +++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/Svc/FileUplink/test/ut/FileUplinkTester.cpp b/Svc/FileUplink/test/ut/FileUplinkTester.cpp index 5fcbe02be9..5ab98cf569 100644 --- a/Svc/FileUplink/test/ut/FileUplinkTester.cpp +++ b/Svc/FileUplink/test/ut/FileUplinkTester.cpp @@ -394,14 +394,30 @@ namespace Svc { ); ASSERT_EVENTS_SIZE(0); - // Simulate duplication of packet 0 - --this->sequenceIndex; + ASSERT_EQ(0, component.m_lastSequenceIndex); + ASSERT_EQ(1, this->sequenceIndex); + ASSERT_EQ(Os::File::MAX_STATUS, component.m_lastPacketWriteStatus); + + // Send data packet 1 + const size_t byteOffset = 0; + this->sendDataPacket(byteOffset, packetData); + ASSERT_TLM_SIZE(1); + ASSERT_TLM_PacketsReceived( + 0, + ++this->expectedPacketsReceived + ); + ASSERT_TLM_Warnings_SIZE(0); // capture the checksum after sending the first packet const ::CFDP::Checksum expectedChecksum(component.m_file.m_checksum); - // Send the data packet (packet 0) again - const size_t byteOffset = 0; + // Simulate duplication of packet 1 + --this->sequenceIndex; + + ASSERT_EQ(this->sequenceIndex, component.m_lastSequenceIndex); + ASSERT_EQ(Os::File::OP_OK, component.m_lastPacketWriteStatus); + + // Send data packet 1 again this->sendDataPacket(byteOffset, packetData); ASSERT_TLM_SIZE(2); ASSERT_TLM_PacketsReceived(