From 34245bb24b4676a71372351ddfdef8f17bc5704e Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Thu, 14 Apr 2022 12:53:18 +0200 Subject: [PATCH 1/9] Fixed BindingManager to make sure it doesn't use invalid context BindingManager uses application context passed to NotifyBoundClusterChanged and doesn't care about its life time. Applications delete the context after calling the method, so if the context will be used asynchronously (like for HandleDeviceConnect) it may have invalid value. * Added for BindingManager method that allows to register handler called when context is not used anymore and can be released. * Added releasing context in applications using context release handler. --- .../all-clusters-common/src/binding-handler.cpp | 6 ++++++ .../efr32/src/binding-handler.cpp | 10 ++++++++-- .../nrfconnect/main/BindingHandler.cpp | 16 +++++++++++++--- .../nrfconnect/main/LightSwitch.cpp | 2 -- .../nrfconnect/main/include/BindingHandler.h | 1 + src/app/clusters/bindings/BindingManager.cpp | 16 ++++++++++++++++ src/app/clusters/bindings/BindingManager.h | 13 +++++++++++++ 7 files changed, 57 insertions(+), 7 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp index 03c96e8c66be09..476ce1b75ceee8 100644 --- a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp +++ b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp @@ -103,12 +103,18 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch } } +static void BoundDeviceContextReleaseHandler(void * context) +{ + (void) context; +} + static void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(BoundDeviceContextReleaseHandler); } CHIP_ERROR InitBindingHandlers() diff --git a/examples/light-switch-app/efr32/src/binding-handler.cpp b/examples/light-switch-app/efr32/src/binding-handler.cpp index 6ce872e811ae47..5c1cf2bdd1fa71 100644 --- a/examples/light-switch-app/efr32/src/binding-handler.cpp +++ b/examples/light-switch-app/efr32/src/binding-handler.cpp @@ -132,6 +132,13 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "LightSwitchContextReleaseHandler: context is null")); + BindingCommandData * data = static_cast(context); + Platform::Delete(data); +} + #ifdef ENABLE_CHIP_SHELL /******************************************************** @@ -384,6 +391,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 @@ -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/nrfconnect/main/BindingHandler.cpp b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp index 54cce9924887fd..b1292f06e58c11 100644 --- a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp +++ b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp @@ -57,9 +57,13 @@ void BindingHandler::OnInvokeCommandFailure(DeviceProxy * aDevice, BindingData & // Set flag to not try recover session multiple times. BindingHandler::GetInstance().mCaseSessionRecovered = true; + // Allocate new object to make sure its life time will be appropriate. + BindingHandler::BindingData * data = Platform::New(); + *data = aBindingData; + // Establish new CASE session and retrasmit command that was not applied. error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId, - static_cast(&aBindingData)); + static_cast(data)); } else { @@ -222,6 +226,13 @@ void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & bi } } +void BindingHandler::LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + BindingData * data = static_cast(context); + Platform::Delete(data); +} + void BindingHandler::InitInternal(intptr_t aArg) { LOG_INF("Initialize binding Handler"); @@ -234,6 +245,7 @@ void BindingHandler::InitInternal(intptr_t aArg) } BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); BindingHandler::GetInstance().PrintBindingTable(); } @@ -298,6 +310,4 @@ void BindingHandler::SwitchWorkerHandler(intptr_t aContext) BindingData * data = reinterpret_cast(aContext); LOG_INF("Notify Bounded Cluster | endpoint: %d cluster: %d", data->EndpointId, data->ClusterId); BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, static_cast(data)); - - Platform::Delete(data); } diff --git a/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp b/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp index 7420d8f5254feb..2b54264eb45b14 100644 --- a/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp +++ b/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp @@ -57,7 +57,6 @@ void LightSwitch::InitiateActionSwitch(Action mAction) } data->IsGroup = BindingHandler::GetInstance().IsGroupBound(); DeviceLayer::PlatformMgr().ScheduleWork(BindingHandler::SwitchWorkerHandler, reinterpret_cast(data)); - Platform::Delete(data); } } @@ -79,6 +78,5 @@ void LightSwitch::DimmerChangeBrightness() data->Value = (uint8_t) sBrightness; data->IsGroup = BindingHandler::GetInstance().IsGroupBound(); DeviceLayer::PlatformMgr().ScheduleWork(BindingHandler::SwitchWorkerHandler, reinterpret_cast(data)); - Platform::Delete(data); } } diff --git a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h index fcd2d4fa8288b3..c3820e1ec33815 100644 --- a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h +++ b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h @@ -54,6 +54,7 @@ class BindingHandler static void OnOffProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LevelControlProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LightSwitchChangedHandler(const EmberBindingTableEntry &, chip::DeviceProxy *, void *); + static void LightSwitchContextReleaseHandler(void * context); static void InitInternal(intptr_t); bool mCaseSessionRecovered = false; diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index a87fe777c7533b..7a51b52944e257 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -161,6 +161,7 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device) fabricToRemove = entry.fabricIndex; nodeToRemove = entry.nodeId; mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext); + mBoundDeviceContextReleaseHandler(pendingNotification.mContext); } } mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove); @@ -176,6 +177,19 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err { // 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())); + + for (PendingNotificationEntry pendingNotification : mPendingNotificationMap) + { + EmberBindingTableEntry entry = BindingTable::GetInstance().GetAt(pendingNotification.mBindingEntryId); + + PeerId peer = PeerIdForNode(mInitParams.mFabricTable, entry.fabricIndex, entry.nodeId); + + if (peerId == peer) + { + mBoundDeviceContextReleaseHandler(pendingNotification.mContext); + } + } + mInitParams.mCASESessionManager->ReleaseSession(peerId); } @@ -204,6 +218,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { // We already have an active connection mBoundDeviceChangedHandler(*iter, peerDevice, context); + mBoundDeviceContextReleaseHandler(context); } else { @@ -214,6 +229,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste else if (iter->type == EMBER_MULTICAST_BINDING) { mBoundDeviceChangedHandler(*iter, nullptr, context); + mBoundDeviceContextReleaseHandler(context); } } } diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 88bedac4df357b..d43d5d07a7f8a7 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -40,6 +40,8 @@ namespace chip { */ using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context); +using BoundDeviceContextReleaseHandler = void (*)(void * context); + struct BindingManagerInitParams { FabricTable * mFabricTable = nullptr; @@ -70,6 +72,16 @@ class BindingManager void RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler handler) { mBoundDeviceChangedHandler = handler; } + /* + * Registers handler that will be called when context used in NotifyBoundClusterChanged will not be needed and could be + * released. + * + */ + void RegisterBoundDeviceContextReleaseHandler(BoundDeviceContextReleaseHandler handler) + { + mBoundDeviceContextReleaseHandler = handler; + } + CHIP_ERROR Init(const BindingManagerInitParams & params); /* @@ -117,6 +129,7 @@ class BindingManager PendingNotificationMap mPendingNotificationMap; BoundDeviceChangedHandler mBoundDeviceChangedHandler; + BoundDeviceContextReleaseHandler mBoundDeviceContextReleaseHandler; BindingManagerInitParams mInitParams; Callback::Callback mOnConnectedCallback; From c36b77a01e15a0b7b665ec9d056d60c6de8941cb Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Fri, 15 Apr 2022 07:32:29 +0200 Subject: [PATCH 2/9] Fixed releasing the same context multiple times. --- src/app/clusters/bindings/BindingManager.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 7a51b52944e257..6d67a26d08da11 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -19,6 +19,7 @@ #include #include #include +#include namespace { @@ -203,6 +204,10 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { VerifyOrReturnError(mInitParams.mFabricTable != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mBoundDeviceChangedHandler, CHIP_NO_ERROR); + VerifyOrReturnError(mBoundDeviceContextReleaseHandler, CHIP_NO_ERROR); + + CHIP_ERROR error = CHIP_NO_ERROR; + bool pendingNotificationAdded = false; for (auto iter = BindingTable::GetInstance().begin(); iter != BindingTable::GetInstance().end(); ++iter) { @@ -218,22 +223,28 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { // We already have an active connection mBoundDeviceChangedHandler(*iter, peerDevice, context); - mBoundDeviceContextReleaseHandler(context); } else { mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), context); - ReturnErrorOnFailure(EstablishConnection(iter->fabricIndex, iter->nodeId)); + pendingNotificationAdded = true; + error = EstablishConnection(iter->fabricIndex, iter->nodeId); + SuccessOrExit(error == CHIP_NO_ERROR); } } else if (iter->type == EMBER_MULTICAST_BINDING) { mBoundDeviceChangedHandler(*iter, nullptr, context); - mBoundDeviceContextReleaseHandler(context); } } } - return CHIP_NO_ERROR; + +exit: + // Call release context handler only if any pending notification is not going to use it. + if (!pendingNotificationAdded) + mBoundDeviceContextReleaseHandler(context); + + return error; } } // namespace chip From 3c1541f55223cf1685419b9098ac10ef4772e51e Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Wed, 20 Apr 2022 08:13:15 +0200 Subject: [PATCH 3/9] Created BindingManagerContext to count all consumers using context and release it in the proper moment. --- .../src/binding-handler.cpp | 2 +- .../efr32/src/binding-handler.cpp | 13 +++--- .../nrfconnect/main/BindingHandler.cpp | 17 ++++---- .../nrfconnect/main/include/BindingHandler.h | 2 +- src/app/clusters/bindings/BindingManager.cpp | 42 +++++++++++++------ src/app/clusters/bindings/BindingManager.h | 28 ++++++++++++- 6 files changed, 76 insertions(+), 28 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp index 476ce1b75ceee8..57bea339994cc4 100644 --- a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp +++ b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp @@ -103,7 +103,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch } } -static void BoundDeviceContextReleaseHandler(void * context) +static void BoundDeviceContextReleaseHandler(chip::BindingManagerContext * context) { (void) context; } diff --git a/examples/light-switch-app/efr32/src/binding-handler.cpp b/examples/light-switch-app/efr32/src/binding-handler.cpp index 5c1cf2bdd1fa71..85a1a4e111d1bd 100644 --- a/examples/light-switch-app/efr32/src/binding-handler.cpp +++ b/examples/light-switch-app/efr32/src/binding-handler.cpp @@ -132,11 +132,13 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro } } -void LightSwitchContextReleaseHandler(void * context) +void LightSwitchContextReleaseHandler(BindingManagerContext * context) { VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "LightSwitchContextReleaseHandler: context is null")); - BindingCommandData * data = static_cast(context); - Platform::Delete(data); + + // Release the internal context stored in BindingManagerContext and then the BindingManagerContext itself. + Platform::Delete(static_cast(context->GetContext())); + Platform::Delete(context); } #ifdef ENABLE_CHIP_SHELL @@ -404,8 +406,9 @@ void SwitchWorkerFunction(intptr_t context) { VerifyOrReturn(context != 0, ChipLogError(NotSpecified, "SwitchWorkerFunction - Invalid work data")); - BindingCommandData * data = reinterpret_cast(context); - BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast(data)); + BindingCommandData * data = reinterpret_cast(context); + BindingManagerContext * bindingContext = Platform::New(static_cast(data)); + BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, bindingContext); } void BindingWorkerFunction(intptr_t context) diff --git a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp index b1292f06e58c11..cf64022b040ae8 100644 --- a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp +++ b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp @@ -60,10 +60,10 @@ void BindingHandler::OnInvokeCommandFailure(DeviceProxy * aDevice, BindingData & // Allocate new object to make sure its life time will be appropriate. BindingHandler::BindingData * data = Platform::New(); *data = aBindingData; + BindingManagerContext * context = Platform::New(static_cast(data)); // Establish new CASE session and retrasmit command that was not applied. - error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId, - static_cast(data)); + error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId, context); } else { @@ -226,11 +226,13 @@ void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & bi } } -void BindingHandler::LightSwitchContextReleaseHandler(void * context) +void BindingHandler::LightSwitchContextReleaseHandler(BindingManagerContext * context) { VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); - BindingData * data = static_cast(context); - Platform::Delete(data); + + // Release the internal context stored in BindingManagerContext and then the BindingManagerContext itself. + Platform::Delete(static_cast(context->GetContext())); + Platform::Delete(context); } void BindingHandler::InitInternal(intptr_t aArg) @@ -307,7 +309,8 @@ void BindingHandler::SwitchWorkerHandler(intptr_t aContext) { VerifyOrReturn(aContext != 0, LOG_ERR("Invalid Swich data")); - BindingData * data = reinterpret_cast(aContext); + BindingData * data = reinterpret_cast(aContext); + BindingManagerContext * context = Platform::New(static_cast(data)); LOG_INF("Notify Bounded Cluster | endpoint: %d cluster: %d", data->EndpointId, data->ClusterId); - BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, static_cast(data)); + BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, context); } diff --git a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h index c3820e1ec33815..1c5df11b416505 100644 --- a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h +++ b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h @@ -54,7 +54,7 @@ class BindingHandler static void OnOffProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LevelControlProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LightSwitchChangedHandler(const EmberBindingTableEntry &, chip::DeviceProxy *, void *); - static void LightSwitchContextReleaseHandler(void * context); + static void LightSwitchContextReleaseHandler(chip::BindingManagerContext * context); static void InitInternal(intptr_t); bool mCaseSessionRecovered = false; diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 6d67a26d08da11..e88723925877ba 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -161,8 +161,15 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device) { fabricToRemove = entry.fabricIndex; nodeToRemove = entry.nodeId; - mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext); - mBoundDeviceContextReleaseHandler(pendingNotification.mContext); + + BindingManagerContext * context = static_cast(pendingNotification.mContext); + mBoundDeviceChangedHandler(entry, device, context->GetContext()); + + context->DecrementConsumersNumber(); + if (context->GetConsumersNumber() == 0) + { + mBoundDeviceContextReleaseHandler(context); + } } } mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove); @@ -187,7 +194,13 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err if (peerId == peer) { - mBoundDeviceContextReleaseHandler(pendingNotification.mContext); + BindingManagerContext * context = static_cast(pendingNotification.mContext); + + context->DecrementConsumersNumber(); + if (context->GetConsumersNumber() == 0) + { + mBoundDeviceContextReleaseHandler(context); + } } } @@ -200,14 +213,15 @@ void BindingManager::FabricRemoved(CompressedFabricId compressedFabricId, Fabric mInitParams.mCASESessionManager->ReleaseSessionsForFabric(compressedFabricId); } -CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, void * context) +CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, BindingManagerContext * context) { VerifyOrReturnError(mInitParams.mFabricTable != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mBoundDeviceChangedHandler, CHIP_NO_ERROR); VerifyOrReturnError(mBoundDeviceContextReleaseHandler, CHIP_NO_ERROR); - CHIP_ERROR error = CHIP_NO_ERROR; - bool pendingNotificationAdded = false; + CHIP_ERROR error = CHIP_NO_ERROR; + + context->IncrementConsumersNumber(); for (auto iter = BindingTable::GetInstance().begin(); iter != BindingTable::GetInstance().end(); ++iter) { @@ -222,27 +236,31 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste if (peerDevice != nullptr && peerDevice->IsConnected()) { // We already have an active connection - mBoundDeviceChangedHandler(*iter, peerDevice, context); + mBoundDeviceChangedHandler(*iter, peerDevice, context->GetContext()); } else { - mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), context); - pendingNotificationAdded = true; - error = EstablishConnection(iter->fabricIndex, iter->nodeId); + mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), static_cast(context)); + context->IncrementConsumersNumber(); + error = EstablishConnection(iter->fabricIndex, iter->nodeId); SuccessOrExit(error == CHIP_NO_ERROR); } } else if (iter->type == EMBER_MULTICAST_BINDING) { - mBoundDeviceChangedHandler(*iter, nullptr, context); + mBoundDeviceChangedHandler(*iter, nullptr, context->GetContext()); } } } exit: + context->DecrementConsumersNumber(); + // Call release context handler only if any pending notification is not going to use it. - if (!pendingNotificationAdded) + if (context->GetConsumersNumber() == 0) + { mBoundDeviceContextReleaseHandler(context); + } return error; } diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index d43d5d07a7f8a7..8e04184cc13e53 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -26,6 +26,26 @@ namespace chip { +class BindingManagerContext +{ +public: + BindingManagerContext(void * context) : mContext(context) {} + void * GetContext() { return mContext; }; + uint8_t GetConsumersNumber() { return mConsumersNumber; } + void IncrementConsumersNumber() { mConsumersNumber++; } + void DecrementConsumersNumber() + { + if (mConsumersNumber > 0) + { + mConsumersNumber--; + } + } + +private: + void * mContext; + uint8_t mConsumersNumber = 0; +}; + /** * Application callback function when a cluster associated with a binding changes. * @@ -40,7 +60,11 @@ namespace chip { */ using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context); -using BoundDeviceContextReleaseHandler = void (*)(void * context); +/** + * Application callback function when a context used in NotifyBoundClusterChanged will not be needed and should be + * released. + */ +using BoundDeviceContextReleaseHandler = void (*)(BindingManagerContext * context); struct BindingManagerInitParams { @@ -112,7 +136,7 @@ class BindingManager * be initiated. The BoundDeviceChangedHandler will be called once the session is established. * */ - CHIP_ERROR NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, void * context); + CHIP_ERROR NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, BindingManagerContext * context); static BindingManager & GetInstance() { return sBindingManager; } From 3fd1fcb7b202b3bdc2585b7af81bf5b284aa4cca Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Thu, 21 Apr 2022 10:38:14 +0200 Subject: [PATCH 4/9] Added removing pending notification after connection failure. --- src/app/clusters/bindings/BindingManager.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index e88723925877ba..32c0304b6d30f0 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -186,6 +186,9 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err // 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())); + FabricIndex fabricToRemove = kUndefinedFabricIndex; + NodeId nodeToRemove = kUndefinedNodeId; + for (PendingNotificationEntry pendingNotification : mPendingNotificationMap) { EmberBindingTableEntry entry = BindingTable::GetInstance().GetAt(pendingNotification.mBindingEntryId); @@ -194,6 +197,9 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err if (peerId == peer) { + fabricToRemove = entry.fabricIndex; + nodeToRemove = entry.nodeId; + BindingManagerContext * context = static_cast(pendingNotification.mContext); context->DecrementConsumersNumber(); @@ -203,6 +209,7 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err } } } + mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove); mInitParams.mCASESessionManager->ReleaseSession(peerId); } From de0c1d71f7e6df2fc23a2d0a89d5992801a86cc2 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Fri, 22 Apr 2022 14:35:14 +0200 Subject: [PATCH 5/9] Moved allocating BindingManagerContext to NotifyBoundCLusterChange --- .../src/binding-handler.cpp | 2 +- .../efr32/src/binding-handler.cpp | 11 +++---- .../nrfconnect/main/BindingHandler.cpp | 15 ++++------ .../nrfconnect/main/include/BindingHandler.h | 2 +- src/app/clusters/bindings/BindingManager.cpp | 30 +++++++++++-------- src/app/clusters/bindings/BindingManager.h | 4 +-- 6 files changed, 31 insertions(+), 33 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp index 57bea339994cc4..476ce1b75ceee8 100644 --- a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp +++ b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp @@ -103,7 +103,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch } } -static void BoundDeviceContextReleaseHandler(chip::BindingManagerContext * context) +static void BoundDeviceContextReleaseHandler(void * context) { (void) context; } diff --git a/examples/light-switch-app/efr32/src/binding-handler.cpp b/examples/light-switch-app/efr32/src/binding-handler.cpp index 85a1a4e111d1bd..6a7f5bd8b238ae 100644 --- a/examples/light-switch-app/efr32/src/binding-handler.cpp +++ b/examples/light-switch-app/efr32/src/binding-handler.cpp @@ -132,13 +132,11 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro } } -void LightSwitchContextReleaseHandler(BindingManagerContext * context) +void LightSwitchContextReleaseHandler(void * context) { VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "LightSwitchContextReleaseHandler: context is null")); - // Release the internal context stored in BindingManagerContext and then the BindingManagerContext itself. - Platform::Delete(static_cast(context->GetContext())); - Platform::Delete(context); + Platform::Delete(static_cast(context)); } #ifdef ENABLE_CHIP_SHELL @@ -406,9 +404,8 @@ void SwitchWorkerFunction(intptr_t context) { VerifyOrReturn(context != 0, ChipLogError(NotSpecified, "SwitchWorkerFunction - Invalid work data")); - BindingCommandData * data = reinterpret_cast(context); - BindingManagerContext * bindingContext = Platform::New(static_cast(data)); - BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, bindingContext); + BindingCommandData * data = reinterpret_cast(context); + BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast(data)); } void BindingWorkerFunction(intptr_t context) diff --git a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp index cf64022b040ae8..9cf98455d66ede 100644 --- a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp +++ b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp @@ -60,10 +60,10 @@ void BindingHandler::OnInvokeCommandFailure(DeviceProxy * aDevice, BindingData & // Allocate new object to make sure its life time will be appropriate. BindingHandler::BindingData * data = Platform::New(); *data = aBindingData; - BindingManagerContext * context = Platform::New(static_cast(data)); // Establish new CASE session and retrasmit command that was not applied. - error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId, context); + error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId, + static_cast(data)); } else { @@ -226,13 +226,11 @@ void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & bi } } -void BindingHandler::LightSwitchContextReleaseHandler(BindingManagerContext * context) +void BindingHandler::LightSwitchContextReleaseHandler(void * context) { VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); - // Release the internal context stored in BindingManagerContext and then the BindingManagerContext itself. - Platform::Delete(static_cast(context->GetContext())); - Platform::Delete(context); + Platform::Delete(static_cast(context)); } void BindingHandler::InitInternal(intptr_t aArg) @@ -309,8 +307,7 @@ void BindingHandler::SwitchWorkerHandler(intptr_t aContext) { VerifyOrReturn(aContext != 0, LOG_ERR("Invalid Swich data")); - BindingData * data = reinterpret_cast(aContext); - BindingManagerContext * context = Platform::New(static_cast(data)); + BindingData * data = reinterpret_cast(aContext); LOG_INF("Notify Bounded Cluster | endpoint: %d cluster: %d", data->EndpointId, data->ClusterId); - BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, context); + BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, static_cast(data)); } diff --git a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h index 1c5df11b416505..c3820e1ec33815 100644 --- a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h +++ b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h @@ -54,7 +54,7 @@ class BindingHandler static void OnOffProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LevelControlProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LightSwitchChangedHandler(const EmberBindingTableEntry &, chip::DeviceProxy *, void *); - static void LightSwitchContextReleaseHandler(chip::BindingManagerContext * context); + static void LightSwitchContextReleaseHandler(void * context); static void InitInternal(intptr_t); bool mCaseSessionRecovered = false; diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 32c0304b6d30f0..f65502ccdb1f80 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -168,7 +168,8 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device) context->DecrementConsumersNumber(); if (context->GetConsumersNumber() == 0) { - mBoundDeviceContextReleaseHandler(context); + mBoundDeviceContextReleaseHandler(context->GetContext()); + Platform::Delete(context); } } } @@ -205,7 +206,8 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err context->DecrementConsumersNumber(); if (context->GetConsumersNumber() == 0) { - mBoundDeviceContextReleaseHandler(context); + mBoundDeviceContextReleaseHandler(context->GetContext()); + Platform::Delete(context); } } } @@ -220,15 +222,16 @@ void BindingManager::FabricRemoved(CompressedFabricId compressedFabricId, Fabric mInitParams.mCASESessionManager->ReleaseSessionsForFabric(compressedFabricId); } -CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, BindingManagerContext * context) +CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, void * context) { VerifyOrReturnError(mInitParams.mFabricTable != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mBoundDeviceChangedHandler, CHIP_NO_ERROR); VerifyOrReturnError(mBoundDeviceContextReleaseHandler, CHIP_NO_ERROR); - CHIP_ERROR error = CHIP_NO_ERROR; + CHIP_ERROR error = CHIP_NO_ERROR; + BindingManagerContext * bindingContext = Platform::New(static_cast(context)); - context->IncrementConsumersNumber(); + bindingContext->IncrementConsumersNumber(); for (auto iter = BindingTable::GetInstance().begin(); iter != BindingTable::GetInstance().end(); ++iter) { @@ -243,30 +246,31 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste if (peerDevice != nullptr && peerDevice->IsConnected()) { // We already have an active connection - mBoundDeviceChangedHandler(*iter, peerDevice, context->GetContext()); + mBoundDeviceChangedHandler(*iter, peerDevice, bindingContext->GetContext()); } else { - mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), static_cast(context)); - context->IncrementConsumersNumber(); + mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), static_cast(bindingContext)); + bindingContext->IncrementConsumersNumber(); error = EstablishConnection(iter->fabricIndex, iter->nodeId); SuccessOrExit(error == CHIP_NO_ERROR); } } else if (iter->type == EMBER_MULTICAST_BINDING) { - mBoundDeviceChangedHandler(*iter, nullptr, context->GetContext()); + mBoundDeviceChangedHandler(*iter, nullptr, bindingContext->GetContext()); } } } exit: - context->DecrementConsumersNumber(); + bindingContext->DecrementConsumersNumber(); - // Call release context handler only if any pending notification is not going to use it. - if (context->GetConsumersNumber() == 0) + // Call release bindingContext handler only if any pending notification is not going to use it. + if (bindingContext->GetConsumersNumber() == 0) { - mBoundDeviceContextReleaseHandler(context); + mBoundDeviceContextReleaseHandler(bindingContext->GetContext()); + Platform::Delete(bindingContext); } return error; diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 8e04184cc13e53..fa3c2c4b1a6e4a 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -64,7 +64,7 @@ using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & bindin * Application callback function when a context used in NotifyBoundClusterChanged will not be needed and should be * released. */ -using BoundDeviceContextReleaseHandler = void (*)(BindingManagerContext * context); +using BoundDeviceContextReleaseHandler = void (*)(void * context); struct BindingManagerInitParams { @@ -136,7 +136,7 @@ class BindingManager * be initiated. The BoundDeviceChangedHandler will be called once the session is established. * */ - CHIP_ERROR NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, BindingManagerContext * context); + CHIP_ERROR NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, void * context); static BindingManager & GetInstance() { return sBindingManager; } From 9923d67a73a920e5478184b26d8865023a27da9b Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Mon, 25 Apr 2022 14:51:16 +0200 Subject: [PATCH 6/9] Removed removing pending notification on connect failure --- src/app/clusters/bindings/BindingManager.cpp | 27 -------------------- 1 file changed, 27 deletions(-) diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index f65502ccdb1f80..d00244dcd4a658 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -186,33 +186,6 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err { // 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())); - - FabricIndex fabricToRemove = kUndefinedFabricIndex; - NodeId nodeToRemove = kUndefinedNodeId; - - for (PendingNotificationEntry pendingNotification : mPendingNotificationMap) - { - EmberBindingTableEntry entry = BindingTable::GetInstance().GetAt(pendingNotification.mBindingEntryId); - - PeerId peer = PeerIdForNode(mInitParams.mFabricTable, entry.fabricIndex, entry.nodeId); - - if (peerId == peer) - { - fabricToRemove = entry.fabricIndex; - nodeToRemove = entry.nodeId; - - BindingManagerContext * context = static_cast(pendingNotification.mContext); - - context->DecrementConsumersNumber(); - if (context->GetConsumersNumber() == 0) - { - mBoundDeviceContextReleaseHandler(context->GetContext()); - Platform::Delete(context); - } - } - } - mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove); - mInitParams.mCASESessionManager->ReleaseSession(peerId); } From 146cc572289ceec34cced63ca8d20f74c8c48342 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Tue, 26 Apr 2022 12:15:47 +0200 Subject: [PATCH 7/9] Addressed review comments --- src/app/clusters/bindings/BindingManager.cpp | 27 ++------- src/app/clusters/bindings/BindingManager.h | 25 +-------- .../bindings/PendingNotificationMap.cpp | 18 +++++- .../bindings/PendingNotificationMap.h | 55 ++++++++++++++++++- 4 files changed, 76 insertions(+), 49 deletions(-) diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index d00244dcd4a658..6f8b301b90f18f 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -161,16 +161,7 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device) { fabricToRemove = entry.fabricIndex; nodeToRemove = entry.nodeId; - - BindingManagerContext * context = static_cast(pendingNotification.mContext); - mBoundDeviceChangedHandler(entry, device, context->GetContext()); - - context->DecrementConsumersNumber(); - if (context->GetConsumersNumber() == 0) - { - mBoundDeviceContextReleaseHandler(context->GetContext()); - Platform::Delete(context); - } + mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext->GetContext()); } } mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove); @@ -199,10 +190,10 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { VerifyOrReturnError(mInitParams.mFabricTable != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mBoundDeviceChangedHandler, CHIP_NO_ERROR); - VerifyOrReturnError(mBoundDeviceContextReleaseHandler, CHIP_NO_ERROR); - CHIP_ERROR error = CHIP_NO_ERROR; - BindingManagerContext * bindingContext = Platform::New(static_cast(context)); + CHIP_ERROR error = CHIP_NO_ERROR; + auto * bindingContext = mPendingNotificationMap.NewPendingNotificationContext(context); + VerifyOrReturnError(bindingContext != nullptr, CHIP_ERROR_NO_MEMORY); bindingContext->IncrementConsumersNumber(); @@ -223,8 +214,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste } else { - mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), static_cast(bindingContext)); - bindingContext->IncrementConsumersNumber(); + mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext); error = EstablishConnection(iter->fabricIndex, iter->nodeId); SuccessOrExit(error == CHIP_NO_ERROR); } @@ -239,13 +229,6 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste exit: bindingContext->DecrementConsumersNumber(); - // Call release bindingContext handler only if any pending notification is not going to use it. - if (bindingContext->GetConsumersNumber() == 0) - { - mBoundDeviceContextReleaseHandler(bindingContext->GetContext()); - Platform::Delete(bindingContext); - } - return error; } diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index fa3c2c4b1a6e4a..28fc11959e62ec 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -26,26 +26,6 @@ namespace chip { -class BindingManagerContext -{ -public: - BindingManagerContext(void * context) : mContext(context) {} - void * GetContext() { return mContext; }; - uint8_t GetConsumersNumber() { return mConsumersNumber; } - void IncrementConsumersNumber() { mConsumersNumber++; } - void DecrementConsumersNumber() - { - if (mConsumersNumber > 0) - { - mConsumersNumber--; - } - } - -private: - void * mContext; - uint8_t mConsumersNumber = 0; -}; - /** * Application callback function when a cluster associated with a binding changes. * @@ -64,7 +44,7 @@ using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & bindin * Application callback function when a context used in NotifyBoundClusterChanged will not be needed and should be * released. */ -using BoundDeviceContextReleaseHandler = void (*)(void * context); +using BoundDeviceContextReleaseHandler = PendingNotificationContextReleaseHandler; struct BindingManagerInitParams { @@ -103,7 +83,7 @@ class BindingManager */ void RegisterBoundDeviceContextReleaseHandler(BoundDeviceContextReleaseHandler handler) { - mBoundDeviceContextReleaseHandler = handler; + mPendingNotificationMap.RegisterPendingNotificationContextReleaseHandler(handler); } CHIP_ERROR Init(const BindingManagerInitParams & params); @@ -153,7 +133,6 @@ class BindingManager PendingNotificationMap mPendingNotificationMap; BoundDeviceChangedHandler mBoundDeviceChangedHandler; - BoundDeviceContextReleaseHandler mBoundDeviceContextReleaseHandler; BindingManagerInitParams mInitParams; Callback::Callback mOnConnectedCallback; diff --git a/src/app/clusters/bindings/PendingNotificationMap.cpp b/src/app/clusters/bindings/PendingNotificationMap.cpp index 7c992677587532..6676a82b779d4d 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.cpp +++ b/src/app/clusters/bindings/PendingNotificationMap.cpp @@ -79,11 +79,15 @@ CHIP_ERROR PendingNotificationMap::FindLRUConnectPeer(FabricIndex * fabric, Node return CHIP_ERROR_NOT_FOUND; } -void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, void * context) +void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context) { RemoveEntry(bindingEntryId); mPendingBindingEntries[mNumEntries] = bindingEntryId; mPendingContexts[mNumEntries] = context; + if (context) + { + context->IncrementConsumersNumber(); + } mNumEntries++; } @@ -98,6 +102,10 @@ void PendingNotificationMap::RemoveEntry(uint8_t bindingEntryId) mPendingContexts[newEntryCount] = mPendingContexts[i]; newEntryCount++; } + else if (mPendingContexts[i] != nullptr) + { + mPendingContexts[i]->DecrementConsumersNumber(); + } } mNumEntries = newEntryCount; } @@ -114,6 +122,10 @@ void PendingNotificationMap::RemoveAllEntriesForNode(FabricIndex fabric, NodeId mPendingContexts[newEntryCount] = mPendingContexts[i]; newEntryCount++; } + else if (mPendingContexts[i] != nullptr) + { + mPendingContexts[i]->DecrementConsumersNumber(); + } } mNumEntries = newEntryCount; } @@ -130,6 +142,10 @@ void PendingNotificationMap::RemoveAllEntriesForFabric(FabricIndex fabric) mPendingContexts[newEntryCount] = mPendingContexts[i]; newEntryCount++; } + else if (mPendingContexts[i] != nullptr) + { + mPendingContexts[i]->DecrementConsumersNumber(); + } } mNumEntries = newEntryCount; } diff --git a/src/app/clusters/bindings/PendingNotificationMap.h b/src/app/clusters/bindings/PendingNotificationMap.h index 2b66fa7efa97fc..fad96ea78cbb74 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.h +++ b/src/app/clusters/bindings/PendingNotificationMap.h @@ -21,11 +21,49 @@ namespace chip { +/** + * Application callback function when a context used in PendingNotificationEntry will not be needed and should be + * released. + */ +using PendingNotificationContextReleaseHandler = void (*)(void * context); + +class PendingNotificationContext +{ +public: + PendingNotificationContext(void * context, PendingNotificationContextReleaseHandler contextReleaseHandler) : + mContext(context), mPendingNotificationContextReleaseHandler(contextReleaseHandler) + {} + void * GetContext() { return mContext; }; + uint8_t GetConsumersNumber() { return mConsumersNumber; } + void IncrementConsumersNumber() { mConsumersNumber++; } + void DecrementConsumersNumber() + { + if (mConsumersNumber > 0) + { + mConsumersNumber--; + } + else + { + // Release the context only if there is no pending notification pointing to it context. + if (mPendingNotificationContextReleaseHandler != nullptr) + { + mPendingNotificationContextReleaseHandler(mContext); + } + Platform::Delete(this); + } + } + +private: + void * mContext; + uint8_t mConsumersNumber = 0; + PendingNotificationContextReleaseHandler mPendingNotificationContextReleaseHandler; +}; + struct PendingNotificationEntry { public: uint8_t mBindingEntryId; - void * mContext; + PendingNotificationContext * mContext; }; // The pool for all the pending comands. @@ -67,7 +105,7 @@ class PendingNotificationMap CHIP_ERROR FindLRUConnectPeer(FabricIndex * fabric, NodeId * node); - void AddPendingNotification(uint8_t bindingEntryId, void * context); + void AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context); void RemoveEntry(uint8_t bindingEntryId); @@ -77,9 +115,20 @@ class PendingNotificationMap void RemoveAllEntriesForFabric(FabricIndex fabric); + void RegisterPendingNotificationContextReleaseHandler(PendingNotificationContextReleaseHandler handler) + { + mPendingNotificationContextReleaseHandler = handler; + } + + PendingNotificationContext * NewPendingNotificationContext(void * context) + { + return Platform::New(context, mPendingNotificationContextReleaseHandler); + }; + private: uint8_t mPendingBindingEntries[kMaxPendingNotifications]; - void * mPendingContexts[kMaxPendingNotifications]; + PendingNotificationContext * mPendingContexts[kMaxPendingNotifications]; + PendingNotificationContextReleaseHandler mPendingNotificationContextReleaseHandler; uint8_t mNumEntries = 0; }; From b6deb4d1b22bf8c652f40fb4a48f9194efb120b0 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Tue, 26 Apr 2022 14:10:37 +0200 Subject: [PATCH 8/9] Addressed second review comments --- src/app/clusters/bindings/PendingNotificationMap.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/app/clusters/bindings/PendingNotificationMap.h b/src/app/clusters/bindings/PendingNotificationMap.h index fad96ea78cbb74..897749325e1baa 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.h +++ b/src/app/clusters/bindings/PendingNotificationMap.h @@ -38,11 +38,8 @@ class PendingNotificationContext void IncrementConsumersNumber() { mConsumersNumber++; } void DecrementConsumersNumber() { - if (mConsumersNumber > 0) - { - mConsumersNumber--; - } - else + VerifyOrDie(mConsumersNumber > 0); + if (--mConsumersNumber == 0) { // Release the context only if there is no pending notification pointing to it context. if (mPendingNotificationContextReleaseHandler != nullptr) From 23174a876e95c0eb6da636a7ff2fbfe9f1ecd4e8 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Wed, 27 Apr 2022 08:13:34 +0200 Subject: [PATCH 9/9] Addressed review comments part 3 --- src/app/clusters/bindings/PendingNotificationMap.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/clusters/bindings/PendingNotificationMap.h b/src/app/clusters/bindings/PendingNotificationMap.h index 897749325e1baa..5ef12ee560cbad 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.h +++ b/src/app/clusters/bindings/PendingNotificationMap.h @@ -34,14 +34,14 @@ class PendingNotificationContext mContext(context), mPendingNotificationContextReleaseHandler(contextReleaseHandler) {} void * GetContext() { return mContext; }; - uint8_t GetConsumersNumber() { return mConsumersNumber; } + uint32_t GetConsumersNumber() { return mConsumersNumber; } void IncrementConsumersNumber() { mConsumersNumber++; } void DecrementConsumersNumber() { VerifyOrDie(mConsumersNumber > 0); if (--mConsumersNumber == 0) { - // Release the context only if there is no pending notification pointing to it context. + // Release the context only if there is no pending notification pointing to us. if (mPendingNotificationContextReleaseHandler != nullptr) { mPendingNotificationContextReleaseHandler(mContext); @@ -52,7 +52,7 @@ class PendingNotificationContext private: void * mContext; - uint8_t mConsumersNumber = 0; + uint32_t mConsumersNumber = 0; PendingNotificationContextReleaseHandler mPendingNotificationContextReleaseHandler; };