Remove [commissioner] redundant TC acknowledgement flag #36966
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
The flag controlling whether to require TC acknowledgement is no longer needed since TC acceptance is now a mandatory pre-condition for commissioning. This flag was originally added to support delayed TC acceptance during the commissioning process, but since that flow has been removed, the flag serves no purpose. The TC acknowledgement state itself is still required and maintained, but the additional boolean flag controlling the requirement is redundant and can be safely removed.
|
Review changes with Changed Files
|
|
PR #36966: Size comparison from 630531c to 2eb233c Full report (42 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36966: Size comparison from 630531c to 19d1806 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
Jan 7, 2025
There was a problem hiding this comment.
@swan-amazon please add a ###Testing section describing how this was tested
bzbarsky-apple
approved these changes
Jan 7, 2025
|
PR #36966: Size comparison from 7558fe1 to 65fa7d4 Full report (70 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36966: Size comparison from 8a0fc4d to d48400a Full report (56 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36966: Size comparison from 8a0fc4d to 97a073f Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
pushed a commit
to cecille/connectedhomeip
that referenced
this pull request
Feb 10, 2025
…#36966) * Remove [commissioner] redundant TC acknowledgement flag The flag controlling whether to require TC acknowledgement is no longer needed since TC acceptance is now a mandatory pre-condition for commissioning. This flag was originally added to support delayed TC acceptance during the commissioning process, but since that flow has been removed, the flag serves no purpose. The TC acknowledgement state itself is still required and maintained, but the additional boolean flag controlling the requirement is redundant and can be safely removed.
andy31415
added a commit
that referenced
this pull request
Feb 11, 2025
* Revert "Update TCUpdateDeadline to be nullable to match spec (#37438) (#37478)" This reverts commit 37a2d68. * Fix zap Something about the last cherry pick broke zap. Re-applying the changes manually and re-running zap. * Feature/enhanced setup flow feature (#34065) * Add initial feature logic for Terms and Conditions (TC) acknowledgements This commit introduces the initial logic for handling Terms and Conditions (TC) acknowledgements in the General Commissioning cluster. The logic includes support for setting and checking TC acknowledgements and versions during the commissioning process. Changes include: - Handling TC acknowledgements and TC acknowledgement version in the pairing command. - Logic to read TC attributes and check TC acceptance in the General Commissioning server. - Introduction of classes to manage TC acceptance logic. - Initialization and use of TC providers in the server setup. - Addition of a new commissioning stage for TC acknowledgements in the commissioning flow. The feature logic is currently disabled and will be enabled in an example in a subsequent commit. * ./scripts/helpers/restyle-diff.sh @{u} * Ignore file reference check on TermsAndConditionsManager.cpp The TermsAndConditionsManager.cpp file is only referenced within sample apps that utilize the Terms and Conditions feature. * Make `terms and conditions required` build configurable: - Moved the configuration from core into app buildconfig - renamed the flag to expand `TC` into `TERMS AND CONDITIONS` - updated includes in general-commissioning to include the right header - added the configuration as a build option into targets.py/host.py - updated unit test * Move terms and conditions to its own target and include cpp file - Create a separate source set for terms and conditions - include the manager cpp in that file - make the build conditional (this required flag moving) - fixed typo in targets.py to make things compile Compile-tested only (the -terms-and-conditions variant of all clusters compiled) * Fix mangled license blurb * Remove edited date for CHIPConfig.h * Fix unit tests dependencies * Add back some includes --------- Co-authored-by: Andrei Litvin <[email protected]> * Decouple ember functions from general commissioning cluster (#36836) * Decouple ember functions from general commissioning cluster * Address review comments * Rename gAttrAccess * Remove new added log info * Flag SetTCAcknowledgements command * Revert "Flag SetTCAcknowledgements command" This reverts commit 90de8a1. * Add the original debug log back * feat: Add terms-and-conditions sample app for CI testing (#36950) * feat: Add terms-and-conditions sample app for CI testing Add a new sample application that demonstrates and tests the terms and conditions feature. This app will be integrated into the CI pipeline to prevent regressions and ensure the feature continues to work as expected. The sample app serves as both a reference implementation and an automated test case for the terms and conditions functionality. ```bash ./scripts/build/build_examples.py --target linux-x64-terms-and-conditions build ``` * Update cluster configurations for app testing - Enable switch cluster for basic app control testing - Enable network commissioning cluster for in-app commission testing - Disable provisional clusters as they are not required - Disable other non-required clusters to streamline testing setup This change focuses the cluster configuration on essential components needed for basic app control and commissioning testing scenarios. * refactor: Remove unused attributes from Switch cluster Remove unused attributes from the Switch cluster to comply with the "On/Off Light" device type specification as defined in the Matter device library v1.4. This change removes several server-side attributes including ServerList, ClientList, PartsList, and various internal lists that are not required for the basic On/Off Light functionality. * feat: Add Descriptor cluster and enable attribute reporting Add the mandatory Descriptor cluster to endpoint 1 and enable attribute reporting for the device. Changes include: - Add Descriptor cluster with all required attributes (deviceTypeList, serverList, etc.) - Enable reportable flag for multiple attributes with min/max reporting intervals - Configure all Descriptor cluster attributes as external callbacks This change ensures compliance with the Matter specification for the On/Off Light device type. * Add Terms and Conditions (T&C) Feature Support for Commissioning (#36863) * Add Terms and Conditions (T&C) Feature Support for Commissioning 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. * Removed the "wait for terms and conditions stage" The wait-stage is not required. The user input availability must be a pre-condition for starting the AutoCommissioner process. The wait stage was previously to support requesting user input after identifying the device VID/PID using a different channel than within the pairing payload. * [doc] Improve Terms and Conditions commissioning arguments documentation Updated documentation for T&C-related commissioning arguments to better reflect their actual usage and purpose: - require-tc-acknowledgements: Clarified the impact on commissioning flow - tc-acknowledgements: Explained the bit-field usage for user acceptance - tc-acknowledgements-version: Added context about version tracking * [controller] Remove T&C acknowledgements from external buffer clearing The Terms and Conditions acknowledgements parameter was incorrectly included in ClearExternalBufferDependentValues(). This parameter contains a fixed-size struct with two uint16_t values and does not reference any external buffers. The CommissioningParameters class appears to be designed for additive-only parameter setting without explicit clear/reset functionality, so removing this inappropriate clearing operation is the correct approach. * [controller] Fix CommissioningStage enum order for T&C acknowledgements Move kConfigureTCAcknowledgments before kCleanup in the CommissioningStage enum to fix cirque test failures. The tests validate that commissioning stages do not exceed kCleanup, which was causing failures when T&C acknowledgements were positioned after it. While the original comment from 2 years ago suggested the enum order was fixed, testing reveals that the stages can be reordered. The cirque tests now pass with this corrected ordering, indicating that any previous constraints on enum ordering no longer apply. * [doc] Clarify required arguments for T&C acknowledgements Update help text for require-tc-acknowledgements to explicitly state which arguments must be provided when T&C acknowledgements are required. Instead of the vague "valid T&C acknowledgements", now specifically mentions that both tc-acknowledgements and tc-acknowledgements-version arguments are mandatory for successful commissioning when this option is enabled. * Update src/python_testing/matter_testing_infrastructure/chip/testing/matter_testing.py Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Restyle * Renamed variable --------- Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Remove [commissioner] redundant TC acknowledgement flag (#36966) * Remove [commissioner] redundant TC acknowledgement flag The flag controlling whether to require TC acknowledgement is no longer needed since TC acceptance is now a mandatory pre-condition for commissioning. This flag was originally added to support delayed TC acceptance during the commissioning process, but since that flow has been removed, the flag serves no purpose. The TC acknowledgement state itself is still required and maintained, but the additional boolean flag controlling the requirement is redundant and can be safely removed. * [chip-tool] Add Enhanced Commissioning Support (T&C Flow via Local DCL) to chip-tool (#37049) * [chip-tool] Add chip-tool dcl fake cluster commands * [chip-tool] Add a fake local dcl server script for testing/developement purposes * [chip-tool] Add chip-tool dcl tc-display and tc-display-by-payload commands * [General Commissioning Server] Dynamically encode the feature map 'GeneralCommissioning::Feature::kTermsAndConditions' if CHIP_CONFIG_TERMS_AND_CONDITIONS_REQUIRED is set * [Examples/platform/linux] Set default TermsAndConditions if requested from the command line * [chip-tool] Add TermsAndConditions support to chip-tool pairing code command * Reduce the feature level on zap file * Re-run generate on terms and conditions zap * test: Implement TC acknowledgement test cases (#37015) * test: Implement TC acknowledgement test cases 2.5-2.8 Implements test cases for Terms & Conditions acknowledgement verification: - TC-*-2.5: SetTCAcknowledgements verification - TC-*-2.6: CommissioningComplete with no terms accepted - TC-*-2.7: CommissioningComplete with invalid terms - TC-*-2.8: TCAcknowledgements reset after Factory Reset Remaining test cases to be implemented in follow-up changes: - TC-*-2.9: Reset after fabric removal - TC-*-2.10: Required terms validation - TC-*-2.11: Post-commission updates Testing: Test cases implemented according to test plan specifications. Each test verifies specific TC acknowledgement behaviors. * test: Implement remaining TC acknowledgement test cases 2.9-2.11 Implements remaining test cases for Terms & Conditions acknowledgement verification: - TC-*-2.9: TCAcknowledgements reset after fabric removal - TC-*-2.10: Required terms validation - TC-*-2.11: Post-commission TC updates This completes the test coverage for TC acknowledgement verification, following up on the previous implementation of test cases 2.5-2.8. The new test cases verify: - TC state after fabric removal - Protection of required terms - Ability to update TC version and acknowledgements post-commissioning Testing: Test cases implemented according to test plan specifications. Each test verifies specific TC acknowledgement behaviors. * wip * feat(testing): Add PICS guard for Terms & Conditions test steps - Wrap test steps in PICS guard checks for "CGEN.S" and "CGEN.S.F00(TC)" - Ensure test steps only execute when Terms & Conditions feature flag is enabled - Maintain test logic while adding conditional execution based on PICS support * Added PICS and PIXIT definitions to src/app/tests/suites/certification/PICS.yaml * refactor: Extract commission_devices functionality into reusable methods Move commissioning logic from CommissionDeviceTest class into standalone functions to improve code reusability. Introduce CommissioningInfo dataclass to encapsulate commissioning parameters and add commission_device/ commission_devices helper functions. The changes: - Move SetupPayloadInfo dataclass to module level - Add new CommissioningInfo dataclass for commissioning parameters - Extract commission_device and commission_devices as standalone async functions - Add commission_devices method to MatterBaseTest class - Maintain backward compatibility with existing usage in CommissionDeviceTest * Addressed PR comments * Improve test assertions and manual testing steps - Use Matter-specific type assertions in TC_CGEN_2_5 - Update TC_CGEN_2_8 manual testing flow and prompts * restyle * refactor: simplify CGEN feature identifier from CGEN.S.F00(TC) to CGEN.S.F00 - Update PICS.yaml and ci-pics-values to use simplified feature ID - Update Python test files to use new feature identifier - Improve error messaging when commissioning feature check fails - Affected test files: TC_CGEN_2_5 through TC_CGEN_2_11 * Update TC_CGEN_2_5 to use Nullable type for TCUpdateDeadline validation * Add type validation for TCAcceptedVersion and TCAcknowledgements * add matter_asserts file from master * Fix imports on new tests * fix CI arguments * Fix bad merge on all_targets_linux_x64.txt * Remove duplicate commission_devices method to fix F811 flake8 error Removes the redundant definition of commission_devices method from MatterBaseTest class to resolve the flake8 F811 error which indicated a redefinition of an unused function from line 1058. * fix: Invert boolean return in GetAcknowledgementsRequest (#37014) * fix: Invert boolean return in GetAcknowledgementsRequest The GetAcknowledgementsRequest was returning an incorrect boolean value, causing unexpected behavior. This change inverts the return value to fix the logical error. Testing: - Issue identified during test plan script development - Test plan changes will be referenced in a separate change --------- Co-authored-by: Cecille Freeman <[email protected]> Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Yufeng Wang <[email protected]> Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> Co-authored-by: Vivien Nicolas <[email protected]>
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.
Fixes #36965
The flag controlling whether to require TC acknowledgement is no longer needed since TC acceptance is now a mandatory pre-condition for commissioning. This flag was originally added to support delayed TC acceptance during the commissioning process, but since that flow has been removed, the flag serves no purpose.
The TC acknowledgement state itself is still required and maintained, but the additional boolean flag controlling the requirement is redundant and can be safely removed.
Testing
Used chip-tool to commission the terms-and-conditions sample app that is currently under review. See pull request #36950