diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 72f47fe1e9ad4a..b97b4d544b7b4e 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -273,12 +273,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio // Per the spec, we restart from after adding the NOC. return GetNextCommissioningStage(CommissioningStage::kSendNOC, lastErr); } - if (mParams.GetCheckForMatchingFabric()) - { - return CommissioningStage::kCheckForMatchingFabric; - } - return CommissioningStage::kArmFailsafe; - case CommissioningStage::kCheckForMatchingFabric: + return CommissioningStage::kReadCommissioningInfo2; + case CommissioningStage::kReadCommissioningInfo2: return CommissioningStage::kArmFailsafe; case CommissioningStage::kArmFailsafe: return CommissioningStage::kConfigRegulatory; @@ -666,11 +662,37 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio // Don't send DST unless the device says it needs it mNeedsDST = false; break; - case CommissioningStage::kCheckForMatchingFabric: { - chip::NodeId nodeId = report.Get().nodeId; - if (nodeId != kUndefinedNodeId) + case CommissioningStage::kReadCommissioningInfo2: { + bool shouldReadCommissioningInfo2 = + mParams.GetCheckForMatchingFabric() || (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore); + if (shouldReadCommissioningInfo2) { - mParams.SetRemoteNodeId(nodeId); + if (!report.Is()) + { + ChipLogError( + Controller, + "[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG."); + } + + ReadCommissioningInfo2 commissioningInfo = report.Get(); + + if (mParams.GetCheckForMatchingFabric()) + { + chip::NodeId nodeId = commissioningInfo.nodeId; + if (nodeId != kUndefinedNodeId) + { + mParams.SetRemoteNodeId(nodeId); + } + } + + if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore) + { + if (commissioningInfo.isIcd) + { + mNeedIcdRegistraion = true; + ChipLogDetail(Controller, "AutoCommissioner: Device is ICD"); + } + } } break; } diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 4bf6217ee4c019..2e13445a113f5d 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -117,6 +117,8 @@ class AutoCommissioner : public CommissioningDelegate ReadCommissioningInfo mDeviceCommissioningInfo; bool mNeedsDST = false; + bool mNeedIcdRegistraion = false; + // TODO: Why were the nonces statically allocated, but the certs dynamically allocated? uint8_t * mDAC = nullptr; uint16_t mDACLen = 0; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index d4b958a5418c0f..87301129dbe743 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1877,8 +1877,8 @@ void DeviceCommissioner::OnDone(app::ReadClient *) case CommissioningStage::kReadCommissioningInfo: ParseCommissioningInfo(); break; - case CommissioningStage::kCheckForMatchingFabric: - ParseFabrics(); + case CommissioningStage::kReadCommissioningInfo2: + ParseCommissioningInfo2(); break; default: // We're not trying to read anything here, just exit @@ -2103,11 +2103,28 @@ void DeviceCommissioner::ParseTimeSyncInfo(ReadCommissioningInfo & info) } } -void DeviceCommissioner::ParseFabrics() +void DeviceCommissioner::ParseCommissioningInfo2() +{ + ReadCommissioningInfo2 info; + CHIP_ERROR return_err = CHIP_NO_ERROR; + + return_err = ParseFabrics(info); + + if (return_err == CHIP_NO_ERROR) + { + return_err = ParseICDInfo(info); + } + + CommissioningDelegate::CommissioningReport report; + report.Set(info); + CommissioningStageComplete(return_err, report); +} + +CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo2 & info) { CHIP_ERROR err; CHIP_ERROR return_err = CHIP_NO_ERROR; - MatchingFabricInfo info; + // We might not have requested a Fabrics attribute at all, so not having a // value for it is not an error. err = mAttributeCache->ForEachAttribute(OperationalCredentials::Id, [this, &info](const app::ConcreteAttributePath & path) { @@ -2170,9 +2187,43 @@ void DeviceCommissioner::ParseFabrics() mPairingDelegate->OnFabricCheck(info.nodeId); } - CommissioningDelegate::CommissioningReport report; - report.Set(info); - CommissioningStageComplete(return_err, report); + return return_err; +} + +CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info) +{ + CHIP_ERROR err; + IcdManagement::Attributes::FeatureMap::TypeInfo::DecodableType featureMap; + + err = mAttributeCache->Get(kRootEndpointId, featureMap); + if (err == CHIP_NO_ERROR) + { + info.isIcd = true; + info.checkInProtocolSupport = !!(featureMap & to_underlying(IcdManagement::Feature::kCheckInProtocolSupport)); + } + else if (err == CHIP_ERROR_KEY_NOT_FOUND) + { + info.isIcd = false; + } + else if (err == CHIP_ERROR_IM_STATUS_CODE_RECEIVED) + { + app::StatusIB statusIB; + err = mAttributeCache->GetStatus( + app::ConcreteAttributePath(kRootEndpointId, IcdManagement::Id, IcdManagement::Attributes::FeatureMap::Id), statusIB); + if (err == CHIP_NO_ERROR) + { + if (statusIB.mStatus == Protocols::InteractionModel::Status::UnsupportedCluster) + { + info.isIcd = false; + } + else + { + err = statusIB.ToChipError(); + } + } + } + + return err; } void DeviceCommissioner::OnArmFailSafe(void * context, @@ -2414,19 +2465,39 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio SendCommissioningReadRequest(proxy, timeout, readPaths, 9); } break; - case CommissioningStage::kCheckForMatchingFabric: { + case CommissioningStage::kReadCommissioningInfo2: { + ChipLogProgress(Controller, "Sending request for commissioning information -- Part 2"); + + size_t numberOfAttributes = 0; + // This is done in a separate step since we've already used up all the available read paths in the previous read step + app::AttributePathParams readPaths[9]; + // Read the current fabrics - if (!params.GetCheckForMatchingFabric()) + if (params.GetCheckForMatchingFabric()) + { + readPaths[numberOfAttributes++] = + app::AttributePathParams(OperationalCredentials::Id, OperationalCredentials::Attributes::Fabrics::Id); + } + + if (params.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore) + { + readPaths[numberOfAttributes++] = + app::AttributePathParams(endpoint, IcdManagement::Id, IcdManagement::Attributes::FeatureMap::Id); + } + + // Current implementation makes sense when we only have a few attributes to read with conditions. Should revisit this if we + // are adding more attributes here. + + if (numberOfAttributes == 0) { // We don't actually want to do this step, so just bypass it - ChipLogProgress(Controller, "kCheckForMatchingFabric step called without parameter set, skipping"); + ChipLogProgress(Controller, "kReadCommissioningInfo2 step called without parameter set, skipping"); CommissioningStageComplete(CHIP_NO_ERROR); } - - // This is done in a separate step since we've already used up all the available read paths in the previous read step - app::AttributePathParams readPaths[1]; - readPaths[0] = app::AttributePathParams(OperationalCredentials::Id, OperationalCredentials::Attributes::Fabrics::Id); - SendCommissioningReadRequest(proxy, timeout, readPaths, 1); + else + { + SendCommissioningReadRequest(proxy, timeout, readPaths, numberOfAttributes); + } } break; case CommissioningStage::kConfigureUTCTime: { diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index a83ca98fca5f3d..9d6a9d6f5a897c 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -935,7 +935,10 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, #if CHIP_CONFIG_ENABLE_READ_CLIENT // Parsers for the two different read clients void ParseCommissioningInfo(); - void ParseFabrics(); + void ParseCommissioningInfo2(); + // Called by ParseCommissioningInfo2 + CHIP_ERROR ParseFabrics(ReadCommissioningInfo2 & info); + CHIP_ERROR ParseICDInfo(ReadCommissioningInfo2 & info); // Called by ParseCommissioningInfo void ParseTimeSyncInfo(ReadCommissioningInfo & info); #endif // CHIP_CONFIG_ENABLE_READ_CLIENT diff --git a/src/controller/CommissioningDelegate.cpp b/src/controller/CommissioningDelegate.cpp index 7100ee0865b392..e19727be9e510b 100644 --- a/src/controller/CommissioningDelegate.cpp +++ b/src/controller/CommissioningDelegate.cpp @@ -37,8 +37,8 @@ const char * StageToString(CommissioningStage stage) return "ReadCommissioningInfo"; break; - case kCheckForMatchingFabric: - return "CheckForMatchingFabric"; + case kReadCommissioningInfo2: + return "ReadCommissioningInfo2"; break; case kArmFailsafe: diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 3ccfd87084dab2..bb95f5eb8fe8f2 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -35,7 +35,7 @@ enum CommissioningStage : uint8_t kError, kSecurePairing, ///< Establish a PASE session with the device kReadCommissioningInfo, ///< Query General Commissioning Attributes, Network Features and Time Synchronization Cluster - kCheckForMatchingFabric, ///< Read the current fabrics on the commissionee + kReadCommissioningInfo2, ///< Query ICD state, check for matching fabric kArmFailsafe, ///< Send ArmFailSafe (0x30:0) command to the device kConfigRegulatory, ///< Send SetRegulatoryConfig (0x30:2) command to the device kConfigureUTCTime, ///< SetUTCTime if the DUT has a time cluster @@ -71,6 +71,13 @@ enum CommissioningStage : uint8_t kNeedsNetworkCreds, }; +enum ICDRegistrationStrategy : uint8_t +{ + kIgnore, ///< Do not check whether the device is an ICD during commissioning + kBeforeComplete, ///< Do commissioner self-registration or external controller registration, + ///< Controller should provide a ICDKey manager for generating symmetric key +}; + const char * StageToString(CommissioningStage stage); struct WiFiCredentials @@ -493,6 +500,13 @@ class CommissioningParameters return *this; } + ICDRegistrationStrategy GetICDRegistrationStrategy() const { return mICDRegistrationStrategy; } + CommissioningParameters & SetICDRegistrationStrategy(ICDRegistrationStrategy icdRegistrationStrategy) + { + mICDRegistrationStrategy = icdRegistrationStrategy; + return *this; + } + // Clear all members that depend on some sort of external buffer. Can be // used to make sure that we are not holding any dangling pointers. void ClearExternalBufferDependentValues() @@ -552,7 +566,8 @@ class CommissioningParameters Optional mAttemptWiFiNetworkScan; Optional mAttemptThreadNetworkScan; // This automatically gets set to false when a ThreadOperationalDataset is set Optional mSkipCommissioningComplete; - bool mCheckForMatchingFabric = false; + ICDRegistrationStrategy mICDRegistrationStrategy = ICDRegistrationStrategy::kIgnore; + bool mCheckForMatchingFabric = false; }; struct RequestedCertificate @@ -634,9 +649,12 @@ struct ReadCommissioningInfo uint8_t maxTimeZoneSize = 1; uint8_t maxDSTSize = 1; }; -struct MatchingFabricInfo + +struct ReadCommissioningInfo2 { - NodeId nodeId = kUndefinedNodeId; + NodeId nodeId = kUndefinedNodeId; + bool isIcd = false; + bool checkInProtocolSupport = false; }; struct TimeZoneResponseInfo @@ -670,7 +688,7 @@ class CommissioningDelegate virtual ~CommissioningDelegate(){}; /* CommissioningReport is returned after each commissioning step is completed. The reports for each step are: * kReadCommissioningInfo - ReadCommissioningInfo - * kCheckForMatchingFabric = MatchingFabricInfo + * kReadCommissioningInfo2: ReadCommissioningInfo2 * kArmFailsafe: CommissioningErrorInfo if there is an error * kConfigRegulatory: CommissioningErrorInfo if there is an error * kConfigureUTCTime: None @@ -695,8 +713,8 @@ class CommissioningDelegate * kCleanup: none */ struct CommissioningReport : Variant + ReadCommissioningInfo, ReadCommissioningInfo2, AttestationErrorInfo, + CommissioningErrorInfo, NetworkCommissioningStatusInfo, TimeZoneResponseInfo> { CommissioningReport() : stageCompleted(CommissioningStage::kError) {} CommissioningStage stageCompleted; diff --git a/src/controller/python/OpCredsBinding.cpp b/src/controller/python/OpCredsBinding.cpp index f78b0135e1f2c4..03f4ca0606975f 100644 --- a/src/controller/python/OpCredsBinding.cpp +++ b/src/controller/python/OpCredsBinding.cpp @@ -311,8 +311,6 @@ class TestCommissioner : public chip::Controller::AutoCommissioner { switch (stage) { - case chip::Controller::CommissioningStage::kCheckForMatchingFabric: - return mParams.GetCheckForMatchingFabric(); case chip::Controller::CommissioningStage::kWiFiNetworkEnable: case chip::Controller::CommissioningStage::kFailsafeBeforeWiFiEnable: case chip::Controller::CommissioningStage::kWiFiNetworkSetup: