Refactor kReadCommissioningInfo step in DeviceCommissioner

src/controller/AutoCommissioner.cpp

@@ -359,8 +359,6 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
             // Per the spec, we restart from after adding the NOC.
             return GetNextCommissioningStage(CommissioningStage::kSendNOC, lastErr);
         }
-        return CommissioningStage::kReadCommissioningInfo2;
-    case CommissioningStage::kReadCommissioningInfo2:
         return CommissioningStage::kArmFailsafe;
     case CommissioningStage::kArmFailsafe:
         return CommissioningStage::kConfigRegulatory;
@@ -764,9 +762,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
         ChipLogProgress(Controller, "Successfully finished commissioning step '%s'", StageToString(report.stageCompleted));
         switch (report.stageCompleted)
         {
-        case CommissioningStage::kReadCommissioningInfo:
-            break;
-        case CommissioningStage::kReadCommissioningInfo2: {
+        case CommissioningStage::kReadCommissioningInfo: {
             mDeviceCommissioningInfo = report.Get<ReadCommissioningInfo>();
 
             if (!mParams.GetFailsafeTimerSeconds().HasValue() && mDeviceCommissioningInfo.general.recommendedFailsafe > 0)

src/controller/CHIPDeviceController.h

@@ -839,7 +839,9 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     CommissioneeDeviceProxy * mDeviceInPASEEstablishment = nullptr;
 
     CommissioningStage mCommissioningStage = CommissioningStage::kSecurePairing;
-    bool mRunCommissioningAfterConnection  = false;
+    uint8_t mReadCommissioningInfoProgress = 0;
+
+    bool mRunCommissioningAfterConnection = false;
     Internal::InvokeCancelFn mInvokeCancelFn;
     Internal::WriteCancelFn mWriteCancelFn;
 
@@ -1050,16 +1052,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     void CancelCASECallbacks();
 
 #if CHIP_CONFIG_ENABLE_READ_CLIENT
-    void ParseCommissioningInfo();
-    // 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
+    void ContinueReadingCommissioningInfo(const CommissioningParameters & params);
+    void FinishReadingCommissioningInfo();
+    CHIP_ERROR ParseGeneralCommissioningInfo(ReadCommissioningInfo & info);
+    CHIP_ERROR ParseBasicInformation(ReadCommissioningInfo & info);
+    CHIP_ERROR ParseNetworkCommissioningInfo(ReadCommissioningInfo & info);
     CHIP_ERROR ParseFabrics(ReadCommissioningInfo & info);
     CHIP_ERROR ParseICDInfo(ReadCommissioningInfo & info);
-    // Called by ParseCommissioningInfo
-    void ParseTimeSyncInfo(ReadCommissioningInfo & info);
+    CHIP_ERROR ParseTimeSyncInfo(ReadCommissioningInfo & info);
 #endif // CHIP_CONFIG_ENABLE_READ_CLIENT
 
     static CHIP_ERROR

src/controller/CommissioningDelegate.cpp

@@ -34,9 +34,6 @@ const char * StageToString(CommissioningStage stage)
     case kReadCommissioningInfo:
         return "ReadCommissioningInfo";
 
-    case kReadCommissioningInfo2:
-        return "ReadCommissioningInfo2";
-
     case kArmFailsafe:
         return "ArmFailSafe";
 
@@ -164,9 +161,6 @@ const char * MetricKeyForCommissioningStage(CommissioningStage stage)
     case kReadCommissioningInfo:
         return "core_commissioning_stage_read_commissioning_info";
 
-    case kReadCommissioningInfo2:
-        return "core_commissioning_stage_read_commissioning_info2";
-
     case kArmFailsafe:
         return "core_commissioning_stage_arm_failsafe";
 

src/controller/CommissioningDelegate.h

@@ -36,8 +36,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
-    kReadCommissioningInfo2,     ///< Query SupportsConcurrentConnection, ICD state, check for matching fabric
+    kReadCommissioningInfo,      ///< Query Attributes relevant to commissioning (can perform multiple read interactions)
     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
@@ -55,7 +54,7 @@ enum CommissioningStage : uint8_t
     kSendTrustedRootCert,        ///< Send AddTrustedRootCertificate (0x3E:11) command to the device
     kSendNOC,                    ///< Send AddNOC (0x3E:6) command to the device
     kConfigureTrustedTimeSource, ///< Configure a trusted time source if one is required and available (must be done after SendNOC)
-    kICDGetRegistrationInfo,     ///< Waiting for the higher layer to provide ICD registraion informations.
+    kICDGetRegistrationInfo,     ///< Waiting for the higher layer to provide ICD registration informations.
     kICDRegistration,            ///< Register for ICD management
     kWiFiNetworkSetup,           ///< Send AddOrUpdateWiFiNetwork (0x31:2) command to the device
     kThreadNetworkSetup,         ///< Send AddOrUpdateThreadNetwork (0x31:3) command to the device
@@ -163,7 +162,7 @@ class CommissioningParameters
     }
 
     // Value to determine whether the node supports Concurrent Connections as read from the GeneralCommissioning cluster.
-    // In the AutoCommissioner, this is automatically set from from the kReadCommissioningInfo2 stage.
+    // In the AutoCommissioner, this is automatically set from from the kReadCommissioningInfo stage.
     Optional<bool> GetSupportsConcurrentConnection() const { return mSupportsConcurrentConnection; }
 
     // The country code to be used for the node, if set.
@@ -691,7 +690,7 @@ struct OperationalNodeFoundData
 struct NetworkClusterInfo
 {
     EndpointId endpoint = kInvalidEndpointId;
-    app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::TypeInfo::DecodableType minConnectionTime;
+    app::Clusters::NetworkCommissioning::Attributes::ConnectMaxTimeSeconds::TypeInfo::DecodableType minConnectionTime = 0;
 };
 struct NetworkClusters
 {
@@ -788,8 +787,7 @@ class CommissioningDelegate
 public:
     virtual ~CommissioningDelegate(){};
     /* CommissioningReport is returned after each commissioning step is completed. The reports for each step are:
-     * kReadCommissioningInfo: Reported together with ReadCommissioningInfo2
-     * kReadCommissioningInfo2: ReadCommissioningInfo
+     * kReadCommissioningInfo: ReadCommissioningInfo
      * kArmFailsafe: CommissioningErrorInfo if there is an error
      * kConfigRegulatory: CommissioningErrorInfo if there is an error
      * kConfigureUTCTime: None

src/controller/python/OpCredsBinding.cpp

@@ -162,7 +162,7 @@ class TestCommissioner : public chip::Controller::AutoCommissioner
                 mCompletionError = err;
             }
         }
-        if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo2)
+        if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo)
         {
             mReadCommissioningInfo = report.Get<chip::Controller::ReadCommissioningInfo>();
         }

src/controller/python/test/test_scripts/commissioning_failure_test.py

@@ -97,7 +97,8 @@ async def main():
 
     # TODO: Start at stage 2 once handling for arming failsafe on pase is done.
     if options.report:
-        for testFailureStage in range(3, 21):
+        # [kArmFailsafe, kConfigureTrustedTimeSource] (inclusive). TODO: https://github.com/project-chip/connectedhomeip/issues/36629
+        for testFailureStage in range(3, 20):
             FailIfNot(await test.TestPaseOnly(ip=options.deviceAddress1,
                                               setuppin=20202021,
                                               nodeid=1),
@@ -106,7 +107,8 @@ async def main():
                       "Commissioning failure tests failed for simulated report failure on stage {}".format(testFailureStage))
 
     else:
-        for testFailureStage in range(3, 21):
+        # [kArmFailsafe, kConfigureTrustedTimeSource] (inclusive). TODO: https://github.com/project-chip/connectedhomeip/issues/36629
+        for testFailureStage in range(3, 20):
             FailIfNot(await test.TestPaseOnly(ip=options.deviceAddress1,
                                               setuppin=20202021,
                                               nodeid=1),

src/darwin/Framework/CHIPTests/MTRPairingTests.m

@@ -42,29 +42,41 @@
 // A no-op MTRDeviceAttestationDelegate which lets us test (by default, in CI)
 // commissioning flows that have such a delegate.
 @interface NoOpAttestationDelegate : NSObject <MTRDeviceAttestationDelegate>
-@property (nonatomic) XCTestExpectation * expectation;
-@property (nonatomic) BOOL blockCommissioning;
 
-- (instancetype)initWithExpectation:(XCTestExpectation *)expectation;
+// The expectation will be fulfilled from deviceAttestationCompletedForController:...
+- (instancetype)initWithExpectation:(nullable XCTestExpectation *)expectation;
+
 // If blockCommissioning is YES, this delegate will never proceed from
 // its attestation verification callback.
-- (instancetype)initWithExpectation:(XCTestExpectation *)expectation blockCommissioning:(BOOL)blockCommissioning;
+- (instancetype)initWithExpectation:(nullable XCTestExpectation *)expectation blockCommissioning:(BOOL)blockCommissioning;
+
+// The callback will be called from deviceAttestationCompletedForController:...
+- (instancetype)initWithCallback:(void (^_Nullable)(void))callback blockCommissioning:(BOOL)blockCommissioning;
+
 @end
 
-@implementation NoOpAttestationDelegate
+@implementation NoOpAttestationDelegate {
+    void (^_Nullable _callback)(void);
+    BOOL _blockCommissioning;
+}
 
 - (instancetype)initWithExpectation:(XCTestExpectation *)expectation
 {
     return [self initWithExpectation:expectation blockCommissioning:NO];
 }
 
 - (instancetype)initWithExpectation:(XCTestExpectation *)expectation blockCommissioning:(BOOL)blockCommissioning;
+{
+    return [self initWithCallback:^{ [expectation fulfill]; } blockCommissioning:blockCommissioning];
+}
+
+- (instancetype)initWithCallback:(void (^)(void))callback blockCommissioning:(BOOL)blockCommissioning
 {
     if (!(self = [super init])) {
         return nil;
     }
 
-    _expectation = expectation;
+    _callback = callback;
     _blockCommissioning = blockCommissioning;
     return self;
 }
@@ -74,14 +86,17 @@ - (void)deviceAttestationCompletedForController:(MTRDeviceController *)controlle
                           attestationDeviceInfo:(MTRDeviceAttestationDeviceInfo *)attestationDeviceInfo
                                           error:(NSError * _Nullable)error
 {
-    [self.expectation fulfill];
     // Hard-coded to what our example server app uses for now.
     XCTAssertEqualObjects(attestationDeviceInfo.vendorID, @(0xFFF2));
     XCTAssertEqualObjects(attestationDeviceInfo.productID, @(0x8001));
     XCTAssertEqualObjects(attestationDeviceInfo.basicInformationVendorID, @(0xFFF1));
     XCTAssertEqualObjects(attestationDeviceInfo.basicInformationProductID, @(0x8000));
 
-    if (!self.blockCommissioning) {
+    if (_callback) {
+        _callback();
+    }
+
+    if (!_blockCommissioning) {
         [controller continueCommissioningDevice:opaqueDeviceHandle ignoreAttestationFailure:NO error:nil];
     }
 }
@@ -355,19 +370,12 @@ - (void)doPairingAndWaitForProgress:(NSString *)trigger attestationDelegate:(nul
     MTRSetLogCallback(MTRLogTypeDetail, ^(MTRLogType type, NSString * moduleName, NSString * message) {
         if ([message containsString:trigger]) {
             [expectation fulfill];
+            [NSThread sleepForTimeInterval:0.5]; // yield and give the test thread a head start
         }
     });
 
-    XCTestExpectation * attestationExpectation;
-    if (attestationDelegate == nil) {
-        attestationExpectation = [self expectationWithDescription:@"Attestation delegate called"];
-        attestationDelegate = [[NoOpAttestationDelegate alloc] initWithExpectation:attestationExpectation];
-    }
-
-    // Make sure we exercise the codepath that has an attestation delegate and
-    // extends the fail-safe while waiting for that delegate.  And make sure our
-    // fail-safe extension is long enough that we actually trigger a fail-safe
-    // extension (so longer than the 1-minute default).
+    // If there is an attestation delegate, make sure sure our fail-safe extension is long
+    // enough that we actually trigger a fail-safe extension (so longer than the 1-minute default).
     __auto_type * controllerDelegate = [[MTRPairingTestControllerDelegate alloc] initWithExpectation:nil
                                                                                  attestationDelegate:attestationDelegate
                                                                                    failSafeExtension:@(90)];
@@ -380,11 +388,10 @@ - (void)doPairingAndWaitForProgress:(NSString *)trigger attestationDelegate:(nul
     XCTAssertTrue([sController setupCommissioningSessionWithPayload:payload newNodeID:@(++sDeviceId) error:&error]);
     XCTAssertNil(error);
 
+    // Wait for the trigger message and then return to the caller
+    // Don't wait for anything else here, since the pairing process is going to
+    // continue asynchronously, and the caller may want to cancel it in a specific state.
     [self waitForExpectations:@[ expectation ] timeout:kPairingTimeoutInSeconds];
-
-    if (attestationExpectation) {
-        [self waitForExpectations:@[ attestationExpectation ] timeout:kTimeoutInSeconds];
-    }
     MTRSetLogCallback(0, nil);
 }
 
@@ -438,12 +445,13 @@ - (void)test008_pairingAfterCancellation_DeviceAttestationVerification
 {
     // Cancel pairing while we are waiting for our client to decide what to do
     // with the attestation information we got.
-    XCTestExpectation * attestationExpectation = [self expectationWithDescription:@"Blocking attestation delegate called"];
-    __auto_type * attestationDelegate = [[NoOpAttestationDelegate alloc] initWithExpectation:attestationExpectation blockCommissioning:YES];
+    __block BOOL delegateCalled = NO;
+    __auto_type * attestationDelegate = [[NoOpAttestationDelegate alloc] initWithCallback:^{
+        delegateCalled = YES;
+    } blockCommissioning:YES];
 
     [self doPairingTestAfterCancellationAtProgress:@"Successfully extended fail-safe timer to handle DA failure" attestationDelegate:attestationDelegate];
-
-    [self waitForExpectations:@[ attestationExpectation ] timeout:kTimeoutInSeconds];
+    XCTAssertTrue(delegateCalled);
 }
 
 @end

src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase+ServerAppRunner.m

@@ -61,7 +61,7 @@ + (NSTask *)doStartAppWithName:(NSString *)name arguments:(NSArray<NSString *> *
         @"--passcode",
         [NSString stringWithFormat:@"%llu", passcode.unsignedLongLongValue],
         @"--KVS",
-        [NSString stringWithFormat:@"/tmp/chip-%@-kvs%u", name, uniqueIndex],
+        [NSString stringWithFormat:@"/tmp/xctest-%d-chip-%@-kvs%u", getpid(), name, uniqueIndex],
         @"--product-id",
         [NSString stringWithFormat:@"%u", parsedPayload.productID.unsignedShortValue],
     ];

src/python_testing/TC_CGEN_2_4.py

@@ -50,14 +50,15 @@
 from mobly import asserts
 
 # Commissioning stage numbers - we should find a better way to match these to the C++ code
-kArmFailsafe = 4
-kConfigRegulatory = 5
-kSendPAICertificateRequest = 10
-kSendDACCertificateRequest = 11
-kSendAttestationRequest = 12
-kSendOpCertSigningRequest = 15
-kSendTrustedRootCert = 18
-kSendNOC = 19
+# TODO: https://github.com/project-chip/connectedhomeip/issues/36629
+kArmFailsafe = 3
+kConfigRegulatory = 4
+kSendPAICertificateRequest = 9
+kSendDACCertificateRequest = 10
+kSendAttestationRequest = 11
+kSendOpCertSigningRequest = 14
+kSendTrustedRootCert = 17
+kSendNOC = 18
 
 
 class TC_CGEN_2_4(MatterBaseTest):