Skip to content
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 Linxura smart controller button quirk #3392

Merged
merged 87 commits into from
Nov 26, 2024
Merged

Conversation

simon3panda
Copy link
Contributor

@simon3panda simon3panda commented Sep 30, 2024

Proposed change

This PR adds a new quirk for the Linxura smart controller's Zigbee mode, enabling Linxura smart controller proper communication and control within the ZHA framework. Linxura smart controller's Zigbee mode support four Zigbee buttons. Each button support click, double click, long press actions.

Additional information

Test file is in tests/test_linxura.py

Checklist

  • The changes are tested and work correctly
  • pre-commit checks pass / the code has been formatted using Black
  • Tests have been added to verify that the new code works

simon3panda and others added 3 commits September 23, 2024 15:40
Linxura smart controller's Zigbee mode support four buttons. Each button support click, double click, long press actions.
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.67%. Comparing base (d9c9df9) to head (9294274).
Report is 69 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3392      +/-   ##
==========================================
+ Coverage   88.49%   89.67%   +1.17%     
==========================================
  Files         305      316      +11     
  Lines        9621    10281     +660     
==========================================
+ Hits         8514     9219     +705     
+ Misses       1107     1062      -45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheJulianJES TheJulianJES removed the needs review This PR should be reviewed soon, as it generally looks good. label Nov 25, 2024
Remove the constant STATUS_REPORT and replace it with getting the id of zone_status.
@simon3panda
Copy link
Contributor Author

simon3panda commented Nov 25, 2024

All suggested modifications has been made. Please review if you spare time. Many thanks!

Comment on lines 199 to 213
@pytest.mark.parametrize("quirk", (zhaquirks.linxura.button.LinxuraButton,))
async def test_edge_case_request(zigpy_device_from_quirk, quirk):
"""Test edge case."""

device = zigpy_device_from_quirk(quirk)

# endpoint 1
cluster = device.endpoints[1].ias_zone
ias_zone_listener = ClusterListener(cluster)
ias_zone_status_attr_id = IasZone.AttributeDefs.zone_status.id

cluster.update_attribute(ias_zone_status_attr_id, 4)
assert (
len(ias_zone_listener.attribute_updates) == 1
) # No update should occur for state >= 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this test? The comment states "No update should occur for state >= 4", but the IAS zone status attribute update always happens, regardless of "state".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I've reworked the tests and removed this for now.

event_args = {
BUTTON: button,
PRESS_TYPE: press_type,
COMMAND_ID: 10,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the COMMAND_ID fully, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I've also removed this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally, it looks good! Thank you!

@TheJulianJES TheJulianJES added smash This PR is close to be merged soon and removed waiting for changes Waiting for changes to the PR labels Nov 26, 2024
@TheJulianJES
Copy link
Collaborator

TheJulianJES commented Nov 26, 2024

Thanks for making the changes!
I've made some improvements to the quirk + tests and made sure the code doesn't break when values 0, 6, 12, or 18 are passed in. Those values shouldn't generate events, right?
The COMMAND_ID is also removed from the event and the device triggers. I don't think we actually need it here.
I've also moved the custom IAS cluster to the button quirk file. This is just to be consistent with other quirks. We only need it in button.py, so let's keep it there.

Please have a look at the new code, let me know what you think, and try to see if everything works still.

Copy link
Collaborator

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution and for testing again!
I think everything looks good to go now.

@TheJulianJES TheJulianJES merged commit 213ce10 into zigpy:dev Nov 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new quirk Adds support for a new device smash This PR is close to be merged soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants