Address review comments

project-chip · Jul 19, 2024 · 1946a0c · 1946a0c
1 parent 114c198
commit 1946a0c
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 19 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.
      *
@@ -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;
 

diff --git a/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp b/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp
@@ -213,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);
@@ -254,6 +251,12 @@ 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);
@@ -264,4 +267,6 @@ TEST(TestQuieterReporting, SufficientChangePredicateWorks)
     now = fakeClock.Advance(1000_ms);
     EXPECT_EQ(attribute.SetValue(NullNullable, now, predicate), AttributeDirtyState::kNoReportNeeded);
     EXPECT_TRUE(attribute.value().IsNull());
+
+
 }