[icd] Add steps for checking ICD during commissioning (project-chip#2…

…9427) * [icd] Add steps for checking ICD during commissioning * Add TODO for attribute used & rename stage to IcdIdentification * Update * Fix -- missing endpoint * Update * Update * Update * Fix * Fix * ReadCommissioningInfo2 may complete with enpty report
shripad621git · Oct 31, 2023 · 1dabcd3 · 1dabcd3
1 parent 85e704a
commit 1dabcd3
Show file tree

Hide file tree

Showing 7 changed files with 151 additions and 37 deletions.
diff --git 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<MatchingFabricInfo>().nodeId;
-            if (nodeId != kUndefinedNodeId)
+        case CommissioningStage::kReadCommissioningInfo2: {
+            bool shouldReadCommissioningInfo2 =
+                mParams.GetCheckForMatchingFabric() || (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore);
+            if (shouldReadCommissioningInfo2)
             {
-                mParams.SetRemoteNodeId(nodeId);
+                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>();
+
+                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
@@ -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
@@ -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<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) {
@@ -2170,9 +2187,43 @@ void DeviceCommissioner::ParseFabrics()
         mPairingDelegate->OnFabricCheck(info.nodeId);
     }
 
-    CommissioningDelegate::CommissioningReport report;
-    report.Set<MatchingFabricInfo>(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<IcdManagement::Attributes::FeatureMap::TypeInfo>(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
@@ -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
@@ -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
@@ -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<bool> mAttemptWiFiNetworkScan;
     Optional<bool> mAttemptThreadNetworkScan; // This automatically gets set to false when a ThreadOperationalDataset is set
     Optional<bool> 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<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData,
-                                         ReadCommissioningInfo, AttestationErrorInfo, CommissioningErrorInfo,
-                                         NetworkCommissioningStatusInfo, MatchingFabricInfo, TimeZoneResponseInfo>
+                                         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: