Try multiple transports in SetUpCodePairer.

src/controller/CHIPDeviceController.cpp

@@ -481,7 +481,7 @@ CHIP_ERROR DeviceCommissioner::Shutdown()
     if (device != nullptr && device->IsSessionSetupInProgress())
     {
         ChipLogDetail(Controller, "Setup in progress, stopping setup before shutting down");
-        OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED);
+        PairingFailed(CHIP_ERROR_CONNECTION_ABORTED);
     }
     // TODO: If we have a commissioning step in progress, is there a way to cancel that callback?
 
@@ -812,7 +812,27 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status)
 
 void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err)
 {
-    // PASE session establishment failure.
+    // Need to null out mDeviceInPASEEstablishment before maybe trying to
+    // establish PASE again, so EstablishPASEConnection won't error out.
+    auto * oldDeviceInPASEEstablishment = mDeviceInPASEEstablishment;
+    mDeviceInPASEEstablishment          = nullptr;
+
+    if (mSetUpCodePairer.TryNextRendezvousParameters())
+    {
+        ChipLogProgress(Controller, "Ignoring PASE error; will try commissioning over a different transport");
+        // We don't need oldDeviceInPASEEstablishment anymore.
+        ReleaseCommissioneeDevice(oldDeviceInPASEEstablishment);
+        return;
+    }
+
+    // Reset mDeviceInPASEEstablishment because PairingFailed checks for that to
+    // send the right notifications.
+    mDeviceInPASEEstablishment = oldDeviceInPASEEstablishment;
+    PairingFailed(err);
+}
+
+void DeviceCommissioner::PairingFailed(CHIP_ERROR err)
+{
     mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this);
 
     if (mPairingDelegate != nullptr)
@@ -831,7 +851,7 @@ void DeviceCommissioner::OnSessionEstablished()
     // We are in the callback for this pairing. Reset so we can pair another device.
     mDeviceInPASEEstablishment = nullptr;
 
-    VerifyOrReturn(device != nullptr, OnSessionEstablishmentError(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR));
+    VerifyOrReturn(device != nullptr, PairingFailed(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR));
 
     PASESession * pairing = &device->GetPairing();
 
@@ -842,7 +862,7 @@ void DeviceCommissioner::OnSessionEstablished()
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(Controller, "Failed in setting up secure channel: err %s", ErrorStr(err));
-        OnSessionEstablishmentError(err);
+        PairingFailed(err);
         return;
     }
 

src/controller/CHIPDeviceController.h

@@ -623,6 +623,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
 
     static void OnSessionEstablishmentTimeoutCallback(System::Layer * aLayer, void * aAppState);
 
+    // Helper to call once we decide that pairing has failed.  This might be
+    // called from OnSessionEstablishmentError if we have no other transports to
+    // try, or in various other failure cases.
+    void PairingFailed(CHIP_ERROR err);
+
     /* This function sends a Device Attestation Certificate chain request to the device.
        The function does not hold a reference to the device object.
      */

src/controller/SetUpCodePairer.cpp

@@ -45,6 +45,8 @@ CHIP_ERROR SetUpCodePairer::PairDevice(NodeId remoteId, const char * setUpCode,
     mRemoteId     = remoteId;
     mSetUpPINCode = payload.setUpPINCode;
 
+    ResetDiscoveryState();
+
     return Connect(payload);
 }
 
@@ -91,17 +93,29 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverBle(SetupPayload & payload)
     mCommissioner->ConnectBleTransportToSelf();
 #endif // CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
     VerifyOrReturnError(mBleLayer != nullptr, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
-    return mBleLayer->NewBleConnectionByDiscriminator(payload.discriminator, this, OnDiscoveredDeviceOverBleSuccess,
-                                                      OnDiscoveredDeviceOverBleError);
+
+    ChipLogProgress(Controller, "Starting commissioning discovery over BLE");
+
+    // Handle possibly-sync callbacks.
+    mWaitingForDiscovery[kBLETransport] = true;
+    CHIP_ERROR err = mBleLayer->NewBleConnectionByDiscriminator(payload.discriminator, this, OnDiscoveredDeviceOverBleSuccess,
+                                                                OnDiscoveredDeviceOverBleError);
+    if (err != CHIP_NO_ERROR)
+    {
+        mWaitingForDiscovery[kBLETransport] = false;
+    }
+    return err;
 #else
     return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
 #endif // CONFIG_NETWORK_LAYER_BLE
 }
 
 CHIP_ERROR SetUpCodePairer::StopConnectOverBle()
 {
+    mWaitingForDiscovery[kBLETransport] = false;
 #if CONFIG_NETWORK_LAYER_BLE
     VerifyOrReturnError(mBleLayer != nullptr, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
+    ChipLogDetail(Controller, "Stopping commissioning discovery over BLE");
     return mBleLayer->CancelBleIncompleteConnection();
 #else
     return CHIP_NO_ERROR;
@@ -110,16 +124,28 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverBle()
 
 CHIP_ERROR SetUpCodePairer::StartDiscoverOverIP(SetupPayload & payload)
 {
+    ChipLogProgress(Controller, "Starting commissioning discovery over DNS-SD");
+
     currentFilter.type = payload.isShortDiscriminator ? Dnssd::DiscoveryFilterType::kShortDiscriminator
                                                       : Dnssd::DiscoveryFilterType::kLongDiscriminator;
     currentFilter.code =
         payload.isShortDiscriminator ? static_cast<uint16_t>((payload.discriminator >> 8) & 0x0F) : payload.discriminator;
-    return mCommissioner->DiscoverCommissionableNodes(currentFilter);
+    // Handle possibly-sync callbacks.
+    mWaitingForDiscovery[kIPTransport] = true;
+    CHIP_ERROR err                     = mCommissioner->DiscoverCommissionableNodes(currentFilter);
+    if (err != CHIP_NO_ERROR)
+    {
+        mWaitingForDiscovery[kIPTransport] = false;
+    }
+    return err;
 }
 
 CHIP_ERROR SetUpCodePairer::StopConnectOverIP()
 {
-    currentFilter.type = Dnssd::DiscoveryFilterType::kNone;
+    ChipLogDetail(Controller, "Stopping commissioning discovery over DNS-SD");
+
+    mWaitingForDiscovery[kIPTransport] = false;
+    currentFilter.type                 = Dnssd::DiscoveryFilterType::kNone;
     return CHIP_NO_ERROR;
 }
 
@@ -130,30 +156,73 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverSoftAP(SetupPayload & payload)
 
 CHIP_ERROR SetUpCodePairer::StopConnectOverSoftAP()
 {
+    mWaitingForDiscovery[kSoftAPTransport] = false;
     return CHIP_NO_ERROR;
 }
 
-void SetUpCodePairer::OnDeviceDiscovered(RendezvousParameters & params)
+bool SetUpCodePairer::ConnectToDiscoveredDevice()
 {
-    if (mConnectionType == SetupCodePairerBehaviour::kCommission)
+    if (mWaitingForConnection)
     {
-        LogErrorOnFailure(mCommissioner->PairDevice(mRemoteId, params.SetSetupPINCode(mSetUpPINCode)));
+        // Nothing to do.  Just wait until we either succeed at that connection
+        // or fail and get asked to try another device.
+        return false;
     }
-    else
+
+    for (auto & storedParams : mDiscoveredParameters)
     {
-        LogErrorOnFailure(mCommissioner->EstablishPASEConnection(mRemoteId, params.SetSetupPINCode(mSetUpPINCode)));
+        if (!storedParams.HasPeerAddress())
+        {
+            continue;
+        }
+
+        // Clear out those params, since we will kick off a connection to them
+        // now.
+        RendezvousParameters params(storedParams);
+        storedParams = RendezvousParameters();
+
+        params.SetSetupPINCode(mSetUpPINCode);
+
+#if CHIP_PROGRESS_LOGGING
+        char buf[Transport::PeerAddress::kMaxToStringSize];
+        params.GetPeerAddress().ToString(buf);
+        ChipLogProgress(Controller, "Attempting PASE connection to %s", buf);
+#endif // CHIP_PROGRESS_LOGGING
+
+        CHIP_ERROR err;
+        if (mConnectionType == SetupCodePairerBehaviour::kCommission)
+        {
+            err = mCommissioner->PairDevice(mRemoteId, params);
+        }
+        else
+        {
+            err = mCommissioner->EstablishPASEConnection(mRemoteId, params);
+        }
+        LogErrorOnFailure(err);
+        if (err == CHIP_NO_ERROR)
+        {
+            mWaitingForConnection = true;
+            return true;
+        }
     }
+
+    return false;
 }
 
 #if CONFIG_NETWORK_LAYER_BLE
 void SetUpCodePairer::OnDiscoveredDeviceOverBle(BLE_CONNECTION_OBJECT connObj)
 {
+    ChipLogProgress(Controller, "Discovered device to be commissioned over BLE");
+
+    mWaitingForDiscovery[kBLETransport] = false;
+
+    // Probably safe to stop connections over other transports at this point?
     LogErrorOnFailure(StopConnectOverIP());
     LogErrorOnFailure(StopConnectOverSoftAP());
 
-    Transport::PeerAddress peerAddress = Transport::PeerAddress::BLE();
-    RendezvousParameters params        = RendezvousParameters().SetPeerAddress(peerAddress).SetConnectionObject(connObj);
-    OnDeviceDiscovered(params);
+    Transport::PeerAddress peerAddress   = Transport::PeerAddress::BLE();
+    mDiscoveredParameters[kBLETransport] = RendezvousParameters().SetPeerAddress(peerAddress).SetConnectionObject(connObj);
+    ConnectToDiscoveredDevice();
 }
 
 void SetUpCodePairer::OnDiscoveredDeviceOverBleSuccess(void * appState, BLE_CONNECTION_OBJECT connObj)
@@ -163,6 +232,13 @@ void SetUpCodePairer::OnDiscoveredDeviceOverBleSuccess(void * appState, BLE_CONN
 
 void SetUpCodePairer::OnDiscoveredDeviceOverBleError(void * appState, CHIP_ERROR err)
 {
+    static_cast<SetUpCodePairer *>(appState)->OnBLEDiscoveryError(err);
+}
+
+void SetUpCodePairer::OnBLEDiscoveryError(CHIP_ERROR err)
+{
+    ChipLogError(Controller, "Commissioning discovery over BLE failed: %" CHIP_ERROR_FORMAT, err.Format());
+    mWaitingForDiscovery[kBLETransport] = false;
     LogErrorOnFailure(err);
 }
 #endif // CONFIG_NETWORK_LAYER_BLE
@@ -192,14 +268,57 @@ void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::Discover
     {
         return;
     }
-    LogErrorOnFailure(StopConnectOverBle());
+
+    ChipLogProgress(Controller, "Discovered device to be commissioned over DNS-SD");
+
+    // Don't stop trying to connect over BLE, because we may be dealing with
+    // stale DNS-SD records.
     LogErrorOnFailure(StopConnectOverIP());
     LogErrorOnFailure(StopConnectOverSoftAP());
 
     Inet::InterfaceId interfaceId      = nodeData.ipAddress[0].IsIPv6LinkLocal() ? nodeData.interfaceId : Inet::InterfaceId::Null();
     Transport::PeerAddress peerAddress = Transport::PeerAddress::UDP(nodeData.ipAddress[0], nodeData.port, interfaceId);
-    RendezvousParameters params        = RendezvousParameters().SetPeerAddress(peerAddress);
-    OnDeviceDiscovered(params);
+    mDiscoveredParameters[kIPTransport] = RendezvousParameters().SetPeerAddress(peerAddress);
+    ConnectToDiscoveredDevice();
+}
+
+bool SetUpCodePairer::TryNextRendezvousParameters()
+{
+    // Don't call PairDevice again; just do PASE as needed.  If we called
+    // PairDevice before and PASE succeeds, the commmissioning that's been
+    // queued up will happen.
+    mConnectionType       = SetupCodePairerBehaviour::kPaseOnly;
+    mWaitingForConnection = false;
+
+    if (ConnectToDiscoveredDevice())
+    {
+        ChipLogProgress(Controller, "Trying connection to commissionee over different transport");
+        return true;
+    }
+
+    for (const auto & waiting : mWaitingForDiscovery)
+    {
+        if (waiting)
+        {
+            ChipLogProgress(Controller, "Waiting to discover commissionees that match our filters");
+            return true;
+        }
+    }
+
+    return false;
+}
+
+void SetUpCodePairer::ResetDiscoveryState()
+{
+    for (auto & waiting : mWaitingForDiscovery)
+    {
+        waiting = false;
+    }
+
+    for (auto & params : mDiscoveredParameters)
+    {
+        params = RendezvousParameters();
+    }
 }
 
 } // namespace Controller

src/controller/SetUpCodePairer.h

@@ -53,7 +53,7 @@ enum class SetupCodePairerBehaviour : uint8_t
 class DLL_EXPORT SetUpCodePairer
 {
 public:
-    SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) {}
+    SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) { ResetDiscoveryState(); }
     virtual ~SetUpCodePairer() {}
 
     CHIP_ERROR PairDevice(chip::NodeId remoteId, const char * setUpCode,
@@ -66,6 +66,18 @@ class DLL_EXPORT SetUpCodePairer
     void SetBleLayer(Ble::BleLayer * bleLayer) { mBleLayer = bleLayer; };
 #endif // CONFIG_NETWORK_LAYER_BLE
 
+    // Called by the commissioner when PASE establishment fails.
+    //
+    // May start a new PASE establishment.
+    //
+    // Will return whether we might in fact have more rendezvous parameters to
+    // try (e.g. because we started a new PASE establishment or are waiting on
+    // more device discovery).
+    //
+    // The commissioner can use the return value to decide whether pairing has
+    // actually failed or not.
+    bool TryNextRendezvousParameters();
+
 private:
     CHIP_ERROR Connect(SetupPayload & paload);
     CHIP_ERROR StartDiscoverOverBle(SetupPayload & payload);
@@ -75,11 +87,26 @@ class DLL_EXPORT SetUpCodePairer
     CHIP_ERROR StartDiscoverOverSoftAP(SetupPayload & payload);
     CHIP_ERROR StopConnectOverSoftAP();
 
-    void OnDeviceDiscovered(RendezvousParameters & params);
+    // Returns whether we have kicked off a new connection attempt.
+    bool ConnectToDiscoveredDevice();
+
+    // Reset our mWaitingForDiscovery/mDiscoveredParameters state to indicate no
+    // pending work.
+    void ResetDiscoveryState();
+
+    // Not an enum class because we use this for indexing into arrays.
+    enum TransportTypes
+    {
+        kBLETransport = 0,
+        kIPTransport,
+        kSoftAPTransport,
+        kTransportTypeCount,
+    };
 
 #if CONFIG_NETWORK_LAYER_BLE
     Ble::BleLayer * mBleLayer = nullptr;
     void OnDiscoveredDeviceOverBle(BLE_CONNECTION_OBJECT connObj);
+    void OnBLEDiscoveryError(CHIP_ERROR err);
     /////////// BLEConnectionDelegate Callbacks /////////
     static void OnDiscoveredDeviceOverBleSuccess(void * appState, BLE_CONNECTION_OBJECT connObj);
     static void OnDiscoveredDeviceOverBleError(void * appState, CHIP_ERROR err);
@@ -92,6 +119,22 @@ class DLL_EXPORT SetUpCodePairer
     chip::NodeId mRemoteId;
     uint32_t mSetUpPINCode                   = 0;
     SetupCodePairerBehaviour mConnectionType = SetupCodePairerBehaviour::kCommission;
+
+    // Boolean will be set to true if we currently have an async discovery
+    // process happening via the relevant transport.
+    bool mWaitingForDiscovery[kTransportTypeCount] = { false };
+
+    // HasPeerAddress() for a given transport type will test true if we have
+    // discovered an address for that transport and not tried connecting to it
+    // yet.  The general discovery/pairing process will terminate once all
+    // parameters test false for HasPeerAddress() and all the booleans in
+    // mWaitingForDiscovery are false.
+    RendezvousParameters mDiscoveredParameters[kTransportTypeCount];
+
+    // mWaitingForConnection is true if we have called either
+    // EstablishPASEConnection or PairDevice on mCommissioner and are now just
+    // waiting to see whether that works.
+    bool mWaitingForConnection = false;
 };
 
 } // namespace Controller