Skip to content

Commit 5559585

Browse files
simonhmorris1bzbarsky-applerestyled-commits
authored andcommitted
Implement Non concurrent mode (#30201)
* Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Correct intermittent missing SSID(#29865) * ConnectNetworkResponse Supression (#29865) * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Fix darwin-tests workflow file syntax. (#30162) There were extra curly braces that failed on some (but not all?) CI runs. * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Only start WiFi on BLE Commissioning pass(#29865) * Remove use of Breadcrumb::Get from BLE (#29865) * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Only start WiFi on BLE Commissioning pass(#29865) * Remove use of Breadcrumb::Get from BLE (#29865) * Tidy comments (#29865) * Default on error, check timeout, tidy up (#29865) * Make log messages user friendly (#29865) * Change to compile time non-concurrent mode(#29865) * Restyled by whitespace * Restyled by clang-format * Improve code style (#29865) * Restyled by clang-format * Add CommandHandler issue reference 30576 (#29865) --------- Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Restyled.io <[email protected]>
1 parent f222ccf commit 5559585

14 files changed

+220
-49
lines changed

examples/platform/linux/AppMain.cpp

+12-5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include <platform/PlatformManager.h>
2121

2222
#include "app/clusters/network-commissioning/network-commissioning.h"
23+
#include <app/server/Dnssd.h>
2324
#include <app/server/OnboardingCodesUtil.h>
2425
#include <app/server/Server.h>
2526
#include <app/util/endpoint-config-api.h>
@@ -238,15 +239,15 @@ void InitNetworkCommissioning()
238239
using chip::Shell::Engine;
239240
#endif
240241

241-
#if CHIP_DEVICE_CONFIG_ENABLE_WPA
242+
#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
242243
/*
243244
* The device shall check every kWiFiStartCheckTimeUsec whether Wi-Fi management
244245
* has been fully initialized. If after kWiFiStartCheckAttempts Wi-Fi management
245246
* still hasn't been initialized, the device configuration is reset, and device
246247
* needs to be paired again.
247248
*/
248-
static constexpr useconds_t kWiFiStartCheckTimeUsec = 100 * 1000; // 100 ms
249-
static constexpr uint8_t kWiFiStartCheckAttempts = 5;
249+
static constexpr useconds_t kWiFiStartCheckTimeUsec = WIFI_START_CHECK_TIME_USEC;
250+
static constexpr uint8_t kWiFiStartCheckAttempts = WIFI_START_CHECK_ATTEMPTS;
250251
#endif
251252

252253
namespace {
@@ -264,6 +265,11 @@ void EventHandler(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg)
264265
{
265266
ChipLogProgress(DeviceLayer, "Receive kCHIPoBLEConnectionEstablished");
266267
}
268+
else if ((event->Type == chip::DeviceLayer::DeviceEventType::kInternetConnectivityChange))
269+
{
270+
// Restart the server on connectivity change
271+
app::DnssdServer::Instance().StartServer();
272+
}
267273
}
268274

269275
void Cleanup()
@@ -298,7 +304,7 @@ void StopSignalHandler(int signal)
298304

299305
} // namespace
300306

301-
#if CHIP_DEVICE_CONFIG_ENABLE_WPA
307+
#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
302308
static bool EnsureWiFiIsStarted()
303309
{
304310
for (int cnt = 0; cnt < kWiFiStartCheckAttempts; cnt++)
@@ -462,9 +468,10 @@ int ChipLinuxAppInit(int argc, char * const argv[], OptionSet * customOptions,
462468
DeviceLayer::ConnectivityMgr().SetBLEAdvertisingEnabled(true);
463469
#endif
464470

465-
#if CHIP_DEVICE_CONFIG_ENABLE_WPA
471+
#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
466472
if (LinuxDeviceOptions::GetInstance().mWiFi)
467473
{
474+
// Start WiFi management in Concurrent mode
468475
DeviceLayer::ConnectivityMgrImpl().StartWiFiManagement();
469476
if (!EnsureWiFiIsStarted())
470477
{

src/app/clusters/network-commissioning/network-commissioning.cpp

+31-1
Original file line numberDiff line numberDiff line change
@@ -171,11 +171,18 @@ void Instance::InvokeCommand(HandlerContext & ctxt)
171171
ctxt, [this](HandlerContext & ctx, const auto & req) { HandleRemoveNetwork(ctx, req); });
172172
return;
173173

174-
case Commands::ConnectNetwork::Id:
174+
case Commands::ConnectNetwork::Id: {
175175
VerifyOrReturn(mFeatureFlags.Has(Feature::kWiFiNetworkInterface) || mFeatureFlags.Has(Feature::kThreadNetworkInterface));
176+
#if CONFIG_NETWORK_LAYER_BLE && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
177+
// If commissionee does not support Concurrent Connections, request the BLE to be stopped.
178+
// Start the ConnectNetwork, but this will not complete until the BLE is off.
179+
ChipLogProgress(NetworkProvisioning, "Closing BLE connections due to non-concurrent mode");
180+
DeviceLayer::DeviceControlServer::DeviceControlSvr().PostCloseAllBLEConnectionsToOperationalNetworkEvent();
181+
#endif
176182
HandleCommand<Commands::ConnectNetwork::DecodableType>(
177183
ctxt, [this](HandlerContext & ctx, const auto & req) { HandleConnectNetwork(ctx, req); });
178184
return;
185+
}
179186

180187
case Commands::ReorderNetwork::Id:
181188
VerifyOrReturn(mFeatureFlags.Has(Feature::kWiFiNetworkInterface) || mFeatureFlags.Has(Feature::kThreadNetworkInterface));
@@ -623,7 +630,20 @@ void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::Connec
623630
memcpy(mConnectingNetworkID, req.networkID.data(), mConnectingNetworkIDLen);
624631
mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler);
625632
mCurrentOperationBreadcrumb = req.breadcrumb;
633+
634+
// In Non-concurrent mode postpone the final execution of ConnectNetwork until the operational
635+
// network has been fully brought up and kWiFiDeviceAvailable is delivered.
636+
// mConnectingNetworkIDLen and mConnectingNetworkID contains the received SSID
637+
#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
626638
mpWirelessDriver->ConnectNetwork(req.networkID, this);
639+
#endif
640+
}
641+
642+
void Instance::HandleNonConcurrentConnectNetwork()
643+
{
644+
ByteSpan nonConcurrentNetworkID = ByteSpan(mConnectingNetworkID, mConnectingNetworkIDLen);
645+
ChipLogProgress(NetworkProvisioning, "HandleNonConcurrentConnectNetwork() SSID=%s", mConnectingNetworkID);
646+
mpWirelessDriver->ConnectNetwork(nonConcurrentNetworkID, this);
627647
}
628648

629649
void Instance::HandleReorderNetwork(HandlerContext & ctx, const Commands::ReorderNetwork::DecodableType & req)
@@ -759,7 +779,13 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i
759779
memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen);
760780
mLastNetworkingStatusValue.SetNonNull(commissioningError);
761781

782+
#if CONFIG_NETWORK_LAYER_BLE && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
783+
ChipLogProgress(NetworkProvisioning, "Non-concurrent mode, ConnectNetworkResponse will NOT be sent");
784+
// Do not send the ConnectNetworkResponse if in non-concurrent mode
785+
// Issue #30576 raised to modify CommandHandler to notify it if no response required
786+
#else
762787
commandHandle->AddResponse(mPath, response);
788+
#endif
763789
if (commissioningError == Status::kSuccess)
764790
{
765791
CommitSavedBreadcrumb();
@@ -956,6 +982,10 @@ void Instance::OnPlatformEventHandler(const DeviceLayer::ChipDeviceEvent * event
956982
{
957983
this_->OnFailSafeTimerExpired();
958984
}
985+
else if (event->Type == DeviceLayer::DeviceEventType::kWiFiDeviceAvailable)
986+
{
987+
this_->HandleNonConcurrentConnectNetwork();
988+
}
959989
}
960990

961991
void Instance::OnCommissioningComplete()

src/app/clusters/network-commissioning/network-commissioning.h

+1
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ class Instance : public CommandHandlerInterface,
117117
void HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveNetwork::DecodableType & req);
118118
void HandleConnectNetwork(HandlerContext & ctx, const Commands::ConnectNetwork::DecodableType & req);
119119
void HandleReorderNetwork(HandlerContext & ctx, const Commands::ReorderNetwork::DecodableType & req);
120+
void HandleNonConcurrentConnectNetwork(void);
120121
void HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryIdentity::DecodableType & req);
121122

122123
public:

src/app/server/CommissioningWindowManager.cpp

+9-1
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv
6464
Cleanup();
6565
mServer->GetSecureSessionManager().ExpireAllPASESessions();
6666
// That should have cleared out mPASESession.
67-
#if CONFIG_NETWORK_LAYER_BLE
67+
#if CONFIG_NETWORK_LAYER_BLE && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
68+
// If in NonConcurrentConnection, this will already have been completed
6869
mServer->GetBleLayerObject()->CloseAllBleConnections();
6970
#endif
7071
}
@@ -82,6 +83,13 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv
8283
app::DnssdServer::Instance().AdvertiseOperational();
8384
ChipLogProgress(AppServer, "Operational advertising enabled");
8485
}
86+
#if CONFIG_NETWORK_LAYER_BLE
87+
else if (event->Type == DeviceLayer::DeviceEventType::kCloseAllBleConnections)
88+
{
89+
ChipLogProgress(AppServer, "Received kCloseAllBleConnections");
90+
mServer->GetBleLayerObject()->CloseAllBleConnections();
91+
}
92+
#endif
8593
}
8694

8795
void CommissioningWindowManager::Shutdown()

src/controller/AutoCommissioner.cpp

+19-22
Original file line numberDiff line numberDiff line change
@@ -687,35 +687,32 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
687687
mNeedsDST = false;
688688
break;
689689
case CommissioningStage::kReadCommissioningInfo2: {
690-
bool shouldReadCommissioningInfo2 =
691-
mParams.GetCheckForMatchingFabric() || (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore);
692-
if (shouldReadCommissioningInfo2)
690+
691+
if (!report.Is<ReadCommissioningInfo2>())
693692
{
694-
if (!report.Is<ReadCommissioningInfo2>())
695-
{
696-
ChipLogError(
697-
Controller,
698-
"[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG.");
699-
}
693+
ChipLogError(
694+
Controller,
695+
"[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG.");
696+
}
700697

701-
ReadCommissioningInfo2 commissioningInfo = report.Get<ReadCommissioningInfo2>();
698+
ReadCommissioningInfo2 commissioningInfo = report.Get<ReadCommissioningInfo2>();
699+
mParams.SetSupportsConcurrentConnection(commissioningInfo.supportsConcurrentConnection);
702700

703-
if (mParams.GetCheckForMatchingFabric())
701+
if (mParams.GetCheckForMatchingFabric())
702+
{
703+
chip::NodeId nodeId = commissioningInfo.nodeId;
704+
if (nodeId != kUndefinedNodeId)
704705
{
705-
chip::NodeId nodeId = commissioningInfo.nodeId;
706-
if (nodeId != kUndefinedNodeId)
707-
{
708-
mParams.SetRemoteNodeId(nodeId);
709-
}
706+
mParams.SetRemoteNodeId(nodeId);
710707
}
708+
}
711709

712-
if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
710+
if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
711+
{
712+
if (commissioningInfo.isIcd)
713713
{
714-
if (commissioningInfo.isIcd)
715-
{
716-
mNeedIcdRegistraion = true;
717-
ChipLogDetail(Controller, "AutoCommissioner: Device is ICD");
718-
}
714+
mNeedIcdRegistraion = true;
715+
ChipLogDetail(Controller, "AutoCommissioner: Device is ICD");
719716
}
720717
}
721718
break;

src/controller/CHIPDeviceController.cpp

+67-14
Original file line numberDiff line numberDiff line change
@@ -1611,6 +1611,27 @@ void OnBasicFailure(void * context, CHIP_ERROR error)
16111611
commissioner->CommissioningStageComplete(error);
16121612
}
16131613

