From 6a4f3a87b9741b384700b885c0ac642d53d7c6de Mon Sep 17 00:00:00 2001 From: shrikar Date: Mon, 16 Oct 2023 18:49:49 +0000 Subject: [PATCH] Updated JsonConfigLoader to parse access for area configs If access is populated in VehicleAreaConfig, access field will remain empty in VehiclePropConfig. Updated VHAL layer tests to test new access mode granularity logic Added new tests and modified others in JsonConfigLoaderUnitTest to test new parsing logic. Added VTS test to enforce exclusively one of VehiclePropConig.access and the VehicleAreaConfig.access fields are populated. Bug: 290801790 Test: atest JsonConfigLoaderUnitTest Test: atest VtsHalAutomotiveVehicle_TargetTest Change-Id: I6cc4f20c289141123d3e2093e45084109fbf4d9d --- .../include/JsonConfigLoader.h | 15 ++- .../JsonConfigLoader/src/JsonConfigLoader.cpp | 51 +++++--- .../test/JsonConfigLoaderUnitTest.cpp | 112 +++++++++++++++++- .../VtsHalAutomotiveVehicle_TargetTest.cpp | 75 ++++++++++-- 4 files changed, 213 insertions(+), 40 deletions(-) diff --git a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/include/JsonConfigLoader.h b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/include/JsonConfigLoader.h index f3bdbd2343..82e5860780 100644 --- a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/include/JsonConfigLoader.h +++ b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/include/JsonConfigLoader.h @@ -130,12 +130,9 @@ class JsonConfigParser { std::vector* outPtr, std::vector* errors); // Parses a JSON field to VehiclePropertyAccess or VehiclePropertyChangeMode. template - void parseAccessChangeMode( - const Json::Value& parentJsonNode, const std::string& fieldName, int propId, - const std::string& propStr, - const std::unordered_map& defaultMap, - T* outPtr, std::vector* errors); + void parseAccessChangeMode(const Json::Value& parentJsonNode, const std::string& fieldName, + const std::string& propStr, const T* defaultAccessChangeModePtr, + T* outPtr, std::vector* errors); // Parses a JSON field to RawPropValues. // @@ -145,8 +142,10 @@ class JsonConfigParser { std::vector* errors); // Prase a JSON field as an array of area configs. - void parseAreas(const Json::Value& parentJsonNode, const std::string& fieldName, - ConfigDeclaration* outPtr, std::vector* errors); + void parseAreas( + const Json::Value& parentJsonNode, const std::string& fieldName, + ConfigDeclaration* outPtr, std::vector* errors, + aidl::android::hardware::automotive::vehicle::VehiclePropertyAccess defaultAccess); }; } // namespace jsonconfigloader_impl diff --git a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/src/JsonConfigLoader.cpp b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/src/JsonConfigLoader.cpp index 3e6e7dcf1d..76db891a5b 100644 --- a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/src/JsonConfigLoader.cpp +++ b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/src/JsonConfigLoader.cpp @@ -487,10 +487,11 @@ bool JsonConfigParser::tryParseJsonArrayToVariable(const Json::Value& parentJson } template -void JsonConfigParser::parseAccessChangeMode( - const Json::Value& parentJsonNode, const std::string& fieldName, int propId, - const std::string& propStr, const std::unordered_map& defaultMap, - T* outPtr, std::vector* errors) { +void JsonConfigParser::parseAccessChangeMode(const Json::Value& parentJsonNode, + const std::string& fieldName, + const std::string& propStr, + const T* defaultAccessChangeModeValuePtr, T* outPtr, + std::vector* errors) { if (!parentJsonNode.isObject()) { errors->push_back("Node: " + parentJsonNode.toStyledString() + " is not an object"); return; @@ -504,12 +505,11 @@ void JsonConfigParser::parseAccessChangeMode( *outPtr = static_cast(result.value()); return; } - auto it = defaultMap.find(static_cast(propId)); - if (it == defaultMap.end()) { + if (defaultAccessChangeModeValuePtr == NULL) { errors->push_back("No " + fieldName + " specified for property: " + propStr); return; } - *outPtr = it->second; + *outPtr = *defaultAccessChangeModeValuePtr; return; } @@ -538,7 +538,8 @@ bool JsonConfigParser::parsePropValues(const Json::Value& parentJsonNode, } void JsonConfigParser::parseAreas(const Json::Value& parentJsonNode, const std::string& fieldName, - ConfigDeclaration* config, std::vector* errors) { + ConfigDeclaration* config, std::vector* errors, + VehiclePropertyAccess defaultAccess) { if (!parentJsonNode.isObject()) { errors->push_back("Node: " + parentJsonNode.toStyledString() + " is not an object"); return; @@ -546,6 +547,7 @@ void JsonConfigParser::parseAreas(const Json::Value& parentJsonNode, const std:: if (!parentJsonNode.isMember(fieldName)) { return; } + std::string propStr = parentJsonNode["property"].toStyledString(); const Json::Value& jsonValue = parentJsonNode[fieldName]; if (!jsonValue.isArray()) { @@ -561,6 +563,8 @@ void JsonConfigParser::parseAreas(const Json::Value& parentJsonNode, const std:: } VehicleAreaConfig areaConfig = {}; areaConfig.areaId = areaId; + parseAccessChangeMode(jsonAreaConfig, "access", propStr, &defaultAccess, &areaConfig.access, + errors); tryParseJsonValueToVariable(jsonAreaConfig, "minInt32Value", /*optional=*/true, &areaConfig.minInt32Value, errors); tryParseJsonValueToVariable(jsonAreaConfig, "maxInt32Value", /*optional=*/true, @@ -608,12 +612,21 @@ std::optional JsonConfigParser::parseEachProperty( configDecl.config.prop = propId; std::string propStr = propJsonValue["property"].toStyledString(); + VehiclePropertyAccess* defaultAccessMode = NULL; + auto itAccess = AccessForVehicleProperty.find(static_cast(propId)); + if (itAccess != AccessForVehicleProperty.end()) { + defaultAccessMode = &itAccess->second; + } + VehiclePropertyChangeMode* defaultChangeMode = NULL; + auto itChangeMode = ChangeModeForVehicleProperty.find(static_cast(propId)); + if (itChangeMode != ChangeModeForVehicleProperty.end()) { + defaultChangeMode = &itChangeMode->second; + } + VehiclePropertyAccess access = VehiclePropertyAccess::NONE; + parseAccessChangeMode(propJsonValue, "access", propStr, defaultAccessMode, &access, errors); - parseAccessChangeMode(propJsonValue, "access", propId, propStr, AccessForVehicleProperty, - &configDecl.config.access, errors); - - parseAccessChangeMode(propJsonValue, "changeMode", propId, propStr, - ChangeModeForVehicleProperty, &configDecl.config.changeMode, errors); + parseAccessChangeMode(propJsonValue, "changeMode", propStr, defaultChangeMode, + &configDecl.config.changeMode, errors); tryParseJsonValueToVariable(propJsonValue, "configString", /*optional=*/true, &configDecl.config.configString, errors); @@ -629,21 +642,23 @@ std::optional JsonConfigParser::parseEachProperty( tryParseJsonValueToVariable(propJsonValue, "maxSampleRate", /*optional=*/true, &configDecl.config.maxSampleRate, errors); - parseAreas(propJsonValue, "areas", &configDecl, errors); - - if (errors->size() != initialErrorCount) { - return std::nullopt; - } + parseAreas(propJsonValue, "areas", &configDecl, errors, access); // If there is no area config, by default we allow variable update rate, so we have to add // a global area config. if (configDecl.config.areaConfigs.size() == 0) { VehicleAreaConfig areaConfig = { .areaId = 0, + .access = access, .supportVariableUpdateRate = true, }; configDecl.config.areaConfigs.push_back(std::move(areaConfig)); } + + if (errors->size() != initialErrorCount) { + return std::nullopt; + } + return configDecl; } diff --git a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/test/JsonConfigLoaderUnitTest.cpp b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/test/JsonConfigLoaderUnitTest.cpp index 98826537e7..a13d3dff4e 100644 --- a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/test/JsonConfigLoaderUnitTest.cpp +++ b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/test/JsonConfigLoaderUnitTest.cpp @@ -286,7 +286,8 @@ TEST_F(JsonConfigLoaderUnitTest, testCheckDefaultAccessChangeMode) { ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& propConfig = configs.begin()->second.config; - ASSERT_EQ(propConfig.access, VehiclePropertyAccess::READ); + ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE); + ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::READ); ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::STATIC); } @@ -307,7 +308,8 @@ TEST_F(JsonConfigLoaderUnitTest, testAccessOverride) { ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& propConfig = configs.begin()->second.config; - ASSERT_EQ(propConfig.access, VehiclePropertyAccess::WRITE); + ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE); + ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::WRITE); ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::STATIC); } @@ -328,7 +330,8 @@ TEST_F(JsonConfigLoaderUnitTest, testChangeModeOverride) { ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& propConfig = configs.begin()->second.config; - ASSERT_EQ(propConfig.access, VehiclePropertyAccess::READ); + ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE); + ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::READ); ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::ON_CHANGE); } @@ -350,7 +353,8 @@ TEST_F(JsonConfigLoaderUnitTest, testCustomProp) { ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& propConfig = configs.begin()->second.config; - ASSERT_EQ(propConfig.access, VehiclePropertyAccess::WRITE); + ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE); + ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::WRITE); ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::ON_CHANGE); } @@ -550,10 +554,12 @@ TEST_F(JsonConfigLoaderUnitTest, testAreas_Simple) { ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& config = configs.begin()->second.config; + ASSERT_EQ(config.access, VehiclePropertyAccess::NONE); ASSERT_EQ(config.areaConfigs.size(), 1u); const VehicleAreaConfig& areaConfig = config.areaConfigs[0]; ASSERT_EQ(areaConfig.minInt32Value, 1); ASSERT_EQ(areaConfig.maxInt32Value, 7); + ASSERT_EQ(areaConfig.access, VehiclePropertyAccess::READ); ASSERT_EQ(areaConfig.areaId, HVAC_ALL); } @@ -635,9 +641,11 @@ TEST_F(JsonConfigLoaderUnitTest, testAreas_HandlesNoSupportedEnumValuesDeclared) ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& config = configs.begin()->second.config; + ASSERT_EQ(config.access, VehiclePropertyAccess::NONE); ASSERT_EQ(config.areaConfigs.size(), 1u); const VehicleAreaConfig& areaConfig = config.areaConfigs[0]; + ASSERT_EQ(areaConfig.access, VehiclePropertyAccess::READ); ASSERT_EQ(areaConfig.areaId, 0); ASSERT_FALSE(areaConfig.supportedEnumValues); } @@ -662,9 +670,11 @@ TEST_F(JsonConfigLoaderUnitTest, testAreas_HandlesSupportedEnumValues) { ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& config = configs.begin()->second.config; + ASSERT_EQ(config.access, VehiclePropertyAccess::NONE); ASSERT_EQ(config.areaConfigs.size(), 1u); const VehicleAreaConfig& areaConfig = config.areaConfigs[0]; + ASSERT_EQ(areaConfig.access, VehiclePropertyAccess::READ); ASSERT_EQ(areaConfig.areaId, 0); ASSERT_TRUE(areaConfig.supportedEnumValues); ASSERT_EQ(areaConfig.supportedEnumValues.value().size(), 2u); @@ -692,13 +702,107 @@ TEST_F(JsonConfigLoaderUnitTest, testAreas_HandlesEmptySupportedEnumValues) { ASSERT_EQ(configs.size(), 1u); const VehiclePropConfig& config = configs.begin()->second.config; + ASSERT_EQ(config.access, VehiclePropertyAccess::NONE); ASSERT_EQ(config.areaConfigs.size(), 1u); const VehicleAreaConfig& areaConfig = config.areaConfigs[0]; + ASSERT_EQ(areaConfig.access, VehiclePropertyAccess::READ); ASSERT_EQ(areaConfig.areaId, 0); ASSERT_FALSE(areaConfig.supportedEnumValues); } +TEST_F(JsonConfigLoaderUnitTest, testAccess_areaOverrideGlobalDefault) { + std::istringstream iss(R"( + { + "properties": [{ + "property": "VehicleProperty::CABIN_LIGHTS_SWITCH", + "areas": [{ + "access": "VehiclePropertyAccess::READ", + "areaId": 0 + }] + }] + } + )"); + + auto result = mLoader.loadPropConfig(iss); + ASSERT_TRUE(result.ok()); + + auto configs = result.value(); + ASSERT_EQ(configs.size(), 1u); + + const VehiclePropConfig& config = configs.begin()->second.config; + ASSERT_EQ(config.access, VehiclePropertyAccess::NONE); + ASSERT_EQ(config.areaConfigs.size(), 1u); + + const VehicleAreaConfig& areaConfig = config.areaConfigs[0]; + ASSERT_EQ(areaConfig.access, VehiclePropertyAccess::READ); + ASSERT_EQ(areaConfig.areaId, 0); +} + +TEST_F(JsonConfigLoaderUnitTest, testAccess_globalOverrideDefault) { + std::istringstream iss(R"( + { + "properties": [{ + "property": "VehicleProperty::CABIN_LIGHTS_SWITCH", + "areas": [{ + "areaId": 0 + }], + "access": "VehiclePropertyAccess::READ" + }] + } + )"); + + auto result = mLoader.loadPropConfig(iss); + ASSERT_TRUE(result.ok()); + + auto configs = result.value(); + ASSERT_EQ(configs.size(), 1u); + + const VehiclePropConfig& config = configs.begin()->second.config; + ASSERT_EQ(config.access, VehiclePropertyAccess::NONE); + ASSERT_EQ(config.areaConfigs.size(), 1u); + + const VehicleAreaConfig& areaConfig = config.areaConfigs[0]; + ASSERT_EQ(areaConfig.access, VehiclePropertyAccess::READ); + ASSERT_EQ(areaConfig.areaId, 0); +} + +TEST_F(JsonConfigLoaderUnitTest, testAccess_areaOverrideGlobal) { + std::istringstream iss(R"( + { + "properties": [{ + "property": "VehicleProperty::CABIN_LIGHTS_SWITCH", + "areas": [{ + "access": "VehiclePropertyAccess::WRITE", + "areaId": 0 + }, + { + "areaId": 1 + }], + "access": "VehiclePropertyAccess::READ", + }] + } + )"); + + auto result = mLoader.loadPropConfig(iss); + ASSERT_TRUE(result.ok()); + + auto configs = result.value(); + ASSERT_EQ(configs.size(), 1u); + + const VehiclePropConfig& config = configs.begin()->second.config; + ASSERT_EQ(config.access, VehiclePropertyAccess::NONE); + ASSERT_EQ(config.areaConfigs.size(), 2u); + + const VehicleAreaConfig& areaConfig1 = config.areaConfigs[0]; + ASSERT_EQ(areaConfig1.access, VehiclePropertyAccess::WRITE); + ASSERT_EQ(areaConfig1.areaId, 0); + + const VehicleAreaConfig& areaConfig2 = config.areaConfigs[1]; + ASSERT_EQ(areaConfig2.access, VehiclePropertyAccess::READ); + ASSERT_EQ(areaConfig2.areaId, 1); +} + } // namespace vehicle } // namespace automotive } // namespace hardware diff --git a/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp b/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp index b5ee335ff6..b0e44ae649 100644 --- a/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp +++ b/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp @@ -119,6 +119,7 @@ class VtsHalAutomotiveVehicleTargetTest : public testing::TestWithParamgetAccess() != toInt(VehiclePropertyAccess::NONE)) { + isReadWrite = (cfg.getAreaConfigs()[0]->getAccess() == + toInt(VehiclePropertyAccess::READ_WRITE)); + } + if (isReadWrite && isBooleanGlobalProp(propId) && !hvacProps.count(propId)) { auto propToGet = mVhalClient->createHalPropValue(propId); auto getValueResult = mVhalClient->getValueSync(*propToGet); @@ -469,6 +475,53 @@ TEST_P(VtsHalAutomotiveVehicleTargetTest, testGetValuesTimestampAIDL) { } } +// Test that access mode is populated in exclusively one of the VehiclePropConfig or the +// VehicleAreaConfigs. Either VehiclePropConfig.access must be populated, or all the +// VehicleAreaConfig.access fields should be populated. +TEST_P(VtsHalAutomotiveVehicleTargetTest, testAccessModeExclusivityAIDL) { + if (!mVhalClient->isAidlVhal()) { + GTEST_SKIP() << "Skip checking access mode for HIDL because the access mode field is only " + "present for AIDL"; + } + + auto result = mVhalClient->getAllPropConfigs(); + ASSERT_TRUE(result.ok()); + for (const auto& cfgPtr : result.value()) { + const IHalPropConfig& cfg = *cfgPtr; + + bool propAccessIsSet = (cfg.getAccess() != toInt(VehiclePropertyAccess::NONE)); + bool unsetAreaAccessExists = false; + bool setAreaAccessExists = false; + + for (const auto& areaConfig : cfg.getAreaConfigs()) { + if (areaConfig->getAccess() == toInt(VehiclePropertyAccess::NONE)) { + unsetAreaAccessExists = true; + } else { + setAreaAccessExists = true; + } + } + + ASSERT_FALSE(propAccessIsSet && setAreaAccessExists) << StringPrintf( + "Both prop and area config access is set for propertyId %d", cfg.getPropId()); + ASSERT_FALSE(!propAccessIsSet && !setAreaAccessExists) << StringPrintf( + "Neither prop and area config access is set for propertyId %d", cfg.getPropId()); + ASSERT_FALSE(unsetAreaAccessExists && setAreaAccessExists) << StringPrintf( + "Area access is only set in some configs for propertyId %d", cfg.getPropId()); + } +} + +void VtsHalAutomotiveVehicleTargetTest::verifyAccessMode(int actualAccess, int expectedAccess) { + if (expectedAccess == toInt(VehiclePropertyAccess::READ_WRITE)) { + ASSERT_TRUE(actualAccess == expectedAccess || + actualAccess == toInt(VehiclePropertyAccess::READ)) + << StringPrintf("Expect to get VehiclePropertyAccess: %i or %i, got %i", + expectedAccess, toInt(VehiclePropertyAccess::READ), actualAccess); + return; + } + ASSERT_EQ(actualAccess, expectedAccess) << StringPrintf( + "Expect to get VehiclePropertyAccess: %i, got %i", expectedAccess, actualAccess); +} + // Helper function to compare actual vs expected property config void VtsHalAutomotiveVehicleTargetTest::verifyProperty(VehicleProperty propId, VehiclePropertyAccess access, @@ -511,7 +564,6 @@ void VtsHalAutomotiveVehicleTargetTest::verifyProperty(VehicleProperty propId, const auto& config = result.value().at(0); int actualPropId = config->getPropId(); - int actualAccess = config->getAccess(); int actualChangeMode = config->getChangeMode(); int actualGroup = actualPropId & toInt(VehiclePropertyGroup::MASK); int actualArea = actualPropId & toInt(VehicleArea::MASK); @@ -520,14 +572,17 @@ void VtsHalAutomotiveVehicleTargetTest::verifyProperty(VehicleProperty propId, ASSERT_EQ(actualPropId, expectedPropId) << StringPrintf("Expect to get property ID: %i, got %i", expectedPropId, actualPropId); - if (expectedAccess == toInt(VehiclePropertyAccess::READ_WRITE)) { - ASSERT_TRUE(actualAccess == expectedAccess || - actualAccess == toInt(VehiclePropertyAccess::READ)) - << StringPrintf("Expect to get VehiclePropertyAccess: %i or %i, got %i", - expectedAccess, toInt(VehiclePropertyAccess::READ), actualAccess); + int globalAccess = config->getAccess(); + if (config->getAreaConfigSize() == 0) { + verifyAccessMode(globalAccess, expectedAccess); } else { - ASSERT_EQ(actualAccess, expectedAccess) << StringPrintf( - "Expect to get VehiclePropertyAccess: %i, got %i", expectedAccess, actualAccess); + for (const auto& areaConfig : config->getAreaConfigs()) { + int areaConfigAccess = areaConfig->getAccess(); + int actualAccess = (areaConfigAccess != toInt(VehiclePropertyAccess::NONE)) + ? areaConfigAccess + : globalAccess; + verifyAccessMode(actualAccess, expectedAccess); + } } ASSERT_EQ(actualChangeMode, expectedChangeMode)