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

[mdns/srp] Implemented delayed mDNS platform initialization #10222

Conversation

kkasperczyk-no
Copy link
Contributor

Problem

The Mdns module doesn't wait for SRP platform to be initialized and ready to operate before requesting adding/removing services. SRP platform needs to remove on init all old services associated with the host.

Change overview

  • Removed assigning mMdnsInitialized=true in DiscoveryImplPlatform::Init() method, to be sure that DiscoveryImplPlatform always waits for platform to be ready and signal it by calling HandleMdnsInit callback
  • Removed stopping services in DiscoveryImplPlatform::Start() method, as it is already done one step earlier by StopPublishDevice() method
  • Added kMdnsPlatformInitialized CHIP event informing that mDNS platform is initalized and posted event in HandleMdnsInit callback
  • Added OnPlatformEvent in Mdns.cpp that handles kMdnsPlatformInitialized and restarts advertising
  • Added removing SRP host and services on SRP platform init
  • Renamed FillMAC method to GetPrimaryMACAddress and moved it to the ConfigurationMgr, as it is useful also for src/lib/mdns and src/platform modules that should not use app/server API.
  • Bump OpenThread repo version
  • Bump OTBR-posix repo version

Testing

Tested manually for:

  • Thread devices on nrfconnect platform:
  1. Commissioned device with Python CHIP controller
  2. Verified that both operational and commissionable discovery services are published and visible on the Thread Border Router
  3. Reset device
  4. Verified that device refreshed services, so commissionable discovery service has different pseudo-random name than previously and old service instance is removed.
  • WiFi device on Linux lighting-app:
  1. Run chip-lighting-app
  2. Verified using avahi-browse that device publishes commissionable discovery service

Copy link
Contributor

@tcarmelveilleux tcarmelveilleux left a comment

Choose a reason for hiding this comment

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

  • Is it possible to add any unit tests to hostname generation and MAC address getter?
  • Please handle both 48 and 64 bits MAC addresses properly

@kkasperczyk-no
Copy link
Contributor Author

kkasperczyk-no commented Oct 6, 2021

  • Is it possible to add any unit tests to hostname generation and MAC address getter?
  • Please handle both 48 and 64 bits MAC addresses properly

@tcarmelveilleux

  • I'm afraid it's not that simple for now and I can't write suitable tests for it. That is because we currently don't have platform that would be able to run OpenThread and test Thread platform features in unit tests. I created a test that verifies buffer sizes and default values, but in the end it will be run only for the WiFi platform, so it will not verify all possible cases. @Damian-Nordic already did a research on running unit tests on Zephyr posix with OpenThread support, but we were lacking time to introduce any change. Definitely will try to take care of that soon, but for now I don't really see a way of running such tests. Any thoughts?
  • I tried to address suggestions, so please verify if it's ok now.

@kkasperczyk-no kkasperczyk-no force-pushed the srp_remove_host_and_services_pr branch 2 times, most recently from 89d742d to 0c234a7 Compare October 6, 2021 10:17
@kkasperczyk-no kkasperczyk-no force-pushed the srp_remove_host_and_services_pr branch from 0c234a7 to b41a065 Compare October 6, 2021 10:19
@kpschoedel
Copy link
Contributor

PR #10222: Size comparison from 89d742d to 9ea2cc4

2 builds
platform target config section 9ea2cc4 89d742d change % change
mbed lighting-app CY8CPROTO_062_4343W+release .bss 172060 172060 0 0.0
.data 5368 5368 0 0.0
.heap 859016 859016 0 0.0
.text 1151144 1151272 128 0.0
lock-app CY8CPROTO_062_4343W+release .bss 170564 170564 0 0.0
.data 5344 5344 0 0.0
.heap 860536 860536 0 0.0
.text 1128032 1128096 64 0.0

@kpschoedel
Copy link
Contributor

PR #10222: Size comparison from 89d742d to 9ea2cc4

Live-testing a script, apologies for the intrusion.

@LuDuda LuDuda requested a review from tcarmelveilleux October 6, 2021 21:06
The Mdns module doesn't wait for SRP platform to be initialized
and ready to operate before requesting adding/removing services.
SRP platform needs to remove on init all old services associated
with the host.

* Removed assigning mMdnsInitialized=true in
DiscoveryImplPlatform::Init() method, to be sure that
DiscoveryImplPlatform always waits for platform to be ready
and signal it by calling HandleMdnsInit callback
* Removed stopping services in DiscoveryImplPlatform::Start()
method, as it is already done one step earlier by StopPublishDevice()
method
* Added kMdnsPlatformInitialized CHIP event informing that mDNS
platform is initalized and posted event in HandleMdnsInit callback
* Added OnPlatformEvent in Mdns.cpp that handles
kMdnsPlatformInitialized and restarts advertising
* Added removing SRP host and services on SRP platform init
* Renamed FillMAC method to GetPrimaryMACAddress and moved it
to the ConfigurationMgr, as it is useful also for src/lib/mdns
and src/platform modules that should not use app/server API.
* Bump OpenThread repo version
* Bump OTBR-posix repo version
@kkasperczyk-no kkasperczyk-no force-pushed the srp_remove_host_and_services_pr branch from b41a065 to 33b5e13 Compare October 7, 2021 06:03
@github-actions
Copy link

github-actions bot commented Oct 7, 2021

Size increase report for "gn_qpg-example-build" from d48d754

File Section File VM
chip-qpg6100-lighting-example.out .text 1296 1296
chip-qpg6100-lighting-example.out .bss 0 24
chip-qpg6100-lighting-example.out .heap 0 -24
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_info,0,688312
.debug_str,0,18476
.debug_abbrev,0,9377
.debug_loc,0,8494
.debug_line,0,3585
.debug_ranges,0,1296
.text,1296,1296
.strtab,0,1251
.symtab,0,704
.debug_frame,0,472
.debug_aranges,0,136
.bss,24,0
.shstrtab,0,1
.heap,-24,0
[Unmapped],0,-1296

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

github-actions bot commented Oct 7, 2021

Size increase report for "nrfconnect-example-build" from d48d754

File Section File VM
chip-shell.elf text 408 408
chip-shell.elf rodata 144 140
chip-shell.elf device_handles -8 -8
chip-lock.elf text 384 384
chip-lock.elf rodata 56 52
chip-lock.elf [LOAD #3 [RW]] 0 24
chip-lock.elf bss 0 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
.debug_info,0,19338
.debug_loc,0,1835
.debug_line,0,1484
.debug_str,0,945
.strtab,0,544
text,408,408
.debug_abbrev,0,366
.debug_ranges,0,208
.symtab,0,176
rodata,140,144
.debug_frame,0,120
.debug_aranges,0,24
device_handles,-8,-8

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

sections,vmsize,filesize
.debug_loc,0,2347
.debug_info,0,1933
.debug_line,0,1565
.debug_str,0,825
.strtab,0,456
text,384,384
.debug_ranges,0,288
.debug_abbrev,0,206
.symtab,0,128
.debug_frame,0,108
rodata,52,56
.debug_aranges,0,24
[LOAD #3 [RW]],24,0
bss,8,0


@github-actions
Copy link

github-actions bot commented Oct 7, 2021

Size increase report for "esp32-example-build" from d48d754

File Section File VM
chip-all-clusters-app.elf .flash.text 84 84
chip-all-clusters-app.elf .flash.rodata -8 -8
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_info,0,4651
.debug_line,0,535
.debug_str,0,334
.strtab,0,181
.debug_ranges,0,136
.debug_loc,0,113
.flash.text,84,84
.debug_abbrev,0,52
.debug_frame,0,44
.symtab,0,16
.debug_aranges,0,8
.riscv.attributes,0,-1
.shstrtab,0,-1
.flash.rodata,-8,-8
[Unmapped],0,-76


@kkasperczyk-no
Copy link
Contributor Author

@tcarmelveilleux could you re-review it and resolve comments if no further requests?

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.

7 participants