Add the ability to read extra attributes during commissioning #36867
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
|
Review changes with |
ksperling-apple
force-pushed
the
read-custom-commissioning-info
branch
from
81f8173 to
30ff35a
Compare
ksperling-apple
force-pushed
the
read-custom-commissioning-info
branch
from
30ff35a to
76768e3
Compare
ksperling-apple
force-pushed
the
read-custom-commissioning-info
branch
2 times, most recently
from
0abedfb to
9492299
Compare
ksperling-apple
changed the title
|
PR #36867: Size comparison from ea94ef7 to 9492299 Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
ksperling-apple
force-pushed
the
read-custom-commissioning-info
branch
from
9492299 to
a9e5aac
Compare
|
PR #36867: Size comparison from c1afc02 to a9e5aac Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
|
PR #36867: Size comparison from c1afc02 to 9d73f29 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| // These values read from the device will be available in ReadCommissioningInfo.attributes. | ||
| // Clients should avoid requesting paths that are already read internally by the commissioner | ||
| // as no consolidation of internally read and extra paths provided here will be performed. | ||
| CommissioningParameters & SetExtraReadPaths(Span<const app::AttributePathParams> paths) |
There was a problem hiding this comment.
Should the dependency on CHIP_CONFIG_ENABLE_READ_CLIENT be documented? Or maybe these APIs should just be conditioned on that?
There was a problem hiding this comment.
It’s that dependency mess again… all of commissioning actually depends on ReadClient but the header is parsed even when it’s disabled.
There was a problem hiding this comment.
Fair, but feels like we should just make sure if the logic backing the API is not present, then the API should not be present.
There was a problem hiding this comment.
None of CommissioningParameters is meaningfully usable without CHIP_CONFIG_ENABLE_READ_CLIENT... I don't really want go further down that path of pretending that CHIP_CONFIG_ENABLE_READ_CLIENT disables a select few methods within these classes... All of DeviceCommissioner and all of CommissioningDelegate.h should be behind the ifdef (or really in a separate header.) I can have a look at making the ifdef more coarse-grained like that as a separate PR. Maybe when folks are back after the break we can look into splitting CHIPDeviceController.{cpp, h} into two files in a git-history-preserving way and tidy this up a bit better that way.
| break; // visited the root, DFS traversal done | ||
| } | ||
| } else /* endpoint->_mark == EndpointMark::Visited */ { | ||
| deque.pop_front(); // nothing else to do |
There was a problem hiding this comment.
So when would we reach this mark == Visited case? Seems to me that it's not reachable, since we put things on the end when we mark them visited, break out when we do that with the root endpoint, and start off with the root endpoint..... What am I missing?
There was a problem hiding this comment.
I've added a comment there and a couple more tests. This can actually happen in two ways:
In testPopulateChildren2, endpoint 2 is visited due to being a child of 1 (and the visit completed), then we visit it again from the root node. This is a valid configuration, the same as testPopulateChildren simply with a different PartsList order.
In testPopulateChildrenInvalidNonTree, endpoint 6 is a child of both 1 and 5, and these are in sibling branches of the tree. This is an invalid configuration, but not easily distinguishable from the other scenario.
(The valid / invalid return value is at this point primarily used for unit testing, since in practice I think we should be tolerant and not let some errant endpoint with invalid meta-data deter us from interacting with an otherwise valid device.)
ksperling-apple
force-pushed
the
read-custom-commissioning-info
branch
from
44abf92 to
200be69
Compare
- Move MTRDeviceTypeRevision out of ServerEndpoint directory - Move MTRProductIdentity into its own file - Implement NSCopying and equality on MTRDeviceType and MTRProductIdentity - Implement description on all 3 types - Add tests
Endpoint information is made availalable to the delegate via an MTRCommissioneeInfo object containing MTREndpointInfo objects.
Co-authored-by: Boris Zbarsky <[email protected]>
…DeviceTypeList Also add some additional comments to parsing logic and a couple more tests.
ksperling-apple
force-pushed
the
read-custom-commissioning-info
branch
from
c81026c to
b78a442
Compare
|
PR #36867: Size comparison from af336ec to b78a442 Full report (25 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, telink, tizen)
|
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.
Endpoint information is made availalable to the delegate via an MTRCommissioneeInfo object containing MTREndpointInfo objects.