From 80de34e1255a5e1fc9fa04b4c875f0f290fde6ca Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Tue, 30 Jul 2024 00:28:18 -0400 Subject: [PATCH] Add Q quality to OperationalState CountdownTime (#34422) * 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 * 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 --- .../operational-state-server.cpp | 70 +++++++++++++++++-- .../operational-state-server.h | 32 +++++++-- src/python_testing/TC_OpstateCommon.py | 22 +++--- 3 files changed, 104 insertions(+), 20 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 index 896cc0ca3856b6..9a1374ebcede60 100644 --- a/src/app/clusters/operational-state-server/operational-state-server.cpp +++ b/src/app/clusters/operational-state-server/operational-state-server.cpp @@ -26,6 +26,7 @@ #include #include #include +#include using namespace chip; using namespace chip::app; @@ -40,6 +41,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) {} @@ -81,6 +85,7 @@ CHIP_ERROR Instance::SetCurrentPhase(const DataModel::Nullable & aPhase if (mCurrentPhase != oldPhase) { MatterReportingAttributeChangeCallback(mEndpointId, mClusterId, Attributes::CurrentPhase::Id); + UpdateCountdownTimeFromClusterLogic(); } return CHIP_NO_ERROR; } @@ -93,9 +98,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); } @@ -104,6 +111,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; } @@ -140,6 +153,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; @@ -153,7 +168,7 @@ void Instance::OnOperationalErrorDetected(const Structs::ErrorStateStruct::Type void Instance::OnOperationCompletionDetected(uint8_t aCompletionErrorCode, const Optional> & aTotalOperationalTime, - const Optional> & aPausedTime) const + const Optional> & aPausedTime) { ChipLogDetail(Zcl, "OperationalStateServer: OnOperationCompletionDetected"); @@ -166,6 +181,8 @@ void Instance::OnOperationCompletionDetected(uint8_t aCompletionErrorCode, ChipLogError(Zcl, "OperationalStateServer: Failed to record OperationCompletion event: %" CHIP_ERROR_FORMAT, error.Format()); } + + UpdateCountdownTimeFromClusterLogic(); } void Instance::ReportOperationalStateListChange() @@ -176,6 +193,43 @@ void Instance::ReportOperationalStateListChange() void Instance::ReportPhaseListChange() { MatterReportingAttributeChangeCallback(ConcreteAttributePath(mEndpointId, mClusterId, Attributes::PhaseList::Id)); + UpdateCountdownTimeFromClusterLogic(); +} + +void Instance::UpdateCountdownTime(bool fromDelegate) +{ + app::DataModel::Nullable 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) @@ -267,6 +321,7 @@ void Instance::InvokeCommand(HandlerContext & handlerContext) ChipLogDetail(Zcl, "OperationalState: Entering handling derived cluster commands"); InvokeDerivedClusterCommand(handlerContext); + break; } } @@ -291,18 +346,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: { @@ -329,18 +384,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 index 5a461c272177ba..56229c02541d60 100644 --- a/src/app/clusters/operational-state-server/operational-state-server.h +++ b/src/app/clusters/operational-state-server/operational-state-server.h @@ -22,7 +22,8 @@ #include #include #include -#include +#include +#include namespace chip { namespace app { @@ -114,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. @@ -130,7 +137,7 @@ class Instance : public CommandHandlerInterface, public AttributeAccessInterface */ void OnOperationCompletionDetected(uint8_t aCompletionErrorCode, const Optional> & aTotalOperationalTime = NullOptional, - const Optional> & aPausedTime = NullOptional) const; + const Optional> & aPausedTime = NullOptional); // List change reporting /** @@ -193,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; @@ -203,6 +223,7 @@ class Instance : public CommandHandlerInterface, public AttributeAccessInterface app::DataModel::Nullable mCurrentPhase; uint8_t mOperationalState = 0; // assume 0 for now. GenericOperationalError mOperationalError = to_underlying(ErrorStateEnum::kNoError); + app::QuieterReportingAttribute mCountdownTime{ DataModel::NullNullable }; /** * This method is inherited from CommandHandlerInterface. @@ -263,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 GetCountdownTime() = 0; diff --git a/src/python_testing/TC_OpstateCommon.py b/src/python_testing/TC_OpstateCommon.py index 9e32aac00ec846..ddea7fabbc0a03 100644 --- 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")