-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[dns-sd] Add support for service subtypes on Thread devices #8252
[dns-sd] Add support for service subtypes on Thread devices #8252
Conversation
use fixed buffer allocator to reduce the memory footprint from N*M to sum(M_i)connectedhomeip/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h Lines 138 to 144 in 228b170
This comment was generated by todo based on a
|
@@ -1119,35 +1119,54 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_AddSrpService(c | |||
|
|||
srpService->mService.mPort = aPort; | |||
|
|||
// Check if there are some optional text entries to add. | |||
if (aTxtEntries && aTxtEntriesSize != 0) | |||
#if OPENTHREAD_API_VERSION >= 132 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Damian-Nordic Happy to accomodate this new feature, but can you advise to which OpenThread commit you bumped for this ? At your side this get's updated through Zephyr I assume.
btw I see another check for API_VERSION >= 126 in OpenThreadUtils.cpp - is the feature expected to work with anything in between ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We bumped OpenThread in Zephyr to 48b129 (API_VERSION == 134). The check for API_VERSION >= 126 was added because with that API version OpenThread switched from otMasterXXX to otNetworkXXX function naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Damian-Nordic Thanks - we will align to that commit to avoid OpenThread inconsistencies
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp
Outdated
Show resolved
Hide resolved
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp
Outdated
Show resolved
Hide resolved
/rebase |
5a86d3b
to
4cf6327
Compare
Implementing commissionable node advertising for Thread devices requires that our OpenThread-based DNS-SD backend be able to register a set of service subtypes. It has become possible recently once the support for service subtypes was added to the OpenThread SRP client.
4cf6327
to
2c8b92f
Compare
Size increase report for "gn_qpg-example-build" from 13888a6
Full report output
|
Size increase report for "esp32-example-build" from 13888a6
Full report output
|
Size increase report for "nrfconnect-example-build" from 13888a6
Full report output
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
…chip#8252) * [dns-sd] Add support for service subtypes on Thread devices Implementing commissionable node advertising for Thread devices requires that our OpenThread-based DNS-SD backend be able to register a set of service subtypes. It has become possible recently once the support for service subtypes was added to the OpenThread SRP client. * Restyled by clang-format * Address code-review comments Co-authored-by: Restyled.io <[email protected]>
Problem
Our OpenThread-based DNS-SD Advertiser implementation does not support service subtypes while it is necessary to enable commissionable node advertising for Thread devices.
Change overview
ThreadStackManager::AddSrpService
method to support additional service subtypes.Advertiser.h
begin()/end()
methods toSpan
.Testing
Tested using nRF Connect Lock example with manually enabled commissionable node advertising:
ot srp client service
shell command that a proper service is registeredNote:
@jmartinez-silabs @doru91 @tima-q you will need to update OpenThread soon to make use of this addition on your platforms.