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

Remove ip cluster flags #9892

Merged
merged 3 commits into from
Sep 24, 2021
Merged

Remove ip cluster flags #9892

merged 3 commits into from
Sep 24, 2021

Conversation

cecille
Copy link
Contributor

@cecille cecille commented Sep 22, 2021

Problem

  • IP commissioning gets short circuited by CASE when using the chip tool - resolve gets called twice and we call the completion callback which cases chip-tool to early exit. This seems to cause a race where sometimes the device was being persisted in the cleanup, but sometimes it was not.
  • The chip tool isn't compiled with the ip clustering flags on by default (chip-device-ctrl is). These have been fairly well tested in the TEs now, so we should just get rid of the gates.

Change overview

  • removes flags for IP commissioning - full ip commissioning is now enabled by default everywhere
  • fixes an early exit on ip commissioning that was causing issues for chip-tool

Testing

Tested with chip-tool ip pairing and command sending using linux lighting app.

@restyled-io restyled-io bot mentioned this pull request Sep 22, 2021
Closed
bzbarsky-apple
bzbarsky-apple approved these changes Sep 22, 2021
View reviewed changes
@andy31415
Copy link
Contributor

@cecille - conflicts

cecille and others added 3 commits September 24, 2021 11:10
@cecille
Remove IP commissioning flags.
b6f5a78 
We've been running with these default on for a while, we don't
need to be gating on this anymore and it looks to be messing up
folks using chip-tool.
@cecille
Don't early return on IP commissioning.
d9c0300 
When we're doing full commissioning, we don't want to call the
completion callback until we're actually done. CASE causes the
address to be resolved multiple time, including after we've done
our resolve.
@restyled-commits @cecille
Restyled by shfmt
6aa4ad2
@cecille cecille force-pushed the remove_ip_cluster_flags branch from ae8036d to 6aa4ad2 Compare September 24, 2021 15:16
@cecille cecille merged commit 41fb0b0 into project-chip:master Sep 24, 2021
@cecille cecille deleted the remove_ip_cluster_flags branch September 24, 2021 17:14
@cecille cecille mentioned this pull request Sep 24, 2021
Merged
yunhanw-google added a commit that referenced this pull request Sep 24, 2021
@yunhanw-google
Revert "Remove ip cluster flags (#9892)"
599ad27 
This reverts commit 41fb0b0.
@yunhanw-google yunhanw-google mentioned this pull request Sep 24, 2021
Merged
andy31415 pushed a commit that referenced this pull request Sep 24, 2021
@yunhanw-google
Revert "Remove ip cluster flags (#9892)" (#9970)
eed0bae 
This reverts commit 41fb0b0.
@andy31415
Copy link
Contributor

This was checked in and reverted in ToT due to CI breakages. Holding off on TE6 cherrypick since it is unclear what breaks CI.

@cecille cecille mentioned this pull request Sep 27, 2021
Merged
@mkardous-silabs mkardous-silabs mentioned this pull request Sep 27, 2021
Merged
@cecille cecille mentioned this pull request Oct 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@andy31415 andy31415 andy31415 approved these changes

@msandstedt msandstedt msandstedt approved these changes

@anush-apple anush-apple Awaiting requested review from anush-apple

@austinh0 austinh0 Awaiting requested review from austinh0

@balducci-apple balducci-apple Awaiting requested review from balducci-apple

@carol-apple carol-apple Awaiting requested review from carol-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@emargolis emargolis Awaiting requested review from emargolis

@franck-apple franck-apple Awaiting requested review from franck-apple

@harimau-qirex harimau-qirex Awaiting requested review from harimau-qirex

@hawk248 hawk248 Awaiting requested review from hawk248

@holbrookt holbrookt Awaiting requested review from holbrookt

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@kpschoedel kpschoedel Awaiting requested review from kpschoedel

@LuDuda LuDuda Awaiting requested review from LuDuda

@mlepage-google mlepage-google Awaiting requested review from mlepage-google

@mrjerryjohns mrjerryjohns Awaiting requested review from mrjerryjohns

@pan-apple pan-apple Awaiting requested review from pan-apple

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@shana-apple shana-apple Awaiting requested review from shana-apple

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@woody-apple woody-apple Awaiting requested review from woody-apple

Assignees
No one assigned
Labels
app controller documentation Improvements or additions to documentation examples platform review - approved scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cecille @andy31415 @msandstedt @bzbarsky-apple @restyled-commits