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,