[controller] merge ReadCommissioningInfo2 into ReadCommissioningInfo (#30978)
* [controller] merge ReadCommissioningInfo2 into ReadCommissioningInfo
* Update src/controller/CHIPDeviceController.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/controller/CHIPDeviceController.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/controller/CHIPDeviceController.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/controller/CHIPDeviceController.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* address comment
---------
Co-authored-by: yunhanw-google <yunhanw@google.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
index 82286e3..1b286a4 100644
--- a/src/controller/AutoCommissioner.cpp
+++ b/src/controller/AutoCommissioner.cpp
@@ -725,6 +725,15 @@
switch (report.stageCompleted)
{
case CommissioningStage::kReadCommissioningInfo:
+ break;
+ case CommissioningStage::kReadCommissioningInfo2: {
+ if (!report.Is<ReadCommissioningInfo>())
+ {
+ ChipLogError(Controller,
+ "[BUG] Should read commissioning info, but report is not ReadCommissioningInfo. THIS IS A BUG.");
+ }
+ ReadCommissioningInfo commissioningInfo = report.Get<ReadCommissioningInfo>();
+
mDeviceCommissioningInfo = report.Get<ReadCommissioningInfo>();
if (!mParams.GetFailsafeTimerSeconds().HasValue() && mDeviceCommissioningInfo.general.recommendedFailsafe > 0)
{
@@ -736,22 +745,12 @@
.SetLocationCapability(mDeviceCommissioningInfo.general.locationCapability);
// Don't send DST unless the device says it needs it
mNeedsDST = false;
- break;
- case CommissioningStage::kReadCommissioningInfo2: {
- if (!report.Is<ReadCommissioningInfo2>())
- {
- ChipLogError(
- Controller,
- "[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG.");
- }
-
- ReadCommissioningInfo2 commissioningInfo = report.Get<ReadCommissioningInfo2>();
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 @@
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 4959196..e695461 100644
--- a/src/controller/CHIPDeviceController.cpp
+++ b/src/controller/CHIPDeviceController.cpp
@@ -1922,25 +1922,50 @@
// 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<ReadCommissioningInfo>(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 @@
{
ChipLogError(Controller, "Error parsing commissioning information");
}
- mAttributeCache = nullptr;
- mReadClient = nullptr;
- if (mPairingDelegate != nullptr)
- {
- mPairingDelegate->OnReadCommissioningInfo(info);
- }
-
- CommissioningDelegate::CommissioningReport report;
- report.Set<ReadCommissioningInfo>(info);
- CommissioningStageComplete(return_err, report);
+ return return_err;
}
void DeviceCommissioner::ParseTimeSyncInfo(ReadCommissioningInfo & info)
@@ -2153,34 +2169,31 @@
}
}
-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<SupportsConcurrentConnection::TypeInfo>(kRootEndpointId, info.supportsConcurrentConnection);
- if (err != CHIP_NO_ERROR)
+
+ if (mAttributeCache->Get<SupportsConcurrentConnection::TypeInfo>(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<ReadCommissioningInfo2>(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 @@
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 @@
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 @@
err = mAttributeCache->Get<IcdManagement::Attributes::FeatureMap::TypeInfo>(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 @@
{
if (statusIB.mStatus == Protocols::InteractionModel::Status::UnsupportedCluster)
{
- info.isIcd = false;
+ info.isLIT = false;
}
else
{
@@ -2458,7 +2471,8 @@
readParams.mpAttributePathParamsList = readPaths;
readParams.mAttributePathParamsListSize = readPathsSize;
- auto attributeCache = Platform::MakeUnique<app::ClusterStateCache>(*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<app::ReadClient>(
engine, proxy->GetExchangeManager(), attributeCache->GetBufferedCallback(), app::ReadClient::InteractionType::Read);
CHIP_ERROR err = readClient->SendRequest(readParams);
@@ -2509,6 +2523,13 @@
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<app::ClusterStateCache>(*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 e013b6a..2007d84 100644
--- a/src/controller/CHIPDeviceController.h
+++ b/src/controller/CHIPDeviceController.h
@@ -952,12 +952,14 @@
void SendCommissioningReadRequest(DeviceProxy * proxy, Optional<System::Clock::Timeout> 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 9a7b4eb..67fe2e6 100644
--- a/src/controller/CommissioningDelegate.h
+++ b/src/controller/CommissioningDelegate.h
@@ -684,18 +684,14 @@
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 @@
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 @@
* kSendComplete: CommissioningErrorInfo if there is an error
* kCleanup: none
*/
- struct CommissioningReport : Variant<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData,
- ReadCommissioningInfo, ReadCommissioningInfo2, AttestationErrorInfo,
- CommissioningErrorInfo, NetworkCommissioningStatusInfo, TimeZoneResponseInfo>
+ struct CommissioningReport
+ : Variant<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData, ReadCommissioningInfo,
+ 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 03f4ca0..79995d1 100644
--- a/src/controller/python/OpCredsBinding.cpp
+++ b/src/controller/python/OpCredsBinding.cpp
@@ -160,7 +160,7 @@
mCompletionError = err;
}
}
- if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo)
+ if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo2)
{
mReadCommissioningInfo = report.Get<chip::Controller::ReadCommissioningInfo>();
}
