Linux / Darwin: Enable passcode / verifier config from PosixConfig #39765
Conversation
This file contains hidden or 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
There was a problem hiding this comment.
Code Review
The pull request effectively implements the stated objectives, enabling the configuration of passcode, verifier, and discriminator settings from PosixConfig while maintaining command-line precedence and providing a guarded fallback to test parameters. The changes improve code correctness and maintainability by introducing const correctness for LinuxDeviceOptions parameters and refactoring the initialization logic into smaller, more focused helper functions. The updated documentation in the header file is also a valuable addition. The overall quality of the changes is high, and no critical, high, or medium severity issues were identified.
e05846d to
977ae4e
Compare
|
PR #39765: Size comparison from 18ec87c to 977ae4e Increases above 0.2%:
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- If discriminator, passcode, verifier and related options are not specified on the command line, look up the relevant keys in PosixConfig if present before falling back to hard-coded test values. - Guard using test config behind CHIP_DEVICE_CONFIG_ENABLE_TEST_SETUP_PARAMS=1 - Make the LinuxDeviceOptions parameters to the CommissionableInit functions const (and don't update the passed in options.payload anymore as this is not necessary or documented behavior).
977ae4e to
1db91a1
Compare
|
PR #39765: Size comparison from c4c6bea to 1db91a1 Increases above 0.2%:
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Linux platform commissioning setup by reading passcode, verifier, salt, iteration count, and discriminator from PosixConfig before falling back to test parameters, and updates function signatures to use const LinuxDeviceOptions&.
- Add helper functions in
CommissionableInit.cppto read optional values from PosixConfig and handle fallbacks. - Remove
TestOnlyCommissionableDataProviderand guard test parameters behindCHIP_DEVICE_CONFIG_ENABLE_TEST_SETUP_PARAMS. - Change
InitCommissionableDataProviderandInitConfigurationManagerto acceptconst LinuxDeviceOptions &and no longer modifyoptions.payload.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| examples/platform/linux/CommissionableInit.h | Updated doc comments; changed signatures to take const LinuxDeviceOptions &. |
| examples/platform/linux/CommissionableInit.cpp | Refactored commissioning init to use PosixConfig, added helper functions, removed test-only provider include. |
Comments suppressed due to low confidence (3)
examples/platform/linux/CommissionableInit.h:36
- [nitpick] The doc comment should note that
optionsis now passed asconstand is not modified by this function to avoid confusion about payload-side effects.
* @param options - LinuxDeviceOptions instance configured via command-line parsing
examples/platform/linux/CommissionableInit.cpp:33
- [nitpick] The helper name
IsNotFoundis quite generic—consider renaming it toIsConfigNotFoundor similar to clarify its purpose.
bool IsNotFound(CHIP_ERROR err)
examples/platform/linux/CommissionableInit.cpp:101
- New logic for reading/falling back salt from PosixConfig and behind test flags isn’t covered by existing CI tests; consider adding unit tests for
GetSpake2pSalt,GetIterationCount, andGetDiscriminator.
CHIP_ERROR GetSpake2pSalt(const LinuxDeviceOptions & options, chip::Optional<std::vector<uint8_t>> & outSalt)
kiel-apple
approved these changes
Jul 3, 2025
arkq
requested changes
Jul 4, 2025
|
PR #39765: Size comparison from c4c6bea to e591584 Increases above 0.2%:
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
arkq
approved these changes
Jul 7, 2025
dsavitsky-dsr
pushed a commit
to dsavitsky-dsr/connectedhomeip
that referenced
this pull request
Jul 7, 2025
…roject-chip#39765) * Linux / Darwin: Enable passcode / verifier config from PosixConfig - If discriminator, passcode, verifier and related options are not specified on the command line, look up the relevant keys in PosixConfig if present before falling back to hard-coded test values. - Guard using test config behind CHIP_DEVICE_CONFIG_ENABLE_TEST_SETUP_PARAMS=1 - Make the LinuxDeviceOptions parameters to the CommissionableInit functions const (and don't update the passed in options.payload anymore as this is not necessary or documented behavior). * Make error handling logic clearer as per review
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.
Summary
Testing
Existing CI tests verify that passing commissionable parameters via command line still works as before. Manually verified that pre-populating the PosixConfig file (kvs on Darwin or chip-factory.ini on Linux) works for configuring these settings as well, and that example apps refuse to start without configuration when compiling with CHIP_DEVICE_CONFIG_ENABLE_TEST_SETUP_PARAMS=0.