From d43e87345f50ae811f530ef9fb8e079ac69d87a7 Mon Sep 17 00:00:00 2001 From: Xiang Wang Date: Tue, 7 Feb 2023 12:10:33 -0800 Subject: [PATCH] Fix the Thermal AIDL example and VTS test The AIDL proxy objects can't be compared directly but their internal IBinder Bug: b/264224315 Test: atest VtsHalThermalTargetTest Change-Id: Icf6174a0ba79fa5efeaec7778d27c18d957cd88d --- .../android/hardware/thermal/IThermal.aidl | 60 +++++++--------- thermal/aidl/default/Thermal.cpp | 70 +++++++++++++++---- thermal/aidl/default/Thermal.h | 3 +- thermal/aidl/default/main.cpp | 2 + thermal/aidl/vts/VtsHalThermalTargetTest.cpp | 20 +++--- 5 files changed, 95 insertions(+), 60 deletions(-) diff --git a/thermal/aidl/android/hardware/thermal/IThermal.aidl b/thermal/aidl/android/hardware/thermal/IThermal.aidl index 7c23c1712c..c94edcda72 100644 --- a/thermal/aidl/android/hardware/thermal/IThermal.aidl +++ b/thermal/aidl/android/hardware/thermal/IThermal.aidl @@ -36,9 +36,8 @@ interface IThermal { * exist on boot. The method always returns and never removes from * the list such cooling devices. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, getMessage() must be populated with the human-readable - * error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ CoolingDevice[] getCoolingDevices(); @@ -54,9 +53,8 @@ interface IThermal { * exist on boot. The method always returns and never removes from * the list such cooling devices. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ CoolingDevice[] getCoolingDevicesWithType(in CoolingType type); @@ -70,9 +68,8 @@ interface IThermal { * they go offline, if these devices exist on boot. The method * always returns and never removes such temperatures. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ Temperature[] getTemperatures(); @@ -88,9 +85,8 @@ interface IThermal { * they go offline, if these devices exist on boot. The method * always returns and never removes such temperatures. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ Temperature[] getTemperaturesWithType(in TemperatureType type); @@ -110,9 +106,8 @@ interface IThermal { * throttling status, use getTemperatures or registerThermalChangedCallback * and listen to the callback. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ TemperatureThreshold[] getTemperatureThresholds(); @@ -135,9 +130,8 @@ interface IThermal { * throttling status, use getTemperatures or registerThermalChangedCallback * and listen to the callback. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ TemperatureThreshold[] getTemperatureThresholdsWithType(in TemperatureType type); @@ -152,12 +146,10 @@ interface IThermal { * thermal events. if nullptr callback is given, the status code will be * STATUS_BAD_VALUE and the operation will fail. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. If callback is given nullptr, the returned status code - * will be STATUS_BAD_VALUE and the exception will be EX_ILLEGAL_ARGUMENT. - * if callback is already registered, the returned status code will be - * STATUS_INVALID_OPERATION, the exception will be EX_ILLEGAL_ARGUMENT. + * @throws EX_ILLEGAL_ARGUMENT If the callback is given nullptr or already registered. And the + * getMessage() must be populated with human-readable error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ void registerThermalChangedCallback(in IThermalChangedCallback callback); @@ -174,12 +166,10 @@ interface IThermal { * STATUS_BAD_VALUE and the operation will fail. * @param type the type to be filtered. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. If callback is given nullptr, the returned status code - * will be STATUS_BAD_VALUE and the exception will be EX_ILLEGAL_ARGUMENT. - * if callback is already registered, the returned status code will be - * STATUS_INVALID_OPERATION, the exception will be EX_ILLEGAL_ARGUMENT. + * @throws EX_ILLEGAL_ARGUMENT If the callback is given nullptr or already registered. And the + * getMessage() must be populated with human-readable error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ void registerThermalChangedCallbackWithType( in IThermalChangedCallback callback, in TemperatureType type); @@ -192,12 +182,10 @@ interface IThermal { * thermal events. if nullptr callback is given, the status code will be * STATUS_BAD_VALUE and the operation will fail. * - * @throws ScopedAStatus Status of the operation. If status code is not - * STATUS_OK, the getMessage() must be populated with the human-readable - * error message. If callback is given nullptr, the returned status code - * will be STATUS_BAD_VALUE and the exception will be EX_ILLEGAL_ARGUMENT. - * if callback is not registered, the returned status code will be - * STATUS_INVALID_OPERATION, the exception will be EX_ILLEGAL_ARGUMENT. + * @throws EX_ILLEGAL_ARGUMENT If the callback is given nullptr or not previously registered. + * And the getMessage() must be populated with human-readable error message. + * @throws EX_ILLEGAL_STATE If the Thermal HAL is not initialized successfully. And the + * getMessage() must be populated with human-readable error message. */ void unregisterThermalChangedCallback(in IThermalChangedCallback callback); } diff --git a/thermal/aidl/default/Thermal.cpp b/thermal/aidl/default/Thermal.cpp index 5771e0edcd..f643d22f1f 100644 --- a/thermal/aidl/default/Thermal.cpp +++ b/thermal/aidl/default/Thermal.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#define LOG_TAG "thermal_service_example" + #include "Thermal.h" #include @@ -22,6 +24,18 @@ namespace aidl::android::hardware::thermal::impl::example { using ndk::ScopedAStatus; +namespace { + +bool interfacesEqual(const std::shared_ptr<::ndk::ICInterface>& left, + const std::shared_ptr<::ndk::ICInterface>& right) { + if (left == nullptr || right == nullptr || !left->isRemote() || !right->isRemote()) { + return left == right; + } + return left->asBinder() == right->asBinder(); +} + +} // namespace + ScopedAStatus Thermal::getCoolingDevices(std::vector* /* out_devices */) { LOG(VERBOSE) << __func__; return ScopedAStatus::ok(); @@ -61,12 +75,20 @@ ScopedAStatus Thermal::registerThermalChangedCallback( const std::shared_ptr& in_callback) { LOG(VERBOSE) << __func__ << " IThermalChangedCallback: " << in_callback; if (in_callback == nullptr) { - return ScopedAStatus::fromStatus(STATUS_BAD_VALUE); + return ndk::ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "Invalid nullptr callback"); } - if (mCallbacks.find(in_callback) != mCallbacks.end()) { - return ScopedAStatus::fromStatus(STATUS_INVALID_OPERATION); + { + std::lock_guard _lock(thermal_callback_mutex_); + if (std::any_of(thermal_callbacks_.begin(), thermal_callbacks_.end(), + [&](const std::shared_ptr& c) { + return interfacesEqual(c, in_callback); + })) { + return ndk::ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "Callback already registered"); + } + thermal_callbacks_.push_back(in_callback); } - mCallbacks.insert(in_callback); return ScopedAStatus::ok(); } @@ -75,26 +97,48 @@ ScopedAStatus Thermal::registerThermalChangedCallbackWithType( LOG(VERBOSE) << __func__ << " IThermalChangedCallback: " << in_callback << ", TemperatureType: " << static_cast(in_type); if (in_callback == nullptr) { - return ScopedAStatus::fromStatus(STATUS_BAD_VALUE); + return ndk::ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "Invalid nullptr callback"); } - if (mCallbacks.find(in_callback) != mCallbacks.end()) { - return ScopedAStatus::fromStatus(STATUS_INVALID_OPERATION); + { + std::lock_guard _lock(thermal_callback_mutex_); + if (std::any_of(thermal_callbacks_.begin(), thermal_callbacks_.end(), + [&](const std::shared_ptr& c) { + return interfacesEqual(c, in_callback); + })) { + return ndk::ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "Callback already registered"); + } + thermal_callbacks_.push_back(in_callback); } - mCallbacks.insert(in_callback); return ScopedAStatus::ok(); } ScopedAStatus Thermal::unregisterThermalChangedCallback( const std::shared_ptr& in_callback) { LOG(VERBOSE) << __func__ << " IThermalChangedCallback: " << in_callback; - bool found = false; if (in_callback == nullptr) { - return ScopedAStatus::fromStatus(STATUS_BAD_VALUE); + return ndk::ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "Invalid nullptr callback"); } - if (mCallbacks.find(in_callback) == mCallbacks.end()) { - return ScopedAStatus::fromStatus(STATUS_INVALID_OPERATION); + { + std::lock_guard _lock(thermal_callback_mutex_); + bool removed = false; + thermal_callbacks_.erase( + std::remove_if(thermal_callbacks_.begin(), thermal_callbacks_.end(), + [&](const std::shared_ptr& c) { + if (interfacesEqual(c, in_callback)) { + removed = true; + return true; + } + return false; + }), + thermal_callbacks_.end()); + if (!removed) { + return ndk::ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "Callback wasn't registered"); + } } - mCallbacks.erase(in_callback); return ScopedAStatus::ok(); } diff --git a/thermal/aidl/default/Thermal.h b/thermal/aidl/default/Thermal.h index 788af4aae1..8885e631fe 100644 --- a/thermal/aidl/default/Thermal.h +++ b/thermal/aidl/default/Thermal.h @@ -54,7 +54,8 @@ class Thermal : public BnThermal { const std::shared_ptr& in_callback) override; private: - std::set> mCallbacks; + std::mutex thermal_callback_mutex_; + std::vector> thermal_callbacks_; }; } // namespace example diff --git a/thermal/aidl/default/main.cpp b/thermal/aidl/default/main.cpp index 61d8ad091c..9f4ddb8b50 100644 --- a/thermal/aidl/default/main.cpp +++ b/thermal/aidl/default/main.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#define LOG_TAG "thermal_service_example" + #include "Thermal.h" #include diff --git a/thermal/aidl/vts/VtsHalThermalTargetTest.cpp b/thermal/aidl/vts/VtsHalThermalTargetTest.cpp index b93250e9d1..73c5dd2c8d 100644 --- a/thermal/aidl/vts/VtsHalThermalTargetTest.cpp +++ b/thermal/aidl/vts/VtsHalThermalTargetTest.cpp @@ -95,21 +95,21 @@ class ThermalAidlTest : public testing::TestWithParam { mThermalCallback = ndk::SharedRefBase::make(); ASSERT_NE(mThermalCallback, nullptr); - auto ret = mThermal->registerThermalChangedCallback(mThermalCallback); - ASSERT_TRUE(ret.isOk()); + auto status = mThermal->registerThermalChangedCallback(mThermalCallback); + ASSERT_TRUE(status.isOk()); // Expect to fail if register again - ret = mThermal->registerThermalChangedCallback(mThermalCallback); - ASSERT_FALSE(ret.isOk()); - ASSERT_TRUE(ret.getStatus() == STATUS_INVALID_OPERATION); + status = mThermal->registerThermalChangedCallback(mThermalCallback); + ASSERT_FALSE(status.isOk()); + ASSERT_EQ(EX_ILLEGAL_ARGUMENT, status.getExceptionCode()); } void TearDown() override { - auto ret = mThermal->unregisterThermalChangedCallback(mThermalCallback); - ASSERT_TRUE(ret.isOk()); + auto status = mThermal->unregisterThermalChangedCallback(mThermalCallback); + ASSERT_TRUE(status.isOk()); // Expect to fail if unregister again - ret = mThermal->unregisterThermalChangedCallback(mThermalCallback); - ASSERT_FALSE(ret.isOk()); - ASSERT_TRUE(ret.getStatus() == STATUS_INVALID_OPERATION); + status = mThermal->unregisterThermalChangedCallback(mThermalCallback); + ASSERT_FALSE(status.isOk()); + ASSERT_EQ(EX_ILLEGAL_ARGUMENT, status.getExceptionCode()); } protected: