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

Refactor SessionHandle #13330

Merged
merged 9 commits into from
Jan 7, 2022
Merged

Refactor SessionHandle #13330

merged 9 commits into from
Jan 7, 2022

Conversation

kghost
Copy link
Contributor

@kghost kghost commented Jan 6, 2022

Problem

Make session easy to use and managable

Change overview

  • Access session via a SessionHandle
    • All session handles become temporary object which can only exist on stack.
    • Introduce a virtual session interface for all types of sessions (secure, group, unauthenticated)
      • Session APIs like GetSubjectDescriptor, GetPeerAddress, GetMRPConfig are virtual now, and their implementations are pushed down.
    • The session object can be converted to actual session class when needed. (not recommaned)
  • Manage session life-cycle using SessionHolder
    • SessionHolder is used to store a session.
    • SessionHolder will clear itself when the session is released.
      • The implementation uses a intrusive list to link all holders, and store the linked-list in the session object.
    • SessionHolderWithDelegate can be used to receive OnSessionReleased event.

Testing

This is a refactor. Verified by unit-tests.

Note: this PR is pretty big, I have tried to split it into multiple PRs, The final part appears coupling sticky, and it is quite hard to tear it down any further. But fortunately, the changes are pretty straightforward, and couldn't be hard to review.

@restyled-io restyled-io bot mentioned this pull request Jan 7, 2022
Closed
@andy31415
Copy link
Contributor

@doru91 - I have patched the NXP SDK to remove the 'define global' as I have not seen its usage and it seems that for matter it is somewhat common to patch the NXP SDK. Please take a look and see if there is a better approach.

@github-actions
Copy link

github-actions bot commented Jan 7, 2022

PR #13330: Size comparison from 36a759d to 0d5d03e

Increases above 0.2%:

platform target config section 36a759d 0d5d03e change % change
linux chip-tool-ipv6only arm64 (read/write) 325073 325985 912 0.3
.bss 54241 54865 624 1.2
thermostat-no-ble arm64 (read/write) 144193 145105 912 0.6
.bss 64033 64657 624 1.0
.data.rel.ro 72392 72624 232 0.3
.got 3952 4008 56 1.4
Increases (2 builds for linux)
platform target config section 36a759d 0d5d03e change % change
linux chip-tool-ipv6only arm64 (read only) 7033996 7039244 5248 0.1
(read/write) 325073 325985 912 0.3
.bss 54241 54865 624 1.2
.data.rel.ro 209064 209296 232 0.1
.got 56992 57040 48 0.1
.rodata 384292 384404 112 0.0
.text 5956932 5961268 4336 0.1
thermostat-no-ble arm64 (read only) 2031036 2033244 2208 0.1
(read/write) 144193 145105 912 0.6
.bss 64033 64657 624 1.0
.data.rel.ro 72392 72624 232 0.3
.got 3952 4008 56 1.4
.rodata 128844 128988 144 0.1
.text 1689280 1690576 1296 0.1
Decreases (3 builds for qpg, telink)
platform target config section 36a759d 0d5d03e change % change
qpg lighting-app qpg6105+debug (read only) 533268 533132 -136 -0.0
.bss 86688 86624 -64 -0.1
.text 527948 527812 -136 -0.0
lock-app qpg6105+debug (read only) 505044 504908 -136 -0.0
.bss 85824 85760 -64 -0.1
.text 499724 499588 -136 -0.0
telink lighting-app tlsr9518adk80d (read/write) 834102 833678 -424 -0.1
bss 86864 86776 -88 -0.1
text 582240 582086 -154 -0.0
Full report (6 builds for linux, qpg, telink)
platform target config section 36a759d 0d5d03e change % change
linux chip-tool-ipv6only arm64 (read only) 7033996 7039244 5248 0.1
(read/write) 325073 325985 912 0.3
.bss 54241 54865 624 1.2
.data 1096 1096 0 0.0
.data.rel.ro 209064 209296 232 0.1
.dynamic 560 560 0 0.0
.got 56992 57040 48 0.1
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 384292 384404 112 0.0
.text 5956932 5961268 4336 0.1
thermostat-no-ble arm64 (read only) 2031036 2033244 2208 0.1
(read/write) 144193 145105 912 0.6
.bss 64033 64657 624 1.0
.data 880 880 0 0.0
.data.rel.ro 72392 72624 232 0.3
.dynamic 560 560 0 0.0
.got 3952 4008 56 1.4
.init 24 24 0 0.0
.init_array 296 296 0 0.0
.rodata 128844 128988 144 0.1
.text 1689280 1690576 1296 0.1
qpg lighting-app qpg6105+debug (read only) 533268 533132 -136 -0.0
(read/write) 146936 146936 0 0.0
.bss 86688 86624 -64 -0.1
.data 1004 1004 0 0.0
.text 527948 527812 -136 -0.0
lock-app qpg6105+debug (read only) 505044 504908 -136 -0.0
(read/write) 146940 146940 0 0.0
.bss 85824 85760 -64 -0.1
.data 952 952 0 0.0
.text 499724 499588 -136 -0.0
persistent-storage-app qpg6105+debug (read only) 106448 106448 0 0.0
(read/write) 146938 146938 0 0.0
.bss 36146 36146 0 0.0
.data 288 288 0 0.0
.text 101128 101128 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 834102 833678 -424 -0.1
bss 86864 86776 -88 -0.1
noinit 37160 37160 0 0.0
text 582240 582086 -154 -0.0

@project-chip project-chip deleted a comment from github-actions bot Jan 7, 2022
@project-chip project-chip deleted a comment from github-actions bot Jan 7, 2022
@project-chip project-chip deleted a comment from github-actions bot Jan 7, 2022
@project-chip project-chip deleted a comment from github-actions bot Jan 7, 2022
@github-actions
Copy link

github-actions bot commented Jan 7, 2022 •
edited
Loading

PR #13330: Size comparison from 36a759d to 4c7c2d7

Increases above 0.2%:

platform target config section 36a759d 4c7c2d7 change % change
linux chip-tool-ipv6only arm64 (read/write) 325073 325985 912 0.3
.bss 54241 54865 624 1.2
thermostat-no-ble arm64 (read/write) 144193 145105 912 0.6
.bss 64033 64657 624 1.0
.data.rel.ro 72392 72624 232 0.3
.got 3952 4008 56 1.4
Increases (2 builds for linux)
platform target config section 36a759d 4c7c2d7 change % change
linux chip-tool-ipv6only arm64 (read only) 7033996 7039244 5248 0.1
(read/write) 325073 325985 912 0.3
.bss 54241 54865 624 1.2
.data.rel.ro 209064 209296 232 0.1
.got 56992 57040 48 0.1
.rodata 384292 384404 112 0.0
.text 5956932 5961268 4336 0.1
thermostat-no-ble arm64 (read only) 2031036 2033244 2208 0.1
(read/write) 144193 145105 912 0.6
.bss 64033 64657 624 1.0
.data.rel.ro 72392 72624 232 0.3
.got 3952 4008 56 1.4
.rodata 128844 128988 144 0.1
.text 1689280 1690576 1296 0.1
Decreases (11 builds for efr32, k32w, p6, qpg, telink)
platform target config section 36a759d 4c7c2d7 change % change
efr32 lighting-app BRD4161A (read only) 831968 828844 -3124 -0.4
(read/write) 127088 126996 -92 -0.1
.bss 125208 125120 -88 -0.1
.text 831960 828836 -3124 -0.4
BRD4161A+rpc (read only) 819148 816040 -3108 -0.4
(read/write) 143744 143656 -88 -0.1
.bss 141768 141680 -88 -0.1
.text 819140 816032 -3108 -0.4
window-app BRD4161A (read only) 805416 802292 -3124 -0.4
(read/write) 126024 125936 -88 -0.1
.bss 124192 124104 -88 -0.1
.text 805408 802284 -3124 -0.4
k32w light k32w061+release (read/write) 655772 655276 -496 -0.1
.bss 76864 76776 -88 -0.1
.text 571260 570852 -408 -0.1
lock k32w061+release (read/write) 660032 659564 -468 -0.1
.bss 77160 77072 -88 -0.1
.text 575204 574824 -380 -0.1
p6 all-clusters-app default (read/write) 2404872 2401592 -3280 -0.1
.bss 117020 116804 -216 -0.2
.text 1363136 1359856 -3280 -0.2
light-app default (read/write) 2326832 2323616 -3216 -0.1
.bss 105888 105672 -216 -0.2
.text 1285096 1281880 -3216 -0.3
lock-app default (read/write) 2299032 2295840 -3192 -0.1
.bss 104768 104552 -216 -0.2
.text 1257296 1254104 -3192 -0.3
qpg lighting-app qpg6105+debug (read only) 533268 533132 -136 -0.0
.bss 86688 86624 -64 -0.1
.text 527948 527812 -136 -0.0
lock-app qpg6105+debug (read only) 505044 504908 -136 -0.0
.bss 85824 85760 -64 -0.1
.text 499724 499588 -136 -0.0
telink lighting-app tlsr9518adk80d (read/write) 834102 833678 -424 -0.1
bss 86864 86776 -88 -0.1
text 582240 582086 -154 -0.0
Full report (14 builds for efr32, k32w, linux, p6, qpg, telink)
platform target config section 36a759d 4c7c2d7 change % change
efr32 lighting-app BRD4161A (read only) 831968 828844 -3124 -0.4
(read/write) 127088 126996 -92 -0.1
.bss 125208 125120 -88 -0.1
.data 1876 1876 0 0.0
.text 831960 828836 -3124 -0.4
BRD4161A+rpc (read only) 819148 816040 -3108 -0.4
(read/write) 143744 143656 -88 -0.1
.bss 141768 141680 -88 -0.1
.data 1976 1976 0 0.0
.text 819140 816032 -3108 -0.4
window-app BRD4161A (read only) 805416 802292 -3124 -0.4
(read/write) 126024 125936 -88 -0.1
.bss 124192 124104 -88 -0.1
.data 1832 1832 0 0.0
.text 805408 802284 -3124 -0.4
k32w light k32w061+release (read/write) 655772 655276 -496 -0.1
.bss 76864 76776 -88 -0.1
.data 1848 1848 0 0.0
.text 571260 570852 -408 -0.1
lock k32w061+release (read/write) 660032 659564 -468 -0.1
.bss 77160 77072 -88 -0.1
.data 1868 1868 0 0.0
.text 575204 574824 -380 -0.1
linux chip-tool-ipv6only arm64 (read only) 7033996 7039244 5248 0.1
(read/write) 325073 325985 912 0.3
.bss 54241 54865 624 1.2
.data 1096 1096 0 0.0
.data.rel.ro 209064 209296 232 0.1
.dynamic 560 560 0 0.0
.got 56992 57040 48 0.1
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 384292 384404 112 0.0
.text 5956932 5961268 4336 0.1
thermostat-no-ble arm64 (read only) 2031036 2033244 2208 0.1
(read/write) 144193 145105 912 0.6
.bss 64033 64657 624 1.0
.data 880 880 0 0.0
.data.rel.ro 72392 72624 232 0.3
.dynamic 560 560 0 0.0
.got 3952 4008 56 1.4
.init 24 24 0 0.0
.init_array 296 296 0 0.0
.rodata 128844 128988 144 0.1
.text 1689280 1690576 1296 0.1
p6 all-clusters-app default (read/write) 2404872 2401592 -3280 -0.1
.bss 117020 116804 -216 -0.2
.data 2592 2592 0 0.0
.text 1363136 1359856 -3280 -0.2
light-app default (read/write) 2326832 2323616 -3216 -0.1
.bss 105888 105672 -216 -0.2
.data 2384 2384 0 0.0
.text 1285096 1281880 -3216 -0.3
lock-app default (read/write) 2299032 2295840 -3192 -0.1
.bss 104768 104552 -216 -0.2
.data 2336 2336 0 0.0
.text 1257296 1254104 -3192 -0.3
qpg lighting-app qpg6105+debug (read only) 533268 533132 -136 -0.0
(read/write) 146936 146936 0 0.0
.bss 86688 86624 -64 -0.1
.data 1004 1004 0 0.0
.text 527948 527812 -136 -0.0
lock-app qpg6105+debug (read only) 505044 504908 -136 -0.0
(read/write) 146940 146940 0 0.0
.bss 85824 85760 -64 -0.1
.data 952 952 0 0.0
.text 499724 499588 -136 -0.0
persistent-storage-app qpg6105+debug (read only) 106448 106448 0 0.0
(read/write) 146938 146938 0 0.0
.bss 36146 36146 0 0.0
.data 288 288 0 0.0
.text 101128 101128 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 834102 833678 -424 -0.1
bss 86864 86776 -88 -0.1
noinit 37160 37160 0 0.0
text 582240 582086 -154 -0.0

