Skip to content

Commit

Permalink
Read the min network connection time from the device (#15862)
Browse files Browse the repository at this point in the history 
* Read the min network connection time from the device

This value indicates how long the commmisioner should wait for
the network connection to work. Read it, and set the timeout from it.

Also remove the read of software version - we're running up against
the read path limit, and it's not used anywhere.

* Update src/controller/AutoCommissioner.cpp

Co-authored-by: Michael Sandstedt <[email protected]>

Co-authored-by: Michael Sandstedt <[email protected]>
  • Loading branch information
@cecille @msandstedt @pull
2 people authored and pull[bot] committed Jul 26, 2023
1 parent 928579f commit 1283715
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 33 deletions.
40 changes: 27 additions & 13 deletions src/controller/AutoCommissioner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,12 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag
// operational network because the provisioning of certificates will trigger the device to start operational advertising.
if (mNeedsNetworkSetup)
{
if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi != kInvalidEndpointId)
if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi.endpoint != kInvalidEndpointId)
{
return CommissioningStage::kWiFiNetworkSetup;
}
else if (mParams.GetThreadOperationalDataset().HasValue() &&
mDeviceCommissioningInfo.network.thread != kInvalidEndpointId)
mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)
{
return CommissioningStage::kThreadNetworkSetup;
}
Expand All @@ -149,8 +149,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag
mParams.GetWiFiCredentials().HasValue() ? "yes" : "no",
mParams.GetThreadOperationalDataset().HasValue() ? "yes" : "no");
ChipLogError(Controller, "Device supports: wifi (%s) thread(%s)",
mDeviceCommissioningInfo.network.wifi == kInvalidEndpointId ? "no" : "yes",
mDeviceCommissioningInfo.network.thread == kInvalidEndpointId ? "no" : "yes");
mDeviceCommissioningInfo.network.wifi.endpoint == kInvalidEndpointId ? "no" : "yes",
mDeviceCommissioningInfo.network.thread.endpoint == kInvalidEndpointId ? "no" : "yes");
lastErr = CHIP_ERROR_INVALID_ARGUMENT;
return CommissioningStage::kCleanup;
}
Expand All @@ -160,7 +160,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag
return CommissioningStage::kFindOperational;
}
case CommissioningStage::kWiFiNetworkSetup:
if (mParams.GetThreadOperationalDataset().HasValue() && mDeviceCommissioningInfo.network.thread != kInvalidEndpointId)
if (mParams.GetThreadOperationalDataset().HasValue() &&
mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)
{
return CommissioningStage::kThreadNetworkSetup;
}
Expand All @@ -169,7 +170,7 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag
return CommissioningStage::kWiFiNetworkEnable;
}
case CommissioningStage::kThreadNetworkSetup:
if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi != kInvalidEndpointId)
if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi.endpoint != kInvalidEndpointId)
{
return CommissioningStage::kWiFiNetworkEnable;
}
Expand All @@ -179,7 +180,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag
}

