add ScanNetworks step to CHIPDeviceController #20766

chrisdecenzo · 2022-07-14T22:00:36Z

Problem

There is no way to trigger a scanNetworks during commissioning
There is no way for apps using the iOS/Android libraries to access the network type of the client (ReadCommissioningInfo data), or access the scan results in order to choose the best network credentials for the device.
There is no way to pause the commissioning flow in the event that async processing (user or cloud) is needed to select and input the best network info.

Change overview

Added an optional commissioning step for performing the ScanNetworks
Added PairingDelegate callbacks for receiving the ReadCommissioningInfo and Scan results
Added pauseCommissioning/resumeCommissioning commands to DeviceCommissioner
Added glue to make this functionality available to Android
TODO: glue to make this functionality available to iOS

Testing

Tested using chip-tool and sample apps.
Tested with efr32

src/controller/CommissioningDelegate.h

selissia · 2022-07-26T21:19:15Z

Can the use of ScanNetworks command be made configurable and skipped by default? And/or can this change go in after the 1.0 release?

Not commenting on the substance of the change but rather on the functionality it touches: This is a very high risk change as it affects most platforms and most test cases. As an example, it unmasks this bug: #21248 and there is no telling what else can surface. Given that we are very close to SVE I think it would be reasonable to delay this.

github-actions · 2022-07-26T22:31:53Z

PR #20766: Size comparison from a7ad1d8 to 2282310

Full report (3 builds for mbed, nrfconnect)

platform	target	config	section	`a7ad1d8`	`2282310`
mbed	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224
			(read/write)	`2449328`	`2449328`
			.bss	214508	214508
			.data	5872	5872
			.text	1411972	1411972
nrfconnect	all-clusters-app	nrf52840dk_nrf52840	(read/write)	`1177131`	`1177131`
			bss	143132	143132
			rodata	142660	142660
			text	812408	812408
	all-clusters-minimal-app	nrf52840dk_nrf52840	(read/write)	`1157183`	`1157183`
			bss	142368	142368
			rodata	134192	134192
			text	801716	801716

github-actions · 2022-07-26T23:10:29Z

PR #20766: Size comparison from a7ad1d8 to f158c8e

Increases (1 build for cc13x2_26x2)

platform	target	config	section	`a7ad1d8`	`f158c8e`	change	% change
cc13x2_26x2	pump-controller-app	LP_CC2652R7	(read only)	666751	666759	8	0.0
			.text	581264	581272	8	0.0

Decreases (2 builds for cc13x2_26x2, nrfconnect)

platform	target	config	section	`a7ad1d8`	`f158c8e`	change	% change
cc13x2_26x2	pump-controller-app	LP_CC2652R7	(read/write)	175768	175760	-8	-0.0
nrfconnect	all-clusters-minimal-app	nrf52840dk_nrf52840	(read/write)	`1157183`	1157167	-16	-0.0
			text	801716	801712	-4	-0.0

Full report (20 builds for bl602, cc13x2_26x2, esp32, k32w, mbed, nrfconnect, p6)

platform	target	config	section	`a7ad1d8`	`f158c8e`	change	% change
bl602	lighting-app	bl602	(read/write)	`1381450`	`1381450`	0	0.0
			.bss	117538	117538	0	0.0
			.data	4480	4480	0	0.0
			.text	1051520	1051520	0	0.0
		bl602+rpc	(read/write)	1426866	1426866	0	0.0
			.bss	124978	124978	0	0.0
			.data	4600	4600	0	0.0
			.text	1083188	1083188	0	0.0
cc13x2_26x2	all-clusters-app	LP_CC2652R7	(read only)	668427	668427	0	0.0
			(read/write)	182932	182932	0	0.0
			.bss	74252	74252	0	0.0
			.data	3356	3356	0	0.0
			.rodata	88411	88411	0	0.0
			.text	579700	579700	0	0.0
	all-clusters-minimal-app	LP_CC2652R7	(read only)	634019	634019	0	0.0
			(read/write)	157820	157820	0	0.0
			.bss	73548	73548	0	0.0
			.data	3356	3356	0	0.0
			.rodata	77635	77635	0	0.0
			.text	556060	556060	0	0.0
	lock-ftd	LP_CC2652R7	(read only)	671567	671567	0	0.0
			(read/write)	169984	169984	0	0.0
			.bss	71332	71332	0	0.0
			.data	3280	3280	0	0.0
			.rodata	76463	76463	0	0.0
			.text	594624	594624	0	0.0
	lock-mtd	LP_CC2652R7	(read only)	653795	653795	0	0.0
			(read/write)	183444	183444	0	0.0
			.bss	67020	67020	0	0.0
			.data	3280	3280	0	0.0
			.rodata	101163	101163	0	0.0
			.text	552152	552152	0	0.0
	pump-app	LP_CC2652R7	(read only)	680999	680999	0	0.0
			(read/write)	161384	161384	0	0.0
			.bss	71396	71396	0	0.0
			.data	3280	3280	0	0.0
			.rodata	89175	89175	0	0.0
			.text	591340	591340	0	0.0
	pump-controller-app	LP_CC2652R7	(read only)	666751	666759	8	0.0
			(read/write)	175768	175760	-8	-0.0
			.bss	71532	71532	0	0.0
			.data	3276	3276	0	0.0
			.rodata	85007	85007	0	0.0
			.text	581264	581272	8	0.0
	shell	LP_CC2652R7	(read only)	660894	660894	0	0.0
			(read/write)	185984	185984	0	0.0
			.bss	76572	76572	0	0.0
			.data	3360	3360	0	0.0
			.rodata	85174	85174	0	0.0
			.text	575404	575404	0	0.0
