From 81270f99e55ca7092acb60291ce59783f4fb5d97 Mon Sep 17 00:00:00 2001
From: Grigoris Pavlakis <grigpavl@ece.auth.gr>
Date: Sun, 18 Aug 2019 00:12:53 +0300
Subject: [PATCH] Fix a bug in reportParameterIds() causing it to report the
IDs with an off-by-1 error
Additional changes:
Fixed tests for parameter setting to accommodate the new way of parameter storage
---
src/Services/ParameterService.cpp | 15 +++--
test/Services/ParameterService.cpp | 104 +++++++++++++++++------------
2 files changed, 71 insertions(+), 48 deletions(-)
diff --git a/src/Services/ParameterService.cpp b/src/Services/ParameterService.cpp
index 22098471..880c8c3d 100644
--- a/src/Services/ParameterService.cpp
+++ b/src/Services/ParameterService.cpp
@@ -44,7 +44,7 @@ void ParameterService::reportParameterIds(Message& paramIds) {
for (uint16_t i = 0; i < numOfIds; i++) {
uint16_t currId = paramIds.readUint16();
try {
- std::pair<ValueType, uint16_t> p = std::make_pair(i, paramsList.at(currId).getCurrentValue());
+ std::pair<uint16_t, ValueType> p = std::make_pair(currId, paramsList.at(currId).getCurrentValue());
// pair containing the parameter's ID as first element and its current value as second
validParams.push_back(p);
validIds++;
@@ -66,7 +66,7 @@ void ParameterService::reportParameterIds(Message& paramIds) {
}
void ParameterService::setParameterIds(Message& newParamValues) {
- newParamValues.assertTC(20, 3);
+ //newParamValues.assertTC(20, 3);
// assertion: correct message, packet and service type (at failure throws an
// InternalError::UnacceptablePacket which gets logged)
@@ -83,12 +83,13 @@ void ParameterService::setParameterIds(Message& newParamValues) {
for (uint16_t i = 0; i < numOfIds; i++) {
uint16_t currId = newParamValues.readUint16();
- if (paramsList.find(currId) != paramsList.end()) {
- paramsList[currId].setCurrentValue(newParamValues.readUint32()); // TODO: add a check here with the new
- // flag functionality
- } else {
+ // TODO: add a check here with the new flag functionality
+ try {
+ paramsList.at(currId).setCurrentValue(newParamValues.readUint32());
+ }
+ catch (etl::map_out_of_bounds &mapOutOfBounds) {
ErrorHandler::reportError(newParamValues,
- ErrorHandler::ExecutionStartErrorType::UnknownExecutionStartError);
+ ErrorHandler::ExecutionStartErrorType::UnknownExecutionStartError);
continue; // generate failed start of execution notification & ignore
}
}
diff --git a/test/Services/ParameterService.cpp b/test/Services/ParameterService.cpp
index 9f693718..a44f800f 100644
--- a/test/Services/ParameterService.cpp
+++ b/test/Services/ParameterService.cpp
@@ -17,11 +17,11 @@ void foo(ValueType* bar) { // sample function
TEST_CASE("Parameter Service - General") {
SECTION("Addition to full map") {
- pserv.addNewParameter(3, 14); // this one has ID 1
- pserv.addNewParameter(1, 7, 12); // this one has 2
- pserv.addNewParameter(4, 12, 3, nullptr); // this one has 3
- pserv.addNewParameter(12, 3, 6, &foo); // this one has 4
- pserv.addNewParameter(15, 7, 3, &foo); //and this one 5
+ pserv.addNewParameter(3, 14); // this one has ID 0
+ pserv.addNewParameter(1, 7, 12); // this one has 1
+ pserv.addNewParameter(4, 12, 3, nullptr); // this one has 2
+ pserv.addNewParameter(12, 3, 6, &foo); // this one has 3
+ pserv.addNewParameter(15, 7, 3, &foo); //and this one 4
REQUIRE_FALSE(pserv.addNewParameter(15, 5, 4)); // addNewParameter should return false
Services.reset(); // reset all services
@@ -36,14 +36,14 @@ TEST_CASE("Parameter Service - General") {
}
TEST_CASE("Parameter Report Subservice") {
-// SECTION("Empty parameter report") {
+// SECTION("All requested parameters invalid") {
//
// }
SECTION("Faulty instruction handling") {
- pserv.addNewParameter(3, 14); // ID 1
- pserv.addNewParameter(1, 7, 12); // ID 2
- pserv.addNewParameter(4, 12, 3, nullptr); // ID 3
+ pserv.addNewParameter(3, 14); // ID 0
+ pserv.addNewParameter(1, 7, 12); // ID 1
+ pserv.addNewParameter(4, 12, 3, nullptr); // ID 2
Message request(20, 1, Message::TC, 1);
request.appendUint16(2); // number of requested IDs
@@ -86,49 +86,71 @@ TEST_CASE("Parameter Report Subservice") {
TEST_CASE("Parameter Setting Subservice") {
SECTION("Faulty Instruction Handling Test") {
- Message setRequest(20, 3, Message::TC, 1);
- Message reportRequest(20, 1, Message::TC, 1);
+ pserv.addNewParameter(3, 14); // ID 0
+ pserv.addNewParameter(1, 7, 12); // ID 1
+ pserv.addNewParameter(4, 12, 3, nullptr); // ID 2
+ 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(16742); // faulty ID in this context
+ setRequest.appendUint16(65535); // faulty ID
setRequest.appendUint32(3131746989); // 0xBAAAAAAD (this shouldn't be found in the report)
- reportRequest.appendUint16(2);
- reportRequest.appendUint16(16742);
- reportRequest.appendUint16(1); // used to be 3, which pointed the bug with
- // numOfValidIds out, now is 1 in order to be a valid ID (a separate test for
- // numOfValidIds shall be introduced)
+ MessageParser::execute(setRequest);
- // Since every reporting and setting is called with the same (sometimes faulty) parameters,
- // and there are errors generated (as should be) it is important to catch and check for
- // them in order to preserve the integrity of the test.
- MessageParser::execute(reportRequest);
- Message errorNotif1 = ServiceTests::get(0);
- CHECK(errorNotif1.messageType == 4);
- CHECK(errorNotif1.serviceType == 1);
- Message before = ServiceTests::get(1);
+ CHECK(ServiceTests::get(0).serviceType == 1);
+ CHECK(ServiceTests::get(0).messageType == 4);
- MessageParser::execute(setRequest);
- Message errorNotif2 = ServiceTests::get(2);
- CHECK(errorNotif2.messageType == 4);
- CHECK(errorNotif2.serviceType == 1);
+ Message reportRequest(20, 1, Message::TC, 1);
+ reportRequest.appendUint16(1);
+ reportRequest.appendUint16(1); // the changed parameter has ID 1
MessageParser::execute(reportRequest);
- Message errorNotif3 = ServiceTests::get(3);
- CHECK(errorNotif3.messageType == 4);
- CHECK(errorNotif3.serviceType == 1);
-
- Message after = ServiceTests::get(4);
-
- before.readUint16();
- after.readUint16(); // skip the number of IDs
+ 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
+ CHECK(report.readUint32() == 3735928559); // whose value is 0xDEADBEEF
- while (after.readPosition <= after.dataSize) {
- CHECK(before.readUint16() == after.readUint16()); // check if all IDs are present
- CHECK_FALSE(after.readUint32() == 0xBAAAAAAD); // fail if any settings are BAAAAAAD :P
- }
+ ServiceTests::reset();
+ Services.reset();
+//
+// reportRequest.appendUint16(2);
+// reportRequest.appendUint16(16742);
+// reportRequest.appendUint16(1); // used to be 3, which pointed the bug with
+// // numOfValidIds out, now is 1 in order to be a valid ID (a separate test for
+// // numOfValidIds shall be introduced)
+//
+// // Since every reporting and setting is called with the same (sometimes faulty) parameters,
+// // and there are errors generated (as should be) it is important to catch and check for
+// // them in order to preserve the integrity of the test.
+// Message errorNotif1 = ServiceTests::get(0);
+// CHECK(errorNotif1.messageType == 4);
+// CHECK(errorNotif1.serviceType == 1);
+//
+// Message before = ServiceTests::get(1);
+//
+// MessageParser::execute(setRequest);
+// Message errorNotif2 = ServiceTests::get(2);
+// CHECK(errorNotif2.messageType == 4);
+// CHECK(errorNotif2.serviceType == 1);
+//
+// MessageParser::execute(reportRequest);
+// Message errorNotif3 = ServiceTests::get(3);
+// CHECK(errorNotif3.messageType == 4);
+// CHECK(errorNotif3.serviceType == 1);
+//
+// Message after = ServiceTests::get(4);
+//
+// before.readUint16();
+// after.readUint16(); // skip the number of IDs
+//
+// while (after.readPosition <= after.dataSize) {
+// CHECK(before.readUint16() == after.readUint16()); // check if all IDs are present
+// CHECK_FALSE(after.readUint32() == 0xBAAAAAAD); // fail if any settings are BAAAAAAD :P
+// }
}
}
--
GitLab