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

Fix discovery issues with minimal mDNS #9102

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

mspang
Copy link
Contributor

@mspang mspang commented Aug 18, 2021

Problem

Discovering the operational service during commissioning fails with minimal
mDNS in certain environments (e.g. otbr + mdnsresponder).

Change overview

  • Don't skip the rest of the packet if it contains an irrelevant
    resource record. DNS packets can contain records other than
    those directly requested.

  • Don't terminate chip-tool commissioning if a device other than
    the commissionee is discovered. There is nothing preventing
    receiving OnAddressUpdateComplete for stale records and this
    will generally happen when doing tests in quick succession
    because stale records accumulate in the SRP server.

Testing

chip-tool pairing ble-thread ex:<dataset-in-base64> 112233 20202021 3840 with Pi + otbr-posix + mdnsresponder

fixes #9099

@mspang mspang requested review from andy31415 and cecille August 18, 2021 02:09
@todo
Copy link

todo bot commented Aug 18, 2021

Fix this comparison which is too loose.

// TODO: Fix this comparison which is too loose.
if (HasQNamePart(data.GetName(), kOperationalServiceName))
{
OnOperationalSrvRecord(data.GetName(), srv);
}
}
else if (mDiscoveryType == DiscoveryType::kCommissionableNode || mDiscoveryType == DiscoveryType::kCommissionerNode)
{
// TODO: Fix this comparison which is too loose.
if (HasQNamePart(data.GetName(), kCommissionableServiceName) || HasQNamePart(data.GetName(), kCommissionerServiceName))
{


This comment was generated by todo based on a TODO comment in 590b5d6 in #9102. cc @mspang.

@todo
Copy link

todo bot commented Aug 18, 2021

Fix this comparison which is too loose.

// TODO: Fix this comparison which is too loose.
if (HasQNamePart(data.GetName(), kCommissionableServiceName) || HasQNamePart(data.GetName(), kCommissionerServiceName))
{
OnCommissionableNodeSrvRecord(data.GetName(), srv);
}
}
break;
}


This comment was generated by todo based on a TODO comment in 590b5d6 in #9102. cc @mspang.

- Don't skip the rest of the packet if it contains an irrelevant
  resource record. DNS packets can contain records other than
  those directly requested.

- Don't terminate chip-tool commissioning if a device other than
  the commissionee is discovered. There is nothing preventing
  receiving OnAddressUpdateComplete for stale records and this
  will generally happen when doing tests in quick succession
  because stale records accumulate in the SRP server.

fixes project-chip#9099
@mspang mspang force-pushed the for-chip/fix-mdns-discovery branch from 590b5d6 to a8d6156 Compare August 18, 2021 02:10
@mspang
Copy link
Contributor Author

mspang commented Aug 18, 2021

@cecille Would your testability work help with being able to write a test for this?

@github-actions
Copy link

Size increase report for "esp32-example-build" from 303c02c

File Section File VM
chip-shell.elf .flash.text -20 -20
chip-bridge-app.elf .flash.text -48 -48
chip-lock-app.elf .flash.text -68 -68
chip-temperature-measurement-app.elf .flash.text 56 56
chip-ipv6only-app.elf .flash.text -172 -172
chip-all-clusters-app.elf .flash.text -8 -8
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
[Unmapped],0,20
.debug_str,0,-1
.debug_info,0,-2
.flash.text,-20,-20
.debug_line,0,-27
.debug_loc,0,-30

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
[Unmapped],0,48
.flash.text,-48,-48

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
[Unmapped],0,68
.flash.text,-68,-68

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.flash.text,56,56
[Unmapped],0,-56

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize
[Unmapped],0,172
.flash.text,-172,-172

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
[Unmapped],0,8
.debug_loc,0,2
.debug_info,0,-2
.flash.text,-8,-8
.debug_line,0,-20


@mspang mspang merged commit 3b145cb into project-chip:master Aug 20, 2021
@mspang mspang deleted the for-chip/fix-mdns-discovery branch August 20, 2021 18:39
sharadb-amazon pushed a commit to sharadb-amazon/connectedhomeip that referenced this pull request Aug 23, 2021
- Don't skip the rest of the packet if it contains an irrelevant
  resource record. DNS packets can contain records other than
  those directly requested.

- Don't terminate chip-tool commissioning if a device other than
  the commissionee is discovered. There is nothing preventing
  receiving OnAddressUpdateComplete for stale records and this
  will generally happen when doing tests in quick succession
  because stale records accumulate in the SRP server.

fixes project-chip#9099
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
- Don't skip the rest of the packet if it contains an irrelevant
  resource record. DNS packets can contain records other than
  those directly requested.

- Don't terminate chip-tool commissioning if a device other than
  the commissionee is discovered. There is nothing preventing
  receiving OnAddressUpdateComplete for stale records and this
  will generally happen when doing tests in quick succession
  because stale records accumulate in the SRP server.

fixes project-chip#9099
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.

minmdns fails to discover devices during discovery
5 participants