Refactor kReadCommissioningInfo step in DeviceCommissioner #36603
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 Changed Files
|
|
PR #36603: Size comparison from 04e6a68 to d16d2bb Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
ksperling-apple
force-pushed
the
commissioner-read-topology
branch
from
d16d2bb to
477feba
Compare
ksperling-apple
force-pushed
the
commissioner-read-topology
branch
from
477feba to
acd3d58
Compare
As of project-chip#32974 we were potentially cancelling later than intended. Also avoid re-using KVS file names to avoid pollution by earlier failed runs.
ksperling-apple
force-pushed
the
commissioner-read-topology
branch
3 times, most recently
from
9bcb5ba to
3cb04a6
Compare
|
PR #36603: Size comparison from 0ac52eb to 3cb04a6 Full report (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
…ack progress internally
…ally Also group requests by cluster. The logic of which attributes to read is unchanged.
ksperling-apple
force-pushed
the
commissioner-read-topology
branch
from
3cb04a6 to
db73e65
Compare
github-actions
bot
added
tests
matter-1.4-te2-script-change
|
PR #36603: Size comparison from 0ac52eb to db73e65 Full report (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
Group by cluster instead of by the stages that no longer exist. Also align error handling and logging a little.
|
PR #36603: Size comparison from 0ac52eb to 3739349 Increases above 0.2%:
Full report (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Use ClusterStateCache::Get variant that decodes directly - No need to loop again to parse ConnectMaxTimeSeconds attributes - Per spec Ethernet does not have ConnectMaxTimeSeconds
ksperling-apple
force-pushed
the
commissioner-read-topology
branch
from
716d4c7 to
e5ab93a
Compare
ksperling-apple
changed the title
|
PR #36603: Size comparison from 29c2647 to e5ab93a Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36603: Size comparison from 29c2647 to 4f8b058 Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
reviewed
Nov 29, 2024
|
PR #36603: Size comparison from 29c2647 to a2f8a86 Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
approved these changes
Dec 5, 2024
src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase+ServerAppRunner.m
Show resolved
Hide resolved
cecille
approved these changes
Dec 5, 2024
yyzhong-g
pushed a commit
to yyzhong-g/connectedhomeip
that referenced
this pull request
Dec 12, 2024
…hip#36603) * Fix trigger-based cancellation in MTRPairingTests As of project-chip#32974 we were potentially cancelling later than intended. Also avoid re-using KVS file names to avoid pollution by earlier failed runs. * Roll kReadCommissioningInfo2 stage into kReadCommissioningInfo and track progress internally * Refactor ContinueReadingCommissioningInfo to group attributes dynamically Also group requests by cluster. The logic of which attributes to read is unchanged. * Refactor FinishReadingCommissioningInfo / Parse*Info Group by cluster instead of by the stages that no longer exist. Also align error handling and logging a little. * Simplify ParseNetworkCommissioningInfo - Use ClusterStateCache::Get variant that decodes directly - No need to loop again to parse ConnectMaxTimeSeconds attributes - Per spec Ethernet does not have ConnectMaxTimeSeconds * Review comments: Use explicit ifs for this logic * Review comments: Explain skip logic * Remove workaround for zero network commissioning feature map * Review comment: document mReadCommissioningInfoProgress * Review comment: avoid paths / attributes ambiguity
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.
This PR refactors the kReadCommissioningInfo DeviceCommissioner step in preparation for adding the ability to read additional data. Having a fixed split of the attributes to be read into two steps in inflexible and makes it hard to optimize the reads that are performed.
Specifically this change
If desired this PR can be reviewed commit by commit; builds and tests were green after each commit.