From 96b97eb17dec57a0d1a82e4d07ede7a328c6ac11 Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Fri, 12 Jul 2024 14:59:04 -0700 Subject: [PATCH] Introduce getPropertyConfig to IVehicleHardware This function gets only one property config instead of getting all. This is useful to let IVehicleHardware to return one config or not_supported for early boot required properties when not all property configs are available. Test: Local run on cf_auto atest DefaultVehicleHalTest Bug: 342470570 Change-Id: I0a8aec95007e901ceffce1ee3ead1f6bbeb7a089 --- .../aidl/impl/grpc/GRPCVehicleHardware.cpp | 11 +++ .../aidl/impl/grpc/GRPCVehicleHardware.h | 4 + .../impl/hardware/include/IVehicleHardware.h | 65 ++++++++++------ .../impl/vhal/include/DefaultVehicleHal.h | 2 + .../aidl/impl/vhal/src/DefaultVehicleHal.cpp | 74 ++++++++++++++----- .../impl/vhal/test/DefaultVehicleHalTest.cpp | 59 +++++++++++++++ .../impl/vhal/test/MockVehicleHardware.cpp | 16 ++++ .../aidl/impl/vhal/test/MockVehicleHardware.h | 7 ++ 8 files changed, 195 insertions(+), 43 deletions(-) diff --git a/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.cpp b/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.cpp index 73bb521ddb..201ddb0eef 100644 --- a/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.cpp +++ b/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.cpp @@ -83,6 +83,17 @@ std::vector GRPCVehicleHardware::getAllPropertyConf return configs; } +std::optional GRPCVehicleHardware::getPropertyConfig( + int32_t propId) const { + // TODO(b/354055835): Use GRPC call to get one config instead of getting all the configs. + for (const auto& config : getAllPropertyConfigs()) { + if (config.prop == propId) { + return config; + } + } + return std::nullopt; +} + aidlvhal::StatusCode GRPCVehicleHardware::setValues( std::shared_ptr callback, const std::vector& requests) { diff --git a/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.h b/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.h index 1edf6580aa..15f473c0bd 100644 --- a/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.h +++ b/automotive/vehicle/aidl/impl/grpc/GRPCVehicleHardware.h @@ -50,6 +50,10 @@ class GRPCVehicleHardware : public IVehicleHardware { // Get all the property configs. std::vector getAllPropertyConfigs() const override; + // Get the config for the specified propId. + std::optional + getPropertyConfig(int32_t propId) const override; + // Set property values asynchronously. Server could return before the property set requests // are sent to vehicle bus or before property set confirmation is received. The callback is // safe to be called after the function returns and is safe to be called in a different thread. diff --git a/automotive/vehicle/aidl/impl/hardware/include/IVehicleHardware.h b/automotive/vehicle/aidl/impl/hardware/include/IVehicleHardware.h index f49d91b30c..06846553da 100644 --- a/automotive/vehicle/aidl/impl/hardware/include/IVehicleHardware.h +++ b/automotive/vehicle/aidl/impl/hardware/include/IVehicleHardware.h @@ -20,6 +20,7 @@ #include #include +#include #include namespace android { @@ -46,33 +47,53 @@ struct SetValueErrorEvent { int32_t areaId; }; +namespace aidlvhal = ::aidl::android::hardware::automotive::vehicle; + // An abstract interface to access vehicle hardware. // For virtualized VHAL, GrpcVehicleHardware would communicate with a VehicleHardware // implementation in another VM through GRPC. For non-virtualzied VHAL, VHAL directly communicates // with a VehicleHardware through this interface. class IVehicleHardware { public: - using SetValuesCallback = std::function)>; - using GetValuesCallback = std::function)>; - using PropertyChangeCallback = std::function)>; + using SetValuesCallback = std::function)>; + using GetValuesCallback = std::function)>; + using PropertyChangeCallback = std::function)>; using PropertySetErrorCallback = std::function)>; virtual ~IVehicleHardware() = default; // Get all the property configs. - virtual std::vector - getAllPropertyConfigs() const = 0; + virtual std::vector getAllPropertyConfigs() const = 0; + + // Get the property configs for the specified propId. This is used for early-boot + // native VHAL clients to access certain property configs when not all property configs are + // available. For example, a config discovery process might be required to determine the + // property config for HVAC. However, for early boot properties, e.g. VHAL_HEARTBEAT, it + // could return before the config discovery process. + // + // Currently Android system may try to access the following properties during early boot: + // STORAGE_ENCRYPTION_BINDING_SEED, WATCHDOG_ALIVE, WATCHDOG_TERMINATE_PROCESS, VHAL_HEARTBEAT, + // CURRENT_POWER_POLICY, POWER_POLICY_REQ, POWER_POLICY_GROUP_REQ. They should return + // quickly otherwise the whole bootup process might be blocked. + virtual std::optional getPropertyConfig(int32_t propId) const { + // The default implementation is to use getAllPropertyConfigs(). This should be + // overridden if getAllPropertyConfigs() takes a while to return for initial boot or + // relies on ethernet or other communication channel that is not available during early + // boot. + for (const auto& config : getAllPropertyConfigs()) { + if (config.prop == propId) { + return config; + } + } + return std::nullopt; + } // Set property values asynchronously. Server could return before the property set requests // are sent to vehicle bus or before property set confirmation is received. The callback is // safe to be called after the function returns and is safe to be called in a different thread. - virtual aidl::android::hardware::automotive::vehicle::StatusCode setValues( + virtual aidlvhal::StatusCode setValues( std::shared_ptr callback, - const std::vector& - requests) = 0; + const std::vector& requests) = 0; // Get property values asynchronously. Server could return before the property values are ready. // The callback is safe to be called after the function returns and is safe to be called in a @@ -86,7 +107,7 @@ class IVehicleHardware { virtual DumpResult dump(const std::vector& options) = 0; // Check whether the system is healthy, return {@code StatusCode::OK} for healthy. - virtual aidl::android::hardware::automotive::vehicle::StatusCode checkHealth() = 0; + virtual aidlvhal::StatusCode checkHealth() = 0; // Register a callback that would be called when there is a property change event from vehicle. // This function must only be called once during initialization. @@ -179,16 +200,14 @@ class IVehicleHardware { // 5. The second subscriber is removed, 'unsubscribe' is called. // The impl can optionally disable the polling for vehicle speed. // - virtual aidl::android::hardware::automotive::vehicle::StatusCode subscribe( - [[maybe_unused]] aidl::android::hardware::automotive::vehicle::SubscribeOptions - options) { - return aidl::android::hardware::automotive::vehicle::StatusCode::OK; + virtual aidlvhal::StatusCode subscribe([[maybe_unused]] aidlvhal::SubscribeOptions options) { + return aidlvhal::StatusCode::OK; } // A [propId, areaId] is unsubscribed. This applies for both continuous or on-change property. - virtual aidl::android::hardware::automotive::vehicle::StatusCode unsubscribe( - [[maybe_unused]] int32_t propId, [[maybe_unused]] int32_t areaId) { - return aidl::android::hardware::automotive::vehicle::StatusCode::OK; + virtual aidlvhal::StatusCode unsubscribe([[maybe_unused]] int32_t propId, + [[maybe_unused]] int32_t areaId) { + return aidlvhal::StatusCode::OK; } // This function is deprecated, subscribe/unsubscribe should be used instead. @@ -216,10 +235,10 @@ class IVehicleHardware { // // If the impl is always polling at {@code maxSampleRate} as specified in config, then this // function can be a no-op. - virtual aidl::android::hardware::automotive::vehicle::StatusCode updateSampleRate( - [[maybe_unused]] int32_t propId, [[maybe_unused]] int32_t areaId, - [[maybe_unused]] float sampleRate) { - return aidl::android::hardware::automotive::vehicle::StatusCode::OK; + virtual aidlvhal::StatusCode updateSampleRate([[maybe_unused]] int32_t propId, + [[maybe_unused]] int32_t areaId, + [[maybe_unused]] float sampleRate) { + return aidlvhal::StatusCode::OK; } }; diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h index b58d0f5302..932a2e21d0 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h +++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h @@ -199,6 +199,8 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { bool checkDumpPermission(); + bool isConfigSupportedForCurrentVhalVersion(const aidlvhal::VehiclePropConfig& config) const; + bool getAllPropConfigsFromHardwareLocked() const EXCLUDES(mConfigLock); // The looping handler function to process all onBinderDied or onBinderUnlinked events in diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp index e062a286b2..0ead81934c 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp @@ -340,32 +340,37 @@ int32_t DefaultVehicleHal::getVhalInterfaceVersion() const { return myVersion; } +bool DefaultVehicleHal::isConfigSupportedForCurrentVhalVersion( + const VehiclePropConfig& config) const { + int32_t myVersion = getVhalInterfaceVersion(); + if (!isSystemProp(config.prop)) { + return true; + } + VehicleProperty property = static_cast(config.prop); + std::string propertyName = aidl::android::hardware::automotive::vehicle::toString(property); + auto it = VersionForVehicleProperty.find(property); + if (it == VersionForVehicleProperty.end()) { + ALOGE("The property: %s is not a supported system property, ignore", propertyName.c_str()); + return false; + } + int requiredVersion = it->second; + if (myVersion < requiredVersion) { + ALOGE("The property: %s is not supported for current client VHAL version, " + "require %d, current version: %d, ignore", + propertyName.c_str(), requiredVersion, myVersion); + return false; + } + return true; +} + bool DefaultVehicleHal::getAllPropConfigsFromHardwareLocked() const { ALOGD("Get all property configs from hardware"); auto configs = mVehicleHardware->getAllPropertyConfigs(); std::vector filteredConfigs; - int32_t myVersion = getVhalInterfaceVersion(); - for (auto& config : configs) { - if (!isSystemProp(config.prop)) { + for (const auto& config : configs) { + if (isConfigSupportedForCurrentVhalVersion(config)) { filteredConfigs.push_back(std::move(config)); - continue; } - VehicleProperty property = static_cast(config.prop); - std::string propertyName = aidl::android::hardware::automotive::vehicle::toString(property); - auto it = VersionForVehicleProperty.find(property); - if (it == VersionForVehicleProperty.end()) { - ALOGE("The property: %s is not a supported system property, ignore", - propertyName.c_str()); - continue; - } - int requiredVersion = it->second; - if (myVersion < requiredVersion) { - ALOGE("The property: %s is not supported for current client VHAL version, " - "require %d, current version: %d, ignore", - propertyName.c_str(), requiredVersion, myVersion); - continue; - } - filteredConfigs.push_back(std::move(config)); } { @@ -431,6 +436,19 @@ ScopedAStatus DefaultVehicleHal::getAllPropConfigs(VehiclePropConfigs* output) { Result DefaultVehicleHal::getConfig(int32_t propId) const { Result result; + + if (!mConfigInit) { + std::optional config = mVehicleHardware->getPropertyConfig(propId); + if (!config.has_value()) { + return Error() << "no config for property, ID: " << propId; + } + if (!isConfigSupportedForCurrentVhalVersion(config.value())) { + return Error() << "property not supported for current VHAL interface, ID: " << propId; + } + + return config.value(); + } + getConfigsByPropId([this, &result, propId](const auto& configsByPropId) { SharedScopedLockAssertion lockAssertion(mConfigLock); @@ -685,6 +703,22 @@ ScopedAStatus DefaultVehicleHal::setValues(const CallbackType& callback, ScopedAStatus DefaultVehicleHal::getPropConfigs(const std::vector& props, VehiclePropConfigs* output) { std::vector configs; + + if (!mConfigInit) { + for (int32_t prop : props) { + auto maybeConfig = mVehicleHardware->getPropertyConfig(prop); + if (!maybeConfig.has_value() || + !isConfigSupportedForCurrentVhalVersion(maybeConfig.value())) { + return ScopedAStatus::fromServiceSpecificErrorWithMessage( + toInt(StatusCode::INVALID_ARG), + StringPrintf("no config for property, ID: %" PRId32, prop).c_str()); + } + configs.push_back(maybeConfig.value()); + } + + return vectorToStableLargeParcelable(std::move(configs), output); + } + ScopedAStatus status = ScopedAStatus::ok(); getConfigsByPropId([this, &configs, &status, &props](const auto& configsByPropId) { SharedScopedLockAssertion lockAssertion(mConfigLock); diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp index 11a8fc7ac1..4891bf56a9 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp @@ -650,6 +650,8 @@ TEST_F(DefaultVehicleHalTest, testGetPropConfigs) { auto hardware = std::make_unique(); hardware->setPropertyConfigs(testConfigs); + // Store the pointer for testing. We are sure it is valid. + MockVehicleHardware* hardwarePtr = hardware.get(); auto vhal = ndk::SharedRefBase::make(std::move(hardware)); std::shared_ptr client = IVehicle::fromBinder(vhal->asBinder()); @@ -658,6 +660,7 @@ TEST_F(DefaultVehicleHalTest, testGetPropConfigs) { ASSERT_TRUE(status.isOk()) << "getPropConfigs failed: " << status.getMessage(); ASSERT_EQ(output.payloads, testConfigs); + ASSERT_FALSE(hardwarePtr->getAllPropertyConfigsCalled()); } TEST_F(DefaultVehicleHalTest, testGetPropConfigsInvalidArg) { @@ -704,6 +707,34 @@ TEST_F(DefaultVehicleHalTest, testGetValuesSmall) { ASSERT_TRUE(maybeGetValueResults.has_value()) << "no results in callback"; EXPECT_EQ(maybeGetValueResults.value().payloads, expectedResults) << "results mismatch"; EXPECT_EQ(countClients(), static_cast(1)); + ASSERT_FALSE(getHardware()->getAllPropertyConfigsCalled()); +} + +TEST_F(DefaultVehicleHalTest, testGetValuesSmall_AfterGetAllPropConfigs) { + GetValueRequests requests; + std::vector expectedResults; + std::vector expectedHardwareRequests; + + // If we already called getAllPropConfigs, the configs will be cached. + VehiclePropConfigs output; + getClient()->getAllPropConfigs(&output); + + ASSERT_TRUE(getValuesTestCases(10, requests, expectedResults, expectedHardwareRequests).ok()); + + getHardware()->addGetValueResponses(expectedResults); + + auto status = getClient()->getValues(getCallbackClient(), requests); + + ASSERT_TRUE(status.isOk()) << "getValues failed: " << status.getMessage(); + + EXPECT_EQ(getHardware()->nextGetValueRequests(), expectedHardwareRequests) + << "requests to hardware mismatch"; + + auto maybeGetValueResults = getCallback()->nextGetValueResults(); + ASSERT_TRUE(maybeGetValueResults.has_value()) << "no results in callback"; + EXPECT_EQ(maybeGetValueResults.value().payloads, expectedResults) << "results mismatch"; + EXPECT_EQ(countClients(), static_cast(1)); + ASSERT_TRUE(getHardware()->getAllPropertyConfigsCalled()); } TEST_F(DefaultVehicleHalTest, testGetValuesLarge) { @@ -1016,6 +1047,34 @@ TEST_F(DefaultVehicleHalTest, testSetValuesSmall) { ASSERT_TRUE(maybeSetValueResults.has_value()) << "no results in callback"; ASSERT_EQ(maybeSetValueResults.value().payloads, expectedResults) << "results mismatch"; EXPECT_EQ(countClients(), static_cast(1)); + ASSERT_FALSE(getHardware()->getAllPropertyConfigsCalled()); +} + +TEST_F(DefaultVehicleHalTest, testSetValuesSmall_AfterGetAllPropConfigs) { + SetValueRequests requests; + std::vector expectedResults; + std::vector expectedHardwareRequests; + + // If we already called getAllPropConfigs, the configs will be cached. + VehiclePropConfigs output; + getClient()->getAllPropConfigs(&output); + + ASSERT_TRUE(setValuesTestCases(10, requests, expectedResults, expectedHardwareRequests).ok()); + + getHardware()->addSetValueResponses(expectedResults); + + auto status = getClient()->setValues(getCallbackClient(), requests); + + ASSERT_TRUE(status.isOk()) << "setValues failed: " << status.getMessage(); + + EXPECT_EQ(getHardware()->nextSetValueRequests(), expectedHardwareRequests) + << "requests to hardware mismatch"; + + auto maybeSetValueResults = getCallback()->nextSetValueResults(); + ASSERT_TRUE(maybeSetValueResults.has_value()) << "no results in callback"; + ASSERT_EQ(maybeSetValueResults.value().payloads, expectedResults) << "results mismatch"; + EXPECT_EQ(countClients(), static_cast(1)); + ASSERT_TRUE(getHardware()->getAllPropertyConfigsCalled()); } TEST_F(DefaultVehicleHalTest, testSetValuesLarge) { diff --git a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp index db15c8942f..e796ce56f5 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp @@ -45,9 +45,20 @@ MockVehicleHardware::~MockVehicleHardware() { std::vector MockVehicleHardware::getAllPropertyConfigs() const { std::scoped_lock lockGuard(mLock); + mGetAllPropertyConfigsCalled = true; return mPropertyConfigs; } +std::optional MockVehicleHardware::getPropertyConfig(int32_t propId) const { + std::scoped_lock lockGuard(mLock); + for (const auto& config : mPropertyConfigs) { + if (config.prop == propId) { + return config; + } + } + return std::nullopt; +} + StatusCode MockVehicleHardware::setValues(std::shared_ptr callback, const std::vector& requests) { std::scoped_lock lockGuard(mLock); @@ -336,6 +347,11 @@ void MockVehicleHardware::sendOnPropertySetErrorEvent( (*mPropertySetErrorCallback)(errorEvents); } +bool MockVehicleHardware::getAllPropertyConfigsCalled() { + std::scoped_lock lockGuard(mLock); + return mGetAllPropertyConfigsCalled; +} + } // namespace vehicle } // namespace automotive } // namespace hardware diff --git a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.h b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.h index eeca582fa4..06e01a8ec0 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.h +++ b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.h @@ -47,6 +47,8 @@ class MockVehicleHardware final : public IVehicleHardware { std::vector getAllPropertyConfigs() const override; + std::optional + getPropertyConfig(int32_t propId) const override; aidl::android::hardware::automotive::vehicle::StatusCode setValues( std::shared_ptr callback, const std::vector& @@ -98,6 +100,9 @@ class MockVehicleHardware final : public IVehicleHardware { std::vector getSubscribeOptions(); void clearSubscribeOptions(); + // Whether getAllPropertyConfigs() has been called, which blocks all all property configs + // being ready. + bool getAllPropertyConfigsCalled(); private: mutable std::mutex mLock; @@ -143,6 +148,8 @@ class MockVehicleHardware final : public IVehicleHardware { DumpResult mDumpResult; + mutable bool mGetAllPropertyConfigsCalled GUARDED_BY(mLock) = false; + // RecurrentTimer is thread-safe. std::shared_ptr mRecurrentTimer; std::unordered_map>>>