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 d91f4f2616..0fd1cc69b4 100644 --- a/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp +++ b/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp @@ -135,6 +135,7 @@ class VtsHalAutomotiveVehicleTargetTest : public testing::TestWithParam>& result, int32_t value); public: + void verifyAccessMode(int actualAccess, int expectedAccess); void verifyProperty(VehicleProperty propId, VehiclePropertyAccess access, VehiclePropertyChangeMode changeMode, VehiclePropertyGroup group, VehicleArea area, VehiclePropertyType propertyType); @@ -322,8 +323,13 @@ TEST_P(VtsHalAutomotiveVehicleTargetTest, setProp) { const IHalPropConfig& cfg = *cfgPtr; int32_t propId = cfg.getPropId(); // test on boolean and writable property - if (cfg.getAccess() == toInt(VehiclePropertyAccess::READ_WRITE) && - isBooleanGlobalProp(propId) && !hvacProps.count(propId)) { + bool isReadWrite = (cfg.getAccess() == toInt(VehiclePropertyAccess::READ_WRITE)); + if (cfg.getAreaConfigSize() != 0 && + cfg.getAreaConfigs()[0]->getAccess() != 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); @@ -580,6 +586,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, @@ -622,7 +675,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); @@ -631,14 +683,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)