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.

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

IP cluster-based commissioning #6798

Merged
merged 9 commits into from
May 26, 2021
Merged

Conversation

cecille
Copy link
Contributor

@cecille cecille commented May 14, 2021

Problem

Commissioning doesn't currently perform any of the required steps for actually commissioning a device.

Change overview

Adds calls to clusters to perform commissioning required for IP-based devices

  • arms failsafe
  • sets regulatory config
  • enables network
  • looks for device on operational network
  • calls commissioning complete.

Testing

manual (requires controller and device) - chip-device-ctrl and M5 in commissionable node mode.
Start M5 with network credentials, go to Setup->Force wifi commissioning. Use chip-device-ctrl to connect (connect -qr "[qrcode]")

Fixes #5715

@todo
Copy link

todo bot commented May 14, 2021

issue #6792 - the secure session parameter should be made non-optional and passed by reference.

// TODO: issue #6792 - the secure session parameter should be made non-optional and passed by reference.
CHIP_ERROR SendCommandRequest(NodeId aNodeId, Transport::AdminId aAdminId, SecureSessionHandle * secureSession = nullptr);
void OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle aPayload) override;


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): This is very dangerous - need to check against real netif name, ensure no password.

// TODO(cecille): This is very dangerous - need to check against real netif name, ensure no password.
static constexpr char ethernetNetifMagicCode[] = "ETH0";
if (networkID.size() == sizeof(ethernetNetifMagicCode) &&
memcmp(networkID.data(), ethernetNetifMagicCode, networkID.size()) == 0)
{
ChipLogProgress(Zcl, "Enabling ethernet network");
ExitNow(err = EMBER_ZCL_NETWORK_COMMISSIONING_ERROR_SUCCESS);
}
for (networkSeq = 0; networkSeq < kMaxNetworks; networkSeq++)
{


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

Commissioning complete means we can finalize the admin in our storage

// TODO: Commissioning complete means we can finalize the admin in our storage
}
else if (event->Type == DeviceLayer::DeviceEventType::kOperationalNetworkEnabled)
{
app::Mdns::AdvertiseOperational();
ChipLogError(Discovery, "Operational advertising enabled");
}
}
CHIP_ERROR RendezvousServer::WaitForPairing(const RendezvousParameters & params, Messaging::ExchangeManager * exchangeManager,
TransportMgrBase * transportMgr, SecureSessionMgr * sessionMgr,


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

remove this once we move all tools / examples onto cluster-based IP commissioning.

// TODO: remove this once we move all tools / examples onto cluster-based IP commissioning.
#if CONFIG_RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE
Cleanup();
#endif
}
ChipLogProgress(AppServer, "Device completed Rendezvous process");
StorablePeerConnection connection(mPairingSession, mAdmin->GetAdminId());


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

Need to check if we're properly commissioned.

// TODO : Need to check if we're properly commissioned.
advertise();
break;
#if CHIP_DEVICE_CONFIG_ENABLE_THREAD


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): For IP rendezvous, these need to go through the state machine once they're implemented.

// TODO(cecille): For IP rendezvous, these need to go through the state machine once they're implemented.
ReturnErrorOnFailure(
SendOperationalCertificate(device, ByteSpan(chipCert.Get(), opCertLen), ByteSpan(signingCert.Get(), signingCertLen)));


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

for softAP, this needs to be network setup

return CommissioningStage::kNetworkEnable; // TODO : for softAP, this needs to be network setup
case CommissioningStage::kNetworkEnable:
#if CHIP_DEVICE_CONFIG_ENABLE_MDNS
return CommissioningStage::kFindOperational; // TODO : once case is working, need to add stages to find and reconnect
// here.
#else
return CommissioningStage::kSendComplete;
#endif
case CommissioningStage::kFindOperational:
return CommissioningStage::kSendComplete;
case CommissioningStage::kSendComplete:


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

once case is working, need to add stages to find and reconnect

