From 346658442167a9d798717c1d9ac69230f7b7af13 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 5 Aug 2022 13:48:11 -0400 Subject: [PATCH] No longer provide OperationDeviceProxy in OnDeviceConnected callback (#21256) Previous implementations of OnDeviceConnected held onto OperationalDeviceProxy when they really should not have could lead to use after free should something else free that OperationalDeviceProxy. --- .../src/binding-handler.cpp | 4 +- .../ameba/main/BindingHandler.cpp | 6 +- .../esp32/main/include/ShellCommands.h | 2 +- .../nxp/mw320/binding-handler.cpp | 4 +- .../esp32/main/include/ShellCommands.h | 2 +- .../commands/clusters/ModelCommand.cpp | 6 +- .../commands/clusters/ModelCommand.h | 3 +- .../commands/pairing/CloseSessionCommand.cpp | 14 +-- .../commands/pairing/CloseSessionCommand.h | 7 +- .../chip-tool/commands/tests/TestCommand.cpp | 5 +- .../chip-tool/commands/tests/TestCommand.h | 7 +- .../ameba/main/BindingHandler.cpp | 6 +- .../efr32/src/binding-handler.cpp | 18 +-- .../esp32/main/BindingHandler.cpp | 18 +-- .../nrfconnect/main/BindingHandler.cpp | 21 +++- .../telink/src/binding-handler.cpp | 18 +-- examples/platform/linux/CommissionerMain.cpp | 17 +-- examples/tv-app/android/java/AppImpl.cpp | 7 +- examples/tv-app/linux/AppImpl.cpp | 7 +- .../commands/clusters/ModelCommand.cpp | 5 +- .../include/MediaCommandBase.h | 21 ++-- .../include/TargetVideoPlayerInfo.h | 24 ++-- .../tv-casting-common/src/CastingServer.cpp | 11 +- .../src/TargetVideoPlayerInfo.cpp | 4 +- .../com/google/chip/chiptool/ChipClient.kt | 4 + src/app/BUILD.gn | 6 +- src/app/CASESessionManager.cpp | 43 +++---- src/app/CASESessionManager.h | 16 +-- src/app/OperationalDeviceProxyPool.h | 93 -------------- ...eProxy.cpp => OperationalSessionSetup.cpp} | 87 +++++++------ ...eviceProxy.h => OperationalSessionSetup.h} | 116 ++++++++++++++---- src/app/OperationalSessionSetupPool.h | 96 +++++++++++++++ src/app/ReadClient.cpp | 17 +-- src/app/ReadClient.h | 4 +- src/app/app-platform/ContentAppPlatform.cpp | 15 ++- src/app/app-platform/ContentAppPlatform.h | 17 +-- src/app/clusters/bindings/BindingManager.cpp | 27 ++-- src/app/clusters/bindings/BindingManager.h | 8 +- .../ota-requestor/DefaultOTARequestor.cpp | 40 +++--- .../ota-requestor/DefaultOTARequestor.h | 11 +- src/app/server/Server.cpp | 2 +- src/app/server/Server.h | 4 +- .../certification/Test_TC_CADMIN_1_15.yaml | 4 +- .../certification/Test_TC_CADMIN_1_16.yaml | 4 +- .../certification/Test_TC_CADMIN_1_17.yaml | 4 +- .../certification/Test_TC_CADMIN_1_18.yaml | 4 +- .../suites/certification/Test_TC_DD_3_1.yaml | 4 +- .../suites/certification/Test_TC_DD_3_5.yaml | 4 +- .../suites/certification/Test_TC_DD_3_7.yaml | 4 +- src/controller/AutoCommissioner.cpp | 6 +- src/controller/AutoCommissioner.h | 2 +- src/controller/CHIPDeviceController.cpp | 32 ++--- src/controller/CHIPDeviceController.h | 12 +- .../CHIPDeviceControllerFactory.cpp | 16 +-- .../CHIPDeviceControllerSystemState.h | 18 +-- .../CommissionerDiscoveryController.cpp | 5 +- .../CommissionerDiscoveryController.h | 16 ++- src/controller/CommissioningDelegate.h | 6 +- src/controller/CommissioningWindowOpener.cpp | 15 +-- src/controller/CommissioningWindowOpener.h | 6 +- src/controller/java/AndroidCallbacks.cpp | 5 +- src/controller/java/AndroidCallbacks.h | 2 +- .../java/CHIPDeviceController-JNI.cpp | 10 ++ .../ChipDeviceController.java | 2 + .../ChipDeviceController-ScriptBinding.cpp | 18 ++- src/controller/python/chip/ChipDeviceCtrl.py | 42 ++++++- .../CHIP/MTRDeviceConnectionBridge.h | 2 +- .../CHIP/MTRDeviceConnectionBridge.mm | 5 +- src/lib/support/logging/CHIPLogging.cpp | 2 +- src/lib/support/logging/Constants.h | 2 +- src/transport/SessionManager.cpp | 16 +++ src/transport/SessionManager.h | 13 ++ 72 files changed, 653 insertions(+), 471 deletions(-) delete mode 100644 src/app/OperationalDeviceProxyPool.h rename src/app/{OperationalDeviceProxy.cpp => OperationalSessionSetup.cpp} (76%) rename src/app/{OperationalDeviceProxy.h => OperationalSessionSetup.h} (69%) create mode 100644 src/app/OperationalSessionSetupPool.h 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..8aff2a6f268bd3 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 @@ -67,7 +67,8 @@ static void RegisterSwitchCommands() } #endif // defined(ENABLE_CHIP_SHELL) -static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::DeviceProxy * peer_device, void * context) +static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::OperationalDeviceProxy * peer_device, + void * context) { using namespace chip; using namespace chip::app; @@ -88,6 +89,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format()); }; + VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady()); if (sSwitchOnOffState) { Clusters::OnOff::Commands::On::Type onCommand; diff --git a/examples/all-clusters-app/ameba/main/BindingHandler.cpp b/examples/all-clusters-app/ameba/main/BindingHandler.cpp index 93638a811bdf77..ccfb210a50a80d 100644 --- a/examples/all-clusters-app/ameba/main/BindingHandler.cpp +++ b/examples/all-clusters-app/ameba/main/BindingHandler.cpp @@ -50,7 +50,8 @@ Engine sShellSwitchBindingSubCommands; namespace { -void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device) +void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, + OperationalDeviceProxy * peer_device) { auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) { ChipLogProgress(NotSpecified, "OnOff command succeeds"); @@ -60,6 +61,7 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format()); }; + VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady()); switch (commandId) { case Clusters::OnOff::Commands::Toggle::Id: @@ -106,7 +108,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl } } -void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context) +void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context) { VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null")); BindingCommandData * data = static_cast(context); diff --git a/examples/all-clusters-app/esp32/main/include/ShellCommands.h b/examples/all-clusters-app/esp32/main/include/ShellCommands.h index 832039dd90358b..cd094d13adceb2 100644 --- a/examples/all-clusters-app/esp32/main/include/ShellCommands.h +++ b/examples/all-clusters-app/esp32/main/include/ShellCommands.h @@ -134,7 +134,7 @@ class CASECommands private: CASECommands() {} - static void OnConnected(void * context, OperationalDeviceProxy * deviceProxy) + static void OnConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { streamer_printf(streamer_get(), "Establish CASESession Success!\r\n"); GetInstance().SetOnConnecting(false); diff --git a/examples/all-clusters-app/nxp/mw320/binding-handler.cpp b/examples/all-clusters-app/nxp/mw320/binding-handler.cpp index 4367ca77d79a3a..6ad25c9d46bc34 100644 --- a/examples/all-clusters-app/nxp/mw320/binding-handler.cpp +++ b/examples/all-clusters-app/nxp/mw320/binding-handler.cpp @@ -68,7 +68,8 @@ static void RegisterSwitchCommands() } #endif // defined(ENABLE_CHIP_SHELL) -static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::DeviceProxy * peer_device, void * context) +static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::OperationalDeviceProxy * peer_device, + void * context) { using namespace chip; using namespace chip::app; @@ -92,6 +93,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch // command (SwitchCommandHandler) { Clusters::OnOff::Commands::Toggle::Type toggleCommand; + VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady()); Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, toggleCommand, onSuccess, onFailure); } diff --git a/examples/all-clusters-minimal-app/esp32/main/include/ShellCommands.h b/examples/all-clusters-minimal-app/esp32/main/include/ShellCommands.h index 832039dd90358b..cd094d13adceb2 100644 --- a/examples/all-clusters-minimal-app/esp32/main/include/ShellCommands.h +++ b/examples/all-clusters-minimal-app/esp32/main/include/ShellCommands.h @@ -134,7 +134,7 @@ class CASECommands private: CASECommands() {} - static void OnConnected(void * context, OperationalDeviceProxy * deviceProxy) + static void OnConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { streamer_printf(streamer_get(), "Establish CASESession Success!\r\n"); GetInstance().SetOnConnecting(false); diff --git a/examples/chip-tool/commands/clusters/ModelCommand.cpp b/examples/chip-tool/commands/clusters/ModelCommand.cpp index c67cebb1491fe0..c1f00738aeac1e 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.cpp +++ b/examples/chip-tool/commands/clusters/ModelCommand.cpp @@ -46,12 +46,14 @@ CHIP_ERROR ModelCommand::RunCommand() &mOnDeviceConnectionFailureCallback); } -void ModelCommand::OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device) +void ModelCommand::OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr, + chip::SessionHandle & sessionHandle) { ModelCommand * command = reinterpret_cast(context); VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "OnDeviceConnectedFn: context is null")); - CHIP_ERROR err = command->SendCommand(device, command->mEndPointId); + chip::OperationalDeviceProxy device(&exchangeMgr, sessionHandle); + CHIP_ERROR err = command->SendCommand(&device, command->mEndPointId); VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err)); } diff --git a/examples/chip-tool/commands/clusters/ModelCommand.h b/examples/chip-tool/commands/clusters/ModelCommand.h index d6581235863160..fa77620f8812e0 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.h +++ b/examples/chip-tool/commands/clusters/ModelCommand.h @@ -69,7 +69,8 @@ class ModelCommand : public CHIPCommand chip::NodeId mDestinationId; std::vector mEndPointId; - static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device); + static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr, + chip::SessionHandle & sessionHandle); static void OnDeviceConnectionFailureFn(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error); chip::Callback::Callback mOnDeviceConnectedCallback; diff --git a/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp b/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp index 210164770a6ff7..6509c84922a270 100644 --- a/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp +++ b/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp @@ -28,17 +28,16 @@ CHIP_ERROR CloseSessionCommand::RunCommand() CommissioneeDeviceProxy * commissioneeDeviceProxy = nullptr; if (CHIP_NO_ERROR == CurrentCommissioner().GetDeviceBeingCommissioned(mDestinationId, &commissioneeDeviceProxy)) { - return CloseSession(commissioneeDeviceProxy); + VerifyOrReturnError(commissioneeDeviceProxy->GetSecureSession().HasValue(), CHIP_ERROR_INCORRECT_STATE); + return CloseSession(*commissioneeDeviceProxy->GetExchangeManager(), commissioneeDeviceProxy->GetSecureSession().Value()); } return CurrentCommissioner().GetConnectedDevice(mDestinationId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback); } -CHIP_ERROR CloseSessionCommand::CloseSession(DeviceProxy * device) +CHIP_ERROR CloseSessionCommand::CloseSession(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle) { - VerifyOrReturnError(device->GetSecureSession().HasValue(), CHIP_ERROR_INCORRECT_STATE); - // TODO perhaps factor out this code into something on StatusReport that // takes an exchange and maybe a SendMessageFlags? SecureChannel::StatusReport statusReport(SecureChannel::GeneralStatusCode::kSuccess, SecureChannel::Id, @@ -51,7 +50,7 @@ CHIP_ERROR CloseSessionCommand::CloseSession(DeviceProxy * device) System::PacketBufferHandle msg = bbuf.Finalize(); VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_NO_MEMORY); - auto * exchange = device->GetExchangeManager()->NewContext(device->GetSecureSession().Value(), nullptr); + auto * exchange = exchangeMgr.NewContext(sessionHandle, nullptr); VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_NO_MEMORY); // Per spec, CloseSession reports are always sent with MRP disabled. @@ -69,12 +68,13 @@ CHIP_ERROR CloseSessionCommand::CloseSession(DeviceProxy * device) return err; } -void CloseSessionCommand::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device) +void CloseSessionCommand::OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) { auto * command = reinterpret_cast(context); VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "OnDeviceConnectedFn: context is null")); - CHIP_ERROR err = command->CloseSession(device); + CHIP_ERROR err = command->CloseSession(exchangeMgr, sessionHandle); VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err)); } diff --git a/examples/chip-tool/commands/pairing/CloseSessionCommand.h b/examples/chip-tool/commands/pairing/CloseSessionCommand.h index 270c45210f4ebe..350c5890783540 100644 --- a/examples/chip-tool/commands/pairing/CloseSessionCommand.h +++ b/examples/chip-tool/commands/pairing/CloseSessionCommand.h @@ -19,7 +19,7 @@ #pragma once #include "../common/CHIPCommand.h" -#include +#include #include #include @@ -46,11 +46,12 @@ class CloseSessionCommand : public CHIPCommand chip::NodeId mDestinationId; chip::Optional mTimeoutSecs; - static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device); + static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr, + chip::SessionHandle & sessionHandle); static void OnDeviceConnectionFailureFn(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error); // Try to send the action CloseSession status report. - CHIP_ERROR CloseSession(chip::DeviceProxy * device); + CHIP_ERROR CloseSession(chip::Messaging::ExchangeManager & exchangeMgr, const chip::SessionHandle & sessionHandle); chip::Callback::Callback mOnDeviceConnectedCallback; chip::Callback::Callback mOnDeviceConnectionFailureCallback; diff --git a/examples/chip-tool/commands/tests/TestCommand.cpp b/examples/chip-tool/commands/tests/TestCommand.cpp index 6de48bd810ac79..22f216c8bdb72f 100644 --- a/examples/chip-tool/commands/tests/TestCommand.cpp +++ b/examples/chip-tool/commands/tests/TestCommand.cpp @@ -51,12 +51,13 @@ CHIP_ERROR TestCommand::WaitForCommissionee(const char * identity, &mOnDeviceConnectionFailureCallback); } -void TestCommand::OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device) +void TestCommand::OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr, + chip::SessionHandle & sessionHandle) { ChipLogProgress(chipTool, " **** Test Setup: Device Connected\n"); auto * command = static_cast(context); VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "Device connected, but cannot run the test, as the context is null")); - command->mDevices[command->GetIdentity()] = device; + command->mDevices[command->GetIdentity()] = std::make_unique(&exchangeMgr, sessionHandle); LogErrorOnFailure(command->ContinueOnChipMainThread(CHIP_NO_ERROR)); } diff --git a/examples/chip-tool/commands/tests/TestCommand.h b/examples/chip-tool/commands/tests/TestCommand.h index e35b64df24b7a3..9a58cc8dac6d92 100644 --- a/examples/chip-tool/commands/tests/TestCommand.h +++ b/examples/chip-tool/commands/tests/TestCommand.h @@ -67,10 +67,11 @@ class TestCommand : public TestRunner, void OnWaitForMs() override { NextTest(); }; /////////// Interaction Model Interface ///////// - chip::DeviceProxy * GetDevice(const char * identity) override { return mDevices[identity]; } + chip::DeviceProxy * GetDevice(const char * identity) override { return mDevices[identity].get(); } void OnResponse(const chip::app::StatusIB & status, chip::TLV::TLVReader * data) override{}; - static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device); + static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr, + chip::SessionHandle & sessionHandle); static void OnDeviceConnectionFailureFn(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error); CHIP_ERROR ContinueOnChipMainThread(CHIP_ERROR err) override; @@ -94,7 +95,7 @@ class TestCommand : public TestRunner, chip::Optional mPICSFilePath; chip::Optional mTimeout; - std::map mDevices; + std::map> mDevices; // When set to false, prevents interaction model events from affecting the current test status. // This flag exists because if an error happens while processing a response the allocated diff --git a/examples/light-switch-app/ameba/main/BindingHandler.cpp b/examples/light-switch-app/ameba/main/BindingHandler.cpp index 93638a811bdf77..ccfb210a50a80d 100644 --- a/examples/light-switch-app/ameba/main/BindingHandler.cpp +++ b/examples/light-switch-app/ameba/main/BindingHandler.cpp @@ -50,7 +50,8 @@ Engine sShellSwitchBindingSubCommands; namespace { -void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device) +void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, + OperationalDeviceProxy * peer_device) { auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) { ChipLogProgress(NotSpecified, "OnOff command succeeds"); @@ -60,6 +61,7 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format()); }; + VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady()); switch (commandId) { case Clusters::OnOff::Commands::Toggle::Id: @@ -106,7 +108,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl } } -void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context) +void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context) { VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null")); BindingCommandData * data = static_cast(context); diff --git a/examples/light-switch-app/efr32/src/binding-handler.cpp b/examples/light-switch-app/efr32/src/binding-handler.cpp index 6a7f5bd8b238ae..90a2463a088e33 100644 --- a/examples/light-switch-app/efr32/src/binding-handler.cpp +++ b/examples/light-switch-app/efr32/src/binding-handler.cpp @@ -51,7 +51,8 @@ Engine sShellSwitchBindingSubCommands; namespace { -void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device) +void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, + Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle) { auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) { ChipLogProgress(NotSpecified, "OnOff command succeeds"); @@ -65,20 +66,17 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa { case Clusters::OnOff::Commands::Toggle::Id: Clusters::OnOff::Commands::Toggle::Type toggleCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - toggleCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, toggleCommand, onSuccess, onFailure); break; case Clusters::OnOff::Commands::On::Id: Clusters::OnOff::Commands::On::Type onCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - onCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, onCommand, onSuccess, onFailure); break; case Clusters::OnOff::Commands::Off::Id: Clusters::OnOff::Commands::Off::Type offCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - offCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, offCommand, onSuccess, onFailure); break; } } @@ -107,7 +105,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl } } -void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context) +void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context) { VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null")); BindingCommandData * data = static_cast(context); @@ -126,7 +124,9 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro switch (data->clusterId) { case Clusters::OnOff::Id: - ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device); + VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady()); + ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(), + peer_device->GetSecureSession().Value()); break; } } diff --git a/examples/light-switch-app/esp32/main/BindingHandler.cpp b/examples/light-switch-app/esp32/main/BindingHandler.cpp index 93638a811bdf77..179c8e4a7c7a41 100644 --- a/examples/light-switch-app/esp32/main/BindingHandler.cpp +++ b/examples/light-switch-app/esp32/main/BindingHandler.cpp @@ -50,7 +50,8 @@ Engine sShellSwitchBindingSubCommands; namespace { -void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device) +void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, + Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle) { auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) { ChipLogProgress(NotSpecified, "OnOff command succeeds"); @@ -64,20 +65,17 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa { case Clusters::OnOff::Commands::Toggle::Id: Clusters::OnOff::Commands::Toggle::Type toggleCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - toggleCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, toggleCommand, onSuccess, onFailure); break; case Clusters::OnOff::Commands::On::Id: Clusters::OnOff::Commands::On::Type onCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - onCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, onCommand, onSuccess, onFailure); break; case Clusters::OnOff::Commands::Off::Id: Clusters::OnOff::Commands::Off::Type offCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - offCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, offCommand, onSuccess, onFailure); break; } } @@ -106,7 +104,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl } } -void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context) +void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context) { VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null")); BindingCommandData * data = static_cast(context); @@ -125,7 +123,9 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro switch (data->clusterId) { case Clusters::OnOff::Id: - ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device); + VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady()); + ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(), + peer_device->GetSecureSession().Value()); break; } } diff --git a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp index 2d36217681dda0..7b810e936ad37a 100644 --- a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp +++ b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp @@ -71,8 +71,8 @@ void BindingHandler::OnInvokeCommandFailure(DeviceProxy * aDevice, BindingData & } } -void BindingHandler::OnOffProcessCommand(CommandId aCommandId, const EmberBindingTableEntry & aBinding, DeviceProxy * aDevice, - void * aContext) +void BindingHandler::OnOffProcessCommand(CommandId aCommandId, const EmberBindingTableEntry & aBinding, + OperationalDeviceProxy * aDevice, void * aContext) { CHIP_ERROR ret = CHIP_NO_ERROR; BindingData * data = reinterpret_cast(aContext); @@ -89,6 +89,12 @@ void BindingHandler::OnOffProcessCommand(CommandId aCommandId, const EmberBindin BindingHandler::OnInvokeCommandFailure(aDevice, dataRef, aError); }; + if (aDevice) + { + // We are validating connection is ready once here instead of multiple times in each case statement below. + VerifyOrDie(aDevice->ConnectionReady()); + } + switch (aCommandId) { case Clusters::OnOff::Commands::Toggle::Id: @@ -144,7 +150,7 @@ void BindingHandler::OnOffProcessCommand(CommandId aCommandId, const EmberBindin } void BindingHandler::LevelControlProcessCommand(CommandId aCommandId, const EmberBindingTableEntry & aBinding, - DeviceProxy * aDevice, void * aContext) + OperationalDeviceProxy * aDevice, void * aContext) { BindingData * data = reinterpret_cast(aContext); @@ -162,6 +168,12 @@ void BindingHandler::LevelControlProcessCommand(CommandId aCommandId, const Embe CHIP_ERROR ret = CHIP_NO_ERROR; + if (aDevice) + { + // We are validating connection is ready once here instead of multiple times in each case statement below. + VerifyOrDie(aDevice->ConnectionReady()); + } + switch (aCommandId) { case Clusters::LevelControl::Commands::MoveToLevel::Id: { @@ -189,7 +201,8 @@ void BindingHandler::LevelControlProcessCommand(CommandId aCommandId, const Embe } } -void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * deviceProxy, void * context) +void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * deviceProxy, + void * context) { VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch handler");); BindingData * data = static_cast(context); diff --git a/examples/light-switch-app/telink/src/binding-handler.cpp b/examples/light-switch-app/telink/src/binding-handler.cpp index e8fcc448daaf5d..9483448c70b34b 100755 --- a/examples/light-switch-app/telink/src/binding-handler.cpp +++ b/examples/light-switch-app/telink/src/binding-handler.cpp @@ -51,7 +51,8 @@ Engine sShellSwitchBindingSubCommands; namespace { -void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device) +void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, + Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle) { auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) { ChipLogProgress(NotSpecified, "OnOff command succeeds"); @@ -65,20 +66,17 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa { case Clusters::OnOff::Commands::Toggle::Id: Clusters::OnOff::Commands::Toggle::Type toggleCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - toggleCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, toggleCommand, onSuccess, onFailure); break; case Clusters::OnOff::Commands::On::Id: Clusters::OnOff::Commands::On::Type onCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - onCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, onCommand, onSuccess, onFailure); break; case Clusters::OnOff::Commands::Off::Id: Clusters::OnOff::Commands::Off::Type offCommand; - Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, - offCommand, onSuccess, onFailure); + Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, offCommand, onSuccess, onFailure); break; } } @@ -107,7 +105,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl } } -void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context) +void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context) { VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null")); BindingCommandData * data = static_cast(context); @@ -127,7 +125,9 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro switch (data->clusterId) { case Clusters::OnOff::Id: - ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device); + VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady()); + ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(), + peer_device->GetSecureSession().Value()); break; } } diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index d70ead1203cfdf..b1b096b3203422 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -233,7 +233,8 @@ class PairingCommand : public Controller::DevicePairingDelegate private: #if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED - static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device); + static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr, + chip::SessionHandle & sessionHandle); static void OnDeviceConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); chip::Callback::Callback mOnDeviceConnectedCallback; @@ -308,28 +309,18 @@ void PairingCommand::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err) #if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED -void PairingCommand::OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device) +void PairingCommand::OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { ChipLogProgress(Controller, "OnDeviceConnectedFn"); CommissionerDiscoveryController * cdc = GetCommissionerDiscoveryController(); - if (device == nullptr) - { - ChipLogProgress(AppServer, "No OperationalDeviceProxy returned from OnDeviceConnectedFn"); - if (cdc != nullptr) - { - cdc->CommissioningFailed(CHIP_ERROR_INCORRECT_STATE); - } - return; - } - if (cdc != nullptr) { uint16_t vendorId = gAutoCommissioner.GetCommissioningParameters().GetRemoteVendorId().Value(); uint16_t productId = gAutoCommissioner.GetCommissioningParameters().GetRemoteProductId().Value(); ChipLogProgress(Support, " ----- AutoCommissioner -- Commissionee vendorId=0x%04X productId=0x%04X", vendorId, productId); - cdc->CommissioningSucceeded(vendorId, productId, gRemoteId, device); + cdc->CommissioningSucceeded(vendorId, productId, gRemoteId, exchangeMgr, sessionHandle); } } diff --git a/examples/tv-app/android/java/AppImpl.cpp b/examples/tv-app/android/java/AppImpl.cpp index 27de2183fb2f61..de3d48505dec67 100644 --- a/examples/tv-app/android/java/AppImpl.cpp +++ b/examples/tv-app/android/java/AppImpl.cpp @@ -60,11 +60,12 @@ MyPincodeService gMyPincodeService; class MyPostCommissioningListener : public PostCommissioningListener { - void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, OperationalDeviceProxy * device) override + void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) override { - ContentAppPlatform::GetInstance().ManageClientAccess(device, vendorId, GetDeviceCommissioner()->GetNodeId(), - OnSuccessResponse, OnFailureResponse); + ContentAppPlatform::GetInstance().ManageClientAccess( + exchangeMgr, sessionHandle, vendorId, GetDeviceCommissioner()->GetNodeId(), OnSuccessResponse, OnFailureResponse); } /* Callback when command results in success */ diff --git a/examples/tv-app/linux/AppImpl.cpp b/examples/tv-app/linux/AppImpl.cpp index 101b4d03821883..ddfa2dff130bfe 100644 --- a/examples/tv-app/linux/AppImpl.cpp +++ b/examples/tv-app/linux/AppImpl.cpp @@ -86,11 +86,12 @@ MyPincodeService gMyPincodeService; class MyPostCommissioningListener : public PostCommissioningListener { - void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, OperationalDeviceProxy * device) override + void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) override { - ContentAppPlatform::GetInstance().ManageClientAccess(device, vendorId, GetDeviceCommissioner()->GetNodeId(), - OnSuccessResponse, OnFailureResponse); + ContentAppPlatform::GetInstance().ManageClientAccess( + exchangeMgr, sessionHandle, vendorId, GetDeviceCommissioner()->GetNodeId(), OnSuccessResponse, OnFailureResponse); } /* Callback when command results in success */ diff --git a/examples/tv-casting-app/tv-casting-common/commands/clusters/ModelCommand.cpp b/examples/tv-casting-app/tv-casting-common/commands/clusters/ModelCommand.cpp index 5982c67f8e41ae..da55ed2888995d 100644 --- a/examples/tv-casting-app/tv-casting-common/commands/clusters/ModelCommand.cpp +++ b/examples/tv-casting-app/tv-casting-common/commands/clusters/ModelCommand.cpp @@ -53,13 +53,14 @@ CHIP_ERROR ModelCommand::RunCommand() return CHIP_NO_ERROR; } -void ModelCommand::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device) +void ModelCommand::OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { ChipLogProgress(chipTool, "ModelCommand::OnDeviceConnectedFn"); ModelCommand * command = reinterpret_cast(context); VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "OnDeviceConnectedFn: context is null")); - CHIP_ERROR err = command->SendCommand(device, command->mEndPointId); + OperationalDeviceProxy device(&exchangeMgr, sessionHandle); + CHIP_ERROR err = command->SendCommand(&device, command->mEndPointId); VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err)); } diff --git a/examples/tv-casting-app/tv-casting-common/include/MediaCommandBase.h b/examples/tv-casting-app/tv-casting-common/include/MediaCommandBase.h index 01c301c8bbaeb0..a125f18d139f92 100644 --- a/examples/tv-casting-app/tv-casting-common/include/MediaCommandBase.h +++ b/examples/tv-casting-app/tv-casting-common/include/MediaCommandBase.h @@ -28,22 +28,25 @@ class MediaCommandBase public: MediaCommandBase(chip::ClusterId clusterId) { mClusterId = clusterId; } - CHIP_ERROR SetTarget(TargetVideoPlayerInfo & mTargetVideoPlayerInfo, chip::EndpointId tvEndpoint) + CHIP_ERROR SetTarget(TargetVideoPlayerInfo & targetVideoPlayerInfo, chip::EndpointId tvEndpoint) { - mOperationalDeviceProxy = mTargetVideoPlayerInfo.GetOperationalDeviceProxy(); - if (mOperationalDeviceProxy == nullptr) + auto deviceProxy = targetVideoPlayerInfo.GetOperationalDeviceProxy(); + if (deviceProxy == nullptr) { ChipLogError(AppServer, "Failed in getting an instance of OperationalDeviceProxy"); return CHIP_ERROR_PEER_NODE_NOT_FOUND; } - - mTvEndpoint = tvEndpoint; + mTargetVideoPlayerInfo = &targetVideoPlayerInfo; + mTvEndpoint = tvEndpoint; return CHIP_NO_ERROR; } CHIP_ERROR Invoke(RequestType request, std::function responseCallback) { - VerifyOrDieWithMsg(mOperationalDeviceProxy != nullptr, AppServer, "Target unknown"); + VerifyOrDieWithMsg(mTargetVideoPlayerInfo != nullptr, AppServer, "Target unknown"); + + auto deviceProxy = mTargetVideoPlayerInfo->GetOperationalDeviceProxy(); + ReturnErrorCodeIf(deviceProxy == nullptr || !deviceProxy->ConnectionReady(), CHIP_ERROR_PEER_NODE_NOT_FOUND); sResponseCallback = responseCallback; @@ -56,8 +59,8 @@ class MediaCommandBase {} }; - MediaClusterBase cluster(*mOperationalDeviceProxy->GetExchangeManager(), - mOperationalDeviceProxy->GetSecureSession().Value(), mClusterId, mTvEndpoint); + MediaClusterBase cluster(*deviceProxy->GetExchangeManager(), deviceProxy->GetSecureSession().Value(), mClusterId, + mTvEndpoint); return cluster.InvokeCommand(request, nullptr, OnSuccess, OnFailure); } @@ -67,7 +70,7 @@ class MediaCommandBase protected: chip::ClusterId mClusterId; - chip::OperationalDeviceProxy * mOperationalDeviceProxy = nullptr; + TargetVideoPlayerInfo * mTargetVideoPlayerInfo = nullptr; chip::EndpointId mTvEndpoint; static std::function sResponseCallback; }; diff --git a/examples/tv-casting-app/tv-casting-common/include/TargetVideoPlayerInfo.h b/examples/tv-casting-app/tv-casting-common/include/TargetVideoPlayerInfo.h index 1e2265d20dd8b3..9a703dbfa830d5 100644 --- a/examples/tv-casting-app/tv-casting-common/include/TargetVideoPlayerInfo.h +++ b/examples/tv-casting-app/tv-casting-common/include/TargetVideoPlayerInfo.h @@ -32,7 +32,14 @@ class TargetVideoPlayerInfo bool IsInitialized() { return mInitialized; } chip::NodeId GetNodeId() const { return mNodeId; } chip::FabricIndex GetFabricIndex() const { return mFabricIndex; } - chip::OperationalDeviceProxy * GetOperationalDeviceProxy() const { return mOperationalDeviceProxy; } + const chip::OperationalDeviceProxy * GetOperationalDeviceProxy() const + { + if (mDeviceProxy.ConnectionReady()) + { + return &mDeviceProxy; + } + return nullptr; + } CHIP_ERROR Initialize(chip::NodeId nodeId, chip::FabricIndex fabricIndex); TargetEndpointInfo * GetOrAddEndpoint(chip::EndpointId endpointId); @@ -41,25 +48,26 @@ class TargetVideoPlayerInfo void PrintInfo(); private: - static void HandleDeviceConnected(void * context, chip::OperationalDeviceProxy * device) + static void HandleDeviceConnected(void * context, chip::Messaging::ExchangeManager & exchangeMgr, + chip::SessionHandle & sessionHandle) { - TargetVideoPlayerInfo * _this = static_cast(context); - _this->mOperationalDeviceProxy = device; - _this->mInitialized = true; + TargetVideoPlayerInfo * _this = static_cast(context); + _this->mDeviceProxy = chip::OperationalDeviceProxy(&exchangeMgr, sessionHandle); + _this->mInitialized = true; ChipLogProgress(AppServer, "HandleDeviceConnected created an instance of OperationalDeviceProxy"); } static void HandleDeviceConnectionFailure(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error) { - TargetVideoPlayerInfo * _this = static_cast(context); - _this->mOperationalDeviceProxy = nullptr; + TargetVideoPlayerInfo * _this = static_cast(context); + _this->mDeviceProxy = chip::OperationalDeviceProxy(); } static constexpr size_t kMaxNumberOfEndpoints = 5; TargetEndpointInfo mEndpoints[kMaxNumberOfEndpoints]; chip::NodeId mNodeId; chip::FabricIndex mFabricIndex; - chip::OperationalDeviceProxy * mOperationalDeviceProxy; + chip::OperationalDeviceProxy mDeviceProxy; chip::Callback::Callback mOnConnectedCallback; chip::Callback::Callback mOnConnectionFailureCallback; diff --git a/examples/tv-casting-app/tv-casting-common/src/CastingServer.cpp b/examples/tv-casting-app/tv-casting-common/src/CastingServer.cpp index 548f45cc1c5ff8..67c5f2f9132bc5 100644 --- a/examples/tv-casting-app/tv-casting-common/src/CastingServer.cpp +++ b/examples/tv-casting-app/tv-casting-common/src/CastingServer.cpp @@ -134,15 +134,16 @@ void CastingServer::ReadServerClustersForNode(NodeId nodeId) void CastingServer::ReadServerClusters(EndpointId endpointId) { - OperationalDeviceProxy * operationalDeviceProxy = mTargetVideoPlayerInfo.GetOperationalDeviceProxy(); - if (operationalDeviceProxy == nullptr) + const OperationalDeviceProxy * deviceProxy = mTargetVideoPlayerInfo.GetOperationalDeviceProxy(); + if (deviceProxy == nullptr) { - ChipLogError(AppServer, "Failed in getting an instance of OperationalDeviceProxy"); + ChipLogError(AppServer, "Failed in getting an instance of DeviceProxy"); return; } - chip::Controller::DescriptorCluster cluster(*operationalDeviceProxy->GetExchangeManager(), - operationalDeviceProxy->GetSecureSession().Value(), endpointId); + // GetOperationalDeviceProxy only passes us a deviceProxy if we can get a SessionHandle. + chip::Controller::DescriptorCluster cluster(*deviceProxy->GetExchangeManager(), deviceProxy->GetSecureSession().Value(), + endpointId); TargetEndpointInfo * endpointInfo = mTargetVideoPlayerInfo.GetOrAddEndpoint(endpointId); diff --git a/examples/tv-casting-app/tv-casting-common/src/TargetVideoPlayerInfo.cpp b/examples/tv-casting-app/tv-casting-common/src/TargetVideoPlayerInfo.cpp index 546c68681b9fb3..b4559757225c28 100644 --- a/examples/tv-casting-app/tv-casting-common/src/TargetVideoPlayerInfo.cpp +++ b/examples/tv-casting-app/tv-casting-common/src/TargetVideoPlayerInfo.cpp @@ -36,12 +36,12 @@ CHIP_ERROR TargetVideoPlayerInfo::Initialize(NodeId nodeId, FabricIndex fabricIn server->GetCASESessionManager()->FindOrEstablishSession(ScopedNodeId(nodeId, fabricIndex), &mOnConnectedCallback, &mOnConnectionFailureCallback); - if (mOperationalDeviceProxy == nullptr) + if (!mDeviceProxy.ConnectionReady()) { ChipLogError(AppServer, "Failed to find an existing instance of OperationalDeviceProxy to the peer"); return CHIP_ERROR_INVALID_ARGUMENT; } - ChipLogProgress(AppServer, "Created an instance of OperationalDeviceProxy"); + ChipLogProgress(AppServer, "Created an instance of DeviceProxy"); mInitialized = true; return CHIP_NO_ERROR; diff --git a/src/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/ChipClient.kt b/src/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/ChipClient.kt index 53d62c1b4f39e3..10d7c32e994a9e 100644 --- a/src/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/ChipClient.kt +++ b/src/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/ChipClient.kt @@ -61,6 +61,10 @@ object ChipClient { * Wrapper around [ChipDeviceController.getConnectedDevicePointer] to return the value directly. */ suspend fun getConnectedDevicePointer(context: Context, nodeId: Long): Long { + // TODO (#21539) This is a memory leak because we currently never call releaseConnectedDevicePointer + // once we are done with the returned device pointer. Memory leak was introduced since the refactor + // that introduced it was very large in order to fix a use after free, which was considered to be + // worse than the memory leak that was introduced. return suspendCoroutine { continuation -> getDeviceController(context).getConnectedDevicePointer( nodeId, diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 9f82625bf8a7eb..9dd72dfc4206e3 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -165,9 +165,9 @@ static_library("app") { "MessageDef/WriteRequestMessage.cpp", "MessageDef/WriteResponseMessage.cpp", "OTAUserConsentCommon.h", - "OperationalDeviceProxy.cpp", - "OperationalDeviceProxy.h", - "OperationalDeviceProxyPool.h", + "OperationalSessionSetup.cpp", + "OperationalSessionSetup.h", + "OperationalSessionSetupPool.h", "ReadClient.cpp", "ReadHandler.cpp", "RequiredPrivilege.cpp", diff --git a/src/app/CASESessionManager.cpp b/src/app/CASESessionManager.cpp index 0f1d08fa46aeca..7ffa85d18ab00b 100644 --- a/src/app/CASESessionManager.cpp +++ b/src/app/CASESessionManager.cpp @@ -34,12 +34,12 @@ void CASESessionManager::FindOrEstablishSession(const ScopedNodeId & peerId, Cal ChipLogDetail(CASESessionManager, "FindOrEstablishSession: PeerId = [%d:" ChipLogFormatX64 "]", peerId.GetFabricIndex(), ChipLogValueX64(peerId.GetNodeId())); - OperationalDeviceProxy * session = FindExistingSession(peerId); + OperationalSessionSetup * session = FindExistingSessionSetup(peerId); if (session == nullptr) { - ChipLogDetail(CASESessionManager, "FindOrEstablishSession: No existing OperationalDeviceProxy instance found"); + ChipLogDetail(CASESessionManager, "FindOrEstablishSession: No existing OperationalSessionSetup instance found"); - session = mConfig.devicePool->Allocate(mConfig.sessionInitParams, peerId); + session = mConfig.sessionSetupPool->Allocate(mConfig.sessionInitParams, peerId, this); if (session == nullptr) { @@ -52,51 +52,48 @@ void CASESessionManager::FindOrEstablishSession(const ScopedNodeId & peerId, Cal } session->Connect(onConnection, onFailure); - if (!session->IsConnected() && !session->IsConnecting() && !session->IsResolvingAddress()) - { - // This session is not making progress toward anything. It will have - // notified the consumer about the failure already via the provided - // callbacks, if any. - // - // Release the peer rather than the pointer in case the failure handler - // has already released the session. - ReleaseSession(peerId); - } } void CASESessionManager::ReleaseSession(const ScopedNodeId & peerId) { - ReleaseSession(FindExistingSession(peerId)); + ReleaseSession(FindExistingSessionSetup(peerId)); } void CASESessionManager::ReleaseSessionsForFabric(FabricIndex fabricIndex) { - mConfig.devicePool->ReleaseDevicesForFabric(fabricIndex); + mConfig.sessionSetupPool->ReleaseAllSessionSetupsForFabric(fabricIndex); } void CASESessionManager::ReleaseAllSessions() { - mConfig.devicePool->ReleaseAllDevices(); + mConfig.sessionSetupPool->ReleaseAllSessionSetup(); } CHIP_ERROR CASESessionManager::GetPeerAddress(const ScopedNodeId & peerId, Transport::PeerAddress & addr) { - OperationalDeviceProxy * session = FindExistingSession(peerId); - VerifyOrReturnError(session != nullptr, CHIP_ERROR_NOT_CONNECTED); - addr = session->GetPeerAddress(); + ReturnErrorOnFailure(mConfig.sessionInitParams.Validate()); + auto optionalSessionHandle = FindExistingSession(peerId); + ReturnErrorCodeIf(!optionalSessionHandle.HasValue(), CHIP_ERROR_NOT_CONNECTED); + addr = optionalSessionHandle.Value()->AsSecureSession()->GetPeerAddress(); return CHIP_NO_ERROR; } -OperationalDeviceProxy * CASESessionManager::FindExistingSession(const ScopedNodeId & peerId) const +OperationalSessionSetup * CASESessionManager::FindExistingSessionSetup(const ScopedNodeId & peerId) const +{ + return mConfig.sessionSetupPool->FindSessionSetup(peerId); +} + +Optional CASESessionManager::FindExistingSession(const ScopedNodeId & peerId) const { - return mConfig.devicePool->FindDevice(peerId); + return mConfig.sessionInitParams.sessionManager->FindSecureSessionForNode(peerId, + MakeOptional(Transport::SecureSession::Type::kCASE)); } -void CASESessionManager::ReleaseSession(OperationalDeviceProxy * session) const +void CASESessionManager::ReleaseSession(OperationalSessionSetup * session) const { if (session != nullptr) { - mConfig.devicePool->Release(session); + mConfig.sessionSetupPool->Release(session); } } diff --git a/src/app/CASESessionManager.h b/src/app/CASESessionManager.h index c74cb47748f874..ddc68b3b833e25 100644 --- a/src/app/CASESessionManager.h +++ b/src/app/CASESessionManager.h @@ -19,8 +19,8 @@ #pragma once #include -#include -#include +#include +#include #include #include #include @@ -34,7 +34,7 @@ namespace chip { struct CASESessionManagerConfig { DeviceProxyInitParams sessionInitParams; - OperationalDeviceProxyPoolDelegate * devicePool = nullptr; + OperationalSessionSetupPoolDelegate * sessionSetupPool = nullptr; }; /** @@ -45,7 +45,7 @@ struct CASESessionManagerConfig * 4. During session establishment, trigger node ID resolution (if needed), and update the DNS-SD cache (if resolution is * successful) */ -class CASESessionManager +class CASESessionManager : public OperationalSessionReleaseDelegate { public: CASESessionManager() = default; @@ -71,9 +71,9 @@ class CASESessionManager void FindOrEstablishSession(const ScopedNodeId & peerId, Callback::Callback * onConnection, Callback::Callback * onFailure); - OperationalDeviceProxy * FindExistingSession(const ScopedNodeId & peerId) const; + OperationalSessionSetup * FindExistingSessionSetup(const ScopedNodeId & peerId) const; - void ReleaseSession(const ScopedNodeId & peerId); + void ReleaseSession(const ScopedNodeId & peerId) override; void ReleaseSessionsForFabric(FabricIndex fabricIndex); @@ -90,7 +90,9 @@ class CASESessionManager CHIP_ERROR GetPeerAddress(const ScopedNodeId & peerId, Transport::PeerAddress & addr); private: - void ReleaseSession(OperationalDeviceProxy * device) const; + Optional FindExistingSession(const ScopedNodeId & peerId) const; + + void ReleaseSession(OperationalSessionSetup * device) const; CASESessionManagerConfig mConfig; }; diff --git a/src/app/OperationalDeviceProxyPool.h b/src/app/OperationalDeviceProxyPool.h deleted file mode 100644 index 5413624c4d9016..00000000000000 --- a/src/app/OperationalDeviceProxyPool.h +++ /dev/null @@ -1,93 +0,0 @@ -/* - * - * Copyright (c) 2021 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#pragma once - -#include -#include -#include - -namespace chip { - -class OperationalDeviceProxyPoolDelegate -{ -public: - virtual OperationalDeviceProxy * Allocate(DeviceProxyInitParams & params, ScopedNodeId peerId) = 0; - - virtual void Release(OperationalDeviceProxy * device) = 0; - - virtual OperationalDeviceProxy * FindDevice(ScopedNodeId peerId) = 0; - - virtual void ReleaseDevicesForFabric(FabricIndex fabricIndex) = 0; - - virtual void ReleaseAllDevices() = 0; - - virtual ~OperationalDeviceProxyPoolDelegate() {} -}; - -template -class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate -{ -public: - ~OperationalDeviceProxyPool() override { mDevicePool.ReleaseAll(); } - - OperationalDeviceProxy * Allocate(DeviceProxyInitParams & params, ScopedNodeId peerId) override - { - return mDevicePool.CreateObject(params, peerId); - } - - void Release(OperationalDeviceProxy * device) override { mDevicePool.ReleaseObject(device); } - - OperationalDeviceProxy * FindDevice(ScopedNodeId peerId) override - { - OperationalDeviceProxy * foundDevice = nullptr; - mDevicePool.ForEachActiveObject([&](auto * activeDevice) { - if (activeDevice->GetPeerId() == peerId) - { - foundDevice = activeDevice; - return Loop::Break; - } - return Loop::Continue; - }); - - return foundDevice; - } - - void ReleaseDevicesForFabric(FabricIndex fabricIndex) override - { - mDevicePool.ForEachActiveObject([&](auto * activeDevice) { - if (activeDevice->GetFabricIndex() == fabricIndex) - { - Release(activeDevice); - } - return Loop::Continue; - }); - } - - void ReleaseAllDevices() override - { - mDevicePool.ForEachActiveObject([&](auto * activeDevice) { - Release(activeDevice); - return Loop::Continue; - }); - } - -private: - ObjectPool mDevicePool; -}; - -}; // namespace chip diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalSessionSetup.cpp similarity index 76% rename from src/app/OperationalDeviceProxy.cpp rename to src/app/OperationalSessionSetup.cpp index 45386ca6626995..ab58a58f8adaa7 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalSessionSetup.cpp @@ -24,7 +24,7 @@ * messages to and from the corresponding CHIP devices. */ -#include +#include #include #include @@ -45,11 +45,11 @@ using chip::AddressResolve::ResolveResult; namespace chip { -void OperationalDeviceProxy::MoveToState(State aTargetState) +void OperationalSessionSetup::MoveToState(State aTargetState) { if (mState != aTargetState) { - ChipLogDetail(Controller, "OperationalDeviceProxy[%u:" ChipLogFormatX64 "]: State change %d --> %d", + ChipLogDetail(Controller, "OperationalSessionSetup[%u:" ChipLogFormatX64 "]: State change %d --> %d", mPeerId.GetFabricIndex(), ChipLogValueX64(mPeerId.GetNodeId()), to_underlying(mState), to_underlying(aTargetState)); mState = aTargetState; @@ -61,7 +61,7 @@ void OperationalDeviceProxy::MoveToState(State aTargetState) } } -bool OperationalDeviceProxy::AttachToExistingSecureSession() +bool OperationalSessionSetup::AttachToExistingSecureSession() { VerifyOrReturnError(mState == State::NeedsAddress || mState == State::ResolvingAddress || mState == State::HasAddress, false); @@ -80,8 +80,8 @@ bool OperationalDeviceProxy::AttachToExistingSecureSession() return true; } -void OperationalDeviceProxy::Connect(Callback::Callback * onConnection, - Callback::Callback * onFailure) +void OperationalSessionSetup::Connect(Callback::Callback * onConnection, + Callback::Callback * onFailure) { CHIP_ERROR err = CHIP_NO_ERROR; bool isConnected = false; @@ -154,10 +154,14 @@ void OperationalDeviceProxy::Connect(Callback::Callback * onC if (err != CHIP_NO_ERROR || isConnected) { DequeueConnectionCallbacks(err); + // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. + // While it is odd to have an explicit return here at the end of the function, we do so + // as a precaution in case someone later on adds something to the end of this function. + return; } } -void OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress & addr, const ReliableMessageProtocolConfig & config) +void OperationalSessionSetup::UpdateDeviceData(const Transport::PeerAddress & addr, const ReliableMessageProtocolConfig & config) { if (mState == State::Uninitialized) { @@ -168,7 +172,7 @@ void OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress & add char peerAddrBuff[Transport::PeerAddress::kMaxToStringSize]; addr.ToString(peerAddrBuff); - ChipLogDetail(Discovery, "OperationalDeviceProxy[%u:" ChipLogFormatX64 "]: Updating device address to %s while in state %d", + ChipLogDetail(Discovery, "OperationalSessionSetup[%u:" ChipLogFormatX64 "]: Updating device address to %s while in state %d", mPeerId.GetFabricIndex(), ChipLogValueX64(mPeerId.GetNodeId()), peerAddrBuff, static_cast(mState)); #endif @@ -191,6 +195,8 @@ void OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress & add if (err != CHIP_NO_ERROR) { DequeueConnectionCallbacks(err); + // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. + return; } } else @@ -208,7 +214,7 @@ void OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress & add } } -CHIP_ERROR OperationalDeviceProxy::EstablishConnection() +CHIP_ERROR OperationalSessionSetup::EstablishConnection() { mCASEClient = mInitParams.clientPool->Allocate(CASEClientInitParams{ mInitParams.sessionManager, mInitParams.sessionResumptionStorage, mInitParams.certificateValidityPolicy, @@ -227,8 +233,8 @@ CHIP_ERROR OperationalDeviceProxy::EstablishConnection() return CHIP_NO_ERROR; } -void OperationalDeviceProxy::EnqueueConnectionCallbacks(Callback::Callback * onConnection, - Callback::Callback * onFailure) +void OperationalSessionSetup::EnqueueConnectionCallbacks(Callback::Callback * onConnection, + Callback::Callback * onFailure) { if (onConnection != nullptr) { @@ -241,7 +247,7 @@ void OperationalDeviceProxy::EnqueueConnectionCallbacks(Callback::CallbackmCall(cb->mContext, mPeerId, error); + cb->mCall(cb->mContext, peerId, error); } } @@ -277,12 +291,16 @@ void OperationalDeviceProxy::DequeueConnectionCallbacks(CHIP_ERROR error) cb->Cancel(); if (error == CHIP_NO_ERROR) { - cb->mCall(cb->mContext, this); + // We know that we for sure have the SessionHandle in the successful case. + VerifyOrDie(exchangeMgr); + cb->mCall(cb->mContext, *exchangeMgr, sessionHandle.Value()); } } + + releaseDelegate->ReleaseSession(peerId); } -void OperationalDeviceProxy::OnSessionEstablishmentError(CHIP_ERROR error) +void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error) { VerifyOrReturn(mState != State::Uninitialized && mState != State::NeedsAddress, ChipLogError(Controller, "HandleCASEConnectionFailure was called while the device was not initialized")); @@ -295,11 +313,10 @@ void OperationalDeviceProxy::OnSessionEstablishmentError(CHIP_ERROR error) MoveToState(State::HasAddress); DequeueConnectionCallbacks(error); - - // Do not touch device instance anymore; it might have been destroyed by a failure callback. + // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } -void OperationalDeviceProxy::OnSessionEstablished(const SessionHandle & session) +void OperationalSessionSetup::OnSessionEstablished(const SessionHandle & session) { VerifyOrReturn(mState != State::Uninitialized, ChipLogError(Controller, "HandleCASEConnected was called while the device was not initialized")); @@ -308,12 +325,12 @@ void OperationalDeviceProxy::OnSessionEstablished(const SessionHandle & session) return; // Got an invalid session, do not change any state MoveToState(State::SecureConnected); - DequeueConnectionCallbacks(CHIP_NO_ERROR); - // Do not touch this instance anymore; it might have been destroyed by a callback. + DequeueConnectionCallbacks(CHIP_NO_ERROR); + // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } -void OperationalDeviceProxy::Disconnect() +void OperationalSessionSetup::Disconnect() { VerifyOrReturn(mState == State::SecureConnected); @@ -333,7 +350,7 @@ void OperationalDeviceProxy::Disconnect() MoveToState(State::HasAddress); } -void OperationalDeviceProxy::CleanupCASEClient() +void OperationalSessionSetup::CleanupCASEClient() { if (mCASEClient) { @@ -342,32 +359,27 @@ void OperationalDeviceProxy::CleanupCASEClient() } } -void OperationalDeviceProxy::OnSessionReleased() +void OperationalSessionSetup::OnSessionReleased() { MoveToState(State::HasAddress); } -void OperationalDeviceProxy::OnFirstMessageDeliveryFailed() +void OperationalSessionSetup::OnFirstMessageDeliveryFailed() { LookupPeerAddress(); } -void OperationalDeviceProxy::OnSessionHang() +void OperationalSessionSetup::OnSessionHang() { Disconnect(); } -void OperationalDeviceProxy::ShutdownSubscriptions() -{ - app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mPeerId.GetFabricIndex(), GetDeviceId()); -} - -OperationalDeviceProxy::~OperationalDeviceProxy() +OperationalSessionSetup::~OperationalSessionSetup() { if (mAddressLookupHandle.IsActive()) { ChipLogDetail(Discovery, - "OperationalDeviceProxy[%u:" ChipLogFormatX64 + "OperationalSessionSetup[%u:" ChipLogFormatX64 "]: Cancelling incomplete address resolution as device is being deleted.", mPeerId.GetFabricIndex(), ChipLogValueX64(mPeerId.GetNodeId())); @@ -387,7 +399,7 @@ OperationalDeviceProxy::~OperationalDeviceProxy() } } -CHIP_ERROR OperationalDeviceProxy::LookupPeerAddress() +CHIP_ERROR OperationalSessionSetup::LookupPeerAddress() { // NOTE: This is public API that can be used to update our stored peer // address even when we are in State::Connected, so we do not make any @@ -395,7 +407,7 @@ CHIP_ERROR OperationalDeviceProxy::LookupPeerAddress() if (mAddressLookupHandle.IsActive()) { ChipLogProgress(Discovery, - "OperationalDeviceProxy[%u:" ChipLogFormatX64 + "OperationalSessionSetup[%u:" ChipLogFormatX64 "]: Operational node lookup already in progress. Will NOT start a new one.", mPeerId.GetFabricIndex(), ChipLogValueX64(mPeerId.GetNodeId())); return CHIP_NO_ERROR; @@ -411,14 +423,14 @@ CHIP_ERROR OperationalDeviceProxy::LookupPeerAddress() return Resolver::Instance().LookupNode(request, mAddressLookupHandle); } -void OperationalDeviceProxy::OnNodeAddressResolved(const PeerId & peerId, const ResolveResult & result) +void OperationalSessionSetup::OnNodeAddressResolved(const PeerId & peerId, const ResolveResult & result) { UpdateDeviceData(result.address, result.mrpRemoteConfig); } -void OperationalDeviceProxy::OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP_ERROR reason) +void OperationalSessionSetup::OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP_ERROR reason) { - ChipLogError(Discovery, "OperationalDeviceProxy[%u:" ChipLogFormatX64 "]: operational discovery failed: %" CHIP_ERROR_FORMAT, + ChipLogError(Discovery, "OperationalSessionSetup[%u:" ChipLogFormatX64 "]: operational discovery failed: %" CHIP_ERROR_FORMAT, mPeerId.GetFabricIndex(), ChipLogValueX64(mPeerId.GetNodeId()), reason.Format()); if (IsResolvingAddress()) @@ -427,6 +439,7 @@ void OperationalDeviceProxy::OnNodeAddressResolutionFailed(const PeerId & peerId } DequeueConnectionCallbacks(reason); + // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } } // namespace chip diff --git a/src/app/OperationalDeviceProxy.h b/src/app/OperationalSessionSetup.h similarity index 69% rename from src/app/OperationalDeviceProxy.h rename to src/app/OperationalSessionSetup.h index f83bb7a2ab9613..a821d040e18bc1 100644 --- a/src/app/OperationalDeviceProxy.h +++ b/src/app/OperationalSessionSetup.h @@ -70,9 +70,79 @@ struct DeviceProxyInitParams } }; -class OperationalDeviceProxy; +/** + * @brief Delegate provided when creating OperationalSessionSetup. + * + * Once OperationalSessionSetup establishes a connection (or errors out) and has notified all + * registered application callbacks via OnDeviceConnected/OnDeviceConnectionFailure, this delegate + * is used to deallocate the OperationalSessionSetup. + */ +class OperationalSessionReleaseDelegate +{ +public: + virtual ~OperationalSessionReleaseDelegate() = default; + // TODO Issue #20452: Once cleanup from #20452 takes place we can provide OperationalSessionSetup * + // instead of ScopedNodeId here. + virtual void ReleaseSession(const ScopedNodeId & peerId) = 0; +}; + +/** + * @brief Minimal implementation of DeviceProxy that encapsulates a SessionHolder to track a CASE session. + * + * Deprecated - Avoid using this object. + * + * OperationalDeviceProxy is a minimal implementation of DeviceProxy. It is meant to provide a transition + * for existing consumers of OperationalDeviceProxy that were delivered a reference to that object in + * their respective OnDeviceConnected callback, but were incorrectly holding onto that object past + * the function call. OperationalDeviceProxy can be held on for as long as is desired, while still + * minimizing the code changes needed to transition to a more final solution by virtue of + * implementing DeviceProxy. + */ +class OperationalDeviceProxy : public DeviceProxy +{ +public: + OperationalDeviceProxy(Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle) : + mExchangeMgr(exchangeMgr), mSecureSession(sessionHandle), mPeerScopedNodeId(sessionHandle->GetPeer()) + {} + OperationalDeviceProxy() {} + + // Recommended to use InteractionModelEngine::ShutdownSubscriptions directly. + void ShutdownSubscriptions() override { VerifyOrDie(false); } // Currently not implemented. + void Disconnect() override + { + if (IsSecureConnected()) + { + GetSecureSession().Value()->AsSecureSession()->MarkAsDefunct(); + } + mSecureSession.Release(); + mExchangeMgr = nullptr; + mPeerScopedNodeId = ScopedNodeId(); + } + Messaging::ExchangeManager * GetExchangeManager() const override { return mExchangeMgr; } + chip::Optional GetSecureSession() const override { return mSecureSession.Get(); } + NodeId GetDeviceId() const override { return mPeerScopedNodeId.GetNodeId(); } + ScopedNodeId GetPeerScopedNodeId() const { return mPeerScopedNodeId; } + + bool ConnectionReady() const { return (mExchangeMgr != nullptr && IsSecureConnected()); } -typedef void (*OnDeviceConnected)(void * context, OperationalDeviceProxy * device); +private: + bool IsSecureConnected() const override { return static_cast(mSecureSession); } + + Messaging::ExchangeManager * mExchangeMgr = nullptr; + SessionHolder mSecureSession; + ScopedNodeId mPeerScopedNodeId; +}; + +/** + * @brief Callback prototype when secure session is established. + * + * Callback implementations are not supposed to store the exchangeMgr or the sessionHandle. Older + * application code does incorrectly hold onto this information so do not follow those incorrect + * implementations as an example. + */ +// TODO: OnDeviceConnected should not return ExchangeManager. Application should have this already. This +// was provided initially to keep code churn down during a large refactor of OnDeviceConnected. +typedef void (*OnDeviceConnected)(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); typedef void (*OnDeviceConnectionFailure)(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); /** @@ -84,27 +154,29 @@ typedef void (*OnDeviceConnectionFailure)(void * context, const ScopedNodeId & p * - Establish a secure channel to it via CASE * - Expose to consumers the secure session for talking to the device. */ -class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, - public SessionDelegate, - public SessionEstablishmentDelegate, - public AddressResolve::NodeListener +class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, + public SessionEstablishmentDelegate, + public AddressResolve::NodeListener { public: - ~OperationalDeviceProxy() override; + ~OperationalSessionSetup() override; - OperationalDeviceProxy(DeviceProxyInitParams & params, ScopedNodeId peerId) : mSecureSession(*this) + OperationalSessionSetup(DeviceProxyInitParams & params, ScopedNodeId peerId, + OperationalSessionReleaseDelegate * releaseDelegate) : + mSecureSession(*this) { mInitParams = params; - if (params.Validate() != CHIP_NO_ERROR) + if (params.Validate() != CHIP_NO_ERROR || releaseDelegate == nullptr) { mState = State::Uninitialized; return; } - mSystemLayer = params.exchangeMgr->GetSessionManager()->SystemLayer(); - mPeerId = peerId; - mFabricTable = params.fabricTable; - mState = State::NeedsAddress; + mSystemLayer = params.exchangeMgr->GetSessionManager()->SystemLayer(); + mPeerId = peerId; + mFabricTable = params.fabricTable; + mReleaseDelegate = releaseDelegate; + mState = State::NeedsAddress; mAddressLookupHandle.SetListener(this); } @@ -156,18 +228,10 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, /** * Mark any open session with the device as expired. */ - void Disconnect() override; - - NodeId GetDeviceId() const override { return mPeerId.GetNodeId(); } + void Disconnect(); ScopedNodeId GetPeerId() const { return mPeerId; } - void ShutdownSubscriptions() override; - - Messaging::ExchangeManager * GetExchangeManager() const override { return mInitParams.exchangeMgr; } - - chip::Optional GetSecureSession() const override { return mSecureSession.Get(); } - Transport::PeerAddress GetPeerAddress() const { return mDeviceAddress; } static Transport::PeerAddress ToPeerAddress(const Dnssd::ResolvedNodeData & nodeData) @@ -204,7 +268,7 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, private: enum class State { - Uninitialized, // Error state: OperationalDeviceProxy is useless + Uninitialized, // Error state: OperationalSessionSetup is useless NeedsAddress, // No address known, lookup not started yet. ResolvingAddress, // Address lookup in progress. HasAddress, // Have an address, CASE handshake not started yet. @@ -233,9 +297,13 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, Callback::CallbackDeque mConnectionSuccess; Callback::CallbackDeque mConnectionFailure; + OperationalSessionReleaseDelegate * mReleaseDelegate; + /// This is used when a node address is required. chip::AddressResolve::NodeLookupHandle mAddressLookupHandle; + ReliableMessageProtocolConfig mRemoteMRPConfig = GetDefaultMRPConfig(); + CHIP_ERROR EstablishConnection(); /* @@ -247,8 +315,6 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, */ bool AttachToExistingSecureSession(); - bool IsSecureConnected() const override { return mState == State::SecureConnected; } - void CleanupCASEClient(); void EnqueueConnectionCallbacks(Callback::Callback * onConnection, diff --git a/src/app/OperationalSessionSetupPool.h b/src/app/OperationalSessionSetupPool.h new file mode 100644 index 00000000000000..bd80276ae21b03 --- /dev/null +++ b/src/app/OperationalSessionSetupPool.h @@ -0,0 +1,96 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include +#include +#include + +namespace chip { + +class OperationalSessionSetupPoolDelegate +{ +public: + virtual OperationalSessionSetup * Allocate(DeviceProxyInitParams & params, ScopedNodeId peerId, + OperationalSessionReleaseDelegate * releaseDelegate) = 0; + + virtual void Release(OperationalSessionSetup * device) = 0; + + virtual OperationalSessionSetup * FindSessionSetup(ScopedNodeId peerId) = 0; + + virtual void ReleaseAllSessionSetupsForFabric(FabricIndex fabricIndex) = 0; + + virtual void ReleaseAllSessionSetup() = 0; + + virtual ~OperationalSessionSetupPoolDelegate() {} +}; + +template +class OperationalSessionSetupPool : public OperationalSessionSetupPoolDelegate +{ +public: + ~OperationalSessionSetupPool() override { mSessionSetupPool.ReleaseAll(); } + + OperationalSessionSetup * Allocate(DeviceProxyInitParams & params, ScopedNodeId peerId, + OperationalSessionReleaseDelegate * releaseDelegate) override + { + return mSessionSetupPool.CreateObject(params, peerId, releaseDelegate); + } + + void Release(OperationalSessionSetup * device) override { mSessionSetupPool.ReleaseObject(device); } + + OperationalSessionSetup * FindSessionSetup(ScopedNodeId peerId) override + { + OperationalSessionSetup * foundDevice = nullptr; + mSessionSetupPool.ForEachActiveObject([&](auto * activeSetup) { + if (activeSetup->GetPeerId() == peerId) + { + foundDevice = activeSetup; + return Loop::Break; + } + return Loop::Continue; + }); + + return foundDevice; + } + + void ReleaseAllSessionSetupsForFabric(FabricIndex fabricIndex) override + { + mSessionSetupPool.ForEachActiveObject([&](auto * activeSetup) { + if (activeSetup->GetFabricIndex() == fabricIndex) + { + Release(activeSetup); + } + return Loop::Continue; + }); + } + + void ReleaseAllSessionSetup() override + { + mSessionSetupPool.ForEachActiveObject([&](auto * activeSetup) { + Release(activeSetup); + return Loop::Continue; + }); + } + +private: + ObjectPool mSessionSetupPool; +}; + +}; // namespace chip diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index be5b7359d38d1a..90d4bf77594108 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -991,13 +991,13 @@ CHIP_ERROR ReadClient::DefaultResubscribePolicy(CHIP_ERROR aTerminationCause) return CHIP_NO_ERROR; } -void ReadClient::HandleDeviceConnected(void * context, OperationalDeviceProxy * device) +void ReadClient::HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { ReadClient * const _this = static_cast(context); VerifyOrDie(_this != nullptr); - ChipLogProgress(DataManagement, "HandleDeviceConnected %d\n", device->GetSecureSession().HasValue()); - _this->mReadPrepareParams.mSessionHolder.Grab(device->GetSecureSession().Value()); + ChipLogProgress(DataManagement, "HandleDeviceConnected"); + _this->mReadPrepareParams.mSessionHolder.Grab(sessionHandle); auto err = _this->SendSubscribeRequest(_this->mReadPrepareParams); if (err != CHIP_NO_ERROR) @@ -1044,17 +1044,6 @@ void ReadClient::OnResubscribeTimerCallback(System::Layer * apSystemLayer, void _this->mReadPrepareParams.mSessionHolder->AsSecureSession()->MarkAsDefunct(); } - // - // TODO: Until #19259 is merged, we cannot actually just get by with the above logic since marking sessions - // defunct has no effect on resident OperationalDeviceProxy instances that are already bound - // to a now-defunct CASE session. - // - auto proxy = caseSessionManager->FindExistingSession(_this->mPeer); - if (proxy != nullptr) - { - proxy->Disconnect(); - } - caseSessionManager->FindOrEstablishSession(_this->mPeer, &_this->mOnConnectedCallback, &_this->mOnConnectionFailureCallback); return; diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index f5c516243b4d16..dba2e7b7af4924 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -33,7 +33,7 @@ #include #include #include -#include +#include #include #include #include @@ -488,7 +488,7 @@ class ReadClient : public Messaging::ExchangeDelegate void StopResubscription(); void ClearActiveSubscriptionState(); - static void HandleDeviceConnected(void * context, OperationalDeviceProxy * device); + static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); static void HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); CHIP_ERROR GetMinEventNumber(const ReadPrepareParams & aReadPrepareParams, Optional & aEventMin); diff --git a/src/app/app-platform/ContentAppPlatform.cpp b/src/app/app-platform/ContentAppPlatform.cpp index 5557c092dcb8b9..de8e53faabcb3b 100644 --- a/src/app/app-platform/ContentAppPlatform.cpp +++ b/src/app/app-platform/ContentAppPlatform.cpp @@ -406,11 +406,11 @@ constexpr ClusterId kClusterIdAudioOutput = 0x050b; // constexpr ClusterId kClusterIdApplicationLauncher = 0x050c; // constexpr ClusterId kClusterIdAccountLogin = 0x050e; -CHIP_ERROR ContentAppPlatform::ManageClientAccess(OperationalDeviceProxy * targetDeviceProxy, uint16_t targetVendorId, - NodeId localNodeId, Controller::WriteResponseSuccessCallback successCb, +CHIP_ERROR ContentAppPlatform::ManageClientAccess(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle, + uint16_t targetVendorId, NodeId localNodeId, + Controller::WriteResponseSuccessCallback successCb, Controller::WriteResponseFailureCallback failureCb) { - VerifyOrReturnError(targetDeviceProxy != nullptr, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(successCb != nullptr, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(failureCb != nullptr, CHIP_ERROR_INVALID_ARGUMENT); @@ -419,9 +419,9 @@ CHIP_ERROR ContentAppPlatform::ManageClientAccess(OperationalDeviceProxy * targe Access::AccessControl::Entry entry; ReturnErrorOnFailure(GetAccessControl().PrepareEntry(entry)); ReturnErrorOnFailure(entry.SetAuthMode(Access::AuthMode::kCase)); - entry.SetFabricIndex(targetDeviceProxy->GetFabricIndex()); + entry.SetFabricIndex(sessionHandle->GetFabricIndex()); ReturnErrorOnFailure(entry.SetPrivilege(vendorPrivilege)); - ReturnErrorOnFailure(entry.AddSubject(nullptr, targetDeviceProxy->GetDeviceId())); + ReturnErrorOnFailure(entry.AddSubject(nullptr, sessionHandle->GetPeer().GetNodeId())); std::vector bindings; @@ -522,13 +522,12 @@ CHIP_ERROR ContentAppPlatform::ManageClientAccess(OperationalDeviceProxy * targe } // TODO: add a subject description on the ACL - ReturnErrorOnFailure(GetAccessControl().CreateEntry(nullptr, targetDeviceProxy->GetFabricIndex(), nullptr, entry)); + ReturnErrorOnFailure(GetAccessControl().CreateEntry(nullptr, sessionHandle->GetFabricIndex(), nullptr, entry)); ChipLogProgress(Controller, "Attempting to update Binding list"); BindingListType bindingList(bindings.data(), bindings.size()); - chip::Controller::BindingCluster cluster(*targetDeviceProxy->GetExchangeManager(), - targetDeviceProxy->GetSecureSession().Value(), kTargetBindingClusterEndpointId); + chip::Controller::BindingCluster cluster(exchangeMgr, sessionHandle, kTargetBindingClusterEndpointId); ReturnErrorOnFailure( cluster.WriteAttribute(bindingList, nullptr, successCb, failureCb)); diff --git a/src/app/app-platform/ContentAppPlatform.h b/src/app/app-platform/ContentAppPlatform.h index 4246914e223990..cac2441d46382e 100644 --- a/src/app/app-platform/ContentAppPlatform.h +++ b/src/app/app-platform/ContentAppPlatform.h @@ -23,7 +23,7 @@ #pragma once #include -#include +#include #include #include #include @@ -130,16 +130,17 @@ class DLL_EXPORT ContentAppPlatform * Add ACLs on this device for the given client, * and create bindings on the given client so that it knows what it has access to. * - * @param[in] targetDeviceProxy OperationalDeviceProxy for the target device. - * @param[in] targetVendorId Vendor ID for the target device. - * @param[in] localNodeId The NodeId for the local device. - * @param[in] successCb The function to be called on success of adding the binding. - * @param[in] failureCb The function to be called on failure of adding the binding. + * @param[in] exchangeMgr Exchange manager to be used to get an exchange context. + * @param[in] sessionHandle Reference to an established session. + * @param[in] targetVendorId Vendor ID for the target device. + * @param[in] localNodeId The NodeId for the local device. + * @param[in] successCb The function to be called on success of adding the binding. + * @param[in] failureCb The function to be called on failure of adding the binding. * * @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error */ - CHIP_ERROR ManageClientAccess(OperationalDeviceProxy * targetDeviceProxy, uint16_t targetVendorId, NodeId localNodeId, - Controller::WriteResponseSuccessCallback successCb, + CHIP_ERROR ManageClientAccess(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle, uint16_t targetVendorId, + NodeId localNodeId, Controller::WriteResponseSuccessCallback successCb, Controller::WriteResponseFailureCallback failureCb); protected: diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 73bcf93e0b9aff..d373e8c15a3f13 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -121,13 +121,13 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) return mLastSessionEstablishmentError; } -void BindingManager::HandleDeviceConnected(void * context, OperationalDeviceProxy * device) +void BindingManager::HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { BindingManager * manager = static_cast(context); - manager->HandleDeviceConnected(device); + manager->HandleDeviceConnected(exchangeMgr, sessionHandle); } -void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device) +void BindingManager::HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { FabricIndex fabricToRemove = kUndefinedFabricIndex; NodeId nodeToRemove = kUndefinedNodeId; @@ -138,11 +138,12 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device) { EmberBindingTableEntry entry = BindingTable::GetInstance().GetAt(pendingNotification.mBindingEntryId); - if (device->GetPeerId() == ScopedNodeId(entry.nodeId, entry.fabricIndex)) + if (sessionHandle->GetPeer() == ScopedNodeId(entry.nodeId, entry.fabricIndex)) { fabricToRemove = entry.fabricIndex; nodeToRemove = entry.nodeId; - mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext->GetContext()); + OperationalDeviceProxy device(&exchangeMgr, sessionHandle); + mBoundDeviceChangedHandler(entry, &device, pendingNotification.mContext->GetContext()); } } @@ -189,19 +190,9 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { if (iter->type == EMBER_UNICAST_BINDING) { - OperationalDeviceProxy * peerDevice = - mInitParams.mCASESessionManager->FindExistingSession(ScopedNodeId(iter->nodeId, iter->fabricIndex)); - if (peerDevice != nullptr && peerDevice->IsConnected()) - { - // We already have an active connection - mBoundDeviceChangedHandler(*iter, peerDevice, bindingContext->GetContext()); - } - else - { - mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext); - error = EstablishConnection(ScopedNodeId(iter->nodeId, iter->fabricIndex)); - SuccessOrExit(error == CHIP_NO_ERROR); - } + mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext); + error = EstablishConnection(ScopedNodeId(iter->nodeId, iter->fabricIndex)); + SuccessOrExit(error == CHIP_NO_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 308b672a482590..578ad4af9e4b76 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -37,8 +37,10 @@ namespace chip { * * E.g. The application will send on/off commands to peer for the OnOff cluster. * + * The handler is not allowed to hold onto the pointer to the SessionHandler that is passed in. */ -using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context); +using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, + void * context); /** * Application callback function when a context used in NotifyBoundClusterChanged will not be needed and should be @@ -123,8 +125,8 @@ class BindingManager private: static BindingManager sBindingManager; - static void HandleDeviceConnected(void * context, OperationalDeviceProxy * device); - void HandleDeviceConnected(OperationalDeviceProxy * device); + static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); + void HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); static void HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); void HandleDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error); diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp b/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp index 1aecdb3022ae27..98f11b3fee0811 100644 --- a/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp +++ b/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp @@ -387,8 +387,7 @@ void DefaultOTARequestor::DisconnectFromProvider() } auto providerNodeId = GetProviderScopedId(); - mCASESessionManager->FindExistingSession(providerNodeId)->Disconnect(); - mCASESessionManager->ReleaseSession(providerNodeId); + mServer->GetSecureSessionManager().MarkSessionsAsDefunct(providerNodeId, MakeOptional(Transport::SecureSession::Type::kCASE)); } // Requestor is directed to cancel image update in progress. All the Requestor state is @@ -416,16 +415,15 @@ CHIP_ERROR DefaultOTARequestor::GetUpdateStateAttribute(EndpointId endpointId, O } // Called whenever FindOrEstablishSession is successful -void DefaultOTARequestor::OnConnected(void * context, OperationalDeviceProxy * deviceProxy) +void DefaultOTARequestor::OnConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { DefaultOTARequestor * requestorCore = static_cast(context); VerifyOrDie(requestorCore != nullptr); - VerifyOrDie(deviceProxy != nullptr); switch (requestorCore->mOnConnectedAction) { case kQueryImage: { - CHIP_ERROR err = requestorCore->SendQueryImageRequest(*deviceProxy); + CHIP_ERROR err = requestorCore->SendQueryImageRequest(exchangeMgr, sessionHandle); if (err != CHIP_NO_ERROR) { @@ -436,7 +434,7 @@ void DefaultOTARequestor::OnConnected(void * context, OperationalDeviceProxy * d break; } case kDownload: { - CHIP_ERROR err = requestorCore->StartDownload(*deviceProxy); + CHIP_ERROR err = requestorCore->StartDownload(exchangeMgr, sessionHandle); if (err != CHIP_NO_ERROR) { @@ -447,7 +445,7 @@ void DefaultOTARequestor::OnConnected(void * context, OperationalDeviceProxy * d break; } case kApplyUpdate: { - CHIP_ERROR err = requestorCore->SendApplyUpdateRequest(*deviceProxy); + CHIP_ERROR err = requestorCore->SendApplyUpdateRequest(exchangeMgr, sessionHandle); if (err != CHIP_NO_ERROR) { @@ -458,7 +456,7 @@ void DefaultOTARequestor::OnConnected(void * context, OperationalDeviceProxy * d break; } case kNotifyUpdateApplied: { - CHIP_ERROR err = requestorCore->SendNotifyUpdateAppliedRequest(*deviceProxy); + CHIP_ERROR err = requestorCore->SendNotifyUpdateAppliedRequest(exchangeMgr, sessionHandle); if (err != CHIP_NO_ERROR) { @@ -724,7 +722,7 @@ CHIP_ERROR DefaultOTARequestor::GenerateUpdateToken() return CHIP_NO_ERROR; } -CHIP_ERROR DefaultOTARequestor::SendQueryImageRequest(OperationalDeviceProxy & deviceProxy) +CHIP_ERROR DefaultOTARequestor::SendQueryImageRequest(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { VerifyOrReturnError(mProviderLocation.HasValue(), CHIP_ERROR_INCORRECT_STATE); @@ -760,8 +758,7 @@ CHIP_ERROR DefaultOTARequestor::SendQueryImageRequest(OperationalDeviceProxy & d args.location.SetValue(CharSpan("XX", strlen("XX"))); } - Controller::OtaSoftwareUpdateProviderCluster cluster(*deviceProxy.GetExchangeManager(), deviceProxy.GetSecureSession().Value(), - mProviderLocation.Value().endpoint); + Controller::OtaSoftwareUpdateProviderCluster cluster(exchangeMgr, sessionHandle, mProviderLocation.Value().endpoint); return cluster.InvokeCommand(args, this, OnQueryImageResponse, OnQueryImageFailure); } @@ -792,7 +789,7 @@ CHIP_ERROR DefaultOTARequestor::ExtractUpdateDescription(const QueryImageRespons return CHIP_NO_ERROR; } -CHIP_ERROR DefaultOTARequestor::StartDownload(OperationalDeviceProxy & deviceProxy) +CHIP_ERROR DefaultOTARequestor::StartDownload(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { VerifyOrReturnError(mBdxDownloader != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -804,13 +801,7 @@ CHIP_ERROR DefaultOTARequestor::StartDownload(OperationalDeviceProxy & devicePro initOptions.FileDesLength = static_cast(mFileDesignator.size()); initOptions.FileDesignator = reinterpret_cast(mFileDesignator.data()); - chip::Messaging::ExchangeManager * exchangeMgr = deviceProxy.GetExchangeManager(); - VerifyOrReturnError(exchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); - - Optional session = deviceProxy.GetSecureSession(); - VerifyOrReturnError(session.HasValue(), CHIP_ERROR_INCORRECT_STATE); - - chip::Messaging::ExchangeContext * exchangeCtx = exchangeMgr->NewContext(session.Value(), &mBdxMessenger); + chip::Messaging::ExchangeContext * exchangeCtx = exchangeMgr.NewContext(sessionHandle, &mBdxMessenger); VerifyOrReturnError(exchangeCtx != nullptr, CHIP_ERROR_NO_MEMORY); mBdxMessenger.Init(mBdxDownloader, exchangeCtx); @@ -831,7 +822,7 @@ CHIP_ERROR DefaultOTARequestor::StartDownload(OperationalDeviceProxy & devicePro return err; } -CHIP_ERROR DefaultOTARequestor::SendApplyUpdateRequest(OperationalDeviceProxy & deviceProxy) +CHIP_ERROR DefaultOTARequestor::SendApplyUpdateRequest(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { VerifyOrReturnError(mProviderLocation.HasValue(), CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(GenerateUpdateToken()); @@ -840,13 +831,13 @@ CHIP_ERROR DefaultOTARequestor::SendApplyUpdateRequest(OperationalDeviceProxy & args.updateToken = mUpdateToken; args.newVersion = mTargetVersion; - Controller::OtaSoftwareUpdateProviderCluster cluster(*deviceProxy.GetExchangeManager(), deviceProxy.GetSecureSession().Value(), - mProviderLocation.Value().endpoint); + Controller::OtaSoftwareUpdateProviderCluster cluster(exchangeMgr, sessionHandle, mProviderLocation.Value().endpoint); return cluster.InvokeCommand(args, this, OnApplyUpdateResponse, OnApplyUpdateFailure); } -CHIP_ERROR DefaultOTARequestor::SendNotifyUpdateAppliedRequest(OperationalDeviceProxy & deviceProxy) +CHIP_ERROR DefaultOTARequestor::SendNotifyUpdateAppliedRequest(Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) { VerifyOrReturnError(mProviderLocation.HasValue(), CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(GenerateUpdateToken()); @@ -855,8 +846,7 @@ CHIP_ERROR DefaultOTARequestor::SendNotifyUpdateAppliedRequest(OperationalDevice args.updateToken = mUpdateToken; args.softwareVersion = mCurrentVersion; - Controller::OtaSoftwareUpdateProviderCluster cluster(*deviceProxy.GetExchangeManager(), deviceProxy.GetSecureSession().Value(), - mProviderLocation.Value().endpoint); + Controller::OtaSoftwareUpdateProviderCluster cluster(exchangeMgr, sessionHandle, mProviderLocation.Value().endpoint); // There is no response for a notify so consider this OTA complete. Clear the provider location and reset any states to indicate // so. diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestor.h b/src/app/clusters/ota-requestor/DefaultOTARequestor.h index f80f7555fe05ee..0dc04efae08f8e 100644 --- a/src/app/clusters/ota-requestor/DefaultOTARequestor.h +++ b/src/app/clusters/ota-requestor/DefaultOTARequestor.h @@ -207,7 +207,6 @@ class DefaultOTARequestor : public OTARequestorInterface, public BDXDownloader:: ScopedNodeId GetProviderScopedId() const { - VerifyOrDie(mProviderLocation.HasValue()); return ScopedNodeId(mProviderLocation.Value().providerNodeID, mProviderLocation.Value().fabricIndex); } @@ -229,7 +228,7 @@ class DefaultOTARequestor : public OTARequestorInterface, public BDXDownloader:: /** * Send QueryImage request using values matching Basic cluster */ - CHIP_ERROR SendQueryImageRequest(OperationalDeviceProxy & deviceProxy); + CHIP_ERROR SendQueryImageRequest(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); /** * Validate and extract mandatory information from QueryImageResponse @@ -260,17 +259,17 @@ class DefaultOTARequestor : public OTARequestorInterface, public BDXDownloader:: /** * Start download of the software image returned in QueryImageResponse */ - CHIP_ERROR StartDownload(OperationalDeviceProxy & deviceProxy); + CHIP_ERROR StartDownload(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); /** * Send ApplyUpdate request using values obtained from QueryImageResponse */ - CHIP_ERROR SendApplyUpdateRequest(OperationalDeviceProxy & deviceProxy); + CHIP_ERROR SendApplyUpdateRequest(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); /** * Send NotifyUpdateApplied request */ - CHIP_ERROR SendNotifyUpdateAppliedRequest(OperationalDeviceProxy & deviceProxy); + CHIP_ERROR SendNotifyUpdateAppliedRequest(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); /** * Store current update information to KVS @@ -285,7 +284,7 @@ class DefaultOTARequestor : public OTARequestorInterface, public BDXDownloader:: /** * Session connection callbacks */ - static void OnConnected(void * context, OperationalDeviceProxy * deviceProxy); + static void OnConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); static void OnConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); Callback::Callback mOnConnectedCallback; Callback::Callback mOnConnectionFailureCallback; diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 15674b10b92b38..bc8f94335fb46e 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -294,7 +294,7 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) .groupDataProvider = mGroupsProvider, .mrpLocalConfig = GetLocalMRPConfig(), }, - .devicePool = &mDevicePool, + .sessionSetupPool = &mSessionSetupPool, }; err = mCASESessionManager.Init(&DeviceLayer::SystemLayer(), caseSessionManagerConfig); diff --git a/src/app/server/Server.h b/src/app/server/Server.h index af4796d33f4597..7e1419e50c320c 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -25,7 +25,7 @@ #include #include #include -#include +#include #include #include #include @@ -491,7 +491,7 @@ class Server CASESessionManager mCASESessionManager; CASEClientPool mCASEClientPool; - OperationalDeviceProxyPool mDevicePool; + OperationalSessionSetupPool mSessionSetupPool; Protocols::SecureChannel::UnsolicitedStatusHandler mUnsolicitedStatusHandler; Messaging::ExchangeManager mExchangeMgr; diff --git a/src/app/tests/suites/certification/Test_TC_CADMIN_1_15.yaml b/src/app/tests/suites/certification/Test_TC_CADMIN_1_15.yaml index 8933123c9d9b88..3c6e8d6c0f794a 100644 --- a/src/app/tests/suites/certification/Test_TC_CADMIN_1_15.yaml +++ b/src/app/tests/suites/certification/Test_TC_CADMIN_1_15.yaml @@ -170,13 +170,13 @@ tests: ./chip-tool basic write node-label te5new 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 ./chip-tool basic read node-label 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 disabled: true diff --git a/src/app/tests/suites/certification/Test_TC_CADMIN_1_16.yaml b/src/app/tests/suites/certification/Test_TC_CADMIN_1_16.yaml index 53669ad633a1c0..de1771e913f767 100644 --- a/src/app/tests/suites/certification/Test_TC_CADMIN_1_16.yaml +++ b/src/app/tests/suites/certification/Test_TC_CADMIN_1_16.yaml @@ -208,13 +208,13 @@ tests: ./chip-tool basic write node-label te5new 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 ./chip-tool basic read node-label 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 disabled: true diff --git a/src/app/tests/suites/certification/Test_TC_CADMIN_1_17.yaml b/src/app/tests/suites/certification/Test_TC_CADMIN_1_17.yaml index ec94c4c5150c93..15f161f42b333a 100644 --- a/src/app/tests/suites/certification/Test_TC_CADMIN_1_17.yaml +++ b/src/app/tests/suites/certification/Test_TC_CADMIN_1_17.yaml @@ -203,13 +203,13 @@ tests: ./chip-tool basic write node-label te5new 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 ./chip-tool basic read node-label 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 disabled: true diff --git a/src/app/tests/suites/certification/Test_TC_CADMIN_1_18.yaml b/src/app/tests/suites/certification/Test_TC_CADMIN_1_18.yaml index 70df68ebac1b88..f27a2d4cae3657 100644 --- a/src/app/tests/suites/certification/Test_TC_CADMIN_1_18.yaml +++ b/src/app/tests/suites/certification/Test_TC_CADMIN_1_18.yaml @@ -215,13 +215,13 @@ tests: ./chip-tool basic write node-label te5new 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 ./chip-tool basic read node-label 2 0 Received error (protocol code 2) during pairing process. ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter - [1651819620.929567][4359:4364] CHIP:CTL: OperationalDeviceProxy[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 + [1651819620.929567][4359:4364] CHIP:CTL: OperationalSessionSetup[B8070CD13C99D367:0000000000000002]: State change 3 --> 2 [1651819620.929700][4359:4364] CHIP:-: ../../third_party/connectedhomeip/src/protocols/secure_channel/CASESession.cpp:1551: CHIP Error 0x00000054: Invalid CASE parameter at ../../commands/clusters/ModelCommand.cpp:53 disabled: true diff --git a/src/app/tests/suites/certification/Test_TC_DD_3_1.yaml b/src/app/tests/suites/certification/Test_TC_DD_3_1.yaml index 7d21c8276e1f1e..7e8d68ca88ab75 100644 --- a/src/app/tests/suites/certification/Test_TC_DD_3_1.yaml +++ b/src/app/tests/suites/certification/Test_TC_DD_3_1.yaml @@ -378,8 +378,8 @@ tests: CHIP:CTL: Commissioning stage next step: "SendNOC" -> "FindOperational" [1653471976.344532][30157:30162] CHIP:CTL: Performing next commissioning step "FindOperational" [1653471976.344586][30157:30162] CHIP:CSM: FindOrEstablishSession: PeerId = CCCB8A2597E4538B:0000000000000001 - [1653471976.344642][30157:30162] CHIP:CSM: FindOrEstablishSession: No existing OperationalDeviceProxy instance found - [1653471976.344715][30157:30162] CHIP:CTL: OperationalDeviceProxy[CCCB8A2597E4538B:0000000000000001]: State change 1 --> 2 + [1653471976.344642][30157:30162] CHIP:CSM: FindOrEstablishSession: No existing OperationalSessionSetup instance found + [1653471976.344715][30157:30162] CHIP:CTL: OperationalSessionSetup[CCCB8A2597E4538B:0000000000000001]: State change 1 --> 2 [1653471976.344783][30157:30162] CHIP:DIS: Resolving CCCB8A2597E4538B:0000000000000001 ... [1653471976.346864][30157:30162] CHIP:DIS: Operational node lookup already in progress. Will NOT start a new one. [1653471976.347000][30157:30162] CHIP:DMG: ICR moving to [AwaitingDe] diff --git a/src/app/tests/suites/certification/Test_TC_DD_3_5.yaml b/src/app/tests/suites/certification/Test_TC_DD_3_5.yaml index 92c0210c262910..3b325e86e2373a 100644 --- a/src/app/tests/suites/certification/Test_TC_DD_3_5.yaml +++ b/src/app/tests/suites/certification/Test_TC_DD_3_5.yaml @@ -275,8 +275,8 @@ tests: Commissioning stage next step: "WiFiNetworkEnable" -> "FindOperational" [1653471976.344532][30157:30162] CHIP:CTL: Performing next commissioning step "FindOperational" [1653471976.344586][30157:30162] CHIP:CSM: FindOrEstablishSession: PeerId = CCCB8A2597E4538B:0000000000000001 - [1653471976.344642][30157:30162] CHIP:CSM: FindOrEstablishSession: No existing OperationalDeviceProxy instance found - [1653471976.344715][30157:30162] CHIP:CTL: OperationalDeviceProxy[CCCB8A2597E4538B:0000000000000001]: State change 1 --> 2 + [1653471976.344642][30157:30162] CHIP:CSM: FindOrEstablishSession: No existing OperationalSessionSetup instance found + [1653471976.344715][30157:30162] CHIP:CTL: OperationalSessionSetup[CCCB8A2597E4538B:0000000000000001]: State change 1 --> 2 [1653471976.344783][30157:30162] CHIP:DIS: Resolving CCCB8A2597E4538B:0000000000000001 ... [1653471976.346864][30157:30162] CHIP:DIS: Operational node lookup already in progress. Will NOT start a new one. [1653471976.347000][30157:30162] CHIP:DMG: ICR moving to [AwaitingDe] diff --git a/src/app/tests/suites/certification/Test_TC_DD_3_7.yaml b/src/app/tests/suites/certification/Test_TC_DD_3_7.yaml index cbf22f85473687..68eaf6b86de336 100644 --- a/src/app/tests/suites/certification/Test_TC_DD_3_7.yaml +++ b/src/app/tests/suites/certification/Test_TC_DD_3_7.yaml @@ -419,7 +419,7 @@ tests: CHIP:CTL: Performing next commissioning step "FindOperational" CHIP:CSM: FindOrEstablishSession: PeerId = BFCBED670D527591:000000000001B669 - CHIP:CSM: FindOrEstablishSession: No existing OperationalDeviceProxy instance found + CHIP:CSM: FindOrEstablishSession: No existing OperationalSessionSetup instance found disabled: true - label: "Reboot TH" @@ -446,7 +446,7 @@ tests: CHIP:CTL: Performing next commissioning step "FindOperational" CHIP:CSM: FindOrEstablishSession: PeerId = BFCBED670D527591:000000000001B669 - CHIP:CSM: FindOrEstablishSession: No existing OperationalDeviceProxy instance found + CHIP:CSM: FindOrEstablishSession: No existing OperationalSessionSetup instance found disabled: true - label: "Commissioner starts discovery of TH using Operational Discovery" diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 6a3b064feaac83..62cf5ff16912da 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -513,7 +513,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio ReleasePAI(); ReleaseDAC(); mCommissioneeDeviceProxy = nullptr; - mOperationalDeviceProxy = nullptr; + mOperationalDeviceProxy = OperationalDeviceProxy(); mDeviceCommissioningInfo = ReadCommissioningInfo(); return CHIP_NO_ERROR; default: @@ -553,9 +553,9 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio DeviceProxy * AutoCommissioner::GetDeviceProxyForStep(CommissioningStage nextStage) { if (nextStage == CommissioningStage::kSendComplete || - (nextStage == CommissioningStage::kCleanup && mOperationalDeviceProxy != nullptr)) + (nextStage == CommissioningStage::kCleanup && mOperationalDeviceProxy.ConnectionReady())) { - return mOperationalDeviceProxy; + return &mOperationalDeviceProxy; } return mCommissioneeDeviceProxy; } diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 422b6ef26911d9..25932e213c69c8 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -84,9 +84,9 @@ class AutoCommissioner : public CommissioningDelegate DeviceCommissioner * mCommissioner = nullptr; CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr; - OperationalDeviceProxy * mOperationalDeviceProxy = nullptr; OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr; CommissioningParameters mParams = CommissioningParameters(); + OperationalDeviceProxy mOperationalDeviceProxy; // Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory uint8_t mSsid[CommissioningParameters::kMaxSsidLen]; uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen]; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 11ea5dca6b5e55..da29daeac289b9 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -43,7 +43,7 @@ #include #include -#include +#include #include #include #include @@ -364,16 +364,15 @@ CHIP_ERROR DeviceController::DisconnectDevice(NodeId nodeId) { ChipLogProgress(Controller, "Force close session for node 0x%" PRIx64, nodeId); - OperationalDeviceProxy * proxy = mSystemState->CASESessionMgr()->FindExistingSession(GetPeerScopedId(nodeId)); - if (proxy == nullptr) + if (SessionMgr()->MarkSessionsAsDefunct(GetPeerScopedId(nodeId), MakeOptional(Transport::SecureSession::Type::kCASE))) { - ChipLogProgress(Controller, "Attempted to close a session that does not exist."); return CHIP_NO_ERROR; } - if (proxy->IsConnected()) + OperationalSessionSetup * proxy = mSystemState->CASESessionMgr()->FindExistingSessionSetup(GetPeerScopedId(nodeId)); + if (proxy == nullptr) { - proxy->Disconnect(); + ChipLogProgress(Controller, "Attempted to close a session that does not exist."); return CHIP_NO_ERROR; } @@ -1615,7 +1614,8 @@ void DeviceCommissioner::CommissioningStageComplete(CHIP_ERROR err, Commissionin } } -void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device) +void DeviceCommissioner::OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) { // CASE session established. DeviceCommissioner * commissioner = static_cast(context); @@ -1629,7 +1629,7 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDevicePr } if (commissioner->mDeviceBeingCommissioned == nullptr || - commissioner->mDeviceBeingCommissioned->GetDeviceId() != device->GetDeviceId()) + commissioner->mDeviceBeingCommissioned->GetDeviceId() != sessionHandle->GetPeer().GetNodeId()) { // Not the device we are trying to commission. return; @@ -1638,7 +1638,7 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDevicePr if (commissioner->mCommissioningDelegate != nullptr) { CommissioningDelegate::CommissioningReport report; - report.Set(OperationalNodeFoundData(device)); + report.Set(OperationalNodeFoundData(OperationalDeviceProxy(&exchangeMgr, sessionHandle))); commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); } } @@ -2316,23 +2316,23 @@ CHIP_ERROR DeviceController::UpdateDevice(NodeId peerNodeId) { VerifyOrReturnError(mState == State::Initialized && mFabricIndex != kUndefinedFabricIndex, CHIP_ERROR_INCORRECT_STATE); - OperationalDeviceProxy * proxy = GetDeviceSession(GetPeerScopedId(peerNodeId)); - VerifyOrReturnError(proxy != nullptr, CHIP_ERROR_NOT_FOUND); + OperationalSessionSetup * sessionSetup = GetDeviceSession(GetPeerScopedId(peerNodeId)); + VerifyOrReturnError(sessionSetup != nullptr, CHIP_ERROR_NOT_FOUND); - return proxy->LookupPeerAddress(); + return sessionSetup->LookupPeerAddress(); } -OperationalDeviceProxy * DeviceController::GetDeviceSession(const ScopedNodeId & peerId) +OperationalSessionSetup * DeviceController::GetDeviceSession(const ScopedNodeId & peerId) { - return mSystemState->CASESessionMgr()->FindExistingSession(peerId); + return mSystemState->CASESessionMgr()->FindExistingSessionSetup(peerId); } -OperationalDeviceProxy * DeviceCommissioner::GetDeviceSession(const ScopedNodeId & peerId) +OperationalSessionSetup * DeviceCommissioner::GetDeviceSession(const ScopedNodeId & peerId) { mSystemState->CASESessionMgr()->FindOrEstablishSession(peerId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback); - // If there is an OperationalDeviceProxy for this peerId now the call to the + // If there is an OperationalSessionSetup for this peerId now the call to the // superclass will return it. return DeviceController::GetDeviceSession(peerId); } diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 8f04fe771ae5f0..2f9734816cae4e 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -31,8 +31,8 @@ #include #include #include -#include -#include +#include +#include #include #include #include @@ -371,12 +371,12 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController /// Fetches the session to use for the current device. Allows overriding /// in case subclasses want to create the session if it does not yet exist - virtual OperationalDeviceProxy * GetDeviceSession(const ScopedNodeId & peerId); + virtual OperationalSessionSetup * GetDeviceSession(const ScopedNodeId & peerId); DiscoveredNodeList GetDiscoveredNodes() override { return DiscoveredNodeList(mCommissionableNodes); } private: - void ReleaseOperationalDevice(OperationalDeviceProxy * device); + void ReleaseOperationalDevice(OperationalSessionSetup * device); }; #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY @@ -661,7 +661,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, void OnDone(app::ReadClient *) override; // Commissioner will establish new device connections after PASE. - OperationalDeviceProxy * GetDeviceSession(const ScopedNodeId & peerId) override; + OperationalSessionSetup * GetDeviceSession(const ScopedNodeId & peerId) override; // Issue an NOC chain using the associated OperationalCredentialsDelegate. The NOC chain will // be provided in X509 DER format. @@ -760,7 +760,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, /* Callback called when adding root cert to device results in failure */ static void OnRootCertFailureResponse(void * context, CHIP_ERROR error); - static void OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device); + static void OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); static void OnDeviceConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); static void OnDeviceAttestationInformationVerification(void * context, Credentials::AttestationVerificationResult result); diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 0b9b4eca0c12b2..1c06e8217f3eeb 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -24,7 +24,7 @@ #include -#include +#include #include #include #include @@ -242,8 +242,8 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) chip::app::DnssdServer::Instance().StartServer(); } - stateParams.operationalDevicePool = Platform::New(); - stateParams.caseClientPool = Platform::New(); + stateParams.sessionSetupPool = Platform::New(); + stateParams.caseClientPool = Platform::New(); DeviceProxyInitParams deviceInitParams = { .sessionManager = stateParams.sessionMgr, @@ -257,7 +257,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) CASESessionManagerConfig sessionManagerConfig = { .sessionInitParams = deviceInitParams, - .devicePool = stateParams.operationalDevicePool, + .sessionSetupPool = stateParams.sessionSetupPool, }; // TODO: Need to be able to create a CASESessionManagerConfig here! @@ -390,13 +390,13 @@ void DeviceControllerSystemState::Shutdown() mCASESessionManager = nullptr; } - // mCASEClientPool and mDevicePool must be deallocated + // mCASEClientPool and mSessionSetupPool must be deallocated // after mCASESessionManager, which uses them. - if (mOperationalDevicePool != nullptr) + if (mSessionSetupPool != nullptr) { - Platform::Delete(mOperationalDevicePool); - mOperationalDevicePool = nullptr; + Platform::Delete(mSessionSetupPool); + mSessionSetupPool = nullptr; } if (mCASEClientPool != nullptr) diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index 9bebb56ed3b1d5..8cf83063464c5c 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -69,8 +69,8 @@ namespace Controller { struct DeviceControllerSystemStateParams { - using OperationalDevicePool = OperationalDeviceProxyPool; - using CASEClientPool = chip::CASEClientPool; + using SessionSetupPool = OperationalSessionSetupPool; + using CASEClientPool = chip::CASEClientPool; // Params that can outlive the DeviceControllerSystemState System::Layer * systemLayer = nullptr; @@ -93,7 +93,7 @@ struct DeviceControllerSystemStateParams secure_channel::MessageCounterManager * messageCounterManager = nullptr; CASEServer * caseServer = nullptr; CASESessionManager * caseSessionManager = nullptr; - OperationalDevicePool * operationalDevicePool = nullptr; + SessionSetupPool * sessionSetupPool = nullptr; CASEClientPool * caseClientPool = nullptr; FabricTable::Delegate * fabricTableDelegate = nullptr; }; @@ -107,8 +107,8 @@ struct DeviceControllerSystemStateParams // owned by DeviceControllerFactory. class DeviceControllerSystemState { - using OperationalDevicePool = DeviceControllerSystemStateParams::OperationalDevicePool; - using CASEClientPool = DeviceControllerSystemStateParams::CASEClientPool; + using SessionSetupPool = DeviceControllerSystemStateParams::SessionSetupPool; + using CASEClientPool = DeviceControllerSystemStateParams::CASEClientPool; public: ~DeviceControllerSystemState() @@ -124,7 +124,7 @@ class DeviceControllerSystemState mUDPEndPointManager(params.udpEndPointManager), mTransportMgr(params.transportMgr), mSessionMgr(params.sessionMgr), mUnsolicitedStatusHandler(params.unsolicitedStatusHandler), mExchangeMgr(params.exchangeMgr), mMessageCounterManager(params.messageCounterManager), mFabrics(params.fabricTable), mCASEServer(params.caseServer), - mCASESessionManager(params.caseSessionManager), mOperationalDevicePool(params.operationalDevicePool), + mCASESessionManager(params.caseSessionManager), mSessionSetupPool(params.sessionSetupPool), mCASEClientPool(params.caseClientPool), mGroupDataProvider(params.groupDataProvider), mFabricTableDelegate(params.fabricTableDelegate), mSessionResumptionStorage(std::move(params.sessionResumptionStorage)) { @@ -164,8 +164,8 @@ class DeviceControllerSystemState { return mSystemLayer != nullptr && mUDPEndPointManager != nullptr && mTransportMgr != nullptr && mSessionMgr != nullptr && mUnsolicitedStatusHandler != nullptr && mExchangeMgr != nullptr && mMessageCounterManager != nullptr && - mFabrics != nullptr && mCASESessionManager != nullptr && mOperationalDevicePool != nullptr && - mCASEClientPool != nullptr && mGroupDataProvider != nullptr; + mFabrics != nullptr && mCASESessionManager != nullptr && mSessionSetupPool != nullptr && mCASEClientPool != nullptr && + mGroupDataProvider != nullptr; }; System::Layer * SystemLayer() const { return mSystemLayer; }; @@ -200,7 +200,7 @@ class DeviceControllerSystemState FabricTable * mFabrics = nullptr; CASEServer * mCASEServer = nullptr; CASESessionManager * mCASESessionManager = nullptr; - OperationalDevicePool * mOperationalDevicePool = nullptr; + SessionSetupPool * mSessionSetupPool = nullptr; CASEClientPool * mCASEClientPool = nullptr; Credentials::GroupDataProvider * mGroupDataProvider = nullptr; FabricTable::Delegate * mFabricTableDelegate = nullptr; diff --git a/src/controller/CommissionerDiscoveryController.cpp b/src/controller/CommissionerDiscoveryController.cpp index 50ce140ecd2a9f..0177e8adecb8c1 100644 --- a/src/controller/CommissionerDiscoveryController.cpp +++ b/src/controller/CommissionerDiscoveryController.cpp @@ -178,7 +178,8 @@ void CommissionerDiscoveryController::Cancel() } void CommissionerDiscoveryController::CommissioningSucceeded(uint16_t vendorId, uint16_t productId, NodeId nodeId, - OperationalDeviceProxy * device) + Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) { mVendorId = vendorId; mProductId = productId; @@ -186,7 +187,7 @@ void CommissionerDiscoveryController::CommissioningSucceeded(uint16_t vendorId, if (mPostCommissioningListener != nullptr) { ChipLogDetail(Controller, "CommissionerDiscoveryController calling listener"); - mPostCommissioningListener->CommissioningCompleted(vendorId, productId, nodeId, device); + mPostCommissioningListener->CommissioningCompleted(vendorId, productId, nodeId, exchangeMgr, sessionHandle); } else { diff --git a/src/controller/CommissionerDiscoveryController.h b/src/controller/CommissionerDiscoveryController.h index 143c16465b1638..8d0c36a09b5674 100644 --- a/src/controller/CommissionerDiscoveryController.h +++ b/src/controller/CommissionerDiscoveryController.h @@ -28,7 +28,7 @@ #pragma once -#include +#include #include #include #include @@ -39,7 +39,7 @@ #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY using chip::NodeId; -using chip::OperationalDeviceProxy; +using chip::OperationalSessionSetup; using chip::Protocols::UserDirectedCommissioning::UDCClientState; using chip::Protocols::UserDirectedCommissioning::UserConfirmationProvider; using chip::Protocols::UserDirectedCommissioning::UserDirectedCommissioningServer; @@ -133,10 +133,12 @@ class DLL_EXPORT PostCommissioningListener * @param[in] vendorId The vendorid from the DAC of the new node. * @param[in] productId The productid from the DAC of the new node. * @param[in] nodeId The node id for the newly commissioned node. - * @param[in] device The device proxy for use in cluster communication. + * @param[in] exchangeMgr The exchange manager to be used to get an exchange context. + * @param[in] sessionHandle A reference to an established session. * */ - virtual void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, OperationalDeviceProxy * device) = 0; + virtual void CommissioningCompleted(uint16_t vendorId, uint16_t productId, NodeId nodeId, + chip::Messaging::ExchangeManager & exchangeMgr, chip::SessionHandle & sessionHandle) = 0; virtual ~PostCommissioningListener() = default; }; @@ -210,10 +212,12 @@ class CommissionerDiscoveryController : public chip::Protocols::UserDirectedComm * @param[in] vendorId The vendorid from the DAC of the new node. * @param[in] productId The productid from the DAC of the new node. * @param[in] nodeId The node id for the newly commissioned node. - * @param[in] device The device proxy for use in cluster communication. + * @param[in] exchangeMgr The exchange manager to be used to get an exchange context. + * @param[in] sessionHandle A reference to an established session. * */ - void CommissioningSucceeded(uint16_t vendorId, uint16_t productId, NodeId nodeId, OperationalDeviceProxy * device); + void CommissioningSucceeded(uint16_t vendorId, uint16_t productId, NodeId nodeId, + chip::Messaging::ExchangeManager & exchangeMgr, chip::SessionHandle & sessionHandle); /** * This method should be called by the commissioner to indicate that commissioning failed. diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 22d651e2c4be03..595978b3049646 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -17,7 +17,7 @@ */ #pragma once -#include +#include #include #include #include @@ -444,8 +444,8 @@ struct NocChain struct OperationalNodeFoundData { - OperationalNodeFoundData(OperationalDeviceProxy * proxy) : operationalProxy(proxy) {} - OperationalDeviceProxy * operationalProxy; + OperationalNodeFoundData(OperationalDeviceProxy proxy) : operationalProxy(proxy) {} + OperationalDeviceProxy operationalProxy; }; struct NetworkClusterInfo diff --git a/src/controller/CommissioningWindowOpener.cpp b/src/controller/CommissioningWindowOpener.cpp index 95eb4d7c7d2a77..9afe3d1dc53ab5 100644 --- a/src/controller/CommissioningWindowOpener.cpp +++ b/src/controller/CommissioningWindowOpener.cpp @@ -124,14 +124,14 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S return mController->GetConnectedDevice(mNodeId, &mDeviceConnected, &mDeviceConnectionFailure); } -CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(OperationalDeviceProxy * device) +CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) { ChipLogProgress(Controller, "OpenCommissioningWindow for device ID %" PRIu64, mNodeId); constexpr EndpointId kAdministratorCommissioningClusterEndpoint = 0; - AdministratorCommissioningCluster cluster(*device->GetExchangeManager(), device->GetSecureSession().Value(), - kAdministratorCommissioningClusterEndpoint); + AdministratorCommissioningCluster cluster(exchangeMgr, sessionHandle, kAdministratorCommissioningClusterEndpoint); if (mCommissioningWindowOption != CommissioningWindowOption::kOriginalSetupCode) { @@ -254,7 +254,8 @@ void CommissioningWindowOpener::OnOpenCommissioningWindowFailure(void * context, } } -void CommissioningWindowOpener::OnDeviceConnectedCallback(void * context, OperationalDeviceProxy * device) +void CommissioningWindowOpener::OnDeviceConnectedCallback(void * context, Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) { auto * self = static_cast(context); @@ -267,7 +268,7 @@ void CommissioningWindowOpener::OnDeviceConnectedCallback(void * context, Operat { case Step::kReadVID: { constexpr EndpointId kBasicClusterEndpoint = 0; - BasicCluster cluster(*device->GetExchangeManager(), device->GetSecureSession().Value(), kBasicClusterEndpoint); + BasicCluster cluster(exchangeMgr, sessionHandle, kBasicClusterEndpoint); err = cluster.ReadAttribute(context, OnVIDReadResponse, OnVIDPIDReadFailureResponse); #if CHIP_ERROR_LOGGING @@ -277,7 +278,7 @@ void CommissioningWindowOpener::OnDeviceConnectedCallback(void * context, Operat } case Step::kReadPID: { constexpr EndpointId kBasicClusterEndpoint = 0; - BasicCluster cluster(*device->GetExchangeManager(), device->GetSecureSession().Value(), kBasicClusterEndpoint); + BasicCluster cluster(exchangeMgr, sessionHandle, kBasicClusterEndpoint); err = cluster.ReadAttribute(context, OnPIDReadResponse, OnVIDPIDReadFailureResponse); #if CHIP_ERROR_LOGGING @@ -286,7 +287,7 @@ void CommissioningWindowOpener::OnDeviceConnectedCallback(void * context, Operat break; } case Step::kOpenCommissioningWindow: { - err = self->OpenCommissioningWindowInternal(device); + err = self->OpenCommissioningWindowInternal(exchangeMgr, sessionHandle); #if CHIP_ERROR_LOGGING messageIfError = "Could not connect to open commissioning window"; #endif // CHIP_ERROR_LOGGING diff --git a/src/controller/CommissioningWindowOpener.h b/src/controller/CommissioningWindowOpener.h index 63a073122e8d3e..4e314669201add 100644 --- a/src/controller/CommissioningWindowOpener.h +++ b/src/controller/CommissioningWindowOpener.h @@ -17,7 +17,7 @@ #pragma once -#include +#include #include #include #include @@ -120,13 +120,13 @@ class CommissioningWindowOpener kOpenCommissioningWindow, }; - CHIP_ERROR OpenCommissioningWindowInternal(OperationalDeviceProxy * device); + CHIP_ERROR OpenCommissioningWindowInternal(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); static void OnPIDReadResponse(void * context, uint16_t value); static void OnVIDReadResponse(void * context, VendorId value); static void OnVIDPIDReadFailureResponse(void * context, CHIP_ERROR error); static void OnOpenCommissioningWindowSuccess(void * context, const app::DataModel::NullObjectType &); static void OnOpenCommissioningWindowFailure(void * context, CHIP_ERROR error); - static void OnDeviceConnectedCallback(void * context, OperationalDeviceProxy * device); + static void OnDeviceConnectedCallback(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); static void OnDeviceConnectionFailureCallback(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); DeviceController * const mController = nullptr; diff --git a/src/controller/java/AndroidCallbacks.cpp b/src/controller/java/AndroidCallbacks.cpp index 1966bd59ecca68..d8732579455be4 100644 --- a/src/controller/java/AndroidCallbacks.cpp +++ b/src/controller/java/AndroidCallbacks.cpp @@ -57,7 +57,8 @@ GetConnectedDeviceCallback::~GetConnectedDeviceCallback() env->DeleteGlobalRef(mJavaCallbackRef); } -void GetConnectedDeviceCallback::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device) +void GetConnectedDeviceCallback::OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, + SessionHandle & sessionHandle) { JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread(); auto * self = static_cast(context); @@ -78,6 +79,8 @@ void GetConnectedDeviceCallback::OnDeviceConnectedFn(void * context, Operational VerifyOrReturn(successMethod != nullptr, ChipLogError(Controller, "Could not find onDeviceConnected method")); static_assert(sizeof(jlong) >= sizeof(void *), "Need to store a pointer in a Java handle"); + + OperationalDeviceProxy * device = new OperationalDeviceProxy(&exchangeMgr, sessionHandle); DeviceLayer::StackUnlock unlock; env->CallVoidMethod(javaCallback, successMethod, reinterpret_cast(device)); } diff --git a/src/controller/java/AndroidCallbacks.h b/src/controller/java/AndroidCallbacks.h index 9ba97362f53340..64232cdd1e1494 100644 --- a/src/controller/java/AndroidCallbacks.h +++ b/src/controller/java/AndroidCallbacks.h @@ -33,7 +33,7 @@ struct GetConnectedDeviceCallback GetConnectedDeviceCallback(jobject wrapperCallback, jobject javaCallback); ~GetConnectedDeviceCallback(); - static void OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device); + static void OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); static void OnDeviceConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); Callback::Callback mOnSuccess; diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index 070c993a3ab674..420a17e20160f8 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -671,6 +671,16 @@ JNI_METHOD(void, getConnectedDevicePointer)(JNIEnv * env, jobject self, jlong ha VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Error invoking GetConnectedDevice")); } +JNI_METHOD(void, releaseConnectedDevicePointer)(JNIEnv * env, jobject self, jlong devicePtr) +{ + chip::DeviceLayer::StackLock lock; + OperationalDeviceProxy * device = reinterpret_cast(devicePtr); + if (device != NULL) + { + delete device; + } +} + JNI_METHOD(void, disconnectDevice)(JNIEnv * env, jobject self, jlong handle, jlong deviceId) { chip::DeviceLayer::StackLock lock; diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java index e617cb39281a49..85809ad2e02085 100644 --- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java +++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java @@ -648,6 +648,8 @@ private native void commissionDevice( private native void getConnectedDevicePointer( long deviceControllerPtr, long deviceId, long callbackHandle); + private native void releaseConnectedDevicePointer(long devicePtr); + private native boolean disconnectDevice(long deviceControllerPtr, long deviceId); private native void deleteDeviceController(long deviceControllerPtr); diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 96a4a51d76209b..194004e0dc93fb 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -193,6 +193,7 @@ void pychip_Stack_SetLogFunct(LogMessageFunct logFunct); ChipError::StorageType pychip_GetConnectedDeviceByNodeId(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId, DeviceAvailableFunc callback); +ChipError::StorageType pychip_FreeOperationalDeviceProxy(chip::OperationalDeviceProxy * deviceProxy); ChipError::StorageType pychip_GetDeviceBeingCommissioned(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId, CommissioneeDeviceProxy ** proxy); ChipError::StorageType pychip_ExpireSessions(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId); @@ -668,16 +669,18 @@ const char * pychip_Stack_StatusReportToString(uint32_t profileId, uint16_t stat } namespace { + struct GetDeviceCallbacks { GetDeviceCallbacks(DeviceAvailableFunc callback) : mOnSuccess(OnDeviceConnectedFn, this), mOnFailure(OnConnectionFailureFn, this), mCallback(callback) {} - static void OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device) + static void OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { - auto * self = static_cast(context); - self->mCallback(device, CHIP_NO_ERROR.AsInteger()); + auto * self = static_cast(context); + auto * operationalDeviceProxy = new OperationalDeviceProxy(&exchangeMgr, sessionHandle); + self->mCallback(operationalDeviceProxy, CHIP_NO_ERROR.AsInteger()); delete self; } @@ -702,6 +705,15 @@ ChipError::StorageType pychip_GetConnectedDeviceByNodeId(chip::Controller::Devic return devCtrl->GetConnectedDevice(nodeId, &callbacks->mOnSuccess, &callbacks->mOnFailure).AsInteger(); } +ChipError::StorageType pychip_FreeOperationalDeviceProxy(chip::OperationalDeviceProxy * deviceProxy) +{ + if (deviceProxy != nullptr) + { + delete deviceProxy; + } + return CHIP_NO_ERROR.AsInteger(); +} + ChipError::StorageType pychip_GetDeviceBeingCommissioned(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId, CommissioneeDeviceProxy ** proxy) { diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 089fdae966f943..6c36350993d44f 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -117,6 +117,30 @@ class DCState(enum.IntEnum): COMMISSIONING = 5 +class DeviceProxyWrapper(): + ''' Encapsulates a pointer to OperationalDeviceProxy on the c++ side that needs to be + freed when DeviceProxyWrapper goes out of scope. There is a potential issue where + if this is copied around that a double free will occure, but how this is used today + that is not an issue that needs to be accounted for and it will become very apparent + if that happens. + ''' + + def __init__(self, deviceProxy: ctypes.c_void_p, dmLib=None): + self._deviceProxy = deviceProxy + self._dmLib = dmLib + + def __del__(self): + if (self._dmLib is not None and builtins.chipStack is not None): + # This destructor is called from any threading context, including on the Matter threading context. + # So, we cannot call chipStack.Call or chipStack.CallAsync which waits for the posted work to + # actually be executed. Instead, we just post/schedule the work and move on. + builtins.chipStack.PostTaskOnChipThread(lambda: self._dmLib.pychip_FreeOperationalDeviceProxy(self._deviceProxy)) + + @property + def deviceProxy(self) -> ctypes.c_void_p: + return self._deviceProxy + + class DiscoveryFilterType(enum.IntEnum): # These must match chip::Dnssd::DiscoveryFilterType values (barring the naming convention) NONE = 0 @@ -626,6 +650,7 @@ def GetClusterHandler(self): return self._Cluster def GetConnectedDeviceSync(self, nodeid, allowPASE=True, timeoutMs: int = None): + ''' Returns DeviceProxyWrapper upon success.''' self.CheckIsActive() returnDevice = c_void_p(None) @@ -647,7 +672,7 @@ def DeviceAvailableCallback(device, err): self.devCtrl, nodeid, byref(returnDevice)), timeoutMs) if res == 0: print('Using PASE connection') - return returnDevice + return DeviceProxyWrapper(returnDevice) res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetConnectedDeviceByNodeId( self.devCtrl, nodeid, DeviceAvailableCallback), timeoutMs) @@ -668,7 +693,8 @@ def DeviceAvailableCallback(device, err): if returnDevice.value is None: raise self._ChipStack.ErrorToException(returnErr) - return returnDevice + + return DeviceProxyWrapper(returnDevice, self._dmLib) def ComputeRoundTripTimeout(self, nodeid, upperLayerProcessingTimeoutMs: int = 0): ''' Returns a computed timeout value based on the round-trip time it takes for the peer at the other end of the session to @@ -680,7 +706,7 @@ def ComputeRoundTripTimeout(self, nodeid, upperLayerProcessingTimeoutMs: int = 0 ''' device = self.GetConnectedDeviceSync(nodeid) res = self._ChipStack.Call(lambda: self._dmLib.pychip_DeviceProxy_ComputeRoundTripTimeout( - device, upperLayerProcessingTimeoutMs)) + device.deviceProxy, upperLayerProcessingTimeoutMs)) return res async def SendCommand(self, nodeid: int, endpoint: int, payload: ClusterObjects.ClusterCommand, responseType=None, timedRequestTimeoutMs: int = None, interactionTimeoutMs: int = None): @@ -700,7 +726,7 @@ async def SendCommand(self, nodeid: int, endpoint: int, payload: ClusterObjects. device = self.GetConnectedDeviceSync(nodeid, timeoutMs=interactionTimeoutMs) res = ClusterCommand.SendCommand( - future, eventLoop, responseType, device, ClusterCommand.CommandPath( + future, eventLoop, responseType, device.deviceProxy, ClusterCommand.CommandPath( EndpointId=endpoint, ClusterId=payload.cluster_id, CommandId=payload.command_id, @@ -739,7 +765,7 @@ async def WriteAttribute(self, nodeid: int, attributes: typing.List[typing.Tuple v[0], v[1], v[2], 1, v[1].value)) res = ClusterAttribute.WriteAttributes( - future, eventLoop, device, attrs, timedRequestTimeoutMs=timedRequestTimeoutMs, interactionTimeoutMs=interactionTimeoutMs) + future, eventLoop, device.deviceProxy, attrs, timedRequestTimeoutMs=timedRequestTimeoutMs, interactionTimeoutMs=interactionTimeoutMs) if res != 0: raise self._ChipStack.ErrorToException(res) return await future @@ -920,7 +946,7 @@ async def Read(self, nodeid: int, attributes: typing.List[typing.Union[ eventPaths = [self._parseEventPathTuple( v) for v in events] if events else None - res = ClusterAttribute.Read(future=future, eventLoop=eventLoop, device=device, devCtrl=self, attributes=attributePaths, dataVersionFilters=clusterDataVersionFilters, events=eventPaths, returnClusterObject=returnClusterObject, + res = ClusterAttribute.Read(future=future, eventLoop=eventLoop, device=device.deviceProxy, devCtrl=self, attributes=attributePaths, dataVersionFilters=clusterDataVersionFilters, events=eventPaths, returnClusterObject=returnClusterObject, subscriptionParameters=ClusterAttribute.SubscriptionParameters(reportInterval[0], reportInterval[1]) if reportInterval else None, fabricFiltered=fabricFiltered, keepSubscriptions=keepSubscriptions) if res != 0: raise self._ChipStack.ErrorToException(res) @@ -1205,6 +1231,10 @@ def _InitLib(self): c_void_p, c_uint64, _DeviceAvailableFunct] self._dmLib.pychip_GetConnectedDeviceByNodeId.restype = c_uint32 + self._dmLib.pychip_FreeOperationalDeviceProxy.argtypes = [ + c_void_p] + self._dmLib.pychip_FreeOperationalDeviceProxy.restype = c_uint32 + self._dmLib.pychip_GetDeviceBeingCommissioned.argtypes = [ c_void_p, c_uint64, c_void_p] self._dmLib.pychip_GetDeviceBeingCommissioned.restype = c_uint32 diff --git a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h index 07e83c7a059419..9bc96cca3cd262 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h +++ b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h @@ -64,7 +64,7 @@ class MTRDeviceConnectionBridge : public chip::ReferenceCounted mOnConnected; chip::Callback::Callback mOnConnectFailed; - static void OnConnected(void * context, chip::OperationalDeviceProxy * device); + static void OnConnected(void * context, chip::Messaging::ExchangeManager & exchangeMgr, chip::SessionHandle & sessionHandle); static void OnConnectionFailure(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error); }; diff --git a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm index 6e9550c8ab0ec0..f61aeeefe493cd 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm @@ -19,10 +19,11 @@ #import "MTRBaseDevice_Internal.h" #import "MTRError_Internal.h" -void MTRDeviceConnectionBridge::OnConnected(void * context, chip::OperationalDeviceProxy * device) +void MTRDeviceConnectionBridge::OnConnected( + void * context, chip::Messaging::ExchangeManager & exchangeMgr, chip::SessionHandle & sessionHandle) { auto * object = static_cast(context); - object->mCompletionHandler(device->GetExchangeManager(), device->GetSecureSession(), nil); + object->mCompletionHandler(&exchangeMgr, chip::MakeOptional(*sessionHandle->AsSecureSession()), nil); object->Release(); } diff --git a/src/lib/support/logging/CHIPLogging.cpp b/src/lib/support/logging/CHIPLogging.cpp index b92e04d7325d2e..a374893d9197ed 100644 --- a/src/lib/support/logging/CHIPLogging.cpp +++ b/src/lib/support/logging/CHIPLogging.cpp @@ -115,7 +115,7 @@ const char ModuleNames[] = "-\0\0" // None "DIS" // Discovery "IM\0" // InteractionModel "TST" // Test - "ODP" // OperationalDeviceProxy + "OSS" // OperationalSessionSetup "ATM" // Automation "CSM" // CASESessionManager ; diff --git a/src/lib/support/logging/Constants.h b/src/lib/support/logging/Constants.h index 37cc149baba661..c91cf1af5da34a 100644 --- a/src/lib/support/logging/Constants.h +++ b/src/lib/support/logging/Constants.h @@ -56,7 +56,7 @@ enum LogModule kLogModule_Discovery, kLogModule_InteractionModel, kLogModule_Test, - kLogModule_OperationalDeviceProxy, + kLogModule_OperationalSessionSetup, kLogModule_Automation, kLogModule_CASESessionManager, diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index a690ec8d1aaede..5fa99e0e677e91 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -445,6 +445,22 @@ void SessionManager::ExpireAllPASESessions() }); } +bool SessionManager::MarkSessionsAsDefunct(const ScopedNodeId & node, const Optional & type) +{ + bool found = false; + mSecureSessions.ForEachSession([&node, &type, &found](auto session) { + if (session->IsActiveSession() && session->GetPeer() == node && + (!type.HasValue() || type.Value() == session->GetSecureSessionType())) + { + session->AsSecureSession()->MarkAsDefunct(); + found = true; + } + return Loop::Continue; + }); + + return found; +} + Optional SessionManager::AllocateSession(SecureSession::Type secureSessionType, const ScopedNodeId & sessionEvictionHint) { diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 2876328bf95382..9454555d28832a 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -349,6 +349,19 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate, public FabricTabl void ExpireAllPASESessions(); + /** + * @brief + * Marks all active sessions that match provided arguments as defunct. + * + * @param node Scoped node ID of the active sessions we should mark as defunct. + * @param type Type of session we are looking to mark as defunct. If matching + * against all types of sessions is desired, NullOptional should + * be passed into type. + * @return True, if at least one session was marked as defunct, otherwise + * return is False. + */ + bool MarkSessionsAsDefunct(const ScopedNodeId & node, const Optional & type); + /** * @brief * Return the System Layer pointer used by current SessionManager.