From d9974b05ebfd74aaccf1a6f6e1d40cebc8ebd143 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Tue, 3 Sep 2024 11:25:25 +1200 Subject: [PATCH] Use IntrusiveList and other minor tweaks --- .../network-commissioning.cpp | 94 +++++++------------ .../network-commissioning.h | 24 +++-- src/include/platform/NetworkCommissioning.h | 14 +-- 3 files changed, 56 insertions(+), 76 deletions(-) diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index e17001d9d53f61..88ebc126031b1c 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -336,6 +336,10 @@ CHIP_ERROR ThreadScanResponseToTLV::EncodeTo(TLV::TLVWriter & writer, TLV::Tag t } // namespace +#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION +IntrusiveList Instance::sInstances; +#endif + Instance::Instance(EndpointId aEndpointId, WiFiDriver * apDelegate) : CommandHandlerInterface(Optional(aEndpointId), Id), AttributeAccessInterface(Optional(aEndpointId), Id), mEndpointId(aEndpointId), mFeatureFlags(WiFiFeatures(apDelegate)), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate) @@ -365,29 +369,18 @@ CHIP_ERROR Instance::Init() mLastNetworkingStatusValue.SetNull(); mLastConnectErrorValue.SetNull(); mLastNetworkIDLen = 0; - RecordNetwork(this); +#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + sInstances.PushBack(this); +#endif return CHIP_NO_ERROR; } void Instance::Shutdown() { +#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + sInstances.Remove(this); +#endif mpBaseDriver->Shutdown(); - - Instance * prev = nullptr; - Instance * curr = sInstanceRecord; - while (curr && curr != this) - { - prev = curr; - curr = curr->mNext; - } - if (!prev) - { - sInstanceRecord = curr->mNext; - } - else - { - prev->mNext = curr->mNext; - } } #if !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION @@ -1048,7 +1041,16 @@ void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::Connec mCurrentOperationBreadcrumb = req.breadcrumb; #if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION - DisconnectOtherNetworks(this); + // Per spec, lingering connections on any other interfaces need to be disconnected at this point. + for (auto & node : sInstances) + { + Instance * instance = static_cast(&node); + if (instance != this) + { + instance->DisconnectLingeringConnection(); + } + } + mpWirelessDriver->ConnectNetwork(req.networkID, this); #else // In Non-concurrent mode postpone the final execution of ConnectNetwork until the operational @@ -1168,54 +1170,24 @@ void Instance::HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryId } #endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC -Instance * Instance::sInstanceRecord = nullptr; - -void Instance::RecordNetwork(Instance * currentNetwork) +void Instance::DisconnectLingeringConnection() { - Instance * curr = sInstanceRecord; - while (curr) - { - if (curr == currentNetwork) + bool haveConnectedNetwork = false; + EnumerateAndRelease(mpBaseDriver->GetNetworks(), [&](const Network & network) { + if (network.connected) { - return; + haveConnectedNetwork = true; + return Loop::Break; } - curr = curr->mNext; - } + return Loop::Continue; + }); - currentNetwork->mNext = sInstanceRecord; - sInstanceRecord = currentNetwork; -} - -void Instance::DisconnectOtherNetworks(Instance * currentNetwork) -{ - Instance * curr = sInstanceRecord; - while (curr) + // If none of the configured networks is `connected`, we may have a + // lingering connection to a different network that we need to disconnect. + // Note: The driver may or may not be actually connected + if (!haveConnectedNetwork) { - if (curr != currentNetwork && curr->mpWirelessDriver) - { - bool haveConnectedNetwork = false; - EnumerateAndRelease(curr->mpBaseDriver->GetNetworks(), [&](const Network & network) { - if (network.connected) - { - haveConnectedNetwork = true; - return Loop::Break; - } - return Loop::Continue; - }); - - // If none of the configured networks is `connected`, we may have a - // lingering connection to a different network that we need to disconnect. - if (!haveConnectedNetwork) - { - /* It's difficult to check the media driver connection state here. - * Check the `Connected` flag in `NetworkInfoStruct` in stead, if not set, - * exec `DisconnectFromNetwork` for driver. However, the physical media connection - * may not necessarily exist in this case, such as after initialization. - */ - ReturnOnFailure(curr->mpWirelessDriver->DisconnectFromNetwork()); - } - } - curr = curr->mNext; + LogErrorOnFailure(mpWirelessDriver->DisconnectFromNetwork()); } } diff --git a/src/app/clusters/network-commissioning/network-commissioning.h b/src/app/clusters/network-commissioning/network-commissioning.h index a4947927c73ec0..5e3d6d7698ff35 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.h +++ b/src/app/clusters/network-commissioning/network-commissioning.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -33,9 +34,17 @@ namespace app { namespace Clusters { namespace NetworkCommissioning { +// Instance inherits privately from this class to participate in Instance::sInstances +class InstanceListNode : public IntrusiveListNodeBase<> +{ +}; + // TODO: Use macro to disable some wifi or thread class Instance : public CommandHandlerInterface, public AttributeAccessInterface, +#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + private InstanceListNode, +#endif public DeviceLayer::NetworkCommissioning::Internal::BaseDriver::NetworkStatusChangeCallback, public DeviceLayer::NetworkCommissioning::Internal::WirelessDriver::ConnectCallback, public DeviceLayer::NetworkCommissioning::WiFiDriver::ScanCallback, @@ -81,14 +90,10 @@ class Instance : public CommandHandlerInterface, void SendNonConcurrentConnectNetworkResponse(); #endif - // NetworkCommissioningInstance records. - static Instance * sInstanceRecord; - // Record current network. - static void RecordNetwork(Instance * currentNetwork); - // Disconnect networks (if connected) in the array other than the current network. - static void DisconnectOtherNetworks(Instance * currentNetwork); - - Instance * mNext; +// TODO: This could be guarded by a separate multi-interface condition instead +#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + static IntrusiveList sInstances; +#endif EndpointId mEndpointId = kInvalidEndpointId; const BitFlags mFeatureFlags; @@ -120,6 +125,9 @@ class Instance : public CommandHandlerInterface, void SetLastNetworkId(ByteSpan lastNetworkId); void ReportNetworksListChanged() const; + // Disconnect if the current connection is not in the Networks list + void DisconnectLingeringConnection(); + // Commits the breadcrumb value saved in mCurrentOperationBreadcrumb to the breadcrumb attribute in GeneralCommissioning // cluster. Will set mCurrentOperationBreadcrumb to NullOptional. void CommitSavedBreadcrumb(); diff --git a/src/include/platform/NetworkCommissioning.h b/src/include/platform/NetworkCommissioning.h index 28755855575b75..a6e145cd6f7faf 100644 --- a/src/include/platform/NetworkCommissioning.h +++ b/src/include/platform/NetworkCommissioning.h @@ -218,13 +218,6 @@ class WirelessDriver : public Internal::BaseDriver */ virtual CHIP_ERROR RevertConfiguration() = 0; -#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION - /** - * @brief Disconnect from network, but maybe it is not connected on media driver. - */ - virtual CHIP_ERROR DisconnectFromNetwork() { return CHIP_ERROR_NOT_IMPLEMENTED; } -#endif - virtual uint8_t GetScanNetworkTimeoutSeconds() = 0; virtual uint8_t GetConnectNetworkTimeoutSeconds() = 0; @@ -254,6 +247,13 @@ class WirelessDriver : public Internal::BaseDriver * called inside ConnectNetwork. */ virtual void ConnectNetwork(ByteSpan networkId, ConnectCallback * callback) = 0; + +#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + /** + * @brief Disconnect from network, if currently connected. + */ + virtual CHIP_ERROR DisconnectFromNetwork() { return CHIP_ERROR_NOT_IMPLEMENTED; } +#endif }; } // namespace Internal