-
Notifications
You must be signed in to change notification settings - Fork 55
Add LevelControl out of band API #325
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
Add LevelControl out of band API #325
Conversation
Summary of ChangesHello @hicklin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the LevelControl cluster by introducing an out-of-band API, allowing external device interactions to seamlessly update the Matter state and trigger Matter commands. It refactors internal notification mechanisms to optimize Matter attribute reporting and improves the flexibility of level transition handling by splitting core functions into blocking and non-blocking variants. Additionally, a new constructor simplifies the initialization of LevelControl attributes, making the API more robust and easier to use. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 valuable out-of-band API for the LevelControl cluster, allowing for better integration with non-Matter device state changes. The refactoring to separate state updates from notifications and the split of move_to_level
into blocking and non-blocking versions are excellent design choices that improve the flexibility and robustness of the handler. My review includes a few suggestions to further enhance maintainability and safety, such as refactoring duplicated code and avoiding a potential panic.
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.
I have just a minor comment on the new AttributeDetails::with_optional_defaults
.
The rest looks fine!
c897406
to
6e32c87
Compare
This PR adds an out-of-band API to the LevelControl cluster. The API allows SDK consumers to notify the
LevelControlHandler
if the device's level has change out-of-band, allowing the handler to update the Matter state. Additionally, this API provides calling matter commands, allowing consistent behaviour between Matter and out-of-band interactions.Further changes made in this PR
set_level
andwrite_remaining_time_quietly
no longer notify Matter but return a boolean indicating if the caller should notify Matter. This enables only one Matter notification when these methods are used in the same execution.move_to_level_transition
logic to handletransition_time = 0
.move_to_level
is split into a blocking and non-blocking version. This removes the need for the non-blocking version to beasync
, enabling calls from non-async methods, namelyhandle_out_of_band_message
.AttributeDefaults::with_optional_defaults
.