Lutron component overhaul and improvements#36902
Lutron component overhaul and improvements#36902cdheiser wants to merge 8 commits intohome-assistant:devfrom
Conversation
|
Hey there @JonGilmore, mind taking a look at this pull request as its been labeled with a integration ( |
|
Excellent @cdheiser! I'll do some testing of this later today and get back to you |
cdheiser
force-pushed
the
pylutron-upgrade
branch
from
e9cbcab to
2aca160
Compare
|
I've rebased and fixed some of the lint errors. I have some outstanding PRs for pylutron to get in to clean up the rest. |
1) Buttons The LutronButton has been updated to support press (and hold), release, and tap events. This allows automations and other integrations to activate buttons like an alarm. 2) Unique IDs. pylutron 0.2.6 added support for unique IDs. These have been added to the entities here.
cdheiser
force-pushed
the
pylutron-upgrade
branch
from
50a1bd0 to
39475ff
Compare
- Make it possible to ask the AreaRegistry to return an area or create it if one does not exist. - Make it possible for a device to assert which area it should be in. This is useful for integration with platforms that also have a concept of areas.
- Upgrade pylutron to 0.2.6
- Add support for configuring via the integrations UI
- Bring in new unique IDs from pylutron
- Properly define devices vs. entities
- Keypads, Switches, Main Repeater, and Remotes are all devices
- Add support for importing Area names and assignments from Lutron
- Users can enable this to import areas and then turn it off to
override specific devices.
- Add services for press-and-hold, release, and tapping of buttons
- This allows for the use of the alarm button where holding the
holding the button is required to keep the alarm state active.
cdheiser
force-pushed
the
pylutron-upgrade
branch
from
39475ff to
6d6463a
Compare
I accidentally dropped the update-after-add for adding entities with the overhaul to use config entries and async code.
|
Just getting around to testing this out. Things are looking good, but I'm seeing some issues with text displaying correctly. For example, when you click options on the Lutron integration, you'll see empty text here: Or, when you add the integration via the UI, it's missing text description labels as well: |
The new version of pylutron properly binds an OccupancyGroup to all areas as defined in the XML.
…ne and assign devices to areas.
|
So that's because translations are missing, and it's not clear to me if I'm
supposed to check in the ones that the local dev tools produce, or if that
should get handled by another process (it's not really well documented)
…On Sun, Jun 21, 2020 at 1:59 PM Jon Gilmore ***@***.***> wrote:
Just getting around to testing this out. Things are looking good, but I'm
seeing some issues with text displaying correctly. For example, when you
click options on the Lutron integration, you'll see empty text here:
[image: image]
<https://user-images.githubusercontent.com/7232986/85235095-ef28f200-b3d7-11ea-8e51-097310517584.png>
Or, when you add the integration via the UI, it's missing text description
labels as well:
[image: image]
<https://user-images.githubusercontent.com/7232986/85235126-24354480-b3d8-11ea-83ce-ca7ee43cf016.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36902 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACQARWXN5UIQ5EENDQVEMFLRXZYCZANCNFSM4OBIDMRQ>
.
|
|
@cdheiser You can use to generate translation files to test locally. But do not commit the generated files. |
|
That's what I thought. Thanks for confirming. So @JonGilmore you can generate the right translation files locally to fill in the words. And you really need to try turning on the option to use areas from Lutron. I can't believe it took me this long to suggest this PR. |
- Lutron now supports config_flow - And also now has an optional setting via the UI to use Areas defined by the Lutron RadioRA2 system.
…f accessing the protected class member.
|
This should now be ready to go. pylutron was updated today as was this PR |
|
thanks @cdheiser - i'll be testing this out today and I'll get back to you with any feedback |
|
What's next? |
|
@homeassistant What's next? |
|
@JonGilmore you have any idea how to get someone assigned to accept and merge this PR? |
|
@MartinHjelmare can you help us in getting this merged? |
|
Ping?
…On Sat, Aug 1, 2020, 6:10 AM Jon Gilmore ***@***.***> wrote:
@MartinHjelmare <https://github.com/MartinHjelmare> can you help us in
getting this merged?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36902 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACQARWUMIRXYKV7RGKH6MXLR6QH53ANCNFSM4OBIDMRQ>
.
|
|
@frenck could you help us out? |
|
Ping? Do I need to file a bug to get someone to review this PR?
…On Sat, Aug 8, 2020, 9:50 AM Jon Gilmore ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36902 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACQARWVEKL3KYHSQ64VT4CLR7V637ANCNFSM4OBIDMRQ>
.
|
|
Please be patient. We'll get to it. |
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Tests for the config flow are missing. Please use the scaffold script to get started with that.
Have the area registry changes been discussed with the core team? I'm not sure we'll approve those.
| from homeassistant import config_entries | ||
| from homeassistant.const import ATTR_ID, CONF_HOST, CONF_PASSWORD, CONF_USERNAME | ||
| from homeassistant.helpers import discovery | ||
| from homeassistant.helpers import area_registry as ar, device_registry as dr |
There was a problem hiding this comment.
Please use longer variable names than two characters.
There was a problem hiding this comment.
@MartinHjelmare I'm asking to learn, but this example shows something very similar, so is the example incorrect?
There was a problem hiding this comment.
This particular case is fine. Normally we want at minimum three characters.
|
|
||
| async def async_setup_entry(hass, entry): | ||
| """Set up a single Lutron deployment.""" | ||
| if not entry.data: |
There was a problem hiding this comment.
Why would entry.data be falsy? That can't happen.
| config = entry.data | ||
| device_registry = await dr.async_get_registry(hass) | ||
| area_registry = await ar.async_get_registry(hass) | ||
| hass_data = { |
There was a problem hiding this comment.
Please name this something that's not so easily confused with hass.data.
| hass_data = { | |
| lutron_data = { |
| }, | ||
| } | ||
| hass.data[DOMAIN][entry.entry_id] = hass_data | ||
| hass_data[LUTRON_CONTROLLER] = Lutron( |
There was a problem hiding this comment.
| hass_data[LUTRON_CONTROLLER] = Lutron( | |
| controller = hass_data[LUTRON_CONTROLLER] = Lutron( |
|
|
||
| hass.data[LUTRON_CONTROLLER].load_xml_db() | ||
| hass.data[LUTRON_CONTROLLER].connect() | ||
| await hass.async_add_executor_job(hass_data[LUTRON_CONTROLLER].load_xml_db) |
There was a problem hiding this comment.
| await hass.async_add_executor_job(hass_data[LUTRON_CONTROLLER].load_xml_db) | |
| await hass.async_add_executor_job(controller.load_xml_db) |
| if user_input is None: | ||
| return self._show_setup_form() | ||
|
|
||
| await self.async_set_unique_id(user_input[CONF_HOST]) |
There was a problem hiding this comment.
Ip address is not allowed as unique id. Is this an ip address?
There was a problem hiding this comment.
If there's no other good unique id, we can just check the existing config entries and make sure we don't set up another entry with the same host. Ie do this without using the unique id api.
| await self.async_set_unique_id(user_input[CONF_HOST]) | ||
| self._abort_if_unique_id_configured() | ||
|
|
||
| return self.async_create_entry(title=user_input[CONF_HOST], data=user_input) |
There was a problem hiding this comment.
Before creating the entry we need to validate the connection to the controller.
| "name": f"{self._area.name} {self._keypad.name}", | ||
| "manufacturer": "Lutron", | ||
| "model": self._keypad.type, | ||
| # "sw_version": self.light.swversion, |
There was a problem hiding this comment.
| # "sw_version": self.light.swversion, |
| "name": f"{self._area.name} {self._keypad.name}", | ||
| "manufacturer": "Lutron", | ||
| "model": self._keypad.type, | ||
| # "sw_version": self.light.swversion, |
There was a problem hiding this comment.
| # "sw_version": self.light.swversion, |
| return self.areas.values() | ||
|
|
||
| @callback | ||
| def async_get_or_create(self, name: str) -> AreaEntry: |
There was a problem hiding this comment.
This needs to be approved first by the core team. Has this been discussed?
There was a problem hiding this comment.
Have the area registry changes been discussed with the core team? I'm not sure we'll approve those.
They have not. How would you suggest we proceed on that specifically? (I could separate them out into a different PR). This is incredibly useful for those of us with systems that have their own notion of areas. Generally speaking, this was the entire motivation for this entire overhaul.
I'll go over the other requested changes and config_flow tests.
There was a problem hiding this comment.
Please open an architecture issue.
https://github.com/home-assistant/architecture/issues
There was a problem hiding this comment.
Sounds good to remove it from this PR if possible so we aren't blocked by that review.
There was a problem hiding this comment.
I've created home-assistant/architecture#425 and a PR specifically for that functionality.
I'll rebase this PR and then split out the area integration support and work on the comments.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
I plan to get back to revising this in the next week or so. |
|
@cdheiser @MartinHjelmare I know & appreciate you guys are busy—would you mind quickly sharing what all still needs to be done here? Happy to try and help. |
|
The comments above need to be addressed. |
|
This PR seems to have gone stale. Closing it. |
|
@cdheiser There's some good changes here. Would you have any interest in merging them in a new PR before the area stuff goes through? I'm happy to help in anyway that I'm able to. |
|
I do have interest in getting changes merged in, I'm just short on time at
the moment.
…On Wed, Feb 10, 2021 at 2:26 PM Josh ***@***.***> wrote:
@cdheiser <https://github.com/cdheiser> There's some good changes here.
Would you have any interest in merging them in a new PR before the area
stuff goes through? I'm happy to help in anyway that I'm able to.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36902 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACQARWXFZQATIM66XI67GOLS6MB2JANCNFSM4OBIDMRQ>
.
|
|
What would be the proper way to pick this up? Do I fork HA and manually add in your changes or do I start with your branch? |
|
I'm attempting to rebase this PR. It's been a while and thus I may not be
successful, but if I can rebase it successfully, it'll be an easier place
to start from.
…On Mon, Mar 15, 2021 at 3:30 PM Josh ***@***.***> wrote:
What would be the proper way to pick this up? Do I fork HA and manually
add in your changes or do I start with your branch?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36902 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACQARWV7BO67Q42H6QBRQB3TD2C7VANCNFSM4OBIDMRQ>
.
|
|
I believe https://github.com/cdheiser/home-assistant/tree/pylutron-upgrade
should now be both caught up to reality, and should work properly with
suggested_area support that was added. (I haven't tested it at all yet).
On Sat, Mar 20, 2021 at 10:18 PM Chris Heiser ***@***.***>
wrote:
… I'm attempting to rebase this PR. It's been a while and thus I may not be
successful, but if I can rebase it successfully, it'll be an easier place
to start from.
On Mon, Mar 15, 2021 at 3:30 PM Josh ***@***.***> wrote:
> What would be the proper way to pick this up? Do I fork HA and manually
> add in your changes or do I start with your branch?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#36902 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACQARWV7BO67Q42H6QBRQB3TD2C7VANCNFSM4OBIDMRQ>
> .
>
|
8 participants
Proposed change
override specific devices.
holding the button is required to keep the alarm state active.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale: