Fix the extended discovery time limit to actually be obeyed properly. #20019
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
Fixes project-chip#16522 Tested that if CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS is set to CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT then we advertise extended discovery in all the conditions where we are not advertising commissionable discovery. If CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS is set to another value, verified that: 1. We do not start advertising extended discovery on startup. 2. If we start advertising commissionable discovery on startup, then after the commissioning window closes (whether due to being commissioned or timing out) we we start advertising extended discovery until that times out. 3. If we open a new commissioning window and then RevokeCommissioning, we start advertising extended discovery until it times out. 4. If we open a new commissioning window and then commission the device, we start advertising extended discovery until it times out. 5. If in a state with two fabrics commissioned we remove a fabric we do _not_ start advertising extended discovery (unlike before this change). 6. If in a state with two fabrics commissioned and extended discovery advertising ongoing we remove one of the fabrics, we keep advertising extended discovery until it times out.
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google and
msandstedt
pullapprove
bot
requested review from
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
msandstedt,
rgoliver,
robszewczyk,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
tehampson,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
|
PR #20019: Size comparison from cba92c0 to 9d01379 Increases (19 builds for cc13x2_26x2, cyw30739, efr32, k32w, nrfconnect, telink)
Decreases (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
chrisdecenzo
approved these changes
Jun 28, 2022
Co-authored-by: chrisdecenzo <[email protected]>
|
PR #20019: Size comparison from cba92c0 to 296692f Increases (19 builds for cc13x2_26x2, cyw30739, efr32, k32w, nrfconnect, telink)
Decreases (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
tcarmelveilleux
approved these changes
Jun 28, 2022
woody-apple
approved these changes
Jun 28, 2022
Addresses review comments.
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.
Fixes #16522
Tested that if CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS is set to
CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT then we advertise extended discovery
in all the conditions where we are not advertising commissionable discovery.
If CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS is set to another value, verified that:
commissioning window closes (whether due to being commissioned or timing out)
we we start advertising extended discovery until that times out.
advertising extended discovery until it times out.
start advertising extended discovery until it times out.
start advertising extended discovery (unlike before this change).
advertising ongoing we remove one of the fabrics, we keep advertising
extended discovery until it times out.
Problem
See #16522
Change overview
Keep better track of when we actually need to start extended discovery and when we need to reset its timer; don't reset the timer if we're not starting a new extended discovery period.
Testing
For all tests I used all-clusters-app compiled with extended discovery enabled. I modified it to allow smaller commissioning window timeouts than 3 minutes to speed up testing.
For item 1: Start commissioned all-clusters app and verify that it only advertises operational.
for item 2: Start uncommissioned all-clusters-app, verify (via
dns-sd) that it advertises commissionable until the commissioning window closes, then advertises extended discovery until that timeout expires.For item 3: Start commissioned all-clusters-app, use
chip-tool pairing open-commissioning-windowandchip-tool administratorcommissioning revoke-commissioning.For item 4: Start commissioned all-clusters-app, use
chip-tool pairing open-commissioning-windowto open a BCM window andchip-tool pairing code --commissioner-name betato pair with the original manual code.For item 5: Starting after item 4, wait until the extended discovery times out, then do a
remove-fabriccommand via chip-tool.For item 6: Repeat item 4 and 5, but do the remove before the extended discovery has timed out.