Fix post-review comments on Q quality utils from project-chip#34266 (p…

…roject-chip#34416) * Fix post-review comments on Q quality utils from project-chip#34266 - PR project-chip#34266 had post review comments See: project-chip#34266 (review) - Fix 0->0 case - Introduce `Timestamp` more places where it makes sense - Simplify some code lines Fixes project-chip#34334 Testing done: - Added unit test for 0->0 - Tests still pass * Restyled by clang-format * Address review comments * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]>
j-ororke · Jul 31, 2024 · 8d0c9b4 · 8d0c9b4
1 parent eace30a
commit 8d0c9b4
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 28 deletions.
diff --git a/src/app/cluster-building-blocks/QuieterReporting.h b/src/app/cluster-building-blocks/QuieterReporting.h
@@ -142,15 +142,6 @@ class QuieterReportingAttribute
         };
     }
 
-    /**
-     * @brief Factory to generate a functor that forces reportable now.
-     * @return a functor usable for the `changedPredicate` arg of `SetValue()`
-     */
-    static SufficientChangePredicate GetForceReportablePredicate()
-    {
-        return [](const SufficientChangePredicateCandidate & candidate) -> bool { return true; };
-    }
-
     Nullable<T> value() const { return mValue; }
     QuieterReportingPolicyFlags & policy() { return mPolicyFlags; }
     const QuieterReportingPolicyFlags & policy() const { return mPolicyFlags; }
@@ -163,6 +154,7 @@ class QuieterReportingAttribute
      * - The policies from `QuieterReportingPolicyEnum` and set via `SetPolicy()` are self-explanatory by name.
      * - The changedPredicate will be called with last dirty <timestamp, value> and new <timestamp value> and may override
      *   the dirty state altogether when it returns true. Use sparingly and default to a functor returning false.
+     *   The changedPredicate is only called on change.
      *
      * Internal recording will be done about last dirty value and last dirty timestamp based on the policies having applied.
      *
@@ -172,13 +164,13 @@ class QuieterReportingAttribute
      * @return AttributeDirtyState::kMustReport if attribute must be marked dirty right away, or
      * AttributeDirtyState::kNoReportNeeded otherwise.
      */
-    AttributeDirtyState SetValue(const chip::app::DataModel::Nullable<T> & newValue, Timestamp now,
-                                 SufficientChangePredicate changedPredicate)
+    AttributeDirtyState SetValue(const DataModel::Nullable<T> & newValue, Timestamp now, SufficientChangePredicate changedPredicate)
     {
-        bool isChangeOfNull       = newValue.IsNull() ^ mValue.IsNull();
-        bool areBothValuesNonNull = !newValue.IsNull() && !mValue.IsNull();
+        bool isChangeOfNull         = newValue.IsNull() != mValue.IsNull();
+        bool areBothValuesNonNull   = !newValue.IsNull() && !mValue.IsNull();
+        bool areBothValuesDifferent = areBothValuesNonNull && (newValue.Value() != mValue.Value());
 
-        bool changeToFromZero = areBothValuesNonNull && (newValue.Value() == 0 || mValue.Value() == 0);
+        bool changeToFromZero = areBothValuesNonNull && areBothValuesDifferent && (newValue.Value() == 0 || mValue.Value() == 0);
         bool isIncrement      = areBothValuesNonNull && (newValue.Value() > mValue.Value());
         bool isDecrement      = areBothValuesNonNull && (newValue.Value() < mValue.Value());
 
@@ -188,13 +180,17 @@ class QuieterReportingAttribute
         isNewlyDirty = isNewlyDirty || (mPolicyFlags.Has(QuieterReportingPolicyEnum::kMarkDirtyOnDecrement) && isDecrement);
         isNewlyDirty = isNewlyDirty || (mPolicyFlags.Has(QuieterReportingPolicyEnum::kMarkDirtyOnIncrement) && isIncrement);
 
-        SufficientChangePredicateCandidate candidate{
-            mLastDirtyTimestampMillis, // lastDirtyTimestamp
-            now,                       // nowTimestamp
-            mLastDirtyValue,           // lastDirtyValue
-            newValue                   // newValue
-        };
-        isNewlyDirty = isNewlyDirty || changedPredicate(candidate);
+        // Only execute predicate on value change from last marked dirty.
+        if (newValue != mLastDirtyValue)
+        {
+            SufficientChangePredicateCandidate candidate{
+                mLastDirtyTimestampMillis, // lastDirtyTimestamp
+                now,                       // nowTimestamp
+                mLastDirtyValue,           // lastDirtyValue
+                newValue                   // newValue
+            };
+            isNewlyDirty = isNewlyDirty || changedPredicate(candidate);
+        }
 
         mValue = newValue;
 
@@ -217,20 +213,20 @@ class QuieterReportingAttribute
      * @return AttributeDirtyState::kMustReport if attribute must be marked dirty right away, or
      * AttributeDirtyState::kNoReportNeeded otherwise.
      */
-    AttributeDirtyState SetValue(const chip::app::DataModel::Nullable<T> & newValue, Timestamp now)
+    AttributeDirtyState SetValue(const DataModel::Nullable<T> & newValue, Timestamp now)
     {
         return SetValue(newValue, now, [](const SufficientChangePredicateCandidate &) -> bool { return false; });
     }
 
 protected:
     // Current value of the attribute.
-    chip::app::DataModel::Nullable<T> mValue;
+    DataModel::Nullable<T> mValue;
     // Last value that was marked as dirty (to use in comparisons for change, e.g. by SufficientChangePredicate).
-    chip::app::DataModel::Nullable<T> mLastDirtyValue;
+    DataModel::Nullable<T> mLastDirtyValue;
     // Enabled internal change detection policies.
     QuieterReportingPolicyFlags mPolicyFlags{ 0 };
     // Timestamp associated with the last time the attribute was marked dirty (to use in comparisons for change).
-    chip::System::Clock::Milliseconds64 mLastDirtyTimestampMillis{};
+    chip::System::Clock::Timestamp mLastDirtyTimestampMillis{};
 };
 
 } // namespace detail

diff --git a/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp b/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp
@@ -74,6 +74,10 @@ TEST(TestQuieterReporting, ChangeToFromZeroPolicyWorks)
     EXPECT_EQ(attribute.SetValue(0, now), AttributeDirtyState::kMustReport);
     EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 0);
 
+    // 0 --> 0, expect NOT marked dirty.
+    EXPECT_EQ(attribute.SetValue(0, now), AttributeDirtyState::kNoReportNeeded);
+    EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 0);
+
     // 0 --> 11, expect marked dirty.
     EXPECT_EQ(attribute.SetValue(11, now), AttributeDirtyState::kMustReport);
     EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 11);
@@ -209,9 +213,6 @@ TEST(TestQuieterReporting, SufficientChangePredicateWorks)
     EXPECT_EQ(attribute.SetValue(10, now), AttributeDirtyState::kNoReportNeeded);
     EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 10);
 
-    // Forcing dirty can be done with a force-true predicate
-    EXPECT_EQ(attribute.SetValue(10, now, attribute.GetForceReportablePredicate()), AttributeDirtyState::kMustReport);
-
     auto predicate = attribute.GetPredicateForSufficientTimeSinceLastDirty(1000_ms);
 
     now = fakeClock.Advance(100_ms);
@@ -250,6 +251,14 @@ TEST(TestQuieterReporting, SufficientChangePredicateWorks)
     EXPECT_EQ(attribute.SetValue(14, now, predicate), AttributeDirtyState::kNoReportNeeded);
     EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 14);
 
+    // Forcing dirty can NOT done with a force-true predicate.
+    decltype(attribute)::SufficientChangePredicate forceTruePredicate{
+        [](const decltype(attribute)::SufficientChangePredicateCandidate &) -> bool { return true; }
+    };
+    now = fakeClock.Advance(1_ms);
+    EXPECT_EQ(attribute.SetValue(12, now, forceTruePredicate), AttributeDirtyState::kNoReportNeeded);
+    EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 12);
+
     // Change to a value that marks dirty no matter what (e.g. null). Should be dirty even
     // before delay.
     now = fakeClock.Advance(1_ms);