Skip to content

Commit

Permalink
[DDOP]: Add validation for DDOP object child references
Browse files Browse the repository at this point in the history 
Added validation of DET child references' object types.
Added sanity checks on other objects' child types even though those are
essentially forced to be correct.
Reworked a couple things in the unit tests to prevent some tests from
interfering with other tests specifically when not using CTest.
  • Loading branch information
@ad3154
ad3154 committed Sep 6, 2023
1 parent 1cfe565 commit 3806256
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 13 deletions.
5 changes: 0 additions & 5 deletions examples/task_controller_client/section_control_implement_sim.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,6 @@ bool SectionControlImplementSimulator::create_ddop(std::shared_ptr<isobus::Devic
sectionCounter += NUMBER_SECTIONS_PER_CONDENSED_MESSAGE;
}

for (std::uint_fast8_t i = 0; i < get_number_of_sections(); i++)
{
boom->add_reference_to_child_object(static_cast<std::uint16_t>(ImplementDDOPObjectIDs::Section1) + i);
}

product->add_reference_to_child_object(static_cast<std::uint16_t>(ImplementDDOPObjectIDs::TankCapacity));
product->add_reference_to_child_object(static_cast<std::uint16_t>(ImplementDDOPObjectIDs::TankVolume));
product->add_reference_to_child_object(static_cast<std::uint16_t>(ImplementDDOPObjectIDs::LifetimeApplicationVolumeTotal));
Expand Down
28 changes: 28 additions & 0 deletions isobus/src/isobus_device_descriptor_object_pool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,16 @@ namespace isobus
retVal = false;
break;
}
else if ((task_controller_object::ObjectTypes::DeviceProcessData != child->get_object_type()) &&
(task_controller_object::ObjectTypes::DeviceProperty != child->get_object_type()))
{
CANStackLogger::error("[DDOP]: Object %u has child %u which is an object type that is not allowed.",
currentDeviceElement->get_child_object_id(i),
child->get_object_id());
CANStackLogger::error("[DDOP]: A DET object may only have DPD and DPT children.");
retVal = false;
break;
}
}
}
}
Expand All @@ -963,6 +973,15 @@ namespace isobus
retVal = false;
break;
}
else if (task_controller_object::ObjectTypes::DeviceValuePresentation != child->get_object_type())
{
CANStackLogger::error("[DDOP]: Object %u has a child %u with an object type that is not allowed." +
currentProcessData->get_device_value_presentation_object_id(),
child->get_object_id());
CANStackLogger::error("[DDOP]: A DPD object may only have DVP children.");
retVal = false;
break;
}
}
}
break;
Expand All @@ -982,6 +1001,15 @@ namespace isobus
retVal = false;
break;
}
else if (task_controller_object::ObjectTypes::DeviceValuePresentation != child->get_object_type())
{
CANStackLogger::error("[DDOP]: Object %u has a child %u with an object type that is not allowed." +
currentProperty->get_device_value_presentation_object_id(),
child->get_object_id());
CANStackLogger::error("[DDOP]: A DPT object may only have DVP children.");
retVal = false;
break;
}
}
}
break;
Expand Down
16 changes: 9 additions & 7 deletions test/core_network_management_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,11 @@ TEST(CORE_TESTS, SimilarControlFunctions)
testFrame.isExtendedFrame = true;
CANNetworkManager::CANNetwork.update();

// Make a partner that is a task controller
const isobus::NAMEFilter filterTaskController(isobus::NAME::NAMEParameters::FunctionCode, static_cast<std::uint8_t>(isobus::NAME::Function::TaskController));
const std::vector<isobus::NAMEFilter> tcNameFilters = { filterTaskController };
auto TestPartnerTC = isobus::PartneredControlFunction::create(0, tcNameFilters);
// Make a partner that is a fuel system
// Using a less common function to avoid interfering with other tests when not running under CTest
const isobus::NAMEFilter filterFuelSystem(isobus::NAME::NAMEParameters::FunctionCode, static_cast<std::uint8_t>(isobus::NAME::Function::FuelSystem));
const std::vector<isobus::NAMEFilter> nameFilters = { filterFuelSystem };
auto TestPartner = isobus::PartneredControlFunction::create(0, nameFilters);

// Request the address claim PGN
const auto PGN = static_cast<std::uint32_t>(CANLibParameterGroupNumber::AddressClaim);
Expand All @@ -301,7 +302,7 @@ TEST(CORE_TESTS, SimilarControlFunctions)
std::this_thread::sleep_for(std::chrono::milliseconds(15));
CANNetworkManager::CANNetwork.update();

std::uint64_t rawNAME = 0xa00082000425e9f8;
std::uint64_t rawNAME = 0xa0000F000425e9f8;

// Force claim some kind of TC
testFrame.identifier = 0x18EEFF7A;
Expand All @@ -318,7 +319,7 @@ TEST(CORE_TESTS, SimilarControlFunctions)
CANNetworkManager::CANNetwork.update();

// Partner should be valid with that same NAME
EXPECT_EQ(TestPartnerTC->get_NAME().get_full_name(), rawNAME);
EXPECT_EQ(TestPartner->get_NAME().get_full_name(), rawNAME);

// Now, claim something else that matches a TC. The original partner should remain the same.
auto testOtherTCNAME = NAME(rawNAME);
Expand All @@ -339,5 +340,6 @@ TEST(CORE_TESTS, SimilarControlFunctions)
CANNetworkManager::CANNetwork.update();

// Partner should never change
EXPECT_EQ(TestPartnerTC->get_NAME().get_full_name(), 0xa00082000425e9f8);
EXPECT_EQ(TestPartner->get_NAME().get_full_name(), 0xa0000F000425e9f8);
EXPECT_TRUE(TestPartner->destroy());
}
10 changes: 10 additions & 0 deletions test/ddop_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,16 @@ TEST(DDOP_TESTS, DeviceElementDesignatorTests)
EXPECT_EQ(1, objectUnderTest->get_number_child_objects());
objectUnderTest->remove_reference_to_child_object(111);
EXPECT_EQ(0, objectUnderTest->get_number_child_objects());

// Test that invalid child objects are rejected
DeviceDescriptorObjectPool testDDOPWithBadChildren(3);
EXPECT_TRUE(testDDOPWithBadChildren.add_device("AgIsoStack++ UnitTest", "1.0.0", "123", "I++1.0", testLanguageInterface.get_localization_raw_data(), std::vector<std::uint8_t>(), 0));
EXPECT_TRUE(testDDOPWithBadChildren.add_device_element("Sprayer", static_cast<std::uint16_t>(SprayerDDOPObjectIDs::MainDeviceElement), 0, task_controller_object::DeviceElementObject::Type::Device, static_cast<std::uint16_t>(SprayerDDOPObjectIDs::MainDeviceElement)));
EXPECT_TRUE(testDDOPWithBadChildren.add_device_element("Junk Element 1", static_cast<std::uint16_t>(SprayerDDOPObjectIDs::MainDeviceElement), 0, task_controller_object::DeviceElementObject::Type::Function, 250));
EXPECT_TRUE(testDDOPWithBadChildren.generate_binary_object_pool(binaryDDOP));
objectUnderTest = std::static_pointer_cast<task_controller_object::DeviceElementObject>(testDDOPWithBadChildren.get_object_by_id(static_cast<std::uint16_t>(SprayerDDOPObjectIDs::MainDeviceElement)));
objectUnderTest->add_reference_to_child_object(250); // Set child as a DET, which is not allowed
EXPECT_FALSE(testDDOPWithBadChildren.generate_binary_object_pool(binaryDDOP));
}

TEST(DDOP_TESTS, ProcessDataTests)
Expand Down
3 changes: 2 additions & 1 deletion test/tc_client_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1435,7 +1435,8 @@ TEST(TASK_CONTROLLER_CLIENT_TESTS, CallbackTests)
clientNAME.set_function_code(static_cast<std::uint8_t>(NAME::Function::RateControl));
auto internalECU = InternalControlFunction::create(clientNAME, 0x86, 0);
const isobus::NAMEFilter filterTaskController(isobus::NAME::NAMEParameters::FunctionCode, static_cast<std::uint8_t>(isobus::NAME::Function::TaskController));
const std::vector<isobus::NAMEFilter> tcNameFilters = { filterTaskController };
const isobus::NAMEFilter filterTaskControllerIdentity(isobus::NAME::NAMEParameters::IdentityNumber, 0x405);
const std::vector<isobus::NAMEFilter> tcNameFilters = { filterTaskController, filterTaskControllerIdentity };
auto TestPartnerTC = isobus::PartneredControlFunction::create(0, tcNameFilters);
auto blankDDOP = std::make_shared<DeviceDescriptorObjectPool>();

Expand Down

0 comments on commit 3806256

Please sign in to comment.