Add EventMode to VehiclePropertyStore writeValue.

Add an option to specify whether to trigger onpropertychange callback
when VehiclePropertyStore.writeValue is called.

Test: atest VehiclePropertyStoreTest
Bug: 237318964
Change-Id: Iefd572c96f67dab2ecd5de56acf2e0d1c9b58939

4 files changed, 110 insertions, 15 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 b64c1a65cc..20c34aa12e 100644
--- a/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp
+++ b/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp
@@ -217,17 +217,16 @@ VhalResult<void> FakeVehicleHardware::setApPowerStateReport(const VehiclePropVal
             [[fallthrough]];
         case toInt(VehicleApPowerStateReport::WAIT_FOR_VHAL):
             // CPMS is in WAIT_FOR_VHAL state, simply move to ON and send back to HAL.
-            // Must erase existing state because in the case when Car Service crashes, the power
-            // state would already be ON when we receive WAIT_FOR_VHAL and thus new property change
-            // event would be generated. However, Car Service always expect a property change event
-            // even though there is not actual state change.
-            mServerSidePropStore->removeValuesForProperty(
-                    toInt(VehicleProperty::AP_POWER_STATE_REQ));
             prop = createApPowerStateReq(VehicleApPowerStateReq::ON);
 
-            // ALWAYS update status for generated property value
+            // ALWAYS update status for generated property value, and force a property update event
+            // because in the case when Car Service crashes, the power state would already be ON
+            // when we receive WAIT_FOR_VHAL and thus new property change event would be generated.
+            // However, Car Service always expect a property change event even though there is no
+            // actual state change.
             if (auto writeResult =
-                        mServerSidePropStore->writeValue(std::move(prop), /*updateStatus=*/true);
+                        mServerSidePropStore->writeValue(std::move(prop), /*updateStatus=*/true,
+                                                         VehiclePropertyStore::EventMode::ALWAYS);
                 !writeResult.ok()) {
                 return StatusError(getErrorCode(writeResult))
                        << "failed to write AP_POWER_STATE_REQ into property store, error: "
@@ -894,10 +893,10 @@ StatusCode FakeVehicleHardware::updateSampleRate(int32_t propId, int32_t areaId,
             return;
         }
         result.value()->timestamp = elapsedRealtimeNano();
-        // Must remove the value before writing, otherwise, we would generate no update event since
-        // the value is the same.
-        mServerSidePropStore->removeValue(*result.value());
-        mServerSidePropStore->writeValue(std::move(result.value()));
+        // For continuous properties, we must generate a new onPropertyChange event periodically
+        // according to the sample rate.
+        mServerSidePropStore->writeValue(std::move(result.value()), /*updateStatus=*/true,
+                                         VehiclePropertyStore::EventMode::ALWAYS);
     });
     mRecurrentTimer->registerTimerCallback(interval, action);
     mRecurrentActions[propIdAreaId] = action;
