Fix

project-chip · Oct 14, 2023 · 1064358 · 1064358
1 parent b244d12
commit 1064358
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 56 deletions.
diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
@@ -631,11 +631,25 @@ 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<MatchingFabricInfo>().nodeId;
-            if (nodeId != kUndefinedNodeId)
+        case CommissioningStage::kReadCommissioningInfo2: {
+            ReadCommissioningInfo2 commissioningInfo = report.Get<ReadCommissioningInfo2>();
+
+            if (mParams.GetCheckForMatchingFabric())
+            {
+                chip::NodeId nodeId = commissioningInfo.nodeId;
+                if (nodeId != kUndefinedNodeId)
+                {
+                    mParams.SetRemoteNodeId(nodeId);
+                }
+            }
+
+            if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
             {
-                mParams.SetRemoteNodeId(nodeId);
+                if (commissioningInfo.isIcd)
+                {
+                    mNeedIcdRegistraion = true;
+                    ChipLogDetail(Controller, "AutoCommissioner: Device is ICD");
+                }
             }
             break;
         }
@@ -701,15 +715,6 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
         case CommissioningStage::kFindOperational:
             mOperationalDeviceProxy = report.Get<OperationalNodeFoundData>().operationalProxy;
             break;
-        case CommissioningStage::kICDIdentification: {
-            IcdInfo icdInfo = report.Get<IcdInfo>();
-            if (icdInfo.isIcd)
-            {
-                mNeedIcdRegistraion = true;
-                ChipLogDetail(Controller, "AutoCommissioner: Device is ICD");
-            }
-            break;
-        }
         case CommissioningStage::kCleanup:
             ReleasePAI();
             ReleaseDAC();

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -1878,14 +1878,7 @@ void DeviceCommissioner::OnDone(app::ReadClient *)
         ParseCommissioningInfo();
         break;
     case CommissioningStage::kReadCommissioningInfo2:
-        if (param.GetCheckForMatchingFabric())
-        {
-            ParseFabrics();
-        }
-        if (param.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
-        {
-            ParseICDInfo();
-        }
+        ParseCommissioningInfo2();
         break;
     default:
         // We're not trying to read anything here, just exit
@@ -2110,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<ReadCommissioningInfo2>(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) {
@@ -2177,15 +2187,12 @@ void DeviceCommissioner::ParseFabrics()
         mPairingDelegate->OnFabricCheck(info.nodeId);
     }
 
-    CommissioningDelegate::CommissioningReport report;
-    report.Set<MatchingFabricInfo>(info);
-    CommissioningStageComplete(return_err, report);
+    return return_err;
 }
 
-void DeviceCommissioner::ParseICDInfo()
+CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info)
 {
     CHIP_ERROR err;
-    IcdInfo info;
     IcdManagement::Attributes::FeatureMap::TypeInfo::DecodableType featureMap;
 
     err = mAttributeCache->Get<IcdManagement::Attributes::FeatureMap::TypeInfo>(kRootEndpointId, featureMap);
@@ -2194,6 +2201,10 @@ void DeviceCommissioner::ParseICDInfo()
         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;
@@ -2212,12 +2223,7 @@ void DeviceCommissioner::ParseICDInfo()
         }
     }
 
-    CommissioningDelegate::CommissioningReport report;
-    if (err == CHIP_NO_ERROR)
-    {
-        report.Set<IcdInfo>(info);
-    }
-    CommissioningStageComplete(err, report);
+    return err;
 }
 
 void DeviceCommissioner::OnArmFailSafe(void * context,
@@ -2482,7 +2488,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         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);
         }
 
@@ -2842,14 +2848,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         );
     }
     break;
-    case CommissioningStage::kICDIdentification: {
-        app::AttributePathParams readPaths[1];
-        // TODO(#29382): We probably want to read "ActiveMode" attribute (to be implemented) for ICD.
-        readPaths[0] = app::AttributePathParams(endpoint, app::Clusters::IcdManagement::Id,
-                                                app::Clusters::IcdManagement::Attributes::FeatureMap::Id);
-        SendCommissioningReadRequest(proxy, timeout, readPaths, 1);
-    }
-    break;
     case CommissioningStage::kSendComplete: {
         GeneralCommissioning::Commands::CommissioningComplete::Type request;
         SendCommand(proxy, request, OnCommissioningCompleteResponse, OnBasicFailure, endpoint, timeout);

diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -935,10 +935,12 @@ 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);
-    void ParseICDInfo();
 #endif // CHIP_CONFIG_ENABLE_READ_CLIENT
 
     static CHIP_ERROR

diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h
@@ -648,17 +648,13 @@ struct ReadCommissioningInfo
     uint8_t maxDSTSize             = 1;
 };
 
-struct IcdInfo
+struct ReadCommissioningInfo2
 {
+    NodeId nodeId               = kUndefinedNodeId;
     bool isIcd                  = false;
     bool checkInProtocolSupport = false;
 };
 
-struct MatchingFabricInfo
-{
-    NodeId nodeId = kUndefinedNodeId;
-};
-
 struct TimeZoneResponseInfo
 {
     bool requiresDSTOffsets;
@@ -690,7 +686,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
@@ -715,8 +711,8 @@ class CommissioningDelegate
      * kCleanup: none
      */
     struct CommissioningReport : Variant<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData,
-                                         ReadCommissioningInfo, AttestationErrorInfo, CommissioningErrorInfo,
-                                         NetworkCommissioningStatusInfo, MatchingFabricInfo, TimeZoneResponseInfo, IcdInfo>
+                                         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
@@ -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: