From 5299982abecfb17618c8c6222cf20a5831aaf524 Mon Sep 17 00:00:00 2001 From: Song GUO Date: Thu, 14 Dec 2023 12:11:32 +0800 Subject: [PATCH] [controller] merge ReadCommissioningInfo2 into ReadCommissioningInfo (#30978) * [controller] merge ReadCommissioningInfo2 into ReadCommissioningInfo * Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky * Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky * Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky * Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky * address comment --------- Co-authored-by: yunhanw-google Co-authored-by: Boris Zbarsky --- src/controller/AutoCommissioner.cpp | 23 +++--- src/controller/CHIPDeviceController.cpp | 89 +++++++++++++++--------- src/controller/CHIPDeviceController.h | 10 +-- src/controller/CommissioningDelegate.h | 30 ++++---- src/controller/python/OpCredsBinding.cpp | 2 +- 5 files changed, 86 insertions(+), 68 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 82286e32619f31..1b286a400bccdb 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -725,6 +725,15 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio switch (report.stageCompleted) { case CommissioningStage::kReadCommissioningInfo: + break; + case CommissioningStage::kReadCommissioningInfo2: { + if (!report.Is()) + { + ChipLogError(Controller, + "[BUG] Should read commissioning info, but report is not ReadCommissioningInfo. THIS IS A BUG."); + } + ReadCommissioningInfo commissioningInfo = report.Get(); + mDeviceCommissioningInfo = report.Get(); if (!mParams.GetFailsafeTimerSeconds().HasValue() && mDeviceCommissioningInfo.general.recommendedFailsafe > 0) { @@ -736,22 +745,12 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio .SetLocationCapability(mDeviceCommissioningInfo.general.locationCapability); // Don't send DST unless the device says it needs it mNeedsDST = false; - break; - case CommissioningStage::kReadCommissioningInfo2: { - - 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(); mParams.SetSupportsConcurrentConnection(commissioningInfo.supportsConcurrentConnection); if (mParams.GetCheckForMatchingFabric()) { - chip::NodeId nodeId = commissioningInfo.nodeId; + chip::NodeId nodeId = commissioningInfo.remoteNodeId; if (nodeId != kUndefinedNodeId) { mParams.SetRemoteNodeId(nodeId); @@ -760,7 +759,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore) { - if (commissioningInfo.isIcd && commissioningInfo.checkInProtocolSupport) + if (commissioningInfo.isLIT && commissioningInfo.checkInProtocolSupport) { mNeedIcdRegistration = true; ChipLogDetail(Controller, "AutoCommissioner: ICD supports the check-in protocol."); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 49591966bd247f..e69546161e661e 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1922,25 +1922,50 @@ void DeviceCommissioner::OnDeviceConnectionRetryFn(void * context, const ScopedN // ClusterStateCache::Callback impl void DeviceCommissioner::OnDone(app::ReadClient *) { + mReadClient = nullptr; switch (mCommissioningStage) { case CommissioningStage::kReadCommissioningInfo: - ParseCommissioningInfo(); + // Silently complete the stage, data will be saved in attribute cache and + // will be parsed after all ReadCommissioningInfo stages are completed. + CommissioningStageComplete(CHIP_NO_ERROR); break; case CommissioningStage::kReadCommissioningInfo2: - ParseCommissioningInfo2(); + // Note: Only parse commissioning info in the last ReadCommissioningInfo stage. + ParseCommissioningInfo(); break; default: - // We're not trying to read anything here, just exit break; } } void DeviceCommissioner::ParseCommissioningInfo() +{ + CHIP_ERROR err = CHIP_NO_ERROR; + ReadCommissioningInfo info; + + err = ParseCommissioningInfo1(info); + if (err == CHIP_NO_ERROR) + { + err = ParseCommissioningInfo2(info); + } + + mAttributeCache = nullptr; + + if (mPairingDelegate != nullptr) + { + mPairingDelegate->OnReadCommissioningInfo(info); + } + + CommissioningDelegate::CommissioningReport report; + report.Set(info); + CommissioningStageComplete(err, report); +} + +CHIP_ERROR DeviceCommissioner::ParseCommissioningInfo1(ReadCommissioningInfo & info) { CHIP_ERROR err; CHIP_ERROR return_err = CHIP_NO_ERROR; - ReadCommissioningInfo info; // Try to parse as much as we can here before returning, even if attributes // are missing or cannot be decoded. @@ -2080,17 +2105,8 @@ void DeviceCommissioner::ParseCommissioningInfo() { ChipLogError(Controller, "Error parsing commissioning information"); } - mAttributeCache = nullptr; - mReadClient = nullptr; - - if (mPairingDelegate != nullptr) - { - mPairingDelegate->OnReadCommissioningInfo(info); - } - CommissioningDelegate::CommissioningReport report; - report.Set(info); - CommissioningStageComplete(return_err, report); + return return_err; } void DeviceCommissioner::ParseTimeSyncInfo(ReadCommissioningInfo & info) @@ -2153,34 +2169,31 @@ void DeviceCommissioner::ParseTimeSyncInfo(ReadCommissioningInfo & info) } } -void DeviceCommissioner::ParseCommissioningInfo2() +CHIP_ERROR DeviceCommissioner::ParseCommissioningInfo2(ReadCommissioningInfo & info) { - ReadCommissioningInfo2 info; - CHIP_ERROR return_err = CHIP_NO_ERROR; + CHIP_ERROR err = CHIP_NO_ERROR; using namespace chip::app::Clusters::GeneralCommissioning::Attributes; - CHIP_ERROR err = - mAttributeCache->Get(kRootEndpointId, info.supportsConcurrentConnection); - if (err != CHIP_NO_ERROR) + + if (mAttributeCache->Get(kRootEndpointId, info.supportsConcurrentConnection) != + CHIP_NO_ERROR) { // May not be present so don't return the error code, non fatal, default concurrent ChipLogError(Controller, "Failed to read SupportsConcurrentConnection: %" CHIP_ERROR_FORMAT, err.Format()); info.supportsConcurrentConnection = true; } - return_err = ParseFabrics(info); + err = ParseFabrics(info); - if (return_err == CHIP_NO_ERROR) + if (err == CHIP_NO_ERROR) { - return_err = ParseICDInfo(info); + err = ParseICDInfo(info); } - CommissioningDelegate::CommissioningReport report; - report.Set(info); - CommissioningStageComplete(return_err, report); + return err; } -CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo2 & info) +CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo & info) { CHIP_ERROR err; CHIP_ERROR return_err = CHIP_NO_ERROR; @@ -2230,7 +2243,7 @@ CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo2 & info) else if (commissionerRootPublicKey.Matches(deviceRootPublicKey)) { ChipLogProgress(Controller, "DeviceCommissioner::OnDone - fabric root keys match"); - info.nodeId = fabricDescriptor.nodeID; + info.remoteNodeId = fabricDescriptor.nodeID; } } } @@ -2244,13 +2257,13 @@ CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo2 & info) if (mPairingDelegate != nullptr) { - mPairingDelegate->OnFabricCheck(info.nodeId); + mPairingDelegate->OnFabricCheck(info.remoteNodeId); } return return_err; } -CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info) +CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo & info) { CHIP_ERROR err; IcdManagement::Attributes::FeatureMap::TypeInfo::DecodableType featureMap; @@ -2258,14 +2271,14 @@ CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info) err = mAttributeCache->Get(kRootEndpointId, featureMap); if (err == CHIP_NO_ERROR) { - info.isIcd = true; + info.isLIT = true; info.checkInProtocolSupport = !!(featureMap & to_underlying(IcdManagement::Feature::kCheckInProtocolSupport)); } else if (err == CHIP_ERROR_KEY_NOT_FOUND) { // This key is optional so not an error err = CHIP_NO_ERROR; - info.isIcd = false; + info.isLIT = false; err = CHIP_NO_ERROR; } else if (err == CHIP_ERROR_IM_STATUS_CODE_RECEIVED) @@ -2277,7 +2290,7 @@ CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info) { if (statusIB.mStatus == Protocols::InteractionModel::Status::UnsupportedCluster) { - info.isIcd = false; + info.isLIT = false; } else { @@ -2458,7 +2471,8 @@ void DeviceCommissioner::SendCommissioningReadRequest(DeviceProxy * proxy, Optio readParams.mpAttributePathParamsList = readPaths; readParams.mAttributePathParamsListSize = readPathsSize; - auto attributeCache = Platform::MakeUnique(*this); + // Take ownership of the attribute cache, so it can be released when SendRequest fails. + auto attributeCache = std::move(mAttributeCache); auto readClient = chip::Platform::MakeUnique( engine, proxy->GetExchangeManager(), attributeCache->GetBufferedCallback(), app::ReadClient::InteractionType::Read); CHIP_ERROR err = readClient->SendRequest(readParams); @@ -2509,6 +2523,13 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio break; case CommissioningStage::kReadCommissioningInfo: { ChipLogProgress(Controller, "Sending read request for commissioning information"); + // Allocate a new ClusterStateCache when starting reading the first batch of attributes. + // The cache will be released in: + // - SendCommissioningReadRequest when failing to send a read request. + // - ParseCommissioningInfo when the last ReadCommissioningInfo stage is completed. + // Currently, we have two ReadCommissioningInfo* stages. + mAttributeCache = Platform::MakeUnique(*this); + // NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9 // See R1.1, 2.11.2 Interaction Model Limits app::AttributePathParams readPaths[9]; diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index e013b6a9da1590..2007d8424b2f88 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -952,12 +952,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, void SendCommissioningReadRequest(DeviceProxy * proxy, Optional timeout, app::AttributePathParams * readPaths, size_t readPathsSize); #if CHIP_CONFIG_ENABLE_READ_CLIENT - // Parsers for the two different read clients void ParseCommissioningInfo(); - void ParseCommissioningInfo2(); + // Parsing attributes read in kReadCommissioningInfo stage. + CHIP_ERROR ParseCommissioningInfo1(ReadCommissioningInfo & info); + // Parsing attributes read in kReadCommissioningInfo2 stage. + CHIP_ERROR ParseCommissioningInfo2(ReadCommissioningInfo & info); // Called by ParseCommissioningInfo2 - CHIP_ERROR ParseFabrics(ReadCommissioningInfo2 & info); - CHIP_ERROR ParseICDInfo(ReadCommissioningInfo2 & info); + CHIP_ERROR ParseFabrics(ReadCommissioningInfo & info); + CHIP_ERROR ParseICDInfo(ReadCommissioningInfo & info); // Called by ParseCommissioningInfo void ParseTimeSyncInfo(ReadCommissioningInfo & info); #endif // CHIP_CONFIG_ENABLE_READ_CLIENT diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 9a7b4ebe0970ea..67fe2e69878dfb 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -684,18 +684,14 @@ struct ReadCommissioningInfo NetworkClusters network; BasicClusterInfo basic; GeneralCommissioningInfo general; - bool requiresUTC = false; - bool requiresTimeZone = false; - bool requiresDefaultNTP = false; - bool requiresTrustedTimeSource = false; - uint8_t maxTimeZoneSize = 1; - uint8_t maxDSTSize = 1; -}; - -struct ReadCommissioningInfo2 -{ - NodeId nodeId = kUndefinedNodeId; - bool isIcd = false; + bool requiresUTC = false; + bool requiresTimeZone = false; + bool requiresDefaultNTP = false; + bool requiresTrustedTimeSource = false; + uint8_t maxTimeZoneSize = 1; + uint8_t maxDSTSize = 1; + NodeId remoteNodeId = kUndefinedNodeId; + bool isLIT = false; bool checkInProtocolSupport = false; bool supportsConcurrentConnection = true; }; @@ -730,8 +726,8 @@ class CommissioningDelegate public: virtual ~CommissioningDelegate(){}; /* CommissioningReport is returned after each commissioning step is completed. The reports for each step are: - * kReadCommissioningInfo: ReadCommissioningInfo - * kReadCommissioningInfo2: ReadCommissioningInfo2 + * kReadCommissioningInfo: Reported together with ReadCommissioningInfo2 + * kReadCommissioningInfo2: ReadCommissioningInfo * kArmFailsafe: CommissioningErrorInfo if there is an error * kConfigRegulatory: CommissioningErrorInfo if there is an error * kConfigureUTCTime: None @@ -755,9 +751,9 @@ class CommissioningDelegate * kSendComplete: CommissioningErrorInfo if there is an error * kCleanup: none */ - struct CommissioningReport : Variant + struct CommissioningReport + : Variant { CommissioningReport() : stageCompleted(CommissioningStage::kError) {} CommissioningStage stageCompleted; diff --git a/src/controller/python/OpCredsBinding.cpp b/src/controller/python/OpCredsBinding.cpp index 03f4ca0606975f..79995d133b45e0 100644 --- a/src/controller/python/OpCredsBinding.cpp +++ b/src/controller/python/OpCredsBinding.cpp @@ -160,7 +160,7 @@ class TestCommissioner : public chip::Controller::AutoCommissioner mCompletionError = err; } } - if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo) + if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo2) { mReadCommissioningInfo = report.Get(); }