Change LevelStep in Valve cluster of example app to properly test the feature.#42849
Merged
andy31415 merged 1 commit intoproject-chip:masterfrom Jan 29, 2026
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request aims to change the levelStep attribute in the Valve Control cluster to facilitate CI testing. The change is correctly applied to both the .matter and .zap files for the levelStep attribute. However, there are several other modifications in the all-clusters-app.zap file that change other attributes from RAM-based storage to external (callback-based) storage. These changes seem unintentional as they are not mentioned in the pull request description and could introduce regressions. I've added a comment to highlight this potential issue.
|
PR #42849: Size comparison from a3ceb44 to ea26542 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
tersal
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve testing coverage for the Valve cluster's Open command by changing the LevelStep attribute value from 1 to 2. The motivation is that with LevelStep = 1, the constraint check (TargetLevel % LevelStep) == 0 is always true for any value, preventing proper testing of the CONSTRAINT_ERROR code path. Changing to LevelStep = 2 enables testing of values that should trigger this error (e.g., odd values like 1, 3, 5).
Changes:
- Changed the
LevelStepattribute of the Valve cluster from external storage with default 1 to RAM storage with default 2 on endpoint 1 - Unintentionally changed three
ClusterRevisionattributes from RAM to External storage (not mentioned in PR description)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| examples/all-clusters-app/all-clusters-common/all-clusters-app.zap | Contains four storage/default value changes: the intended LevelStep change and three undocumented ClusterRevision changes for Unit Localization and Scenes Management clusters |
| examples/all-clusters-app/all-clusters-common/all-clusters-app.matter | Reflects only the intended LevelStep change from callback to RAM storage with default value 2 |
andy31415
approved these changes
Jan 29, 2026
andy31415
added
the
sdk-maintainer-approved
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
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
A change to modify the LevelStep of the Valve cluster to properly test the Open feature.
The previous implementation in the example app used for testing the Valve cluster had a
LevelStepvalue of 1 and theOpencommand should returnConstraintErrorwhen theTargetLevelfield of said command is either 0, 100 or any value such as(TargetLevel % LevelStep) == 0.Since both values are
uintthat last check will always be true and this won't allow to properly test the error handling.Related issues
#42778
Testing
CI should pass.