From 5629590e2ee28373ea2bb728fc562142d80ada1d Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Mon, 15 Aug 2022 19:23:32 -0400 Subject: [PATCH] Remove methods from DeviceController that are no longer valid (#21886) With the refactor for OperationalSessionSetup, there are methods in DeviceController that we can now permanently remove. --- src/app/CASESessionManager.cpp | 5 -- src/app/CASESessionManager.h | 6 +- src/app/OperationalSessionSetup.cpp | 63 ++++------------ src/app/OperationalSessionSetup.h | 60 +++++++--------- src/app/clusters/bindings/BindingManager.cpp | 2 - .../operational-credentials-server.cpp | 12 ---- .../ota-requestor/DefaultOTARequestor.cpp | 13 +++- .../ota-requestor/DefaultOTARequestor.h | 1 + src/controller/CHIPDeviceController.cpp | 72 ++----------------- src/controller/CHIPDeviceController.h | 39 ---------- .../java/CHIPDeviceController-JNI.cpp | 23 ------ .../ChipDeviceController.java | 6 -- .../ChipDeviceController-ScriptBinding.cpp | 28 +++----- .../Framework/CHIP/MTRDeviceController.mm | 7 +- src/transport/SessionManager.cpp | 8 +-- src/transport/SessionManager.h | 4 +- 16 files changed, 72 insertions(+), 277 deletions(-) diff --git a/src/app/CASESessionManager.cpp b/src/app/CASESessionManager.cpp index 7e38ccd583e974..93787a9040bdf5 100644 --- a/src/app/CASESessionManager.cpp +++ b/src/app/CASESessionManager.cpp @@ -56,11 +56,6 @@ void CASESessionManager::FindOrEstablishSession(const ScopedNodeId & peerId, Cal session->Connect(onConnection, onFailure); } -void CASESessionManager::ReleaseSession(const ScopedNodeId & peerId) -{ - ReleaseSession(FindExistingSessionSetup(peerId)); -} - void CASESessionManager::ReleaseSessionsForFabric(FabricIndex fabricIndex) { mConfig.sessionSetupPool->ReleaseAllSessionSetupsForFabric(fabricIndex); diff --git a/src/app/CASESessionManager.h b/src/app/CASESessionManager.h index d5243a930d2bbe..c8c252ee815749 100644 --- a/src/app/CASESessionManager.h +++ b/src/app/CASESessionManager.h @@ -78,10 +78,6 @@ class CASESessionManager : public OperationalSessionReleaseDelegate, public Sess void FindOrEstablishSession(const ScopedNodeId & peerId, Callback::Callback * onConnection, Callback::Callback * onFailure); - OperationalSessionSetup * FindExistingSessionSetup(const ScopedNodeId & peerId, bool forAddressUpdate = false) const; - - void ReleaseSession(const ScopedNodeId & peerId); - void ReleaseSessionsForFabric(FabricIndex fabricIndex); void ReleaseAllSessions(); @@ -103,6 +99,8 @@ class CASESessionManager : public OperationalSessionReleaseDelegate, public Sess void UpdatePeerAddress(ScopedNodeId peerId) override; private: + OperationalSessionSetup * FindExistingSessionSetup(const ScopedNodeId & peerId, bool forAddressUpdate = false) const; + Optional FindExistingSession(const ScopedNodeId & peerId) const; CASESessionManagerConfig mConfig; diff --git a/src/app/OperationalSessionSetup.cpp b/src/app/OperationalSessionSetup.cpp index 8cafbe9697e59d..11ceb8bb6c0e26 100644 --- a/src/app/OperationalSessionSetup.cpp +++ b/src/app/OperationalSessionSetup.cpp @@ -261,20 +261,14 @@ void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error) mConnectionFailure.DequeueAll(failureReady); mConnectionSuccess.DequeueAll(successReady); - // TODO Issue #20452: For now we need to make a copy of member fields inside `this` before calling any of the - // callbacks since the callbacks themselves can potentially release `this` OperationalSessionSetup. - auto * exchangeMgr = mInitParams.exchangeMgr; - auto sessionHandle = mSecureSession.Get(); - auto peerId = mPeerId; - VerifyOrDie(mReleaseDelegate != nullptr); - auto * releaseDelegate = mReleaseDelegate; - // // If we encountered no error, go ahead and call all success callbacks. Otherwise, // call the failure callbacks. // while (failureReady.mNext != &failureReady) { + // We expect that we only have callbacks if we are not performing just address update. + VerifyOrDie(!mPerformingAddressUpdate); Callback::Callback * cb = Callback::Callback::FromCancelable(failureReady.mNext); @@ -282,24 +276,28 @@ void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error) if (error != CHIP_NO_ERROR) { - cb->mCall(cb->mContext, peerId, error); + cb->mCall(cb->mContext, mPeerId, error); } } while (successReady.mNext != &successReady) { + // We expect that we only have callbacks if we are not performing just address update. + VerifyOrDie(!mPerformingAddressUpdate); Callback::Callback * cb = Callback::Callback::FromCancelable(successReady.mNext); cb->Cancel(); if (error == CHIP_NO_ERROR) { - // We know that we for sure have the SessionHandle in the successful case. + auto * exchangeMgr = mInitParams.exchangeMgr; VerifyOrDie(exchangeMgr); - cb->mCall(cb->mContext, *exchangeMgr, sessionHandle.Value()); + // We know that we for sure have the SessionHandle in the successful case. + auto optionalSessionHandle = mSecureSession.Get(); + cb->mCall(cb->mContext, *exchangeMgr, optionalSessionHandle.Value()); } } - - releaseDelegate->ReleaseSession(this); + VerifyOrDie(mReleaseDelegate != nullptr); + mReleaseDelegate->ReleaseSession(this); } void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error) @@ -307,13 +305,6 @@ void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error) VerifyOrReturn(mState != State::Uninitialized && mState != State::NeedsAddress, ChipLogError(Controller, "HandleCASEConnectionFailure was called while the device was not initialized")); - // - // We don't need to reset the state all the way back to NeedsAddress since all that transpired - // was just CASE connection failure. So let's re-use the cached address to re-do CASE again - // if need-be. - // - MoveToState(State::HasAddress); - DequeueConnectionCallbacks(error); // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } @@ -332,26 +323,6 @@ void OperationalSessionSetup::OnSessionEstablished(const SessionHandle & session // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } -void OperationalSessionSetup::Disconnect() -{ - VerifyOrReturn(mState == State::SecureConnected); - - if (mSecureSession) - { - // - // Mark the session as defunct to signal that we no longer want to use this - // session anymore for further interactions to this peer. However, if we receive - // messages back from that peer on the defunct session, it will bring it back into an active - // state again. - // - mSecureSession.Get().Value()->AsSecureSession()->MarkAsDefunct(); - } - - mSecureSession.Release(); - - MoveToState(State::HasAddress); -} - void OperationalSessionSetup::CleanupCASEClient() { if (mCASEClient) @@ -366,11 +337,6 @@ void OperationalSessionSetup::OnSessionReleased() MoveToState(State::HasAddress); } -void OperationalSessionSetup::OnSessionHang() -{ - Disconnect(); -} - OperationalSessionSetup::~OperationalSessionSetup() { if (mAddressLookupHandle.IsActive()) @@ -456,12 +422,7 @@ void OperationalSessionSetup::OnNodeAddressResolutionFailed(const PeerId & peerI ChipLogError(Discovery, "OperationalSessionSetup[%u:" ChipLogFormatX64 "]: operational discovery failed: %" CHIP_ERROR_FORMAT, mPeerId.GetFabricIndex(), ChipLogValueX64(mPeerId.GetNodeId()), reason.Format()); - if (IsResolvingAddress()) - { - MoveToState(State::NeedsAddress); - } - - // No need to set mPerformingAddressUpdate to false since call below releases `this`. + // No need to modify any variables in `this` since call below releases `this`. DequeueConnectionCallbacks(reason); // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } diff --git a/src/app/OperationalSessionSetup.h b/src/app/OperationalSessionSetup.h index ccea4843457b0d..d0db84f6c69ce1 100644 --- a/src/app/OperationalSessionSetup.h +++ b/src/app/OperationalSessionSetup.h @@ -140,19 +140,29 @@ class OperationalDeviceProxy : public DeviceProxy * 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); /** - * Represents a connection path to a device that is in an operational state. + * Object used to either establish a connection to peer or performing address lookup to a peer. * - * Handles the lifetime of communicating with such a device: - * - Discover the device using DNSSD (find out what IP address to use and what - * communication parameters are appropriate for it) - * - Establish a secure channel to it via CASE - * - Expose to consumers the secure session for talking to the device. + * OperationalSessionSetup is capable of either: + * 1. Establishing a CASE session connection to a peer. Upon success or failure, the OnDeviceConnected or + * OnDeviceConnectionFailure callback will be called to notify the caller the results of trying to + * estblish a CASE session. Some additional details about the steps to establish a connection are: + * - Discover the device using DNSSD (find out what IP address to use and what + * communication parameters are appropriate for it) + * - Establish a secure channel to it via CASE + * - Expose to consumers the secure session for talking to the device via OnDeviceConnected + * callback. + * 2. Performing an address lookup for given a scoped nodeid. On success, it will call into + * SessionManager to update the addresses for all matching sessions in the session table. + * + * OperationalSessionSetup has a very limited lifetime. Once it has completed its purpose outlined above, + * it will use `releaseDelegate` to release itself. + * + * It is possible to determine which of the two purposes the OperationalSessionSetup is for by calling + * IsForAddressUpdate(). */ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, public SessionEstablishmentDelegate, @@ -198,21 +208,10 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, */ void Connect(Callback::Callback * onConnection, Callback::Callback * onFailure); - bool IsConnected() const { return mState == State::SecureConnected; } - bool IsConnecting() const { return mState == State::Connecting; } bool IsForAddressUpdate() const { return mPerformingAddressUpdate; } - /** - * IsResolvingAddress returns true if we are doing an address resolution - * that needs to happen before we can establish CASE. We can be in the - * middle of doing address updates at other times too (e.g. when we are - * IsConnected()), but those will not cause a true return from - * IsResolvingAddress(). - */ - bool IsResolvingAddress() const { return mState == State::ResolvingAddress; } - //////////// SessionEstablishmentDelegate Implementation /////////////// void OnSessionEstablished(const SessionHandle & session) override; void OnSessionEstablishmentError(CHIP_ERROR error) override; @@ -221,19 +220,9 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, // Called when a connection is closing. The object releases all resources associated with the connection. void OnSessionReleased() override; - // Called when a connection is hanging. Try to re-establish another session, and shift to the new session when done, the - // original session won't be touched during the period. - void OnSessionHang() override; - - /** - * Mark any open session with the device as expired. - */ - void Disconnect(); ScopedNodeId GetPeerId() const { return mPeerId; } - Transport::PeerAddress GetPeerAddress() const { return mDeviceAddress; } - static Transport::PeerAddress ToPeerAddress(const Dnssd::ResolvedNodeData & nodeData) { Inet::InterfaceId interfaceId = Inet::InterfaceId::Null(); @@ -256,11 +245,6 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, */ FabricIndex GetFabricIndex() const { return mPeerId.GetFabricIndex(); } - /** - * Triggers a DNSSD lookup to find a usable peer address for this operational device. - */ - CHIP_ERROR LookupPeerAddress(); - void PerformAddressUpdate(); // AddressResolve::NodeListener - notifications when dnssd finds a node IP address @@ -331,9 +315,17 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, * If error == CHIP_NO_ERROR, only success callbacks are invoked. * Otherwise, only failure callbacks are invoked. * + * This uses mReleaseDelegate to release ourselves (aka `this`). As a + * result any caller should return right away without touching `this`. + * */ void DequeueConnectionCallbacks(CHIP_ERROR error); + /** + * Triggers a DNSSD lookup to find a usable peer address. + */ + CHIP_ERROR LookupPeerAddress(); + /** * This function will set new IP address, port and MRP retransmission intervals of the device. */ diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index d373e8c15a3f13..db07abe3ed9f34 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -111,7 +111,6 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) if (mPendingNotificationMap.FindLRUConnectPeer(peerToRemove) == CHIP_NO_ERROR) { mPendingNotificationMap.RemoveAllEntriesForNode(peerToRemove); - mInitParams.mCASESessionManager->ReleaseSession(peerToRemove); // Now retry mLastSessionEstablishmentError = CHIP_NO_ERROR; @@ -160,7 +159,6 @@ void BindingManager::HandleDeviceConnectionFailure(const ScopedNodeId & peerId, { // Simply release the entry, the connection will be re-established as needed. ChipLogError(AppServer, "Failed to establish connection to node 0x" ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId())); - mInitParams.mCASESessionManager->ReleaseSession(peerId); mLastSessionEstablishmentError = error; } diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index ae60c18f70fe4f..baf6a97877df81 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -296,18 +296,6 @@ void FailSafeCleanup(const chip::DeviceLayer::ChipDeviceEvent * event) // Session Context at the Server. if (event->FailSafeTimerExpired.addNocCommandHasBeenInvoked || event->FailSafeTimerExpired.updateNocCommandHasBeenInvoked) { - // TODO(#19259): The following scope will no longer need to exist after #19259 is fixed - { - CASESessionManager * caseSessionManager = Server::GetInstance().GetCASESessionManager(); - if (caseSessionManager) - { - const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex); - VerifyOrReturn(fabricInfo != nullptr); - - caseSessionManager->ReleaseSessionsForFabric(fabricInfo->GetFabricIndex()); - } - } - SessionManager & sessionMgr = Server::GetInstance().GetSecureSessionManager(); CleanupSessionsForFabric(sessionMgr, fabricIndex); } diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp b/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp index 98f11b3fee0811..f31bb2e0c5408a 100644 --- a/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp +++ b/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp @@ -386,8 +386,15 @@ void DefaultOTARequestor::DisconnectFromProvider() return; } - auto providerNodeId = GetProviderScopedId(); - mServer->GetSecureSessionManager().MarkSessionsAsDefunct(providerNodeId, MakeOptional(Transport::SecureSession::Type::kCASE)); + auto optionalSessionHandle = mSessionHolder.Get(); + if (optionalSessionHandle.HasValue()) + { + if (optionalSessionHandle.Value()->IsActiveSession()) + { + optionalSessionHandle.Value()->AsSecureSession()->MarkAsDefunct(); + } + } + mSessionHolder.Release(); } // Requestor is directed to cancel image update in progress. All the Requestor state is @@ -419,6 +426,7 @@ void DefaultOTARequestor::OnConnected(void * context, Messaging::ExchangeManager { DefaultOTARequestor * requestorCore = static_cast(context); VerifyOrDie(requestorCore != nullptr); + requestorCore->mSessionHolder.Grab(sessionHandle); switch (requestorCore->mOnConnectedAction) { @@ -476,6 +484,7 @@ void DefaultOTARequestor::OnConnectionFailure(void * context, const ScopedNodeId { DefaultOTARequestor * requestorCore = static_cast(context); VerifyOrDie(requestorCore != nullptr); + requestorCore->mSessionHolder.Release(); ChipLogError(SoftwareUpdate, "Failed to connect to node 0x" ChipLogFormatX64 ": %" CHIP_ERROR_FORMAT, ChipLogValueX64(peerId.GetNodeId()), error.Format()); diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestor.h b/src/app/clusters/ota-requestor/DefaultOTARequestor.h index 0dc04efae08f8e..9f9902858c3141 100644 --- a/src/app/clusters/ota-requestor/DefaultOTARequestor.h +++ b/src/app/clusters/ota-requestor/DefaultOTARequestor.h @@ -331,6 +331,7 @@ class DefaultOTARequestor : public OTARequestorInterface, public BDXDownloader:: // persistent storage (if available), used for sending the NotifyApplied message, and then cleared. This will ensure determinism // in the OTARequestorDriver on reboot. Optional mProviderLocation; + SessionHolder mSessionHolder; }; } // namespace chip diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index da29daeac289b9..d409d727f405dd 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -353,41 +353,6 @@ void DeviceController::Shutdown() mDeviceDiscoveryDelegate = nullptr; } -void DeviceController::ReleaseOperationalDevice(NodeId remoteNodeId) -{ - VerifyOrReturn(mState == State::Initialized && mFabricIndex != kUndefinedFabricIndex, - ChipLogError(Controller, "ReleaseOperationalDevice was called in incorrect state")); - mSystemState->CASESessionMgr()->ReleaseSession(GetPeerScopedId(remoteNodeId)); -} - -CHIP_ERROR DeviceController::DisconnectDevice(NodeId nodeId) -{ - ChipLogProgress(Controller, "Force close session for node 0x%" PRIx64, nodeId); - - if (SessionMgr()->MarkSessionsAsDefunct(GetPeerScopedId(nodeId), MakeOptional(Transport::SecureSession::Type::kCASE))) - { - return CHIP_NO_ERROR; - } - - OperationalSessionSetup * proxy = mSystemState->CASESessionMgr()->FindExistingSessionSetup(GetPeerScopedId(nodeId)); - if (proxy == nullptr) - { - ChipLogProgress(Controller, "Attempted to close a session that does not exist."); - return CHIP_NO_ERROR; - } - - if (proxy->IsConnecting()) - { - ChipLogError(Controller, "Attempting to disconnect while connection in progress"); - return CHIP_ERROR_INCORRECT_STATE; - } - - // TODO: logic here is unclear. Possible states are "uninitialized, needs address, initialized" - // and disconnecting in those states is unclear (especially for needds-address). - ChipLogProgress(Controller, "Disconnect attempt while not in connected/connecting state"); - return CHIP_NO_ERROR; -} - CHIP_ERROR DeviceController::GetPeerAddressAndPort(NodeId peerId, Inet::IPAddress & addr, uint16_t & port) { VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); @@ -1664,7 +1629,6 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, const Scope { commissioner->CommissioningStageComplete(error); } - commissioner->mSystemState->CASESessionMgr()->ReleaseSession(peerId); } // ClusterStateCache::Callback impl @@ -2286,13 +2250,10 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } break; case CommissioningStage::kFindOperational: { - CHIP_ERROR err = UpdateDevice(proxy->GetDeviceId()); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Controller, "Unable to proceed to operational discovery\n"); - CommissioningStageComplete(err); - return; - } + // If there is an error, CommissioningStageComplete will be called from OnDeviceConnectionFailureFn. + auto scopedPeerId = GetPeerScopedId(proxy->GetDeviceId()); + mSystemState->CASESessionMgr()->FindOrEstablishSession(scopedPeerId, &mOnDeviceConnectedCallback, + &mOnDeviceConnectionFailureCallback); } break; case CommissioningStage::kSendComplete: { @@ -2312,31 +2273,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } } -CHIP_ERROR DeviceController::UpdateDevice(NodeId peerNodeId) -{ - VerifyOrReturnError(mState == State::Initialized && mFabricIndex != kUndefinedFabricIndex, CHIP_ERROR_INCORRECT_STATE); - - OperationalSessionSetup * sessionSetup = GetDeviceSession(GetPeerScopedId(peerNodeId)); - VerifyOrReturnError(sessionSetup != nullptr, CHIP_ERROR_NOT_FOUND); - - return sessionSetup->LookupPeerAddress(); -} - -OperationalSessionSetup * DeviceController::GetDeviceSession(const ScopedNodeId & peerId) -{ - return mSystemState->CASESessionMgr()->FindExistingSessionSetup(peerId); -} - -OperationalSessionSetup * DeviceCommissioner::GetDeviceSession(const ScopedNodeId & peerId) -{ - mSystemState->CASESessionMgr()->FindOrEstablishSession(peerId, &mOnDeviceConnectedCallback, - &mOnDeviceConnectionFailureCallback); - - // If there is an OperationalSessionSetup for this peerId now the call to the - // superclass will return it. - return DeviceController::GetDeviceSession(peerId); -} - CHIP_ERROR DeviceController::GetCompressedFabricIdBytes(MutableByteSpan & outBytes) const { const auto * fabricInfo = GetFabricInfo(); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 2f9734816cae4e..9e3f77cc90bb46 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -214,14 +214,6 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController return CHIP_NO_ERROR; } - /** - * DEPRECATED - to be removed - * - * Forces a DNSSD lookup for the specified device. It finds the corresponding session - * for the given peerNodeId and initiates a DNSSD lookup to find/update the node address - */ - CHIP_ERROR UpdateDevice(NodeId peerNodeId); - /** * @brief * Compute a PASE verifier and passcode ID for the desired setup pincode. @@ -301,29 +293,8 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController return mSystemState->Fabrics(); } - // TODO(#20452): This should be removed/renamed once #20452 is fixed - void ReleaseOperationalDevice(NodeId remoteNodeId); - OperationalCredentialsDelegate * GetOperationalCredentialsDelegate() { return mOperationalCredentialsDelegate; } - /** - * TODO(#20452): This needs to be refactored to reflect what it actually does (which is not disconnecting anything) - * - * TEMPORARY - DO NOT USE or if you use please request review on why/how to - * officially support such an API. - * - * This was added to support the 'reuse session' logic in cirque integration - * tests however since that is the only client, the correct update is to - * use 'ConnectDevice' and wait for connect success/failure inside the CI - * logic. The current code does not do that because python was not set up - * to wait for timeouts on success/fail, hence this temporary method. - * - * TODO(andy31415): update cirque test and remove this method. - * - * Returns success if a session with the given peer does not exist yet. - */ - CHIP_ERROR DisconnectDevice(NodeId nodeId); - /** * @brief * Reconfigures a new set of operational credentials to be used with this @@ -369,14 +340,7 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController chip::VendorId mVendorId; - /// 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 OperationalSessionSetup * GetDeviceSession(const ScopedNodeId & peerId); - DiscoveredNodeList GetDiscoveredNodes() override { return DiscoveredNodeList(mCommissionableNodes); } - -private: - void ReleaseOperationalDevice(OperationalSessionSetup * device); }; #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY @@ -660,9 +624,6 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, // ClusterStateCache::Callback impl void OnDone(app::ReadClient *) override; - // Commissioner will establish new device connections after PASE. - OperationalSessionSetup * GetDeviceSession(const ScopedNodeId & peerId) override; - // Issue an NOC chain using the associated OperationalCredentialsDelegate. The NOC chain will // be provided in X509 DER format. // NOTE: This is only valid assuming that `mOperationalCredentialsDelegate` is what is desired diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index 21f9adcc1a753f..724c22b41a1706 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -681,15 +681,6 @@ JNI_METHOD(void, releaseOperationalDevicePointer)(JNIEnv * env, jobject self, jl } } -JNI_METHOD(void, disconnectDevice)(JNIEnv * env, jobject self, jlong handle, jlong deviceId) -{ - chip::DeviceLayer::StackLock lock; - AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle); - - ChipLogProgress(Controller, "disconnectDevice() called with deviceId"); - wrapper->Controller()->ReleaseOperationalDevice(deviceId); -} - JNI_METHOD(void, shutdownSubscriptions)(JNIEnv * env, jobject self, jlong handle, jlong devicePtr) { chip::DeviceLayer::StackLock lock; @@ -787,20 +778,6 @@ JNI_METHOD(jlong, getCompressedFabricId)(JNIEnv * env, jobject self, jlong handl return wrapper->Controller()->GetCompressedFabricId(); } -JNI_METHOD(void, updateDevice)(JNIEnv * env, jobject self, jlong handle, jlong fabricId, jlong deviceId) -{ - chip::DeviceLayer::StackLock lock; - - AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle); - - CHIP_ERROR err = wrapper->Controller()->UpdateDevice(static_cast(deviceId)); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Controller, "Failed to update device"); - JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err); - } -} - JNI_METHOD(void, discoverCommissionableNodes)(JNIEnv * env, jobject self, jlong handle) { 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 a57e772ba7234c..dbc0c54d53ab18 100644 --- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java +++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java @@ -262,10 +262,6 @@ public void releaseConnectedDevicePointer(long devicePtr) { releaseOperationalDevicePointer(devicePtr); } - public boolean disconnectDevice(long deviceId) { - return disconnectDevice(deviceControllerPtr, deviceId); - } - public void onConnectDeviceComplete() { completionListener.onConnectDeviceComplete(); } @@ -636,8 +632,6 @@ private native void getConnectedDevicePointer( private native void releaseOperationalDevicePointer(long devicePtr); - private native boolean disconnectDevice(long deviceControllerPtr, long deviceId); - private native void deleteDeviceController(long deviceControllerPtr); private native String getIpAddress(long deviceControllerPtr, long deviceId); diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 194004e0dc93fb..7a4189a52680d2 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -378,8 +378,6 @@ ChipError::StorageType pychip_DeviceController_ConnectIP(chip::Controller::Devic addr.SetTransportType(chip::Transport::Type::kUdp).SetIPAddress(peerAddr); params.SetPeerAddress(addr).SetDiscriminator(0); - devCtrl->ReleaseOperationalDevice(nodeid); - return devCtrl->PairDevice(nodeid, params, sCommissioningParameters).AsInteger(); } @@ -455,28 +453,19 @@ ChipError::StorageType pychip_DeviceController_SetWiFiCredentials(const char * s ChipError::StorageType pychip_DeviceController_CloseSession(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid) { -#if 0 // // Since we permit multiple controllers per fabric and each is associated with a unique fabric index, closing a session // requires us to do so across all controllers on the same logical fabric. // - // TODO: Enable this and remove the call below to DisconnectDevice once #19259 is completed. This is because - // OperationalDeviceProxy instances that are currently active will remain un-affected by this call and still - // provide a valid SessionHandle in the OnDeviceConnected call later when we call DeviceController::GetConnectedDevice. - // However, it provides a SessionHandle that is incapable of actually vending exchanges since it is in a defunct state. - // - // For now, calling DisconnectDevice will at least just correctly de-activate a currently active OperationalDeviceProxy - // instance and ensure that subsequent attempts to acquire one will correctly re-establish CASE on the fabric associated - // with the provided devCtrl. - // - auto err = devCtrl->SessionMgr()->ForEachCollidingSession(ScopedNodeId(nodeid, devCtrl->GetFabricIndex()), [](auto *session) { - session->MarkAsDefunct(); - }); + devCtrl->SessionMgr()->ForEachMatchingSessionOnLogicalFabric(ScopedNodeId(nodeid, devCtrl->GetFabricIndex()), + [](auto * session) { + if (session->IsActiveSession()) + { + session->MarkAsDefunct(); + } + }); - ReturnErrorOnFailure(err.AsInteger()); -#else - return devCtrl->DisconnectDevice(nodeid).AsInteger(); -#endif + return CHIP_NO_ERROR.AsInteger(); } ChipError::StorageType pychip_DeviceController_EstablishPASESessionIP(chip::Controller::DeviceCommissioner * devCtrl, @@ -724,7 +713,6 @@ ChipError::StorageType pychip_GetDeviceBeingCommissioned(chip::Controller::Devic ChipError::StorageType pychip_ExpireSessions(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId) { VerifyOrReturnError((devCtrl != nullptr) && (devCtrl->SessionMgr() != nullptr), CHIP_ERROR_INVALID_ARGUMENT.AsInteger()); - (void) devCtrl->ReleaseOperationalDevice(nodeId); // // Since we permit multiple controllers on the same fabric each associated with a different fabric index, expiring a session diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 112fd18fbf1407..425be4e5a1ac26 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -805,8 +805,11 @@ - (void)invalidateCASESessionForNode:(chip::NodeId)nodeID; return; } - // TODO: This is a hack and needs to go away or use some sane API. - self->_cppCommissioner->DisconnectDevice(nodeID); + auto sessionMgr = self->_cppCommissioner->SessionMgr(); + VerifyOrDie(sessionMgr != nullptr); + + sessionMgr->MarkSessionsAsDefunct( + self->_cppCommissioner->GetPeerScopedId(nodeID), chip::MakeOptional(chip::Transport::SecureSession::Type::kCASE)); }); } @end diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index e388f13ea23e9c..a0ad834b033027 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -445,20 +445,16 @@ void SessionManager::ExpireAllPASESessions() }); } -bool SessionManager::MarkSessionsAsDefunct(const ScopedNodeId & node, const Optional & type) +void SessionManager::MarkSessionsAsDefunct(const ScopedNodeId & node, const Optional & type) { - bool found = false; - mSecureSessions.ForEachSession([&node, &type, &found](auto session) { + mSecureSessions.ForEachSession([&node, &type](auto session) { if (session->IsActiveSession() && session->GetPeer() == node && (!type.HasValue() || type.Value() == session->GetSecureSessionType())) { session->MarkAsDefunct(); - found = true; } return Loop::Continue; }); - - return found; } void SessionManager::UpdateAllSessionsPeerAddress(const ScopedNodeId & node, const Transport::PeerAddress & addr) diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index c40a8af4dd47f5..4337c84ee49408 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -357,10 +357,8 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate, public FabricTabl * @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); + void MarkSessionsAsDefunct(const ScopedNodeId & node, const Optional & type); /** * @brief