From 1f801456a29ee581e83a1eece570c257d3a0904c Mon Sep 17 00:00:00 2001 From: "tennessee.carmelveilleux@gmail.com" Date: Wed, 10 Jul 2024 09:47:55 -0400 Subject: [PATCH] Address more review comments --- .../QuieterReporting.h | 111 ++++++++++++------ .../tests/TestQuieterReporting.cpp | 24 ++-- 2 files changed, 90 insertions(+), 45 deletions(-) diff --git a/src/app/cluster-building-blocks/QuieterReporting.h b/src/app/cluster-building-blocks/QuieterReporting.h index 9739feb7ae11b7..7de3b1644d5d57 100644 --- a/src/app/cluster-building-blocks/QuieterReporting.h +++ b/src/app/cluster-building-blocks/QuieterReporting.h @@ -19,6 +19,7 @@ #pragma once #include +#include #include #include @@ -43,53 +44,88 @@ using Timestamp = System::Clock::Milliseconds64; template using Nullable = DataModel::Nullable; + /** * This class helps track reporting state of an attribute to properly keep track of whether * it needs to be marked as dirty or not for purposes of reporting using * "7.7.9 Quieter Reporting Quality" (Q quality) * - * The class can be configured via `SetPolicy` to have some/all of the common reasons + * The class can be configured via `policy()` to have some/all of the common reasons * for reporting (e.g. increment only, decrement only, change to/from zero). * * Changes of null to non-null or non-null to null are always considered dirty. * - * It is possible to force mark the attribute as dirty (see `ForceDirty`) such as + * It is possible to force mark the attribute as dirty (see `ForceDirty()`) such as * for conditions like "When there is any increase or decrease in the estimated time * remaining that was due to progressing insight of the server's control logic". * - * Usage: - * - * - Call `SetValue()` with the new value and current monotonic system timestamp - * - There is an overload with a `SufficientChangePredicate` which will apply externally - * provided checks between old/new value and time last marked dirty to allow for complex - * rules like marking dirty less than once per second, etc. - * - *Maybe* call `ForceDirty()` in some choice situations (e.g. know for sure it's - * an important update, like at the edge of an operational state change). - * - Call `WasJustMarkedDirty()`. If it returns true, mark the attribute dirty, with the - * method most suitable at the call site (e.g. `MatterReportingAttributeChangeCallback` call - * or similar methods). - * - * Example of situations that require dirty, which are possible to support with this class: - * - * - If attribute has changed due to a change in the X or Y attributes, or - * - When it changes from 0 to any other value and vice versa, or - * - When it changes from null to any other value and vice versa, or - * - When it increases, or - * - When there is any increase or decrease in the estimated time remaining that was due to progressing insight of the server's - * control logic, or + * Class maintains a `current value` and a timestamped `dirty` state. The value is + * marked as dirty if it considered sufficiently changed. + * + * - `SetValue()` has internal rules for null/non-null changes and policy-based rules + * - `SetValue()` with a `SufficientChangePredicate` uses the internal rules in addition to + * the predicate to determine dirty state + * - `ForceDirty()` marks it immediately dirty and sets the current timestamp provided as last + * dirty time. + * - `WasJustMarkedDirty()` is a getter that will set the dirty state back to false. + * + * See [QuieterReportingPolicyEnum] for policy flags on when a value is considered dirty + * beyond non/non-null changes. + * + * Common quieter reporting usecases that can be supported by this class are: + * - If attribute has changed due to a change in the X or Y attributes + * - Use SufficientChangePredicate version + * - When it changes from 0 to any other value and vice versa + * - Use `kMarkDirtyOnChangeToFromZero` internal policy. + * - When it changes from null to any other value and vice versa + * - Built-in rule. + * - When it increases + * - Use `kMarkDirtyOnIncrement` internal policy. + * - When it decreases + * - Use `kMarkDirtyOnDecrement` internal policy. + * - When there is any increase or decrease in the estimated time remaining that was + * due to progressing insight of the server's control logic + * - Use `ForceDirty()` * - When it changes at a rate significantly different from one unit per second. + * - Use SufficientChangePredicate version + * + * Example usage in-situ: + * + * Class has: + * QuieterReportingAttribute mAttrib; + * + * Code at time of setting new value has: + * + * uint8_t newValue = driver.GetNewValue(); + * auto now = SystemClock().GetMonotonicTimestamp(); + * mAttrib.SetValue(newValue, now); + * bool dirty = mAttrib.WasJustMarkedDirty(); + * if (dirty) + * { + * MatterReportingAttributeChangeCallback(path_for_attribute); + * } * * @tparam T - the type of underlying numerical value that will be held by the class. */ -template +template ::value, bool> = true> class QuieterReportingAttribute { public: explicit QuieterReportingAttribute(const Nullable & initialValue) : mValue(initialValue), mLastDirtyValue(initialValue) {} - using SufficientChangePredicate = - std::function & /* previousDirtyValue */, - const Nullable & /* newValue */)>; + struct SufficientChangePredicateCandidate + { + // Timestamp of last time attribute was marked dirty. + Timestamp lastDirtyTimestamp; + // New (`now`) timestamp passed in `SetValue()`. + Timestamp nowTimestamp; + // Value last marked as dirty. + const Nullable& lastDirtyValue; + // New value passed in `SetValue()`, to compare against lastDirtyValue for sufficient change if needed. + const Nullable& newValue; + }; + + using SufficientChangePredicate = std::function; /** * @brief Factory to generate a functor for "attribute was last reported" at least `minimumDurationMillis` ago. @@ -100,9 +136,8 @@ class QuieterReportingAttribute static SufficientChangePredicate GetPredicateForSufficientTimeSinceLastDirty(System::Clock::Milliseconds64 minimumDurationMillis) { - return [minimumDurationMillis](Timestamp previousDirtyTime, Timestamp now, const Nullable & oldDirtyValue, - const Nullable & newValue) -> bool { - return (oldDirtyValue != newValue) && ((now - previousDirtyTime) >= minimumDurationMillis); + return [minimumDurationMillis](const SufficientChangePredicateCandidate& candidate) -> bool { + return (candidate.lastDirtyValue != candidate.newValue) && ((candidate.nowTimestamp - candidate.lastDirtyTimestamp) >= minimumDurationMillis); }; } @@ -110,8 +145,6 @@ class QuieterReportingAttribute QuieterReportingPolicyFlags & policy() { return mPolicyFlags; } const QuieterReportingPolicyFlags & policy() const { return mPolicyFlags; } - void SetPolicy(QuieterReportingPolicyFlags policyFlags) { mPolicyFlags = policyFlags; } - /** * When this returns true, attribute should be marked for reporting. Auto-resets to false after call. */ @@ -164,7 +197,14 @@ class QuieterReportingAttribute mIsDirty = mIsDirty || (mPolicyFlags.Has(QuieterReportingPolicyEnum::kMarkDirtyOnChangeToFromZero) && changeToFromZero); mIsDirty = mIsDirty || (mPolicyFlags.Has(QuieterReportingPolicyEnum::kMarkDirtyOnDecrement) && isDecrement); mIsDirty = mIsDirty || (mPolicyFlags.Has(QuieterReportingPolicyEnum::kMarkDirtyOnIncrement) && isIncrement); - mIsDirty = mIsDirty || changedPredicate(mLastDirtyTimestampMillis, now, mLastDirtyValue, newValue); + + SufficientChangePredicateCandidate candidate{ + mLastDirtyTimestampMillis, // lastDirtyTimestamp + now, // nowTimestamp + mLastDirtyValue, // lastDirtyValue + newValue // newValue + }; + mIsDirty = mIsDirty || changedPredicate(candidate); mValue = newValue; @@ -185,14 +225,19 @@ class QuieterReportingAttribute */ void SetValue(const chip::app::DataModel::Nullable & newValue, Timestamp now) { - SetValue(newValue, now, [](Timestamp, Timestamp, const Nullable &, const Nullable &) -> bool { return false; }); + SetValue(newValue, now, [](const SufficientChangePredicateCandidate&) -> bool { return false; }); } protected: + // Current value of the attribute. chip::app::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; + // Whether the attribute is currently considered "just dirty". This is cleared on read by `WasJustMarkedDirty()`. bool mIsDirty = false; + // 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{}; }; diff --git a/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp b/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp index af96dd392e7430..5fba98ed1e393e 100644 --- a/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp +++ b/src/app/cluster-building-blocks/tests/TestQuieterReporting.cpp @@ -65,8 +65,8 @@ TEST(TestQuieterReporting, ChangeToFromZeroPolicyWorks) auto now = fakeClock.now(); - attribute.SetPolicy(QuieterReportingPolicyFlags{ QuieterReportingPolicyEnum::kMarkDirtyOnChangeToFromZero }); - EXPECT_EQ(attribute.policy(), QuieterReportingPolicyFlags{ QuieterReportingPolicyEnum::kMarkDirtyOnChangeToFromZero }); + attribute.policy().Set(QuieterReportingPolicyEnum::kMarkDirtyOnChangeToFromZero); + EXPECT_TRUE(attribute.policy().HasOnly(QuieterReportingPolicyEnum::kMarkDirtyOnChangeToFromZero)); // 10 --> 11, expect not marked dirty yet. attribute.SetValue(11, now); @@ -89,8 +89,8 @@ TEST(TestQuieterReporting, ChangeToFromZeroPolicyWorks) EXPECT_FALSE(attribute.WasJustMarkedDirty()); // Reset policy, expect 12 --> 0 does not mark dirty due to no longer having the policy that causes it. - attribute.SetPolicy(QuieterReportingPolicyFlags{}); - EXPECT_EQ(attribute.policy(), QuieterReportingPolicyFlags{}); + attribute.policy().ClearAll(); + EXPECT_FALSE(attribute.policy().HasAny()); attribute.SetValue(0, now); EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 0); @@ -110,8 +110,8 @@ TEST(TestQuieterReporting, ChangeOnIncrementPolicyWorks) auto now = fakeClock.now(); - attribute.SetPolicy(QuieterReportingPolicyFlags{ QuieterReportingPolicyEnum::kMarkDirtyOnIncrement }); - EXPECT_EQ(attribute.policy(), QuieterReportingPolicyFlags{ QuieterReportingPolicyEnum::kMarkDirtyOnIncrement }); + attribute.policy().Set(QuieterReportingPolicyEnum::kMarkDirtyOnIncrement); + EXPECT_TRUE(attribute.policy().HasOnly(QuieterReportingPolicyEnum::kMarkDirtyOnIncrement)); // 10 --> 9, expect not marked dirty yet. attribute.SetValue(9, now); @@ -149,8 +149,8 @@ TEST(TestQuieterReporting, ChangeOnIncrementPolicyWorks) EXPECT_TRUE(attribute.WasJustMarkedDirty()); // Reset policy, expect 11 --> 12 does not mark dirty due to no longer having the policy that causes it. - attribute.SetPolicy(QuieterReportingPolicyFlags{}); - EXPECT_EQ(attribute.policy(), QuieterReportingPolicyFlags{}); + attribute.policy().ClearAll(); + EXPECT_FALSE(attribute.policy().HasAny()); attribute.SetValue(12, now); EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 12); @@ -170,8 +170,8 @@ TEST(TestQuieterReporting, ChangeOnDecrementPolicyWorks) auto now = fakeClock.now(); - attribute.SetPolicy(QuieterReportingPolicyFlags{ QuieterReportingPolicyEnum::kMarkDirtyOnDecrement }); - EXPECT_EQ(attribute.policy(), QuieterReportingPolicyFlags{ QuieterReportingPolicyEnum::kMarkDirtyOnDecrement }); + attribute.policy().Set(QuieterReportingPolicyEnum::kMarkDirtyOnDecrement); + EXPECT_TRUE(attribute.policy().HasOnly(QuieterReportingPolicyEnum::kMarkDirtyOnDecrement)); // 9 --> 10, expect not marked dirty yet. attribute.SetValue(10, now); @@ -212,8 +212,8 @@ TEST(TestQuieterReporting, ChangeOnDecrementPolicyWorks) EXPECT_TRUE(attribute.WasJustMarkedDirty()); // Reset policy, expect 11 --> 10 does not mark dirty due to no longer having the policy that causes it. - attribute.SetPolicy(QuieterReportingPolicyFlags{}); - EXPECT_EQ(attribute.policy(), QuieterReportingPolicyFlags{}); + attribute.policy().ClearAll(); + EXPECT_FALSE(attribute.policy().HasAny()); attribute.SetValue(10, now); EXPECT_EQ(attribute.value().ValueOr(INT_MAX), 10);