Make SetUpCodePairer only browse for nodes that are in commissioning mode. #17356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
anush-apple,
austinh0,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
kpschoedel,
lazarkov,
LuDuda,
lzgrablic02,
msandstedt and
robszewczyk
pullapprove
bot
requested review from
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
…mode. If we don't browse for the _CM subtype, we might find (and ignore) a node that's not yet in commissioning mode (but has extended commissioning discovery turned on) and when it enters commissioning mode we won't notice because there won't be any new service instances of the sort we are looking for, just TXT record changes.
bzbarsky-apple
force-pushed
the
setupcode-pairer-filter-commissionable
branch
from
7c1a1a2 to
a0cfcbe
Compare
|
PR #17356: Size comparison from b63d897 to a0cfcbe Full report (29 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
Damian-Nordic
approved these changes
Apr 14, 2022
msandstedt
approved these changes
Apr 15, 2022
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Jun 7, 2022
This reverts commit a689c84. Now that we always register a new instance name when opening a new commissioning window the problem PR project-chip#17356 was trying to work around no longer applies. On the other hand, the new setup introduced a new problem: if there are multiple things advertising the _CM subtype (i.e. multiple things in comissioning mode at once), then we might find the first several (however much fits in a DNS packet) and then platform mdns will stop delivering results, per project-chip#19194 (which is about Darwin, but other platforms have similar issues). If we browse by discrimnator instead, the chance of multiple results is much lower, and hence the chance of finding the thing we care about is much higher.
bzbarsky-apple
added a commit
that referenced
this pull request
Jun 7, 2022
) This reverts commit a689c84. Now that we always register a new instance name when opening a new commissioning window the problem PR #17356 was trying to work around no longer applies. On the other hand, the new setup introduced a new problem: if there are multiple things advertising the _CM subtype (i.e. multiple things in comissioning mode at once), then we might find the first several (however much fits in a DNS packet) and then platform mdns will stop delivering results, per #19194 (which is about Darwin, but other platforms have similar issues). If we browse by discrimnator instead, the chance of multiple results is much lower, and hence the chance of finding the thing we care about is much higher.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we don't browse for the _CM subtype, we might find (and ignore) a
node that's not yet in commissioning mode (but has extended
commissioning discovery turned on) and when it enters commissioning
mode we won't notice because there won't be any new service instances
of the sort we are looking for, just TXT record changes.
Problem
See above.
Change overview
Browse for
_CM._sub._matterc._udp, then filter on discriminator, instead of browsing for_LNNN._sub._matterc._udp.Testing
@bluebin14 tested this in his stress-test that caught the initial issue, and it seems to resolve it.