Add Philips RDM002 Tap Dial switch support, refactor Hue remotes #2340
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2340 +/- ##
==========================================
+ Coverage 88.71% 89.22% +0.51%
==========================================
Files 306 307 +1
Lines 9820 9870 +50
==========================================
+ Hits 8712 8807 +95
+ Misses 1108 1063 -45 ☔ View full report in Codecov by Sentry. |
zhaquirks/philips/__init__.py
Outdated
| @@ -145,8 +145,7 @@ class PhilipsRemoteCluster(CustomCluster): | |||
| "param2": t.uint24_t, | |||
| "press_type": t.uint8_t, | |||
| "param4": t.uint8_t, | |||
| "param5": t.uint8_t, | |||
| "param6": t.uint8_t, | |||
| "duration": t.uint16_t, | |||
There was a problem hiding this comment.
This seems to be supported by many(/all?) of Philips' button devices. It's measured in 10th of seconds.
Confirmed that for ROM001, RWL022, RDM002.
There was a problem hiding this comment.
Also same for RDM001 in Push mode (0x01, 0x03)
There was a problem hiding this comment.
Is there a chance that you also have a RWL020 or RWL021 dimmer (one of the older ones) to test this with?
If not, I'll search for one of mine to test this with.
There was a problem hiding this comment.
RWL020 is the first gen Dimmer switch ?
That i have in the box for bad Zigbee devices.
@fholzer If you is near W1110 you can having my for testing its easy picking up then im at home if you like (U3 Enkplatz).
MW
There was a problem hiding this comment.
Yeah, that would be appreciated. From the RWLs I only have the RWL022. My email is ferdinand.holzer ( at ) gmail.com. Get in touch and we can arrange a time later today if possible.
There was a problem hiding this comment.
Got Matt's RWL020, though have to wait for new CR2450s to arrive.
fholzer
changed the title
zhaquirks/philips/rdm002.py
Outdated
| PRESS_TYPES = { | ||
| 0: COMMAND_PRESS, | ||
| 1: COMMAND_HOLD, | ||
| 2: SHORT_RELEASE, | ||
| 3: LONG_RELEASE, | ||
| } |
There was a problem hiding this comment.
The press types are exactly the same as in PhilipsRemoteCluster, right?
There, the constants aren't used though. Can you change that there (and remove the above code)?:
There was a problem hiding this comment.
The (SHORT|LONG)_RELEASE constants used in rdm002.py are set to "remote_button_short_release" and "remote_button_long_release", while the string literals "short_release" and "long_release" are used in __init__.py. "short_release" and "long_release" correspond to COMMAND_M_SHORT_RELEASE and COMMAND_M_LONG_RELEASE. Not entirely sure if something would break when changing the PhilipsRemoteCluster's PRESS_TYPE values. Which constants would you prefer?
Also, I'm not sure if changing these values would break anything for existing users. Would it not break anything if we also change PhilipsROM001's device_automation_triggers values?
Actually I was thinking about changing the implementation, introducing new PhilipsRDMRemoteCluster and PhilipsRWLRemoteCluster classes inheriting from PhilipsRemoteCluster, then change PhilipsROM001 PhilipsROM002 to inherit from PhilipsRDMRemoteCluster, and PhilipsRWL022 from PhilipsRWLRemoteCluster, but as I'm not sure this can be done without breaking anything for existing users when changing things like button name and press type values, I haven't started working on this.
Servus MattWestb :)
There was a problem hiding this comment.
The (SHORT|LONG)_RELEASE constants used in rdm002.py are set to "remote_button_short_release" and "remote_button_long_release", while the string literals "short_release" and "long_release" are used
Ah, I missed that. The HUE_REMOTE_DEVICE_TRIGGERS that are used for all(?) other Hue remotes at the moment use those suffixes in the triggers.
IMO, the constants used in rdm002.py with remote_button_xxx_release aren't optimal. Personally, I think the press types that are currently used make more sense:
Do you maybe want to remove the overriding PRESS_TYPES from your class, so we use all same press types for all devices?
And btw., your device triggers don't actually include short/long release ones for the buttons. Not sure if intended or not?
There was a problem hiding this comment.
Actually I was thinking about changing the implementation, introducing new PhilipsRDMRemoteCluster...
Hmm, the current implementation isn't optimal, as rdm001 also copies the complete custom cluster.
RWL022 also uses the "wrong button types".
If you want to have a go at changing the implementation, feel free to do so. If possible, we shouldn't really break device triggers / event data (used for blueprints).
The RWL022 event data is somewhat wrong though and should be changed maybe(?)
There was a problem hiding this comment.
Off the bat, sorry for the long post. I'd like to understand how I can help.
Personally, I think the press types that are currently used make more sense: [...]
Is it OK to use constants here, or do you prefer string literals? Could still update PRESS_TYPES in __init__.py to use constants with the same value as the string literals currently used.
And btw., your device triggers don't actually include short/long release ones for the buttons. Not sure if intended or not?
You're right. Also realized this and took a note below. Will have a look in the next couple of days.
RWL022 also uses the "wrong button types".
What do you mean?
HUE_REMOTE_DEVICE_TRIGGERS
- ...is used by all RWL type devices, which are the 4-button dimmer remote controls.
- ROM001, the single button remote, is using separately defined
device_automation_triggers, which is fine. - And RDM001, the wall switch, is also using separately defined
device_automation_triggers, but I think the trigger naming could be improved. I.e. instead ofTURN_ON/RIGHT, to either useLEFT/RIGHTorBUTTON_1/BUTTON_2.
PhilipsRemoteCluster
- ... is defined in
__init__.pyand used directly for ROM and RWL devices. - RDM002 currently inherits from it and overrides the
BUTTONSandPRESS_TYPESwithout providing triggers forLONG_PRESSor(SHORT|LONG)_RELEASEevents. (Need to check why i didn't add those - it's been some time since i wrote this). - And finally we have RDM001, which defined it's own
PhilipsRemoteCluster. Furthermore RDM001'sdevice_automation_triggerslists multi-press triggers (e.g. double-, triple-, etc. -press events ) but I wonder how that works, as it'sPhilipsRemoteClusteris missing thesend_press_eventfunction that generates similar events in__init__.py'sPhilipsRemoteCluster. For easy comparison of the two classes, I made a diff here.
Questions
I would love to fix & streamline all that, but as i'm new to the code base i have a couple of questions:
- My understanding is that changing keys in the
device_automation_triggersdict influences only how the triggers are presented in the UI, while changing the value (ie.COMMAND) would break things for existing users. Is that correct?- Even so, changing keys might break things that directly listen for zha_events and inspect the event arguments. Is that acceptable or not?
- Would you like to have a single
PhilipsRemoteClusterdefined in__init__.pyand used all applicable devices instead of copying the class to device-specific files and use overrides where needed. Then all devices would support features like synthetic multi-press events.
There was a problem hiding this comment.
I still need to test all this, but already pushed my changes so you can have a look. The device_automation_triggers is unchanged for all existing devices.
There was a problem hiding this comment.
To get back to your original statement about PRESS_TYPES not being constants. Right, so in this new implementation I derive the device_automation_triggers from the PRESS_TYPES, and those device_automation_triggers were using these constants before.
There was a problem hiding this comment.
Is it OK to use constants here, or do you prefer string literals? Could still update PRESS_TYPES in init.py to use constants with the same value as the string literals currently used.
No real preference for now. Constants are nicer if used in multiple places, but string literals should also be fine if it's used just once.
RWL022 also uses the "wrong button types".
What do you mean?
The device has a shared on/off button (that's just an "on" button in the triggers + brightness buttons (fine) + a "hue" button ("off" button in the triggers).
It shouldn't be changed in this PR though, as it would break blueprints (react on command in zha_event) and device triggers, as those should also be "renamed".
And RDM001, the wall switch, is also using separately defined device_automation_triggers, but I think the trigger naming could be improved. I.e. instead of TURN_ON/RIGHT, to either use LEFT/RIGHT or BUTTON_1/BUTTON_2.
Yes, it would break device triggers + blueprints though, as far as I can see.
All Hue remote related device triggers should probably be changed later in a separate PR (that's clearly marked as a breaking change). I'm not sure how much we're allowed to "break" now though, as there's some 6 month breaking change guideline in HA now. Not sure if/how that applies to libraries.
And finally we have RDM001, which defined it's own PhilipsRemoteCluster. Furthermore RDM001's device_automation_triggers lists multi-press triggers (e.g. double-, triple-, etc. -press events ) but I wonder how that works, as it's PhilipsRemoteCluster is missing the send_press_event function that generates similar events in init.py's PhilipsRemoteCluster. For easy comparison of the two classes, I made a diff here.
That's a good question on why those triggers are there if they can never be fired.
My understanding is that changing keys in the device_automation_triggers dict influences only how the triggers are presented in the UI, while changing the value (ie. COMMAND) would break things for existing users. Is that correct?
Even so, changing keys might break things that directly listen for zha_events and inspect the event arguments. Is that acceptable or not?
I'd have to check at this point, but I think you're right. For device triggers to not break in automations, the keys (type + subtype) have to stay the same. Technically, the command can change IIRC.
A lot of blueprints listen to the command though and we shouldn't really break/change those for now, so (sadly) we should keep both the commandand the device automation trigger the same. I'll need to properly look at your changes later, but do your current changes have both stay the same?
("Edit": Hmm, from a quick look, I think the commands are changed now, as both BUTTONS and PRESS_TYPES are changed.
Although I prefer your changes, commands (and device triggers) should stay the same for now (to be non-breaking).
(Also, for the "multi-press" stuff (ll 177-185 in __init__.py), should the appropriate press type come from PRESS_TYPES instead of string literals? Would be a bit cleaner maybe?)
Would you like to have a single PhilipsRemoteCluster defined in init.py and used all applicable devices instead of copying the class to device-specific files and use overrides where needed. Then all devices would support features like synthetic multi-press events.
(I always found the idea of having synthetic multi-press events in the Philips quirks a bit weird, but I guess that's what we're doing for now.)
Since the class would mostly/somewhat be the same code for the devices, having one class in a central places makes the most sense to me. (Seems like that's what you did for now)
There was a problem hiding this comment.
(Also, for the "multi-press" stuff (ll 177-185 in init.py), should the appropriate press type come from PRESS_TYPES instead of string literals? Would be a bit cleaner maybe?)
the PRESS_TYPES have keys that correspond to data we get in the event, so that dict can't be easily changes. The single/double/triple/... entries are not subsequent entried, so the best we can do there is use the same constants in the multipress part of the code that we use in the PRESS_TYPES like so: c2fdd4d
|
The only test is not liking is |
|
Ops by the way: |
fholzer
force-pushed
the
philips-rdm002
branch
from
3f1bb74 to
edc3560
Compare
|
lint issue fixed |
fholzer
force-pushed
the
philips-rdm002
branch
4 times, most recently
from
7b67a4e to
72afe06
Compare
|
Mostly just a note for me when I hopefully get back to this PR: Another note: (Also sorry for not being able to have a close look yet. Very limited on time at the moment. Your changes look good from a quick glance and are very much appreciated!) EDIT: Basically, existing device triggers have to stay for now. The (I'll put this into draft for now, so it's not accidentally merged yet.) |
TheJulianJES
changed the title
|
Dimming wheel acts as a switch with this quirk, would it be possible to use ROTATED LEFT/RIGHT instead? And maybe also include params like step_size? |
Will have a look this week. Also, the RWL020 remote I got from Matt doesn't support the long press events, so will have to remove those as well. Not sure about RWL021 though.
Will also look into changing my PR so that it keeps all existing PRESS_TYPES, commands and device_automation_triggers unchanged. |
|
I think the RWL020 is running on the latest firmware (=old then its one early HUE device) https://github.com/dresden-elektronik/deconz-rest-plugin/wiki/OTA-Image-Types---Firmware-versions#philips-hue-signify was the latest upgrade i was doing on it if i remember right. |
|
@cod3gen for me rotating the dial on my RDM002 using the code in this MR yields both, button press events, and "Steps with On Off" events, the latter includes direction & step size. I just pushed some changes. Can you check these? @TheJulianJES wrt the dial, can you give guidance? In my last commit just now I suppose these duplicate button press events rotating the dial yields, and added device automation triggers. I see COMMAND_STEP_ON_OFF is used by a couple of quirks(1, 2), though always in combination with "press" events. That would make the UI suggest the wheel is something that is "pressed": Is there some better alternative? I tried finding the strings available in the UI. Found this, but not sure if I am looking in the right place. Also, it doesn't list anything that's better suited. P.S.: Regarding how to deal with breaking changes... I could make a version of this PR that only adds changes needed for RDM002, and keep all other devices unchanged. If you also want to merge those changes to other devices, these could be dealt with separately, as per breaking changes guideline. |
|
With the requirement to not break anything, device automation trigger keys and values, params, parameter names, button names, press types, I'm not sure this can be done in one generic class without having a lot of code just for handling these "exceptions". I'm leaning towards discarding all changes except those for RDM002. |
|
Dim up and dim down is now present, step_size can be taken from params. Nice 👍 Does the double click, triple click etc. work on your tap dial? Seems to have no function here, seems to be responding only to pressed, continuesly pressed, released and released after long press on my dial. |
My RDM0002's only responds to pressed, continuously pressed, released and released after long press as well. |
|
Hi everyone, thanks @fholzer and all for working on this device! Just so you know, I will completely make an extensive use of it as soon as available ^^ I haven't yet contributed to the custom quirks, but if you need assistance let me know, I will try to dig in. |
|
Thank you for this. I am hoping that this will eventually get incorporated into the official ZHA quirks. In the meantime, I noticed that on my RDM002, if a button is long pressed, 'Press', 'Hold' and 'Long Release' events are fired, whereas for a single press, 'Short Release' followed by 'Press' events. Double and triple presses have 'Short Release' followed by 'Double Press' and 'Triple Press' respectively, and no 'Press'. So, 'Press' is fired for a single short press and a long press but not double or triple presses. I am wondering if the 'Press' event should not be fired for a long press, but a 'Long Press' event instead? I am trying to create an automation that responds to short and long presses but the short press automation gets activated too. I hope I am making sense! |
|
Came here when looking into the workings of the RDM001, since it is not firing multiple button presses at the moment. I would love to see this refactor get merged, since is resolves this issue. |
|
@TheJulianJES @MattWestb , would you be able to take a final look at this PR? |
|
Sorry for the long absence. Just to clarify, at this point this PR is not backwards compatible, and also there's an issue (at least for me) where when turning the RDM002's dial e.g. up, it start dimming my light up, but in between jumps back to lower brightness before going back up. So that definitely needs to be fixed before considering merging this. Also someone reported issues, need to check what exactly that was. |
|
I have updated blueprints available for use and testing. These versions include updated documentation, a couple of small bugfixes, and much improved organization including collapsible sections. Media controls. This blueprint is for the use of the Tap Dial to control a media endpoint. It has built-in support for volume and transport controls while allowing additional customization: Generic Controls. This is the second file in the gist. It doesn't assume a media controller but does allow the user to fully customize rotational actions as well as all button presses. Both support generating logbook events. https://gist.github.com/gtunes-dev/b2636c6f1c4932cb15fa9b5a10ae820b I hope someone finds these useful. I'll post them to the community when these changes near release. |
TheJulianJES
added
code quality
|
@gtunes-dev I tested your general blueprint. I had to rename It works really well with this as custom quirk and the blueprint after that minor fix. I hope this can be merged soon :) |
|
Thanks for looking at the blueprint, @tverlaan!
I thought I fixed that in the update I posted a couple of weeks ago. Is it possible you're using the older version? In addition to fixing the issue you found, the newer version has collapsible sections for each button - they start off collapsed and you need expand them to customize. Does the version you're using have these collapsed sections? The latest version is the second file (you need to scroll down) in this gist: https://gist.github.com/gtunes-dev/b2636c6f1c4932cb15fa9b5a10ae820b I also hope this gets reviewed and approved soon! |
|
@TheJulianJES - can you possibly review this? Thanks. |
|
Hi, @fholzer. I believe you and I are still running similar environments for testing - docker with your changes mapped into the container. This has been working fine for me but is now broken with the latest Home Assistant container from linuxserver.io. There have been multiple 2024.10.2 images released. As of this post, the latest has the tag "2024.10.2-ls35". With this one, your changes prevent Home Assistant from starting. If I remove the mapping of your changes, it works fine. If I revert to the previous 10.2 release, or anything earlier than that, your changes also work fine. So I'm back to the image with the tag "2024.10.2-ls34". In my case, this problem exists with the "latest" tag, these are just the version specific tags. For me, at least, something broke not in 10.2 but in something that is different in the .35 release. If you can't repro this, I can get you logs. Thanks. |
|
Thanks for pointing this out. Will try to look into it this week. |
|
One of those moments where you wish github had direct messages. Some notes on the issue I'm experience. @TheJulianJES - this issue is very specific to my environment and I hope you don't defer your code review until it's root caused. @fholzer - I think the issue is related to specific changes the linuxserver team made to python package management. They went from pip to uv a few months ago but in this latest ls35 release, they made additional changes. My theory at the moment is that there's a permissions issue with the subfolder of quirks that I've mapped in and the consequence is that it can't write to it (might be trying to download a directory or to update pycache). So this is specifically about the use of Docker, the use of this specific Docker image, and the fact that I'm trying to map just your changes, in a Philips directory) directly into the zhaquirks directory with this mapping: I believe that what's happening is that when uv tries to do this update, it's doing it as a user within the container called abc:abc, not using the user and group ids that I configured my container with. I don't think that user/group can write to my mapped in folder and I'm not sure how to make it possible. Here's a compare of the ls34 to ls35 that will help you see the changes they made: linuxserver/docker-homeassistant@2024.10.2-ls34...2024.10.2-ls35 In any case, If I'm remotely right about this, this is very specific to my (our?) environments and what we're doing to try make zhaquirks work for testing purposes. This problem won't exist when the changes go public. |
|
Thanks, @TheJulianJES! And another Thank You! to @fholzer for this work!! |
|
Apparently, there's a new revision of the Hue wall switch module. Just another model string to add ( EDIT: Merged this PR first. Afterwards merged: (Feel free to PR changes if I missed something in that PR) |
I'm just a cheerleader and somewhat helpful tester but I've been hoping to see the RDM002 supported for a very, very long time and would prefer to see this PR go forward without converging with the wall switch. I understand if that's not what you prefer. |
There was a problem hiding this comment.
Unrelated to this PR, but I just noticed we're overriding bind() in the PhilipsBasicCluster to set some attributes.
I'm pretty sure bind() isn't actually called for the Basic cluster by default, as ZHA explicitly doesn't bind it (some devices can only bind a few clusters).
Do we know what the "philips" attribute (0x0031) does?
There was a problem hiding this comment.
The PhilipsBasicCluster for RDM001 also has another "mode" attribute (0x0034) that should be set to 0x2. I don't think that'll happen either.
Also not sure what both attributes do tbh.
It's not something that needs to be addressed in this PR, but we should still take a look at it.
|
To answer the second question myself, the "mode" options seems to configure how the wall switch modules act. |
|
To get back to the first question about the "philips" attribute, I only found this: https://github.com/zigpy/zha-device-handlers/pull/388/files#r450561439 ... which still isn't quite clear to me 😄 |
TheJulianJES
added
ready
Add suport for Philps Hue Tap Dial Switch (RDM002).
This PR also refactors all Hue remotes to "generate" the device triggers and removes a lot of duplicated code.
The device triggers changed are backwards-compatible (verified in tests).