-
Notifications
You must be signed in to change notification settings - Fork 2.2k
add and fix dynamic-lighting-cluster.xml #39959
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
base: master
Are you sure you want to change the base?
Conversation
|
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.
Code Review
This pull request introduces a new XML file, dynamic-lighting-cluster.xml
, to define the Dynamic Lighting cluster. The new file specifies the necessary enums, structs, attributes, and commands for this functionality.
I have reviewed the changes in accordance with the provided style guide. The guide specifically instructs not to comment on the content of XML files for cluster definitions, with an exception for typos. After a careful review, I found no typos or other actionable issues based on the provided guidelines. The new XML file is well-structured and consistent with other cluster definitions in the repository.
PR #39959: Size comparison from bde480e to 7c00f53 Full report (72 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see #39906 (review)
And if closing that PR and opening this one was just an attempt to avoid the "changes requested", that is very much not OK.
Sorry for the mix-up! As a newbie, I’m still figuring out GitHub’s PR workflow (no intent to skip "changes requested"). I’ve made the requested changes, but I clearly messed up the process. Could you share how to properly update a PR in the future? Your guidance would be really helpful. Thanks!
方楠 | 开发主管
TEL : 15913142338
智岩科技廉洁违规举报渠道 :
0755-26924207 或 ***@***.***
------------------------------------------------------------------
发件人:Boris Zbarsky ***@***.***>
发送时间:2025年7月11日(周五) 23:59
***@***.***>
***@***.***>; ***@***.***>
主 题:Re: [project-chip/connectedhomeip] add and fix dynamic-lighting-cluster.xml (PR #39959)
@bzbarsky-apple requested changes on this pull request.
On src/app/zap-templates/zcl/data-model/chip/dynamic-lighting-cluster.xml <#39959 (comment) >:
Please see #39906 (review) <#39906 (review) >
And if closing that PR and opening this one was just an attempt to avoid the "changes requested", that is very much not OK.
—
Reply to this email directly, view it on GitHub <#39959 (review) >, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJRLXOXKV2PA6YLZQBBL5I33H7NN5AVCNFSM6AAAAACBJKTIZSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAMJQHA4TMNZYHA >.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
PR #39959: Size comparison from ad2e3f5 to 24fc17b Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
You can just push (or force push as needed) to the PR branch to update an existing PR. For this PR, again: where is this XML definition coming from? This cluster does not look to be in the Matter specification. Ideally, the PR description would indicate which exact git SHA of the Matter specification was used as the basis for the XML. |
Summary
Related issues
Testing
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines