From fda1197d79cabfd98d70850bf374f32e4aebcd16 Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Fri, 23 Jun 2023 17:43:17 -0700 Subject: [PATCH] Pass property set error to subscribed clients. Pass the async property set error generated by VehicleHardware layer to subscribed clients Test: atest DefaultVehicleHalTest Bug: 286384730 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:7b1448edcfd3f88365932e2d94659952444aae2b) Merged-In: Iadd92e1c0c741ad6450a0508fe9e6301bdfe66c5 Change-Id: Iadd92e1c0c741ad6450a0508fe9e6301bdfe66c5 --- .../hardware/src/FakeVehicleHardware.cpp | 1 + .../aidl/impl/vhal/include/ConnectedClient.h | 8 ++- .../impl/vhal/include/DefaultVehicleHal.h | 6 +- .../impl/vhal/include/SubscriptionManager.h | 6 ++ .../aidl/impl/vhal/src/ConnectedClient.cpp | 31 +++++++++- .../aidl/impl/vhal/src/DefaultVehicleHal.cpp | 37 +++++++++--- .../impl/vhal/src/SubscriptionManager.cpp | 27 +++++++++ .../impl/vhal/test/DefaultVehicleHalTest.cpp | 58 +++++++++++++++++++ .../impl/vhal/test/MockVehicleCallback.cpp | 20 ++++++- .../aidl/impl/vhal/test/MockVehicleCallback.h | 5 ++ .../impl/vhal/test/MockVehicleHardware.cpp | 11 +++- .../aidl/impl/vhal/test/MockVehicleHardware.h | 2 + 12 files changed, 198 insertions(+), 14 deletions(-) diff --git a/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp b/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp index 3f5e4c4273..46c67a517a 100644 --- a/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp +++ b/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp @@ -1808,6 +1808,7 @@ void FakeVehicleHardware::registerOnPropertyChangeEvent( void FakeVehicleHardware::registerOnPropertySetErrorEvent( std::unique_ptr callback) { + // In FakeVehicleHardware, we will never use mOnPropertySetErrorCallback. if (mOnPropertySetErrorCallback != nullptr) { ALOGE("registerOnPropertySetErrorEvent must only be called once"); return; diff --git a/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h b/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h index 2e7298ff0b..b3f4a0f722 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h +++ b/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h @@ -107,12 +107,18 @@ class SubscriptionClient final : public ConnectedClient { // Gets the callback to be called when the request for this client has finished. std::shared_ptr getResultCallback(); - // Marshals the updated values into largeParcelable and sents it through {@code onPropertyEvent} + // Marshals the updated values into largeParcelable and sends it through {@code onPropertyEvent} // callback. static void sendUpdatedValues( CallbackType callback, std::vector&& updatedValues); + // Marshals the set property error events into largeParcelable and sends it through + // {@code onPropertySetError} callback. + static void sendPropertySetErrors( + CallbackType callback, + std::vector&& + vehiclePropErrors); protected: // Gets the callback to be called when the request for this client has timeout. diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h index 2c2cf1a5bd..74ad7eaf3e 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h +++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h @@ -249,10 +249,14 @@ class DefaultVehicleHal final : public aidl::android::hardware::automotive::vehi const CallbackType& callback, std::shared_ptr pendingRequestPool); static void onPropertyChangeEvent( - std::weak_ptr subscriptionManager, + const std::weak_ptr& subscriptionManager, const std::vector& updatedValues); + static void onPropertySetErrorEvent( + const std::weak_ptr& subscriptionManager, + const std::vector& errorEvents); + static void checkHealth(IVehicleHardware* hardware, std::weak_ptr subscriptionManager); diff --git a/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h b/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h index 14799d9f84..301d56c270 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h +++ b/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h @@ -99,6 +99,12 @@ class SubscriptionManager final { const std::vector& updatedValues); + // For a list of set property error events, returns a map that maps clients subscribing to the + // properties to a list of errors for each client. + std::unordered_map> + getSubscribedClientsForErrorEvents(const std::vector& errorEvents); + // Checks whether the sample rate is valid. static bool checkSampleRateHz(float sampleRateHz); diff --git a/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp b/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp index 81d231c87f..fb23a25047 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp @@ -38,6 +38,8 @@ using ::aidl::android::hardware::automotive::vehicle::IVehicleCallback; using ::aidl::android::hardware::automotive::vehicle::SetValueResult; using ::aidl::android::hardware::automotive::vehicle::SetValueResults; using ::aidl::android::hardware::automotive::vehicle::StatusCode; +using ::aidl::android::hardware::automotive::vehicle::VehiclePropError; +using ::aidl::android::hardware::automotive::vehicle::VehiclePropErrors; using ::aidl::android::hardware::automotive::vehicle::VehiclePropValue; using ::aidl::android::hardware::automotive::vehicle::VehiclePropValues; using ::android::base::Result; @@ -300,7 +302,34 @@ void SubscriptionClient::sendUpdatedValues(std::shared_ptr cal if (ScopedAStatus callbackStatus = callback->onPropertyEvent(vehiclePropValues, sharedMemoryFileCount); !callbackStatus.isOk()) { - ALOGE("subscribe: failed to call UpdateValues callback, client ID: %p, error: %s, " + ALOGE("subscribe: failed to call onPropertyEvent callback, client ID: %p, error: %s, " + "exception: %d, service specific error: %d", + callback->asBinder().get(), callbackStatus.getMessage(), + callbackStatus.getExceptionCode(), callbackStatus.getServiceSpecificError()); + } +} + +void SubscriptionClient::sendPropertySetErrors(std::shared_ptr callback, + std::vector&& vehiclePropErrors) { + if (vehiclePropErrors.empty()) { + return; + } + + VehiclePropErrors vehiclePropErrorsLargeParcelable; + ScopedAStatus status = vectorToStableLargeParcelable(std::move(vehiclePropErrors), + &vehiclePropErrorsLargeParcelable); + if (!status.isOk()) { + int statusCode = status.getServiceSpecificError(); + ALOGE("subscribe: failed to marshal result into large parcelable, error: " + "%s, code: %d", + status.getMessage(), statusCode); + return; + } + + if (ScopedAStatus callbackStatus = + callback->onPropertySetError(vehiclePropErrorsLargeParcelable); + !callbackStatus.isOk()) { + ALOGE("subscribe: failed to call onPropertySetError callback, client ID: %p, error: %s, " "exception: %d, service specific error: %d", callback->asBinder().get(), callbackStatus.getMessage(), callbackStatus.getExceptionCode(), callbackStatus.getServiceSpecificError()); diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp index 98cfc398af..0d5c070c54 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp @@ -144,6 +144,11 @@ DefaultVehicleHal::DefaultVehicleHal(std::unique_ptr vehicleHa [subscriptionManagerCopy](std::vector updatedValues) { onPropertyChangeEvent(subscriptionManagerCopy, updatedValues); })); + mVehicleHardware->registerOnPropertySetErrorEvent( + std::make_unique( + [subscriptionManagerCopy](std::vector errorEvents) { + onPropertySetErrorEvent(subscriptionManagerCopy, errorEvents); + })); // Register heartbeat event. mRecurrentAction = std::make_shared>( @@ -177,7 +182,7 @@ DefaultVehicleHal::~DefaultVehicleHal() { } void DefaultVehicleHal::onPropertyChangeEvent( - std::weak_ptr subscriptionManager, + const std::weak_ptr& subscriptionManager, const std::vector& updatedValues) { auto manager = subscriptionManager.lock(); if (manager == nullptr) { @@ -194,6 +199,20 @@ void DefaultVehicleHal::onPropertyChangeEvent( } } +void DefaultVehicleHal::onPropertySetErrorEvent( + const std::weak_ptr& subscriptionManager, + const std::vector& errorEvents) { + auto manager = subscriptionManager.lock(); + if (manager == nullptr) { + ALOGW("the SubscriptionManager is destroyed, DefaultVehicleHal is ending"); + return; + } + auto vehiclePropErrorsByClient = manager->getSubscribedClientsForErrorEvents(errorEvents); + for (auto& [callback, vehiclePropErrors] : vehiclePropErrorsByClient) { + SubscriptionClient::sendPropertySetErrors(callback, std::move(vehiclePropErrors)); + } +} + template std::shared_ptr DefaultVehicleHal::getOrCreateClient( std::unordered_map>* clients, @@ -692,15 +711,19 @@ ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback, // Create a new SubscriptionClient if there isn't an existing one. mSubscriptionClients->maybeAddClient(callback); - // Since we have already check the sample rates, the following functions must succeed. if (!onChangeSubscriptions.empty()) { - return toScopedAStatus(mSubscriptionManager->subscribe(callback, onChangeSubscriptions, - /*isContinuousProperty=*/false)); + auto result = mSubscriptionManager->subscribe(callback, onChangeSubscriptions, + /*isContinuousProperty=*/false); + if (!result.ok()) { + return toScopedAStatus(result); + } } if (!continuousSubscriptions.empty()) { - return toScopedAStatus(mSubscriptionManager->subscribe(callback, - continuousSubscriptions, - /*isContinuousProperty=*/true)); + auto result = mSubscriptionManager->subscribe(callback, continuousSubscriptions, + /*isContinuousProperty=*/true); + if (!result.ok()) { + return toScopedAStatus(result); + } } } return ScopedAStatus::ok(); diff --git a/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp b/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp index bba730f6f4..1f2690e340 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp @@ -36,6 +36,7 @@ constexpr float ONE_SECOND_IN_NANO = 1'000'000'000.; using ::aidl::android::hardware::automotive::vehicle::IVehicleCallback; using ::aidl::android::hardware::automotive::vehicle::StatusCode; using ::aidl::android::hardware::automotive::vehicle::SubscribeOptions; +using ::aidl::android::hardware::automotive::vehicle::VehiclePropError; using ::aidl::android::hardware::automotive::vehicle::VehiclePropValue; using ::android::base::Error; using ::android::base::Result; @@ -269,6 +270,32 @@ SubscriptionManager::getSubscribedClients(const std::vector& u return clients; } +std::unordered_map, std::vector> +SubscriptionManager::getSubscribedClientsForErrorEvents( + const std::vector& errorEvents) { + std::scoped_lock lockGuard(mLock); + std::unordered_map, std::vector> clients; + + for (const auto& errorEvent : errorEvents) { + PropIdAreaId propIdAreaId{ + .propId = errorEvent.propId, + .areaId = errorEvent.areaId, + }; + if (mClientsByPropIdArea.find(propIdAreaId) == mClientsByPropIdArea.end()) { + continue; + } + + for (const auto& [_, client] : mClientsByPropIdArea[propIdAreaId]) { + clients[client].push_back({ + .propId = errorEvent.propId, + .areaId = errorEvent.areaId, + .errorCode = errorEvent.errorCode, + }); + } + } + return clients; +} + bool SubscriptionManager::isEmpty() { std::scoped_lock lockGuard(mLock); return mSubscribedPropsByClient.empty() && mClientsByPropIdArea.empty(); diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp index 05e569ab8a..96b71f0af0 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp @@ -62,6 +62,7 @@ using ::aidl::android::hardware::automotive::vehicle::SubscribeOptions; using ::aidl::android::hardware::automotive::vehicle::VehicleAreaWindow; using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfig; using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfigs; +using ::aidl::android::hardware::automotive::vehicle::VehiclePropError; using ::aidl::android::hardware::automotive::vehicle::VehiclePropErrors; using ::aidl::android::hardware::automotive::vehicle::VehicleProperty; using ::aidl::android::hardware::automotive::vehicle::VehiclePropertyAccess; @@ -1653,6 +1654,63 @@ TEST_F(DefaultVehicleHalTest, testDumpCallerShouldNotDump) { ASSERT_EQ(msg.find("Vehicle HAL State: "), std::string::npos); } +TEST_F(DefaultVehicleHalTest, testOnPropertySetErrorEvent) { + std::vector options = { + { + .propId = GLOBAL_ON_CHANGE_PROP, + .areaIds = {0}, + }, + { + .propId = GLOBAL_CONTINUOUS_PROP, + .areaIds = {0}, + .sampleRate = 1, + }, + }; + auto status = getClient()->subscribe(getCallbackClient(), options, 0); + ASSERT_TRUE(status.isOk()) << "subscribe failed: " << status.getMessage(); + std::vector errorEvents = { + { + .propId = GLOBAL_ON_CHANGE_PROP, + .areaId = 0, + .errorCode = StatusCode::INTERNAL_ERROR, + }, + { + .propId = GLOBAL_ON_CHANGE_PROP, + .areaId = 0, + .errorCode = StatusCode::ACCESS_DENIED, + }, + { + .propId = GLOBAL_CONTINUOUS_PROP, + .areaId = 0, + .errorCode = StatusCode::INVALID_ARG, + }, + }; + std::vector expectedResults = { + { + .propId = GLOBAL_ON_CHANGE_PROP, + .areaId = 0, + .errorCode = StatusCode::INTERNAL_ERROR, + }, + { + .propId = GLOBAL_ON_CHANGE_PROP, + .areaId = 0, + .errorCode = StatusCode::ACCESS_DENIED, + }, + { + .propId = GLOBAL_CONTINUOUS_PROP, + .areaId = 0, + .errorCode = StatusCode::INVALID_ARG, + }, + }; + getHardware()->sendOnPropertySetErrorEvent(errorEvents); + + ASSERT_EQ(getCallback()->countOnPropertySetErrorResults(), 1u); + auto maybeVehiclePropErrors = getCallback()->nextOnPropertySetErrorResults(); + ASSERT_TRUE(maybeVehiclePropErrors.has_value()); + const auto& vehiclePropErrors = maybeVehiclePropErrors.value(); + ASSERT_THAT(vehiclePropErrors.payloads, UnorderedElementsAreArray(expectedResults)); +} + } // namespace vehicle } // namespace automotive } // namespace hardware diff --git a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.cpp b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.cpp index f51ce5cb39..54fede1f3d 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.cpp @@ -81,8 +81,14 @@ ScopedAStatus MockVehicleCallback::onPropertyEvent(const VehiclePropValues& resu return result; } -ScopedAStatus MockVehicleCallback::onPropertySetError(const VehiclePropErrors&) { - return ScopedAStatus::ok(); +ScopedAStatus MockVehicleCallback::onPropertySetError(const VehiclePropErrors& results) { + ScopedAStatus result; + { + std::scoped_lock lockGuard(mLock); + result = storeResults(results, &mOnPropertySetErrorResults); + } + mCond.notify_all(); + return result; } std::optional MockVehicleCallback::nextGetValueResults() { @@ -105,6 +111,16 @@ size_t MockVehicleCallback::countOnPropertyEventResults() { return mOnPropertyEventResults.size(); } +std::optional MockVehicleCallback::nextOnPropertySetErrorResults() { + std::scoped_lock lockGuard(mLock); + return pop(mOnPropertySetErrorResults); +} + +size_t MockVehicleCallback::countOnPropertySetErrorResults() { + std::scoped_lock lockGuard(mLock); + return mOnPropertySetErrorResults.size(); +} + bool MockVehicleCallback::waitForSetValueResults(size_t size, size_t timeoutInNano) { std::unique_lock lk(mLock); return mCond.wait_for(lk, std::chrono::nanoseconds(timeoutInNano), [this, size] { diff --git a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.h b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.h index f17b273b18..1545eae08f 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.h +++ b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleCallback.h @@ -63,6 +63,9 @@ class MockVehicleCallback final nextSetValueResults(); std::optional nextOnPropertyEventResults(); + size_t countOnPropertySetErrorResults(); + std::optional + nextOnPropertySetErrorResults(); size_t countOnPropertyEventResults(); bool waitForSetValueResults(size_t size, size_t timeoutInNano); bool waitForGetValueResults(size_t size, size_t timeoutInNano); @@ -77,6 +80,8 @@ class MockVehicleCallback final std::list mOnPropertyEventResults GUARDED_BY(mLock); int32_t mSharedMemoryFileCount GUARDED_BY(mLock); + std::list + mOnPropertySetErrorResults GUARDED_BY(mLock); }; } // namespace vehicle diff --git a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp index 4df4e1aea5..ba0d33dfde 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.cpp @@ -131,8 +131,9 @@ void MockVehicleHardware::registerOnPropertyChangeEvent( } void MockVehicleHardware::registerOnPropertySetErrorEvent( - std::unique_ptr) { - // TODO(b/200737967): mock this. + std::unique_ptr callback) { + std::scoped_lock lockGuard(mLock); + mPropertySetErrorCallback = std::move(callback); } void MockVehicleHardware::setPropertyConfigs(const std::vector& configs) { @@ -254,6 +255,12 @@ template StatusCode MockVehicleHardware::handleRequestsLocked>* storedRequests, std::list>* storedResponses) const; +void MockVehicleHardware::sendOnPropertySetErrorEvent( + const std::vector& errorEvents) { + std::scoped_lock lockGuard(mLock); + (*mPropertySetErrorCallback)(errorEvents); +} + } // 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 743841c216..46b30b9405 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.h +++ b/automotive/vehicle/aidl/impl/vhal/test/MockVehicleHardware.h @@ -85,6 +85,7 @@ class MockVehicleHardware final : public IVehicleHardware { aidl::android::hardware::automotive::vehicle::StatusCode status); void setSleepTime(int64_t timeInNano); void setDumpResult(DumpResult result); + void sendOnPropertySetErrorEvent(const std::vector& errorEvents); private: mutable std::mutex mLock; @@ -104,6 +105,7 @@ class MockVehicleHardware final : public IVehicleHardware { mStatusByFunctions GUARDED_BY(mLock); int64_t mSleepTime GUARDED_BY(mLock) = 0; std::unique_ptr mPropertyChangeCallback GUARDED_BY(mLock); + std::unique_ptr mPropertySetErrorCallback GUARDED_BY(mLock); std::function, const std::vector&)>