[app] add binding cluster support #12981
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gjc13
force-pushed
the
add-binding-manager
branch
from
2fab02a to
0af24be
Compare
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
LuDuda,
lzgrablic02,
msandstedt and
mspang
|
PR #12981: Size comparison from 628d7ba to a9aafb3 Increases above 0.2%:
Increases (11 builds for esp32, k32w, linux, mbed, p6, qpg, telink)
Decreases (17 builds for efr32, k32w, nrfconnect, p6, qpg)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
PR #12981: Size comparison from 628d7ba to d93edf7 Increases above 0.2%:
Increases (11 builds for esp32, k32w, linux, mbed, p6, qpg, telink)
Decreases (17 builds for efr32, k32w, nrfconnect, p6, qpg)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
andy31415
dismissed stale reviews from bzbarsky-apple and mrjerryjohns
Re-review was requested after code changes.
bzbarsky-apple
approved these changes
Jan 19, 2022
The change adds `BindingManager` class for managing the connections to bound devices and forward events on bound clusters to the application.
gjc13
force-pushed
the
add-binding-manager
branch
from
d93edf7 to
9ad1478
Compare
|
PR #12981: Size comparison from 3853608 to 9ad1478 Increases above 0.2%:
Increases (7 builds for k32w, linux, mbed, p6, qpg, telink)
Decreases (23 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (30 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
PR #12981: Size comparison from 3853608 to b58253f Increases above 0.2%:
Increases (3 builds for linux, telink)
Decreases (6 builds for k32w, linux, qpg, telink)
Full report (8 builds for k32w, linux, qpg, telink)
|
gjc13
force-pushed
the
add-binding-manager
branch
from
b58253f to
88864ff
Compare
|
PR #12981: Size comparison from b700fb8 to 88864ff Increases above 0.2%:
Increases (11 builds for esp32, k32w, linux, mbed, p6, qpg, telink)
Decreases (17 builds for efr32, k32w, nrfconnect, p6, qpg)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
chshu
approved these changes
Jan 20, 2022
kedars
approved these changes
Jan 20, 2022
| { | ||
| // Release the least recently used entry | ||
| // TODO: Some reference counting mechanism shall be added the CASESessionManager | ||
| // so that other session clients don't get accidentally closed. |
There was a problem hiding this comment.
The actual logic to decide which CASE sessions to evict won't be here right? i.e this is just short term?
There was a problem hiding this comment.
Yes, we'll submit follow up PRs about the CASE session management.
| static void ToggleSwitchOnOff(bool newState) | ||
| { | ||
| sSwitchOnOffState = newState; | ||
| chip::BindingManager::GetInstance().NotifyBoundClusterChanged(1, chip::app::Clusters::OnOff::Id, nullptr); |
There was a problem hiding this comment.
What happens if the switch is toggled quickly 'N' times in succession, and before BoundDeviceChangedHandler can be called? Does it result in N calls being made to BoundDeviceChangedHandler from the Binding manager?
There was a problem hiding this comment.
If we need to connect to the unicast binding first, we'll check for duplication when pushing to the pending nofication queue. Only one call will be made under such circumstance.
If the binding is ready, we'll directly call the handler. It will be up to the application to do anti-jittering.
selissia
pushed a commit
to selissia/connectedhomeip
that referenced
this pull request
Jan 28, 2022
* [app] add binding cluster support The change adds `BindingManager` class for managing the connections to bound devices and forward events on bound clusters to the application. * fix review comments & add docs * decouple client from server attributes * Use interaction engine to send On/Off commands * add group to the callback signature * fix review comments * add docs for binding callback arguments * fix unbind * modify callback signature * make BindingManager instance optional * Modify struct layout to save RAM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Binding cluster is missing in the current SDK
Change overview
The change adds
BindingManagerclass for managing the connections tobound devices and forward events on bound clusters to the application.
TODOs
Following tasks need to be accomplished to fully support binding in the future PRs
bind/unbindcomand.Testing
Build
all-cluster-appwith gn argschip_build_libshell=trueRun two linux all-cluster-app in different docker containers or ip namespaces.
Now in the second device's shell: