Backport the fan-control circular callback changes to v1.4-branch (project-chip#40351)

* fan_control_server: Fix circular callback issue (project-chip#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 <[email protected]>

* Addressed review suggestions

---------

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

* fan-control-server: Fix FanMode circular callback issue (project-chip#36515)

Similar to what was done for Speed and Percent, this PR fixes a bug where
a FanMode could result in a circular callback.

For example, setting the FanMode to kAuto, could trigger this issue.

---------

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

Author: 3 people

src/app/clusters/fan-control-server/fan-control-server.cpp

@@ -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>
 
@@ -85,6 +86,11 @@ 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);
+ScopedChangeOnly gFanModeWriteInProgress(false);
+
 Status SetFanModeToOff(EndpointId endpointId)
 {
     FanModeEnum currentFanMode;
@@ -155,12 +161,16 @@ MatterFanControlClusterServerPreAttributeChangedCallback(const ConcreteAttribute
     switch (attributePath.mAttributeId)
     {
     case FanMode::Id: {
+        if (gFanModeWriteInProgress)
+        {
+            return Status::WriteIgnored;
+        }
         if (*value == to_underlying(FanModeEnum::kOn))
         {
             FanMode::Set(attributePath.mEndpointId, FanModeEnum::kHigh);
-            res = Status::WriteIgnored;
+            return Status::WriteIgnored;
         }
-        else if (*value == to_underlying(FanModeEnum::kSmart))
+        if (*value == to_underlying(FanModeEnum::kSmart))
         {
             FanModeSequenceEnum fanModeSequence;
             Status status = FanModeSequence::Get(attributePath.mEndpointId, &fanModeSequence);
@@ -185,6 +195,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 +238,10 @@ MatterFanControlClusterServerPreAttributeChangedCallback(const ConcreteAttribute
         break;
     }
     case PercentSetting::Id: {
+        if (gPercentWriteInProgress)
+        {
+            return Status::WriteIgnored;
+        }
         // Check if the PercentSetting is null.
         if (NumericAttributeTraits<Percent>::IsNullValue(*value))
         {
@@ -315,6 +333,9 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt
         Status status = FanMode::Get(attributePath.mEndpointId, &mode);
         VerifyOrReturn(Status::Success == status);
 
+        // Avoid circular callback calls
+        ScopedChange FanModeWriteInProgress(gFanModeWriteInProgress, true);
+
         // Setting the FanMode value to Off SHALL set the values of PercentSetting, PercentCurrent,
         // SpeedSetting, SpeedCurrent attributes to 0 (zero).
         if (mode == FanModeEnum::kOff)
@@ -362,39 +383,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;
     }
@@ -404,7 +418,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)
             {
@@ -415,25 +435,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;
     }