Add Terms and Conditions (T&C) Feature Support for Commissioning #36863
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
This commit introduces comprehensive support for handling Terms and
Conditions (T&C) during the commissioning process, enhancing
compatibility with Matter device certification requirements.
Key changes include:
1. **Commissioning Process Updates**:
- Introduced `SetRequireTermsAndConditionsAcknowledgement`,
`SetTermsAcknowledgements`, and `SetSkipCommissioningComplete` APIs
in the commissioning library.
- Updated commissioning stages to include `kGetTCAcknowledgments` and
`kConfigureTCAcknowledgments` for seamless integration of T&C
acknowledgements.
- Added methods for processing T&C acknowledgements and advancing
commissioning stages upon user response.
2. **Test Framework Enhancements**:
- Added arguments (`tc_version`, `tc_user_response`,
`in_test_commissioning_method`) for specifying T&C configuration in
tests.
- Enhanced `populate_commissioning_args` to manage new T&C-related
arguments.
- Updated Python test bindings and Matter test infrastructure to
support T&C workflows.
3. **Chip-Tool Improvements**:
- Extended `PairingCommand` to handle T&C-related arguments
(`require-tc-acknowledgements`, `tc-acknowledgements`,
`tc-acknowledgements-version`) for test scenarios.
- Ensured backward compatibility by defaulting new parameters to
preserve pre-1.4 behavior.
|
Review changes with Changed Files
|
github-actions
bot
added
tools
examples
controller
tests
matter-1.4-te2-script-change
|
PR #36863: Size comparison from 43f66f0 to 30d5e4d 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
requested changes
Dec 16, 2024
Comment on lines
+207
to
+208
| "Terms and Conditions acknowledgements is known to be required or not, (when required, the commissioner " | ||
| "will wait until they are provided)"); |
There was a problem hiding this comment.
This does not seem like the right description, in that there is no waiting; when required you can provide them via the other args, right?
Also descriptions for optional args should say what the default behavior is.
| return; | ||
| } | ||
|
|
||
| ChipLogProgress(Controller, "Waiting for Terms and Conditions"); |
There was a problem hiding this comment.
Doesn't this need to extend the fail-safe or something? What happens if the user takes 10 minutes to read the terms (which is a bare minimum for how long it can possibly take to actually read those things typically)?
| /** | ||
| * @brief | ||
| * This function is called by the upper layer application to indicate that the required terms and conditions | ||
| * acknowledgements have been set. This function should be called after the terms and conditions bitmask and version |
There was a problem hiding this comment.
"Have been set" where and how?
| * @brief | ||
| * This function is called by the upper layer application to indicate that the required terms and conditions | ||
| * acknowledgements have been set. This function should be called after the terms and conditions bitmask and version | ||
| * have been defined using the appropriate configuration macros and the application has gathered the necessary |
There was a problem hiding this comment.
Which configuration macros, exactly?
| * have been defined using the appropriate configuration macros and the application has gathered the necessary | ||
| * acknowledgements from the user. | ||
| * | ||
| * The upper layer application should call this method once it has successfully presented and obtained acknowledgements |
There was a problem hiding this comment.
How does the upper layer application know it needs to do that?
| kRemoveThreadNetworkConfig, ///< Remove Thread network config. | ||
| kGetTCAcknowledgments, ///< Waiting for the higher layer to provide terms and conditions acknowledgements. | ||
| kConfigureTCAcknowledgments, ///< Send SetTCAcknowledgements (0x30:6) command to the device | ||
| kCleanup, ///< Call delegates with status, free memory, clear timers and state |
There was a problem hiding this comment.
The comments above claim this value cannot be moved later in the list. Either it can and the comments need fixing, or it cannot and should not be.
| @@ -581,6 +609,7 @@ class CommissioningParameters | |||
| mAttestationNonce.ClearValue(); | |||
| mWiFiCreds.ClearValue(); | |||
| mCountryCode.ClearValue(); | |||
| mTermsAndConditionsAcknowledgement.ClearValue(); | |||
There was a problem hiding this comment.
Why? This is not an "ExternalBufferDependentValue", is it?
|
PR #36863: Size comparison from 4e44586 to 5ce33b6 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)
|
| * @return CHIP_ERROR The return status. Returns CHIP_ERROR_INCORRECT_STATE if the function is called when the device | ||
| * is not in the correct state to accept terms and conditions acknowledgements. | ||
| */ | ||
| CHIP_ERROR TermsAndConditionsAcknowledgementsReady(); |
There was a problem hiding this comment.
One other very important thing: this API surface would ensure that we can never do multiple commissionings in parallel, because nothing ties this call to a specific instance of the commissioning process. This is even an issue or "serial" commissionings: one might get canceled and another started, then this call comes in for the first one....
This needs a better solution.
|
PR #36863: Size comparison from 37fa873 to 5ce33b6 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)
|
|
This PR added 14720 bytes to the fabric-sync app |
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 commit introduces comprehensive support for handling Terms and Conditions (T&C) during the commissioning process, enhancing compatibility with Matter device certification requirements.
Key changes include:
Commissioning Process Updates:
SetRequireTermsAndConditionsAcknowledgement,SetTermsAcknowledgements, andSetSkipCommissioningCompleteAPIs in the commissioning library.kGetTCAcknowledgmentsandkConfigureTCAcknowledgmentsfor seamless integration of T&C acknowledgements.Test Framework Enhancements:
tc_version,tc_user_response,in_test_commissioning_method) for specifying T&C configuration in tests.populate_commissioning_argsto manage new T&C-related arguments.Chip-Tool Improvements:
PairingCommandto handle T&C-related arguments (require-tc-acknowledgements,tc-acknowledgements,tc-acknowledgements-version) for test scenarios.