@andy31415 andy31415 merged commit 7a4028d into project-chip:master Jan 7, 2022
msandstedt added a commit to msandstedt/connectedhomeip that referenced this pull request Jan 7, 2022
@msandstedt
Fix project-chip#13377 for compatibility with project-chip#13330
8f3933a 
The pull requests project-chip#13330 and project-chip#13377 were merged around the same time,
and have caused a small build breakage.  This fixes it.
@msandstedt msandstedt mentioned this pull request Jan 7, 2022
Merged
andy31415 pushed a commit that referenced this pull request Jan 7, 2022
@msandstedt
Fix #13377 for compatibility with #13330 (#13382)
c25c4b3 
The pull requests #13330 and #13377 were merged around the same time,
and have caused a small build breakage.  This fixes it.
msandstedt added a commit to msandstedt/connectedhomeip that referenced this pull request Jan 7, 2022
@msandstedt
Revise for compatibility with project-chip#13330
fac4c76
msandstedt added a commit to msandstedt/connectedhomeip that referenced this pull request Jan 7, 2022
@msandstedt
Revise for compatibility with project-chip#13330
9357cff
This was referenced Jan 8, 2022
Closed
Open
@kghost kghost deleted the exchange-session branch January 9, 2022 06:51
@doru91
Copy link
Contributor

