Skip to content

Commit

Permalink
fan_control_server: Fix circular callback issue (#36489)
Browse files Browse the repository at this point in the history 
* fan_control_server: Fix circular callback issue

This PR fixes a circular callback bug in fan control server using flags
when updating SpeedSetting and PercentSetting.

Before this change, a PercentSetting write to 25% would end up circling
back to 30% as shown:

```
[MatterTest] 11-12 19:16:40.792 INFO @@@ WRITE PercentSetting to 25
[MatterTest] 11-12 19:16:40.801 INFO @@@ ATTRIB: EP1/FanControl/SpeedSetting: 3
[MatterTest] 11-12 19:16:40.802 INFO @@@ ATTRIB: EP1/FanControl/SpeedCurrent: 3
[MatterTest] 11-12 19:16:40.802 INFO @@@ ATTRIB: EP1/FanControl/PercentSetting: 30
[MatterTest] 11-12 19:16:40.802 INFO @@@ ATTRIB: EP1/FanControl/PercentCurrent: 30
```

Now it behaves as expected:
```
[MatterTest] 11-13 18:54:27.961 INFO @@@ WRITE PercentSetting to 25
[MatterTest] 11-13 18:54:27.970 INFO @@@ ATTRIB: EP1/FanControl/SpeedSetting: 3
[MatterTest] 11-13 18:54:27.970 INFO @@@ ATTRIB: EP1/FanControl/SpeedCurrent: 3
[MatterTest] 11-13 18:54:27.970 INFO @@@ ATTRIB: EP1/FanControl/PercentSetting: 25
[MatterTest] 11-13 18:54:27.971 INFO @@@ ATTRIB: EP1/FanControl/PercentCurrent: 25
```

Co-authored-by: lpbeliveau-silabs <[email protected]>

* Addressed review suggestions

---------

Co-authored-by: lpbeliveau-silabs <[email protected]>
  • Loading branch information
@soares-sergio @lpbeliveau-silabs
soares-sergio and lpbeliveau-silabs authored Nov 15, 2024
1 parent 57489d1 commit c0dbaaf
Showing 1 changed file with 32 additions and 33 deletions.
65 changes: 32 additions & 33 deletions src/app/clusters/fan-control-server/fan-control-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <app/util/attribute-storage.h>
#include <app/util/config.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/Scoped.h>
#include <lib/support/logging/CHIPLogging.h>
#include <protocols/interaction_model/StatusCode.h>

Expand Down Expand Up @@ -85,6 +86,10 @@ namespace {
// Indicates if the write operation is from the cluster server itself
bool gWriteFromClusterLogic = false;

// Avoid circular callback calls when adjusting SpeedSetting and PercentSetting together.
ScopedChangeOnly gSpeedWriteInProgress(false);
ScopedChangeOnly gPercentWriteInProgress(false);

Status SetFanModeToOff(EndpointId endpointId)
{
FanModeEnum currentFanMode;
Expand Down Expand Up @@ -185,6 +190,10 @@ MatterFanControlClusterServerPreAttributeChangedCallback(const ConcreteAttribute
break;
}
case SpeedSetting::Id: {
if (gSpeedWriteInProgress)
{
return Status::WriteIgnored;
}
if (SupportsMultiSpeed(attributePath.mEndpointId))
{
// Check if the SpeedSetting is null.
Expand Down Expand Up @@ -224,6 +233,10 @@ MatterFanControlClusterServerPreAttributeChangedCallback(const ConcreteAttribute
break;
}
case PercentSetting::Id: {
if (gPercentWriteInProgress)
{
return Status::WriteIgnored;
}
// Check if the PercentSetting is null.
if (NumericAttributeTraits<Percent>::IsNullValue(*value))
{
Expand Down Expand Up @@ -362,39 +375,32 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt
DataModel::Nullable<Percent> percentSetting;
Status status = PercentSetting::Get(attributePath.mEndpointId, percentSetting);
VerifyOrReturn(Status::Success == status && !percentSetting.IsNull());
uint8_t speedMax;
status = SpeedMax::Get(attributePath.mEndpointId, &speedMax);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to get SpeedMax with error: 0x%02x", to_underlying(status)));

// Avoid circular callback calls
ScopedChange PercentWriteInProgress(gPercentWriteInProgress, true);
// If PercentSetting is set to 0, the server SHALL set the FanMode attribute value to Off.
if (percentSetting.Value() == 0)
{
status = SetFanModeToOff(attributePath.mEndpointId);
VerifyOrReturn(Status::Success == status,
VerifyOrReturn(status == Status::Success,
ChipLogError(Zcl, "Failed to set FanMode to off with error: 0x%02x", to_underlying(status)));
}

if (SupportsMultiSpeed(attributePath.mEndpointId))
{
// Adjust SpeedSetting from a percent value change for PercentSetting
// speed = ceil( SpeedMax * (percent * 0.01) )
uint8_t speedMax;
status = SpeedMax::Get(attributePath.mEndpointId, &speedMax);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to get SpeedMax with error: 0x%02x", to_underlying(status)));

DataModel::Nullable<uint8_t> currentSpeedSetting;
status = SpeedSetting::Get(attributePath.mEndpointId, currentSpeedSetting);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to get SpeedSetting with error: 0x%02x", to_underlying(status)));

uint16_t percent = percentSetting.Value();
// Plus 99 then integer divide by 100 instead of multiplying 0.01 to avoid floating point precision error
uint8_t speedSetting = static_cast<uint8_t>((speedMax * percent + 99) / 100);

if (currentSpeedSetting.IsNull() || speedSetting != currentSpeedSetting.Value())
{
status = SpeedSetting::Set(attributePath.mEndpointId, speedSetting);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to set SpeedSetting with error: 0x%02x", to_underlying(status)));
}
status = SpeedSetting::Set(attributePath.mEndpointId, speedSetting);
VerifyOrDo(Status::Success == status,
ChipLogError(Zcl, "Failed to set SpeedSetting with error: 0x%02x", to_underlying(status)));
}
break;
}
Expand All @@ -404,7 +410,13 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt
DataModel::Nullable<uint8_t> speedSetting;
Status status = SpeedSetting::Get(attributePath.mEndpointId, speedSetting);
VerifyOrReturn(Status::Success == status && !speedSetting.IsNull());
uint8_t speedMax;
status = SpeedMax::Get(attributePath.mEndpointId, &speedMax);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to get SpeedMax with error: 0x%02x", to_underlying(status)));

// Avoid circular callback calls
ScopedChange SpeedWriteInProgress(gSpeedWriteInProgress, true);
// If SpeedSetting is set to 0, the server SHALL set the FanMode attribute value to Off.
if (speedSetting.Value() == 0)
{
Expand All @@ -415,25 +427,12 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt

// Adjust PercentSetting from a speed value change for SpeedSetting
// percent = floor( speed/SpeedMax * 100 )
uint8_t speedMax;
status = SpeedMax::Get(attributePath.mEndpointId, &speedMax);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to get SpeedMax with error: 0x%02x", to_underlying(status)));

DataModel::Nullable<Percent> currentPercentSetting;
status = PercentSetting::Get(attributePath.mEndpointId, currentPercentSetting);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to get PercentSetting with error: 0x%02x", to_underlying(status)));

float speed = speedSetting.Value();
Percent percentSetting = static_cast<Percent>(speed / speedMax * 100);

if (currentPercentSetting.IsNull() || percentSetting != currentPercentSetting.Value())
{
status = PercentSetting::Set(attributePath.mEndpointId, percentSetting);
VerifyOrReturn(Status::Success == status,
ChipLogError(Zcl, "Failed to set PercentSetting with error: 0x%02x", to_underlying(status)));
}
status = PercentSetting::Set(attributePath.mEndpointId, percentSetting);
VerifyOrDo(Status::Success == status,
ChipLogError(Zcl, "Failed to set PercentSetting with error: 0x%02x", to_underlying(status)));
}
break;
}
Expand Down

0 comments on commit c0dbaaf

Please sign in to comment.