From d08c595cbb2c41c51cf852f7ed11e7cfe8a650bb Mon Sep 17 00:00:00 2001
From: athatheo <vostidi@hotmail.com>
Date: Sun, 12 Apr 2020 03:53:49 +0300
Subject: [PATCH] Parameter Service overhaul - addressed review
---
CMakeLists.txt | 2 +-
inc/ECSS_Definitions.hpp | 8 +-
inc/ErrorHandler.hpp | 18 +-
inc/Parameters/Parameters.hpp | 17 --
.../x86/Parameters/SystemParameters.hpp | 27 +++
inc/Services/Parameter.hpp | 9 +-
inc/Services/ParameterService.hpp | 26 +--
src/Parameters/Parameters.cpp | 3 -
.../x86/Parameters/SystemParameters.cpp | 3 +
src/Services/ParameterService.cpp | 27 ++-
test/Services/Parameter.cpp | 10 +
test/Services/ParameterService.cpp | 208 +++++++++++-------
12 files changed, 205 insertions(+), 153 deletions(-)
delete mode 100644 inc/Parameters/Parameters.hpp
create mode 100644 inc/Platform/x86/Parameters/SystemParameters.hpp
delete mode 100644 src/Parameters/Parameters.cpp
create mode 100644 src/Platform/x86/Parameters/SystemParameters.cpp
create mode 100644 test/Services/Parameter.cpp
diff --git a/CMakeLists.txt b/CMakeLists.txt
index fdfe6916..50c416bb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -31,7 +31,7 @@ add_library(common OBJECT
src/Helpers/CRCHelper.cpp
src/Helpers/TimeAndDate.cpp
src/Helpers/TimeHelper.cpp
- src/Parameters/Parameters.cpp
+ src/Platform/x86/Parameters/SystemParameters.cpp
src/Services/EventReportService.cpp
src/Services/MemoryManagementService.cpp
src/Services/ParameterService.cpp
diff --git a/inc/ECSS_Definitions.hpp b/inc/ECSS_Definitions.hpp
index 8c3df99f..40131723 100644
--- a/inc/ECSS_Definitions.hpp
+++ b/inc/ECSS_Definitions.hpp
@@ -144,11 +144,5 @@
/**
* @brief Size of the map holding the Parameter objects for the ST[20] parameter service
*/
-#define ECSS_ST_20_MAX_PARAMETERS 5
-
-/**
- * @brief Maximum etl::string output length in bytes for each \ref Parameter of ST[20]
- */
-
-#define ECSS_ST_20_MAX_STRING_LENGTH 5
+#define ECSS_PARAMETER_COUNT 3
#endif // ECSS_SERVICES_ECSS_DEFINITIONS_H
diff --git a/inc/ErrorHandler.hpp b/inc/ErrorHandler.hpp
index b0714882..407a6bf1 100644
--- a/inc/ErrorHandler.hpp
+++ b/inc/ErrorHandler.hpp
@@ -73,18 +73,10 @@ public:
* Attempt to insert new element in a full map ST[08]
*/
MapFull = 10,
- /**
- * Attempt to access a non existing parameter (ST[20])
- */
- GetNonExistingParameter = 11,
- /**
- * Attempt to change the value of a non existing parameter (ST[20])
- */
- SetNonExistingParameter = 12,
/**
* A Message that is included within another message is too large
*/
- NestedMessageTooLarge = 13
+ NestedMessageTooLarge = 11
};
/**
@@ -111,6 +103,14 @@ public:
* Cannot parse a Message, because there is an error in its secondary header
*/
UnacceptableMessage = 5,
+ /**
+ * Attempt to change the value of a non existing parameter (ST[20])
+ */
+ InvalidParameterId = 6,
+ /**
+ * Attempt to access a non existing parameter (ST[20])
+ */
+ GetNonExistingParameter = 7,
};
/**
diff --git a/inc/Parameters/Parameters.hpp b/inc/Parameters/Parameters.hpp
deleted file mode 100644
index f8651abb..00000000
--- a/inc/Parameters/Parameters.hpp
+++ /dev/null
@@ -1,17 +0,0 @@
-#include "Services/Parameter.hpp"
-#include "etl/vector.h"
-
-class SystemParameters {
-public:
- Parameter<uint8_t> parameter1 = Parameter<uint8_t>(5);
- Parameter<uint16_t> parameter2 = Parameter<uint16_t>(5);
- Parameter<uint32_t> parameter3 = Parameter<uint32_t>(5);
- etl::vector<std::reference_wrapper<ParameterBase>, ECSS_ST_20_MAX_PARAMETERS> parametersArray;
- SystemParameters() {
- parametersArray[0] = parameter1;
- parametersArray[1] = parameter2;
- parametersArray[2] = parameter3;
- }
-};
-
-extern SystemParameters systemParameters;
diff --git a/inc/Platform/x86/Parameters/SystemParameters.hpp b/inc/Platform/x86/Parameters/SystemParameters.hpp
new file mode 100644
index 00000000..c0b21c2c
--- /dev/null
+++ b/inc/Platform/x86/Parameters/SystemParameters.hpp
@@ -0,0 +1,27 @@
+#include "Services/Parameter.hpp"
+#include "etl/vector.h"
+/**
+ * @author Athanasios Theocharis <athatheoc@gmail.com>
+ */
+
+/**
+ * This class was created for the purpose of initializing and storing explicitly
+ * parameters (that are instances of the \ref Parameter class). It stores all the parameters
+ * of the specific application. Different subsystems should have their own implementations of this class.
+ * The position of the parameter in the vector is also called the parameter ID.
+ *
+ * It is initialised statically.
+ *
+ * The parameters here are under the responsibility of \ref ParameterService.
+ */
+class SystemParameters {
+public:
+ Parameter<uint8_t> parameter1 = Parameter<uint8_t>(3);
+ Parameter<uint16_t> parameter2 = Parameter<uint16_t>(7);
+ Parameter<uint32_t> parameter3 = Parameter<uint32_t>(10);
+ etl::array<std::reference_wrapper<ParameterBase>, ECSS_PARAMETER_COUNT> parametersArray = {
+ parameter1, parameter2, parameter3};
+ SystemParameters() = default;
+};
+
+extern SystemParameters systemParameters;
diff --git a/inc/Services/Parameter.hpp b/inc/Services/Parameter.hpp
index 947e574d..0d47847a 100644
--- a/inc/Services/Parameter.hpp
+++ b/inc/Services/Parameter.hpp
@@ -30,16 +30,17 @@
* would exclude from consideration constructs like etl::variant due to limitations on
* the number of supported distinct types, a custom solution was needed.
* Furthermore, the \ref ParameterService should provide ID-based access to parameters.
- *
*/
-
class ParameterBase {
public:
virtual void appendValueToMessage(Message& message) = 0;
virtual void setValueFromMessage(Message& message) = 0;
};
-
+/**
+ * Implementation of a parameter containing its value. See \ref ParameterBase for more information.
+ * @tparam DataType
+ */
template <typename DataType>
class Parameter : public ParameterBase {
private:
@@ -54,7 +55,7 @@ public:
currentValue = value;
}
- inline DataType getValue() {
+ DataType getValue() {
return currentValue;
}
diff --git a/inc/Services/ParameterService.hpp b/inc/Services/ParameterService.hpp
index f6da64c9..61286e03 100644
--- a/inc/Services/ParameterService.hpp
+++ b/inc/Services/ParameterService.hpp
@@ -6,7 +6,7 @@
#include "ErrorHandler.hpp"
#include "Parameter.hpp"
#include "etl/vector.h"
-#include "Parameters/Parameters.hpp"
+#include "Parameters/SystemParameters.hpp"
/**
* Implementation of the ST[20] parameter management service,
@@ -18,17 +18,13 @@
/**
* Parameter manager - ST[20]
- * Holds the list with the parameters and provides functions
- * for parameter reporting and modification.
*
- * The parameter list is stored in a map with the parameter IDs as keys and values
- * corresponding Parameter classes containing the parameter's value.
+ * The purpose of this class is to handle functions regarding the access and modification
+ * of the various parameters of the CubeSat.
+ * The parameters to be managed are initialized and kept in \ref SystemParameters.
*/
class ParameterService : public Service {
public:
- /**
- * @brief Fills the parameter map.
- */
ParameterService() = default;
/**
@@ -36,29 +32,17 @@ public:
* containing the current configuration
* **for the parameters specified in the carried valid IDs**.
*
- * The packet is checked for errors in service and message type, as well as for the
- * validity of the IDs contained. For every invalid ID an ExecutionStartErrorType::UnknownExecutionStartError
- * is raised.
- * If the packet has an incorrect header and service type, an InternalError::UnacceptableMessage is raised.
- * If no IDs are correct, the returned message shall be empty.
- *
* @param paramId: a TC[20, 1] packet carrying the requested parameter IDs
* @return None (messages are stored using storeMessage())
- *
- * Everything apart from the setting data is uint16 (setting data are uint32 for now)
*/
void reportParameters(Message& paramIds);
/**
* This function receives a TC[20, 3] message and after checking whether its type is correct,
* iterates over all contained parameter IDs and replaces the settings for each valid parameter,
- * while ignoring all invalid IDs. If the manual update flag is not set, the parameter's value should
- * not change.
+ * while ignoring all invalid IDs.
*
* @param newParamValues: a valid TC[20, 3] message carrying parameter ID and replacement value
- * @return None
- *
- * @todo Use pointers for changing and storing addresses to comply with the standard
*/
void setParameters(Message& newParamValues);
diff --git a/src/Parameters/Parameters.cpp b/src/Parameters/Parameters.cpp
deleted file mode 100644
index 6505b00b..00000000
--- a/src/Parameters/Parameters.cpp
+++ /dev/null
@@ -1,3 +0,0 @@
-#include "Parameters/Parameters.hpp"
-
-SystemParameters systemParameters;
diff --git a/src/Platform/x86/Parameters/SystemParameters.cpp b/src/Platform/x86/Parameters/SystemParameters.cpp
new file mode 100644
index 00000000..b5f68de3
--- /dev/null
+++ b/src/Platform/x86/Parameters/SystemParameters.cpp
@@ -0,0 +1,3 @@
+#include "Parameters/SystemParameters.hpp"
+
+SystemParameters systemParameters;
diff --git a/src/Services/ParameterService.cpp b/src/Services/ParameterService.cpp
index 264b8da5..a9a395eb 100644
--- a/src/Services/ParameterService.cpp
+++ b/src/Services/ParameterService.cpp
@@ -6,9 +6,8 @@
void ParameterService::reportParameters(Message& paramIds) {
// TM[20,2]
- Message reqParam(20, 2, Message::TM, 1);
+ Message parameterReport(20, 2, Message::TM, 1);
- paramIds.resetRead();
ErrorHandler::assertRequest(paramIds.packetType == Message::TC, paramIds,
ErrorHandler::AcceptanceErrorType::UnacceptableMessage);
ErrorHandler::assertRequest(paramIds.messageType == 1, paramIds,
@@ -17,21 +16,26 @@ void ParameterService::reportParameters(Message& paramIds) {
ErrorHandler::AcceptanceErrorType::UnacceptableMessage);
uint16_t numOfIds = paramIds.readUint16();
- uint16_t validIds = 0;
- reqParam.appendUint16(validIds); // Shouldn't you count first the actually fucking valid ids?
-
+ uint16_t numberOfValidIds = 0;
+ for (uint16_t counter = 0; counter < numOfIds; counter++) {
+ if (paramIds.readUint16() < systemParameters.parametersArray.size()) {
+ numberOfValidIds++;
+ }
+ }
+ parameterReport.appendUint16(numberOfValidIds);
+ paramIds.resetRead();
+ numOfIds = paramIds.readUint16();
for (uint16_t i = 0; i < numOfIds; i++) {
uint16_t currId = paramIds.readUint16();
if (currId < systemParameters.parametersArray.size()) {
- reqParam.appendUint16(currId);
- systemParameters.parametersArray[currId].get().appendValueToMessage(reqParam);
- validIds++;
+ parameterReport.appendUint16(currId);
+ systemParameters.parametersArray[currId].get().appendValueToMessage(parameterReport);
} else {
- ErrorHandler::reportInternalError(ErrorHandler::GetNonExistingParameter);
+ ErrorHandler::reportError(paramIds, ErrorHandler::GetNonExistingParameter);
}
}
- storeMessage(reqParam);
+ storeMessage(parameterReport);
}
void ParameterService::setParameters(Message& newParamValues) {
@@ -50,7 +54,8 @@ void ParameterService::setParameters(Message& newParamValues) {
if (currId < systemParameters.parametersArray.size()) {
systemParameters.parametersArray[currId].get().setValueFromMessage(newParamValues);
} else {
- ErrorHandler::reportInternalError(ErrorHandler::SetNonExistingParameter);
+ ErrorHandler::reportError(newParamValues, ErrorHandler::InvalidParameterId);
+ break;
}
}
}
diff --git a/test/Services/Parameter.cpp b/test/Services/Parameter.cpp
new file mode 100644
index 00000000..12938fa0
--- /dev/null
+++ b/test/Services/Parameter.cpp
@@ -0,0 +1,10 @@
+#include "catch2/catch.hpp"
+#include "Services/ParameterService.hpp"
+#include "Services/Parameter.hpp"
+#include "Message.hpp"
+#include "ServiceTests.hpp"
+
+
+TEST_CASE("Parameter Functions") {
+
+}
\ No newline at end of file
diff --git a/test/Services/ParameterService.cpp b/test/Services/ParameterService.cpp
index b31c3418..71aa55ed 100644
--- a/test/Services/ParameterService.cpp
+++ b/test/Services/ParameterService.cpp
@@ -1,10 +1,7 @@
#include "catch2/catch.hpp"
-#include "Services/ParameterService.hpp"
#include "Message.hpp"
#include "ServiceTests.hpp"
-ParameterService& pserv = Services.parameterManagement;
-
TEST_CASE("Parameter Report Subservice") {
SECTION("All requested parameters invalid") {
Message request = Message(20, 1, Message::TC, 1);
@@ -15,11 +12,11 @@ TEST_CASE("Parameter Report Subservice") {
MessageParser::execute(request);
CHECK(ServiceTests::get(0).serviceType == 1);
- CHECK(ServiceTests::get(0).messageType == 4);
+ CHECK(ServiceTests::get(0).messageType == 2);
CHECK(ServiceTests::get(1).serviceType == 1);
- CHECK(ServiceTests::get(1).messageType == 4);
+ CHECK(ServiceTests::get(1).messageType == 2);
CHECK(ServiceTests::get(2).serviceType == 1);
- CHECK(ServiceTests::get(2).messageType == 4);
+ CHECK(ServiceTests::get(2).messageType == 2);
Message report = ServiceTests::get(3);
CHECK(report.serviceType == 20);
@@ -30,42 +27,52 @@ TEST_CASE("Parameter Report Subservice") {
Services.reset();
}
- SECTION("Faulty instruction handling") {
- auto param0 = Parameter<uint16_t>(1);
- auto param1 = Parameter<uint16_t>(12);
- auto param2 = Parameter<uint16_t>(3);
- systemParameters.parametersArray.push_back(param0);
- systemParameters.parametersArray.push_back(param1);
- systemParameters.parametersArray.push_back(param2);
-
- Message request(20, 1, Message::TC, 1);
- request.appendUint16(2); // number of requested IDs
- request.appendUint16(65535); // invalid ID in this context
- request.appendUint16(1); // valid
+ SECTION("Some requested parameters invalid") {
+ Message request = Message(20, 1, Message::TC, 1);
+ request.appendUint16(3);
+ request.appendUint16(1);
+ request.appendUint16(2);
+ request.appendUint16(10000);
MessageParser::execute(request);
- Message report = ServiceTests::get(1);
-
- CHECK(ServiceTests::get(0).messageType == 4);
CHECK(ServiceTests::get(0).serviceType == 1);
- // check for an ST[1,4] message caused by the faulty ID
- CHECK((ServiceTests::thrownError(ErrorHandler::ExecutionStartErrorType::UnknownExecutionStartError)));
- // check for the thrown UnknownExecutionStartError
+ CHECK(ServiceTests::get(0).messageType == 2);
- CHECK(report.messageType == 2);
+ Message report = ServiceTests::get(1);
CHECK(report.serviceType == 20);
- // check for an ST[20,2] message (the one that contains the settings)
+ CHECK(report.messageType == 2);
+ CHECK(report.readUint16() == 2);
+ CHECK(report.readUint16() == 1);
+ CHECK(report.readUint16() == 7);
+ CHECK(report.readUint16() == 2);
+ CHECK(report.readUint32() == 10);
+
+ ServiceTests::reset();
+ Services.reset();
+ }
+
+ SECTION("Parameters are of different types") {
+ Message request = Message(20, 1, Message::TC, 1);
+ request.appendUint16(3);
+ request.appendUint16(0);
+ request.appendUint16(1);
+ request.appendUint16(2);
- CHECK(report.readUint16() == 1); // only one parameter shall be contained
+ MessageParser::execute(request);
- CHECK(report.readUint16() == 1); // check for parameter ID
- uint8_t data[ECSS_ST_20_MAX_STRING_LENGTH];
- report.readString(data, ECSS_ST_20_MAX_STRING_LENGTH);
- String<ECSS_ST_20_MAX_STRING_LENGTH> str = String<ECSS_ST_20_MAX_STRING_LENGTH>(data);
- CHECK(str.compare("12")); // check for value as string (defined when adding parameters)
+ Message report = ServiceTests::get(0);
+ CHECK(report.serviceType == 20);
+ CHECK(report.messageType == 2);
+ CHECK(report.readUint16() == 3);
+ CHECK(report.readUint16() == 0);
+ CHECK(report.readUint8() == 3);
+ CHECK(report.readUint16() == 1);
+ CHECK(report.readUint16() == 7);
+ CHECK(report.readUint16() == 2);
+ CHECK(report.readUint32() == 10);
- ServiceTests::reset(); // clear all errors
- Services.reset(); // reset the services
+ ServiceTests::reset();
+ Services.reset();
}
SECTION("Wrong Message Type Handling Test") {
@@ -80,68 +87,109 @@ TEST_CASE("Parameter Report Subservice") {
}
TEST_CASE("Parameter Setting Subservice") {
- SECTION("Faulty Instruction Handling Test") {
- auto param0 = Parameter<uint16_t>(1);
- auto param1 = Parameter<uint16_t>(12);
- auto param2 = Parameter<uint16_t>(3);
- systemParameters.parametersArray.push_back(param0);
- systemParameters.parametersArray.push_back(param1);
- systemParameters.parametersArray.push_back(param2);
-
- Message setRequest(20, 3, Message::TC, 1);
- setRequest.appendUint16(2); // total number of IDs
- setRequest.appendUint16(1); // correct ID in this context
- setRequest.appendUint32(3735928559); // 0xDEADBEEF in hex (new setting)
- setRequest.appendUint16(65535); // faulty ID
- setRequest.appendUint32(3131746989); // 0xBAAAAAAD (this shouldn't be found in the report)
-
- MessageParser::execute(setRequest);
+ SECTION("All parameter ids are invalid") {
+ Message request = Message(20, 3, Message::TC, 1);
+ request.appendUint16(3);
+ request.appendUint16(54432);
+ request.appendUint16(1);
+ request.appendUint16(60000);
+ request.appendUint16(1);
+ request.appendUint16(65534);
+ request.appendUint16(1);
+ MessageParser::execute(request);
CHECK(ServiceTests::get(0).serviceType == 1);
- CHECK(ServiceTests::get(0).messageType == 4);
+ CHECK(ServiceTests::get(0).messageType == 2); // Just one error, because it should break after on invalid param
- Message reportRequest(20, 1, Message::TC, 1);
- reportRequest.appendUint16(1);
- reportRequest.appendUint16(1); // the changed parameter has ID 1
+ CHECK(systemParameters.parameter1.getValue() == 3);
+ CHECK(systemParameters.parameter2.getValue() == 7);
+ CHECK(systemParameters.parameter3.getValue() == 10);
- MessageParser::execute(reportRequest);
- Message report = ServiceTests::get(1);
- CHECK(report.serviceType == 20);
- CHECK(report.messageType == 2);
- CHECK(report.readUint16() == 1); // only 1 ID contained
- CHECK(report.readUint16() == 1); // contained ID should be ID 1
+ ServiceTests::reset();
+ Services.reset();
+ }
+
+ SECTION("The last parameter ids are invalid") {
+ Message request = Message(20, 3, Message::TC, 1);
+ request.appendUint16(3);
+ request.appendUint16(0);
+ request.appendUint8(1);
+ request.appendUint16(1);
+ request.appendUint16(2);
+ request.appendUint16(65534);
+ request.appendUint16(1);
+
+ MessageParser::execute(request);
+ CHECK(ServiceTests::get(0).serviceType == 1);
+ CHECK(ServiceTests::get(0).messageType == 2);
+
+ CHECK(systemParameters.parameter1.getValue() == 1);
+ CHECK(systemParameters.parameter2.getValue() == 2);
+ CHECK(systemParameters.parameter3.getValue() == 10);
- char data[ECSS_ST_20_MAX_STRING_LENGTH];
- report.readString(data, ECSS_ST_20_MAX_STRING_LENGTH);
- String<ECSS_ST_20_MAX_STRING_LENGTH> str = String<ECSS_ST_20_MAX_STRING_LENGTH>(data);
- CHECK(str.compare("3735928559")); // whose value is the string 0xDEADBEEF
+ systemParameters.parameter1.setValue(3);
+ systemParameters.parameter2.setValue(7);
+ systemParameters.parameter3.setValue(10);
ServiceTests::reset();
Services.reset();
}
- SECTION("Attempt to set parameter with no manual update availability") {
- auto param1 = Parameter<uint16_t>(12);
- systemParameters.parametersArray.push_back(param1);
+ SECTION("The last parameter ids are invalid") {
+ Message request = Message(20, 3, Message::TC, 1);
+ request.appendUint16(3);
+ request.appendUint16(0);
+ request.appendUint8(1);
+ request.appendUint16(65534);
+ request.appendUint16(1);
+ request.appendUint16(2);
+ request.appendUint16(3);
- Message setRequest = Message(20, 3, Message::TC, 1);
- setRequest.appendUint16(1);
- setRequest.appendUint16(1);
- setRequest.appendUint32(0xBAAAAAAD);
+ MessageParser::execute(request);
+ CHECK(ServiceTests::get(0).serviceType == 1);
+ CHECK(ServiceTests::get(0).messageType == 2);
- MessageParser::execute(setRequest);
+ CHECK(systemParameters.parameter1.getValue() == 1);
+ CHECK(systemParameters.parameter2.getValue() == 7);
+ CHECK(systemParameters.parameter3.getValue() == 10);
- Message infoRequest = Message(20, 1, Message::TC, 1);
- infoRequest.appendUint16(1);
- infoRequest.appendUint16(1);
+ systemParameters.parameter1.setValue(3);
+ systemParameters.parameter2.setValue(7);
+ systemParameters.parameter3.setValue(10);
- MessageParser::execute(infoRequest);
+ ServiceTests::reset();
+ Services.reset();
+ }
- Message report = ServiceTests::get(0);
+ SECTION("All ids are valid") {
+ Message request = Message(20, 3, Message::TC, 1);
+ request.appendUint16(3);
+ request.appendUint16(0);
+ request.appendUint8(1);
+ request.appendUint16(1);
+ request.appendUint16(2);
+ request.appendUint16(2);
+ request.appendUint32(3);
- CHECK(report.readUint16() == 1);
- CHECK(report.readUint16() == 1);
- CHECK_FALSE(report.readUint32() == 0xBAAAAAAD);
+ MessageParser::execute(request);
+
+ CHECK(systemParameters.parameter1.getValue() == 1);
+ CHECK(systemParameters.parameter2.getValue() == 2);
+ CHECK(systemParameters.parameter3.getValue() == 3);
+
+ systemParameters.parameter1.setValue(3);
+ systemParameters.parameter2.setValue(7);
+ systemParameters.parameter3.setValue(10);
+
+ ServiceTests::reset();
+ Services.reset();
+ }
+
+ SECTION("Wrong Message Type Handling Test") {
+ Message falseRequest(62, 3, Message::TM, 1); // a totally wrong message
+
+ MessageParser::execute(falseRequest);
+ CHECK(ServiceTests::thrownError(ErrorHandler::InternalErrorType::OtherMessageType));
ServiceTests::reset();
Services.reset();
--
GitLab