-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Align Scenes with device_library #20031
Align Scenes with device_library #20031
Conversation
PR #20031: Size comparison from 9186af7 to bdf474f Increases (1 build for telink)
Decreases (1 build for cyw30739)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markus-becker-tridonic-com please update the summary - it still contains filler text from the templates. The "testing" section should probably just say not tested.
@@ -342,7 +342,7 @@ limitations under the License. | |||
<requireCommand>RemoveAllGroups</requireCommand> | |||
<requireCommand>AddGroupIfIdentifying</requireCommand> | |||
</include> | |||
<include cluster="Scenes" client="true" server="true" clientLocked="true" serverLocked="true"> | |||
<include cluster="Scenes" client="false" server="true" clientLocked="true" serverLocked="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this one clientLocked="true"
while On/Off light was clientLocked="false"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the semantics of clientLocked? If it is true, the GUI user cannot change whether the client's default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a question for @tecimovic ...
@@ -1577,7 +1577,7 @@ limitations under the License. | |||
<requireCommand>RemoveAllGroups</requireCommand> | |||
<requireCommand>AddGroupIfIdentifying</requireCommand> | |||
</include> | |||
<include cluster="Scenes" client="true" server="true" clientLocked="true" serverLocked="true"> | |||
<include cluster="Scenes" client="true" server="false" clientLocked="true" serverLocked="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this coming from? I see nothing in the Matter device library with an M client Scenes cluster. This applies to a bunch of other cases in this PR too.
Problem
Mandatoriness for Scenes does not match the device_library.
Change overview
Changing Server and Client mandatoriness for various device types.
When reviewing on GitHub, unfold above the change to see the device type that is being changed.
Testing
How was this tested? (at least one bullet point required)
Not yet tested. For discussion in Slack.