return CommissioningStage::kFindOperational; // TODO : once case is working, need to add stages to find and reconnect
// here.
#else
return CommissioningStage::kSendComplete;
#endif
case CommissioningStage::kFindOperational:
return CommissioningStage::kSendComplete;
case CommissioningStage::kSendComplete:
return CommissioningStage::kCleanup;
// Currently unimplemented.


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): We probably want something better than this for breadcrumbs.

// TODO(cecille): We probably want something better than this for breadcrumbs.
uint64_t breadcrumb = static_cast<uint64_t>(nextStage);
// TODO(cecille): This should be customized per command.
uint32_t commandTimeoutMs = 1000;
switch (nextStage)
{
case CommissioningStage::kArmFailsafe: {
// TODO(cecille): This is NOT the right way to do this - we should consider attaching an im delegate per command or
// something. Per exchange context?


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): This should be customized per command.

// TODO(cecille): This should be customized per command.
uint32_t commandTimeoutMs = 1000;
switch (nextStage)
{
case CommissioningStage::kArmFailsafe: {
// TODO(cecille): This is NOT the right way to do this - we should consider attaching an im delegate per command or
// something. Per exchange context?
if (mDefaultIMDelegate == nullptr)
{
ChipLogProgress(Controller, "swapping delegate for IM");


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): This is NOT the right way to do this - we should consider attaching an im delegate per command or

// TODO(cecille): This is NOT the right way to do this - we should consider attaching an im delegate per command or
// something. Per exchange context?
if (mDefaultIMDelegate == nullptr)
{
ChipLogProgress(Controller, "swapping delegate for IM");
mDefaultIMDelegate = chip::Platform::New<DeviceControllerInteractionModelDelegate>();
chip::app::InteractionModelEngine::GetInstance()->SetDelegate(mDefaultIMDelegate);
device->InitCommandSender();
}
ChipLogProgress(Controller, "Arming failsafe");


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): Find a way to enumerate the clusters here.

// TODO(cecille): Find a way to enumerate the clusters here.
GeneralCommissioningCluster genCom;
uint16_t commissioningExpirySeconds = 5;
// TODO: should get the endpoint information from the descriptor cluster.
genCom.Associate(device, 0);
genCom.ArmFailSafe(success->Cancel(), failure->Cancel(), commissioningExpirySeconds, breadcrumb, commandTimeoutMs);
}
break;
case CommissioningStage::kConfigRegulatory: {
// To set during config phase:
// UTC time


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

should get the endpoint information from the descriptor cluster.

// TODO: should get the endpoint information from the descriptor cluster.
genCom.Associate(device, 0);
genCom.ArmFailSafe(success->Cancel(), failure->Cancel(), commissioningExpirySeconds, breadcrumb, commandTimeoutMs);
}
break;
case CommissioningStage::kConfigRegulatory: {
// To set during config phase:
// UTC time
// time zone
// dst offset
// Regulatory config


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): Set time as well once the time cluster is implemented

// TODO(cecille): Set time as well once the time cluster is implemented
// TODO(cecille): Worthwhile to keep this around as part of the class?
// TODO(cecille): Where is the country config actually set?
ChipLogProgress(Controller, "Setting Regulatory Config");
uint32_t regulatoryLocation;
CHIP_ERROR status = DeviceLayer::ConfigurationMgr().GetRegulatoryLocation(regulatoryLocation);
if (status != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Unable to find regulatory location, defaulting to outdoor");
regulatoryLocation = EMBER_ZCL_REGULATORY_LOCATION_TYPE_OUTDOOR;
}


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): Worthwhile to keep this around as part of the class?

// TODO(cecille): Worthwhile to keep this around as part of the class?
// TODO(cecille): Where is the country config actually set?
ChipLogProgress(Controller, "Setting Regulatory Config");
uint32_t regulatoryLocation;
CHIP_ERROR status = DeviceLayer::ConfigurationMgr().GetRegulatoryLocation(regulatoryLocation);
if (status != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Unable to find regulatory location, defaulting to outdoor");
regulatoryLocation = EMBER_ZCL_REGULATORY_LOCATION_TYPE_OUTDOOR;
}


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): Where is the country config actually set?