esp32	all-clusters-app	c3devkit	(read only)	1022114	1022114	0	0.0
			(read/write)	1486586	1486586	0	0.0
			.dram0.bss	70288	70288	0	0.0
			.dram0.data	14600	14600	0	0.0
			.flash.rodata	216264	216264	0	0.0
			.flash.text	1022114	1022114	0	0.0
			.iram0.text	62902	62902	0	0.0
		m5stack	(read only)	1075771	1075771	0	0.0
			(read/write)	488608	488608	0	0.0
			.dram0.bss	75800	75800	0	0.0
			.dram0.data	34144	34144	0	0.0
			.flash.rodata	246668	246668	0	0.0
			.flash.text	1070387	1070387	0	0.0
			.iram0.text	123267	123267	0	0.0
k32w	light	k32w0+release	(read/write)	641776	641776	0	0.0
			.bss	69728	69728	0	0.0
			.data	2028	2028	0	0.0
			.text	567292	567292	0	0.0
	lock	k32w0+release	(read/write)	699056	699056	0	0.0
			.bss	70168	70168	0	0.0
			.data	2036	2036	0	0.0
			.text	624124	624124	0	0.0
mbed	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	`2449328`	`2449328`	0	0.0
			.bss	214508	214508	0	0.0
			.data	5872	5872	0	0.0
			.text	1411972	1411972	0	0.0
nrfconnect	all-clusters-app	nrf52840dk_nrf52840	(read/write)	`1177131`	`1177131`	0	0.0
			bss	143132	143132	0	0.0
			rodata	142660	142660	0	0.0
			text	812408	812408	0	0.0
	all-clusters-minimal-app	nrf52840dk_nrf52840	(read/write)	`1157183`	1157167	-16	-0.0
			bss	142368	142368	0	0.0
			rodata	134192	134192	0	0.0
			text	801716	801712	-4	-0.0
p6	all-clusters-app	default	(read only)	881568	881568	0	0.0
			(read/write)	1687044	1687044	0	0.0
			.bss	149128	149128	0	0.0
			.data	2648	2648	0	0.0
			.text	1526880	1526880	0	0.0
	all-clusters-minimal-app	default	(read only)	882288	882288	0	0.0
			(read/write)	1631148	1631148	0	0.0
			.bss	148408	148408	0	0.0
			.data	2648	2648	0	0.0
			.text	`1471704`	`1471704`	0	0.0
	light-app	default	(read only)	890592	890592	0	0.0
			(read/write)	1551516	1551516	0	0.0
			.bss	140312	140312	0	0.0
			.data	2440	2440	0	0.0
			.text	`1400376`	`1400376`	0	0.0
	lock-app	default	(read only)	886120	886120	0	0.0
			(read/write)	`1589116`	`1589116`	0	0.0
			.bss	144768	144768	0	0.0
			.data	2456	2456	0	0.0
			.text	`1433504`	`1433504`	0	0.0

chrisdecenzo · 2022-07-26T23:44:30Z

Can the use of ScanNetworks command be made configurable and skipped by default? And/or can this change go in after the 1.0 release?

Not commenting on the substance of the change but rather on the functionality it touches: This is a very high risk change as it affects most platforms and most test cases. As an example, it unmasks this bug: #21248 and there is no telling what else can surface. Given that we are very close to SVE I think it would be reasonable to delay this.

In the latest, its turned off by default when thread credentials are provided. But we need this functionality for 1.0

src/controller/AutoCommissioner.cpp

src/controller/AutoCommissioner.h

src/controller/CHIPDeviceController.cpp

src/controller/DevicePairingDelegate.h

src/controller/java/AndroidDeviceControllerWrapper.cpp

src/controller/java/AndroidDeviceControllerWrapper.h

* DRAFT: add ScanNetworks step to CHIPDeviceController * Add android hooks and callbacks for network scan * Add controller parameters for failsafe timeout, and scans * Add callback for ReadCommissioningInfo * straggler file * address comments * fix android build * DRAFT: add ScanNetworks step to CHIPDeviceController * Add android hooks and callbacks for network scan * Add controller parameters for failsafe timeout, and scans * Add callback for ReadCommissioningInfo * straggler file * address comments * fix android build * Restyle DRAFT: add ScanNetworks step to CHIPDeviceController (#20808) * Restyled by whitespace * Restyled by google-java-format Co-authored-by: Restyled.io <commits@restyled.io> * fix CI * fix kotlin build issue * fix java method signature lookup * fix cirq tests, add name for ScanNetworks step * attempt to fix cirq tests * Address comments * Address comments * fix stragglers, restyle * address feedback * address feedback * fix build * fix build * fix build Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>

jonsmirl · 2022-07-28T23:00:12Z

Android chip-tool needs to fixed up due to this commit changing the API.

* DRAFT: add ScanNetworks step to CHIPDeviceController * Add android hooks and callbacks for network scan * Add controller parameters for failsafe timeout, and scans * Add callback for ReadCommissioningInfo * straggler file * address comments * fix android build * DRAFT: add ScanNetworks step to CHIPDeviceController * Add android hooks and callbacks for network scan * Add controller parameters for failsafe timeout, and scans * Add callback for ReadCommissioningInfo * straggler file * address comments * fix android build * Restyle DRAFT: add ScanNetworks step to CHIPDeviceController (#20808) * Restyled by whitespace * Restyled by google-java-format Co-authored-by: Restyled.io <commits@restyled.io> * fix CI * fix kotlin build issue * fix java method signature lookup * fix cirq tests, add name for ScanNetworks step * attempt to fix cirq tests * Address comments * Address comments * fix stragglers, restyle * address feedback * address feedback * fix build * fix build * fix build Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>

* DRAFT: add ScanNetworks step to CHIPDeviceController * Add android hooks and callbacks for network scan * Add controller parameters for failsafe timeout, and scans * Add callback for ReadCommissioningInfo * straggler file * address comments * fix android build * DRAFT: add ScanNetworks step to CHIPDeviceController * Add android hooks and callbacks for network scan * Add controller parameters for failsafe timeout, and scans * Add callback for ReadCommissioningInfo * straggler file * address comments * fix android build * Restyle DRAFT: add ScanNetworks step to CHIPDeviceController (project-chip#20808) * Restyled by whitespace * Restyled by google-java-format Co-authored-by: Restyled.io <commits@restyled.io> * fix CI * fix kotlin build issue * fix java method signature lookup * fix cirq tests, add name for ScanNetworks step * attempt to fix cirq tests * Address comments * Address comments * fix stragglers, restyle * address feedback * address feedback * fix build * fix build * fix build Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>

DRAFT: add ScanNetworks step to CHIPDeviceController

36bb9b3

bzbarsky-apple reviewed Jul 20, 2022

View reviewed changes

src/controller/CommissioningDelegate.h Outdated Show resolved Hide resolved

Address comments

3ae3de4

selissia mentioned this pull request Jul 26, 2022

Crash in EFR32 lighting app when scanNetworks is issued during commissioning #21248

Closed

chrisdecenzo added 2 commits July 26, 2022 14:48

Address comments

8776a7e

sync to master

2282310

restyled-io bot mentioned this pull request Jul 26, 2022

Restyle add ScanNetworks step to CHIPDeviceController #21251

Closed

fix stragglers, restyle

f158c8e

bzbarsky-apple reviewed Jul 27, 2022

View reviewed changes

address feedback

2a5e0a3

This was referenced Jul 28, 2022

CI cirque tests are fragile, tightly coupled to commissioning steps #21313

Open

CHIPDeviceController needs proper support for setting network credentials during a commissioning session #21314

Closed

address feedback

90cf820

bzbarsky-apple approved these changes Jul 28, 2022

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Jul 28, 2022

chrisdecenzo added 3 commits July 28, 2022 05:35

fix build

16a7ecc

fix build

390b792

fix build

5083311

chrisdecenzo merged commit 1679342 into master Jul 28, 2022

chrisdecenzo deleted the tvapps-android16 branch July 28, 2022 15:00

vivien-apple mentioned this pull request Aug 29, 2022

Changes from PR 22104 break commissioning flows where the network info is provided later #22195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ScanNetworks step to CHIPDeviceController #20766

add ScanNetworks step to CHIPDeviceController #20766

chrisdecenzo commented Jul 14, 2022 •

edited

Loading

selissia commented Jul 26, 2022

github-actions bot commented Jul 26, 2022

github-actions bot commented Jul 26, 2022 •

edited

Loading

chrisdecenzo commented Jul 26, 2022 •

edited

Loading

jonsmirl commented Jul 28, 2022

add ScanNetworks step to CHIPDeviceController #20766

add ScanNetworks step to CHIPDeviceController #20766

Conversation

chrisdecenzo commented Jul 14, 2022 • edited Loading

Problem

Change overview

Testing

selissia commented Jul 26, 2022

github-actions bot commented Jul 26, 2022

github-actions bot commented Jul 26, 2022 • edited Loading

chrisdecenzo commented Jul 26, 2022 • edited Loading

jonsmirl commented Jul 28, 2022

chrisdecenzo commented Jul 14, 2022 •

edited

Loading

github-actions bot commented Jul 26, 2022 •

edited

Loading

chrisdecenzo commented Jul 26, 2022 •

edited

Loading