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

Fix the check for duplicate endpoints #12934

Merged
merged 1 commit into from
Dec 13, 2021
Merged

Fix the check for duplicate endpoints #12934

merged 1 commit into from
Dec 13, 2021

Conversation

chiragatal
Copy link
Contributor

Problem

  • When adding a dynamic endpoint with endpoint id 0, a duplicate endpoint exists is returned even if there are no endpoints with id 0.

Change overview

  • For duplicate endpoints, in addition to checking for endpoint id, now also checking for endpointType, which ensures that it has actually been created.

Testing

  • Tested that endpoint 0 is created successfully.

@boring-cyborg boring-cyborg bot added the app label Dec 13, 2021
@CLAassistant
Copy link

CLAassistant commented Dec 13, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

github-actions bot commented Dec 13, 2021 •
edited
Loading

PR #12934: Size comparison from c3572d4 to ff0b1da

Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section c3572d4 ff0b1da change % change
efr32 lighting-app BRD4161A (read only) 815980 815980 0 0.0
(read/write) 123152 123152 0 0.0
.bss 121288 121288 0 0.0
.data 1864 1864 0 0.0
.text 815972 815972 0 0.0
BRD4161A+rpc (read only) 803612 803612 0 0.0
(read/write) 139828 139828 0 0.0
.bss 137864 137864 0 0.0
.data 1964 1964 0 0.0
.text 803604 803604 0 0.0
window-app BRD4161A (read only) 793056 793056 0 0.0
(read/write) 122104 122104 0 0.0
.bss 120272 120272 0 0.0
.data 1828 1828 0 0.0
.text 793048 793048 0 0.0
esp32 all-clusters-app c3devkit (read only) 857268 857268 0 0.0
(read/write) 1304378 1304378 0 0.0
.dram0.bss 64696 64696 0 0.0
.dram0.data 14124 14124 0 0.0
.flash.rodata 172520 172520 0 0.0
.flash.text 857268 857268 0 0.0
.iram0.text 62076 62076 0 0.0
m5stack (read only) 960819 960819 0 0.0
(read/write) 450092 450092 0 0.0
.dram0.bss 73976 73976 0 0.0
.dram0.data 34048 34048 0 0.0
.flash.rodata 210280 210280 0 0.0
.flash.text 955435 955435 0 0.0
.iram0.text 123451 123451 0 0.0
k32w lighting-app k32w061+se05x+release (read/write) 670436 670436 0 0.0
.bss 73872 73872 0 0.0
.data 1896 1896 0 0.0
.text 588868 588868 0 0.0
lock-app k32w061+debug (read/write) 620372 620372 0 0.0
.bss 72024 72024 0 0.0
.data 1856 1856 0 0.0
.text 540692 540692 0 0.0
shell k32w061+debug (read/write) 625740 625740 0 0.0
.bss 74332 74332 0 0.0
.data 1836 1836 0 0.0
.text 543772 543772 0 0.0
linux chip-tool-ipv6only arm64 (read only) 6863180 6863180 0 0.0
(read/write) 318737 318737 0 0.0
.bss 51889 51889 0 0.0
.data 1192 1192 0 0.0
.data.rel.ro 208416 208416 0 0.0
.dynamic 560 560 0 0.0
.got 53512 53512 0 0.0
.init 24 24 0 0.0
.init_array 176 176 0 0.0
.rodata 364988 364988 0 0.0
.text 5817828 5817828 0 0.0
thermostat-no-ble arm64 (read only) 1954356 1954356 0 0.0
(read/write) 137425 137425 0 0.0
.bss 59537 59537 0 0.0
.data 832 832 0 0.0
.data.rel.ro 70528 70528 0 0.0
.dynamic 560 560 0 0.0
.got 3648 3648 0 0.0
.init 24 24 0 0.0
.init_array 280 280 0 0.0
.rodata 125780 125780 0 0.0
.text 1620464 1620464 0 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2314104 2314104 0 0.0
.bss 183964 183964 0 0.0
.data 5240 5240 0 0.0
.heap 847240 847240 0 0.0
.text 1276680 1276680 0 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2309960 2309960 0 0.0
.bss 175824 175824 0 0.0
.data 5536 5536 0 0.0
.heap 855088 855088 0 0.0
.text 1272560 1272560 0 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2283072 2283072 0 0.0
.bss 174864 174864 0 0.0
.data 5528 5528 0 0.0
.heap 856056 856056 0 0.0
.text 1245672 1245672 0 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1140008 1140008 0 0.0
.bss 11756 11756 0 0.0
.data 4376 4376 0 0.0
.heap 1020312 1020312 0 0.0
.text 103392 103392 0 0.0
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2053944 2053944 0 0.0
.bss 156980 156980 0 0.0
.data 4864 4864 0 0.0
.heap 874600 874600 0 0.0
.text 1016544 1016544 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 907735 907735 0 0.0
bss 116300 116300 0 0.0
rodata 102008 102008 0 0.0
text 613828 613828 0 0.0
nrf52840dk_nrf52840+rpc (read/write) 870803 870803 0 0.0
bss 112648 112648 0 0.0
rodata 93308 93308 0 0.0
text 588572 588572 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 833738 833738 0 0.0
bss 117672 117672 0 0.0
rodata 97268 97268 0 0.0
text 544256 544256 0 0.0
lock-app nrf52840dk_nrf52840 (read/write) 880351 880351 0 0.0
bss 113560 113560 0 0.0
rodata 98128 98128 0 0.0
text 593264 593264 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 806606 806606 0 0.0
bss 114972 114972 0 0.0
rodata 93416 93416 0 0.0
text 523784 523784 0 0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 497463 497463 0 0.0
bss 51820 51820 0 0.0
rodata 45852 45852 0 0.0
text 339492 339492 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 885303 885303 0 0.0
bss 113472 113472 0 0.0
rodata 99480 99480 0 0.0
text 596868 596868 0 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 878543 878543 0 0.0
bss 113352 113352 0 0.0
rodata 97616 97616 0 0.0
text 592064 592064 0 0.0
shell nrf52840dk_nrf52840 (read/write) 782427 782427 0 0.0
bss 109624 109624 0 0.0
rodata 74296 74296 0 0.0
text 523996 523996 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 697506 697506 0 0.0
bss 110604 110604 0 0.0
rodata 68940 68940 0 0.0
text 444636 444636 0 0.0
p6 all-clusters-app default (read/write) 2361000 2361000 0 0.0
.bss 112164 112164 0 0.0
.data 2528 2528 0 0.0
.heap 918648 918648 0 0.0
.text 1319264 1319264 0 0.0
light-app default (read/write) 2301600 2301600 0 0.0
.bss 101080 101080 0 0.0
.data 2368 2368 0 0.0
.heap 929896 929896 0 0.0
.text 1259864 1259864 0 0.0
lock-app default (read/write) 2277672 2277672 0 0.0
.bss 99960 99960 0 0.0
.data 2328 2328 0 0.0
.heap 931056 931056 0 0.0
.text 1235936 1235936 0 0.0
qpg lighting-app qpg6100+debug (read only) 522184 522184 0 0.0
(read/write) 122332 122332 0 0.0
.bss 82712 82712 0 0.0
.data 996 996 0 0.0
.text 516864 516864 0 0.0
lock-app qpg6100+debug (read only) 496564 496564 0 0.0
(read/write) 122336 122336 0 0.0
.bss 81848 81848 0 0.0
.data 952 952 0 0.0
.text 491244 491244 0 0.0
persistent-storage-app qpg6100+debug (read only) 108104 108104 0 0.0
(read/write) 122336 122336 0 0.0
.bss 36160 36160 0 0.0
.data 288 288 0 0.0
.text 102784 102784 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 812282 812282 0 0.0
bss 82864 82864 0 0.0
noinit 37160 37160 0 0.0
text 567284 567284 0 0.0

@andy31415 andy31415 merged commit 228bd4b into project-chip:master Dec 13, 2021
bzbarsky-apple
bzbarsky-apple reviewed Dec 15, 2021
View reviewed changes
src/app/util/attribute-storage.cpp
@@ -183,7 +183,7 @@ EmberAfStatus emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, EmberAfEn
index = static_cast<uint16_t>(realIndex);
for (uint16_t i = FIXED_ENDPOINT_COUNT; i < MAX_ENDPOINT_COUNT; i++)
{
if (emAfEndpoints[i].endpoint == id)
if (emAfEndpoints[i].endpoint == id && emAfEndpoints[i].endpointType != NULL)
Copy link
Contributor

Choose a reason for hiding this comment

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

It might have been better to change the init code in emberAfEndpointConfigure to set all the endpoint ids on the non-fixed endpoints to the invalid endpoint id....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised a PR for this: #13814

@chiragatal chiragatal mentioned this pull request Jan 21, 2022
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 left review comments

@dhrishi dhrishi dhrishi approved these changes

@andy31415 andy31415 andy31415 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

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

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

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

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

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@emargolis emargolis Awaiting requested review from emargolis

@erjiaqing erjiaqing Awaiting requested review from erjiaqing

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

@gjc13 gjc13 Awaiting requested review from gjc13

@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

@LuDuda LuDuda Awaiting requested review from LuDuda

@lzgrablic02 lzgrablic02 Awaiting requested review from lzgrablic02

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

@msandstedt msandstedt Awaiting requested review from msandstedt

@mspang mspang Awaiting requested review from mspang

@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

@selissia selissia Awaiting requested review from selissia

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@tecimovic tecimovic Awaiting requested review from tecimovic

@vijs vijs Awaiting requested review from vijs

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

@wbschiller wbschiller Awaiting requested review from wbschiller

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

@xylophone21 xylophone21 Awaiting requested review from xylophone21

Assignees
No one assigned
Labels
app review - pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chiragatal @CLAassistant @andy31415 @dhrishi @bzbarsky-apple