[binding] Make binding table an attribute #14874
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
pullapprove
bot
requested review from
andy31415,
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
LuDuda,
lzgrablic02,
mlepage-google and
msandstedt
pullapprove
bot
requested review from
sagar-apple,
saurabhst,
selissia,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
gjc13
force-pushed
the
binding-attribute
branch
from
3151416 to
0d225c7
Compare
gjc13
force-pushed
the
binding-attribute
branch
from
0d225c7 to
205a6df
Compare
|
PR #14874: Size comparison from 2f785f4 to 205a6df Increases above 0.2%:
Increases (8 builds for cyw30739, k32w, linux, p6, qpg, telink)
Decreases (6 builds for efr32, linux, p6)
Full report (17 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
|
PR #14874: Size comparison from 2f785f4 to 66840dd Increases above 0.2%:
Increases (28 builds for cyw30739, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (7 builds for efr32, linux, p6)
Full report (43 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
woody-apple
approved these changes
Feb 8, 2022
andy31415
approved these changes
Feb 8, 2022
Damian-Nordic
approved these changes
Feb 8, 2022
| @@ -37,7 +37,7 @@ class BridgedActionsAttrAccess : public AttributeAccessInterface | |||
| // Register for the Bridged Actions cluster on all endpoints. | |||
| BridgedActionsAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), BridgedActions::Id) {} | |||
|
|
|||
| CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; | |||
| CHIP_ERROR Read(FabricIndex fabricIndex, const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; | |||
There was a problem hiding this comment.
This change should not have been made. The fabric index was already available as aEncoder.AccessingFabricIndex().
| { | ||
| // We already have an active connection | ||
| mBoundDeviceChangedHandler(&entry, peerDevice, context); | ||
| } | ||
| else | ||
| { | ||
| mPendingNotificationMap.AddPendingNotification(i, context); | ||
| ReturnErrorOnFailure(EstablishConnection(entry.fabricIndex, entry.nodeId)); | ||
| if (!peerDevice->IsConnecting()) |
There was a problem hiding this comment.
But what if peerDevice is null here? Won't this crash?
|
|
||
| CHIP_ERROR BindingTableAccess::ReadBindingTable(FabricIndex fabricIndex, EndpointId endpoint, AttributeValueEncoder & encoder) | ||
| { | ||
| DeviceLayer::AttributeList<Structs::BindingEntry::Type, EMBER_BINDING_TABLE_SIZE> bindingTable; |
There was a problem hiding this comment.
Why? Why can't we just encode as we go? Why does the whole list need to be built on the stack? It doesn't.
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Need to check for decode failure, no?
|
|
||
| ReturnErrorOnFailure(decoder.Decode(newBindingList)); | ||
|
|
||
| // Add entries currently not in the binding table |
There was a problem hiding this comment.
This is wrong. Per spec, the order here matters, and this is not going to preserve the order of the incoming list...
| emberDeleteBinding(bindingIndex); | ||
| emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_SUCCESS); | ||
| return true; | ||
| // Remove entries not in the new binding list |
There was a problem hiding this comment.
Ignoring the ordering for the moment, this will completely fail if I try to overwrite the whole list with an equal length list, no? If EMBER_BINDING_TABLE_SIZE == 16 and we already have 16 bindings and I try to write 16 new ones... we will try to add before we remove and it will just fail.
Comment on lines
+20
to
+26
| <struct name="BindingEntry"> | ||
| <cluster code="0x001e"/> | ||
| <item name="nodeId" type="NODE_ID"/> | ||
| <item name="groupId" type="GROUP_ID"/> | ||
| <item name="endpointId" type="ENDPOINT_NO" /> | ||
| <item name="clusterId" type="CLUSTER_ID" /> | ||
| </struct> |
There was a problem hiding this comment.
I don't know where this came from, but this does not match the spec. The struct has a different name, it's supposed to have a fabric index, it's supposed to have optional fields, etc, etc.
Had this matched the spec, there would have been no need to add that fabric index argument at all, because it would be right in the struct, as required by the spec.
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Feb 8, 2022
This reverts commit 5834518. The PR made an unnecessary API mass change that should be reverted no matter what. It also introduces a null-dereference crash. Apart from that, the new code does not follow the spec in various ways: 1) Incorrect encoding of the binding target structs. 2) Does not preserve order, as the spec requires. 3) Will fail on legal writes of a list that completely replaces an existing list if the _sum_ of the lengths does not fit in our storage. 5) Does not do required error-checking on inputs.
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Feb 9, 2022
This reverts commit 5834518. The PR made an unnecessary API mass change that should be reverted no matter what. It also introduces a null-dereference crash. Apart from that, the new code does not follow the spec in various ways: 1) Incorrect encoding of the binding target structs. 2) Does not preserve order, as the spec requires. 3) Will fail on legal writes of a list that completely replaces an existing list if the _sum_ of the lengths does not fit in our storage. 5) Does not do required error-checking on inputs.
bzbarsky-apple
added a commit
that referenced
this pull request
Feb 9, 2022
This reverts commit 5834518. The PR made an unnecessary API mass change that should be reverted no matter what. It also introduces a null-dereference crash. Apart from that, the new code does not follow the spec in various ways: 1) Incorrect encoding of the binding target structs. 2) Does not preserve order, as the spec requires. 3) Will fail on legal writes of a list that completely replaces an existing list if the _sum_ of the lengths does not fit in our storage. 5) Does not do required error-checking on inputs.
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
According to the spec the binding table should be an attribute while it's now exposed as commands.
Change overview
bindings.cppTesting
How was this tested? (at least one bullet point required)
Run and pair two linux
all-cluster-app.The read result is correct.
Type
switch onin node 2 and the command are received in node 1.The read result is correct and the
switchcommand cannot control node 1.