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.

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

fix BLE commissioning on commissionable Matter TVs #15525

Merged
merged 5 commits into from
Mar 2, 2022
Merged

Conversation

chrisdecenzo
Copy link
Contributor

Problem

What is being fixed? Examples:

Change overview

By default, when there is more than one BleBase transport, the second to initialize overrides the global BleLayer transport with itself. This fix adds Ble transport initialization parameters so that this override behavior can be managed, and the default behavior is to only set the global BleLayer transport if it is not already set.

Testing

  • Tested by commissioning the tv-app using BLE

@github-actions
Copy link

github-actions bot commented Feb 24, 2022

PR #15525: Size comparison from 960eba3 to 1c10bf8

Increases (5 builds for linux, mbed)
platform target config section 960eba3 1c10bf8 change % change
linux chip-tool-ipv6only arm64 (read only) 8747692 8747900 208 0.0
.rodata 456596 456676 80 0.0
.text 7426692 7426820 128 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read/write) 2449656 2449808 152 0.0
.text 1412228 1412380 152 0.0
lighting-app CY8CPROTO_062_4343W+release (read/write) 2414040 2414128 88 0.0
.text 1376612 1376700 88 0.0
lock-app CY8CPROTO_062_4343W+release (read/write) 2349796 2349948 152 0.0
.text 1312396 1312548 152 0.0
shell CY8CPROTO_062_4343W+release (read/write) 2340096 2340248 152 0.0
.text 1302668 1302820 152 0.0
Full report (8 builds for linux, mbed, telink)
platform target config section 960eba3 1c10bf8 change % change
linux chip-tool-ipv6only arm64 (read only) 8747692 8747900 208 0.0
(read/write) 412241 412241 0 0.0
.bss 59265 59265 0 0.0
.data 1216 1216 0 0.0
.data.rel.ro 296568 296568 0 0.0
.dynamic 560 560 0 0.0
.got 51392 51392 0 0.0
.init 24 24 0 0.0
.init_array 208 208 0 0.0
.rodata 456596 456676 80 0.0
.text 7426692 7426820 128 0.0
thermostat-no-ble arm64 (read only) 2161156 2161156 0 0.0
(read/write) 149217 149217 0 0.0
.bss 65905 65905 0 0.0
.data 1056 1056 0 0.0
.data.rel.ro 75080 75080 0 0.0
.dynamic 560 560 0 0.0
.got 4216 4216 0 0.0
.init 24 24 0 0.0
.init_array 352 352 0 0.0
.rodata 132308 132308 0 0.0
.text 1810544 1810544 0 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2449656 2449808 152 0.0
.bss 191664 191664 0 0.0
.data 5512 5512 0 0.0
.text 1412228 1412380 152 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2414040 2414128 88 0.0
.bss 188836 188836 0 0.0
.data 5800 5800 0 0.0
.text 1376612 1376700 88 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2349796 2349948 152 0.0
.bss 187740 187740 0 0.0
.data 5776 5776 0 0.0
.text 1312396 1312548 152 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1155428 1155428 0 0.0
.bss 11952 11952 0 0.0
.data 4512 4512 0 0.0
.text 118812 118812 0 0.0
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2340096 2340248 152 0.0
.bss 186384 186384 0 0.0
.data 5608 5608 0 0.0
.text 1302668 1302820 152 0.0
telink lighting-app tlsr9518adk80d (read/write) 882362 882362 0 0.0
bss 86352 86352 0 0.0
noinit 37160 37160 0 0.0
text 623590 623590 0 0.0

src/controller/CHIPDeviceController.h Outdated Show resolved Hide resolved
Copy link

@Byungjoo-Lee Byungjoo-Lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified BLE commissioning

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a cleaner way of getting to the BLE layer. Using GetImplAtIndex does not seem right.

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to approve as an unblocking for TV.

From a separate chat with @chrisdecenzo , the situation we have is:

  • ControllerFactory creates a transportmgr shared by all controllers
  • Server creates a transport for server stuff

These two conflict and generally the BLE on the server is only needed for commissioning and before commissioning, the controller side would not be functional. There should be a way for either sharing the same trasnport (so BLE does not conflict) or sequencing initializations (do server first, once commissioned, start the controller side).

This needs revision and Chris agreed to take a look at it while working on "make TVs run without comissioness" with a rough estimate of before 8 weeks (i.e. should happen reasonably soon).

@chrisdecenzo
Copy link
Contributor Author

Created issue #15732 to remove this workaround for manually changing the BleLayer transport from CHIPDeviceControlleer (see issue for proposed solution).

@woody-apple
Copy link
Contributor

/rebase

@github-actions
Copy link

github-actions bot commented Mar 2, 2022

PR #15525: Size comparison from 1ccc2af to d9f7211

Increases above 0.2%:

platform target config section 1ccc2af d9f7211 change % change
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1039967 1076371 36404 3.5
bss 125828 127680 1852 1.5
rodata 135736 141724 5988 4.4
text 701156 729560 28404 4.1
Increases (6 builds for esp32, mbed, nrfconnect, p6)
platform target config section 1ccc2af d9f7211 change % change
esp32 all-clusters-app m5stack (read only) 1005951 1005991 40 0.0
(read/write) 458176 458264 88 0.0
.flash.rodata 224432 224520 88 0.0
.flash.text 1000567 1000607 40 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read/write) 2350908 2350996 88 0.0
.text 1313508 1313596 88 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1039967 1076371 36404 3.5
bss 125828 127680 1852 1.5
rodata 135736 141724 5988 4.4
text 701156 729560 28404 4.1
p6 all-clusters-app default (read/write) 2489400 2489536 136 0.0
.text 1447664 1447800 136 0.0
light-app default (read/write) 2396568 2396712 144 0.0
.text 1354832 1354976 144 0.0
lock-app default (read/write) 2360080 2360224 144 0.0
.text 1318344 1318488 144 0.0
Full report (6 builds for esp32, mbed, nrfconnect, p6)
platform target config section 1ccc2af d9f7211 change % change
esp32 all-clusters-app m5stack (read only) 1005951 1005991 40 0.0
(read/write) 458176 458264 88 0.0
.dram0.bss 68168 68168 0 0.0
.dram0.data 34080 34080 0 0.0
.flash.rodata 224432 224520 88 0.0
.flash.text 1000567 1000607 40 0.0
.iram0.text 122767 122767 0 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2350908 2350996 88 0.0
.bss 187156 187156 0 0.0
.data 5784 5784 0 0.0
.text 1313508 1313596 88 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1039967 1076371 36404 3.5
bss 125828 127680 1852 1.5
rodata 135736 141724 5988 4.4
text 701156 729560 28404 4.1
p6 all-clusters-app default (read/write) 2489400 2489536 136 0.0
.bss 118736 118736 0 0.0
.data 2696 2696 0 0.0
.text 1447664 1447800 136 0.0
light-app default (read/write) 2396568 2396712 144 0.0
.bss 113048 113048 0 0.0
.data 2544 2544 0 0.0
.text 1354832 1354976 144 0.0
lock-app default (read/write) 2360080 2360224 144 0.0
.bss 112792 112792 0 0.0
.data 2504 2504 0 0.0
.text 1318344 1318488 144 0.0

@chrisdecenzo chrisdecenzo merged commit 654104c into master Mar 2, 2022
@chrisdecenzo chrisdecenzo deleted the tv-apps7 branch March 2, 2022 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CHIP-TOOL can't provision CHIP-TV using BLE
5 participants