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

[nrfconnect] Enable commissionable node advertising #8454

Merged

Conversation

Damian-Nordic
Copy link
Contributor

@Damian-Nordic Damian-Nordic commented Jul 16, 2021

Problem

Thread platforms such as nRF Connect SDK don't support commissionable node advertising.

Change overview

  1. Clean up DNS-SD default configuration as different settings were defined in two different places.
  2. Clean up conditions for enabling commissionable node advertising.
  3. Enable commissionable node advertising for nRF Connect platform.

Testing

Tested using nRF Connect Lock example:

  1. Checked that after pressing Button 3 the device advertises both operational and commissionable node services.
  2. Likewise, checked that after commissioning with Python CHIP Controller the device advertises both operational and commissionable node services.
  3. Verified that commissionable node services are searchable by subtypes, i.e. commands like avahi-browse -rt _S0._sub._matterc._udp return correct data.

Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

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

It looks like this changes logic that is shared amongst multiple platforms. What unit tests cover this shared code? It seems like we should be adding new unit tests here...

@Damian-Nordic
Copy link
Contributor Author

It looks like this changes logic that is shared amongst multiple platforms. What unit tests cover this shared code? It seems like we should be adding new unit tests here...

Which logic do you mean? We probably can't unit-test the OpenThread changes as they are only used by platforms that are run on hardware.

@Damian-Nordic
Copy link
Contributor Author

Damian-Nordic commented Jul 19, 2021

It looks like this changes logic that is shared amongst multiple platforms. What unit tests cover this shared code? It seems like we should be adding new unit tests here...

I reduced changes in the common code to one condition change in server/Mdns.cpp which seems to make sense anyway - we shouldn't advertise operational nodes with any default node IDs. Apparently, some changes to OTBR must be made to be able to test mDNS in the cirque framework - before that I'm not sure how we can test that part automatically. But I'm open to any suggestions.

@kkasperczyk-no
Copy link
Contributor

@bzbarsky-apple @woody-apple could you re-review and see if there are any other changes to request? I think we are still missing here approval from your side.

@Damian-Nordic
Copy link
Contributor Author

@woody-apple PTAL?

@github-actions
Copy link

Size increase report for "esp32-example-build" from 762a204

File Section File VM
chip-all-clusters-app.elf .flash.text -24 -24
chip-all-clusters-app.elf .flash.rodata -56 -56
chip-lock-app.elf .flash.text 52 52
chip-lock-app.elf .flash.rodata -48 -48
chip-temperature-measurement-app.elf .flash.text -28 -28
chip-temperature-measurement-app.elf .flash.rodata -56 -56
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_loc,0,183
[Unmapped],0,80
.debug_line,0,59
.strtab,0,52
.debug_ranges,0,48
.debug_info,0,43
.debug_frame,0,36
.symtab,0,16
.debug_aranges,0,8
.debug_abbrev,0,6
.riscv.attributes,0,1
.flash.text,-24,-24
.flash.rodata,-56,-56

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_info,0,123
.debug_line,0,66
.debug_ranges,0,64
.flash.text,52,52
.strtab,0,52
.debug_loc,0,36
.debug_frame,0,24
.symtab,0,16
.debug_abbrev,0,9
.debug_aranges,0,8
.debug_str,0,-2
[Unmapped],0,-4
.flash.rodata,-48,-48

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

sections,vmsize,filesize
.debug_info,0,123
[Unmapped],0,84
.debug_line,0,60
.debug_ranges,0,56
.strtab,0,52
.debug_loc,0,28
.debug_frame,0,24
.debug_abbrev,0,17
.symtab,0,16
.debug_aranges,0,8
.flash.text,-28,-28
.flash.rodata,-56,-56

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

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 762a204

File Section File VM
chip-qpg6100-lighting-example.out .text -84 -84
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_loc,0,549
.debug_info,0,183
[Unmapped],0,84
.debug_line,0,82
.debug_ranges,0,56
.debug_frame,0,52
.symtab,0,48
.debug_abbrev,0,46
.debug_aranges,0,16
.strtab,0,-68
.text,-84,-84

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 762a204

File Section File VM
chip-lock.elf bss 0 1268
chip-lock.elf text 1196 1196
chip-lock.elf rodata 352 352
chip-lock.elf device_handles 4 4
chip-lock.elf [LOAD #3 [RW]] 0 -20
chip-shell.elf bss 0 1248
chip-shell.elf text 68 68
chip-shell.elf device_handles -4 -4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,31670
.debug_loc,0,4058
.debug_line,0,3917
.debug_abbrev,0,3245
.debug_str,0,1662
.strtab,0,1371
bss,1268,0
text,1196,1196
.debug_ranges,0,960
.symtab,0,736
rodata,352,352
.debug_frame,0,192
.debug_aranges,0,64
device_handles,4,4
.shstrtab,0,-3
[LOAD #3 [RW]],-20,0

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

sections,vmsize,filesize
bss,1248,0
.debug_loc,0,414
.debug_info,0,256
.debug_line,0,148
text,68,68
.debug_ranges,0,40
.debug_abbrev,0,24
.debug_frame,0,4
.debug_str,0,2
device_handles,-4,-4


@Damian-Nordic Damian-Nordic force-pushed the commissionable-thread branch from 0172601 to b9863e9 Compare July 28, 2021 11:07
@boring-cyborg boring-cyborg bot added the lib label Jul 28, 2021
1. Clean up DNS-SD default configuration as different
   settings were defined in two different places.
3. Clean up conditions for enabling commissionable node
   advertising.
4. Enable commissionable node advertising for nRF Connect
   platform.
@Damian-Nordic Damian-Nordic force-pushed the commissionable-thread branch from b9863e9 to aded3bb Compare July 30, 2021 08:38
@bzbarsky-apple
Copy link
Contributor

What unit tests cover this shared code?

Right now nothing. The thing to unit-test here would be what sorts of advertisements we actually produce in various situations, and we don't have very good infrastructure for this right now....

I filed #8769 for tracking the tests getting added, but don't think we should block the PR on them.

@bzbarsky-apple bzbarsky-apple dismissed woody-apple’s stale review August 3, 2021 16:43

Issues generally addressed, request for tests tracked in #8769

@bzbarsky-apple bzbarsky-apple merged commit 3b9c48c into project-chip:master Aug 3, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* [nrfconnect] Enable commissionable node advertising

1. Clean up DNS-SD default configuration as different
   settings were defined in two different places.
3. Clean up conditions for enabling commissionable node
   advertising.
4. Enable commissionable node advertising for nRF Connect
   platform.

* Fix issue with StartResolver removing existing services
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.

9 participants