Include VID/PID in ECM commissioning Setup Payload #11894
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
|
@msandstedt, @jelderton, @andy31415 any review feedback? |
msandstedt
approved these changes
Nov 19, 2021
tcarmelveilleux
previously requested changes
Nov 19, 2021
There was a problem hiding this comment.
This change pushes into the core communication primitives a lot of policy/choices that should be the responsibility the of client/administrator.
Changes requested:
- Move generation of setup payload from passcode/vid/pid to the client (e.g. chip-tool as an example), since there should be one way to do it from passcode/discriminator/vid/pid in the SDK, not one for initial commissioning examples and one for commissioning window opening/
- Do not force the reading of the VID/PID every time from basic info cluster, but rather let the caller application determine if it's needed (since many/most will actually already know VID/PID at time of openign a window). You can take the code and move it to chip tool command of open commissioning Window, to show how an application could do it if it needed.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
@tcarmelveilleux @pan-apple what are the next steps here? This is part of the spec that we should implement in the SDK. |
woody-apple
approved these changes
Nov 30, 2021
c574fb9 to
fccb851
Compare
1964680 to
c98a87c
Compare
2 tasks
|
The latest change makes the reading of VID/PID optional. This would let applications control if they don't need the API to read VID/PID (e.g. if it's already known). This PR is needed for TE7.5, and there's not enough time to implement the suggested changes. |
|
PR #11894: Size comparison from 46fbb06 to c98a87c Increases above 0.2%:
Increases (2 builds for linux)
Decreases (2 builds for linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
Review comments seem addressed, and it's been a couple days.
|
Fast tracking, given this has been been in review for a while and comments seem addressed |
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
Change overview
Include VID and PID in the generated Setup Payload when commissioning window is opened using ECM mode.
Testing
Tested using all-cluster-app and chip-tool
chip-tool payload parse-setup-payloadcommand. Check that VID and PID are part of the code.