commit fb4e33acfe0c432d19c1716d7d3405238ecd039e
author Anthony Stange <stange@google.com> Wed Sep 29 15:47:53 2021 +0000
committer Anthony Stange <stange@google.com> Thu Sep 30 17:23:24 2021 +0000
tree b9cb5e125d2f9423c8132f5a81818ad8e5293f4c
parent 979587e1f43c27557eb68c1d4031edd57100f553

Block dynamic connection event if sensor not found

A bug in the Sensors HIDL spec marks onDynamicSensorsConnected as oneway
which means that HALs are not blocked on the framework processing that
event. This means the onDynamicSensorsConnected invocation and dynamic
meta event can be received out of order. Blocking processing of the
event gives time for onDynamicSensorsConnected to be invoked so that the
event can be correctly processed.

Bug: 201529167
Test: Connect dynamic sensor and validate that the HAL can deliver the
event and method invocation without any sleeps on the HAL side.

Change-Id: Ie1cad22f26de60a7cecb3c6048280c4919165dbb

services/sensorservice/SensorDevice.cpp

diff --git a/services/sensorservice/SensorDevice.cpp b/services/sensorservice/SensorDevice.cpp
index c233bf0..e0511bf 100644
--- a/services/sensorservice/SensorDevice.cpp
+++ b/services/sensorservice/SensorDevice.cpp
@@ -614,6 +614,8 @@
 
 Return<void> SensorDevice::onDynamicSensorsConnected(
         const hidl_vec<SensorInfo> &dynamicSensorsAdded) {
+    std::unique_lock<std::mutex> lock(mDynamicSensorsMutex);
+
     // Allocate a sensor_t structure for each dynamic sensor added and insert
     // it into the dictionary of connected dynamic sensors keyed by handle.
     for (size_t i = 0; i < dynamicSensorsAdded.size(); ++i) {
@@ -629,6 +631,8 @@
                 std::make_pair(sensor->handle, sensor));
     }
 
+    mDynamicSensorsCv.notify_all();
+
     return Return<void>();
 }
 
@@ -1174,8 +1178,20 @@
         dst->dynamic_sensor_meta.connected = dyn.connected;
         dst->dynamic_sensor_meta.handle = dyn.sensorHandle;
         if (dyn.connected) {
+            std::unique_lock<std::mutex> lock(mDynamicSensorsMutex);
+            // Give MAX_DYN_SENSOR_WAIT_SEC for onDynamicSensorsConnected to be invoked since it
+            // can be received out of order from this event due to a bug in the HIDL spec that
+            // marks it as oneway.
             auto it = mConnectedDynamicSensors.find(dyn.sensorHandle);
-            CHECK(it != mConnectedDynamicSensors.end());
+            if (it == mConnectedDynamicSensors.end()) {
+                mDynamicSensorsCv.wait_for(lock, MAX_DYN_SENSOR_WAIT,
+                        [&, dyn]{
+                            return mConnectedDynamicSensors.find(dyn.sensorHandle)
+                                    != mConnectedDynamicSensors.end();
+                });
+                it = mConnectedDynamicSensors.find(dyn.sensorHandle);
+                CHECK(it != mConnectedDynamicSensors.end());
+            }
 
             dst->dynamic_sensor_meta.sensor = it->second;
 

services/sensorservice/SensorDevice.h

diff --git a/services/sensorservice/SensorDevice.h b/services/sensorservice/SensorDevice.h
index 75da7bb..bc8d20f 100644
--- a/services/sensorservice/SensorDevice.h
+++ b/services/sensorservice/SensorDevice.h
@@ -139,6 +139,14 @@
     Vector<sensor_t> mSensorList;
     std::unordered_map<int32_t, sensor_t*> mConnectedDynamicSensors;
 
+    // A bug in the Sensors HIDL spec which marks onDynamicSensorsConnected as oneway causes dynamic
+    // meta events and onDynamicSensorsConnected to be received out of order. This mutex + CV are
+    // used to block meta event processing until onDynamicSensorsConnected is received to simplify
+    // HAL implementations.
+    std::mutex mDynamicSensorsMutex;
+    std::condition_variable mDynamicSensorsCv;
+    static constexpr std::chrono::seconds MAX_DYN_SENSOR_WAIT{5};
+
     static const nsecs_t MINIMUM_EVENTS_PERIOD =   1000000; // 1000 Hz
     mutable Mutex mLock; // protect mActivationCount[].batchParams
     // fixed-size array after construction