Skip to content

Use standard entity_ids for zwave entities#7786

Merged
emlove merged 9 commits into
home-assistant:devfrom
emlove:zwave-standard-entity-ids
Jun 16, 2017
Merged

Use standard entity_ids for zwave entities#7786
emlove merged 9 commits into
home-assistant:devfrom
emlove:zwave-standard-entity-ids

Conversation

@emlove
Copy link
Copy Markdown
Contributor

@emlove emlove commented May 26, 2017

Description:

This PR removes the entity_id generation logic, and allows zwave entities to use the normal hass entity_id generation from the entity name. After this change, if two entities would be given the same name, hass will automatically suffix them _2, _3, etc. using the normal conflict resolution logic used by other platforms. This creates non-deterministic entity naming, since they may be added in any order. This was previously not correctable by the user, so the node/index workarounds were introduced. After #7780 is merged, users will have the tools necessary to disambiguate these names themselves. The end result will bring the zwave behavior closer in line with other platforms, and give users greater control over the final entity_id names, without introducing additional layers of workarounds.

This also introduces a small API breakage, where EVENT_SCENE_ACTIVATED and EVENT_NODE_EVENT will no longer supply an object_id. They will now be tied to the node entity_id.

Pull request in Polymer: home-assistant/frontend#305

See also:

#3759
#6912
#7089

CC: @balloob @andrey-git @turbokongen

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2815
home-assistant/home-assistant.io#2816

@mention-bot
Copy link
Copy Markdown

@armills, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @turbokongen and @fabaff to be potential reviewers.

@turbokongen
Copy link
Copy Markdown
Contributor

So, this will piss of a lot of people 😰
How will this behave over restarts, since zwave is based on discovery, and not extracted from the config?
Does the same node, and same value be recognized as the same entity_id again and again over restarts?

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented May 27, 2017

Yeah, it will definitely be a big ordeal, and we'll want to let people know it's coming long before we merge it. But it should finally give people the control over the entity_ids that they've been looking for.

The value label changes are persisted in zwcfg.xml across restarts, so the same nodes gets recognized as the same entity_id. It also means that if users re-include a node and rename it to its old name, it will get the old entity_id back, so users don't have to change they're whole configuration.yaml.

@andrey-git
Copy link
Copy Markdown
Contributor

The downside is that zwcfg.xml is now not merely a safe-to-delete cache, like it was, but a critical part of a config.

@andrey-git
Copy link
Copy Markdown
Contributor

This is bringing a breaking change to implement a feature that can be implemented without a breaking change like proposed in the comments here: #7089

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented May 27, 2017

It's definitely true that zwcfg.xml is now a file that needs to be saved.

I understand that breaking changes suck, but I'm more focused on what this platform is going to look like a year from now. I'm approaching this with the goals of making this more conformant with the beahvior of the rest of Home Assistant, and trying to reduce code complexity. I really don't want to stack another workaround on top of entity_ids because we needed to define entity_ids as a workaround in the first place. I think now that we have a little more stability in zwave with the threading improvements and unit testing, we can start to wrangle some of the code that wasn't ideal but necessary to grow.

I'm also not saying this should be merged right away. I agree with all of your concerns, and I want to make sure everything is addressed before we think about merging. We'd want several devs running this for a while to ensure that the rollout goes as smoothly as possible.

@andrey-git
Copy link
Copy Markdown
Contributor

Then for a transition period maybe add an attribute that will contain the "old" ID? This was people can edit their configs more easily.

Maybe even for a pre-transition period add the "new" ID as attribute so people can pre-edit their configs.

I.e.

  1. Add service & UI for label changing. Add newID as attribute.
  2. [At least 1 release later] Start using the new ID and put oldID in attribute.
  3. [After a few releases] Remove the extra attribute.

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented May 28, 2017

I like the idea of a transition period where the alternate ID is available as an attribute. What about a transition period of several releases where we let people know the IDs will be changed soon, but they can opt-in earlier with a temporary config option? Hopefully that would give people plenty of time to update their configs without forcing a breakage right away.

@emlove emlove changed the title [RFC] Use standard entity_ids for zwave entities Use standard entity_ids for zwave entities Jun 15, 2017
@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 15, 2017

OK, all the prerequisites for this have now been merged. I updated this so that it is opt-in for now. I've created a blog post PR explaining the change here: home-assistant/home-assistant.io#2816

generated_id = generate_entity_id(DOMAIN + '.{}', name, [])
else:
generated_id = entity.entity_id
node_config = device_config.get(generated_id)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the ID list in line 327 is empty it will just slugify it without collision prevention.
So in case of collision both device will have the same ID here pulling the same config.

Also, since rename_value acts by value_id which is not exposed to the UI in any way, does user using the UI rename card has a way to determine which of the two identical entities they are renaming?

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.

So for the first point, unfortunately the ID post collision detection can't be used to ignore a device, since that ID is only available after the entity is added to hass. Colliding entity_ids aren't really meant to be used long-term anyway, so after they are renamed it won't be an issue.

For the second point, what do you think about exposing the value ID in the device state attributes, then including it in the rename card drop-down?

@andrey-git
Copy link
Copy Markdown
Contributor

andrey-git commented Jun 15, 2017

Here is the flow that is going to break with the new IDs:
Each zwave device adds lots of entities, some of which are redundant. Currently I deal with those using globs. I.e. ignore fibaro_222_*_11 - globbing out the node_id. Thus when I add another fibaro - it looks pretty from the start.

Now I have to rename all useful entities, and to either manually ignore or rename all nonuseful entities.

Now kidding aside - could you add an attribute with the old and new IDs so that people can look at it and edit their configs before switching the optin value?

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 15, 2017

I'll add the attribute with the old entity_id. (Forgot to include that)

For the Fibaro devices, you should be able to do the same thing still, right? The entity IDs will be in the format node_name_value_name, so you could just glob out the node name portion.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 16, 2017

oops. I accidentally already merged the frontend part as I didn't read the description. Good thing is that even with the values not present, the frontend won't break, just contain some undefined text.

@andrey-git
Copy link
Copy Markdown
Contributor

andrey-git commented Jun 16, 2017

New ID is needed as attributes too. Here is what I see as a conversion flow (add to blog post if you agree):

  1. Run without new ID.
  2. check would-be ids in the attributes for conflicts.
  3. Rename entities using the ui card and back to (2)
    3.5 probably restart hass so that name change take effect.
  4. Update all places mentioning IDs (groups, automation, customization, etc.) By editing Yaml.
  5. Run with new IDs
  6. Fix broken things using the old id in attributes.

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 16, 2017

OK, I've got the new entity IDs in there as well.

I like the instructions, but right now the blog post is mostly prose/informational. I think what we should do is add those instructions into the release notes, and we can link to the blog post for more background information. That way the users that just want to get it working have a quick guide and can skip the reading entirely if they want.

@andrey-git
Copy link
Copy Markdown
Contributor

Should this be picked for 0.47?

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 16, 2017

I think so. The polymer portion got pushed already, and it's still opt-in at this point.

@emlove emlove added this to the 0.47 milestone Jun 16, 2017
@emlove emlove merged commit afb9cba into home-assistant:dev Jun 16, 2017
@emlove emlove deleted the zwave-standard-entity-ids branch June 16, 2017 17:07
emlove added a commit that referenced this pull request Jun 16, 2017
* Use standard entity_ids for zwave entities

* Include temporary opt-in for new entity ids

* Update link to blog post

* Update tests

* Add old entity_id as state attribute

* Expose ZWave value details

* Update tests

* Also show new_entity_id

* Just can't win with this one
@balloob balloob mentioned this pull request Jun 16, 2017
@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 17, 2017

Cherry-picked for 0.47

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants