From 3a339bc944e0e4d5059a1cd4bbb464942c8cd5ad Mon Sep 17 00:00:00 2001 From: C Freeman Date: Fri, 3 Dec 2021 14:48:02 -0500 Subject: [PATCH] SetUpCodePairer: don't use DeviceDiscoveryDelegate (#12320) * SetUpCodePairer: don't use DeviceDiscoveryDelegate This class replaces the device discovery delegate that gets passed in in the commissioning parameters which will cause the original delegate to stop receiving callbacks if this class is used. Instead, just have the commissioner notify this class directly. The commissioner owns this part, so it doesn't need to use a derived delegate because we know what we're talking to. Also adding a discovery filter. This wasn't as necessary before because the class would stop receiving notifications by setting the delegate to null. However, that is still a race because it is possible to get spurious mdns responses. * Address review comments. * Add back define guard on dns-sd function. --- src/controller/CHIPDeviceController.cpp | 10 ++++++--- src/controller/CHIPDeviceController.h | 16 ++++++++------ src/controller/SetUpCodePairer.cpp | 29 ++++++++++++++++++++----- src/controller/SetUpCodePairer.h | 18 ++++++++------- 4 files changed, 50 insertions(+), 23 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 6d4857e02ba217..18b6cffd2fe5e7 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1608,18 +1608,22 @@ void DeviceCommissioner::OnUserDirectedCommissioningRequest(UDCClientState state ChipLogDetail(Controller, "------To Accept Enter: discover udc-commission "); } +#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY + +#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD void DeviceCommissioner::OnNodeDiscoveryComplete(const chip::Dnssd::DiscoveredNodeData & nodeData) { +#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY if (mUdcServer != nullptr) { mUdcServer->OnCommissionableNodeFound(nodeData); } - +#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY AbstractDnssdDiscoveryController::OnNodeDiscoveryComplete(nodeData); + mSetUpCodePairer.NotifyCommissionableDeviceDiscovered(nodeData); } - -#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY +#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD void BasicSuccess(void * context, uint16_t val) { diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 998bfd9b5ffa75..346a36a304518d 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -629,20 +629,22 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, /** * @brief - * Overrides method from AbstractDnssdDiscoveryController - * - * @param nodeData DNS-SD node information + * Return the UDC Server instance * */ - void OnNodeDiscoveryComplete(const chip::Dnssd::DiscoveredNodeData & nodeData) override; + UserDirectedCommissioningServer * GetUserDirectedCommissioningServer() { return mUdcServer; } +#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY +#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD /** * @brief - * Return the UDC Server instance + * Overrides method from AbstractDnssdDiscoveryController + * + * @param nodeData DNS-SD node information * */ - UserDirectedCommissioningServer * GetUserDirectedCommissioningServer() { return mUdcServer; } -#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY + void OnNodeDiscoveryComplete(const chip::Dnssd::DiscoveredNodeData & nodeData) override; +#endif void RegisterPairingDelegate(DevicePairingDelegate * pairingDelegate) { mPairingDelegate = pairingDelegate; } diff --git a/src/controller/SetUpCodePairer.cpp b/src/controller/SetUpCodePairer.cpp index 6763e7aa946075..22f425bac3fa08 100644 --- a/src/controller/SetUpCodePairer.cpp +++ b/src/controller/SetUpCodePairer.cpp @@ -108,9 +108,9 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverBle() CHIP_ERROR SetUpCodePairer::StartDiscoverOverIP(uint16_t discriminator, bool isShort) { #if CHIP_DEVICE_CONFIG_ENABLE_DNSSD - mCommissioner->RegisterDeviceDiscoveryDelegate(this); - Dnssd::DiscoveryFilter filter(isShort ? Dnssd::DiscoveryFilterType::kShort : Dnssd::DiscoveryFilterType::kLong, discriminator); - return mCommissioner->DiscoverCommissionableNodes(filter); + currentFilter.type = isShort ? Dnssd::DiscoveryFilterType::kShort : Dnssd::DiscoveryFilterType::kLong; + currentFilter.code = discriminator; + return mCommissioner->DiscoverCommissionableNodes(currentFilter); #else return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; #endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD @@ -119,7 +119,7 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverIP(uint16_t discriminator, bool isS CHIP_ERROR SetUpCodePairer::StopConnectOverIP() { #if CHIP_DEVICE_CONFIG_ENABLE_DNSSD - mCommissioner->RegisterDeviceDiscoveryDelegate(nullptr); + currentFilter.type = Dnssd::DiscoveryFilterType::kNone; #endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD return CHIP_NO_ERROR; } @@ -162,8 +162,27 @@ void SetUpCodePairer::OnDiscoveredDeviceOverBleError(void * appState, CHIP_ERROR #endif // CONFIG_NETWORK_LAYER_BLE #if CHIP_DEVICE_CONFIG_ENABLE_DNSSD -void SetUpCodePairer::OnDiscoveredDevice(const Dnssd::DiscoveredNodeData & nodeData) + +bool SetUpCodePairer::NodeMatchesCurrentFilter(const Dnssd::DiscoveredNodeData & nodeData) +{ + switch (currentFilter.type) + { + case Dnssd::DiscoveryFilterType::kShort: + return ((nodeData.longDiscriminator >> 8) & 0x0F) == currentFilter.code; + case Dnssd::DiscoveryFilterType::kLong: + return nodeData.longDiscriminator == currentFilter.code; + default: + return false; + } + return false; +} + +void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::DiscoveredNodeData & nodeData) { + if (!NodeMatchesCurrentFilter(nodeData)) + { + return; + } LogErrorOnFailure(StopConnectOverBle()); LogErrorOnFailure(StopConnectOverIP()); LogErrorOnFailure(StopConnectOverSoftAP()); diff --git a/src/controller/SetUpCodePairer.h b/src/controller/SetUpCodePairer.h index a55fe61ecb6b5f..3b72831a331164 100644 --- a/src/controller/SetUpCodePairer.h +++ b/src/controller/SetUpCodePairer.h @@ -48,9 +48,6 @@ namespace Controller { class DeviceCommissioner; class DLL_EXPORT SetUpCodePairer -#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD - : public DeviceDiscoveryDelegate -#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD { public: SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) {} @@ -58,6 +55,11 @@ class DLL_EXPORT SetUpCodePairer CHIP_ERROR PairDevice(chip::NodeId remoteId, const char * setUpCode); +// Called by the DeviceCommissioner to notify that we have discovered a new device. +#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD + void NotifyCommissionableDeviceDiscovered(const chip::Dnssd::DiscoveredNodeData & nodeData); +#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD + #if CONFIG_NETWORK_LAYER_BLE void SetBleLayer(Ble::BleLayer * bleLayer) { mBleLayer = bleLayer; }; #endif // CONFIG_NETWORK_LAYER_BLE @@ -73,11 +75,6 @@ class DLL_EXPORT SetUpCodePairer void OnDeviceDiscovered(RendezvousParameters & params); -#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD - /////////// DeviceDiscoveryDelegate Interface ///////// - void OnDiscoveredDevice(const chip::Dnssd::DiscoveredNodeData & nodeData) override; -#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD - #if CONFIG_NETWORK_LAYER_BLE Ble::BleLayer * mBleLayer = nullptr; void OnDiscoveredDeviceOverBle(BLE_CONNECTION_OBJECT connObj); @@ -86,6 +83,11 @@ class DLL_EXPORT SetUpCodePairer static void OnDiscoveredDeviceOverBleError(void * appState, CHIP_ERROR err); #endif // CONFIG_NETWORK_LAYER_BLE +#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD + bool NodeMatchesCurrentFilter(const Dnssd::DiscoveredNodeData & nodeData); + Dnssd::DiscoveryFilter currentFilter; +#endif + DeviceCommissioner * mCommissioner = nullptr; chip::NodeId mRemoteId; uint32_t mSetUpPINCode = 0;