// TODO(cecille): Where is the country config actually set?
ChipLogProgress(Controller, "Setting Regulatory Config");
uint32_t regulatoryLocation;
CHIP_ERROR status = DeviceLayer::ConfigurationMgr().GetRegulatoryLocation(regulatoryLocation);
if (status != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Unable to find regulatory location, defaulting to outdoor");
regulatoryLocation = EMBER_ZCL_REGULATORY_LOCATION_TYPE_OUTDOOR;
}
static constexpr size_t kMaxCountryCodeSize = 3;


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): Once this is implemented through the clusters, it should be moved to the proper stage and the callback

// TODO(cecille): Once this is implemented through the clusters, it should be moved to the proper stage and the callback
// should advance the commissioning stage
CHIP_ERROR status = SendOperationalCertificateSigningRequestCommand(device);
if (status != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Failed in sending opcsr request command to the device: err %s", ErrorStr(err));
OnSessionEstablishmentError(err);
return;
}
}
break;


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

Right now, these stages are not implemented as a separate stage because they are no-ops.

// TODO: Right now, these stages are not implemented as a separate stage because they are no-ops.
// Once these are implemented through the clusters, these should be moved into their separate stages and the callbacks
// should advance the commissioning stage.
case CommissioningStage::kConfigACL:
case CommissioningStage::kNetworkSetup:
case CommissioningStage::kScanNetworks:
// TODO: Implement
break;
case CommissioningStage::kNetworkEnable: {
ChipLogProgress(Controller, "Enabling Network");
// TODO: For ethernet, we actually need a scan stage to get the ethernet netif name. Right now, default to using a magic


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

Implement

// TODO: Implement
break;
case CommissioningStage::kNetworkEnable: {
ChipLogProgress(Controller, "Enabling Network");
// TODO: For ethernet, we actually need a scan stage to get the ethernet netif name. Right now, default to using a magic
// value to enable without checks.
NetworkCommissioningCluster netCom;
// TODO: should get the endpoint information from the descriptor cluster.
netCom.Associate(device, 0);
// TODO: Once network credential sending is implemented, attempting to set wifi credential on an ethernet only device
// will cause an error to be sent back. At that point, we should scan and we shoud see the proper ethernet network ID


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

For ethernet, we actually need a scan stage to get the ethernet netif name. Right now, default to using a magic

// TODO: For ethernet, we actually need a scan stage to get the ethernet netif name. Right now, default to using a magic
// value to enable without checks.
NetworkCommissioningCluster netCom;
// TODO: should get the endpoint information from the descriptor cluster.
netCom.Associate(device, 0);
// TODO: Once network credential sending is implemented, attempting to set wifi credential on an ethernet only device
// will cause an error to be sent back. At that point, we should scan and we shoud see the proper ethernet network ID
// returned in the scan results. For now, we use magic.
char magicNetworkEnableCode[] = "ETH0";
netCom.EnableNetwork(success->Cancel(), failure->Cancel(),
ByteSpan(reinterpret_cast<uint8_t *>(&magicNetworkEnableCode), sizeof(magicNetworkEnableCode)),


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

should get the endpoint information from the descriptor cluster.

// TODO: should get the endpoint information from the descriptor cluster.
netCom.Associate(device, 0);
// TODO: Once network credential sending is implemented, attempting to set wifi credential on an ethernet only device
// will cause an error to be sent back. At that point, we should scan and we shoud see the proper ethernet network ID
// returned in the scan results. For now, we use magic.
char magicNetworkEnableCode[] = "ETH0";
netCom.EnableNetwork(success->Cancel(), failure->Cancel(),
ByteSpan(reinterpret_cast<uint8_t *>(&magicNetworkEnableCode), sizeof(magicNetworkEnableCode)),
breadcrumb, commandTimeoutMs);
}
break;


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

Once network credential sending is implemented, attempting to set wifi credential on an ethernet only device

// TODO: Once network credential sending is implemented, attempting to set wifi credential on an ethernet only device
// will cause an error to be sent back. At that point, we should scan and we shoud see the proper ethernet network ID
// returned in the scan results. For now, we use magic.
char magicNetworkEnableCode[] = "ETH0";
netCom.EnableNetwork(success->Cancel(), failure->Cancel(),
ByteSpan(reinterpret_cast<uint8_t *>(&magicNetworkEnableCode), sizeof(magicNetworkEnableCode)),
breadcrumb, commandTimeoutMs);
}
break;
case CommissioningStage::kFindOperational: {
#if CHIP_DEVICE_CONFIG_ENABLE_MDNS


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

this is actualy not correct - we must reconnect over CASE to send this command.

// TODO this is actualy not correct - we must reconnect over CASE to send this command.
ChipLogProgress(Controller, "Calling commissioning complete");
GeneralCommissioningCluster genCom;
genCom.Associate(device, 0);
genCom.CommissioningComplete(success->Cancel(), failure->Cancel());
}
break;
case CommissioningStage::kCleanup:
ChipLogProgress(Controller, "Rendezvous cleanup");
RendezvousCleanup(CHIP_NO_ERROR);
break;


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): This should just specify wifi or thread since we assume at most 1.

// TODO(cecille): This should just specify wifi or thread since we assume at most 1.
int network;
} OperationalNetwork;
};
void Clear() { memset(this, 0, sizeof(*this)); }


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

@todo
Copy link

todo bot commented May 14, 2021

(cecille): This command fails on ESP32, but it's blocking IP cluster-based commissioning so for now just return a success

// TODO(cecille): This command fails on ESP32, but it's blocking IP cluster-based commissioning so for now just return a success
// status.
return CHIP_NO_ERROR;
}
CHIP_ERROR DeviceControlServer::EnableNetworkForOperational(ByteSpan networkID)
{
ChipDeviceEvent event;
event.Type = DeviceEventType::kOperationalNetworkEnabled;
// TODO(cecille): This should be some way to specify thread or wifi.
event.OperationalNetwork.network = 0;


This comment was generated by todo based on a TODO comment in 1b2c80a in #6798. cc @cecille.

This was referenced May 26, 2021
Damian-Nordic added a commit to Damian-Nordic/connectedhomeip that referenced this pull request May 31, 2021
PR project-chip#6798 added another call to "advertiseOpertional()" which
starts publishing the opertional node service on a receipt
of a valid EnableNetwork command. Consequently, the code
which publishes the same service on a change in the network
configuration became redundant and actually harmful due to
some issues with updating an SRP service.
andy31415 pushed a commit that referenced this pull request Jun 1, 2021
PR #6798 added another call to "advertiseOpertional()" which
starts publishing the opertional node service on a receipt
of a valid EnableNetwork command. Consequently, the code
which publishes the same service on a change in the network
configuration became redundant and actually harmful due to
some issues with updating an SRP service.
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* Cluster-based commissioning - phase 1.

Adds calls to clusters to commission a device properly. This is
intended to be used to setup a device that is already on the
network, but not already on a CHIP network. Hence, network
credentials are not required, but we do need to send all the
remaining commissioning information.

Challenges:
IM cluster commands time out more often than the non-IM commands,
even when the device is clearly sending a reply.
Need to find the race and get rid of it.

Still to do:
- add operational credentials into state machine
- add more commissioning phases (time set etc.)
- add proper scan command for determining ethernet netif name
- swap over to CASE before sending commissioning complete command
- once we're totally in IM, combine some commands
- add post commissioning setup commands (trusted node ids etc.)

* Restyled by gn

* Address review comments.

* Add back some lines lost in a rebase.

* Address review comments

- remove heap allocation from callbacks
- overwrite node ID resolution functions rather than using a delegate

* Restyled by gn

* Restyled by shfmt

Co-authored-by: Restyled.io <[email protected]>
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…p#7254)

PR project-chip#6798 added another call to "advertiseOpertional()" which
starts publishing the opertional node service on a receipt
of a valid EnableNetwork command. Consequently, the code
which publishes the same service on a change in the network
configuration became redundant and actually harmful due to
some issues with updating an SRP service.
@cecille cecille deleted the commissioning branch April 1, 2022 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

on-network pairing for chip-device-tool
9 participants