case CommissioningStage::kWiFiNetworkEnable:
if (mParams.GetThreadOperationalDataset().HasValue() && mDeviceCommissioningInfo.network.thread != kInvalidEndpointId)
if (mParams.GetThreadOperationalDataset().HasValue() &&
mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)
{
return CommissioningStage::kThreadNetworkEnable;
}
Expand Down Expand Up @@ -211,10 +213,10 @@ EndpointId AutoCommissioner::GetEndpoint(const CommissioningStage & stage)
{
case CommissioningStage::kWiFiNetworkSetup:
case CommissioningStage::kWiFiNetworkEnable:
return mDeviceCommissioningInfo.network.wifi;
return mDeviceCommissioningInfo.network.wifi.endpoint;
case CommissioningStage::kThreadNetworkSetup:
case CommissioningStage::kThreadNetworkEnable:
return mDeviceCommissioningInfo.network.thread;
return mDeviceCommissioningInfo.network.thread.endpoint;
default:
return kRootEndpointId;
}
Expand Down Expand Up @@ -247,10 +249,22 @@ CHIP_ERROR AutoCommissioner::StartCommissioning(DeviceCommissioner * commissione

Optional<System::Clock::Timeout> AutoCommissioner::GetCommandTimeout(CommissioningStage stage)
{
// Per spec, all commands that are sent with the arm failsafe held need at least a 30s timeout. Using 30s everywhere for
// simplicity. 30s appears to be sufficient for long-running network commands (enable wifi and enable thread), but we may wish
// to increase the timeout for those commissioning stages at a later time.
return MakeOptional(System::Clock::Timeout(System::Clock::Seconds16(30)));
// Per spec, all commands that are sent with the arm failsafe held need at least a 30s timeout.
// Network clusters can indicate the time required to connect, so if we are connecting, use that time as long as it is > 30s.
app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::TypeInfo::DecodableType seconds = 30;
switch (stage)
{
case CommissioningStage::kWiFiNetworkEnable:
ChipLogError(Controller, "Setting wifi connection time min = %u", mDeviceCommissioningInfo.network.wifi.minConnectionTime);
seconds = std::max(mDeviceCommissioningInfo.network.wifi.minConnectionTime, seconds);
break;
case CommissioningStage::kThreadNetworkEnable:
seconds = std::max(mDeviceCommissioningInfo.network.thread.minConnectionTime, seconds);
break;
default:
break;
}
return MakeOptional(System::Clock::Timeout(System::Clock::Seconds16(seconds)));
}

CHIP_ERROR AutoCommissioner::NOCChainGenerated(ByteSpan noc, ByteSpan icac, ByteSpan rcac, AesCcm128KeySpan ipk,
Expand Down
52 changes: 38 additions & 14 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1586,8 +1586,7 @@ void DeviceCommissioner::OnDone()

err = mAttributeCache->ForEachAttribute(app::Clusters::Basic::Id, [this, &info](const app::ConcreteAttributePath & path) {
if (path.mAttributeId != app::Clusters::Basic::Attributes::VendorID::Id &&
path.mAttributeId != app::Clusters::Basic::Attributes::ProductID::Id &&
path.mAttributeId != app::Clusters::Basic::Attributes::SoftwareVersion::Id)
path.mAttributeId != app::Clusters::Basic::Attributes::ProductID::Id)
{
// Continue on
return CHIP_NO_ERROR;
Expand All @@ -1599,16 +1598,14 @@ void DeviceCommissioner::OnDone()
return this->mAttributeCache->Get<app::Clusters::Basic::Attributes::VendorID::TypeInfo>(path, info.basic.vendorId);
case app::Clusters::Basic::Attributes::ProductID::Id:
return this->mAttributeCache->Get<app::Clusters::Basic::Attributes::ProductID::TypeInfo>(path, info.basic.productId);
case app::Clusters::Basic::Attributes::SoftwareVersion::Id:
return this->mAttributeCache->Get<app::Clusters::Basic::Attributes::SoftwareVersion::TypeInfo>(
path, info.basic.softwareVersion);
default:
return CHIP_NO_ERROR;
}
});
// Try to parse as much as we can here before returning, even if this is an error.
return_err = err == CHIP_NO_ERROR ? return_err : err;

// Set the network cluster endpoints first so we can match up the connection times.
err = mAttributeCache->ForEachAttribute(
app::Clusters::NetworkCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) {
if (path.mAttributeId != app::Clusters::NetworkCommissioning::Attributes::FeatureMap::Id)
Expand All @@ -1623,28 +1620,28 @@ void DeviceCommissioner::OnDone()
{
if (features.Has(app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kWiFiNetworkInterface))
{
info.network.wifi = path.mEndpointId;
info.network.wifi.endpoint = path.mEndpointId;
}
else if (features.Has(
app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kThreadNetworkInterface))
{
info.network.thread = path.mEndpointId;
info.network.thread.endpoint = path.mEndpointId;
}
else if (features.Has(
app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kEthernetNetworkInterface))
{
info.network.eth = path.mEndpointId;
info.network.eth.endpoint = path.mEndpointId;
}
else
{
// TODO: Gross workaround for the empty feature map on all clusters. Remove.
if (info.network.thread == kInvalidEndpointId)
if (info.network.thread.endpoint == kInvalidEndpointId)
{
info.network.thread = path.mEndpointId;
info.network.thread.endpoint = path.mEndpointId;
}
if (info.network.wifi == kInvalidEndpointId)
if (info.network.wifi.endpoint == kInvalidEndpointId)
{
info.network.wifi = path.mEndpointId;
info.network.wifi.endpoint = path.mEndpointId;
}
}
}
Expand All @@ -1653,6 +1650,32 @@ void DeviceCommissioner::OnDone()
});
return_err = err == CHIP_NO_ERROR ? return_err : err;

err = mAttributeCache->ForEachAttribute(
app::Clusters::NetworkCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) {
if (path.mAttributeId != app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::Id)
{
return CHIP_NO_ERROR;
}
app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::TypeInfo::DecodableArgType time;
ReturnErrorOnFailure(
this->mAttributeCache->Get<app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::TypeInfo>(path,
time));
if (path.mEndpointId == info.network.wifi.endpoint)
{
info.network.wifi.minConnectionTime = time;
}
else if (path.mEndpointId == info.network.thread.endpoint)
{
info.network.thread.minConnectionTime = time;
}
else if (path.mEndpointId == info.network.eth.endpoint)
{
info.network.eth.minConnectionTime = time;
}
return CHIP_NO_ERROR;
});
return_err = err == CHIP_NO_ERROR ? return_err : err;

if (return_err != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Error parsing commissioning information");
Expand Down Expand Up @@ -1758,8 +1781,9 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
readPaths[5] = app::AttributePathParams(endpoint, app::Clusters::Basic::Id, app::Clusters::Basic::Attributes::VendorID::Id);
readPaths[6] =
app::AttributePathParams(endpoint, app::Clusters::Basic::Id, app::Clusters::Basic::Attributes::ProductID::Id);
readPaths[7] =
app::AttributePathParams(endpoint, app::Clusters::Basic::Id, app::Clusters::Basic::Attributes::SoftwareVersion::Id);
// Read the requested minimum connection times from all network commissioning clusters
readPaths[7] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id,
app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::Id);

readParams.mpAttributePathParamsList = readPaths;
readParams.mAttributePathParamsListSize = 8;
Expand Down
16 changes: 10 additions & 6 deletions src/controller/CommissioningDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,17 +284,21 @@ struct OperationalNodeFoundData
OperationalDeviceProxy * operationalProxy;
};

struct NetworkClusterInfo
{
EndpointId endpoint = kInvalidEndpointId;
app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::TypeInfo::DecodableType minConnectionTime;
};
struct NetworkClusters
{
EndpointId wifi = kInvalidEndpointId;
EndpointId thread = kInvalidEndpointId;
EndpointId eth = kInvalidEndpointId;
NetworkClusterInfo wifi;
NetworkClusterInfo thread;
NetworkClusterInfo eth;
};
struct BasicClusterInfo
{
VendorId vendorId = VendorId::Common;
uint16_t productId = 0;
uint32_t softwareVersion = 0;
VendorId vendorId = VendorId::Common;
uint16_t productId = 0;
};
struct GeneralCommissioningInfo
{
Expand Down

0 comments on commit 1283715

Please sign in to comment.