Skip to content

Conversation

BetaRavener
Copy link

Description

Adds additional effects to supported Philips Hue lights. To control the effects, commands are sent to a exposed cluster.
Implements HA / UI portion of zigpy/zha-device-handlers#2517, which contains detailed discussion.

The effect is turned on by command 0x22 that accepts brightness and the effect ID.
To turn off, command 0x20 is used that will preserve most recent color and brightness while stopping further effect execution.

Testing

Tested with Philips Hue Go. See video below:
https://github.com/user-attachments/assets/9d6236f6-4d8f-40ee-8525-82e674d4ca85

Prerequisite

PR implementing quirk for the Philips cluster: zigpy/zha-device-handlers#2517

@BetaRavener
Copy link
Author

Test asserts are failing, but seem to be correct - tests will need to be changed. All lights that provide cluster 0xFC03 are failing, while those missing it (e.g. signify-netherlands-b-v-rwl022.json) are not.

@TheJulianJES
Copy link
Contributor

You can delete the JSON test files for now then. We'll eventually regenerate and upload them again.
In the future, we might have a tool to re-generate these tests automatically (without needing to grab them from a running instance).

@BetaRavener BetaRavener force-pushed the feature/philips-effects branch from e5b8630 to 00a3125 Compare February 12, 2025 14:36
@BetaRavener
Copy link
Author

@TheJulianJES I pushed changes to match the last renaming from quirk PR.

I haven't resolved the failing tests yet as I lost track which JSONs were problematic, but I realized I also don't know why they were failing in the first place. Given that I had old cluster name, even if the quirk PR got already picked up during tests, my class shouldn't have matched any device yet.

I've added CLUSTER_HANDLER_PHILIPS_HUE_LIGHT to aux_cluster_handlers to give HueEffectLight just small extra weight over HueLight class thinking that strict_match would require the new cluster to be present. But looking at strict_matched, is it true the aux clusters are not considered for matching? Should I then move the CLUSTER_HANDLER_PHILIPS_HUE_LIGHT to cluster_handler_names instead?

@jtbandes
Copy link

jtbandes commented Apr 7, 2025

Hi @TheJulianJES, just wondering if you were able to provide any feedback on the above?

@jtbandes
Copy link

@puddly hi there, just a friendly ping - there are several folks in the HA community who would love to see this merged (see zigpy/zha-device-handlers#2517) Could you help us figure out what the next steps are?

@TheJulianJES
Copy link
Contributor

TheJulianJES commented Jun 23, 2025

The "light turn on" call in the class does a lot. I think the current approach for the Hue class might be fine, but I hope to have a look soon to see if it can/should be done differently. Right now, we have to make sure the effect on/off behavior exactly matches the one in the base light class. That might not be ideal.

One other thing I can think of right now: Can we somehow poll the current Hue effect in async_update?
Otherwise, I'll try to play around with this soon-ish.

@jtbandes
Copy link

@BetaRavener let us know if you are able to continue driving this forward, otherwise I am sure someone else can try to pick it up :)

@Orange-GT3
Copy link

Just giving this a nudge, @BetaRavener or at @TheJulianJES.

@BetaRavener
Copy link
Author

I could finish this PR but unfortunately I still don't have an answer regarding the clusters, if I should keep it in aux list or move to the "main" list as I mentioned above.

The rest from Julian sounded like possible but non-blocking improvements.

The single question about polling current effect - I honestly don't know. I only found people successfully snooping and reverse engineering the set effect command, I didn't come across one to query current effect. It would also have to be part of the manufacturer specific cluster if it exists.

@Orange-GT3
Copy link

I could finish this PR but unfortunately I still don't have an answer regarding the clusters, if I should keep it in aux list or move to the "main" list as I mentioned above.

The rest from Julian sounded like possible but non-blocking improvements.

The single question about polling current effect - I honestly don't know. I only found people successfully snooping and reverse engineering the set effect command, I didn't come across one to query current effect. It would also have to be part of the manufacturer specific cluster if it exists.

Possibly a redundant question, but have you seen this: Zigbee format for Philips Hue manufacturer-specific light updates?

@BetaRavener
Copy link
Author

BetaRavener commented Jul 24, 2025

Yes, I think it's also mentioned on the original issue. But this doesn't allow query as I said, let's say somebody uses different app or buttons on the light to change the effect, the HA will get out of sync because we can't get the new state.

Also, the more advanced configuration features are not helpful as there's no interface to build these up and include in effect list. So if you want more reddish, slower, candle effect, you'll have to still send command to cluster yourself. This is aimed at non-technical users who (like me earlier) just click around, and then having an effect readibly available in dropdown might be already good enough for them.

@Orange-GT3
Copy link

OK, I just thought I'd check.

I understand what you are trying to achieve with your interface and fully support it. Other than testing, though, I can't contribute to coding as it's all totally beyond me.

I'd love to see a fully fledged Philips Hue Integration but I realise that would be an absolute tonne of work but maybe if parts get done here and there, it would be possible for the HA team to pull them together into a single integration.

@Orange-GT3
Copy link

But this doesn't allow query as I said, let's say somebody uses different app or buttons on the light to change the effect, the HA will get out of sync because we can't get the new state.

Correct me if I'm wrong, but it's possible to read cluster values so couldn't the state be checked by reading the cluster and then decoding the return value according the Frame Format detailed at the above link?

@BetaRavener
Copy link
Author

BetaRavener commented Jul 24, 2025

I'm bit out of the loop but yes you're right you can read values from cluster. What we are missing (I think) is from where, i.e. the address of the attribute in cluster that would provide us this value is unknown.

I don't think there's a return value other than command success / failure.

@jtbandes
Copy link

@TheJulianJES could you help answer this question from above?

I've added CLUSTER_HANDLER_PHILIPS_HUE_LIGHT to aux_cluster_handlers to give HueEffectLight just small extra weight over HueLight class thinking that strict_match would require the new cluster to be present. But looking at strict_matched, is it true the aux clusters are not considered for matching? Should I then move the CLUSTER_HANDLER_PHILIPS_HUE_LIGHT to cluster_handler_names instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants