skodaconnect / homeassistant-myskoda

Homeassistant integration for MySkoda.
96 stars 15 forks source link

Add VIN number and/or integration name to ID of sensors #57

Closed arjanvroege closed 1 month ago

arjanvroege commented 2 months ago

Currently all sensors have no link to the device and/or the integration. Example is that the milage sensor is sensor.milage and door sensor is binary_sensor.doors. It would be better if the sensors get a link to the skodaconnect integration or to the device (car) itself. So I would suggest one of these sensor names:

WebSpider commented 2 months ago

The sensors have a link to the device and the device has a link to the integration, however it indeed is not in the name. This hierarchy for instance is displayed when you go to Settings > Integrations > MySkoda.

I think what you mean here is change the default name of the sensor to include the VIN, am I understanding that correctly?

arjanvroege commented 2 months ago

I'm trying to clarify it with some screenshots. Take for example the last updated sensor. The sensor within the integration has the following entity_id: image So if I search for updated I find the sensor.last_updated as entity ID but I'm not sure if this is the one for the SKODA integration.

Looking to other integrations they have added some identification in relation to the device in the entity_id: image

WebSpider commented 2 months ago

Thanks. All clear! :+1:

WebSpider commented 1 month ago

Small update: In order to make this possible, we will have to rewrite some essential parts of the integration as the for HA important unique_id is calculated off of this.

WebSpider commented 1 month ago

I had a look at a few other integrations:

1) SkodaConnect calculates unique_id from vin, component (which is an entity type such as switch or lock) and attribute, which is similar to the one we're using now (last_updated). So a unique_id looks something like <vin>.sensor.last_updated and the Entity ID is sensor.sensor.last_updated 2) BMW calculates unique_id from vin and key. key is a lower case value similar to the one we're using now (last_updated). So a unique_id looks something like <vin>.last_updated and the Entity ID is last_updated 3) tesla_fleet calculates unique_id from vin and key. key is a lower case value similar to the one we're using now (last_updated). So a unique_id looks something like <vin>.last_updated and the Entity ID is last_updated

The key for all these example vehicle integrations does not contain the VIN. The end-user behaviour for BMW and Tesla is the same as we have now. This would mean that when you have multiple cars, the second car would get an Entity ID of sensor.last_updated_2

We could use the VIN as part of the Entity ID tho. The Entity ID that the user sees, is the key. This would result in the unique_id, that the average HA user does not see, to look like <VIN>_<VIN>_device_tracker. I personally have no objections of having the in the unique_id twice The Entity ID for the end-user would look like <VIN>_last_updated.

Any thoughts @dvx76 @Prior99 ?

dvx76 commented 1 month ago

With the old integration I never found the VIN being part of the entity name (is that the same as the unique_id ?) to be particularly appealing. So I'm definitely not in favor of having the VIN in there.

Just adding an incrementing number to entity names for additional cars seems even worse though.

From a UX point of view what I think would be ideal is to allow the user to use an alias/nickname for each vehicle the first time, and then use that. So that you end up with, for e.g. enyaq.sensor.last_updated and suberb.sensor.last_updated.

But that's a bit impractical since it requires additional user interaction.

The next best thing might be to use the name or model from the garage or info endpoint response?

dvx76 commented 1 month ago

Ok so our unique_id is fine, right? We already have self._attr_unique_id = f"{vin}_{self.entity_description.key}" on the base MySkodaEntity class.

The problem is with the entity id, which I think is the same as the key, (and name, maybe).

I still like my idea above where binary_sensor.charger_connected would become binary_sensor.enyaq_charger_connected.

This works, but it would require defining an __init__() in every individual entity class. It will also result in the unique_id changing (since it's based on the entity_description.key. Ideally I guess the unique_id would remain the same. Feels like there must be a better way to do this ...

from slugify import slugify

class ChargerConnected(AirConditioningBinarySensor):
    """Detects if the charger is connected to the car."""

    def __init__(
        self,
        coordinator,
        vin,
    ):
        vehicle_name = coordinator.data.vehicle.info.name
        vehicle_name_slug = slugify(vehicle_name, separator="_")
        self.entity_description = BinarySensorEntityDescription(
            key=f"{vehicle_name_slug}_charger_connected",
            name=f"{vehicle_name} Charger Connected",
            device_class=BinarySensorDeviceClass.PLUG,
            translation_key="charger_connected",
        )
        super().__init__(coordinator, vin)
dvx76 commented 1 month ago

The entity names and IDs will include the device name, which is the vehicle title (e.g. Skoda Enyaq), not the VIN. The VIN is still used in the entity unique_ids but is impractical in entity names and ids. Using the device name is also the standard approach in HA.

Example:

Before:

entitiy_id = sensor.charger_connected
name = Charger Connected

After:

entitiy_id = sensor.skoda_enyaq_charger_connected
name = Skoda Enyaq Charger Connected

To get the new names and IDs users will need to delete their existing MySkoda device(s) (Hubs) and add them again.