Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check descriptor clusters during commissioning #14410

Closed
wants to merge 7 commits into from
Closed

Check descriptor clusters during commissioning #14410

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c569a80
Read parts list from descriptor cluster
cecille Jan 26, 2022
9e8f49a
Add step to read server clusters from descriptor.
cecille Jan 25, 2022
eb8b946
Read basic cluster information during commissioning.
cecille Jan 26, 2022
8afd91b
Pass back status from iter.
cecille Jan 28, 2022
69a9284
Merge remote-tracking branch 'upstream/master' into check_descriptor_…
cecille Jan 28, 2022
9556c90
sequence network cluster existance and feature checks.
cecille Jan 28, 2022
420142b
Restyled by clang-format
restyled-commits Jan 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 55 additions & 4 deletions src/controller/AutoCommissioner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,26 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag
{
return CommissioningStage::kCleanup;
}
mEndpoint = 0;
switch (currentStage)
{
case CommissioningStage::kSecurePairing:
return CommissioningStage::kReadVendorId;
case CommissioningStage::kReadVendorId:
return CommissioningStage::kReadProductId;
case CommissioningStage::kReadProductId:
return CommissioningStage::kReadSoftwareVersion;
case CommissioningStage::kReadSoftwareVersion:
return CommissioningStage::kGetPartsList;
case CommissioningStage::kGetPartsList:
return CommissioningStage::kCheckEndpointIsCommissionable;
case CommissioningStage::kCheckEndpointIsCommissionable:
if (mAllEndpoints.numEndpoints > 0)
{
// Cycle through the list of endpoints from the end, checking for network cluster.
cecille marked this conversation as resolved.
Show resolved Hide resolved
mEndpoint = mAllEndpoints.endpoints[--mAllEndpoints.numEndpoints];
return CommissioningStage::kCheckEndpointIsCommissionable;
}
return CommissioningStage::kArmFailsafe;
case CommissioningStage::kArmFailsafe:
return CommissioningStage::kConfigRegulatory;
Expand Down Expand Up @@ -194,8 +211,9 @@ void AutoCommissioner::StartCommissioning(CommissioneeDeviceProxy * proxy)
{
// TODO: check that there is no commissioning in progress currently.
mCommissioneeDeviceProxy = proxy;
mCommissioner->PerformCommissioningStep(mCommissioneeDeviceProxy, CommissioningStage::kArmFailsafe, mParams, this, 0,
GetCommandTimeout(CommissioningStage::kArmFailsafe));
mCommissioner->PerformCommissioningStep(mCommissioneeDeviceProxy,
GetNextCommissioningStage(CommissioningStage::kSecurePairing, CHIP_NO_ERROR), mParams,
this, 0, GetCommandTimeout(CommissioningStage::kArmFailsafe));
}

Optional<System::Clock::Timeout> AutoCommissioner::GetCommandTimeout(CommissioningStage stage)
Expand Down Expand Up @@ -248,12 +266,43 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
{
if (err != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Failed to perform commissioning step %d", static_cast<int>(report.stageCompleted));
if (report.stageCompleted == CommissioningStage::kCheckEndpointIsCommissionable && mEndpoint != 0)
{
ChipLogError(Controller, "No descriptor cluster found for endpoint %u, ignoring", mEndpoint);
err = CHIP_NO_ERROR;
}
else
{
ChipLogError(Controller, "Failed to perform commissioning step %d", static_cast<int>(report.stageCompleted));
}
}
else
{
switch (report.stageCompleted)
{
case CommissioningStage::kReadVendorId:
mVendorId = report.Get<BasicVendor>().vendorId;
break;
case CommissioningStage::kReadProductId:
mProductId = report.Get<BasicProduct>().productId;
break;
case CommissioningStage::kReadSoftwareVersion:
mSoftwareVersion = report.Get<BasicSoftware>().softwareVersion;
break;
case CommissioningStage::kGetPartsList:
mAllEndpoints = report.Get<EndpointParts>();
break;
case CommissioningStage::kCheckEndpointIsCommissionable:
if (mEndpoint == 0 && !report.Get<EndpointCommissioningInfo>().isCommissionable)
{
ChipLogError(Controller, "Device endpoint is not commissionable");
return CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR;
}
if (report.Get<EndpointCommissioningInfo>().hasNetworkCluster)
{
mNetworkEndpoints.endpoints[mNetworkEndpoints.numEndpoints++] = mEndpoint;
}
break;
case CommissioningStage::kSendPAICertificateRequest:
SetPAI(report.Get<RequestedCertificate>().certificate);
break;
Expand Down Expand Up @@ -294,6 +343,8 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
mCommissioneeDeviceProxy = nullptr;
mOperationalDeviceProxy = nullptr;
mParams = CommissioningParameters();
mAllEndpoints = EndpointParts();
mNetworkEndpoints = EndpointParts();
return CHIP_NO_ERROR;
default:
break;
Expand Down Expand Up @@ -321,7 +372,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio

mParams.SetCompletionStatus(err);
// TODO: Get real endpoint
mCommissioner->PerformCommissioningStep(proxy, nextStage, mParams, this, 0, GetCommandTimeout(nextStage));
mCommissioner->PerformCommissioningStep(proxy, nextStage, mParams, this, mEndpoint, GetCommandTimeout(nextStage));
return CHIP_NO_ERROR;
}

Expand Down
7 changes: 6 additions & 1 deletion src/controller/AutoCommissioner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ class AutoCommissioner : public CommissioningDelegate
OperationalDeviceProxy * mOperationalDeviceProxy = nullptr;
OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr;
CommissioningParameters mParams = CommissioningParameters();
EndpointParts mAllEndpoints;
EndpointParts mNetworkEndpoints;
EndpointId mEndpoint;
VendorId mVendorId;
uint16_t mProductId;
uint32_t mSoftwareVersion;
// 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];
Expand All @@ -72,6 +78,5 @@ class AutoCommissioner : public CommissioningDelegate
uint8_t mNOCertBuffer[Credentials::kMaxCHIPCertLength];
uint8_t mICACertBuffer[Credentials::kMaxCHIPCertLength];
};

} // namespace Controller
} // namespace chip
123 changes: 120 additions & 3 deletions src/controller/CHIPDeviceController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1580,10 +1580,88 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer
void DeviceCommissioner::SetupCluster(ClusterBase & base, DeviceProxy * proxy, EndpointId endpoint,
Optional<System::Clock::Timeout> timeout)
{
base.Associate(proxy, 0);
base.Associate(proxy, endpoint);
base.SetCommandTimeout(timeout);
}

void DescriptorClusterPartsCallback(void * context, const chip::app::DataModel::DecodableList<chip::EndpointId> & data)
{
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
EndpointParts parts;
auto iter = data.begin();
while (iter.Next() && parts.numEndpoints < parts.kMaxEndpoints)
{
parts.endpoints[parts.numEndpoints++] = iter.GetValue();
}
CommissioningDelegate::CommissioningReport report;
cecille marked this conversation as resolved.
Show resolved Hide resolved
report.Set<EndpointParts>(parts);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
}

void DescriptorClusterServerCallback(void * context, const chip::app::DataModel::DecodableList<chip::ClusterId> & data)
{
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);

bool hasGeneralCommissioning = false;
bool hasNetworkCommissioning = false;
bool hasBasic = false;
bool hasOperationalCredentials = false;

auto iter = data.begin();
while (iter.Next())
{
switch (iter.GetValue())
{
case app::Clusters::GeneralCommissioning::Id:
hasGeneralCommissioning = true;
break;
case app::Clusters::NetworkCommissioning::Id:
hasNetworkCommissioning = true;
break;
case app::Clusters::Basic::Id:
hasBasic = true;
break;
case app::Clusters::OperationalCredentials::Id:
hasOperationalCredentials = true;
break;
}
}
bool isCommissionable = hasGeneralCommissioning && hasBasic && hasOperationalCredentials;
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
cecille marked this conversation as resolved.
Show resolved Hide resolved
CommissioningDelegate::CommissioningReport report;
report.Set<EndpointCommissioningInfo>(EndpointCommissioningInfo(isCommissionable, hasNetworkCommissioning));
commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
}

void BasicVendorCallback(void * context, VendorId vendorId)
{
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
CommissioningDelegate::CommissioningReport report;
report.Set<BasicVendor>(vendorId);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
}

