Skip to content

Set InsteonEntity name to be combo of description and address.#17262

Merged
balloob merged 12 commits intohome-assistant:devfrom
wonderslug:insteon_entity_name_17194
Dec 14, 2018
Merged

Set InsteonEntity name to be combo of description and address.#17262
balloob merged 12 commits intohome-assistant:devfrom
wonderslug:insteon_entity_name_17194

Conversation

@wonderslug
Copy link
Copy Markdown
Contributor

Description:

The Insteon initial entity_id is currently only the unique address of the Insteon device. This adds the device description to the name to allow easier identification

ie "LampLinc Dimmer 26453a" or "Keypad Dimmer 291abb_2"

Cleans up the unique_id so there is no duplicated code

Related issue (if applicable): fixes #17194

**Pull request in home-assistant.io with documentation (if applicable): NA

Example entry for configuration.yaml (if applicable):

insteon:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

ie "LampLinc Dimmer 26453a" or "Keypad Dimmer 291abb_2"

Using a centralized network name
@ghost ghost added the in progress label Oct 8, 2018
@wonderslug wonderslug changed the title Set InsteonEntity name to be combo of description and address. WIP Set InsteonEntity name to be combo of description and address. Oct 8, 2018
@wonderslug
Copy link
Copy Markdown
Contributor Author

Opened to clean up #17195 instead of rebase

@wonderslug
Copy link
Copy Markdown
Contributor Author

info about integration
#17195 (comment)

name = '{:s}_{:d}'.format(self._insteon_device.id,
self._insteon_device_state.group)
name = '{:s} {:s}'.format(self._insteon_device.description,
addr)
Copy link
Copy Markdown
Contributor

@teharris1 teharris1 Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is name expected to be a human readable name? If so, can I suggest using the following:

name = '{:s} {:s} Group {:d}'.format(self._insteon_device.description,
                                     self._insteon_device.address.human,
                                     self._insteon_device_state.group)

This will create a name that looks like KeyPadLinc Dimmer 1A.2B.3C Group 5

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Human address is great and will add it in. As well I'm looking at making it even more contextual, adding "Fan"/"Light" for a FanLinc or "Relay"/"Sensor" for an I/OLinc instead of the group number id. Ill post an update to the PR soon.

@wonderslug
Copy link
Copy Markdown
Contributor Author

This ends up doing a human readable address and contextually relevant labels instead of group id number

Example

@teharris1
Copy link
Copy Markdown
Contributor

@wonderslug
I like it! This looks great, especially for new users.

Just to be clear, hopefully we don't break the entity_id, especially for the current Hub users. When we merged insteon_plm and insteon_local all the Hub users had to recreate their entity maps for automations, etc. I don't what to do that to them again.

Just out of curiosity, do you use a PLM or a Hub?

@wonderslug
Copy link
Copy Markdown
Contributor Author

wonderslug commented Oct 10, 2018

@teharris1 Thanks. Im trying to catch anything which has multiple states under the device, but I don't have the thermostat in there yet and I'm not sure how its going to respond to one. Don't have one to play with.

This IS a breaking change on entity id but only if there is an upgrade without moving to a release that uses the new entity registry first. It needs to be relabeled as so. But based on @balloob 's comment on the original PR and comment on the unique_id addition the idea was that we would wait until 84 to release to allow for the new registry to get well used and catch most users once the unique_id is being used and the entity_id is set in the registry. That would keep the one thats already been set and used just as if its a customized one.

I actually have both the Hub and a PLM. Im doing a migration from Indigo to Hass and have the PLM on my Indigo and the hub, which has all the same devices registered, connected to by Hass. My goal is to get all my migration done using the Hub and then switch over to the PLM once I'm done.

@teharris1
Copy link
Copy Markdown
Contributor

This still feels like a lot of work for what you are trying to accomplish. Can we look at adding a field to the IPDB class and using the statename to get what you are looking for? So, for example an IPDB state would look like this:

State(OnOffSwitch_OutletTop, 'switch', 'Top')

The only states I can see having an issue are the OnOffKeypad and DimmableKeypad. All of the other ones are pretty clean I think.

The other option is to look at the InsteonEntity._insteon_device_state.name attribute. While this is not a human-readable name, it could be used in a lookup. For example, the OnOffKeypad states have generated names using "keypadButton{}".format(button_list[group]) where button_list is the same as your button_8_label and button_6_label (albeit with some minor differences).

@wonderslug
Copy link
Copy Markdown
Contributor Author

This still feels like a lot of work for what you are trying to accomplish. Can we look at adding a field to the IPDB class and using the statename to get what you are looking for? So, for example an IPDB state would look like this:

State(OnOffSwitch_OutletTop, 'switch', 'Top')

The only states I can see having an issue are the OnOffKeypad and DimmableKeypad. All of the other ones are pretty clean I think.

I looked at this. The main issues are duplicate state types in the states list and the index in the states matters to define the what to label as.

The motion sensor for example has three OnOffSensors being states with 0x01 as Motion, 0x02 as Light, and 0x03 as Battery. The Water Leak sensor has something similar. So the combination of which device has a state and which index is the state matters to the context of the label.

The other option is to look at the InsteonEntity._insteon_device_state.name attribute. While this is not a human-readable name, it could be used in a lookup. For example, the OnOffKeypad states have generated names using "keypadButton{}".format(button_list[group]) where button_list is the same as your button_8_label and button_6_label (albeit with some minor differences).

I think this might work. I will explore it a bit more. The debug I did so far looks promising.

@wonderslug
Copy link
Copy Markdown
Contributor Author

I moved the label lookup to a simple dict mapping. This should work fine and allow mapping to changes made in insteonplm. Any new state name added to insteonplm will need to be added, but I don't see that as happening often.

@wonderslug
Copy link
Copy Markdown
Contributor Author

@teharris1 Hey what do you think about updating the insteonplm lib to 0.15.0 as part of this PR since its been released now?

@teharris1
Copy link
Copy Markdown
Contributor

A PR has already been merged with it. Update your branch to see it.

@wonderslug
Copy link
Copy Markdown
Contributor Author

Well then nevermind that then :)

@teharris1
Copy link
Copy Markdown
Contributor

It supports python 3.7 and has some hub improvements so rather than wait for this more extensive PR it made sense to push it out quick.

@balloob balloob changed the title WIP Set InsteonEntity name to be combo of description and address. Set InsteonEntity name to be combo of description and address. Dec 14, 2018
@balloob balloob merged commit a30921e into home-assistant:dev Dec 14, 2018
@ghost ghost removed the in progress label Dec 14, 2018
dshokouhi pushed a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018
…assistant#17262)

* Set InsteonEntity name to be combo of description and address.

ie "LampLinc Dimmer 26453a" or "Keypad Dimmer 291abb_2"

Using a centralized network name

* Updated the name to have a more contextual references for device
functions then just the group id.

* Cleanup for hound

* Removed the _generate_network_address function.  Not used anymore

* Cleanup for lint

* clean for hound

* Moved descriptor mapper to be a class variable of the InsteonEntity in order to fix lib loading issue for lint

* Well, moved DescriptorMapper instance to a function variable now...

* fix hound

* Lint Cleanup

* Clean up docstrings

* Simplify Label lookup based on state name
@balloob balloob mentioned this pull request Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adjust Insteon initial entity id to be more descriptive

6 participants