From 109660427d8ed7ee222d795d439b0f64791b8d7e Mon Sep 17 00:00:00 2001 From: Sergio Soares Date: Fri, 15 Nov 2024 07:30:54 -0500 Subject: [PATCH] fan_control_server: Fix circular callback issue (#36489) * 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 * Addressed review suggestions --------- Co-authored-by: lpbeliveau-silabs --- .../fan-control-server/fan-control-server.cpp | 65 +++++++++---------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/src/app/clusters/fan-control-server/fan-control-server.cpp b/src/app/clusters/fan-control-server/fan-control-server.cpp index 46f4c6d7c0bb70..7ece1a1b575a24 100644 --- a/src/app/clusters/fan-control-server/fan-control-server.cpp +++ b/src/app/clusters/fan-control-server/fan-control-server.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -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; @@ -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. @@ -224,6 +233,10 @@ MatterFanControlClusterServerPreAttributeChangedCallback(const ConcreteAttribute break; } case PercentSetting::Id: { + if (gPercentWriteInProgress) + { + return Status::WriteIgnored; + } // Check if the PercentSetting is null. if (NumericAttributeTraits::IsNullValue(*value)) { @@ -362,12 +375,18 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt DataModel::Nullable 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))); } @@ -375,26 +394,13 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt { // 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 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((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; } @@ -404,7 +410,13 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt DataModel::Nullable 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) { @@ -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 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(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; }