Skip to content

HomeMatic: Enable entity registry#15950

Merged
MartinHjelmare merged 3 commits intohome-assistant:devfrom
danielperna84:homematic_entity_registry
Aug 14, 2018
Merged

HomeMatic: Enable entity registry#15950
MartinHjelmare merged 3 commits intohome-assistant:devfrom
danielperna84:homematic_entity_registry

Conversation

@danielperna84
Copy link
Copy Markdown
Contributor

Description:

Enabeling support for the entity registry. The entity IDs the HomeMatic component creates by default are uniqe and for every device always the same entity will be generated. This also works when the option to generate userfriendly entity IDs is being used. The entity ID can change though when using the userfriendly names (when the friendly name is changed in the hub-device). This will generate an entry in the registry that becomes stale.
The clean approach would be to disable the possibility the generate userfriendly names altogether. But as far as I know quite a few users are using this option, and removing support for that would break these configurations fundamentally. The way it is implemented looks acceptable to me since only a subset of the HomeMatic users would be affected by stale entries.

Checklist:

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

@ghost ghost assigned danielperna84 Aug 12, 2018
@ghost ghost added the in progress label Aug 12, 2018
@danielperna84 danielperna84 requested a review from pvizeli August 12, 2018 21:38
@MartinHjelmare
Copy link
Copy Markdown
Member

Is it possible to check if a friendly name is used instead of a unique name?

We shouldn't return a name that is not unique, ever. That's against the rules for unique_id. If we can't return a unique name we can return None. This can be selected on a per entity basis.

@property
def unique_id(self):
"""Return unique ID. HomeMatic entity IDs are unique by default."""
return self._name
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please clarify if the user can make this non unique or if this will always be constant (unique). Reading this PR description and the linked issue, it's not clear to me what you mean might change due to setting friendly name (using the resolve names function).

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Aug 13, 2018

Choose a reason for hiding this comment

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

As long as the unique_id is unique and doesn't change for an entity, the entity_id for that entity will not change during restarts, even if later another entity_id is suggested (eg by the user in device app).

That's the whole point of the entity registry. The entity registry is the truth for the entity_ids. So after an entity has been registered, the entity_id can only be changed via the entity registry or an api that uses the entity registry.

@danielperna84
Copy link
Copy Markdown
Contributor Author

Is it possible to check if a friendly name is used instead of a unique name?

I'm not sure if it's possible to pass on that bit of information easily. There's an option for the component called resolvenames. If this has a value, then the component makes and extra request to the hub and asks for the names. So if entities are able to know about the component-config, then yes, the unique id could be conditional on this.

Please clarify if the user can make this non unique or if this will always be constant (unique). Reading this PR description and the linked issue, it's not clear to me what you mean might change due to setting friendly name (using the resolve names function).

By default the entity IDs of HomeMatic entities look like this: sensor.ABC123456_1_TEMPERATURE, where the ABC...-part is a unique serial number of the device, the single digit in the middle is the channel the specific entity operates on, and (optionally) there's even a specific parameter (in case the device has more than one).
If userfriendly names a fetched from the hub (they can be renamed there just like e.g. Hue Bulbs), then the resulting entity will become sensor.KITCHEN_THERMOSTAT_1_TEMPERATURE. So the part where the serial number has been will be replaced with the friendly name. So yes, this can change when changing the name on the hub. But I think users won't really do this because that currently changes their entity IDs, and therefore breaks all their automations.

Later today I will try how it works if I manually set the unique ID to always be the same value as when no friendly names are being resolved. I tried that yesterday and has some trouble with that. But I did it in a stupid way. Maybe the one I got on my midn now works better.

@danielperna84
Copy link
Copy Markdown
Contributor Author

Ok, so I changed the code to this:

    @property
    def unique_id(self):
        """Return unique ID. HomeMatic entity IDs are unique by default."""
        if self._state:
            return "{}_{}_{}".format(self._address, self._channel, self._state)
        return "{}_{}".format(self._address, self._channel)

