chef: Fix FanControl mode/speed interdependence behavior (project-chip#36256)

* chef: Fix FanControl mode/speed interdepency behavior

This PR implements the FanControl logic to ensure the interdependent
behavior between Mode/Speed/Percent is coherent.

This is based on what was done in the AirPurifier examples.

Changes:
* Removed Custom Write/Read
* Updated Zap file `SpeedMax` default to 10 instead of 100
* Forward `MatterPostAttributeChangeCallback` FanControl-related changes
  to `ChefFanControlManager`
* Implements Mode/Speed/Percent interdependency logic based on the
  AirPurifier example

Test:
* Verified that Mode, Speed and SpeedPercent change accordingly when any
  of those attributes change and cross a boundary.

* ifdef out code for non-fancontrol examples.

* Address Boris' review feedback

* Update examples/chef/common/chef-fan-control-manager.cpp

Co-authored-by: Andrei Litvin <[email protected]>

* Andrei's review suggestions

* review suggestions: clarify Auto behavior and use struct for ranges.

* Fix minor bug introduced when addressing review suggestions (!= vs ==)

* Update examples/chef/common/chef-fan-control-manager.cpp

Co-authored-by: Andrei Litvin <[email protected]>

* fix const

* Apply suggestions from Andrei's code review

Co-authored-by: Andrei Litvin <[email protected]>

* Restyled by clang-format

* Apply Boris' suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

* Address Boris' review suggestions

---------

Co-authored-by: Andrei Litvin <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>

Author: 4 people

examples/chef/common/chef-fan-control-manager.cpp

@@ -38,20 +38,45 @@ using namespace chip::app::Clusters::FanControl::Attributes;
 using Protocols::InteractionModel::Status;
 
 namespace {
-class ChefFanControlManager : public AttributeAccessInterface, public Delegate
+class ChefFanControlManager : public Delegate
 {
 public:
-    ChefFanControlManager(EndpointId aEndpointId) :
-        AttributeAccessInterface(Optional<EndpointId>(aEndpointId), FanControl::Id), Delegate(aEndpointId)
-    {}
+    ChefFanControlManager(EndpointId aEndpointId) : Delegate(aEndpointId) {}
 
-    CHIP_ERROR Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) override;
-    CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;
+    void Init();
+    void HandleFanControlAttributeChange(AttributeId attributeId, uint8_t type, uint16_t size, uint8_t * value);
     Status HandleStep(StepDirectionEnum aDirection, bool aWrap, bool aLowestOff) override;
+    DataModel::Nullable<uint8_t> GetSpeedSetting();
+    DataModel::Nullable<Percent> GetPercentSetting();
 
 private:
-    Nullable<uint8_t> mPercentSetting{};
-    Nullable<uint8_t> mSpeedSetting{};
+    uint8_t mPercentCurrent = 0;
+    uint8_t mSpeedCurrent   = 0;
+
+    // Fan Mode Limits
+    struct Range
+    {
+        bool Contains(int value) const { return value >= low && value <= high; }
+        int Low() const { return low; }
+        int High() const { return high; }
+
+        int low;
+        int high;
+    };
+    static constexpr Range kFanModeLowSpeedRange    = { 1, 3 };
+    static constexpr Range kFanModeMediumSpeedRange = { 4, 7 };
+    static constexpr Range kFanModeHighSpeedRange   = { 8, 10 };
+
+    static_assert(kFanModeLowSpeedRange.low <= kFanModeLowSpeedRange.high);
+    static_assert(kFanModeLowSpeedRange.high + 1 == kFanModeMediumSpeedRange.low);
+    static_assert(kFanModeMediumSpeedRange.high + 1 == kFanModeHighSpeedRange.low);
+    static_assert(kFanModeHighSpeedRange.low <= kFanModeHighSpeedRange.high);
+
+    void FanModeWriteCallback(FanControl::FanModeEnum aNewFanMode);
+    void SetSpeedCurrent(uint8_t aNewSpeedCurrent);
+    void SetPercentCurrent(uint8_t aNewPercentCurrent);
+    void SetSpeedSetting(DataModel::Nullable<uint8_t> aNewSpeedSetting);
+    static FanControl::FanModeEnum SpeedToFanMode(uint8_t speed);
 };
 
 static std::unique_ptr<ChefFanControlManager> mFanControlManager;
@@ -99,98 +124,222 @@ Status ChefFanControlManager::HandleStep(StepDirectionEnum aDirection, bool aWra
     return SpeedSetting::Set(mEndpoint, newSpeedSetting);
 }
 
-CHIP_ERROR ChefFanControlManager::Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder)
+void ChefFanControlManager::HandleFanControlAttributeChange(AttributeId attributeId, uint8_t type, uint16_t size, uint8_t * value)
 {
-    VerifyOrDie(aPath.mClusterId == FanControl::Id);
-    VerifyOrDie(aPath.mEndpointId == mEndpoint);
-
-    switch (aPath.mAttributeId)
+    ChipLogProgress(NotSpecified, "ChefFanControlManager::HandleFanControlAttributeChange");
+    switch (attributeId)
     {
-    case SpeedSetting::Id: {
-        Nullable<uint8_t> newSpeedSetting;
-        ReturnErrorOnFailure(aDecoder.Decode(newSpeedSetting));
-
-        // Ensure new speed is in bounds
+    case FanControl::Attributes::PercentSetting::Id: {
+        ChipLogProgress(NotSpecified, "ChefFanControlManager::HandleFanControlAttributeChange  PercentSetting");
+        DataModel::Nullable<Percent> percentSetting;
+        if (!NumericAttributeTraits<Percent>::IsNullValue(*value))
         {
-            uint8_t maxSpeedSetting                    = 0;
-            Protocols::InteractionModel::Status status = SpeedMax::Get(mEndpoint, &maxSpeedSetting);
-            VerifyOrReturnError(status == Protocols::InteractionModel::Status::Success, CHIP_IM_GLOBAL_STATUS(Failure));
-
-            if (!newSpeedSetting.IsNull() && newSpeedSetting.Value() > maxSpeedSetting)
-            {
-                return CHIP_IM_GLOBAL_STATUS(ConstraintError);
-            }
+            percentSetting.SetNonNull(NumericAttributeTraits<Percent>::StorageToWorking(*value));
         }
-
-        // Only act on changed.
-        if (newSpeedSetting != mSpeedSetting)
+        else
         {
-            mSpeedSetting = newSpeedSetting;
-
-            // Mark both the setting AND the current dirty, since the current always
-            // tracks the target for our product.
-            MatterReportingAttributeChangeCallback(mEndpoint, FanControl::Id, Attributes::SpeedSetting::Id);
-            MatterReportingAttributeChangeCallback(mEndpoint, FanControl::Id, Attributes::SpeedCurrent::Id);
+            percentSetting.SetNull();
         }
 
+        // The cluster code in fan-control-server.cpp is the only one allowed to set PercentSetting to null.
+        // This happens as a consequence of setting the FanMode to kAuto. In auto mode, percentCurrent should continue to report the
+        // real fan speed percentage. In this example, we set PercentCurrent to 0 here as we don't have a real value for the Fan
+        // speed or a FanAutoMode simulator.
+        // When not Null, SpeedCurrent tracks SpeedSetting's value.
+        SetPercentCurrent(percentSetting.ValueOr(0));
         break;
     }
-    case PercentSetting::Id: {
-        Nullable<uint8_t> newPercentSetting;
-        ReturnErrorOnFailure(aDecoder.Decode(newPercentSetting));
 
-        // Ensure new speed in percent is valid.
-        if (!newPercentSetting.IsNull() && newPercentSetting.Value() > 100)
+    case FanControl::Attributes::SpeedSetting::Id: {
+        ChipLogProgress(NotSpecified, "ChefFanControlManager::HandleFanControlAttributeChange  SpeedSetting");
+        DataModel::Nullable<uint8_t> speedSetting;
+        if (!NumericAttributeTraits<uint8_t>::IsNullValue(*value))
         {
-            return CHIP_IM_GLOBAL_STATUS(ConstraintError);
+            speedSetting.SetNonNull(NumericAttributeTraits<uint8_t>::StorageToWorking(*value));
         }
-
-        // Only act on changed.
-        if (newPercentSetting != mPercentSetting)
+        else
         {
-            mPercentSetting = newPercentSetting;
-
-            // Mark both the setting AND the current dirty, since the current always
-            // tracks the target for our product.
-            MatterReportingAttributeChangeCallback(mEndpoint, FanControl::Id, Attributes::PercentSetting::Id);
-            MatterReportingAttributeChangeCallback(mEndpoint, FanControl::Id, Attributes::PercentCurrent::Id);
+            speedSetting.SetNull();
         }
 
+        // The cluster code in fan-control-server.cpp is the only one allowed to set speedSetting to null.
+        // This happens as a consequence of setting the FanMode to kAuto. In auto mode, speedCurrent should continue to report the
+        // real fan speed. In this example, we set SpeedCurrent to 0 here as we don't have a real value for the Fan speed or a
+        // FanAutoMode simulator.
+        // When not Null, SpeedCurrent tracks SpeedSetting's value.
+        SetSpeedCurrent(speedSetting.ValueOr(0));
+        // Determine if the speed change should also change the fan mode
+        FanControl::Attributes::FanMode::Set(mEndpoint, SpeedToFanMode(mSpeedCurrent));
         break;
     }
-    default:
+
+    case FanControl::Attributes::FanMode::Id: {
+        ChipLogProgress(NotSpecified, "ChefFanControlManager::HandleFanControlAttributeChange  FanMode");
+
+        static_assert(sizeof(FanControl::FanModeEnum) == 1);
+        FanControl::FanModeEnum fanMode = static_cast<FanControl::FanModeEnum>(*value);
+        FanModeWriteCallback(fanMode);
         break;
     }
 
-    // Fall through goes to attribute store legacy handling.
-    return CHIP_NO_ERROR;
+    default: {
+        break;
+    }
+    }
+}
+
+FanControl::FanModeEnum ChefFanControlManager::SpeedToFanMode(uint8_t speed)
+{
+    if (speed == 0)
+    {
+        return FanControl::FanModeEnum::kOff;
+    }
+    if (kFanModeLowSpeedRange.Contains(speed))
+    {
+        return FanControl::FanModeEnum::kLow;
+    }
+    if (kFanModeMediumSpeedRange.Contains(speed))
+    {
+        return FanControl::FanModeEnum::kMedium;
+    }
+    return FanControl::FanModeEnum::kHigh;
 }
 
-CHIP_ERROR ChefFanControlManager::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
+void ChefFanControlManager::SetPercentCurrent(uint8_t aNewPercentCurrent)
 {
-    VerifyOrDie(aPath.mClusterId == FanControl::Id);
-    VerifyOrDie(aPath.mEndpointId == mEndpoint);
+    if (aNewPercentCurrent == mPercentCurrent)
+    {
+        return;
+    }
 
-    switch (aPath.mAttributeId)
+    ChipLogDetail(NotSpecified, "ChefFanControlManager::SetPercentCurrent: %d", aNewPercentCurrent);
+    mPercentCurrent = aNewPercentCurrent;
+    Status status   = FanControl::Attributes::PercentCurrent::Set(mEndpoint, mPercentCurrent);
+    if (status != Status::Success)
     {
-    case PercentCurrent::Id: {
-        // Current percents always tracks setting immediately in our implementation.
-        return aEncoder.Encode(mPercentSetting.ValueOr(0));
+        ChipLogError(NotSpecified, "ChefFanControlManager::SetPercentCurrent: failed to set PercentCurrent attribute: %d",
+                     to_underlying(status));
     }
-    case PercentSetting::Id: {
-        return aEncoder.Encode(mPercentSetting);
+}
+
+void ChefFanControlManager::SetSpeedCurrent(uint8_t aNewSpeedCurrent)
+{
+    if (aNewSpeedCurrent == mSpeedCurrent)
+    {
+        return;
     }
-    case SpeedCurrent::Id: {
-        // Current speed always tracks setting immediately in our implementation.
-        return aEncoder.Encode(mSpeedSetting.ValueOr(0));
+
+    ChipLogDetail(NotSpecified, "ChefFanControlManager::SetSpeedCurrent: %d", aNewSpeedCurrent);
+    mSpeedCurrent = aNewSpeedCurrent;
+    Status status = FanControl::Attributes::SpeedCurrent::Set(mEndpoint, aNewSpeedCurrent);
+    if (status != Status::Success)
+    {
+        ChipLogError(NotSpecified, "ChefFanControlManager::SetSpeedCurrent: failed to set SpeedCurrent attribute: %d",
+                     to_underlying(status));
     }
-    case SpeedSetting::Id: {
-        return aEncoder.Encode(mSpeedSetting);
+}
+
+void ChefFanControlManager::FanModeWriteCallback(FanControl::FanModeEnum aNewFanMode)
+{
+    ChipLogDetail(NotSpecified, "ChefFanControlManager::FanModeWriteCallback: %d", to_underlying(aNewFanMode));
+    switch (aNewFanMode)
+    {
+    case FanControl::FanModeEnum::kOff: {
+        if (mSpeedCurrent != 0)
+        {
+            DataModel::Nullable<uint8_t> speedSetting(0);
+            SetSpeedSetting(speedSetting);
+        }
+        break;
     }
-    default:
+    case FanControl::FanModeEnum::kLow: {
+        if (!kFanModeLowSpeedRange.Contains(mSpeedCurrent))
+        {
+            DataModel::Nullable<uint8_t> speedSetting(kFanModeLowSpeedRange.Low());
+            SetSpeedSetting(speedSetting);
+        }
         break;
     }
-    return CHIP_NO_ERROR;
+    case FanControl::FanModeEnum::kMedium: {
+        if (!kFanModeMediumSpeedRange.Contains(mSpeedCurrent))
+        {
+            DataModel::Nullable<uint8_t> speedSetting(kFanModeMediumSpeedRange.Low());
+            SetSpeedSetting(speedSetting);
+        }
+        break;
+    }
+    case FanControl::FanModeEnum::kOn:
+    case FanControl::FanModeEnum::kHigh: {
+        if (!kFanModeHighSpeedRange.Contains(mSpeedCurrent))
+        {
+            DataModel::Nullable<uint8_t> speedSetting(kFanModeHighSpeedRange.Low());
+            SetSpeedSetting(speedSetting);
+        }
+        break;
+    }
+    case FanControl::FanModeEnum::kSmart:
+    case FanControl::FanModeEnum::kAuto: {
+        ChipLogProgress(NotSpecified, "ChefFanControlManager::FanModeWriteCallback: Auto");
+        break;
+    }
+    case FanControl::FanModeEnum::kUnknownEnumValue: {
+        ChipLogProgress(NotSpecified, "ChefFanControlManager::FanModeWriteCallback: Unknown");
+        break;
+    }
+    }
+}
+
+void ChefFanControlManager::SetSpeedSetting(DataModel::Nullable<uint8_t> aNewSpeedSetting)
+{
+    if (aNewSpeedSetting.IsNull())
+    {
+        ChipLogError(NotSpecified, "ChefFanControlManager::SetSpeedSetting: null value is invalid");
+        return;
+    }
+
+    if (aNewSpeedSetting.Value() != mSpeedCurrent)
+    {
+        Status status = FanControl::Attributes::SpeedSetting::Set(mEndpoint, aNewSpeedSetting);
+        if (status != Status::Success)
+        {
+            ChipLogError(NotSpecified, "ChefFanControlManager::SetSpeedSetting: failed to set SpeedSetting attribute: %d",
+                         to_underlying(status));
+        }
+    }
+}
+
+void ChefFanControlManager::Init()
+{
+    SetPercentCurrent(GetPercentSetting().ValueOr(0));
+    SetSpeedCurrent(GetSpeedSetting().ValueOr(0));
+}
+
+DataModel::Nullable<Percent> ChefFanControlManager::GetPercentSetting()
+{
+    DataModel::Nullable<Percent> percentSetting;
+    Status status = FanControl::Attributes::PercentSetting::Get(mEndpoint, percentSetting);
+
+    if (status != Status::Success)
+    {
+        ChipLogError(NotSpecified, "ChefFanControlManager::GetPercentSetting: failed to get PercentSetting attribute: %d",
+                     to_underlying(status));
+    }
+
+    return percentSetting;
+}
+
+DataModel::Nullable<uint8_t> ChefFanControlManager::GetSpeedSetting()
+{
+    DataModel::Nullable<uint8_t> speedSetting;
+    Status status = FanControl::Attributes::SpeedSetting::Get(mEndpoint, speedSetting);
+
+    if (status != Status::Success)
+    {
+        ChipLogError(NotSpecified, "ChefFanControlManager::GetSpeedSetting: failed to get SpeedSetting attribute: %d",
+                     to_underlying(status));
+    }
+
+    return speedSetting;
 }
 
 } // anonymous namespace
@@ -199,6 +348,11 @@ void emberAfFanControlClusterInitCallback(EndpointId endpoint)
 {
     VerifyOrDie(!mFanControlManager);
     mFanControlManager = std::make_unique<ChefFanControlManager>(endpoint);
-    AttributeAccessInterfaceRegistry::Instance().Register(mFanControlManager.get());
     FanControl::SetDefaultDelegate(endpoint, mFanControlManager.get());
+    mFanControlManager->Init();
+}
+
+void HandleFanControlAttributeChange(AttributeId attributeId, uint8_t type, uint16_t size, uint8_t * value)
+{
+    mFanControlManager->HandleFanControlAttributeChange(attributeId, type, size, value);
 }

examples/chef/common/chef-fan-control-manager.h

@@ -0,0 +1,22 @@
+/*
+ *
+ *    Copyright (c) 2024 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <lib/core/DataModelTypes.h>
+
+#ifdef MATTER_DM_PLUGIN_FAN_CONTROL_SERVER
+void HandleFanControlAttributeChange(AttributeId attributeId, uint8_t type, uint16_t size, uint8_t * value);
+#endif

examples/chef/common/stubs.cpp

@@ -74,6 +74,9 @@ const Clusters::Descriptor::Structs::SemanticTagStruct::Type freezerTagList[]
 #include "chef-operational-state-delegate-impl.h"
 #endif // MATTER_DM_PLUGIN_OPERATIONAL_STATE_SERVER
 
+#ifdef MATTER_DM_PLUGIN_FAN_CONTROL_SERVER
+#include "chef-fan-control-manager.h"
+#endif // MATTER_DM_PLUGIN_FAN_CONTROL_SERVER
 #ifdef MATTER_DM_PLUGIN_TEMPERATURE_CONTROL_SERVER
 #include "temperature-control/static-supported-temperature-levels.h"
 #endif // MATTER_DM_PLUGIN_TEMPERATURE_CONTROL_SERVER
@@ -253,6 +256,12 @@ void MatterPostAttributeChangeCallback(const chip::app::ConcreteAttributePath &
 
         // WIP Apply attribute change to Light
     }
+#ifdef MATTER_DM_PLUGIN_FAN_CONTROL_SERVER
+    else if (clusterId == FanControl::Id)
+    {
+        HandleFanControlAttributeChange(attributeId, type, size, value);
+    }
+#endif // MATTER_DM_PLUGIN_FAN_CONTROL_SERVER
 }
 
 /** @brief OnOff Cluster Init