Add Q quality to OperationalState CountdownTime (#34422)

* Fix post-review comments on Q quality utils from #34266 - PR #34266 had post review comments See: #34266 (review) - 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 * Add Q quality to OperationalState CountdownTIme - Update operational state cluster server to report countdown time at edges. - Add a way for cluster delegate to request an update of time. Issue #34421 Testing done: - Existing tests still pass - Will add cert test when the text is finalized (week of July 22) * Restyled by clang-format * Address review comments * Address review comments * Update src/app/clusters/operational-state-server/operational-state-server.cpp * Fix several Opstate test cases * Restyled by autopep8 * Fix minor grammar typo * Restyled by autopep8 * Fix TC-OPSTATE-2.2 --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · Jul 30, 2024 · 8060d45 · 8060d45
1 parent 87097db
commit 8060d45
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 19 deletions.
diff --git a/src/app/clusters/operational-state-server/operational-state-server.cpp b/src/app/clusters/operational-state-server/operational-state-server.cpp
@@ -29,6 +29,7 @@
 #include <app/InteractionModelEngine.h>
 #include <app/reporting/reporting.h>
 #include <app/util/attribute-storage.h>
+#include <lib/support/logging/CHIPLogging.h>
 
 using namespace chip;
 using namespace chip::app;
@@ -43,6 +44,9 @@ Instance::Instance(Delegate * aDelegate, EndpointId aEndpointId, ClusterId aClus
     mDelegate(aDelegate), mEndpointId(aEndpointId), mClusterId(aClusterId)
 {
     mDelegate->SetInstance(this);
+    mCountdownTime.policy()
+        .Set(QuieterReportingPolicyEnum::kMarkDirtyOnIncrement)
+        .Set(QuieterReportingPolicyEnum::kMarkDirtyOnChangeToFromZero);
 }
 
 Instance::Instance(Delegate * aDelegate, EndpointId aEndpointId) : Instance(aDelegate, aEndpointId, OperationalState::Id) {}
@@ -84,6 +88,7 @@ CHIP_ERROR Instance::SetCurrentPhase(const DataModel::Nullable<uint8_t> & aPhase
     if (mCurrentPhase != oldPhase)
     {
         MatterReportingAttributeChangeCallback(mEndpointId, mClusterId, Attributes::CurrentPhase::Id);
+        UpdateCountdownTimeFromClusterLogic();
     }
     return CHIP_NO_ERROR;
 }
@@ -96,9 +101,11 @@ CHIP_ERROR Instance::SetOperationalState(uint8_t aOpState)
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
+    bool countdownTimeUpdateNeeded = false;
     if (mOperationalError.errorStateID != to_underlying(ErrorStateEnum::kNoError))
     {
         mOperationalError.Set(to_underlying(ErrorStateEnum::kNoError));
+        countdownTimeUpdateNeeded = true;
         MatterReportingAttributeChangeCallback(mEndpointId, mClusterId, Attributes::OperationalError::Id);
     }
 
@@ -107,6 +114,12 @@ CHIP_ERROR Instance::SetOperationalState(uint8_t aOpState)
     if (mOperationalState != oldState)
     {
         MatterReportingAttributeChangeCallback(mEndpointId, mClusterId, Attributes::OperationalState::Id);
+        countdownTimeUpdateNeeded = true;
+    }
+
+    if (countdownTimeUpdateNeeded)
+    {
+        UpdateCountdownTimeFromClusterLogic();
     }
     return CHIP_NO_ERROR;
 }
@@ -143,6 +156,8 @@ void Instance::OnOperationalErrorDetected(const Structs::ErrorStateStruct::Type
         MatterReportingAttributeChangeCallback(mEndpointId, mClusterId, Attributes::OperationalError::Id);
     }
 
+    UpdateCountdownTimeFromClusterLogic();
+
     // Generate an ErrorDetected event
     GenericErrorEvent event(mClusterId, aError);
     EventNumber eventNumber;
@@ -156,7 +171,7 @@ void Instance::OnOperationalErrorDetected(const Structs::ErrorStateStruct::Type
 
 void Instance::OnOperationCompletionDetected(uint8_t aCompletionErrorCode,
                                              const Optional<DataModel::Nullable<uint32_t>> & aTotalOperationalTime,
-                                             const Optional<DataModel::Nullable<uint32_t>> & aPausedTime) const
+                                             const Optional<DataModel::Nullable<uint32_t>> & aPausedTime)
 {
     ChipLogDetail(Zcl, "OperationalStateServer: OnOperationCompletionDetected");
 
@@ -169,6 +184,8 @@ void Instance::OnOperationCompletionDetected(uint8_t aCompletionErrorCode,
         ChipLogError(Zcl, "OperationalStateServer: Failed to record OperationCompletion event: %" CHIP_ERROR_FORMAT,
                      error.Format());
     }
+
+    UpdateCountdownTimeFromClusterLogic();
 }
 
 void Instance::ReportOperationalStateListChange()
@@ -179,6 +196,43 @@ void Instance::ReportOperationalStateListChange()
 void Instance::ReportPhaseListChange()
 {
     MatterReportingAttributeChangeCallback(ConcreteAttributePath(mEndpointId, mClusterId, Attributes::PhaseList::Id));
+    UpdateCountdownTimeFromClusterLogic();
+}
+
+void Instance::UpdateCountdownTime(bool fromDelegate)
+{
+    app::DataModel::Nullable<uint32_t> newCountdownTime = mDelegate->GetCountdownTime();
+    auto now                                            = System::SystemClock().GetMonotonicTimestamp();
+
+    bool markDirty = false;
+
+    if (fromDelegate)
+    {
+        // Updates from delegate are reduce-reported to every 10s max (choice of this implementation), in addition
+        // to default change-from-null, change-from-zero and increment policy.
+        auto predicate = [](const decltype(mCountdownTime)::SufficientChangePredicateCandidate & candidate) -> bool {
+            if (candidate.lastDirtyValue.IsNull() || candidate.newValue.IsNull())
+            {
+                return false;
+            }
+
+            uint32_t lastDirtyValue           = candidate.lastDirtyValue.Value();
+            uint32_t newValue                 = candidate.newValue.Value();
+            uint32_t kNumSecondsDeltaToReport = 10;
+            return (newValue < lastDirtyValue) && ((lastDirtyValue - newValue) > kNumSecondsDeltaToReport);
+        };
+        markDirty = (mCountdownTime.SetValue(newCountdownTime, now, predicate) == AttributeDirtyState::kMustReport);
+    }
+    else
+    {
+        auto predicate = [](const decltype(mCountdownTime)::SufficientChangePredicateCandidate &) -> bool { return true; };
+        markDirty      = (mCountdownTime.SetValue(newCountdownTime, now, predicate) == AttributeDirtyState::kMustReport);
+    }
+
+    if (markDirty)
+    {
+        MatterReportingAttributeChangeCallback(mEndpointId, mClusterId, Attributes::CountdownTime::Id);
+    }
 }
 
 bool Instance::IsSupportedPhase(uint8_t aPhase)
@@ -270,6 +324,7 @@ void Instance::InvokeCommand(HandlerContext & handlerContext)
         ChipLogDetail(Zcl, "OperationalState: Entering handling derived cluster commands");
 
         InvokeDerivedClusterCommand(handlerContext);
+        break;
     }
 }
 
@@ -294,18 +349,18 @@ CHIP_ERROR Instance::Read(const ConcreteReadAttributePath & aPath, AttributeValu
             }
             return err;
         });
+        break;
     }
-    break;
 
     case OperationalState::Attributes::OperationalState::Id: {
         ReturnErrorOnFailure(aEncoder.Encode(GetCurrentOperationalState()));
+        break;
     }
-    break;
 
     case OperationalState::Attributes::OperationalError::Id: {
         ReturnErrorOnFailure(aEncoder.Encode(mOperationalError));
+        break;
     }
-    break;
 
     case OperationalState::Attributes::PhaseList::Id: {
 
@@ -332,18 +387,19 @@ CHIP_ERROR Instance::Read(const ConcreteReadAttributePath & aPath, AttributeValu
                 ReturnErrorOnFailure(encoder.Encode(phase2));
             }
         });
+        break;
     }
-    break;
 
     case OperationalState::Attributes::CurrentPhase::Id: {
         ReturnErrorOnFailure(aEncoder.Encode(GetCurrentPhase()));
+        break;
     }
-    break;
 
     case OperationalState::Attributes::CountdownTime::Id: {
+        // Read through to get value closest to reality.
         ReturnErrorOnFailure(aEncoder.Encode(mDelegate->GetCountdownTime()));
+        break;
     }
-    break;
     }
     return CHIP_NO_ERROR;
 }

diff --git a/src/app/clusters/operational-state-server/operational-state-server.h b/src/app/clusters/operational-state-server/operational-state-server.h
@@ -22,6 +22,8 @@
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app/AttributeAccessInterface.h>
 #include <app/CommandHandlerInterface.h>
+#include <app/cluster-building-blocks/QuieterReporting.h>
+#include <app/data-model/Nullable.h>
 
 namespace chip {
 namespace app {
@@ -113,6 +115,12 @@ class Instance : public CommandHandlerInterface, public AttributeAccessInterface
      */
     void GetCurrentOperationalError(GenericOperationalError & error) const;
 
+    /**
+     * @brief Whenever application delegate wants to possibly report a new updated time,
+     *        call this method. The `GetCountdownTime()` method will be called on the delegate.
+     */
+    void UpdateCountdownTimeFromDelegate() { UpdateCountdownTime(/* fromDelegate = */ true); }
+
     // Event triggers
     /**
      * @brief Called when the Node detects a OperationalError has been raised.
@@ -129,7 +137,7 @@ class Instance : public CommandHandlerInterface, public AttributeAccessInterface
      */
     void OnOperationCompletionDetected(uint8_t aCompletionErrorCode,
                                        const Optional<DataModel::Nullable<uint32_t>> & aTotalOperationalTime = NullOptional,
-                                       const Optional<DataModel::Nullable<uint32_t>> & aPausedTime           = NullOptional) const;
+                                       const Optional<DataModel::Nullable<uint32_t>> & aPausedTime           = NullOptional);
 
     // List change reporting
     /**
@@ -192,6 +200,19 @@ class Instance : public CommandHandlerInterface, public AttributeAccessInterface
      */
     virtual void InvokeDerivedClusterCommand(HandlerContext & handlerContext) { return; };
 
+    /**
+     * Causes reporting/udpating of CountdownTime attribute from driver if sufficient changes have
+     * occurred (based on Q quality definition for operational state). Calls the Delegate::GetCountdownTime() method.
+     *
+     * @param fromDelegate true if the change notice was triggered by the delegate, false if internal to cluster logic.
+     */
+    void UpdateCountdownTime(bool fromDelegate);
+
+    /**
+     * @brief Whenever the cluster logic thinks time should be updated, call this.
+     */
+    void UpdateCountdownTimeFromClusterLogic() { UpdateCountdownTime(/* fromDelegate=*/false); }
+
 private:
     Delegate * mDelegate;
 
@@ -202,6 +223,7 @@ class Instance : public CommandHandlerInterface, public AttributeAccessInterface
     app::DataModel::Nullable<uint8_t> mCurrentPhase;
     uint8_t mOperationalState                 = 0; // assume 0 for now.
     GenericOperationalError mOperationalError = to_underlying(ErrorStateEnum::kNoError);
+    app::QuieterReportingAttribute<uint32_t> mCountdownTime{ DataModel::NullNullable };
 
     /**
      * This method is inherited from CommandHandlerInterface.
@@ -262,9 +284,10 @@ class Delegate
     virtual ~Delegate() = default;
 
     /**
-     * Get the countdown time.
-     * NOTE: Changes to this attribute should not be reported.
-     * From the spec: Changes to this value SHALL NOT be reported in a subscription.
+     * Get the countdown time. This will get called on many edges such as
+     * commands to change operational state, or when the delegate deals with
+     * changes. Make sure it becomes null whenever it is appropriate.
+     *
      * @return The current countdown time.
      */
     virtual app::DataModel::Nullable<uint32_t> GetCountdownTime() = 0;

diff --git a/src/python_testing/TC_OpstateCommon.py b/src/python_testing/TC_OpstateCommon.py
@@ -354,7 +354,7 @@ async def TEST_TC_OPSTATE_BASE_2_1(self, endpoint=1):
             if phase_list is not NullValue:
                 phase_list_len = len(phase_list)
                 asserts.assert_less_equal(phase_list_len, 32,
-                                          f"PhaseList length({phase_list_len}) must be less than 32!")
+                                          f"PhaseList length({phase_list_len}) must be at most 32 entries!")
 
         # STEP 3: TH reads from the DUT the CurrentPhase attribute
         self.step(3)
@@ -364,8 +364,9 @@ async def TEST_TC_OPSTATE_BASE_2_1(self, endpoint=1):
             if (phase_list == NullValue) or (not phase_list):
                 asserts.assert_true(current_phase == NullValue, f"CurrentPhase({current_phase}) should be null")
             else:
-                asserts.assert_true(0 <= current_phase and current_phase < phase_list_len,
-                                    f"CurrentPhase({current_phase}) must be between 0 and {(phase_list_len - 1)}")
+                asserts.assert_greater_equal(current_phase, 0, f"CurrentPhase({current_phase}) must be >= 0")
+                asserts.assert_less(current_phase, phase_list_len,
+                                    f"CurrentPhase({current_phase}) must be less than {phase_list_len}")
 
         # STEP 4: TH reads from the DUT the CountdownTime attribute
         self.step(4)
@@ -652,7 +653,7 @@ async def TEST_TC_OPSTATE_BASE_2_2(self, endpoint=1):
             if phase_list is not NullValue:
                 phase_list_len = len(phase_list)
                 asserts.assert_less_equal(phase_list_len, 32,
-                                          f"PhaseList length({phase_list_len}) must be less than 32!")
+                                          f"PhaseList length({phase_list_len}) must be at most 32 entries!")
 
         # STEP 9: TH reads from the DUT the CurrentPhase attribute
         self.step(9)
@@ -662,10 +663,10 @@ async def TEST_TC_OPSTATE_BASE_2_2(self, endpoint=1):
             if (phase_list == NullValue) or (not phase_list):
                 asserts.assert_equal(current_phase, NullValue, f"CurrentPhase({current_phase}) should be null")
             else:
-                asserts.assert_less_equal(0, current_phase,
-                                          f"CurrentPhase({current_phase}) must be greater or equal than 0")
-                asserts.assert_less(current_phase < phase_list_len,
-                                    f"CurrentPhase({current_phase}) must be less then {(phase_list_len - 1)}")
+                asserts.assert_greater_equal(current_phase, 0,
+                                             f"CurrentPhase({current_phase}) must be greater or equal to 0")
+                asserts.assert_less(current_phase, phase_list_len,
+                                    f"CurrentPhase({current_phase}) must be less than {(phase_list_len)}")
 
         # STEP 10: TH waits for {PIXIT.WAITTIME.COUNTDOWN}
         self.step(10)
@@ -679,6 +680,8 @@ async def TEST_TC_OPSTATE_BASE_2_2(self, endpoint=1):
                                                             attribute=attributes.CountdownTime)
 
             if (countdown_time is not NullValue) and (initial_countdown_time is not NullValue):
+                logging.info(f" -> Initial countdown time: {initial_countdown_time}")
+                logging.info(f" -> New countdown time: {countdown_time}")
                 asserts.assert_less_equal(countdown_time, (initial_countdown_time - wait_time),
                                           f"The countdown time shall have decreased at least {wait_time:.1f} since start command")
 
@@ -821,6 +824,7 @@ async def TEST_TC_OPSTATE_BASE_2_3(self, endpoint=1):
             initial_countdown_time = await self.read_expect_success(endpoint=endpoint,
                                                                     attribute=attributes.CountdownTime)
             if initial_countdown_time is not NullValue:
+                logging.info(f" -> Initial ountdown time: {initial_countdown_time}")
                 asserts.assert_true(0 <= initial_countdown_time <= 259200,
                                     f"CountdownTime({initial_countdown_time}) must be between 0 and 259200")
 
@@ -835,6 +839,8 @@ async def TEST_TC_OPSTATE_BASE_2_3(self, endpoint=1):
                                                             attribute=attributes.CountdownTime)
 
             if (countdown_time is not NullValue) and (initial_countdown_time is not NullValue):
+                logging.info(f" -> Initial countdown time: {initial_countdown_time}")
+                logging.info(f" -> New countdown time: {countdown_time}")
                 asserts.assert_equal(countdown_time, initial_countdown_time,
                                      "The countdown time shall be equal since pause command")