Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FileUplink packet sequence repeat and CRC #1378 #3007

Merged
merged 7 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions Svc/FileUplink/Events.fppi
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,25 @@ 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
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: {}"
23 changes: 23 additions & 0 deletions Svc/FileUplink/FileUplink.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
FileUplinkComponentBase(name),
m_receiveMode(START),
m_lastSequenceIndex(0),
m_lastPacketWriteStatus(Os::File::OP_OK),
m_filesReceived(this),
m_packetsReceived(this),
m_warnings(this)
Expand Down Expand Up @@ -120,7 +121,15 @@
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();
Expand All @@ -136,6 +145,8 @@
if (status != Os::File::OP_OK) {
this->m_warnings.fileWrite(this->m_file.name);
}

this->m_lastPacketWriteStatus = status;
}

void FileUplink ::
Expand Down Expand Up @@ -174,6 +185,18 @@
this->m_lastSequenceIndex = sequenceIndex;
}

bool FileUplink ::
checkDuplicatedPacket(const U32 sequenceIndex)
Comment on lines +188 to +189

Check notice

Code scanning / CodeQL

Use of basic integral type Note

checkDuplicatedPacket uses the basic integral type bool rather than a typedef with size and signedness.
{
// check for duplicate packet
if (sequenceIndex == this->m_lastSequenceIndex) {
this->m_warnings.packetDuplicate(sequenceIndex);
LeStarch marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

return false;
}

void FileUplink ::
compareChecksums(const Fw::FilePacket::EndPacket& endPacket)
{
Expand Down
11 changes: 11 additions & 0 deletions Svc/FileUplink/FileUplink.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -246,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);

Expand All @@ -267,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;

Expand Down
11 changes: 11 additions & 0 deletions Svc/FileUplink/Warnings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@
this->warning();
}

void FileUplink::Warnings ::
packetDuplicate(
const U32 sequenceIndex
)
{
this->m_fileUplink->log_WARNING_HI_PacketDuplicate(
sequenceIndex

Check warning

Code scanning / CodeQL

Unchecked function argument Warning

This use of parameter sequenceIndex has not been checked.
);
this->warning();
}

void FileUplink::Warnings ::
fileWrite(Fw::LogStringArg& fileName)
{
Expand Down
5 changes: 5 additions & 0 deletions Svc/FileUplink/test/ut/FileUplinkMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
42 changes: 42 additions & 0 deletions Svc/FileUplink/test/ut/FileUplinkTester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
4 changes: 4 additions & 0 deletions Svc/FileUplink/test/ut/FileUplinkTester.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading