Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6a4f3a87 authored by shrikar's avatar shrikar
Browse files

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
parent 36d82938

automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/include/JsonConfigLoader.h

+7 −8
Original line number Diff line number Diff line
@@ -130,11 +130,8 @@ class JsonConfigParser { 
                                     std::vector<T>* outPtr, std::vector<std::string>* errors);

    // Parses a JSON field to VehiclePropertyAccess or VehiclePropertyChangeMode.

    template <class T>

    void parseAccessChangeMode(

            const Json::Value& parentJsonNode, const std::string& fieldName, int propId,

            const std::string& propStr,

            const std::unordered_map<aidl::android::hardware::automotive::vehicle::VehicleProperty,

                                     T>& defaultMap,

    void parseAccessChangeMode(const Json::Value& parentJsonNode, const std::string& fieldName,

                               const std::string& propStr, const T* defaultAccessChangeModePtr,

                               T* outPtr, std::vector<std::string>* errors);



    // Parses a JSON field to RawPropValues.
@@ -145,8 +142,10 @@ class JsonConfigParser { 
                         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);

    void parseAreas(

            const Json::Value& parentJsonNode, const std::string& fieldName,

            ConfigDeclaration* outPtr, std::vector<std::string>* errors,

            aidl::android::hardware::automotive::vehicle::VehiclePropertyAccess defaultAccess);

};



}  // namespace jsonconfigloader_impl

automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/src/JsonConfigLoader.cpp

+33 −18
Original line number Diff line number Diff line
@@ -487,10 +487,11 @@ bool JsonConfigParser::tryParseJsonArrayToVariable(const Json::Value& parentJson 
}



template <class T>

void JsonConfigParser::parseAccessChangeMode(

        const Json::Value& parentJsonNode, const std::string& fieldName, int propId,

        const std::string& propStr, const std::unordered_map<VehicleProperty, T>& defaultMap,

        T* outPtr, std::vector<std::string>* errors) {

void JsonConfigParser::parseAccessChangeMode(const Json::Value& parentJsonNode,

                                             const std::string& fieldName,

                                             const std::string& propStr,

                                             const T* defaultAccessChangeModeValuePtr, T* outPtr,

                                             std::vector<std::string>* 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<T>(result.value());

        return;

    }

    auto it = defaultMap.find(static_cast<VehicleProperty>(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<std::string>* errors) {

                                  ConfigDeclaration* config, std::vector<std::string>* 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<ConfigDeclaration> JsonConfigParser::parseEachProperty( 


    configDecl.config.prop = propId;

    std::string propStr = propJsonValue["property"].toStyledString();

    VehiclePropertyAccess* defaultAccessMode = NULL;

    auto itAccess = AccessForVehicleProperty.find(static_cast<VehicleProperty>(propId));

    if (itAccess != AccessForVehicleProperty.end()) {

        defaultAccessMode = &itAccess->second;

    }

    VehiclePropertyChangeMode* defaultChangeMode = NULL;

    auto itChangeMode = ChangeModeForVehicleProperty.find(static_cast<VehicleProperty>(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<ConfigDeclaration> 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;

}

automotive/vehicle/aidl/impl/default_config/JsonConfigLoader/test/JsonConfigLoaderUnitTest.cpp

+108 −4
Original line number Diff line number Diff line
@@ -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

automotive/vehicle/vts/src/VtsHalAutomotiveVehicle_TargetTest.cpp

+65 −10
Original line number Diff line number Diff line
@@ -119,6 +119,7 @@ class VtsHalAutomotiveVehicleTargetTest : public testing::TestWithParam<ServiceD 
    bool checkIsSupported(int32_t propertyId);



  public:

    void verifyAccessMode(int actualAccess, int expectedAccess);

    void verifyProperty(VehicleProperty propId, VehiclePropertyAccess access,

                        VehiclePropertyChangeMode changeMode, VehiclePropertyGroup group,

                        VehicleArea area, VehiclePropertyType propertyType);
@@ -250,8 +251,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);
@@ -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)