Skip to content

Commit

Permalink
Add a way to force reporting when setting attr value in ember store. (#…
Browse files Browse the repository at this point in the history
…34521)

When using a QuieterReportingAttribute to track whether reporting is needed, we
can end up in a situation where we set a value that we have not reported yet,
and then an edge happens that requires reporting to be triggered, without the
relevant value changing.  In that situation the QuieterReportingAttribute
remembers the last-reported value (not just the last-set one), and will return
AttributeDirtyState::kMustReport.

What was missing was a way to tell the ember data store "go ahead and report
this, even though we are not in fact changing the value".  We want that, in this
case, since the ember data store just knows the last-set value, not the
last-reported one.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Dec 11, 2024
1 parent a609c29 commit 1900129
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 6 deletions.
10 changes: 5 additions & 5 deletions src/app/clusters/level-control/level-control.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ static void reallyUpdateCoupledColorTemp(EndpointId endpoint)
/*
* @brief
* This function is used to update the current level attribute
* while respecting it's defined quiet reporting quality:
* while respecting its defined quiet reporting quality:
* The attribute will be reported:
* - At most once per second, or
* - At the start of the movement/transition, or
Expand All @@ -386,8 +386,7 @@ static Status SetCurrentLevelQuietReport(EndpointId endpoint, EmberAfLevelContro
DataModel::Nullable<uint8_t> newValue, bool isStartOrEndOfTransition)
{
AttributeDirtyState dirtyState;
MarkAttributeDirty markDirty = MarkAttributeDirty::kNo;
auto now = System::SystemClock().GetMonotonicTimestamp();
auto now = System::SystemClock().GetMonotonicTimestamp();

if (isStartOrEndOfTransition)
{
Expand All @@ -406,9 +405,10 @@ static Status SetCurrentLevelQuietReport(EndpointId endpoint, EmberAfLevelContro
dirtyState = state->quietCurrentLevel.SetValue(newValue, now, predicate);
}

MarkAttributeDirty markDirty = MarkAttributeDirty::kNo;
if (dirtyState == AttributeDirtyState::kMustReport)
{
markDirty = MarkAttributeDirty::kIfChanged;
markDirty = MarkAttributeDirty::kYes;
}
return Attributes::CurrentLevel::Set(endpoint, state->quietCurrentLevel.value(), markDirty);
}
Expand Down Expand Up @@ -542,7 +542,7 @@ static void writeRemainingTime(EndpointId endpoint, uint16_t remainingTimeMs)
// - kMarkDirtyOnIncrement : When the value increases.
if (state->quietRemainingTime.SetValue(remainingTimeDs, now) == AttributeDirtyState::kMustReport)
{
markDirty = MarkAttributeDirty::kIfChanged;
markDirty = MarkAttributeDirty::kYes;
}

Attributes::RemainingTime::Set(endpoint, state->quietRemainingTime.value().ValueOr(0), markDirty);
Expand Down
5 changes: 5 additions & 0 deletions src/app/util/af-types.h
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,11 @@ enum class MarkAttributeDirty
{
kIfChanged,
kNo,
// kYes might need to be used if the attribute value was previously changed
// without reporting, and now is being set in a situation where we know
// reporting needs to be triggered (e.g. because QuieterReportingAttribute
// indicated that).
kYes,
};

} // namespace app
Expand Down
7 changes: 6 additions & 1 deletion src/app/util/attribute-table.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,12 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at

if (!valueChanging)
{
// Just do nothing.
// Just do nothing, except triggering reporting if forced.
if (markDirty == MarkAttributeDirty::kYes)
{
MatterReportingAttributeChangeCallback(endpoint, cluster, attributeID);
}

return Status::Success;
}

Expand Down

0 comments on commit 1900129

Please sign in to comment.