From 227498399004e14110eb2f15a111ed704877c585 Mon Sep 17 00:00:00 2001
From: Grigoris Pavlakis <grigpavl@ece.auth.gr>
Date: Sun, 18 Aug 2019 20:55:39 +0300
Subject: [PATCH] Add safeguard for overwriting a parameter using an
already-existing ID
---
inc/Services/ParameterService.hpp | 18 ++++++++++--------
src/Services/ParameterService.cpp | 22 ++++++++++++++--------
test/Services/ParameterService.cpp | 10 +++++++---
3 files changed, 31 insertions(+), 19 deletions(-)
diff --git a/inc/Services/ParameterService.hpp b/inc/Services/ParameterService.hpp
index db4ed28d..0338d76d 100644
--- a/inc/Services/ParameterService.hpp
+++ b/inc/Services/ParameterService.hpp
@@ -9,7 +9,6 @@
// Number of stored parameters. MAX_PARAMS is just a dummy number for now.
#define MAX_PARAMS 5
-// TODO: 1) Rework the parameter setting and report functions
// TODO: 2) Implement flags and use them above
// TODO: 3) Write more and better tests
// TODO: 4) Make sure that docs are up to date
@@ -28,7 +27,7 @@
* for parameter reporting and modification.
*
* The parameter list is stored in a map with the parameter IDs as keys and values
- * corresponding Parameter structs containing the PTC, PFC and the parameter's value.
+ * corresponding Parameter classes containing the PTC, PFC and the parameter's value.
*/
@@ -38,16 +37,18 @@ private:
public:
/**
- * Initializes the parameter list.
+ * @brief Initializes the parameter list.
*/
ParameterService();
/**
- * Adds a new parameter. If the parameter has not been added (either because the map is full or because it already
- * exists in it) then returns false.
- * The parameter IDs are given sequentially, starting from 0.
+ * @brief Adds a new parameter. Returns false if the parameter has not been added
+ * (either because the map is full or because it already exists in it).
+ * @param id: the desired ID for this parameter
+ * @param param: the parameter field to be included
+ * @param flags: the flags to be set for this field (see Parameter.hpp)
*/
- bool addNewParameter(uint8_t id, Parameter param, const char* flags = "110");
+ bool addNewParameter(uint16_t id, Parameter param, const char* flags = "110");
/**
* This function receives a TC[20, 1] packet and returns a TM[20, 2] packet
@@ -70,7 +71,8 @@ public:
/**
* 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.
+ * while ignoring all invalid IDs. If the manual update flag is not set, the parameter's value should
+ * not change.
*
* @param newParamValues: a valid TC[20, 3] message carrying parameter ID and replacement value
* @return None
diff --git a/src/Services/ParameterService.cpp b/src/Services/ParameterService.cpp
index f79d99b3..ff178411 100644
--- a/src/Services/ParameterService.cpp
+++ b/src/Services/ParameterService.cpp
@@ -7,21 +7,29 @@ ParameterService::ParameterService() {
// addNewParameter(3, 14);
}
-bool ParameterService::addNewParameter(uint8_t id, Parameter param, const char* flags) {
+bool ParameterService::addNewParameter(uint16_t id, Parameter param, const char* flags) {
try {
- // second element of the returned std::pair is whether the given item was inserted or not
- param.setFlag(flags);
- paramsList.insert(std::make_pair(id, param));
- return true;
+ try {
+ paramsList.at(id);
+ return false;
+ // if it exists, an iterator will be returned with no exception thrown,
+ // so the function should return false
+ // todo: is it a better idea to use find() and if instead of an exception here?
+ }
+ catch (etl::map_out_of_bounds& mapOutOfBounds) {
+ param.setFlag(flags);
+ paramsList.insert(std::make_pair(id, param));
+ return true;
+ }
}
catch (etl::map_full &mapFull) {
return false;
+ // if the map is full, return false
}
}
void ParameterService::reportParameterIds(Message& paramIds) {
etl::vector<std::pair<uint16_t, ValueType>, MAX_PARAMS> validParams;
-// paramIds.assertTC(20, 1);
Message reqParam(20, 2, Message::TM, 1);
// empty TM[20, 2] parameter report message
@@ -66,8 +74,6 @@ void ParameterService::reportParameterIds(Message& paramIds) {
}
void ParameterService::setParameterIds(Message& newParamValues) {
- //newParamValues.assertTC(20, 3);
-
// assertion: correct message, packet and service type (at failure throws an
// InternalError::UnacceptablePacket which gets logged)
diff --git a/test/Services/ParameterService.cpp b/test/Services/ParameterService.cpp
index 57e5a1c2..65bb50b3 100644
--- a/test/Services/ParameterService.cpp
+++ b/test/Services/ParameterService.cpp
@@ -36,9 +36,13 @@ TEST_CASE("Parameter Service - General") {
Services.reset(); // reset all services
}
-// SECTION("Addition of already existing parameter") {
-//
-// }
+ SECTION("Addition of already existing parameter") {
+ Parameter param0 = Parameter(1, 3);
+ pserv.addNewParameter(0, param0);
+
+ CHECK_FALSE(pserv.addNewParameter(0, param0));
+ Services.reset();
+ }
//SECTION("Passing of null-pointer as update function on construction")
--
GitLab