void BasicProductCallback(void * context, uint16_t productId)
{
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
CommissioningDelegate::CommissioningReport report;
report.Set<BasicProduct>(productId);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
}

void BasicSoftwareCallback(void * context, uint32_t softwareVersion)
{
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
CommissioningDelegate::CommissioningReport report;
report.Set<BasicSoftware>(softwareVersion);
commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
}

void AttributeReadFailure(void * context, CHIP_ERROR status)
{
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
commissioner->CommissioningStageComplete(status);
}

void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, CommissioningStage step, CommissioningParameters & params,
CommissioningDelegate * delegate, EndpointId endpoint,
Optional<System::Clock::Timeout> timeout)
Expand All @@ -1602,11 +1680,50 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio

switch (step)
{
case CommissioningStage::kReadVendorId: {
ChipLogProgress(Controller, "Reading vendor ID");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of round-trips.... Do we actually want to read these attributes one at a time? Why do we want to walk the list of all endpoints starting from the end looking for network commissioning things, instead of starting at the front and stopping when we find the first network commissioning thing which supports a network technology we support?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but as far as I can tell, we don't have support for bulk reading attributes right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all checked in order to address the request for supporting network commissioning clusters on other endpoints (multiple radios). There is always a NetworkCommissioning cluster on EP0, but you had a request in the network tecnology PR (#13829) to support network commissioning clusters that could be present on endpoints with manufacturer specific device types.

see #14412

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have support for bulk reading attributes right now

We certainly do. You just have to use a slightly lower-level API to do it, because the results of such a read can't be expressed as a single C++ value.... Doing this can be a followup, though.

They're all checked in order to address the request for supporting network commissioning clusters on other endpoints

This just seems like an odd approach to addressing that request. What I would have expected is we start with the network commissioning on EP0. If we can't commission using that (rare case), we look for another one.

It feels like this is something where people are going to want to do a variety of things and UX flows, and reading all endpoints can be quite costly (latency) for devices with lots of endpoints....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's always going to be a network commissioning cluster on EP0, so if we're starting there, we're ending there. If we want to allow devices to specify more than one networking technology, we have to read them all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following. If what's on EP0 is a WiFi cluster but we don't have any WiFi credentials, but do have Thread ones, then we would want to look for other network commissioning clusters to see if the device supports Thread. But if EP0 has a network technology we're happy to commission with, why would we spend time looking at the other endpoints?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually argue that dual network commissioning clusters is dubiously supported anyway and unlikely to happen in a real device. The only place where this would be really beneficial would be for a border router, and the network commissioning clusters are insufficient there anyway because we there aren't sufficient commands to support creating a thread network.

The alternate is to assume EP0 for the networking cluster allow devices that require a more complicated setup use a custom commissioning flow. If we want to be able to commission border routers, the network commissioning cluster needs some additional commands, and we could then add a parts list to make the endpoint walk easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you know to send in only thread credentials if you didn't know what the device supported in the first place? That assumes that the commissioning parameters are the source of truth, whereas what we really want is for the device to be the source of truth.

I suppose we could cut out some of the back and forth if we only have one set of credentials and it happens to match the feature map for the network cluster on EP0.

I wrote it this way originally because I was going to double-purpose this for a convenience function to allow devs to get device information before calling the commission command. Let me see if I can maybe disentangle this use case and short-circut the full walk during commissioning if we only get one set of network credentials that matches EP0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've updated the PR to just go with the first networking cluster it finds. Working with jerry on a follow up to just query all the network cluster feature maps using wild card - I didn't realize that was implemented now, but it's a bit more of a change.

BasicCluster basic;
SetupCluster(basic, proxy, endpoint, timeout);
basic.ReadAttribute<chip::app::Clusters::Basic::Attributes::VendorID::TypeInfo>(this, BasicVendorCallback,
AttributeReadFailure);
}
break;
case CommissioningStage::kReadProductId: {
ChipLogProgress(Controller, "Reading product ID");
BasicCluster basic;
SetupCluster(basic, proxy, endpoint, timeout);
basic.ReadAttribute<chip::app::Clusters::Basic::Attributes::ProductID::TypeInfo>(this, BasicProductCallback,
AttributeReadFailure);
}
break;
case CommissioningStage::kReadSoftwareVersion: {
ChipLogProgress(Controller, "Reading software version");
BasicCluster basic;
SetupCluster(basic, proxy, endpoint, timeout);
basic.ReadAttribute<chip::app::Clusters::Basic::Attributes::SoftwareVersion::TypeInfo>(this, BasicSoftwareCallback,
AttributeReadFailure);
}
break;

case CommissioningStage::kGetPartsList: {
ChipLogProgress(Controller, "Reading descriptor cluster parts list");
DescriptorCluster desc;
SetupCluster(desc, proxy, endpoint, timeout);
desc.ReadAttribute<chip::app::Clusters::Descriptor::Attributes::PartsList::TypeInfo>(this, DescriptorClusterPartsCallback,
AttributeReadFailure);
}
break;
case CommissioningStage::kCheckEndpointIsCommissionable: {
ChipLogProgress(Controller, "Reading descriptor cluster server list for endpoint %u", endpoint);
DescriptorCluster desc;
SetupCluster(desc, proxy, endpoint, timeout);
desc.ReadAttribute<chip::app::Clusters::Descriptor::Attributes::ServerList::TypeInfo>(this, DescriptorClusterServerCallback,
AttributeReadFailure);
}
break;
case CommissioningStage::kArmFailsafe: {
ChipLogProgress(Controller, "Arming failsafe");
// TODO(cecille): Find a way to enumerate the clusters here.
GeneralCommissioningCluster genCom;
// TODO: should get the endpoint information from the descriptor cluster.
SetupCluster(genCom, proxy, endpoint, timeout);
genCom.ArmFailSafe(mSuccess.Cancel(), mFailure.Cancel(), params.GetFailsafeTimerSeconds(), breadcrumb, kCommandTimeoutMs);
}
Expand Down
45 changes: 42 additions & 3 deletions src/controller/CommissioningDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ enum CommissioningStage : uint8_t
{
kError,
kSecurePairing,
kReadVendorId,
kReadProductId,
kReadSoftwareVersion,
kGetPartsList,
kCheckEndpointIsCommissionable,
kArmFailsafe,
// kConfigTime, // NOT YET IMPLEMENTED
// kConfigTimeZone, // NOT YET IMPLEMENTED
Expand Down Expand Up @@ -232,16 +237,50 @@ struct OperationalNodeFoundData
OperationalNodeFoundData(OperationalDeviceProxy * proxy) : operationalProxy(proxy) {}
OperationalDeviceProxy * operationalProxy;
};

struct EndpointParts
{
EndpointParts() : numEndpoints(0) {}
// TODO: I don't think this is specified anywhere in the spec
// Is 10 reasonable? This is just to find the network commissioning clusters
static constexpr size_t kMaxEndpoints = 10;
EndpointId endpoints[kMaxEndpoints];
size_t numEndpoints;
};
struct EndpointCommissioningInfo
{
EndpointCommissioningInfo(bool commissionable, bool network) : isCommissionable(commissionable), hasNetworkCluster(network) {}
bool isCommissionable = false;
bool hasNetworkCluster = false;
};

struct BasicVendor
{
BasicVendor(VendorId id) : vendorId(id) {}
VendorId vendorId;
};

struct BasicProduct
{
BasicProduct(uint16_t id) : productId(id) {}
uint16_t productId;
};

struct BasicSoftware
{
BasicSoftware(uint32_t version) : softwareVersion(version) {}
uint32_t softwareVersion;
};

class CommissioningDelegate
{
public:
virtual ~CommissioningDelegate(){};

struct CommissioningReport : Variant<RequestedCertificate, AttestationResponse, NocChain, OperationalNodeFoundData>
struct CommissioningReport : Variant<RequestedCertificate, AttestationResponse, NocChain, OperationalNodeFoundData,
EndpointParts, EndpointCommissioningInfo, BasicVendor, BasicProduct, BasicSoftware>
{
CommissioningReport() : stageCompleted(CommissioningStage::kError) {}
CommissioningStage stageCompleted;
// TODO: Add other things the delegate needs to know.
};
virtual CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningReport report) = 0;
};
Expand Down