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.

Sign up for GitHub

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

Jump to bottom

Extended discovery timeout is extended on each DnssdServer::StartServer #16522

Closed
Damian-Nordic opened this issue Mar 22, 2022 · 2 comments · Fixed by #20019
Closed

Extended discovery timeout is extended on each DnssdServer::StartServer #16522

Damian-Nordic opened this issue Mar 22, 2022 · 2 comments · Fixed by #20019
Assignees
@bzbarsky-apple
Labels
spec Mismatch between spec and implementation V1.0

Comments

@Damian-Nordic
Copy link
Contributor

Problem

Extended discovery timeout is reset every time DnssdServer::StartServer is called (that is, whenever published DNS-SD services may need to be refreshed) while the timeout should only be scheduled when the extended discovery starts.

Proposed Solution

  1. Start the extended discovery timeout in DnssdServer::StartServer only if it's not yet started.
  2. Cancel the extended discovery timeout when DnssdServer::StartServer is called and the commissioning window is open.
@Damian-Nordic Damian-Nordic mentioned this issue Mar 22, 2022
Merged
@bzbarsky-apple bzbarsky-apple added spec Mismatch between spec and implementation V1.0 labels May 22, 2022
@woody-apple
Copy link
Contributor

Spec Review: We believe this is resolved in the linked PR.

@bzbarsky-apple
Copy link
Contributor

This is NOT fixed. This issue is a follow-up from the linked PR, that was identified during review of it.

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 27, 2022
@bzbarsky-apple
Fix the extended discovery time limit to actually be obeyed properly.
9d01379 
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.
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 27, 2022
Merged
@bzbarsky-apple bzbarsky-apple self-assigned this Jun 27, 2022
bzbarsky-apple added a commit that referenced this issue Jun 28, 2022
@bzbarsky-apple @chrisdecenzo
Fix the extended discovery time limit to actually be obeyed properly. (…
cab048f 
…#20019)

* Fix the extended discovery time limit to actually be obeyed properly.

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:

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.

* Fix typo in comment.

Co-authored-by: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com>

* Advertise extended discovery on startup.

Addresses review comments.

Co-authored-by: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bzbarsky-apple bzbarsky-apple

Labels
spec Mismatch between spec and implementation V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the extended discovery time limit to actually be obeyed properly. bzbarsky-apple/connectedhomeip
3 participants
@woody-apple @bzbarsky-apple @Damian-Nordic