1614+
static void NonConcurrentTimeout(void * context, CHIP_ERROR error)
1615+
{
1616+
if (error == CHIP_ERROR_TIMEOUT)
1617+
{
1618+
ChipLogProgress(Controller, "Non-concurrent mode: Expected NetworkResponse Timeout, do nothing");
1619+
}
1620+
else
1621+
{
1622+
ChipLogProgress(Controller, "Non-concurrent mode: Received failure response %" CHIP_ERROR_FORMAT, error.Format());
1623+
}
1624+
}
1625+
1626+
static void NonConcurrentNetworkResponse(void * context,
1627+
const NetworkCommissioning::Commands::ConnectNetworkResponse::DecodableType & data)
1628+
{
1629+
// In Non Concurrent mode the commissioning network should have been shut down and not sent the
1630+
// ConnectNetworkResponse. In case it does send it this handles the message
1631+
ChipLogError(Controller, "Non-concurrent Mode : Received Unexpected ConnectNetwork response, ignoring. Status=%u",
1632+
to_underlying(data.networkingStatus));
1633+
}
1634+
16141635
void DeviceCommissioner::CleanupCommissioning(DeviceProxy * proxy, NodeId nodeId, const CompletionStatus & completionStatus)
16151636
{
16161637
commissioningCompletionStatus = completionStatus;
@@ -2111,6 +2132,16 @@ void DeviceCommissioner::ParseCommissioningInfo2()
21112132
ReadCommissioningInfo2 info;
21122133
CHIP_ERROR return_err = CHIP_NO_ERROR;
21132134

2135+
using namespace chip::app::Clusters::GeneralCommissioning::Attributes;
2136+
CHIP_ERROR err =
2137+
mAttributeCache->Get<SupportsConcurrentConnection::TypeInfo>(kRootEndpointId, info.supportsConcurrentConnection);
2138+
if (err != CHIP_NO_ERROR)
2139+
{
2140+
// May not be present so don't return the error code, non fatal, default concurrent
2141+
ChipLogError(Controller, "Failed to read SupportsConcurrentConnection: %" CHIP_ERROR_FORMAT, err.Format());
2142+
info.supportsConcurrentConnection = true;
2143+
}
2144+
21142145
return_err = ParseFabrics(info);
21152146

21162147
if (return_err == CHIP_NO_ERROR)
@@ -2206,6 +2237,8 @@ CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info)
22062237
}
22072238
else if (err == CHIP_ERROR_KEY_NOT_FOUND)
22082239
{
2240+
// This key is optional so not an error
2241+
err = CHIP_NO_ERROR;
22092242
info.isIcd = false;
22102243
}
22112244
else if (err == CHIP_ERROR_IM_STATUS_CODE_RECEIVED)
@@ -2440,6 +2473,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
24402473
case CommissioningStage::kReadCommissioningInfo: {
24412474
ChipLogProgress(Controller, "Sending read request for commissioning information");
24422475
// NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9
2476+
// See R1.1, 2.11.2 Interaction Model Limits
24432477
app::AttributePathParams readPaths[9];
24442478
// Read all the feature maps for all the networking clusters on any endpoint to determine what is supported
24452479
readPaths[0] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id,
@@ -2471,7 +2505,14 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
24712505
case CommissioningStage::kReadCommissioningInfo2: {
24722506
size_t numberOfAttributes = 0;
24732507
// This is done in a separate step since we've already used up all the available read paths in the previous read step
2474-
app::AttributePathParams readPaths[9];
2508+
// NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9
2509+
// See R1.1, 2.11.2 Interaction Model Limits
2510+
app::AttributePathParams readPaths[3];
2511+
2512+
// Mandatory attribute
2513+
readPaths[numberOfAttributes++] =
2514+
app::AttributePathParams(endpoint, app::Clusters::GeneralCommissioning::Id,
2515+
app::Clusters::GeneralCommissioning::Attributes::SupportsConcurrentConnection::Id);
24752516

24762517
// Read the current fabrics
24772518
if (params.GetCheckForMatchingFabric())
@@ -2486,18 +2527,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
24862527
app::AttributePathParams(endpoint, IcdManagement::Id, IcdManagement::Attributes::FeatureMap::Id);
24872528
}
24882529

2489-
// Current implementation makes sense when we only have a few attributes to read with conditions. Should revisit this if we
2490-
// are adding more attributes here.
2491-
2492-
if (numberOfAttributes == 0)
2493-
{
2494-
// We don't actually want to do this step, so just bypass it
2495-
ChipLogProgress(Controller, "kReadCommissioningInfo2 step called without parameter set, skipping");
2496-
CommissioningStageComplete(CHIP_NO_ERROR);
2497-
return;
2498-
}
2499-
2500-
ChipLogProgress(Controller, "Sending request for commissioning information -- Part 2");
25012530
SendCommissioningReadRequest(proxy, timeout, readPaths, numberOfAttributes);
25022531
}
25032532
break;
@@ -2918,7 +2947,31 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
29182947
NetworkCommissioning::Commands::ConnectNetwork::Type request;
29192948
request.networkID = params.GetWiFiCredentials().Value().ssid;
29202949
request.breadcrumb.Emplace(breadcrumb);
2921-
CHIP_ERROR err = SendCommand(proxy, request, OnConnectNetworkResponse, OnBasicFailure, endpoint, timeout);
2950+
2951+
CHIP_ERROR err = CHIP_NO_ERROR;
2952+
GeneralCommissioning::Attributes::SupportsConcurrentConnection::TypeInfo::Type supportsConcurrentConnection;
2953+
supportsConcurrentConnection = params.GetSupportsConcurrentConnection().Value();
2954+
ChipLogProgress(Controller, "SendCommand kWiFiNetworkEnable, supportsConcurrentConnection=%d",
2955+
supportsConcurrentConnection);
2956+
if (supportsConcurrentConnection)
2957+
{
2958+
err = SendCommand(proxy, request, OnConnectNetworkResponse, OnBasicFailure, endpoint, timeout);
2959+
}
2960+
else
2961+
{
2962+
// Concurrent Connections not allowed. Send the ConnectNetwork command but do not wait for the
2963+
// ConnectNetworkResponse on the Commissioning network as it will not be present. Log the expected timeout
2964+
// and run what would have been in the onConnectNetworkResponse callback.
2965+
err = SendCommand(proxy, request, NonConcurrentNetworkResponse, NonConcurrentTimeout, endpoint, NullOptional);
2966+
if (err == CHIP_NO_ERROR)
2967+
{
2968+
// As there will be no ConnectNetworkResponse, it is an implicit kSuccess so a default report is fine
2969+
CommissioningDelegate::CommissioningReport report;
2970+
CommissioningStageComplete(CHIP_NO_ERROR, report);
2971+
return;
2972+
}
2973+
}
2974+
29222975
if (err != CHIP_NO_ERROR)
29232976
{
29242977
// We won't get any async callbacks here, so just complete our stage.

0 commit comments

Comments
 (0)