diff --git a/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h b/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h
index ddc4f684aa..3d25cd3a41 100644
--- a/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h
+++ b/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h
@@ -46,6 +46,33 @@ class VehiclePropertyStore final {
     using ValueResultType = VhalResult<VehiclePropValuePool::RecyclableType>;
     using ValuesResultType = VhalResult<std::vector<VehiclePropValuePool::RecyclableType>>;
 
+    enum class EventMode : uint8_t {
+        /**
+         * Only invoke OnValueChangeCallback if the new property value (ignoring timestamp) is
+         * different than the existing value.
+         *
+         * This should be used for regular cases.
+         */
+        ON_VALUE_CHANGE,
+        /**
+         * Always invoke OnValueChangeCallback.
+         *
+         * This should be used for the special properties that are used for delivering event, e.g.
+         * HW_KEY_INPUT.
+         */
+        ALWAYS,
+        /**
+         * Never invoke OnValueChangeCallback.
+         *
+         * This should be used for continuous property subscription when the sample rate for the
+         * subscription is smaller than the refresh rate for the property. E.g., the vehicle speed
+         * is refreshed at 20hz, but we are only subscribing at 10hz. In this case, we want to
+         * generate the property change event at 10hz, not 20hz, but we still want to refresh the
+         * timestamp (via writeValue) at 20hz.
+         */
+        NEVER,
+    };
+
     explicit VehiclePropertyStore(std::shared_ptr<VehiclePropValuePool> valuePool)
         : mValuePool(valuePool) {}
 
@@ -72,8 +99,10 @@ class VehiclePropertyStore final {
     // 'status' would be initialized to {@code VehiclePropertyStatus::AVAILABLE}, if this is to
     // override an existing value, the status for the existing value would be used for the
     // overridden value.
+    // 'EventMode' controls whether the 'OnValueChangeCallback' will be called for this operation.
     VhalResult<void> writeValue(VehiclePropValuePool::RecyclableType propValue,
-                                bool updateStatus = false);
+                                bool updateStatus = false,
+                                EventMode mode = EventMode::ON_VALUE_CHANGE);
 
     // Remove a given property value from the property store. The 'propValue' would be used to
     // generate the key for the value to remove.
diff --git a/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp b/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp
index c8fb994684..646dc0e618 100644
--- a/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp
+++ b/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp
@@ -106,7 +106,8 @@ void VehiclePropertyStore::registerProperty(const VehiclePropConfig& config,
 }
 
 VhalResult<void> VehiclePropertyStore::writeValue(VehiclePropValuePool::RecyclableType propValue,
-                                                  bool updateStatus) {
+                                                  bool updateStatus,
+                                                  VehiclePropertyStore::EventMode eventMode) {
     std::scoped_lock<std::mutex> g(mLock);
 
     int32_t propId = propValue->prop;
@@ -145,7 +146,12 @@ VhalResult<void> VehiclePropertyStore::writeValue(VehiclePropValuePool::Recyclab
     }
 
     record->values[recId] = std::move(propValue);
-    if (valueUpdated && mOnValueChangeCallback != nullptr) {
+
+    if (eventMode == EventMode::NEVER) {
+        return {};
+    }
+
+    if ((eventMode == EventMode::ALWAYS || valueUpdated) && mOnValueChangeCallback != nullptr) {
         mOnValueChangeCallback(*(record->values[recId]));
     }
     return {};
diff --git a/automotive/vehicle/aidl/impl/utils/common/test/VehiclePropertyStoreTest.cpp b/automotive/vehicle/aidl/impl/utils/common/test/VehiclePropertyStoreTest.cpp
index 4d6f811795..fea5034db9 100644
--- a/automotive/vehicle/aidl/impl/utils/common/test/VehiclePropertyStoreTest.cpp
+++ b/automotive/vehicle/aidl/impl/utils/common/test/VehiclePropertyStoreTest.cpp
@@ -448,6 +448,67 @@ TEST_F(VehiclePropertyStoreTest, testPropertyChangeCallbackNoUpdate) {
     ASSERT_EQ(updatedValue.prop, INVALID_PROP_ID);
 }
 
+TEST_F(VehiclePropertyStoreTest, testPropertyChangeCallbackNoUpdateForTimestampChange) {
+    VehiclePropValue updatedValue{
+            .prop = INVALID_PROP_ID,
+    };
+    VehiclePropValue fuelCapacity = {
+            .prop = toInt(VehicleProperty::INFO_FUEL_CAPACITY),
+            .value = {.floatValues = {1.0}},
+    };
+    ASSERT_RESULT_OK(mStore->writeValue(mValuePool->obtain(fuelCapacity)));
+
+    mStore->setOnValueChangeCallback(
+            [&updatedValue](const VehiclePropValue& value) { updatedValue = value; });
+
+    // Write the same value with different timestamp should succeed but should not trigger callback.
+    fuelCapacity.timestamp = 1;
+    ASSERT_RESULT_OK(mStore->writeValue(mValuePool->obtain(fuelCapacity)));
+
+    ASSERT_EQ(updatedValue.prop, INVALID_PROP_ID);
+}
+
+TEST_F(VehiclePropertyStoreTest, testPropertyChangeCallbackForceUpdate) {
+    VehiclePropValue updatedValue{
+            .prop = INVALID_PROP_ID,
+    };
+    VehiclePropValue fuelCapacity = {
+            .prop = toInt(VehicleProperty::INFO_FUEL_CAPACITY),
+            .value = {.floatValues = {1.0}},
+    };
+    ASSERT_RESULT_OK(mStore->writeValue(mValuePool->obtain(fuelCapacity)));
+
+    mStore->setOnValueChangeCallback(
+            [&updatedValue](const VehiclePropValue& value) { updatedValue = value; });
+
+    fuelCapacity.timestamp = 1;
+    ASSERT_RESULT_OK(mStore->writeValue(mValuePool->obtain(fuelCapacity), /*updateStatus=*/false,
+                                        VehiclePropertyStore::EventMode::ALWAYS));
+
+    ASSERT_EQ(updatedValue, fuelCapacity);
+}
+
+TEST_F(VehiclePropertyStoreTest, testPropertyChangeCallbackForceNoUpdate) {
+    VehiclePropValue updatedValue{
+            .prop = INVALID_PROP_ID,
+    };
+    VehiclePropValue fuelCapacity = {
+            .prop = toInt(VehicleProperty::INFO_FUEL_CAPACITY),
+            .value = {.floatValues = {1.0}},
+    };
+    ASSERT_RESULT_OK(mStore->writeValue(mValuePool->obtain(fuelCapacity)));
+
+    mStore->setOnValueChangeCallback(
+            [&updatedValue](const VehiclePropValue& value) { updatedValue = value; });
+    fuelCapacity.value.floatValues[0] = 2.0;
+    fuelCapacity.timestamp = 1;
+
+    ASSERT_RESULT_OK(mStore->writeValue(mValuePool->obtain(fuelCapacity), /*updateStatus=*/false,
+                                        VehiclePropertyStore::EventMode::NEVER));
+
+    ASSERT_EQ(updatedValue.prop, INVALID_PROP_ID);
+}
+
 }  // namespace vehicle
 }  // namespace automotive
 }  // namespace hardware