Skip to content
Snippets Groups Projects
Unverified Commit 81270f99 authored by Grigoris Pavlakis's avatar Grigoris Pavlakis
Browse files

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
parent 0e657a94
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/Services/ParameterService.cpp
+ 8
− 7
View file @ 81270f99
......@@ -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
}
}
......
test/Services/ParameterService.cpp
+ 63
− 41
View file @ 81270f99
......@@ -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
// }
}
}
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment