Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for accessory DNS browse and resolve on Thread devices. #8132

Merged
merged 2 commits into from
Jul 13, 2021

Conversation

kkasperczyk-no
Copy link
Contributor

@kkasperczyk-no kkasperczyk-no commented Jul 6, 2021

Problem

Currently Thread platform doesn't provide mDNS browse and resolve implementation. Moreover there is no way to trigger browse or resolve queries on accessory devices to perform operational discovery from accessory device perspective.

Change overview

For all:

  • Added set of dns shell commands allowing to invoke dns browse and dns resolve commands.

For Thread:

  • Implemented ChipMdnsBrowse and ChipMdnsResolve.
  • Implemented ThreadStackMgr DnsResolve and DnsBrowse commands using OpenThread API for performing DNS queries.
  • Changed SRP service buffers sizes to allow handling Commissionable Discovery records (currently it was assumed to support only Operational Discovery and sizes are not big enough).
  • Added configs allowing to decide whether Thread DNS client and Thread Commissionable Discovery should be enabled
    (to avoid potential memory consumption problems on different platforms as Commissionable Discovery data are pretty heavy).

For nrfconnect:

  • Enabled DNS client and commissionable discovery support by default.
  • Extended shell commands documentation about dns.

What this change doesn't introduce:

  • Support for commissionable discovery on Thread devices, it only allows for handling commissionable discovery records
    responses when performing dns browse.
  • DNS browsing/resolving support on platforms other than nrfconnect, as it needs to be enabled individually by setting
    CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT

Testing

It was tested only for Thread devices using set of dns CHIP shell commands on nrfconnect platform.
Prerequisities:

  • Having Thread Border Router set with some SRP records applied to browse or resolve. It's worth to notice that current Discovery_ImplPlatform.cpp implementation uses dns browse only for searching _matterc._udp records (so commissionable ones) and resolve for searching _matter._tcp records.
  • Flashed firmware with CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT and chip_build_libshell enabled.

Steps to test:

  1. Join accessory device to the Thread Border Router either by completing commissioning or by using some default Thread credentials (it doesn't really matter).
  2. Call dns resolve <fabric-id> <node-id> (e.g. dns resolve 5544332211 1) command in the accessory shell terminal. For nrfconnect platform CHIP shell commands must be additionally preceded by matter prefix (so matter dns resolve ...). Expected output on success:
uart:~$ matter dns resolve 5544332211 12344321
Resolving ...
I: 88135 [DIS]Node ID resolved for 0x0000000000BC5C01 to fd10:5e9b:7b87:ad2b:fa74:ee2a:cb6a:6717
DNS resolve for 000000014A77CBB3-0000000000BC5C01 succeeded:
  Address: fd10:5e9b:7b87:ad2b:fa74:ee2a:cb6a:6717
  Port: 11097

Expected output on failure (service not found):

uart:~$ matter dns resolve 5544332211 1
Resolving ...
uart:~$ E: 118514 [DIS]Node ID resolved failed with OpenThread Error 9000023 (0x00895457): NotFound
  1. Call dns browse command in the accessory shell terminal. For nrfconnect platform CHIP shell commands must be additionally preceded by matter prefix (so matter dns browse).
    Expected output on success:
uart:~$ matter dns browse
Browsing ...
DNS browse succeeded: 
   Hostname: 0E824F0CA6DE309C
   Vendor ID: 9050
   Product ID: 20043
   Long discriminator: 3840
   Device type: 0
   Device name: 
   Commissioning mode: 0
   IP addresses:
      fd08:b65e:db8e:f9c7:2cc2:2043:1366:3b31

Expected output on empty browse result:

uart:~$ matter dns browse
Browsing ...

Currently Thread platform doesn't provide mDNS browse and resolve
implementation. Moreover there is no way to trigger browse
or resolve queries on accessory devices to perform operational
discovery from accessory device perspective.

For all:
* Added set of dns shell commands allowing to invoke dns browse
and dns resolve commands.

For Thread:
* Implemented ChipMdnsBrowse and ChipMdnsResolve.
* Implemented ThreadStackMgr DnsResolve and DnsBrowse commands
using OpenThread API for performing DNS queries.
* Changed SRP service buffers sizes to allow handling Commissionable
Discovery records (currently it was assumed to support only
Operational Discovery and sizes are not big enough).
* Added configs allowing to decide whether Thread DNS client
and Thread Commissionable Discovery should be enabled
(to avoid potential memory consumption problems on different
platforms as Commissionable Discovery data are pretty heavy).

For nrfconnect:
* Enabled DNS client and commissionable discovery support
by default.
* Extended shell commands documentation about dns.

What this change doesn't introduce:
* Support for commissionable discovery on Thread devices,
it only allows for handling commissionable discovery records
responses when performing dns browse.
* DNS browsing/resolving support on platforms other than
nrfconnect, as it needs to be enabled individually by setting
CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT
@tima-q
Copy link
Contributor

tima-q commented Jul 9, 2021

Is there a bloat report available for this change ?
Like to have an indication on the Flash/RAM impact.

@kkasperczyk-no
Copy link
Contributor Author

Is there a bloat report available for this change ?
Like to have an indication on the Flash/RAM impact.

@tima-q I don't know, could you give me a hint how to enable it if I should be able to do that?

@tima-q
Copy link
Contributor

tima-q commented Jul 9, 2021

Is there a bloat report available for this change ?
Like to have an indication on the Flash/RAM impact.

@tima-q I don't know, could you give me a hint how to enable it if I should be able to do that?

@kkasperczyk-no Typically it triggers automatically on your PR as part of the CI. (did see some talk about issues with it)
If you have any local diffs around size for this feature, that would be helpful as well.

@kkasperczyk-no
Copy link
Contributor Author

kkasperczyk-no commented Jul 9, 2021

Is there a bloat report available for this change ?
Like to have an indication on the Flash/RAM impact.

@tima-q I don't know, could you give me a hint how to enable it if I should be able to do that?

@kkasperczyk-no Typically it triggers automatically on your PR as part of the CI. (did see some talk about issues with it)
If you have any local diffs around size for this feature, that would be helpful as well.

Sure, so in my particular case for lock-app on nrfconnect sizes changed accordingly:
FLASH: increased by 2899 B
SRAM: increased by 320 B

That includes building with new shell DNS commands, DNS client support and CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY = y, so assuming bigger txt arrays for commissionable records. Nevertheless it may not show real memory impact, as browse and resolve result objects are created in callbacks, so it will be allocated on stack and it's hard to predict real usage.

if (!isResolverStarted)
{
chip::Mdns::Resolver::Instance().StartResolver(&chip::DeviceLayer::InetLayer, chip::Mdns::kMdnsPort);
chip::Mdns::Resolver::Instance().SetResolverDelegate(&sDnsShellResolverDelegate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally concerned at all of the code we're building up that enforces the singleton pattern for the mDNS module. I'm not sure why this is necessary. But it's going to force platforms that cannot tolerate this to interact with mDNS outside of the sdk, reducing the usefulness of the sdk's mDNS code in such cases.

And I think such scenarios are likely. All it would take for this to break is a single case where multiple separate, but concurrent discovery operations need to occur at the same time. I can think of several cases where a node would want to do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msandstedt Can you provide any use cases where using the singleton pattern for mDNS can be limiting? I always thought that the biggest issue in our codebase is that we allow a single delegate in certain components, so it's hard to spawn multiple jobs of the same kind in parallel. I thought that some pattern like:

struct ResolveRequest
{
  NodeId mNodeId;
  // ... other request params
  OnComplete mOnCompleteCallback;
  ResolveRequest* mNext;

  void Complete(ResolveResult* result);
  void Cancel();
};

might be a good idea to solve this issue. But I would welcome any arguments for multiple instances of the mDNS server in CHIP as well.

@Damian-Nordic Damian-Nordic merged commit 13888a6 into project-chip:master Jul 13, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…roject-chip#8132)

* Add support for accessory DNS browse and resolve on Thread devices.

Currently Thread platform doesn't provide mDNS browse and resolve
implementation. Moreover there is no way to trigger browse
or resolve queries on accessory devices to perform operational
discovery from accessory device perspective.

For all:
* Added set of dns shell commands allowing to invoke dns browse
and dns resolve commands.

For Thread:
* Implemented ChipMdnsBrowse and ChipMdnsResolve.
* Implemented ThreadStackMgr DnsResolve and DnsBrowse commands
using OpenThread API for performing DNS queries.
* Changed SRP service buffers sizes to allow handling Commissionable
Discovery records (currently it was assumed to support only
Operational Discovery and sizes are not big enough).
* Added configs allowing to decide whether Thread DNS client
and Thread Commissionable Discovery should be enabled
(to avoid potential memory consumption problems on different
platforms as Commissionable Discovery data are pretty heavy).

For nrfconnect:
* Enabled DNS client and commissionable discovery support
by default.
* Extended shell commands documentation about dns.

What this change doesn't introduce:
* Support for commissionable discovery on Thread devices,
it only allows for handling commissionable discovery records
responses when performing dns browse.
* DNS browsing/resolving support on platforms other than
nrfconnect, as it needs to be enabled individually by setting
CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT

* Addressed review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants