Skip to content

Commit

Permalink
SetUpCodePairer: don't use DeviceDiscoveryDelegate (#12320)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
cecille authored Dec 3, 2021
1 parent 4f4268e commit 3a339bc
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 23 deletions.
10 changes: 7 additions & 3 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1608,18 +1608,22 @@ void DeviceCommissioner::OnUserDirectedCommissioningRequest(UDCClientState state

ChipLogDetail(Controller, "------To Accept Enter: discover udc-commission <pincode> <udc-client-index>");
}
#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)
{
Expand Down
16 changes: 9 additions & 7 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -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; }

Expand Down
29 changes: 24 additions & 5 deletions src/controller/SetUpCodePairer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}
Expand Down Expand Up @@ -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());
Expand Down
18 changes: 10 additions & 8 deletions src/controller/SetUpCodePairer.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,18 @@ 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) {}
virtual ~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
Expand All @@ -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);
Expand All @@ -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;
Expand Down

0 comments on commit 3a339bc

Please sign in to comment.