From 49f76801ac17bdcade23d3317a48b24e88053127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Josefsen?= <69624991+ReneJosefsen@users.noreply.github.com> Date: Thu, 19 Jan 2023 15:36:13 +0100 Subject: [PATCH] [Binding] Improve binding validation and disable connection on init (#23749) * Ability to notify specific binding and disable connection on init * Aligned param naming * Fixes from restyle * Missing restyle * Changed member name and added documentation * Updated validation in bindings and removed AtIndex function * Added missing include file * Fixed missing emberAfContainsClient by using other available functions * Added check of emberAfFindEndpointType in case it returns nullptr * Reintroduced emberAfContainsClient and reverted bindings to use this * Enabled onOff client on all-clusters-app to fix failing tests * Fixed spelling error and added notify if not list operation * Update src/app/clusters/bindings/bindings.cpp Co-authored-by: Boris Zbarsky * Try to fix CI failure Co-authored-by: Boris Zbarsky --- .../all-clusters-app.matter | 48 +++++++++++++++ .../all-clusters-common/all-clusters-app.zap | 2 +- src/app/clusters/bindings/BindingManager.cpp | 16 +++-- src/app/clusters/bindings/BindingManager.h | 2 + src/app/clusters/bindings/bindings.cpp | 58 ++++++++++++++++--- src/app/util/af.h | 8 +++ src/app/util/attribute-storage.cpp | 12 ++++ 7 files changed, 131 insertions(+), 15 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter index 5360be9589d7c4..18ca22ae62fa7c 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter @@ -215,6 +215,53 @@ server cluster Scenes = 5 { fabric command GetSceneMembership(GetSceneMembershipRequest): GetSceneMembershipResponse = 6; } +client cluster OnOff = 6 { + enum OnOffDelayedAllOffEffectVariant : ENUM8 { + kFadeToOffIn0p8Seconds = 0; + kNoFade = 1; + k50PercentDimDownIn0p8SecondsThenFadeToOffIn12Seconds = 2; + } + + enum OnOffDyingLightEffectVariant : ENUM8 { + k20PercenterDimUpIn0p5SecondsThenFadeToOffIn1Second = 0; + } + + enum OnOffEffectIdentifier : ENUM8 { + kDelayedAllOff = 0; + kDyingLight = 1; + } + + enum OnOffStartUpOnOff : ENUM8 { + kOff = 0; + kOn = 1; + kTogglePreviousOnOff = 2; + } + + bitmap OnOffControl : BITMAP8 { + kAcceptOnlyWhenOn = 0x1; + } + + bitmap OnOffFeature : BITMAP32 { + kLighting = 0x1; + } + + bitmap SceneFeatures : BITMAP32 { + kSceneNames = 0x1; + } + + readonly attribute boolean onOff = 0; + readonly attribute boolean globalSceneControl = 16384; + attribute int16u onTime = 16385; + attribute int16u offWaitTime = 16386; + attribute access(write: manage) nullable OnOffStartUpOnOff startUpOnOff = 16387; + readonly attribute bitmap32 featureMap = 65532; + readonly attribute int16u clusterRevision = 65533; + + command Off(): DefaultSuccess = 0; + command On(): DefaultSuccess = 1; + command Toggle(): DefaultSuccess = 2; +} + server cluster OnOff = 6 { enum OnOffDelayedAllOffEffectVariant : ENUM8 { kFadeToOffIn0p8Seconds = 0; @@ -4281,6 +4328,7 @@ endpoint 0 { } endpoint 1 { device type onofflight = 256, version 1; + binding cluster OnOff; server cluster Identify { ram attribute identifyTime; diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap index 96fa40024e8596..492ab63d4b5a91 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap @@ -9366,7 +9366,7 @@ "mfgCode": null, "define": "ON_OFF_CLUSTER", "side": "client", - "enabled": 0, + "enabled": 1, "commands": [ { "name": "Off", diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index ef657300accecc..35ebecda2b3be4 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -83,13 +83,19 @@ CHIP_ERROR BindingManager::Init(const BindingManagerInitParams & params) } else { - for (const EmberBindingTableEntry & entry : BindingTable::GetInstance()) + // In case the application does not want the BindingManager to establish a CASE session + // to the available bindings, it can be disabled by setting mEstablishConnectionOnInit + // to false. + if (params.mEstablishConnectionOnInit) { - if (entry.type == EMBER_UNICAST_BINDING) + for (const EmberBindingTableEntry & entry : BindingTable::GetInstance()) { - // The CASE connection can also fail if the unicast peer is offline. - // There is recovery mechanism to retry connection on-demand so ignore error. - (void) UnicastBindingCreated(entry.fabricIndex, entry.nodeId); + if (entry.type == EMBER_UNICAST_BINDING) + { + // The CASE connection can also fail if the unicast peer is offline. + // There is recovery mechanism to retry connection on-demand so ignore error. + (void) UnicastBindingCreated(entry.fabricIndex, entry.nodeId); + } } } } diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 5dab4772506555..c5828b42ac30a6 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -53,6 +53,7 @@ struct BindingManagerInitParams FabricTable * mFabricTable = nullptr; CASESessionManager * mCASESessionManager = nullptr; PersistentStorageDelegate * mStorage = nullptr; + bool mEstablishConnectionOnInit = true; }; /** @@ -61,6 +62,7 @@ struct BindingManagerInitParams * when a binding is ready to be communicated with. * * A CASE connection will be triggered when: + * - During init of the BindingManager, unless the application actively disables this using mEstablishConnectionOnInit * - The binding cluster adds a unicast entry to the binding table. * - A watched cluster changes with a unicast binding but we cannot find an active connection to the peer. * diff --git a/src/app/clusters/bindings/bindings.cpp b/src/app/clusters/bindings/bindings.cpp index a3b10c27f39bd5..65d2c90a8d65c7 100644 --- a/src/app/clusters/bindings/bindings.cpp +++ b/src/app/clusters/bindings/bindings.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include using namespace chip; @@ -45,6 +46,7 @@ class BindingTableAccess : public AttributeAccessInterface CHIP_ERROR Read(const ConcreteReadAttributePath & path, AttributeValueEncoder & encoder) override; CHIP_ERROR Write(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder) override; + void OnListWriteEnd(const app::ConcreteAttributePath & aPath, bool aWriteWasSuccessful) override; private: CHIP_ERROR ReadBindingTable(EndpointId endpoint, AttributeValueEncoder & encoder); @@ -55,19 +57,45 @@ class BindingTableAccess : public AttributeAccessInterface BindingTableAccess gAttrAccess; -bool IsValidBinding(const TargetStructType & entry) +bool IsValidBinding(const EndpointId localEndpoint, const TargetStructType & entry) { - return (!entry.group.HasValue() && entry.endpoint.HasValue() && entry.node.HasValue()) || - (!entry.endpoint.HasValue() && !entry.node.HasValue() && entry.group.HasValue()); + bool isValid = false; + + // Entry has endpoint, node id and no group id + if (!entry.group.HasValue() && entry.endpoint.HasValue() && entry.node.HasValue()) + { + if (entry.cluster.HasValue()) + { + if (emberAfContainsClient(localEndpoint, entry.cluster.Value())) + { + // Valid node/endpoint/cluster binding + isValid = true; + } + } + else + { + // Valid node/endpoint (no cluster id) binding + isValid = true; + } + } + // Entry has group id and no endpoint and node id + else if (!entry.endpoint.HasValue() && !entry.node.HasValue() && entry.group.HasValue()) + { + // Valid group binding + isValid = true; + } + + return isValid; } -CHIP_ERROR CheckValidBindingList(const DecodableBindingListType & bindingList, FabricIndex accessingFabricIndex) +CHIP_ERROR CheckValidBindingList(const EndpointId localEndpoint, const DecodableBindingListType & bindingList, + FabricIndex accessingFabricIndex) { size_t listSize = 0; auto iter = bindingList.begin(); while (iter.Next()) { - VerifyOrReturnError(IsValidBinding(iter.GetValue()), CHIP_IM_GLOBAL_STATUS(ConstraintError)); + VerifyOrReturnError(IsValidBinding(localEndpoint, iter.GetValue()), CHIP_IM_GLOBAL_STATUS(ConstraintError)); listSize++; } ReturnErrorOnFailure(iter.GetStatus()); @@ -159,6 +187,12 @@ CHIP_ERROR BindingTableAccess::Write(const ConcreteDataAttributePath & path, Att return CHIP_NO_ERROR; } +void BindingTableAccess::OnListWriteEnd(const app::ConcreteAttributePath & aPath, bool aWriteWasSuccessful) +{ + // Notify binding table has changed + LogErrorOnFailure(NotifyBindingsChanged()); +} + CHIP_ERROR BindingTableAccess::WriteBindingTable(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder) { FabricIndex accessingFabricIndex = decoder.AccessingFabricIndex(); @@ -167,7 +201,7 @@ CHIP_ERROR BindingTableAccess::WriteBindingTable(const ConcreteDataAttributePath DecodableBindingListType newBindingList; ReturnErrorOnFailure(decoder.Decode(newBindingList)); - ReturnErrorOnFailure(CheckValidBindingList(newBindingList, accessingFabricIndex)); + ReturnErrorOnFailure(CheckValidBindingList(path.mEndpointId, newBindingList, accessingFabricIndex)); // Clear all entries for the current accessing fabric and endpoint auto bindingTableIter = BindingTable::GetInstance().begin(); @@ -193,19 +227,25 @@ CHIP_ERROR BindingTableAccess::WriteBindingTable(const ConcreteDataAttributePath { CreateBindingEntry(iter.GetValue(), path.mEndpointId); } - LogErrorOnFailure(NotifyBindingsChanged()); + + // If this was not caused by a list operation, OnListWriteEnd is not going to be triggered + // so a notification is sent here. + if (!path.IsListOperation()) + { + // Notify binding table has changed + LogErrorOnFailure(NotifyBindingsChanged()); + } return CHIP_NO_ERROR; } if (path.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem) { TargetStructType target; ReturnErrorOnFailure(decoder.Decode(target)); - if (!IsValidBinding(target)) + if (!IsValidBinding(path.mEndpointId, target)) { return CHIP_IM_GLOBAL_STATUS(ConstraintError); } CreateBindingEntry(target, path.mEndpointId); - LogErrorOnFailure(NotifyBindingsChanged()); return CHIP_NO_ERROR; } return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); diff --git a/src/app/util/af.h b/src/app/util/af.h index 9c9bad4b3f1345..9bc5dbca599bda 100644 --- a/src/app/util/af.h +++ b/src/app/util/af.h @@ -80,6 +80,14 @@ bool emberAfContainsServer(chip::EndpointId endpoint, chip::ClusterId clusterId) */ bool emberAfContainsServerFromIndex(uint16_t index, chip::ClusterId clusterId); +/** + * @brief Returns true if endpoint contains the ZCL client with specified id. + * + * This function returns true if + * the endpoint contains client of a given cluster. + */ +bool emberAfContainsClient(chip::EndpointId endpoint, chip::ClusterId clusterId); + /** * @brief write an attribute, performing all the checks. * diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index e30b990546b62d..db55e2bd6e1d7e 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -675,6 +675,18 @@ bool emberAfContainsServer(EndpointId endpoint, ClusterId clusterId) return (emberAfFindServerCluster(endpoint, clusterId) != nullptr); } +// Returns whether the given endpoint has the client of the given cluster on it. +bool emberAfContainsClient(EndpointId endpoint, ClusterId clusterId) +{ + uint16_t ep = emberAfIndexFromEndpoint(endpoint); + if (ep == kEmberInvalidEndpointIndex) + { + return false; + } + + return (emberAfFindClusterInType(emAfEndpoints[ep].endpointType, clusterId, CLUSTER_MASK_CLIENT) != nullptr); +} + // This will find the first server that has the clusterId given from the index of endpoint. bool emberAfContainsServerFromIndex(uint16_t index, ClusterId clusterId) {