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.

Sign up for GitHub

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

Jump to bottom

Fix Sinope device triggers #3504

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Fix Sinope device triggers #3504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
254f9fa
fix device triggers definition
ckm2k1 Nov 10, 2024
3c25d48
tests
ckm2k1 Nov 10, 2024
10170b7
remove debug
ckm2k1 Nov 10, 2024
3bae0da
comment
ckm2k1 Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions tests/test_sinope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,18 @@ async def test_sinope_light_switch_reporting(zigpy_device_from_quirk, quirk):

assert len(request_mock.mock_calls) == 1
assert len(bind_mock.mock_calls) == 1


@pytest.mark.parametrize("quirk", (SinopeTechnologieslight,))
async def test_sinope_light_device_triggers_def(zigpy_device_from_quirk, quirk):
"""Test device automation triggers.

Make sure that values are actual ints and not instances of an enum class.
"""

device: Device = zigpy_device_from_quirk(quirk)

for config in device.device_automation_triggers.values():
val = config.get("args", {}).get("value")
if val is not None:
assert type(val) is int, type(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with this test, but like I said in the other comment, I don't think it's forced anywhere that the value field is an int. I personally think zha should fix rules like this tho (so we don't have to do it in the sinope test, but all fields would be tested at once)

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 agree in principal, and would happily participate in a discussion with the ZHA maintainers, but since there are no guarantees, I'd prefer to patch things in a simple way and have our automations work like before.

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 don't think it's forced anywhere that the value field is an int

This is true, but it's indirectly forced by the fact that Voluptuous fails to handle user types. I did find the root of the problem, described in my comment here: #3504 (comment)

16 changes: 8 additions & 8 deletions zhaquirks/sinope/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_ON,
VALUE: ButtonAction.Pressed_on,
VALUE: int(ButtonAction.Pressed_on),
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, sorry about it.

I personally prefer VALUE: ButtonAction.Pressed_on.value to match the syntax when we emit.

Another fix would be to change to DESCRIPTION: ButtonAction.Pressed_on.name, but I don't have a strong preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do I :)

Problem is that when I tested using .value, it turned out that it's of type <class 'zigpy.types.basic.uint8_t'> and not an int! This still made Voluptuous raise an invalid type exception, so I opted for the next simplest solution, coercing to int.

},
},
(SHORT_PRESS, TURN_OFF): {
Expand All @@ -63,7 +63,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_OFF,
VALUE: ButtonAction.Pressed_off,
VALUE: int(ButtonAction.Pressed_off),
},
},
(SHORT_RELEASE, TURN_ON): {
Expand All @@ -74,7 +74,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_ON,
VALUE: ButtonAction.Released_on,
VALUE: int(ButtonAction.Released_on),
},
},
(SHORT_RELEASE, TURN_OFF): {
Expand All @@ -85,7 +85,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_OFF,
VALUE: ButtonAction.Released_off,
VALUE: int(ButtonAction.Released_off),
},
},
(DOUBLE_PRESS, TURN_ON): {
Expand All @@ -96,7 +96,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_ON,
VALUE: ButtonAction.Double_on,
VALUE: int(ButtonAction.Double_on),
},
},
(DOUBLE_PRESS, TURN_OFF): {
Expand All @@ -107,7 +107,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_OFF,
VALUE: ButtonAction.Double_off,
VALUE: int(ButtonAction.Double_off),
},
},
(LONG_PRESS, TURN_ON): {
Expand All @@ -118,7 +118,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_ON,
VALUE: ButtonAction.Long_on,
VALUE: int(ButtonAction.Long_on),
},
},
(LONG_PRESS, TURN_OFF): {
Expand All @@ -129,7 +129,7 @@ class ButtonAction(t.enum8):
ATTRIBUTE_ID: 84,
ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
BUTTON: TURN_OFF,
VALUE: ButtonAction.Long_off,
VALUE: int(ButtonAction.Long_off),
},
},
}
Expand Down