Homeworks support #52
Conversation
There was a problem hiding this comment.
Sorry for the long lack of response, finally went through it all.
In the future I recommend you clean up your patchstack so that the changes collect related code. Seems like you just kept typing "git commit -m 'foo'" periodically, without organizing your improvements into logical changes :)
|
Guys with more advanced Homeworks systems desperately need this branch merged. The default one can't handle phantoms, any VCRX stuff, international keypads, etc... the CSD001 detection was a great idea. Please, we need this... or can anyone tell me how to force Hassio to use this instead of the default???? |
|
@jthop I can share my version of the |
|
@rzulian that would be great... I'm just not familiar with HA yet, and not clear how to force it to use this pylutron... I could obviously just replace the docker's pylutron, but not sure how to make it persistent across updates, etc... I've had my own Lutron program running for a while, to implement some custom keypad buttons in my house... but now I'm thinking I can just do everything with HA... Also, for me the binary sensors for the occupancy groups don't seem to be working, do they for you? In my own software I see the correct occupancy events coming from the processor, but in HA my occupancy states never seem to be accurate.... not sure if that's another Homeworks vs ra thing or what? |
I would be happy to test both the lutron component in Hassio and this branch of pylutron on my Homeworks QS system if you would like to share the lutron component. |
|
Oh yes, I would love to test as well.... |
|
@txwindsurfer would love to see it... scratching my head why these major upgrades for homeworks take so long to be approved??? |
|
The main problem is that it's quite a major change to the code, so I really
have the time to review and then test the changes against my radio ra 2
system.
…On Thu, Jun 11, 2020, 8:28 AM Jamie Hopper ***@***.***> wrote:
@txwindsurfer <https://github.com/txwindsurfer> would love to see it...
scratching my head why these major upgrades for homeworks take so long to
be approved???
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#52 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALQIMMR4P5EOMBUGZSWQ23RWDZ2XANCNFSM4MN27M4Q>
.
|
|
@rzulian fwiw would be nice to have access to homework variables as well... They use command SYSVAR and have integration IDs just like everything else. they aren't reported by default, you have to do a "#MONITORING,10,1" to see them... just an idea, you guys are doing great. |
pylutron/__init__.py
Outdated
| led_logic = button_xml.get('LedLogic') | ||
| name = button_xml.get('Name') | ||
| led_logic = 0 if button_xml.get('LedLogic') is None else int(button_xml.get('LedLogic')) | ||
| name = f"keypad {keypad.id}: Btn {component_number}" |
There was a problem hiding this comment.
I still strongly dislike sticking all the other cruft into the name. It should be up to the client to decide how to combine the various names. cdheiser's recent change should allow everything to have a true unique identifier so none of this should be necessary for uniqueness.
Please remove this here and everywhere else, keeping the name as is (and removing engraving).
pylutron/__init__.py
Outdated
| @@ -446,7 +446,8 @@ def __init__(self, host, user, password): | |||
| @property | |||
| def areas(self): | |||
| """Return the areas that were discovered for this Lutron controller.""" | |||
| return self._areas | |||
| # return self._areas | |||
| return tuple(area for area in self._areas) | |||
There was a problem hiding this comment.
Heh. Only reason I can see is if someone was taking the returned list and modifying it and you wanted to isolate us from that? Is that a problem somehow in HASS?
Since we don't do this everywhere else, I suggest you removed this and keep as is. If that's a worry somehow, it should be a separate PR to generate a copy for all these APIs to keep us safe, and we can discuss the merits then.
pylutron/__init__.py
Outdated
| HOLD_RELEASED = 5 | ||
|
|
||
|
|
||
| def __init__(self, lutron, keypad, name, num, button_type, direction): | ||
| def __init__(self, lutron, keypad, name, engraving, num, button_type, direction, led_logic): |
There was a problem hiding this comment.
Ah, LED state, ok. Yeah I only have radio ra2 and their feature-set is quite minimal (I get on/off 😞 ), sadly. I'm jelly of Homeworks. I'd do anything for that double-tap :)
I really don't want to expose things like led_logic as bare ints without proper enum constants is that it ends up spreading weird droppings like this with no concept of what they mean, and suddenly HASS has a random 5 somewhere. You need to add a better API for the client to figure these things out. At least provide a named enum for 0-3 that we can keep stable, and also add a CUSTOM or INTEGRATION for your 5.
Thought, actually this is all a property of an LED, and not of the button, isn't it? Why can't you instead delegate the LED control to the Led class?
Also, Does LED state change gets its own event? I don't remember.
pylutron/__init__.py
Outdated
| button_type=button_type, | ||
| direction=direction) | ||
| direction=direction, | ||
| led_logic=led_logic) |
There was a problem hiding this comment.
I think the problem is that you have like 20+ changes with stuff scatted all over them with no logical breakdown between the individual changes. So I'm sure this gets used, but in some other git commit. Github almost incentivizes bad habits :( I'm used to, and prefer, seeing one git commit per logical change, and a stack of commits that logicall follow each other adding incremental functionality.
|
Hi, I wanted to note that as of Homeworks v16, and with the new Homeworks QSX processor (released earlier this year) it appears that telnet support is being discontinued in favor of Lutron's LEAP API. I can't find published specs to that yet. Basically, it looks like Lutron just removed telnet support entirely in Homeworks v16. No depreciation warning/time period. |
|
Ok yes and FYI there are no published specs as of now from what I
understand. Lutron worked with all the majors (Crestron, Control4, Savant,
etc) and they all have drivers developed already... they do plan to release
the API in the future but not sure when. Anyway this is a major change,
but you gotta admit that an API using telnet is a bit outdated....
…On Mon, Aug 24, 2020 at 7:59 AM Robby Dermody ***@***.***> wrote:
Hi, I wanted to note that as of Homeworks v16, and with the new Homeworks
QSX processor (released earlier this year) it appears that telnet support
is being discontinued in favor of Lutron's LEAP API. I can't find published
specs to that yet.
More info:
https://forums.lutron.com/showthread.php/15404-Is-QSX-replacing-QS-processors-or-will-they-serve-different-purposes
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#52 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWCG2TYP6VDTLTGRQLBIJLSCJP3VANCNFSM4MN27M4Q>
.
--
Jamie Hopper
Megahertz Technology, Inc.
214-365-0933
www.mhztech.com
|
|
That would be amazing. The telnet API is terrible :)
…On Mon, Aug 24, 2020, 7:09 AM Jamie Hopper ***@***.***> wrote:
Ok yes and FYI there are no published specs as of now from what I
understand. Lutron worked with all the majors (Crestron, Control4, Savant,
etc) and they all have drivers developed already... they do plan to release
the API in the future but not sure when. Anyway this is a major change,
but you gotta admit that an API using telnet is a bit outdated....
On Mon, Aug 24, 2020 at 7:59 AM Robby Dermody ***@***.***>
wrote:
> Hi, I wanted to note that as of Homeworks v16, and with the new Homeworks
> QSX processor (released earlier this year) it appears that telnet support
> is being discontinued in favor of Lutron's LEAP API. I can't find
published
> specs to that yet.
>
> More info:
>
https://forums.lutron.com/showthread.php/15404-Is-QSX-replacing-QS-processors-or-will-they-serve-different-purposes
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#52 (comment)>,
or
> unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAWCG2TYP6VDTLTGRQLBIJLSCJP3VANCNFSM4MN27M4Q
>
> .
>
--
Jamie Hopper
Megahertz Technology, Inc.
214-365-0933
www.mhztech.com
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#52 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALQIMNMTWSWAVLFIHUYZKLSCJYDHANCNFSM4MN27M4Q>
.
|
|
@thecynic : could you please have a final review and approve this PR? @txwindsurfer @jthop : If you want to try the current branch I've a repo with the Lutron custom component here https://github.com/rzulian/lutron_custom_component |
|
Great work @rzulian! I added your files into custom_components/lutron and upgraded to the latest version of HA (0.116.0.dev20200923) on my test system and now I have the buttons on my keypads mapped as scenes (albeit no unique id so can't manage in UI). I also now have occupancy binary_sensors mapped for each room but since I don't have occupancy sensors I am guessing they are default entries in the Lutron schema. From a naming convention, I wasn't using the 1st floor or 2nd floor designation in the names of my objects so if I adopt your integration I will need to change lots of names. Adding unique ids to the objects would greatly help in managing the names from the UI. Thanks for sharing and your continued work!! |
|
|
I appreciate again your work and realize that designing for may different systems is difficult if not impossible. So with that premise, I kindly offer the following comments: 1a) First ignore my previous comment on unique ids for the scenes as it really doesn't make sense to edit scenes in the UI as an edited scene cannot be passed back to Lutron. Editing scenes for Lutron keypads is best done in Lutron Designer. 1b) Second, the addition of the keypad_name didn't add information for my installation since the DeviceGroup Name already identified the keypad.
Just FYI, the XML file also has an OccupancyGroups section even though I don't have any sensors so that should be ignored. Thanks again for your great work. It is really appreciated |
|
@txwindsurfer |
|
@rzulian |
|
@txwindsurfer |
Merged changes from rzulian pr thecynic#52 with pylutron 0.2.12 https://github.com/thecynic/pylutron/releases/tag/0.2.12
Initial merge for Homeworks QS support from thecynic#52
|
@rzulian @thecynic Thank you so much for all the attention to the Lutron Integration. Could you please add 'HWI_SLIM' to the list of keypads around lines 302. With this addition, my keypad buttons show up as intended on the event bus and as event entities. From my DbXmlInfo for one of the keypads: |
|
@txwindsurfer
|
|
Can you sync and rebase this CL to resolve merge conflicts and we can do another pass? |
|
Looks like the rebase didn't work quite right. You have files like publish.yml that shouldn't be present but this PR believes it's adding them (when they already exist). |
|
I don't know what happened for the publish.yml, but the other files are correct. |
|
@cdheiser what about this new rebase? |
pylutron/__init__.py
Outdated
| @@ -737,6 +742,18 @@ def level(self, new_level): | |||
| # self._lutron.send(Lutron.OP_EXECUTE, Output._CMD_TYPE, | |||
| # Output._ACTION_ZONE_LEVEL, new_level, fade_time, delay) | |||
|
|
|||
| def start_raising(self): | |||
There was a problem hiding this comment.
Are these used on anything other than Shades? The Shade class already implements this.
| # keypad standard name is CSD 001, we use the integration ID nme instead | ||
| name = keypad_xml.get('Name') | ||
| if (keypad_xml.get('Name') == "CSD 001"): | ||
| name = f"keypad {keypad_xml.get('IntegrationID')}" |
There was a problem hiding this comment.
This is something to handle in Home Assistant. As long as these have UUIDs, then home assistant can handle 2 keypads with the same name.
pylutron/__init__.py
Outdated
| keypad = self._parse_keypad(device_xml, device_group) | ||
| area.add_keypad(keypad) | ||
| elif device_xml.get('DeviceType') == 'MOTION_SENSOR': | ||
| motion_sensor = self._parse_motion_sensor(device_xml) | ||
| area.add_sensor(motion_sensor) | ||
| #elif device_xml.get('DeviceType') == 'VISOR_CONTROL_RECEIVER': | ||
| else: | ||
| #phantom keypad doesn't have a DeviceType |
There was a problem hiding this comment.
I think about here is where the logic for a phantom keypad should live. Check if it's CSD 001, and if so, call self._parse_phantom_keypad(...). Put in the custom logic for the device type, etc... there.
There was a problem hiding this comment.
"CSD 001" is used for example for INTERNATIONAL_SEETOUCH_KEYPAD. Note that I've no UUID in any component. I don't know if this due to the Homeworks QS or it's version.
Phantom keypads are not the CDS 001 keypads, but the one that you can create in the integration
There was a problem hiding this comment.
@cdheiser can we resolve this?
There was a problem hiding this comment.
ok... I'm just trying to follow along and make suggestions on how to get some custom handling out of the keypad class. I'd like it to not care about odd variants. Odd variants should either be prepped outside the class initialization, or as their own distinct type.
There was a problem hiding this comment.
I've moved the device_type before the the parse_keypad
| # keypad standard name is CSD 001, we use the integration ID nme instead | ||
| name = keypad_xml.get('Name') | ||
| if (keypad_xml.get('Name') == "CSD 001"): | ||
| name = f"keypad {keypad_xml.get('IntegrationID')}" |
There was a problem hiding this comment.
Can you post some sample XML in the PR? A lot has gone into using UUIDs in the home assistant integration and we should make sure it's not going to cause problems.
In theory, as long as everything has uniquie integration IDs, the fallback mechanism of using integration IDs should still result in unique devices and entities that can be named/renamed as you'd like. But if someone configures their setup to have 5 devices all named keypad in the same room, who are we to tell them they should be unique names?
pylutron/__init__.py
Outdated
| keypad = self._parse_keypad(device_xml, device_group) | ||
| area.add_keypad(keypad) | ||
| elif device_xml.get('DeviceType') == 'MOTION_SENSOR': | ||
| motion_sensor = self._parse_motion_sensor(device_xml) | ||
| area.add_sensor(motion_sensor) | ||
| #elif device_xml.get('DeviceType') == 'VISOR_CONTROL_RECEIVER': | ||
| else: | ||
| #phantom keypad doesn't have a DeviceType |
There was a problem hiding this comment.
ok... I'm just trying to follow along and make suggestions on how to get some custom handling out of the keypad class. I'd like it to not care about odd variants. Odd variants should either be prepped outside the class initialization, or as their own distinct type.
| button_type = button_xml.get('ButtonType') | ||
| direction = button_xml.get('Direction') | ||
| led_logic = 0 if button_xml.get('LedLogic') is None else int(button_xml.get('LedLogic')) | ||
| name = f"Btn {component_number}" |
There was a problem hiding this comment.
This is going to significantly change behavior for users of this library. Why not keep the name as the engraving?
There was a problem hiding this comment.
- This is the XML for the CSD.
As you can see the name is always CSD 001, and there can be multiple keypads in the same area. So CSD 001 is not the name. We could infer the name from the DeviceGroup instead, which is actually the location of the device, as in my XML a deviceGroup contains always only one keypad, but is that true for everyone?
-
Phantom keypads. Is not an odd variant, it's included in the documentation and it's a very useful way to create custom button to launch scenes. I agree with you that inferring that it's a phantom keypad from an else is not the best method.
-
I understand this, but the was an original mistake. The name of the button in my XML is always "Button x", and you cannot customise it. So that should be the name, not the engraving. Using the engraving can be an option on the HA side, as I'm proposing to do with the location name in the area name, and to use the device name in the keypad components and events. Moreover it's not unusual to have multiple keypads on large rooms, that have button with the same engraving, so the name it's not unique.
There was a problem hiding this comment.
If we could try to keep the replies attached to the code they're about, it'll make the review easier to manage.
Both RadioRA2 and Homeworks appear to have the same quirk:
- The button names are practically worthless (non-descriptive)
- Engravings can be rather common
This nuance is already handled in Home Assistant today. So I'm trying to understand why you'd want significantly different behavior for Homeworks. I agree that this library probably should have stayed true to the data model and exported engraving and name as separate properties, and I'd be happy to address that in a separate PR. But for this one, I think we should stay consistent.
Or to put it another way: The addition of Homeworks should in no way change the behavior for RadioRA2 users.
There was a problem hiding this comment.
Why we don't address this problem on the HA side instead? Use a flag "Use Engraving as Button Name", default True, and let people decide?
In my case I have 34 keypads. Every keypad has btn1 as the main light for the room, with "Luce" as engraving. As it it's I would end up with 34 buttons and scenes with "Luce_x" as entity_id, and I would have to change each entity id to something meaningful.
There was a problem hiding this comment.
If we could try to keep the replies attached to the code they're about, it'll make the review easier to manage.
I would, but in github for a lot of comments I cannot add replies, but only at the end of a group.
pylutron/__init__.py
Outdated
| @@ -737,6 +742,18 @@ def level(self, new_level): | |||
| # self._lutron.send(Lutron.OP_EXECUTE, Output._CMD_TYPE, | |||
| # Output._ACTION_ZONE_LEVEL, new_level, fade_time, delay) | |||
|
|
|||
| def start_raising(self): | |||
There was a problem hiding this comment.
Looking at documentation, jogging is ONLY in Homeworks, so it would make more sense to me to have:
class Motor(Shade):
And add jogging there.
|
Below a phantom keypad. There are no properties. And the Name is CDS 001 |
…s-support # Conflicts: # pylutron/__init__.py
This PR is adding: