Populate both VehiclePropConfig and VehicleAreaConfig.access
Bug: 323122049
Test: atest JsonConfigLoaderTest VtsHalAutomotiveVehicle_TargetTest
Change-Id: I398b136e705df805c25d541f7721f36c47813273
diff --git a/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehicleAreaConfig.aidl b/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehicleAreaConfig.aidl
index aab3c46..96c4a74 100644
--- a/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehicleAreaConfig.aidl
+++ b/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehicleAreaConfig.aidl
@@ -53,21 +53,25 @@
/**
* Defines if the area ID for this property is READ, WRITE or READ_WRITE. This only applies if
* the property is defined in the framework as a READ_WRITE property. Access (if set) should be
- * equal to, or a superset of, the VehiclePropConfig.access of the property.
+ * equal to, or a superset of, the VehiclePropConfig.access of the property. If access is not
+ * set for this VehicleAreaConfig (i.e. access == VehiclePropertyAccess.NONE), then it will
+ * automatically be assumed that the areaId access is the same as the VehiclePropConfig.access
+ * of the property.
*
* For example, if a property is defined as READ_WRITE, but the OEM wants to specify certain
* area Ids as READ-only, the corresponding areaIds should have an access set to READ, while the
* others must be set to READ_WRITE. We do not support setting specific area Ids to WRITE-only
* when the property is READ-WRITE.
*
- * Exclusively one of VehiclePropConfig and the VehicleAreaConfigs should be specified for a
- * single property. If VehiclePropConfig.access is populated, none of the
- * VehicleAreaConfig.access values should be populated. If VehicleAreaConfig.access values are
- * populated, VehiclePropConfig.access must not be populated.
+ * VehiclePropConfig.access should be equal the maximal subset of the accesses set in
+ * VehiclePropConfig.areaConfigs, excluding those with access == VehiclePropertyAccess.NONE. For
+ * example, if a VehiclePropConfig has some area configs with an access of
+ * VehiclePropertyAccess.READ and others with an access of VehiclePropertyAccess.READ_WRITE, the
+ * VehiclePropConfig object's access should be VehiclePropertyAccess.READ.
*
- * VehicleAreaConfigs should not be partially populated with access. If the OEM wants to specify
- * access for one area Id, all other configs should be populated with their access levels as
- * well.
+ * In the scenario where the OEM actually wants to set VehicleAreaConfig.access =
+ * VehiclePropertyAccess.NONE, the maximal subset rule should apply with this area config
+ * included, making the VehiclePropConfig.access = VehiclePropertyAccess.NONE.
*/
VehiclePropertyAccess access = VehiclePropertyAccess.NONE;
diff --git a/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehiclePropConfig.aidl b/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehiclePropConfig.aidl
index 1135b26..c629b82 100644
--- a/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehiclePropConfig.aidl
+++ b/automotive/vehicle/aidl/android/hardware/automotive/vehicle/VehiclePropConfig.aidl
@@ -29,9 +29,20 @@
/**
* Defines if the property is read or write or both.
*
- * If populating VehicleAreaConfig.access fields for this property, this field should not be
- * populated. If the OEM decides to populate this field, none of the VehicleAreaConfig.access
- * fields should be populated.
+ * If any VehicleAreaConfig.access is not set (i.e. VehicleAreaConfig.access ==
+ * VehiclePropertyAccess.NONE) for this property, it will automatically be assumed that the
+ * areaId access is the same as the VehiclePropConfig.access.
+ *
+ * VehiclePropConfig.access should be equal the maximal subset of the accesses set in its
+ * areaConfigs, excluding those with access == VehiclePropertyAccess.NONE. For example, if a
+ * VehiclePropConfig has some area configs with an access of VehiclePropertyAccess.READ and
+ * others with an access of VehiclePropertyAccess.READ_WRITE, the VehiclePropConfig object's
+ * access should be VehiclePropertyAccess.READ.
+ *
+ * In the scenario where the OEM actually wants to set VehicleAreaConfig.access =
+ * VehiclePropertyAccess.NONE for a particular area config, the maximal subset rule should apply
+ * with this area config included, making the VehiclePropConfig.access =
+ * VehiclePropertyAccess.NONE.
*/
VehiclePropertyAccess access = VehiclePropertyAccess.NONE;
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 82e5860..00c497f 100644
--- a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/include/JsonConfigLoader.h
+++ b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/include/JsonConfigLoader.h
@@ -142,10 +142,8 @@
std::vector<std::string>* 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<std::string>* errors,
- aidl::android::hardware::automotive::vehicle::VehiclePropertyAccess defaultAccess);
+ void parseAreas(const Json::Value& parentJsonNode, const std::string& fieldName,
+ ConfigDeclaration* outPtr, std::vector<std::string>* errors);
};
} // 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 76db891..7156b7e 100644
--- a/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/src/JsonConfigLoader.cpp
+++ b/automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/src/JsonConfigLoader.cpp
@@ -538,8 +538,7 @@
}
void JsonConfigParser::parseAreas(const Json::Value& parentJsonNode, const std::string& fieldName,
- ConfigDeclaration* config, std::vector<std::string>* errors,
- VehiclePropertyAccess defaultAccess) {
+ ConfigDeclaration* config, std::vector<std::string>* errors) {
if (!parentJsonNode.isObject()) {
errors->push_back("Node: " + parentJsonNode.toStyledString() + " is not an object");
return;
@@ -563,8 +562,8 @@
}
VehicleAreaConfig areaConfig = {};
areaConfig.areaId = areaId;
- parseAccessChangeMode(jsonAreaConfig, "access", propStr, &defaultAccess, &areaConfig.access,
- errors);
+ parseAccessChangeMode(jsonAreaConfig, "access", propStr, &(config->config.access),
+ &areaConfig.access, errors);
tryParseJsonValueToVariable(jsonAreaConfig, "minInt32Value", /*optional=*/true,
&areaConfig.minInt32Value, errors);
tryParseJsonValueToVariable(jsonAreaConfig, "maxInt32Value", /*optional=*/true,
@@ -622,8 +621,8 @@
if (itChangeMode != ChangeModeForVehicleProperty.end()) {
defaultChangeMode = &itChangeMode->second;
}
- VehiclePropertyAccess access = VehiclePropertyAccess::NONE;
- parseAccessChangeMode(propJsonValue, "access", propStr, defaultAccessMode, &access, errors);
+ parseAccessChangeMode(propJsonValue, "access", propStr, defaultAccessMode,
+ &configDecl.config.access, errors);
parseAccessChangeMode(propJsonValue, "changeMode", propStr, defaultChangeMode,
&configDecl.config.changeMode, errors);
@@ -642,14 +641,14 @@
tryParseJsonValueToVariable(propJsonValue, "maxSampleRate", /*optional=*/true,
&configDecl.config.maxSampleRate, errors);
- parseAreas(propJsonValue, "areas", &configDecl, errors, access);
+ parseAreas(propJsonValue, "areas", &configDecl, errors);
// 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,
+ .access = configDecl.config.access,
.supportVariableUpdateRate = true,
};
configDecl.config.areaConfigs.push_back(std::move(areaConfig));
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 a13d3df..54afbd4 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,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& propConfig = configs.begin()->second.config;
- ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(propConfig.access, VehiclePropertyAccess::READ);
ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::READ);
ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::STATIC);
}
@@ -308,7 +308,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& propConfig = configs.begin()->second.config;
- ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(propConfig.access, VehiclePropertyAccess::WRITE);
ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::WRITE);
ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::STATIC);
}
@@ -330,7 +330,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& propConfig = configs.begin()->second.config;
- ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(propConfig.access, VehiclePropertyAccess::READ);
ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::READ);
ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::ON_CHANGE);
}
@@ -353,7 +353,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& propConfig = configs.begin()->second.config;
- ASSERT_EQ(propConfig.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(propConfig.access, VehiclePropertyAccess::WRITE);
ASSERT_EQ(propConfig.areaConfigs[0].access, VehiclePropertyAccess::WRITE);
ASSERT_EQ(propConfig.changeMode, VehiclePropertyChangeMode::ON_CHANGE);
}
@@ -554,7 +554,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& config = configs.begin()->second.config;
- ASSERT_EQ(config.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(config.access, VehiclePropertyAccess::READ);
ASSERT_EQ(config.areaConfigs.size(), 1u);
const VehicleAreaConfig& areaConfig = config.areaConfigs[0];
ASSERT_EQ(areaConfig.minInt32Value, 1);
@@ -641,7 +641,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& config = configs.begin()->second.config;
- ASSERT_EQ(config.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(config.access, VehiclePropertyAccess::READ);
ASSERT_EQ(config.areaConfigs.size(), 1u);
const VehicleAreaConfig& areaConfig = config.areaConfigs[0];
@@ -670,7 +670,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& config = configs.begin()->second.config;
- ASSERT_EQ(config.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(config.access, VehiclePropertyAccess::READ);
ASSERT_EQ(config.areaConfigs.size(), 1u);
const VehicleAreaConfig& areaConfig = config.areaConfigs[0];
@@ -702,7 +702,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& config = configs.begin()->second.config;
- ASSERT_EQ(config.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(config.access, VehiclePropertyAccess::READ);
ASSERT_EQ(config.areaConfigs.size(), 1u);
const VehicleAreaConfig& areaConfig = config.areaConfigs[0];
@@ -731,7 +731,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& config = configs.begin()->second.config;
- ASSERT_EQ(config.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(config.access, VehiclePropertyAccess::READ_WRITE);
ASSERT_EQ(config.areaConfigs.size(), 1u);
const VehicleAreaConfig& areaConfig = config.areaConfigs[0];
@@ -759,7 +759,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& config = configs.begin()->second.config;
- ASSERT_EQ(config.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(config.access, VehiclePropertyAccess::READ);
ASSERT_EQ(config.areaConfigs.size(), 1u);
const VehicleAreaConfig& areaConfig = config.areaConfigs[0];
@@ -791,7 +791,7 @@
ASSERT_EQ(configs.size(), 1u);
const VehiclePropConfig& config = configs.begin()->second.config;
- ASSERT_EQ(config.access, VehiclePropertyAccess::NONE);
+ ASSERT_EQ(config.access, VehiclePropertyAccess::READ);
ASSERT_EQ(config.areaConfigs.size(), 2u);
const VehicleAreaConfig& areaConfig1 = config.areaConfigs[0];
diff --git a/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp b/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp
index 0fd1cc6..2b054a1 100644
--- a/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp
+++ b/automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp
@@ -58,6 +58,7 @@
using ::android::base::StringPrintf;
using ::android::frameworks::automotive::vhal::ErrorCode;
using ::android::frameworks::automotive::vhal::HalPropError;
+using ::android::frameworks::automotive::vhal::IHalAreaConfig;
using ::android::frameworks::automotive::vhal::IHalPropConfig;
using ::android::frameworks::automotive::vhal::IHalPropValue;
using ::android::frameworks::automotive::vhal::ISubscriptionCallback;
@@ -136,6 +137,9 @@
public:
void verifyAccessMode(int actualAccess, int expectedAccess);
+ void verifyGlobalAccessIsMaximalAreaAccessSubset(
+ int propertyLevelAccess,
+ const std::vector<std::unique_ptr<IHalAreaConfig>>& areaConfigs) const;
void verifyProperty(VehicleProperty propId, VehiclePropertyAccess access,
VehiclePropertyChangeMode changeMode, VehiclePropertyGroup group,
VehicleArea area, VehiclePropertyType propertyType);
@@ -254,6 +258,23 @@
}
}
+TEST_P(VtsHalAutomotiveVehicleTargetTest, testPropConfigs_globalAccessIsMaximalAreaAccessSubset) {
+ if (!mVhalClient->isAidlVhal()) {
+ GTEST_SKIP() << "Skip for HIDL VHAL because HAL interface run-time version is only"
+ << "introduced for AIDL";
+ }
+
+ auto result = mVhalClient->getAllPropConfigs();
+ ASSERT_TRUE(result.ok()) << "Failed to get all property configs, error: "
+ << result.error().message();
+
+ const auto& configs = result.value();
+ for (size_t i = 0; i < configs.size(); i++) {
+ verifyGlobalAccessIsMaximalAreaAccessSubset(configs[i]->getAccess(),
+ configs[i]->getAreaConfigs());
+ }
+}
+
// Test get() return current value for properties.
TEST_P(VtsHalAutomotiveVehicleTargetTest, get) {
ALOGD("VtsHalAutomotiveVehicleTargetTest::get");
@@ -586,42 +607,10 @@
}
}
-// 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 (actualAccess == toInt(VehiclePropertyAccess::NONE)) {
+ return;
+ }
if (expectedAccess == toInt(VehiclePropertyAccess::READ_WRITE)) {
ASSERT_TRUE(actualAccess == expectedAccess ||
actualAccess == toInt(VehiclePropertyAccess::READ))
@@ -633,6 +622,44 @@
"Expect to get VehiclePropertyAccess: %i, got %i", expectedAccess, actualAccess);
}
+void VtsHalAutomotiveVehicleTargetTest::verifyGlobalAccessIsMaximalAreaAccessSubset(
+ int propertyLevelAccess,
+ const std::vector<std::unique_ptr<IHalAreaConfig>>& areaConfigs) const {
+ bool readOnlyPresent = false;
+ bool writeOnlyPresent = false;
+ bool readWritePresent = false;
+ int maximalAreaAccessSubset = toInt(VehiclePropertyAccess::NONE);
+ for (size_t i = 0; i < areaConfigs.size(); i++) {
+ int access = areaConfigs[i]->getAccess();
+ switch (access) {
+ case toInt(VehiclePropertyAccess::READ):
+ readOnlyPresent = true;
+ break;
+ case toInt(VehiclePropertyAccess::WRITE):
+ writeOnlyPresent = true;
+ break;
+ case toInt(VehiclePropertyAccess::READ_WRITE):
+ readWritePresent = true;
+ break;
+ default:
+ ASSERT_EQ(access, toInt(VehiclePropertyAccess::NONE)) << StringPrintf(
+ "Area access can be NONE only if global property access is also NONE");
+ return;
+ }
+ }
+
+ if (readOnlyPresent && !writeOnlyPresent) {
+ maximalAreaAccessSubset = toInt(VehiclePropertyAccess::READ);
+ } else if (writeOnlyPresent) {
+ maximalAreaAccessSubset = toInt(VehiclePropertyAccess::WRITE);
+ } else if (readWritePresent) {
+ maximalAreaAccessSubset = toInt(VehiclePropertyAccess::READ_WRITE);
+ }
+ ASSERT_EQ(propertyLevelAccess, maximalAreaAccessSubset) << StringPrintf(
+ "Expected global access to be equal to maximal area access subset %d, Instead got %d",
+ maximalAreaAccessSubset, propertyLevelAccess);
+}
+
// Helper function to compare actual vs expected property config
void VtsHalAutomotiveVehicleTargetTest::verifyProperty(VehicleProperty propId,
VehiclePropertyAccess access,