From 19a774a9386937bca410f5303b0b18b5f92db028 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Fri, 19 Jul 2024 18:14:53 -0400 Subject: [PATCH] Fix post-review comments on Q quality utils from #34266 (#34416) * Fix post-review comments on Q quality utils from #34266 - PR #34266 had post review comments See: https://github.com/project-chip/connectedhomeip/pull/34266#pullrequestreview-2173994817 - Fix 0->0 case - Introduce `Timestamp` more places where it makes sense - Simplify some code lines Fixes #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 --- .../QuieterReporting.h | 46 +++++++++---------- .../tests/TestQuieterReporting.cpp | 15 ++++-- 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/src/app/cluster-building-blocks/QuieterReporting.h b/src/app/cluster-building-blocks/QuieterReporting.h index d44f12b0f2b0bf..bf6b4fe36346d2 100644 --- 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 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 and new 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 & newValue, Timestamp now, - SufficientChangePredicate changedPredicate) + AttributeDirtyState SetValue(const DataModel::Nullable & 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 & newValue, Timestamp now) + AttributeDirtyState SetValue(const DataModel::Nullable & newValue, Timestamp now) { return SetValue(newValue, now, [](const SufficientChangePredicateCandidate &) -> bool { return false; }); } protected: // Current value of the attribute. - chip::app::DataModel::Nullable mValue; + DataModel::Nullable mValue; // Last value that was marked as dirty (to use in comparisons for change, e.g. by SufficientChangePredicate). - chip::app::DataModel::Nullable mLastDirtyValue; + DataModel::Nullable 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 index a7750380285438..ad7800d556afa4 100644 --- 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);