Merge "Clean up connection to sensor hidl service"

5 files changed, 157 insertions, 32 deletions

diff --git a/services/sensorservice/Android.bp b/services/sensorservice/Android.bp
index 8d381b1c31..a7f3a5235e 100644
--- a/services/sensorservice/Android.bp
+++ b/services/sensorservice/Android.bp
@@ -14,6 +14,7 @@ cc_library_shared {
         "RecentEventLogger.cpp",
         "RotationVectorSensor.cpp",
         "SensorDevice.cpp",
+        "SensorDeviceUtils.cpp",
         "SensorDirectConnection.cpp",
         "SensorEventConnection.cpp",
         "SensorFusion.cpp",
diff --git a/services/sensorservice/SensorDevice.cpp b/services/sensorservice/SensorDevice.cpp
index da3b2758d1..fdb69d3ecc 100644
--- a/services/sensorservice/SensorDevice.cpp
+++ b/services/sensorservice/SensorDevice.cpp
@@ -26,11 +26,10 @@
 #include <cinttypes>
 #include <thread>
 
-using android::hardware::hidl_vec;
-
 using namespace android::hardware::sensors::V1_0;
 using namespace android::hardware::sensors::V1_0::implementation;
-
+using android::hardware::hidl_vec;
+using android::SensorDeviceUtils::HidlServiceRegistrationWaiter;
 
 namespace android {
 // ---------------------------------------------------------------------------
@@ -52,7 +51,8 @@ static status_t StatusFromResult(Result result) {
     }
 }
 
-SensorDevice::SensorDevice() : mHidlTransportErrors(20) {
+SensorDevice::SensorDevice()
+        : mHidlTransportErrors(20), mRestartWaiter(new HidlServiceRegistrationWaiter()) {
     if (!connectHidlService()) {
         return;
     }
@@ -87,35 +87,29 @@ SensorDevice::SensorDevice() : mHidlTransportErrors(20) {
 }
 
 bool SensorDevice::connectHidlService() {
-    // SensorDevice may wait upto 100ms * 10 = 1s for hidl service.
-    constexpr auto RETRY_DELAY = std::chrono::milliseconds(100);
+    // SensorDevice will wait for HAL service to start if HAL is declared in device manifest.
     size_t retry = 10;
 
-    while (true) {
-        int initStep = 0;
+    while (retry-- > 0) {
         mSensors = ISensors::getService();
-        if (mSensors != nullptr) {
-            ++initStep;
-            // Poke ISensor service. If it has lingering connection from previous generation of
-            // system server, it will kill itself. There is no intention to handle the poll result,
-            // which will be done since the size is 0.
-            if(mSensors->poll(0, [](auto, const auto &, const auto &) {}).isOk()) {
-                // ok to continue
-                break;
-            }
-            // hidl service is restarting, pointer is invalid.
-            mSensors = nullptr;
+        if (mSensors == nullptr) {
+            // no sensor hidl service found
+            break;
         }
 
-        if (--retry <= 0) {
-            ALOGE("Cannot connect to ISensors hidl service!");
+        mRestartWaiter->reset();
+        // Poke ISensor service. If it has lingering connection from previous generation of
+        // system server, it will kill itself. There is no intention to handle the poll result,
+        // which will be done since the size is 0.
+        if(mSensors->poll(0, [](auto, const auto &, const auto &) {}).isOk()) {
+            // ok to continue
             break;
         }
-        // Delay 100ms before retry, hidl service is expected to come up in short time after
-        // crash.
-        ALOGI("%s unsuccessful, try again soon (remaining retry %zu).",
-                (initStep == 0) ? "getService()" : "poll() check", retry);
-        std::this_thread::sleep_for(RETRY_DELAY);
+
+        // hidl service is restarting, pointer is invalid.
+        mSensors = nullptr;
+        ALOGI("%s unsuccessful, remaining retry %zu.", __FUNCTION__, retry);
+        mRestartWaiter->wait();
     }
     return (mSensors != nullptr);
 }
@@ -173,7 +167,7 @@ ssize_t SensorDevice::getSensorList(sensor_t const** list) {
 }
 
 status_t SensorDevice::initCheck() const {
-    return mSensors != NULL ? NO_ERROR : NO_INIT;
+    return mSensors != nullptr ? NO_ERROR : NO_INIT;
 }
 
 ssize_t SensorDevice::poll(sensors_event_t* buffer, size_t count) {
diff --git a/services/sensorservice/SensorDevice.h b/services/sensorservice/SensorDevice.h
index fd6cee67ea..6d7505158d 100644
--- a/services/sensorservice/SensorDevice.h
+++ b/services/sensorservice/SensorDevice.h
@@ -17,6 +17,7 @@
 #ifndef ANDROID_SENSOR_DEVICE_H
 #define ANDROID_SENSOR_DEVICE_H
 
+#include "SensorDeviceUtils.h"
 #include "SensorServiceUtils.h"
 
 #include <sensor/Sensor.h>
@@ -39,12 +40,9 @@
 namespace android {
 
 // ---------------------------------------------------------------------------
-using SensorServiceUtil::Dumpable;
-using hardware::Return;
 
-class SensorDevice : public Singleton<SensorDevice>, public Dumpable {
+class SensorDevice : public Singleton<SensorDevice>, public SensorServiceUtil::Dumpable {
 public:
-
     class HidlTransportErrorLog {
      public:
 
@@ -105,7 +103,7 @@ public:
 private:
     friend class Singleton<SensorDevice>;
 
-    sp<android::hardware::sensors::V1_0::ISensors> mSensors;
+    sp<hardware::sensors::V1_0::ISensors> mSensors;
     Vector<sensor_t> mSensorList;
     std::unordered_map<int32_t, sensor_t*> mConnectedDynamicSensors;
 
@@ -174,6 +172,8 @@ private:
         }
         return std::move(ret);
     }
+    //TODO(b/67425500): remove waiter after bug is resolved.
+    sp<SensorDeviceUtils::HidlServiceRegistrationWaiter> mRestartWaiter;
 
     bool isClientDisabled(void* ident);
     bool isClientDisabledLocked(void* ident);
diff --git a/services/sensorservice/SensorDeviceUtils.cpp b/services/sensorservice/SensorDeviceUtils.cpp
new file mode 100644
index 0000000000..b1344becd7
--- /dev/null
+++ b/services/sensorservice/SensorDeviceUtils.cpp
@@ -0,0 +1,70 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "SensorDeviceUtils.h"
+
+#include <android/hardware/sensors/1.0/ISensors.h>
+#include <utils/Log.h>
+
+#include <chrono>
+#include <thread>
+
+using ::android::hardware::Void;
+using namespace android::hardware::sensors::V1_0;
+
+namespace android {
+namespace SensorDeviceUtils {
+
+HidlServiceRegistrationWaiter::HidlServiceRegistrationWaiter()
+        : mRegistered(ISensors::registerForNotifications("default", this)) {
+}
+
+Return<void> HidlServiceRegistrationWaiter::onRegistration(
+        const hidl_string &fqName, const hidl_string &name, bool preexisting) {
+    ALOGV("onRegistration fqName %s, name %s, preexisting %d",
+          fqName.c_str(), name.c_str(), preexisting);
+
+    {
+        std::lock_guard<std::mutex> lk(mLock);
+        mRestartObserved = true;
+    }
+    mCondition.notify_all();
+    return Void();
+}
+
+void HidlServiceRegistrationWaiter::reset() {
+    std::lock_guard<std::mutex> lk(mLock);
+    mRestartObserved = false;
+}
+
+bool HidlServiceRegistrationWaiter::wait() {
+    constexpr int DEFAULT_WAIT_MS = 100;
+    constexpr int TIMEOUT_MS = 1000;
+
+    if (!mRegistered) {
+        ALOGW("Cannot register service notification, use default wait(%d ms)", DEFAULT_WAIT_MS);
+        std::this_thread::sleep_for(std::chrono::milliseconds(DEFAULT_WAIT_MS));
+        // not sure if service is actually restarted
+        return false;
+    }
+
+    std::unique_lock<std::mutex> lk(mLock);
+    return mCondition.wait_for(lk, std::chrono::milliseconds(TIMEOUT_MS),
+            [this]{return mRestartObserved;});
+}
+
+} // namespace SensorDeviceUtils
+} // namespace android
diff --git a/services/sensorservice/SensorDeviceUtils.h b/services/sensorservice/SensorDeviceUtils.h
new file mode 100644
index 0000000000..da36928105
--- /dev/null
+++ b/services/sensorservice/SensorDeviceUtils.h
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ANDROID_SENSOR_DEVICE_UTIL
+#define ANDROID_SENSOR_DEVICE_UTIL
+
+#include <android/hidl/manager/1.0/IServiceNotification.h>
+
+#include <condition_variable>
+#include <thread>
+
+using ::android::hardware::hidl_string;
+using ::android::hardware::Return;
+using ::android::hidl::manager::V1_0::IServiceNotification;
+
+namespace android {
+namespace SensorDeviceUtils {
+
+class HidlServiceRegistrationWaiter : public IServiceNotification {
+public:
+
+    HidlServiceRegistrationWaiter();
+
+    Return<void> onRegistration(const hidl_string &fqName,
+                                const hidl_string &name,
+                                bool preexisting) override;
+
+    void reset();
+
+    /**
+     * Wait for service restart
+     *
+     * @return true if service is restart since last reset(); false otherwise.
+     */
+    bool wait();
+private:
+    const bool mRegistered;
+
+    std::mutex mLock;
+    std::condition_variable mCondition;
+    bool mRestartObserved;
+};
+
+} // namespace SensorDeviceUtils
+} // namespace android;
+
+#endif // ANDROID_SENSOR_SERVICE_UTIL