fix review comments

project-chip · Jan 27, 2022 · 5ff4f87 · 5ff4f87
1 parent dd8ca50
commit 5ff4f87
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 63 deletions.
diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp
@@ -36,15 +36,9 @@ namespace chip {
 
 BindingManager BindingManager::sBindingManager;
 
-CHIP_ERROR BindingManager::UnicastBindingCreated(uint8_t bindingEntryId)
+CHIP_ERROR BindingManager::UnicastBindingCreated(const EmberBindingTableEntry & bindingEntry)
 {
-    EmberBindingTableEntry entry{};
-    emberGetBinding(bindingEntryId, &entry);
-    if (entry.type != EMBER_UNICAST_BINDING)
-    {
-        return CHIP_ERROR_INVALID_ARGUMENT;
-    }
-    return EstablishConnection(entry.fabricIndex, entry.nodeId);
+    return EstablishConnection(bindingEntry.fabricIndex, bindingEntry.nodeId);
 }
 
 CHIP_ERROR BindingManager::UnicastBindingRemoved(uint8_t bindingEntryId)
@@ -72,13 +66,12 @@ CHIP_ERROR BindingManager::EstablishConnection(FabricIndex fabric, NodeId node)
         // Release the least recently used entry
         // TODO: Some reference counting mechanism shall be added the CASESessionManager
         // so that other session clients don't get accidentally closed.
-        int16_t lruBindingEntryIndex = mPendingNotificationMap.FindLRUBindingEntryIndex();
-        if (lruBindingEntryIndex >= 0)
+        FabricIndex fabricToRemove;
+        NodeId nodeToRemove;
+        if (mPendingNotificationMap.FindLRUConnectPeer(&fabricToRemove, &nodeToRemove) == CHIP_NO_ERROR)
         {
-            EmberBindingTableEntry entry;
-            emberGetBinding(static_cast<uint8_t>(lruBindingEntryIndex), &entry);
-            mPendingNotificationMap.RemoveAllEntriesForNode(entry.fabricIndex, entry.nodeId);
-            PeerId lruPeer = PeerIdForNode(mAppServer->GetFabricTable(), entry.fabricIndex, entry.nodeId);
+            mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove);
+            PeerId lruPeer = PeerIdForNode(mAppServer->GetFabricTable(), fabricToRemove, nodeToRemove);
             mAppServer->GetCASESessionManager()->ReleaseSession(lruPeer);
             // Now retry
             error = mAppServer->GetCASESessionManager()->FindOrEstablishSession(peer, &mOnConnectedCallback,
@@ -96,6 +89,9 @@ void BindingManager::HandleDeviceConnected(void * context, OperationalDeviceProx
 
 void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device)
 {
+    FabricIndex fabricToRemove = kUndefinedFabricIndex;
+    NodeId nodeToRemove        = kUndefinedNodeId;
+
     for (const PendingNotificationEntry & pendingNotification : mPendingNotificationMap)
     {
         EmberBindingTableEntry entry;
@@ -104,9 +100,12 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device)
         PeerId peer = PeerIdForNode(mAppServer->GetFabricTable(), entry.fabricIndex, entry.nodeId);
         if (device->GetPeerId() == peer)
         {
+            fabricToRemove = entry.fabricIndex;
+            nodeToRemove   = entry.nodeId;
             mBoundDeviceChangedHandler(&entry, device, pendingNotification.mContext);
         }
     }
+    mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove);
 }
 
 void BindingManager::HandleDeviceConnectionFailure(void * context, PeerId peerId, CHIP_ERROR error)

diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h
@@ -67,10 +67,10 @@ class BindingManager
      * Notifies the BindingManager that a new unicast binding is created.
      *
      */
-    CHIP_ERROR UnicastBindingCreated(uint8_t bindingEntryId);
+    CHIP_ERROR UnicastBindingCreated(const EmberBindingTableEntry & bindingEntry);
 
     /*
-     * Notifies the BindingManager that a new unicast binding is removed.
+     * Notifies the BindingManager that a unicast binding is about to be removed from the given index.
      *
      */
     CHIP_ERROR UnicastBindingRemoved(uint8_t bindingEntryId);

diff --git a/src/app/clusters/bindings/PendingNotificationMap.cpp b/src/app/clusters/bindings/PendingNotificationMap.cpp
@@ -22,7 +22,7 @@
 
 namespace chip {
 
-int16_t PendingNotificationMap::FindLRUBindingEntryIndex()
+CHIP_ERROR PendingNotificationMap::FindLRUConnectPeer(FabricIndex * fabric, NodeId * node)
 {
     uint8_t bindingWithSamePeer[EMBER_BINDING_TABLE_SIZE];
 
@@ -64,8 +64,8 @@ int16_t PendingNotificationMap::FindLRUBindingEntryIndex()
         lastAppear[bindingWithSamePeer[pendingNotification.mBindingEntryId]] = appearIndex;
         appearIndex++;
     }
-    int16_t lruBindingEntryIndex = -1;
-    uint16_t minApperValue       = UINT16_MAX;
+    uint8_t lruBindingEntryIndex;
+    uint16_t minApperValue = UINT16_MAX;
     for (uint8_t i = 0; i < EMBER_BINDING_TABLE_SIZE; i++)
     {
         if (lastAppear[i] < minApperValue)
@@ -74,65 +74,56 @@ int16_t PendingNotificationMap::FindLRUBindingEntryIndex()
             minApperValue        = lastAppear[i];
         }
     }
-    return lruBindingEntryIndex;
+    if (minApperValue < UINT16_MAX)
+    {
+        EmberBindingTableEntry entry;
+        emberGetBinding(lruBindingEntryIndex, &entry);
+        *fabric = entry.fabricIndex;
+        *node   = entry.nodeId;
+        return CHIP_NO_ERROR;
+    }
+    return CHIP_ERROR_NOT_FOUND;
 }
 
 void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, void * context)
 {
     RemoveEntry(bindingEntryId);
-    uint8_t addIndex                 = mNumEntries < kMaxPendingNotifications ? mNumEntries : mNextToOverride;
-    mPendingBindingEntries[addIndex] = bindingEntryId;
-    mPendingContexts[addIndex]       = context;
-    if (mNumEntries < kMaxPendingNotifications)
-    {
-        mNumEntries++;
-    }
-    else
-    {
-        mNextToOverride++;
-    }
+    mPendingBindingEntries[mNumEntries] = bindingEntryId;
+    mPendingContexts[mNumEntries]       = context;
+    mNumEntries++;
 }
 
 void PendingNotificationMap::RemoveEntry(uint8_t bindingEntryId)
 {
-    uint8_t newBindingEntries[kMaxPendingNotifications];
-    void * newContexts[kMaxPendingNotifications];
     uint8_t newEntryCount = 0;
-    for (const PendingNotificationEntry & pendingNotification : *this)
+    for (int i = 0; i < mNumEntries; i++)
     {
-        if (pendingNotification.mBindingEntryId != bindingEntryId)
+        if (mPendingBindingEntries[i] != bindingEntryId)
         {
-            newBindingEntries[newEntryCount] = pendingNotification.mBindingEntryId;
-            newContexts[newEntryCount]       = pendingNotification.mContext;
+            mPendingBindingEntries[newEntryCount] = mPendingBindingEntries[i];
+            mPendingContexts[newEntryCount]       = mPendingContexts[i];
             newEntryCount++;
         }
     }
-    memcpy(mPendingBindingEntries, newBindingEntries, sizeof(newBindingEntries));
-    memcpy(mPendingContexts, newContexts, sizeof(newContexts));
-    mNextToOverride = 0;
-    mNumEntries     = newEntryCount;
+    mNumEntries = newEntryCount;
 }
 
 void PendingNotificationMap::RemoveAllEntriesForNode(FabricIndex fabric, NodeId node)
 {
-    uint8_t newBindingEntries[kMaxPendingNotifications];
-    void * newContexts[kMaxPendingNotifications];
     uint8_t newEntryCount = 0;
-    for (const PendingNotificationEntry & pendingNotification : *this)
+    for (int i = 0; i < mNumEntries; i++)
     {
         EmberBindingTableEntry entry;
-        emberGetBinding(pendingNotification.mBindingEntryId, &entry);
+        emberGetBinding(mPendingBindingEntries[i], &entry);
+
         if (entry.fabricIndex != fabric || entry.nodeId != node)
         {
-            newBindingEntries[newEntryCount] = pendingNotification.mBindingEntryId;
-            newContexts[newEntryCount]       = pendingNotification.mContext;
+            mPendingBindingEntries[newEntryCount] = mPendingBindingEntries[i];
+            mPendingContexts[newEntryCount]       = mPendingContexts[i];
             newEntryCount++;
         }
     }
-    memcpy(mPendingBindingEntries, newBindingEntries, sizeof(newBindingEntries));
-    memcpy(mPendingContexts, newContexts, sizeof(newContexts));
-    mNextToOverride = 0;
-    mNumEntries     = newEntryCount;
+    mNumEntries = newEntryCount;
 }
 
 } // namespace chip
diff --git a/src/app/clusters/bindings/PendingNotificationMap.h b/src/app/clusters/bindings/PendingNotificationMap.h
@@ -48,12 +48,6 @@ class PendingNotificationMap
         Iterator operator++()
         {
             mIndex++;
-            if (mIndex == mMap->mNumEntries)
-            {
-                // end of iteration
-                mIndex = -1;
-            }
-            mIndex = static_cast<int16_t>(mIndex % kMaxPendingNotifications);
             return *this;
         }
 
@@ -64,11 +58,11 @@ class PendingNotificationMap
         int16_t mIndex;
     };
 
-    Iterator begin() { return Iterator(this, mNumEntries > 0 ? mNextToOverride : -1); }
+    Iterator begin() { return Iterator(this, 0); }
 
-    Iterator end() { return Iterator(this, -1); }
+    Iterator end() { return Iterator(this, mNumEntries); }
 
-    int16_t FindLRUBindingEntryIndex();
+    CHIP_ERROR FindLRUConnectPeer(FabricIndex * fabric, NodeId * node);
 
     void AddPendingNotification(uint8_t bindingEntryId, void * context);
 
@@ -80,8 +74,7 @@ class PendingNotificationMap
     uint8_t mPendingBindingEntries[kMaxPendingNotifications];
     void * mPendingContexts[kMaxPendingNotifications];
 
-    uint8_t mNumEntries     = 0;
-    uint8_t mNextToOverride = 0;
+    uint8_t mNumEntries = 0;
 };
 
 } // namespace chip
diff --git a/src/app/clusters/bindings/bindings.cpp b/src/app/clusters/bindings/bindings.cpp
@@ -111,7 +111,7 @@ bool emberAfBindingClusterBindCallback(app::CommandHandler * commandObj, const a
     emberSetBinding(bindingIndex, &bindingEntry);
     if (nodeId)
     {
-        CHIP_ERROR err = BindingManager::GetInstance().UnicastBindingCreated(bindingIndex);
+        CHIP_ERROR err = BindingManager::GetInstance().UnicastBindingCreated(bindingEntry);
         if (err != CHIP_NO_ERROR)
         {
             ChipLogProgress(