From 5db37658dd1e6974abf8b04e7cfa5726e486d0d5 Mon Sep 17 00:00:00 2001
From: kongr45gpen <electrovesta@gmail.com>
Date: Tue, 27 Aug 2019 20:57:41 +0300
Subject: [PATCH] Add tests to ensure the correctness of the
Message::finalize() functions
Switch order of execution to improve performance
Fix the build
---
inc/Message.hpp | 47 ++++++++++++++------
src/Message.cpp | 1 -
src/ServicePool.cpp | 4 +-
test/Message.cpp | 12 ++++-
test/Services/LargePacketTransferService.cpp | 3 +-
test/Services/TimeBasedSchedulingService.cpp | 17 +++----
6 files changed, 56 insertions(+), 28 deletions(-)
diff --git a/inc/Message.hpp b/inc/Message.hpp
index 55a5f05b..904c8f67 100644
--- a/inc/Message.hpp
+++ b/inc/Message.hpp
@@ -34,23 +34,40 @@ public:
* @brief Overload the equality operator to compare messages
* @details Compare two @ref ::Message objects, based on their contents and type
* @param The message content to compare against
- * @todo Activate the dataSize check when the Message object data field is defined with a
- * fixed size
* @return The result of comparison
*/
bool operator==(const Message& msg) const {
- // todo: Enable the following check when the message data field has a fixed size padded
- // with zeros. At the moment the data array is not padded with zeros to fulfil the
- // maximum set number of a TC request message.
- // if (this->dataSize != msg.dataSize) return false;
-
- for (uint16_t i = 0; i < ECSS_MAX_MESSAGE_SIZE; i++) {
- if (this->data[i] != msg.data[i]) {
- return false;
- }
+ if (dataSize != msg.dataSize) {
+ return false;
}
- return (this->packetType == msg.packetType) && (this->messageType == msg.messageType) &&
- (this->serviceType == msg.serviceType);
+
+ if (not isSameType(*this, msg)) {
+ return false;
+ }
+
+ return std::equal(data, data + dataSize, msg.data);
+ }
+
+ /**
+ * Checks the first \ref Message::dataSize bytes of \p msg for equality
+ *
+ * This performs an equality check for the first `[0, this->dataSize)` bytes of two messages. Useful to compare
+ * two messages that have the same content, but one of which does not know its length.
+ *
+ * @param msg The message to check. Its `dataSize` must be smaller than the object calling the function
+ * @return False if the messages are not of the same type, if `msg.dataSize < this->dataSize`, or if the first
+ * `this->dataSize` bytes are not equal between the two messages.
+ */
+ bool bytesEqualWith(const Message& msg) const {
+ if (msg.dataSize < dataSize) {
+ return false;
+ }
+
+ if (not isSameType(*this, msg)) {
+ return false;
+ }
+
+ return std::equal(data, data + dataSize, msg.data);
}
enum PacketType {
@@ -84,7 +101,9 @@ public:
// Pointer to the contents of the message (excluding the PUS header)
// We allocate this data statically, in order to make sure there is predictability in the
// handling and storage of messages
- // TODO: Is it a good idea to not initialise this to 0?
+ //
+ // @note This is initialized to 0 in order to prevent any mishaps with non-properly initialized values. \ref
+ // Message::appendBits() relies on this in order to easily OR the requested bits.
uint8_t data[ECSS_MAX_MESSAGE_SIZE] = { 0 };
// private:
diff --git a/src/Message.cpp b/src/Message.cpp
index a5fea247..dcb3bbd8 100644
--- a/src/Message.cpp
+++ b/src/Message.cpp
@@ -39,7 +39,6 @@ void Message::finalize() {
// Define the spare field in telemetry and telecommand user data field (7.4.3.2.c and 7.4.4.2.c)
if (currentBit != 0) {
currentBit = 0;
- data[dataSize] = 0; // Make sure that the last byte is zero
dataSize++;
}
diff --git a/src/ServicePool.cpp b/src/ServicePool.cpp
index f3a529fd..b8e7c963 100644
--- a/src/ServicePool.cpp
+++ b/src/ServicePool.cpp
@@ -14,7 +14,7 @@ void ServicePool::reset() {
}
uint16_t ServicePool::getAndUpdateMessageTypeCounter(uint8_t serviceType, uint8_t messageType) {
- uint16_t key = (serviceType << 8u) | messageType; // Create the key of the map
+ uint16_t key = (serviceType << 8U) | messageType; // Create the key of the map
return (messageTypeCounter[key])++; // Fetch and increase the value
}
@@ -22,7 +22,7 @@ uint16_t ServicePool::getAndUpdatePacketSequenceCounter() {
uint16_t value = packetSequenceCounter;
// Increase the value
- if ((++packetSequenceCounter) >= (1u << 14u)) { // The value of the packet sequence counter is <= (2^14 - 1)
+ if ((++packetSequenceCounter) >= (1U << 14U)) { // The value of the packet sequence counter is <= (2^14 - 1)
packetSequenceCounter = 0;
}
diff --git a/test/Message.cpp b/test/Message.cpp
index f6eba246..2c92a13e 100644
--- a/test/Message.cpp
+++ b/test/Message.cpp
@@ -178,17 +178,19 @@ TEST_CASE("Spare field", "[message]") {
message1.appendByte(1);
message1.appendHalfword(2);
- message1.appendBits(1, 5);
+ message1.appendBits(1, 1);
message1.finalize();
+ CHECK(message1.data[3] == 0b10000000);
CHECK(message1.dataSize == 4);
Message message2(0, 0, Message::TM, 0);
message2.appendByte(1);
message2.appendHalfword(2);
- message2.appendBits(2, 5);
+ message2.appendBits(2, 3);
message2.finalize();
+ CHECK(message2.data[3] == 0b11000000);
CHECK(message2.dataSize == 4);
Message message3(0, 0, Message::TM, 0);
@@ -198,6 +200,7 @@ TEST_CASE("Spare field", "[message]") {
message3.appendBits(3, 5);
message3.finalize();
+ CHECK(message3.data[3] == 0b10100000);
CHECK(message3.dataSize == 4);
Message message4(0, 0, Message::TM, 0);
@@ -207,6 +210,7 @@ TEST_CASE("Spare field", "[message]") {
message4.appendBits(4, 5);
message4.finalize();
+ CHECK(message4.data[3] == 0b01010000);
CHECK(message4.dataSize == 4);
Message message5(0, 0, Message::TM, 0);
@@ -216,6 +220,7 @@ TEST_CASE("Spare field", "[message]") {
message5.appendBits(5, 5);
message5.finalize();
+ CHECK(message5.data[3] == 0b00101000);
CHECK(message5.dataSize == 4);
Message message6(0, 0, Message::TM, 0);
@@ -225,6 +230,7 @@ TEST_CASE("Spare field", "[message]") {
message6.appendBits(6, 5);
message6.finalize();
+ CHECK(message6.data[3] == 0b00010100);
CHECK(message6.dataSize == 4);
Message message7(0, 0, Message::TM, 0);
@@ -234,6 +240,7 @@ TEST_CASE("Spare field", "[message]") {
message7.appendBits(7, 5);
message7.finalize();
+ CHECK(message7.data[3] == 0b00001010);
CHECK(message7.dataSize == 4);
Message message8(0, 0, Message::TM, 0);
@@ -243,6 +250,7 @@ TEST_CASE("Spare field", "[message]") {
message8.appendBits(8, 5);
message8.finalize();
+ CHECK(message8.data[3] == 0b00000101);
CHECK(message8.dataSize == 4);
Message message9(0, 0, Message::TM, 0);
diff --git a/test/Services/LargePacketTransferService.cpp b/test/Services/LargePacketTransferService.cpp
index 4d75409a..be5c9fdf 100644
--- a/test/Services/LargePacketTransferService.cpp
+++ b/test/Services/LargePacketTransferService.cpp
@@ -86,5 +86,6 @@ TEST_CASE("Split function", "[no][service]") {
message5.appendUint8(ServiceTests::get(i).readUint8());
}
}
- CHECK(message == message5);
+
+ CHECK(message.bytesEqualWith(message5));
}
diff --git a/test/Services/TimeBasedSchedulingService.cpp b/test/Services/TimeBasedSchedulingService.cpp
index 56a7267d..b56a8bd3 100644
--- a/test/Services/TimeBasedSchedulingService.cpp
+++ b/test/Services/TimeBasedSchedulingService.cpp
@@ -115,14 +115,15 @@ TEST_CASE("TC[11,4] Activity Insertion", "[service][st11]") {
auto scheduledActivities = activityInsertion(timeBasedService);
REQUIRE(scheduledActivities.size() == 4);
+
REQUIRE(scheduledActivities.at(0)->requestReleaseTime == currentTime + 1556435);
REQUIRE(scheduledActivities.at(1)->requestReleaseTime == currentTime + 1726435);
REQUIRE(scheduledActivities.at(2)->requestReleaseTime == currentTime + 1957232);
REQUIRE(scheduledActivities.at(3)->requestReleaseTime == currentTime + 17248435);
- REQUIRE(scheduledActivities.at(0)->request == testMessage1);
- REQUIRE(scheduledActivities.at(1)->request == testMessage3);
- REQUIRE(scheduledActivities.at(2)->request == testMessage2);
- REQUIRE(scheduledActivities.at(3)->request == testMessage4);
+ REQUIRE(testMessage1.bytesEqualWith(scheduledActivities.at(0)->request));
+ REQUIRE(testMessage3.bytesEqualWith(scheduledActivities.at(1)->request));
+ REQUIRE(testMessage2.bytesEqualWith(scheduledActivities.at(2)->request));
+ REQUIRE(testMessage4.bytesEqualWith(scheduledActivities.at(3)->request));
SECTION("Error throw test") {
Message receivedMessage(11, 4, Message::TC, 1);
@@ -198,7 +199,7 @@ TEST_CASE("TC[11,7] Time shift activities by ID", "[service][st11]") {
// Make sure the new value is inserted sorted
REQUIRE(scheduledActivities.at(3)->requestReleaseTime == currentTime + 1957232 + timeShift);
- REQUIRE(scheduledActivities.at(3)->request == testMessage2);
+ REQUIRE(testMessage2.bytesEqualWith(scheduledActivities.at(3)->request));
}
SECTION("Negative Shift") {
@@ -213,7 +214,7 @@ TEST_CASE("TC[11,7] Time shift activities by ID", "[service][st11]") {
// Output should be sorted
REQUIRE(scheduledActivities.at(1)->requestReleaseTime == currentTime + 1957232 - 250000);
- REQUIRE(scheduledActivities.at(1)->request == testMessage2);
+ REQUIRE(testMessage2.bytesEqualWith(scheduledActivities.at(1)->request));
}
SECTION("Error throw on wrong request ID") {
@@ -382,7 +383,7 @@ TEST_CASE("TC[11,16] Detail report all scheduled activities", "[service][st11]")
receivedTCPacket = msgParser.parseRequestTC(receivedDataStr);
REQUIRE(receivedReleaseTime == scheduledActivities.at(i)->requestReleaseTime);
- REQUIRE(scheduledActivities.at(i)->request == receivedTCPacket);
+ REQUIRE(receivedTCPacket.bytesEqualWith(scheduledActivities.at(i)->request));
}
}
@@ -408,7 +409,7 @@ TEST_CASE("TC[11,5] Activity deletion by ID", "[service][st11]") {
REQUIRE(scheduledActivities.size() == 3);
REQUIRE(scheduledActivities.at(2)->requestReleaseTime == currentTime + 17248435);
- REQUIRE(scheduledActivities.at(2)->request == testMessage4);
+ REQUIRE(testMessage4.bytesEqualWith(scheduledActivities.at(2)->request));
}
SECTION("Error throw on wrong request ID") {
--
GitLab