From 4180ddb83a6b7646180b2b7a0e9b3919c48a6760 Mon Sep 17 00:00:00 2001
From: Dimitrios Stoupis <dimitris.apple@gmail.com>
Date: Wed, 21 Nov 2018 22:29:13 +0000
Subject: [PATCH] Implement all of the code review suggestions
---
inc/Message.hpp | 34 +++++++++++++++--------
src/Message.cpp | 20 ++++++++++++-
src/Services/MemoryManagementService.cpp | 1 -
src/main.cpp | 2 --
test/Services/MemoryManagementService.cpp | 8 ++----
5 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/inc/Message.hpp b/inc/Message.hpp
index ce2c5944..6ccef49a 100644
--- a/inc/Message.hpp
+++ b/inc/Message.hpp
@@ -78,6 +78,16 @@ public:
*/
void appendString(uint8_t size, const char *value);
+ /**
+ * Appends \p size bytes to the message
+ *
+ * @param size The amount of byte to append
+ * @param value An array containing at least \p size bytes
+ * @todo See if more than uint8_t strings will be supported
+ * @todo Is uint16_t size too much or not enough? It has to be defined
+ */
+ void appendString(uint16_t size, uint8_t *value);
+
/**
* Reads the next \p numBits bits from the the message in a big-endian format
* @param numBits
@@ -108,6 +118,15 @@ public:
*/
void readString(char *string, uint8_t size);
+ /**
+ * Reads the next \p size bytes from the message, and stores them into the allocated \p string
+ *
+ * NOTE: We assume that \p string is already allocated, and its size is at least
+ * ECSS_MAX_STRING_SIZE. This function does placs a \0 at the end of the created string
+ * @todo Is uint16_t size too much or not enough? It has to be defined
+ */
+ void readString(uint8_t *value, uint16_t size);
+
public:
Message(uint8_t serviceType, uint8_t messageType, PacketType packetType,
uint16_t applicationId);
@@ -243,11 +262,8 @@ public:
* PTC = 7, PFC = 0
*/
void appendOctetString(uint16_t size, uint8_t *byteString) {
- assert(size < ECSS_MAX_STRING_SIZE);
-
- for (std::size_t i = 0; i < size; i++) {
- appendByte(byteString[i]);
- }
+ appendUint16(size);
+ appendString(size, byteString);
}
/**
@@ -385,12 +401,8 @@ public:
*
* PTC = 7, PFC = 0
*/
- void readOctetString(uint8_t *byteString, uint16_t size) {
- assert(size < ECSS_MAX_STRING_SIZE);
-
- for (std::size_t i = 0; i < size; i++) {
- byteString[i] = readByte();
- }
+ void readOctetString(uint16_t size, uint8_t *byteString) {
+ readString(byteString, size);
}
/**
diff --git a/src/Message.cpp b/src/Message.cpp
index 292ec695..0e95ad32 100644
--- a/src/Message.cpp
+++ b/src/Message.cpp
@@ -74,6 +74,15 @@ void Message::appendString(uint8_t size, const char *value) {
dataSize += size;
}
+void Message::appendString(uint16_t size, uint8_t *value) {
+ assert(dataSize + size <= ECSS_MAX_MESSAGE_SIZE);
+ assert(size < ECSS_MAX_STRING_SIZE);
+
+ memcpy(data + dataSize, value, size);
+
+ dataSize += size;
+}
+
uint16_t Message::readBits(uint8_t numBits) {
assert(numBits <= 16);
// TODO: Add assert
@@ -135,7 +144,16 @@ void Message::readString(char *string, uint8_t size) {
assert(size < ECSS_MAX_STRING_SIZE);
memcpy(string, data + readPosition, size);
- string[size] = '\0';
+ string[size] = '\0'; // todo: Use that for now to avoid problems. Later to be removed
+
+ readPosition += size;
+}
+
+void Message::readString(uint8_t *string, uint16_t size) {
+ assert(readPosition + size <= ECSS_MAX_MESSAGE_SIZE);
+ assert(size < ECSS_MAX_STRING_SIZE);
+
+ memcpy(string, data + readPosition, size);
readPosition += size;
}
diff --git a/src/Services/MemoryManagementService.cpp b/src/Services/MemoryManagementService.cpp
index 72c086a5..5ba62e64 100644
--- a/src/Services/MemoryManagementService.cpp
+++ b/src/Services/MemoryManagementService.cpp
@@ -108,7 +108,6 @@ void MemoryManagementService::RawDataMemoryManagement::dumpRawData(Message &requ
// This part is repeated N-times (N = iteration count)
report.appendUint64(startAddress); // Start address
- report.appendUint16(readLength); // Data read length
report.appendOctetString(readLength, readData); // Save the read data
}
// todo: implement and append the checksum part of the reporting packet
diff --git a/src/main.cpp b/src/main.cpp
index d1a628e2..f9bf03e4 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -56,10 +56,8 @@ int main() {
rcvPack.appendEnum8(MemoryManagementService::MemoryID::RAM); // Memory ID
rcvPack.appendUint16(2); // Iteration count
rcvPack.appendUint64(reinterpret_cast<uint64_t >(pStr)); // Start address
- rcvPack.appendUint16(2); // Data length to append
rcvPack.appendOctetString(2, data);
rcvPack.appendUint64(reinterpret_cast<uint64_t >(pStr + 1)); // Start address
- rcvPack.appendUint16(1);
rcvPack.appendOctetString(1, data);
memMangService.rawDataMemorySubservice.loadRawData(rcvPack);
diff --git a/test/Services/MemoryManagementService.cpp b/test/Services/MemoryManagementService.cpp
index 824866cb..984c51f9 100644
--- a/test/Services/MemoryManagementService.cpp
+++ b/test/Services/MemoryManagementService.cpp
@@ -17,10 +17,8 @@ TEST_CASE("TM[6,2]", "[service][st06]") {
receivedPacket.appendEnum8(MemoryManagementService::MemoryID::RAM); // Memory ID
receivedPacket.appendUint16(2); // Iteration count
receivedPacket.appendUint64(reinterpret_cast<uint64_t >(pStr)); // Start address
- receivedPacket.appendUint16(2); // Data length to append
receivedPacket.appendOctetString(2, data);
receivedPacket.appendUint64(reinterpret_cast<uint64_t >(pStr + 2)); // Start address
- receivedPacket.appendUint16(1);
receivedPacket.appendOctetString(1, data);
memMangService.rawDataMemorySubservice.loadRawData(receivedPacket);
@@ -62,7 +60,7 @@ TEST_CASE("TM[6,5]", "[service][st06]") {
CHECK(response.readUint64() == reinterpret_cast<uint64_t >(testString_1));
readSize = sizeof(testString_1)/ sizeof(testString_1[0]);
CHECK(response.readUint16() == readSize);
- response.readOctetString(checkString, readSize);
+ response.readOctetString(readSize, checkString);
CHECK(checkString[0] == 'F');
CHECK(checkString[1] == 'S');
CHECK(checkString[2] == 't');
@@ -73,7 +71,7 @@ TEST_CASE("TM[6,5]", "[service][st06]") {
CHECK(response.readUint64() == reinterpret_cast<uint64_t >(testString_2));
readSize = sizeof(testString_2)/ sizeof(testString_2[0]);
CHECK(response.readUint16() == readSize);
- response.readOctetString(checkString, readSize);
+ response.readOctetString(readSize, checkString);
CHECK(checkString[0] == 'S');
CHECK(checkString[1] == 'e');
CHECK(checkString[2] == 'c');
@@ -86,7 +84,7 @@ TEST_CASE("TM[6,5]", "[service][st06]") {
CHECK(response.readUint64() == reinterpret_cast<uint64_t >(testString_3));
readSize = sizeof(testString_3)/ sizeof(testString_3[0]);
CHECK(response.readUint16() == readSize);
- response.readOctetString(checkString, readSize);
+ response.readOctetString(readSize, checkString);
CHECK(checkString[0] == 5);
CHECK(checkString[1] == 8);
}
--
GitLab