Skip to content
This repository has been archived by the owner on Oct 1, 2021. It is now read-only.

Pass the device name without mapping to the component #184

Merged
merged 6 commits into from
Apr 8, 2018
Merged

Pass the device name without mapping to the component #184

merged 6 commits into from
Apr 8, 2018

Conversation

syssi
Copy link
Contributor

@syssi syssi commented Apr 4, 2018

No description provided.

@rytilahti
Copy link
Contributor

Is there a need to be so specific when matching for type? I think I would simply merge all those different versions into one, if there's no specific reason for that.

Or more broadly, is it even necessary to have this type information available at all?

@syssi
Copy link
Contributor Author

syssi commented Apr 4, 2018

The type will be important some day because every ceiling lamp has different features for example. But you are right... we shouldn't map the device type here. The full name should be passed to the component.

@syssi syssi changed the title mDNS prefix of the Yeelight Ceiling Light 4 (Jiaoyue 650) added Pass the device name without mapping to the component Apr 5, 2018
@syssi
Copy link
Contributor Author

syssi commented Apr 5, 2018

This PR will change the entity id of discovered yeelights:

https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/light/yeelight.py#L126-L128

We could introduce some legacy handling. Suggestions?

@@ -30,11 +31,12 @@ def info_from_entry(self, entry):
elif entry.name.startswith("yeelink-light-ceiling2_"):
device_type = "ceiling2"
else:
logging.warning("Unknown miio device found: %s", entry)
device_type = \
entry.name.replace(DEVICE_NAME_PREFIX, '').rsplit('_', 1)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why wouldn't we always do this? It seems unnecessary for us to make up our own device types instead of using the ones provided by Yeelink.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The device_type is part of the entity id if the device is auto-discovered. The old mapping provides stable entity ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the previous mapping would be a breaking change for consistency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It will only be a breaking change if not resolved in Home Assistant. I think that it is weird that a discovery library is introducing their own names for types. We should follow the vendor types.

@syssi
Copy link
Contributor Author

syssi commented Apr 8, 2018

I've removed the mapping and will introduce it at Home Assistant.

@balloob balloob merged commit 5b4ff1e into home-assistant-libs:master Apr 8, 2018
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.

4 participants