From 3ee653da37feb186b2b42ccbc9bb1e4ce60d343d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 30 Nov 2022 10:25:06 -0500 Subject: [PATCH] Move Level Control OnOff feature check into OnOff cluster. (#23820) This simplifies the coupling a bit by doing this runtime feature check in the same place where we are already doing the runtime check for existence of a relevant Level Control cluster. This allows us to remove some code in Level Control that was duplicating existing code in On/Off. --- .../clusters/level-control/level-control.cpp | 16 +-------------- .../clusters/level-control/level-control.h | 13 ++++++++---- .../clusters/on-off-server/on-off-server.cpp | 20 +++++++++++++++++-- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/app/clusters/level-control/level-control.cpp b/src/app/clusters/level-control/level-control.cpp index 22db98f75f2a0a..723cc3a1edaf33 100644 --- a/src/app/clusters/level-control/level-control.cpp +++ b/src/app/clusters/level-control/level-control.cpp @@ -1084,20 +1084,6 @@ void emberAfOnOffClusterLevelControlEffectCallback(EndpointId endpoint, bool new return; } - // if the OnOff feature is not supported, the effect on LevelControl is ignored - if (!HasFeature(endpoint, chip::app::Clusters::LevelControl::LevelControlFeature::kOnOff)) - { - emberAfLevelControlClusterPrintln("OnOff feature not supported, ignore LevelControlEffect"); - if (!newValue) - { - // OnOff server expects LevelControl to handle the OnOff attribute change, - // when going to off state. The attribute is set directly rather - // than using setOnOff function to avoid misleading comments in log. - OnOff::Attributes::OnOff::Set(endpoint, OnOff::Commands::Off::Id); - } - return; - } - uint8_t minimumLevelAllowedForTheDevice = state->minLevel; // "Temporarily store CurrentLevel." @@ -1312,7 +1298,7 @@ static bool areStartUpLevelControlServerAttributesNonVolatile(EndpointId endpoin void emberAfPluginLevelControlClusterServerPostInitCallback(EndpointId endpoint) {} -bool HasFeature(chip::EndpointId endpoint, chip::app::Clusters::LevelControl::LevelControlFeature feature) +bool LevelControlHasFeature(EndpointId endpoint, LevelControlFeature feature) { bool success; uint32_t featureMap; diff --git a/src/app/clusters/level-control/level-control.h b/src/app/clusters/level-control/level-control.h index d87d41d6ee7482..0ae5105d93a841 100644 --- a/src/app/clusters/level-control/level-control.h +++ b/src/app/clusters/level-control/level-control.h @@ -26,16 +26,21 @@ #include -#include +#include #include -/** @brief On/off Cluster Server Post Init +/** @brief Level Control Cluster Server Post Init * - * Following resolution of the On/Off state at startup for this endpoint, perform any + * Following resolution of the Level Control state at startup for this endpoint, perform any * additional initialization needed; e.g., synchronize hardware state. * * @param endpoint Endpoint that is being initialized Ver.: always */ void emberAfPluginLevelControlClusterServerPostInitCallback(chip::EndpointId endpoint); -bool HasFeature(chip::EndpointId endpoint, chip::app::Clusters::LevelControl::LevelControlFeature feature); +/** + * Check whether the instance of the Level Control cluster on the given endpoint + * has the given feature. The implementation is allowed to assume there is in + * fact an instance of Level Control on the given endpoint. + */ +bool LevelControlHasFeature(chip::EndpointId endpoint, chip::app::Clusters::LevelControl::LevelControlFeature feature); diff --git a/src/app/clusters/on-off-server/on-off-server.cpp b/src/app/clusters/on-off-server/on-off-server.cpp index 24acfcb2d81d06..260051d636a78c 100644 --- a/src/app/clusters/on-off-server/on-off-server.cpp +++ b/src/app/clusters/on-off-server/on-off-server.cpp @@ -28,6 +28,10 @@ #include #endif // EMBER_AF_PLUGIN_SCENES +#ifdef EMBER_AF_PLUGIN_LEVEL_CONTROL +#include +#endif // EMBER_AF_PLUGIN_LEVEL_CONTROL + using namespace chip; using namespace chip::app::Clusters; using namespace chip::app::Clusters::OnOff; @@ -77,6 +81,18 @@ EmberAfStatus OnOffServer::getOnOffValue(chip::EndpointId endpoint, bool * curre return status; } +#ifdef EMBER_AF_PLUGIN_LEVEL_CONTROL +static bool LevelControlWithOnOffFeaturePresent(EndpointId endpoint) +{ + if (!emberAfContainsServer(endpoint, LevelControl::Id)) + { + return false; + } + + return LevelControlHasFeature(endpoint, LevelControl::LevelControlFeature::kOnOff); +} +#endif // EMBER_AF_PLUGIN_LEVEL_CONTROL + /** @brief On/off Cluster Set Value * * This function is called when the on/off value needs to be set, either through @@ -153,7 +169,7 @@ EmberAfStatus OnOffServer::setOnOffValue(chip::EndpointId endpoint, chip::Comman #ifdef EMBER_AF_PLUGIN_LEVEL_CONTROL // If initiatedByLevelChange is false, then we assume that the level change // ZCL stuff has not happened and we do it here - if (!initiatedByLevelChange && emberAfContainsServer(endpoint, LevelControl::Id)) + if (!initiatedByLevelChange && LevelControlWithOnOffFeaturePresent(endpoint)) { emberAfOnOffClusterLevelControlEffectCallback(endpoint, newValue); } @@ -183,7 +199,7 @@ EmberAfStatus OnOffServer::setOnOffValue(chip::EndpointId endpoint, chip::Comman #ifdef EMBER_AF_PLUGIN_LEVEL_CONTROL // If initiatedByLevelChange is false, then we assume that the level change // ZCL stuff has not happened and we do it here - if (!initiatedByLevelChange && emberAfContainsServer(endpoint, LevelControl::Id)) + if (!initiatedByLevelChange && LevelControlWithOnOffFeaturePresent(endpoint)) { emberAfOnOffClusterLevelControlEffectCallback(endpoint, newValue); }