diff --git a/examples/all-clusters-app/ameba/main/BindingHandler.cpp b/examples/all-clusters-app/ameba/main/BindingHandler.cpp index ccfb210a50a80d..29acdaae952896 100644 --- a/examples/all-clusters-app/ameba/main/BindingHandler.cpp +++ b/examples/all-clusters-app/ameba/main/BindingHandler.cpp @@ -133,12 +133,20 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); + + Platform::Delete(static_cast(context)); +} + void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } #ifdef CONFIG_ENABLE_CHIP_SHELL @@ -400,8 +408,6 @@ void SwitchWorkerFunction(intptr_t context) BindingCommandData * data = reinterpret_cast(context); BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast(data)); - - Platform::Delete(data); } void BindingWorkerFunction(intptr_t context) diff --git a/examples/light-switch-app/ameba/main/BindingHandler.cpp b/examples/light-switch-app/ameba/main/BindingHandler.cpp index ccfb210a50a80d..29acdaae952896 100644 --- a/examples/light-switch-app/ameba/main/BindingHandler.cpp +++ b/examples/light-switch-app/ameba/main/BindingHandler.cpp @@ -133,12 +133,20 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); + + Platform::Delete(static_cast(context)); +} + void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } #ifdef CONFIG_ENABLE_CHIP_SHELL @@ -400,8 +408,6 @@ void SwitchWorkerFunction(intptr_t context) BindingCommandData * data = reinterpret_cast(context); BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast(data)); - - Platform::Delete(data); } void BindingWorkerFunction(intptr_t context) diff --git a/examples/light-switch-app/esp32/main/BindingHandler.cpp b/examples/light-switch-app/esp32/main/BindingHandler.cpp index 179c8e4a7c7a41..5cc520fd2accd0 100644 --- a/examples/light-switch-app/esp32/main/BindingHandler.cpp +++ b/examples/light-switch-app/esp32/main/BindingHandler.cpp @@ -131,12 +131,20 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); + + Platform::Delete(static_cast(context)); +} + void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } #ifdef CONFIG_ENABLE_CHIP_SHELL @@ -398,8 +406,6 @@ void SwitchWorkerFunction(intptr_t context) BindingCommandData * data = reinterpret_cast(context); BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast(data)); - - Platform::Delete(data); } void BindingWorkerFunction(intptr_t context) diff --git a/examples/light-switch-app/telink/src/binding-handler.cpp b/examples/light-switch-app/telink/src/binding-handler.cpp index 9483448c70b34b..ed6b07f8df467e 100755 --- a/examples/light-switch-app/telink/src/binding-handler.cpp +++ b/examples/light-switch-app/telink/src/binding-handler.cpp @@ -133,6 +133,13 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); + + Platform::Delete(static_cast(context)); +} + #ifdef ENABLE_CHIP_SHELL /******************************************************** @@ -385,6 +392,7 @@ void InitBindingHandlerInternal(intptr_t arg) chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } } // namespace @@ -413,8 +421,6 @@ void SwitchWorkerFunction(intptr_t context) BindingCommandData * data = reinterpret_cast(context); BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast(data)); - - Platform::Delete(data); } void BindingWorkerFunction(intptr_t context) diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index db07abe3ed9f34..ad13376a0dfe42 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -101,12 +101,12 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) VerifyOrReturnError(mInitParams.mCASESessionManager != nullptr, CHIP_ERROR_INCORRECT_STATE); mLastSessionEstablishmentError = CHIP_NO_ERROR; - mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, &mOnConnectedCallback, &mOnConnectionFailureCallback); + auto * connectionCallback = Platform::New(*this); + mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, connectionCallback->GetOnDeviceConnected(), + connectionCallback->GetOnDeviceConnectionFailure()); if (mLastSessionEstablishmentError == CHIP_ERROR_NO_MEMORY) { // 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. ScopedNodeId peerToRemove; if (mPendingNotificationMap.FindLRUConnectPeer(peerToRemove) == CHIP_NO_ERROR) { @@ -114,18 +114,15 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) // Now retry mLastSessionEstablishmentError = CHIP_NO_ERROR; - mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, &mOnConnectedCallback, &mOnConnectionFailureCallback); + // At this point connectionCallback is null since it deletes itself when the callback is called. + connectionCallback = Platform::New(*this); + mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, connectionCallback->GetOnDeviceConnected(), + connectionCallback->GetOnDeviceConnectionFailure()); } } return mLastSessionEstablishmentError; } -void BindingManager::HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) -{ - BindingManager * manager = static_cast(context); - manager->HandleDeviceConnected(exchangeMgr, sessionHandle); -} - void BindingManager::HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { FabricIndex fabricToRemove = kUndefinedFabricIndex; @@ -149,17 +146,15 @@ void BindingManager::HandleDeviceConnected(Messaging::ExchangeManager & exchange mPendingNotificationMap.RemoveAllEntriesForNode(ScopedNodeId(nodeToRemove, fabricToRemove)); } -void BindingManager::HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error) -{ - BindingManager * manager = static_cast(context); - manager->HandleDeviceConnectionFailure(peerId, error); -} - void BindingManager::HandleDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error) { // Simply release the entry, the connection will be re-established as needed. ChipLogError(AppServer, "Failed to establish connection to node 0x" ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId())); mLastSessionEstablishmentError = error; + // We don't release the entry when connection fails, because inside + // BindingManager::EstablishConnection we may try again the connection. + // TODO(#22173): The logic in there doesn't actually make any sense with how + // mPendingNotificationMap and CASESessionManager are implemented today. } void BindingManager::FabricRemoved(FabricIndex fabricIndex) @@ -188,9 +183,10 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { if (iter->type == EMBER_UNICAST_BINDING) { - mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext); + error = mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext); + SuccessOrExit(error); error = EstablishConnection(ScopedNodeId(iter->nodeId, iter->fabricIndex)); - SuccessOrExit(error == CHIP_NO_ERROR); + SuccessOrExit(error); } else if (iter->type == EMBER_MULTICAST_BINDING) { diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 578ad4af9e4b76..646281f6718e4f 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -72,9 +72,7 @@ struct BindingManagerInitParams class BindingManager { public: - BindingManager() : - mOnConnectedCallback(HandleDeviceConnected, this), mOnConnectionFailureCallback(HandleDeviceConnectionFailure, this) - {} + BindingManager() {} void RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler handler) { mBoundDeviceChangedHandler = handler; } @@ -123,13 +121,47 @@ class BindingManager static BindingManager & GetInstance() { return sBindingManager; } private: - static BindingManager sBindingManager; - - static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); - void HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); + /* + * Used when providing OnConnection/Failure callbacks to CASESessionManager when establishing session. + * + * Since the BindingManager calls EstablishConnection inside of a loop, and it is possible that the + * callback is called some time after the loop is completed, we need a separate callbacks for each + * connection we are trying to establish. Failure to provide different instances of the callback + * to CASESessionManager may result in the callback only be called for that last EstablishConnection + * that was called when it establishes the connections asynchronously. + * + */ + class ConnectionCallback + { + public: + ConnectionCallback(BindingManager & bindingManager) : + mBindingManager(bindingManager), mOnConnectedCallback(HandleDeviceConnected, this), + mOnConnectionFailureCallback(HandleDeviceConnectionFailure, this) + {} + + Callback::Callback * GetOnDeviceConnected() { return &mOnConnectedCallback; } + Callback::Callback * GetOnDeviceConnectionFailure() { return &mOnConnectionFailureCallback; } + + private: + static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) + { + ConnectionCallback * _this = static_cast(context); + _this->mBindingManager.HandleDeviceConnected(exchangeMgr, sessionHandle); + Platform::Delete(_this); + } + static void HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error) + { + ConnectionCallback * _this = static_cast(context); + _this->mBindingManager.HandleDeviceConnectionFailure(peerId, error); + Platform::Delete(_this); + } + + BindingManager & mBindingManager; + Callback::Callback mOnConnectedCallback; + Callback::Callback mOnConnectionFailureCallback; + }; - static void HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); - void HandleDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error); + static BindingManager sBindingManager; CHIP_ERROR EstablishConnection(const ScopedNodeId & nodeId); @@ -137,8 +169,8 @@ class BindingManager BoundDeviceChangedHandler mBoundDeviceChangedHandler; BindingManagerInitParams mInitParams; - Callback::Callback mOnConnectedCallback; - Callback::Callback mOnConnectionFailureCallback; + void HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); + void HandleDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error); // Used to keep track of synchronous failures from FindOrEstablishSession. CHIP_ERROR mLastSessionEstablishmentError; diff --git a/src/app/clusters/bindings/PendingNotificationMap.cpp b/src/app/clusters/bindings/PendingNotificationMap.cpp index a7e6adb595ce59..7e279b2449c7fb 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.cpp +++ b/src/app/clusters/bindings/PendingNotificationMap.cpp @@ -78,9 +78,14 @@ CHIP_ERROR PendingNotificationMap::FindLRUConnectPeer(ScopedNodeId & nodeId) return CHIP_ERROR_NOT_FOUND; } -void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context) +CHIP_ERROR PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context) { RemoveEntry(bindingEntryId); + if (mNumEntries == EMBER_BINDING_TABLE_SIZE) + { + // We know that the RemoveEntry above did not do anything so we don't need to try restoring it. + return CHIP_ERROR_NO_MEMORY; + } mPendingBindingEntries[mNumEntries] = bindingEntryId; mPendingContexts[mNumEntries] = context; if (context) @@ -88,6 +93,7 @@ void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, Pend context->IncrementConsumersNumber(); } mNumEntries++; + return CHIP_NO_ERROR; } void PendingNotificationMap::RemoveEntry(uint8_t bindingEntryId) diff --git a/src/app/clusters/bindings/PendingNotificationMap.h b/src/app/clusters/bindings/PendingNotificationMap.h index 2e70a0718d047b..99dd3e8eb73cd3 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.h +++ b/src/app/clusters/bindings/PendingNotificationMap.h @@ -102,7 +102,7 @@ class PendingNotificationMap CHIP_ERROR FindLRUConnectPeer(ScopedNodeId & nodeId); - void AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context); + CHIP_ERROR AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context); void RemoveEntry(uint8_t bindingEntryId); diff --git a/src/app/tests/TestPendingNotificationMap.cpp b/src/app/tests/TestPendingNotificationMap.cpp index 271b427f31013a..7f5f091a253252 100644 --- a/src/app/tests/TestPendingNotificationMap.cpp +++ b/src/app/tests/TestPendingNotificationMap.cpp @@ -64,8 +64,11 @@ void TestAddRemove(nlTestSuite * aSuite, void * aContext) CreateDefaultFullBindingTable(BindingTable::GetInstance()); for (uint8_t i = 0; i < EMBER_BINDING_TABLE_SIZE; i++) { - pendingMap.AddPendingNotification(i, nullptr); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(i, nullptr) == CHIP_NO_ERROR); } + // Confirm adding in one more element fails + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(EMBER_BINDING_TABLE_SIZE, nullptr) == CHIP_ERROR_NO_MEMORY); + auto iter = pendingMap.begin(); for (uint8_t i = 0; i < EMBER_BINDING_TABLE_SIZE; i++) { @@ -102,11 +105,11 @@ void TestLRUEntry(nlTestSuite * aSuite, void * aContext) PendingNotificationMap pendingMap; ClearBindingTable(BindingTable::GetInstance()); CreateDefaultFullBindingTable(BindingTable::GetInstance()); - pendingMap.AddPendingNotification(0, nullptr); - pendingMap.AddPendingNotification(1, nullptr); - pendingMap.AddPendingNotification(5, nullptr); - pendingMap.AddPendingNotification(7, nullptr); - pendingMap.AddPendingNotification(11, nullptr); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(0, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(1, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(5, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(7, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(11, nullptr) == CHIP_NO_ERROR); chip::ScopedNodeId node;