But this gives me the same strange behavior as yesterday. Don't know if this also could be a bug in the registry.

Here's what happens:

  1. I have an entity with the entity ID climate.abc123456 and an unique ID of ABC123456_1 (it includes the main channel).
  2. On first start the registry gets populated with an entry like this:
climate.abc123456:
  config_entry_id:
  name:
  platform: homematic
  unique_id: ABC123456_1
  1. When I use the GUI to change the friendly name and entity ID I get an additional entry which look like this:
climate.abc123456:
  config_entry_id:
  name:
  platform: homematic
  unique_id: ABC123456_1_SET_TEMPERATURE
climate.mynewentityid:
  config_entry_id:
  name: myfriendlyname
  platform: homematic
  unique_id: ABC123456_1
  1. I have to restart to fully activate the changes.

With the solution I so far have in this PR the changes become active instantly and I don't get 2 entries.

The second entry I get with this new approach is what's making me scratch my head, since it doesn't show up in the dev-panel. So it looks like an intermediate entity that was created by the registry-code. If it helps: the prepended _SET_TEMPERATURE is the paramter the device uses to target the current (set) temperature - the state-parameter that is being used when querying the hub on startup.

This second approach definitely is not functional, althouth in theory this would mitigate the issues with the uncertain entity IDs the component creates when it uses the names from the hub.

@danielperna84
Copy link
Copy Markdown
Contributor Author

danielperna84 commented Aug 13, 2018

I changed the code now to look at the name, and only if it (still) includes the serial number, then the unique ID is generated. Assuming people don't include the serial in the friendly name they have set up on the hub (which is highly probable since that's why they change the name in the first place), this will exclude those users from the entity registry, and therefore the requirement of static uniqe IDs is met.

This means that people who want to use the registry have to disable the component-feature (resolving names). So it's kind of opt-in for them, and enabled by default for the others. Is this an acceptable solution?

I'll see if I can come up with a helper-script ran manually by HomeMatic users that makes it easy to transition from a setup with resolved names to the regular mode.

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

That was a long time on my task list. But it is not so easy like this PR :) Would be nice if you can finish it:

  • _create_ha_name should be -> _create_ha_unique (or so?) and use the address instead name to generate a unique ID. Name should be work like before, se we should be able to reuse this function, for unique ID we call it with address and for name we call it with name like now.

@danielperna84
Copy link
Copy Markdown
Contributor Author

I feel sooooo stupid! Works great, even with resolvenames enabled and later disabled.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@MartinHjelmare MartinHjelmare merged commit 69b694f into home-assistant:dev Aug 14, 2018
@ghost ghost removed the in progress label Aug 14, 2018
@frenck
Copy link
Copy Markdown
Member

frenck commented Aug 14, 2018

@danielperna84 I think I'm missing a documentation PR for this one?

@danielperna84 danielperna84 deleted the homematic_entity_registry branch August 14, 2018 08:32
@danielperna84
Copy link
Copy Markdown
Contributor Author

@frenck
I'm not sure. The Hue component (also supports the regitry) does not mention support in the documentation. Is there another place where support should be listed? The final solution of this PR works for all users, regardless of their configuration.

@frenck
Copy link
Copy Markdown
Member

frenck commented Aug 14, 2018

@danielperna84
Interesting... Not sure about it now...
Leave it as is for now.

@MartinHjelmare
Copy link
Copy Markdown
Member

Yeah, we don't track entity registry support consistently at the moment. If someone adds the framework for that we can start doing it.

@balloob balloob mentioned this pull request Aug 29, 2018
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
* Set unique ID

* Excluding setups that resolve names

* Added support for resolvenames again
vrih pushed a commit to vrih/home-assistant that referenced this pull request Sep 29, 2018
* Set unique ID

* Excluding setups that resolve names

* Added support for resolvenames again
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants