[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 <[email protected]> * Try to fix CI failure Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Feb 5, 2024 · 49f7680 · 49f7680
1 parent 153864f
commit 49f7680
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -26,6 +26,7 @@
 #include <app/CommandHandler.h>
 #include <app/ConcreteAttributePath.h>
 #include <app/clusters/bindings/bindings.h>
+#include <app/util/af.h>
 #include <app/util/attribute-storage.h>
 #include <lib/support/logging/CHIPLogging.h>
 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
@@ -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
@@ -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)
 {