-
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
Add support for commisionable advertisement #4337
Add support for commisionable advertisement #4337
Conversation
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.
please make the enablement/disablement of this feature driven by a member variable whose default value can be changed by the device/application using the SDK.
@chrisdecenzo - could you elaborate a bit? This is chip spec requirement (we have commisioning, commisionable and operational advertisement) and generally moving into each mode is app/library controlled. What is the concern? I believe currently this is enabled by a 'has mdns' flag which is designed to be used for on/off on thread devices (which may use other dns-sd methods than mdns to save battery). Are there additional flags required? |
uint8_t shortDiscriminator = 52; | ||
uint16_t longDiscriminator = 840; | ||
Optional<uint16_t> vendorId; | ||
Optional<uint16_t> productId; | ||
Optional<const char *> pairingInstr; |
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.
Is this required for commisionable as well? if not, I think we should add a comment that these 2 are for commisionable only.
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.
Only required for commisionable. Will add a comment.
examples/minimal-mdns/advertiser.cpp
Outdated
@@ -61,6 +64,8 @@ constexpr uint16_t kOptionCommisioningShordDiscriminator = 's'; | |||
constexpr uint16_t kOptionCommisioningLongDiscriminaotr = 'l'; | |||
constexpr uint16_t kOptionCommisioningVendorId = 0x100; // v is used by 'version' | |||
constexpr uint16_t kOptionCommisioningProductId = 'p'; | |||
constexpr uint16_t kOptionCommisioningPairingInstr = 'I'; | |||
constexpr uint16_t kOptionCommisioningPairingHint = 'H'; |
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.
using uppercase seems a bit different than existing flags. I imagine 'H' is to not conflict with 'h'. I would either then update vendorId to use 'V' (and maybe make all others uppercase too) or make the conflicting ones only have a long version like '--pairing-hint'.
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.
I think sticking to the long version for conflicting ones is more readable. Will do that.
examples/minimal-mdns/advertiser.cpp
Outdated
@@ -81,6 +86,10 @@ bool HandleOptions(const char * aProgram, OptionSet * aOptions, int aIdentifier, | |||
{ | |||
gOptions.advertisingMode = AdvertisingMode::kCommisioning; | |||
} | |||
else if ((strcmp(aValue, "commisionable") == 0) || (strcmp(aValue, "d") == 0)) |
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.
the 'd' is awkward. Please remove 'c' from above instead since 'c' seems ambigous for commisiong/commisionable
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.
I used 'd' because for commisionable we advertise as _chipd while for commisioning we advertise as _chipc and hence 'c' was used.
char txtPairingInstrHint[128]; | ||
if (params.GetPairingHint().HasValue()) | ||
{ | ||
// sprintf(txtPairingInstrHint, "P=%d", params.GetPairingHint().Value()); |
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.
remove commented out code.
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.
Will do
if (params.GetPairingHint().HasValue()) | ||
{ | ||
// sprintf(txtPairingInstrHint, "P=%d", params.GetPairingHint().Value()); | ||
sprintf(txtPairingInstrHint, "P=%s+%d", params.GetPairingInstr().Value(), params.GetPairingHint().Value()); |
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.
Need to ensure that GetPairingInstr() HasValue.
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.
Will do
This is an optional feature. Some devices will want to turn it off. |
Addressed this in the follow-up patch |
Size increase report for "esp32-example-build" from 872015b
Full report output
|
Size increase report for "nrfconnect-example-build" from 872015b
Full report output
|
This comment has been minimized.
This comment has been minimized.
examples/minimal-mdns/advertiser.cpp
Outdated
@@ -33,19 +33,28 @@ enum class AdvertisingMode | |||
{ | |||
kCommisioning, | |||
kOperational, | |||
#if CHIP_ENABLE_COMMISIONABLE_DISCOVERY |
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.
No need for an ifdef in example code. Please remove - this conditional occurs in too many places of the code.
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.
after discussion with Andrei, we agreed not to incur the complexity overhead of a compile-time flag for this feature, thanks
@@ -129,6 +183,9 @@ class ServiceAdvertiser | |||
/// Advertises the CHIP node as a commisioning node | |||
virtual CHIP_ERROR Advertise(const CommisioningAdvertisingParameters & params) = 0; | |||
|
|||
/// Advertises the CHIP node as a commisionable node | |||
virtual CHIP_ERROR Advertise(const CommisionableAdvertisingParameters & params) = 0; | |||
|
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.
is there a method to stop this advertisement?
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.
Not currently - we assume that 'once a chip device, always a chip device and we advertise'. Having a 'StopAdvertising' seems possible though.
After discussions with the group, it was decided that we don't need a compile time option to enable/disable the commisionable mode, but instead have some kind of a stop advertising command to do this.
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.
approving changes. Arun will create a separate issue for disabling commissionable discovery
Problem
We need support for "commisionable" discovery advertisement according to Discovery spec for CHIP
Summary of Changes
This adds support for "commisionable" discovery advertisement.
This builds on top of PR #4200 which adds "commisioning" and "operational" discovery advertisements
This adds the Advertise() function in lib/mdns/Advertiser_ImplMinimalMdns.cpp to advertise service as _chipd._udp while running in commisionable mode.
This also adds the class CommisionableAdvertisingParameters which additionally contains pairingInstr and pairingHint
Finally, this adds some test code in the example/minimal-mdns/advertiser.cpp to test this mode.