Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Physical device #12

Merged
merged 9 commits into from
Sep 15, 2020
Merged

Physical device #12

merged 9 commits into from
Sep 15, 2020

Conversation

rospogrigio
Copy link
Owner

Merges #6 and #11.
Moreover, introduces the creation of Devices under Configuration-Devices, regrouping related subswitches.

@postlund
Copy link
Collaborator

postlund commented Sep 15, 2020

I don't really understand why you are adding friendly_name here? Why can't you just use name as-is? Now we have two identifiers, one shown for the device name and one shown for the config entry (in Integrations). Why can't these be the same?

And why are you storing the names inside the pytuya instance? IMHO a name field in the library should only exist if the library itself could provide the name, e.g. by asking the device. The name is already present via name in the cover class (and all other platform classes), so there's no need for this. I suggest that you remove all those changes and just commit the device_info part.

@postlund
Copy link
Collaborator

Ok, now I looked at it again and I see the problem. We can't do it like this, we need to manually set up a device in the device registry that corresponds to the actual hardware. We need link out entities using "via_device" in device_info. Otherwise we basically say that every entity is an independent device, which isn't true.

Setting up the device is done roughly like here:

https://github.com/postlund/hass-atv-beta/blob/6e8190ce9d4ded0b03e92def88c7d583f4580efb/custom_components/apple_tv/__init__.py#L335

The "via_device" should not be there though. Each platform then link to this device like this:

https://github.com/postlund/hass-atv-beta/blob/6e8190ce9d4ded0b03e92def88c7d583f4580efb/custom_components/apple_tv/media_player.py#L228

Sorry about the confusion here. Th device registry is still a bit new to me. Device name (we publish in the first step) is fetched from the config entry, I.e. config_entry.data[CONF_NAME].

@rospogrigio
Copy link
Owner Author

rospogrigio commented Sep 15, 2020

Yes, probably since you have explained that the device_id can't be set from code probably it's better to remove the name attribute.
I had put the friendly_name into pytuya in order to be used within device_info, but actually it's not needed... ok let me refactor better this part.

@rospogrigio
Copy link
Owner Author

Ok, now I looked at it again and I see the problem. We can't do it like this, we need to manually set up a device in the device registry that corresponds to the actual hardware. We need link out entities using "via_device" in device_info. Otherwise we basically say that every entity is an independent device, which isn't true.

OK so now it's time to clarify the names. In case of a smart plug, the device is the physical device (identified by the deviceId), and then each subswitch/sensor (for voltage...) is an entity (identified by deviceId_dps), I think we might agree over this.
Now, I am not so sure that the entities are not independent. My idea was to store the device structure (i.e., the pytuya object) within hass.data , so that each of its child entities can forward the needed calls to the device. The device, then, would open the connection at the startup and keep it persistent.
I read that the via_device should be used in other situations, such as if you are using an external hub, am I wrong? https://developers.home-assistant.io/docs/device_registry_index/
I would just keep a direct device-entities connection in this architecture.
Comments?

@postlund
Copy link
Collaborator

Yes, I think we are on the same page there. How I look at it, the physical device is a "hub" and the things it can control are the entities. This simply because none if the entities themselves can function without the physical device (hub) handling communication. From a logical POV each entity, e.g. switches, are independent. A double gang socket is presented as two switches in the UI with seemingly nothing in common, since they control two different things independently and the user might not even know that it's the same physical socket. How it's implemented is a different story and I agree, we should store the shared instance in hass.data. The described model doesn't prevent us from doing so.

@rospogrigio
Copy link
Owner Author

What sounds strange to me using the via_device is that here:
https://developers.home-assistant.io/docs/device_registry_index/
it is used to introduce a different device (...bridge_id) from the one described under "identifiers" (...self.unique_id). I really don't see the advantage in using this via_device, and how we can exploit it.
I removed unneeded "friendly/names", especially from pytuya, and since it is stable I am merging to master so we can proceed from there.

@rospogrigio
Copy link
Owner Author

Moreover, I'd say next step is to get rid of TuyaCache, which is actually the wrapper for our Device.

@rospogrigio rospogrigio merged commit baee524 into master Sep 15, 2020
@postlund
Copy link
Collaborator

Lets take an example, again with double gang switch as that is an interesting case. We connect the fridge to one of the sockets and freezer to the other. The naming scheme could then be "Kitchen Home Appliances" for the physical device, "Fridge" for the entity connected to the fridge and "Freezer" for the entity connected to the... freezer. I would expect three "devices" for this in Home Assistant with the mentioned names. The physical device would have device_id as identifier in device_info and the sun-entities their unique identifiers and put device_id in via_device, so we are not making up something new here. We already have three identifiers and those are the ones we use. This gives us a logical topology so we can see how the devices are structured.

I think you still left a small bug in your revert, because you pass name of the integration into the device_info instead of friendly_name of each platform. This would correspond to naming each of the entities "Kitchen Home Appliances" in the example above, which is wrong. Revert that part as well and just use self.name as name.

@rospogrigio
Copy link
Owner Author

I think you still left a small bug in your revert, because you pass name of the integration into the device_info instead of friendly_name of each platform. This would correspond to naming each of the entities "Kitchen Home Appliances" in the example above, which is wrong. Revert that part as well and just use self.name as name.

Ooops, right. fixing it.

Mmm, here is where I don't agree much, or maybe I am not getting it. I would just expect 1 device, and 2 entities, not 3 devices.
Let's take my smart plug as another example. Have you tried to run this PR? What I currently have is, running this PR (see also the attached images):

  1. 1 Integration, named "Stereo Smart Plug Wifi (YAML)" (in italian 😄 )
  2. 1 Device, named "Stereo Smart Plug Wifi" , that has its local_deviceId as unique_id
  3. 2 Entities, representing the 2 subswitches ("Stereo plug" and "Stereo plug USB"), that have their local_deviceId_dpsId as unique_id
    What is missing now is that I would also expect 3 more Entities, i.e. the sensors for voltage/current, instead of getting them as template sensors.
    What do you think is wrong/improvable in this scenario?

image
image

@postlund
Copy link
Collaborator

One problem is that the "device" you see comes from the switches. What does the information you see even mean here? What if you were to return different firmware versions or models for each switch, then what happens? Or if you add entities with other platforms? Is it still a "SmartSwitch"? I believe the UI will still call it an entity even if it is implemented as a device, but I can be wrong. The upside of having devices is that you can use device automations. I'll look around to see if I can find any examples of what other people have done.

@rospogrigio
Copy link
Owner Author

I have another interesting use case: my cover. It has 1 cover, and 1 switch actually (the backlight).
The way I had thought it was that the device (I mean, the TuyaDevice) was created by the first platform being set up and created in hass.data, and that the second would create it only if it still doesn't exist (after searching in hass.data).
Yes, let's study what other people do because it's important to have clear ideas before proceeding. Maybe the daikin integration can be a nice example (since I own it and I am using it 🤣 ).

@postlund
Copy link
Collaborator

It would not be the responsibility of the platform to set that up, we would have to move that into the component instead (async_setup_entry in __init__.py). We should create and store it there then just fetch it in the platforms.

Yeah, I will try to figure something out. But what we are doing know isn't really correct IMHO at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants