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

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);
 }

thermal/aidl/default/Thermal.cpp

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#define LOG_TAG "thermal_service_example"
+
 #include "Thermal.h"
 
 #include <android-base/logging.h>
@@ -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<CoolingDevice>* /* out_devices */) {
     LOG(VERBOSE) << __func__;
     return ScopedAStatus::ok();
@@ -61,12 +75,20 @@ ScopedAStatus Thermal::registerThermalChangedCallback(
         const std::shared_ptr<IThermalChangedCallback>& 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<std::mutex> _lock(thermal_callback_mutex_);
+        if (std::any_of(thermal_callbacks_.begin(), thermal_callbacks_.end(),
+                        [&](const std::shared_ptr<IThermalChangedCallback>& 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<int32_t>(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<std::mutex> _lock(thermal_callback_mutex_);
+        if (std::any_of(thermal_callbacks_.begin(), thermal_callbacks_.end(),
+                        [&](const std::shared_ptr<IThermalChangedCallback>& 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<IThermalChangedCallback>& 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<std::mutex> _lock(thermal_callback_mutex_);
+        bool removed = false;
+        thermal_callbacks_.erase(
+                std::remove_if(thermal_callbacks_.begin(), thermal_callbacks_.end(),
+                               [&](const std::shared_ptr<IThermalChangedCallback>& 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();
 }
 

thermal/aidl/default/Thermal.h

@@ -54,7 +54,8 @@ class Thermal : public BnThermal {
             const std::shared_ptr<IThermalChangedCallback>& in_callback) override;
 
   private:
-    std::set<std::shared_ptr<IThermalChangedCallback>> mCallbacks;
+    std::mutex thermal_callback_mutex_;
+    std::vector<std::shared_ptr<IThermalChangedCallback>> thermal_callbacks_;
 };
 
 }  // namespace example

thermal/aidl/default/main.cpp

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#define LOG_TAG "thermal_service_example"
+
 #include "Thermal.h"
 
 #include <android-base/logging.h>

thermal/aidl/vts/VtsHalThermalTargetTest.cpp

@@ -95,21 +95,21 @@ class ThermalAidlTest : public testing::TestWithParam<std::string> {
 
         mThermalCallback = ndk::SharedRefBase::make<ThermalCallback>();
         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: