Skip to content

Commit

Permalink
Add Q quality to OperationalState CountdownTime (project-chip#34422)
Browse files Browse the repository at this point in the history 
* Fix post-review comments on Q quality utils from project-chip#34266

- PR project-chip#34266 had post review comments
  See: project-chip#34266 (review)
- Fix 0->0 case
- Introduce `Timestamp` more places where it makes sense
- Simplify some code lines

Fixes project-chip#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 project-chip#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]>
  • Loading branch information
@tcarmelveilleux @restyled-commits @erwinpan1
2 people authored and erwinpan1 committed Nov 11, 2024
1 parent 226f964 commit 80de34e
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 20 deletions.
70 changes: 63 additions & 7 deletions src/app/clusters/operational-state-server/operational-state-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,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;
Expand All @@ -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) {}
Expand Down Expand Up @@ -81,6 +85,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;
}
Expand All @@ -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);
}

Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand All @@ -153,7 +168,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");

Expand All @@ -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()
Expand All @@ -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<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)
Expand Down Expand Up @@ -267,6 +321,7 @@ void Instance::InvokeCommand(HandlerContext & handlerContext)
ChipLogDetail(Zcl, "OperationalState: Entering handling derived cluster commands");

InvokeDerivedClusterCommand(handlerContext);
break;
}
}

Expand All @@ -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: {

Expand All @@ -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;
}
Expand Down
32 changes: 27 additions & 5 deletions src/app/clusters/operational-state-server/operational-state-server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
#include <app-common/zap-generated/cluster-objects.h>
#include <app/AttributeAccessInterface.h>
#include <app/CommandHandlerInterface.h>
#include <app/util/af.h>
#include <app/cluster-building-blocks/QuieterReporting.h>
#include <app/data-model/Nullable.h>

namespace chip {
namespace app {
Expand Down Expand Up @@ -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.
Expand All @@ -130,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
/**
Expand Down Expand Up @@ -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;

Expand All @@ -203,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.
Expand Down Expand Up @@ -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<uint32_t> GetCountdownTime() = 0;
Expand Down
22 changes: 14 additions & 8 deletions src/python_testing/TC_OpstateCommon.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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")

Expand Down Expand Up @@ -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")

Expand All @@ -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")

Expand Down

0 comments on commit 80de34e

Please sign in to comment.