From 77a205e7206a5e78d832913235a632ab2755b6e5 Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Fri, 26 Nov 2021 17:02:58 -0500 Subject: [PATCH 1/3] 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. --- src/controller/CHIPDeviceController.cpp | 10 +++++-- src/controller/CHIPDeviceController.h | 14 +++++----- src/controller/SetUpCodePairer.cpp | 37 +++++++++++++++++++++---- src/controller/SetUpCodePairer.h | 18 ++++++------ 4 files changed, 56 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..e9557bb9f76b3b 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -629,20 +629,20 @@ 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 /** * @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; void RegisterPairingDelegate(DevicePairingDelegate * pairingDelegate) { mPairingDelegate = pairingDelegate; } diff --git a/src/controller/SetUpCodePairer.cpp b/src/controller/SetUpCodePairer.cpp index 6763e7aa946075..88b3ab82e53206 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,35 @@ 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) == currentFilter.code; + case Dnssd::DiscoveryFilterType::kLong: + return nodeData.longDiscriminator == currentFilter.code; + case Dnssd::DiscoveryFilterType::kVendor: + return nodeData.vendorId == currentFilter.code; + case Dnssd::DiscoveryFilterType::kDeviceType: + return nodeData.deviceType == currentFilter.code; + case Dnssd::DiscoveryFilterType::kCommissioningMode: + return nodeData.commissioningMode == currentFilter.code; + case Dnssd::DiscoveryFilterType::kNone: + case Dnssd::DiscoveryFilterType::kInstanceName: + case Dnssd::DiscoveryFilterType::kCommissioner: + case Dnssd::DiscoveryFilterType::kCompressedFabricId: + 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; From 6d63a132f9c58a403fba04805abdca3a7f4ab0b2 Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Tue, 30 Nov 2021 12:02:10 -0500 Subject: [PATCH 2/3] Address review comments. --- src/controller/SetUpCodePairer.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/controller/SetUpCodePairer.cpp b/src/controller/SetUpCodePairer.cpp index 88b3ab82e53206..22f425bac3fa08 100644 --- a/src/controller/SetUpCodePairer.cpp +++ b/src/controller/SetUpCodePairer.cpp @@ -168,23 +168,15 @@ bool SetUpCodePairer::NodeMatchesCurrentFilter(const Dnssd::DiscoveredNodeData & switch (currentFilter.type) { case Dnssd::DiscoveryFilterType::kShort: - return (nodeData.longDiscriminator >> 8) == currentFilter.code; + return ((nodeData.longDiscriminator >> 8) & 0x0F) == currentFilter.code; case Dnssd::DiscoveryFilterType::kLong: return nodeData.longDiscriminator == currentFilter.code; - case Dnssd::DiscoveryFilterType::kVendor: - return nodeData.vendorId == currentFilter.code; - case Dnssd::DiscoveryFilterType::kDeviceType: - return nodeData.deviceType == currentFilter.code; - case Dnssd::DiscoveryFilterType::kCommissioningMode: - return nodeData.commissioningMode == currentFilter.code; - case Dnssd::DiscoveryFilterType::kNone: - case Dnssd::DiscoveryFilterType::kInstanceName: - case Dnssd::DiscoveryFilterType::kCommissioner: - case Dnssd::DiscoveryFilterType::kCompressedFabricId: + default: return false; } return false; } + void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::DiscoveredNodeData & nodeData) { if (!NodeMatchesCurrentFilter(nodeData)) From c5571a017d1a499e07dc0f07c1ffaba1a51df0b7 Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Fri, 3 Dec 2021 11:33:20 -0500 Subject: [PATCH 3/3] Add back define guard on dns-sd function. --- src/controller/CHIPDeviceController.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index e9557bb9f76b3b..346a36a304518d 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -635,6 +635,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, UserDirectedCommissioningServer * GetUserDirectedCommissioningServer() { return mUdcServer; } #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY +#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD /** * @brief * Overrides method from AbstractDnssdDiscoveryController @@ -643,6 +644,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, * */ void OnNodeDiscoveryComplete(const chip::Dnssd::DiscoveredNodeData & nodeData) override; +#endif void RegisterPairingDelegate(DevicePairingDelegate * pairingDelegate) { mPairingDelegate = pairingDelegate; }