diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 8b8a168a134288..44cf55dc229c08 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/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; } diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index c2365bb122fd5e..385654f724f0c9 100644 --- a/src/controller/CHIPDeviceController.h +++ b/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. */ diff --git a/src/controller/SetUpCodePairer.cpp b/src/controller/SetUpCodePairer.cpp index cda0978655f74c..4405da2d9f9ed6 100644 --- a/src/controller/SetUpCodePairer.cpp +++ b/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,8 +93,18 @@ 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 @@ -100,8 +112,10 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverBle(SetupPayload & payload) 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((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(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 diff --git a/src/controller/SetUpCodePairer.h b/src/controller/SetUpCodePairer.h index 19b57999f0eeae..1f3bce6705f427 100644 --- a/src/controller/SetUpCodePairer.h +++ b/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