-
Notifications
You must be signed in to change notification settings - Fork 626
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 Philips effect cluster and enable it for Hue Go #2989
base: dev
Are you sure you want to change the base?
Conversation
1f7385c
to
bcfea78
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2989 +/- ##
==========================================
+ Coverage 87.85% 87.87% +0.01%
==========================================
Files 301 302 +1
Lines 9212 9226 +14
==========================================
+ Hits 8093 8107 +14
Misses 1119 1119 ☔ View full report in Codecov by Sentry. |
0x00: foundation.ZCLCommandDef( | ||
"set_effect", | ||
schema={ | ||
"param1": t.uint8_t, |
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.
do we know what these params represent?
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.
As far as I know they were just snooped from the Hue gateway. I've commented on the original issue where I found some additional behavior. The param1 seems to act like some kind of a command ID, and then the rest of params behave different based on that, but that's as much as I managed to observe.
Thanks for the PR! Adding custom Hue effects to ZHA is also on my list of things to do eventually. We'd also need to add the (two) custom effects to ZHA, but only for the Hue lights. This can be done by matching a Ideally, I want to avoid matching all Hue light models in quirks individually. We had this in the past for "startup options" (now part of ZCL) and there are way more models now. I think that this is fine for an initial implementation though. Like mentioned, giving the command params a more descriptive name would be good. |
cluster_id = 0xFC03 | ||
ep_attribute = "philips_effect" | ||
|
||
server_commands = { |
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.
Ideally, we should also use the new "zigpy command definition style", similar to this:
https://github.com/zigpy/zigpy/blob/1853ce109de5d1528333e9c78c325131f8aeed3a/zigpy/zcl/clusters/general.py#L570-L573
242: { | ||
PROFILE_ID: 41440, | ||
DEVICE_TYPE: 97, |
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.
for profile id and device type, use this:
242: {
PROFILE_ID: zgp.PROFILE_ID,
DEVICE_TYPE: zgp.DeviceType.PROXY_BASIC,
INPUT_CLUSTERS: [],
OUTPUT_CLUSTERS: [GreenPowerProxy.cluster_id],
},
(and also for the replacement
section)
Regarding the effect for ZHA, I have commented on the issue trying to piece the information together from source code. Eventually, I've arrived at something like (saved in
This does work - it adds the new effect on Hue Go, and selecting it turns the effect on, but it kind of breaks the HA state - for example it's possible to change brightness when the effect is on, but turning the effect on with this command causes the lamp to switch to set (but unknown) brightness, so the brightness slider is now not in sync with actual lamp brightness, As you can see in ZHA I match against the Effect cluster, so all lamps that support it will have this. However on zigpy side, you still need to make the quirk itself apply to specific lights that we know actually support the effects. Or do you plan to apply the quirk to all bulbs that report the |
@@ -0,0 +1,97 @@ | |||
"""Philips Hue Go 7602031P7 device.""" |
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.
We have a new way to define quirks now. I still need to write documentation for it but I think we can replace the majority of this file with:
from zhaquirks.philips import PHILIPS, PhilipsEffectCluster
from zigpy.quirks.v2 import add_to_registry_v2
(
add_to_registry_v2(
PHILIPS, "7602031P7"
)
.replaces(PhilipsEffectCluster, endpoint_id=11)
)
Here is a complete self contained example tested as a custom quirk on my dev instance with model: 7602031U7 note this will require a new release of zigpy w/ the bug fix here: zigpy/zigpy#1348 https://github.com/zigpy/zigpy/releases/tag/0.63.3 """Test Hue Go v2."""
from typing import Final
from zigpy.quirks import CustomCluster
from zigpy.quirks.v2 import add_to_registry_v2
import zigpy.types as t
from zigpy.zcl import foundation
PHILIPS = "Philips"
class PhilipsEffectCluster(CustomCluster):
"""Philips effect cluster."""
cluster_id = 0xFC03
ep_attribute = "philips_effect"
class ServerCommandDefs(foundation.BaseCommandDefs):
"""Server command definitions."""
set_effect: Final = foundation.ZCLCommandDef(
id=0x00,
schema={
"param1": t.uint8_t,
"param2": t.uint8_t,
"param3": t.uint8_t,
"param4": t.uint8_t,
},
direction=foundation.Direction.Client_to_Server,
is_manufacturer_specific=True,
)
(
add_to_registry_v2(PHILIPS, "7602031P7")
.also_applies_to(PHILIPS, "7602031U7")
.replaces(PhilipsEffectCluster, endpoint_id=11)
) |
Proposed change
Adds a new, manufacturer specific, Philips cluster that is used to set effects on certain Hue devices, such as Hue Go.
Additional information
Fixes #2517.
Checklist
pre-commit
checks pass / the code has been formatted using Black