doru91 commented Jan 10, 2022 •
edited
Loading

@doru91 - I have patched the NXP SDK to remove the 'define global' as I have not seen its usage and it seems that for matter it is somewhat common to patch the NXP SDK. Please take a look and see if there is a better approach.

@andy31415 the fix is good to me. I also like the way you've chosen to patch only the required lines instead of copying the entire file (as is done in the existing script file).

@andersbangGF andersbangGF mentioned this pull request Jan 12, 2022
Closed
@bzbarsky-apple bzbarsky-apple mentioned this pull request Jan 12, 2022
Closed
@andersbangGF andersbangGF mentioned this pull request Jan 13, 2022
Merged
msandstedt added a commit to msandstedt/connectedhomeip that referenced this pull request Jan 27, 2022
@msandstedt
Revise for compatibility with project-chip#13330
290bf85
msandstedt added a commit to msandstedt/connectedhomeip that referenced this pull request Feb 3, 2022
@msandstedt
Revise for compatibility with project-chip#13330
ef6f5bb
step0035 pushed a commit to hank820/connectedhomeip that referenced this pull request Feb 8, 2022
@kghost @andy31415 @step0035
Refactor SessionHandle (project-chip#13330)
b1e9788 
* Fix EPS32 build warning

* app/ReadPrepareParams: store SessionHolder instead of SessionHandle

* Refactor SessionHandle

* Fix GetFabricIndex/GetPeerNodeId calls in general commissioning server

* Fix usage of . instead of -> (tested with esp32 compile)

* Remove the define of global for nxp compilation: conflicts with STL usage and does not seem to be utilized at least for matter  builds

* Restyle fixes

* Fix getting peer address compilation for exchange context

Co-authored-by: Andrei Litvin <[email protected]>
step0035 pushed a commit to hank820/connectedhomeip that referenced this pull request Feb 8, 2022
@msandstedt @step0035
Fix project-chip#13377 for compatibility with project-chip#13330 (pro…
79320b1 
…ject-chip#13382)

The pull requests project-chip#13330 and project-chip#13377 were merged around the same time,
and have caused a small build breakage.  This fixes it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msandstedt msandstedt msandstedt approved these changes

@andy31415 andy31415 andy31415 approved these changes

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

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

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

@bzbarsky-apple bzbarsky-apple Awaiting requested review from bzbarsky-apple

@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

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@emargolis emargolis Awaiting requested review from emargolis

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

@gjc13 gjc13 Awaiting requested review from gjc13

@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

@lzgrablic02 lzgrablic02 Awaiting requested review from lzgrablic02

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

@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

@turon turon Awaiting requested review from turon

@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

@yufengwangca yufengwangca Awaiting requested review from yufengwangca

Assignees
No one assigned
Labels
app controller examples lib review - approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kghost @andy31415 @doru91 @msandstedt