From 1c5e130e75846c6c958662ebb5e821fadd5bb545 Mon Sep 17 00:00:00 2001 From: Sergio Soares Date: Tue, 12 Nov 2024 15:28:58 -0500 Subject: [PATCH] chef: Fix FanControl mode/speed interdependence behavior (#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 * 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 * fix const * Apply suggestions from Andrei's code review Co-authored-by: Andrei Litvin * Restyled by clang-format * Apply Boris' suggestions from code review Co-authored-by: Boris Zbarsky * Address Boris' review suggestions --------- Co-authored-by: Andrei Litvin Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- .../chef/common/chef-fan-control-manager.cpp | 294 +++++++++++++----- .../chef/common/chef-fan-control-manager.h | 22 ++ examples/chef/common/stubs.cpp | 9 + .../devices/rootnode_fan_7N2TobIlOX.matter | 2 +- .../chef/devices/rootnode_fan_7N2TobIlOX.zap | 32 +- 5 files changed, 266 insertions(+), 93 deletions(-) create mode 100644 examples/chef/common/chef-fan-control-manager.h diff --git a/examples/chef/common/chef-fan-control-manager.cpp b/examples/chef/common/chef-fan-control-manager.cpp index 8d9ed5ae8bb699..7c2aec389e08a4 100644 --- a/examples/chef/common/chef-fan-control-manager.cpp +++ b/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(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 GetSpeedSetting(); + DataModel::Nullable GetPercentSetting(); private: - Nullable mPercentSetting{}; - Nullable 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 aNewSpeedSetting); + static FanControl::FanModeEnum SpeedToFanMode(uint8_t speed); }; static std::unique_ptr 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 newSpeedSetting; - ReturnErrorOnFailure(aDecoder.Decode(newSpeedSetting)); - - // Ensure new speed is in bounds + case FanControl::Attributes::PercentSetting::Id: { + ChipLogProgress(NotSpecified, "ChefFanControlManager::HandleFanControlAttributeChange PercentSetting"); + DataModel::Nullable percentSetting; + if (!NumericAttributeTraits::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::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 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 speedSetting; + if (!NumericAttributeTraits::IsNullValue(*value)) { - return CHIP_IM_GLOBAL_STATUS(ConstraintError); + speedSetting.SetNonNull(NumericAttributeTraits::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(*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 speedSetting(0); + SetSpeedSetting(speedSetting); + } + break; } - default: + case FanControl::FanModeEnum::kLow: { + if (!kFanModeLowSpeedRange.Contains(mSpeedCurrent)) + { + DataModel::Nullable speedSetting(kFanModeLowSpeedRange.Low()); + SetSpeedSetting(speedSetting); + } break; } - return CHIP_NO_ERROR; + case FanControl::FanModeEnum::kMedium: { + if (!kFanModeMediumSpeedRange.Contains(mSpeedCurrent)) + { + DataModel::Nullable speedSetting(kFanModeMediumSpeedRange.Low()); + SetSpeedSetting(speedSetting); + } + break; + } + case FanControl::FanModeEnum::kOn: + case FanControl::FanModeEnum::kHigh: { + if (!kFanModeHighSpeedRange.Contains(mSpeedCurrent)) + { + DataModel::Nullable 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 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 ChefFanControlManager::GetPercentSetting() +{ + DataModel::Nullable 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 ChefFanControlManager::GetSpeedSetting() +{ + DataModel::Nullable 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(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); } diff --git a/examples/chef/common/chef-fan-control-manager.h b/examples/chef/common/chef-fan-control-manager.h new file mode 100644 index 00000000000000..36a90e6a886e92 --- /dev/null +++ b/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 + +#ifdef MATTER_DM_PLUGIN_FAN_CONTROL_SERVER +void HandleFanControlAttributeChange(AttributeId attributeId, uint8_t type, uint16_t size, uint8_t * value); +#endif diff --git a/examples/chef/common/stubs.cpp b/examples/chef/common/stubs.cpp index 5cbe80aa83f162..8c389028cd0530 100644 --- a/examples/chef/common/stubs.cpp +++ b/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 diff --git a/examples/chef/devices/rootnode_fan_7N2TobIlOX.matter b/examples/chef/devices/rootnode_fan_7N2TobIlOX.matter index 78e524d7eb140a..1acdbca51279a4 100644 --- a/examples/chef/devices/rootnode_fan_7N2TobIlOX.matter +++ b/examples/chef/devices/rootnode_fan_7N2TobIlOX.matter @@ -1878,7 +1878,7 @@ endpoint 1 { ram attribute fanModeSequence default = 2; persist attribute percentSetting default = 0x00; ram attribute percentCurrent default = 0x00; - ram attribute speedMax default = 100; + ram attribute speedMax default = 10; persist attribute speedSetting default = 0x00; persist attribute speedCurrent default = 0; ram attribute rockSupport default = 0x03; diff --git a/examples/chef/devices/rootnode_fan_7N2TobIlOX.zap b/examples/chef/devices/rootnode_fan_7N2TobIlOX.zap index 1f976f24c67632..39239eaef90230 100644 --- a/examples/chef/devices/rootnode_fan_7N2TobIlOX.zap +++ b/examples/chef/devices/rootnode_fan_7N2TobIlOX.zap @@ -1,6 +1,6 @@ { "fileFormat": 2, - "featureLevel": 103, + "featureLevel": 104, "creator": "zap", "keyValuePairs": [ { @@ -41,14 +41,16 @@ "code": 22, "profileId": 259, "label": "MA-rootdevice", - "name": "MA-rootdevice" + "name": "MA-rootdevice", + "deviceTypeOrder": 0 }, "deviceTypes": [ { "code": 22, "profileId": 259, "label": "MA-rootdevice", - "name": "MA-rootdevice" + "name": "MA-rootdevice", + "deviceTypeOrder": 0 } ], "deviceVersions": [ @@ -2291,14 +2293,16 @@ "code": 43, "profileId": 259, "label": "MA-fan", - "name": "MA-fan" + "name": "MA-fan", + "deviceTypeOrder": 0 }, "deviceTypes": [ { "code": 43, "profileId": 259, "label": "MA-fan", - "name": "MA-fan" + "name": "MA-fan", + "deviceTypeOrder": 0 } ], "deviceVersions": [ @@ -2885,7 +2889,7 @@ "storageOption": "RAM", "singleton": 0, "bounded": 0, - "defaultValue": "100", + "defaultValue": "10", "reportable": 1, "minInterval": 1, "maxInterval": 65534, @@ -3035,22 +3039,6 @@ "maxInterval": 65534, "reportableChange": 0 }, - { - "name": "EventList", - "code": 65530, - "mfgCode": null, - "side": "server", - "type": "array", - "included": 1, - "storageOption": "External", - "singleton": 0, - "bounded": 0, - "defaultValue": null, - "reportable": 1, - "minInterval": 1, - "maxInterval": 65534, - "reportableChange": 0 - }, { "name": "AttributeList", "code": 65531,