From bcc7109f004449dacae01e3489b2df6588aa3fe1 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Thu, 31 Oct 2024 09:51:41 -0400 Subject: [PATCH 1/2] Added write attribute access interface to prevent circular callbacks when changing speed --- .../fan-control-server/fan-control-server.cpp | 82 +++++++++++++++---- .../fan-control-server/fan-control-server.h | 10 +++ src/app/util/util.cpp | 1 - 3 files changed, 76 insertions(+), 17 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 46f4c6d7c0..05c7422338 100644 --- a/src/app/clusters/fan-control-server/fan-control-server.cpp +++ b/src/app/clusters/fan-control-server/fan-control-server.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -50,6 +51,8 @@ static_assert(kFanControlDelegateTableSize <= kEmberInvalidEndpointIndex, "FanCo Delegate * gDelegateTable[kFanControlDelegateTableSize] = { nullptr }; +FanControlAttributeAccessInterface gFanControlAttributeAccess; + } // anonymous namespace namespace chip { @@ -413,26 +416,67 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt ChipLogError(Zcl, "Failed to set FanMode to off with error: 0x%02x", to_underlying(status))); } - // 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))); + // Adjust PercentSetting from a speed value change for SpeedSetting only when the SpeedSetting change was received + // on a write command, not when it was changed by the server or the app logic. This avoids circular logic such as + //(with a SpeedMax of 10): + // 1. Client sets the PercetSetting to 25% + // 2. Server sets the SpeedSetting to 3 through the server callbackm, which sets the PercentSetting to 30% + // 3. Server sets the PercentSetting to 30% through the server callback + } + break; + } + default: + break; + } +} - 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))); +CHIP_ERROR FanControlAttributeAccessInterface::Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) +{ + Status status = Status::Success; + switch (aPath.mAttributeId) + { + case SpeedSetting::Id: { + DataModel::Nullable speedSetting; + ReturnErrorOnFailure(aDecoder.Decode(speedSetting)); - float speed = speedSetting.Value(); - Percent percentSetting = static_cast(speed / speedMax * 100); + DataModel::Nullable currentSpeedSetting; + status = SpeedSetting::Get(aPath.mEndpointId, currentSpeedSetting); + ReturnLogErrorOnFailure(StatusIB(status).ToChipError()); - if (currentPercentSetting.IsNull() || percentSetting != currentPercentSetting.Value()) + if (speedSetting != currentSpeedSetting) + { + status = SpeedSetting::Set(aPath.mEndpointId, speedSetting); + ReturnLogErrorOnFailure(StatusIB(status).ToChipError()); + // Skip the last step if we are writing NULL + VerifyOrReturnValue(!speedSetting.IsNull(), CHIP_NO_ERROR); + + if (SupportsMultiSpeed(aPath.mEndpointId)) { - status = PercentSetting::Set(attributePath.mEndpointId, percentSetting); - VerifyOrReturn(Status::Success == status, - ChipLogError(Zcl, "Failed to set PercentSetting with error: 0x%02x", to_underlying(status))); + // If SpeedSetting is set to 0, the server SHALL set the FanMode attribute value to Off. + if (speedSetting.Value() == 0) + { + status = SetFanModeToOff(aPath.mEndpointId); + ReturnLogErrorOnFailure(StatusIB(status).ToChipError()); + } + + // Adjust PercentSetting from a speed value change for SpeedSetting + // percent = floor( speed/SpeedMax * 100 ) + uint8_t speedMax; + status = SpeedMax::Get(aPath.mEndpointId, &speedMax); + ReturnLogErrorOnFailure(StatusIB(status).ToChipError()); + + DataModel::Nullable currentPercentSetting; + status = PercentSetting::Get(aPath.mEndpointId, currentPercentSetting); + ReturnLogErrorOnFailure(StatusIB(status).ToChipError()); + + float speed = speedSetting.Value(); + Percent percentSetting = static_cast(speed / speedMax * 100); + + if (currentPercentSetting.IsNull() || percentSetting != currentPercentSetting.Value()) + { + status = PercentSetting::Set(aPath.mEndpointId, percentSetting); + ReturnLogErrorOnFailure(StatusIB(status).ToChipError()); + } } } break; @@ -440,6 +484,7 @@ void MatterFanControlClusterServerAttributeChangedCallback(const app::ConcreteAt default: break; } + return CHIP_NO_ERROR; } bool emberAfFanControlClusterStepCallback(app::CommandHandler * commandObj, const app::ConcreteCommandPath & commandPath, @@ -477,3 +522,8 @@ bool emberAfFanControlClusterStepCallback(app::CommandHandler * commandObj, cons commandObj->AddStatus(commandPath, status); return true; } + +void MatterFanControlPluginServerInitCallback() +{ + AttributeAccessInterfaceRegistry::Instance().Register(&gFanControlAttributeAccess); +} diff --git a/src/app/clusters/fan-control-server/fan-control-server.h b/src/app/clusters/fan-control-server/fan-control-server.h index 5721e34b4e..dd53b0e026 100644 --- a/src/app/clusters/fan-control-server/fan-control-server.h +++ b/src/app/clusters/fan-control-server/fan-control-server.h @@ -19,6 +19,7 @@ #include "fan-control-delegate.h" #include +#include #include namespace chip { @@ -26,6 +27,15 @@ namespace app { namespace Clusters { namespace FanControl { +class FanControlAttributeAccessInterface : public AttributeAccessInterface +{ +public: + FanControlAttributeAccessInterface() : AttributeAccessInterface(Optional(), Id) {} + + CHIP_ERROR Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) override; + CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override { return CHIP_NO_ERROR; } +}; + void SetDefaultDelegate(EndpointId aEndpoint, Delegate * aDelegate); Delegate * GetDelegate(EndpointId aEndpoint); diff --git a/src/app/util/util.cpp b/src/app/util/util.cpp index 3072e8aafd..1a7089d362 100644 --- a/src/app/util/util.cpp +++ b/src/app/util/util.cpp @@ -107,7 +107,6 @@ void MatterUnitLocalizationPluginServerInitCallback() {} void MatterProxyValidPluginServerInitCallback() {} void MatterProxyDiscoveryPluginServerInitCallback() {} void MatterProxyConfigurationPluginServerInitCallback() {} -void MatterFanControlPluginServerInitCallback() {} void MatterActivatedCarbonFilterMonitoringPluginServerInitCallback() {} void MatterHepaFilterMonitoringPluginServerInitCallback() {} void MatterAirQualityPluginServerInitCallback() {} From f8f84093428d18f26a631b5b8f126d9ce38e2832 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Thu, 31 Oct 2024 14:22:06 -0400 Subject: [PATCH 2/2] Updated All-cluster app --- .../all-clusters-app/all-clusters-common/src/fan-stub.cpp | 7 ++----- src/app/clusters/fan-control-server/fan-control-server.cpp | 7 ------- src/app/clusters/fan-control-server/fan-control-server.h | 4 ++-- src/app/util/util.cpp | 1 + 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/src/fan-stub.cpp b/examples/all-clusters-app/all-clusters-common/src/fan-stub.cpp index e0fb68c95f..06de80e2f9 100644 --- a/examples/all-clusters-app/all-clusters-common/src/fan-stub.cpp +++ b/examples/all-clusters-app/all-clusters-common/src/fan-stub.cpp @@ -19,7 +19,6 @@ #include #include #include -#include #include #include #include @@ -34,13 +33,11 @@ using namespace chip::app::Clusters::FanControl::Attributes; using Protocols::InteractionModel::Status; namespace { -class FanControlManager : public AttributeAccessInterface, public Delegate +class FanControlManager : public FanControlAttributeAccessInterface, public Delegate { public: // Register for the FanControl cluster on all endpoints. - FanControlManager(EndpointId aEndpointId) : - AttributeAccessInterface(Optional(aEndpointId), FanControl::Id), Delegate(aEndpointId) - {} + FanControlManager(EndpointId aEndpointId) : FanControlAttributeAccessInterface(aEndpointId), Delegate(aEndpointId) {} CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; Status HandleStep(StepDirectionEnum aDirection, bool aWrap, bool aLowestOff) override; 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 05c7422338..636511dadd 100644 --- a/src/app/clusters/fan-control-server/fan-control-server.cpp +++ b/src/app/clusters/fan-control-server/fan-control-server.cpp @@ -51,8 +51,6 @@ static_assert(kFanControlDelegateTableSize <= kEmberInvalidEndpointIndex, "FanCo Delegate * gDelegateTable[kFanControlDelegateTableSize] = { nullptr }; -FanControlAttributeAccessInterface gFanControlAttributeAccess; - } // anonymous namespace namespace chip { @@ -522,8 +520,3 @@ bool emberAfFanControlClusterStepCallback(app::CommandHandler * commandObj, cons commandObj->AddStatus(commandPath, status); return true; } - -void MatterFanControlPluginServerInitCallback() -{ - AttributeAccessInterfaceRegistry::Instance().Register(&gFanControlAttributeAccess); -} diff --git a/src/app/clusters/fan-control-server/fan-control-server.h b/src/app/clusters/fan-control-server/fan-control-server.h index dd53b0e026..5047cf53da 100644 --- a/src/app/clusters/fan-control-server/fan-control-server.h +++ b/src/app/clusters/fan-control-server/fan-control-server.h @@ -18,8 +18,8 @@ #pragma once #include "fan-control-delegate.h" -#include #include +#include #include namespace chip { @@ -30,7 +30,7 @@ namespace FanControl { class FanControlAttributeAccessInterface : public AttributeAccessInterface { public: - FanControlAttributeAccessInterface() : AttributeAccessInterface(Optional(), Id) {} + FanControlAttributeAccessInterface(EndpointId aEndpoint) : AttributeAccessInterface(Optional(aEndpoint), Id) {} CHIP_ERROR Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) override; CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override { return CHIP_NO_ERROR; } diff --git a/src/app/util/util.cpp b/src/app/util/util.cpp index 1a7089d362..3c6acab8a0 100644 --- a/src/app/util/util.cpp +++ b/src/app/util/util.cpp @@ -110,6 +110,7 @@ void MatterProxyConfigurationPluginServerInitCallback() {} void MatterActivatedCarbonFilterMonitoringPluginServerInitCallback() {} void MatterHepaFilterMonitoringPluginServerInitCallback() {} void MatterAirQualityPluginServerInitCallback() {} +void MatterFanControlPluginServerInitCallback() {} void MatterCarbonMonoxideConcentrationMeasurementPluginServerInitCallback() {} void MatterCarbonDioxideConcentrationMeasurementPluginServerInitCallback() {} void MatterFormaldehydeConcentrationMeasurementPluginServerInitCallback() {}