Bulk read all the things #14752
Merged
Bulk read all the things #14752
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,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
LuDuda,
lzgrablic02,
mrjerryjohns,
msandstedt and
robszewczyk
pullapprove
bot
requested review from
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
|
PR #14752: Size comparison from 1ee9366 to a17e345 Increases (1 build for linux)
Decreases (1 build for linux)
Full report (17 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
|
PR #14752: Size comparison from 1ee9366 to 7d45fbb Increases (33 builds for cyw30739, efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (5 builds for esp32, linux)
Full report (43 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
andy31415
approved these changes
Feb 3, 2022
| } | ||
| // Using ForEachAttribute because this attribute can be queried on any endpoint. | ||
| err = mAttributeCache->ForEachAttribute( | ||
| app::Clusters::GeneralCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) { |
There was a problem hiding this comment.
For what it's worth, the device library spec does define that General Commissioning has to be on endpoint 0 and only endpoint 0.
There was a problem hiding this comment.
Root node device type needs to be on EP0, but it doesn't seem to preclude having a general commissioning cluster on other endpoints - similar to the argument that was made for having multiple networking clusters. It's less likely, but I don't think it's banned.
There was a problem hiding this comment.
It absolutely does preclude it. See device library spec. The "General Commissioning" cluster has the "I" quality, which means "singleton". "Network Commissioning", on the other hand, does not have that quality.
So "General Commissioning" is required to be present only once, and is required to be present on endpoint 0.
Followup OK to deal with this, in the interests of getting this merged.
| DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context); | ||
| commissioner->CommissioningStageComplete(status); | ||
| } | ||
| err = mAttributeCache->ForEachAttribute(app::Clusters::Basic::Id, [this, &info](const app::ConcreteAttributePath & path) { |
There was a problem hiding this comment.
Likewise, per spec this is always only on endpoint 0.
woody-apple
approved these changes
Feb 8, 2022
|
/rebase |
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Get calls need explicit templating.
woody-apple
force-pushed
the
bulk_read_all_the_things
branch
from
784f89a to
427e929
Compare
|
PR #14752: Size comparison from b5776df to 427e929 Increases (1 build for linux)
Decreases (1 build for linux)
Full report (34 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
approved these changes
Feb 8, 2022
| } | ||
| app::Clusters::GeneralCommissioning::Attributes::BasicCommissioningInfo::TypeInfo::DecodableType basicInfo; | ||
| ReturnErrorOnFailure( | ||
| this->mAttributeCache->Get<app::Clusters::GeneralCommissioning::Attributes::BasicCommissioningInfo::TypeInfo>( |
There was a problem hiding this comment.
Ugh. I did not realize that the attribute cache requires the template arg to work right... but of course it does so it can look up things on it.
I'll see if there's anything we can do to make this better.
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
Given that we're already bulk reading the network feature maps, we may as well just piggy back all the other attribute reads in there as well because it's fewer interactions. Also now that #14671 has landed, we can use the general commissioning cluster attributes.
Change overview
Testing
Commissioned BLE (M5) and on network (lightning app). Confirmed recommended failsafe comes from the device.