icanos / hassio-plejd

Hass.io add-on for Plejd home automation devices
Apache License 2.0
126 stars 37 forks source link

Warning after HASS upgrade to 2023.8 #279

Closed markus-lassfolk closed 11 months ago

markus-lassfolk commented 1 year ago

Notice showing up:

This stops working in version 2024.2.0. Please address before upgrading

Some MQTT entities have an entity name equal to the device name. This is not expected. The entity name is set to null as a work-a-round to avoid a duplicate name. Please inform the maintainer of the software application that supplies the affected entities to fix this issue.

https://developers.home-assistant.io/blog/2023-057-21-change-naming-mqtt-entities/

faanskit commented 1 year ago

I suspect the resolution to this issue resides in MqttClient.js, getOutputDeviceDiscoveryPayload().

As can be seen below, both name: and device: { name: } picks up its input from device.name, making the entity name equal to the device name as reported by Home Assistant Warning Message.

MqttClient.js

const getOutputDeviceDiscoveryPayload = (
  /** @type {import('./types/DeviceRegistry').OutputDevice} */ device,
) => ({
  name: device.name,
  unique_id: device.uniqueId,
  '~': getBaseTopic(device.uniqueId, device.type),
  state_topic: `~/${TOPIC_TYPES.STATE}`,
  command_topic: `~/${TOPIC_TYPES.COMMAND}`,
  availability_topic: `~/${TOPIC_TYPES.AVAILABILITY}`,
  optimistic: false,
  qos: 1,
  retain: true,
  device: {
    identifiers: `${device.uniqueId}`,
    manufacturer: 'Plejd',
    model: device.typeName,
    name: device.name,
    ...(device.roomName !== undefined ? { suggested_area: device.roomName } : {}),
    sw_version: device.version,
  },
  ...(device.type === MQTT_TYPES.LIGHT ? { brightness: device.dimmable, schema: 'json' } : {}),
});

image

A quick and dirty way to fix this would be to prefix/suffix the device name.

From what I can read (here), it also appears as it is possible to completely remove the device name. image

I'm still not fully understood the model of the Device Registry of Home Assistant and how to map that to Plejd. @SweVictor did reach out to the developers of HA once when there was a discussion on Entity vs. Device before.

The complexity comes with devices like DIM-02 and REL-02, which currently is considered two devices and two entities. With the new and revised model of HA, it appears as the DIM-02/REL-02 should be considered one device with two entities and the MQTT discovery should make use of connection to link multiple entities to a single device. But this is not proven right and likely a complex fix.

@SweVictor, are you still maintaining this integration on behalf of @icanos? As you can see, the integration will stop working from HA 2024.2.0

/Marcus

SweVictor commented 1 year ago

Nice heads up! And regarding "maintaining", maybe that's saying too much... Helping out from time to time 😄

"Stop working" might be a bit harsh, but it sounds like something we should address.

Diving a bit deeper into the documentation, I found this:

The friendly_name state attribute is generated by combining then entity name with the device name as follows:

The entity is not a member of a device: friendly_name = entity.name The entity is a member of a device and entity.name is not None: friendly_name = f"{device.name} {entity.name}" The entity is a member of a device and entity.name is None: friendly_name = f"{device.name}" Entity names should start with a capital letter, the rest of the words are lower case (unless it's a proper noun or a capitalized abbreviation of course).

(from https://developers.home-assistant.io/docs/core/entity/#entity-naming)

So, it sounds like the expectation is that we set entity.name = Null.

So, I actually think we should send (from your example):

name: Null,
unique_id: ...
...
device: {
  name: Taklampor
  ...
}

Thoughts? /Victor

faanskit commented 1 year ago

"Stop working" might be a bit harsh, but it sounds like something we should address.

image

Stop working is actually what Home Assistant says. This message starts to appear with the upgrade to Home Assistant 2023.8.1.

Thoughts? /Victor

I think you are right and I fear this may be one of these breaking changes. Guess someone need to try this in a friendly environment first.

timjackson commented 1 year ago

Without having more knowledge of HA than as a user, I looked at this a bit, and also concluded that entity name should probably be null, using the current method of enumerating devices/entities.

However, I noted that technically the way devices/entities are modelled could possibly be made more "correct" according to the HA model - if I understand correctly (and I might not), today Plejd devices with multiple channels (e.g. DIM-02 or REL-02) are modelled as two separate HA devices with one entity each, whereas they should perhaps technically be one device with two entities - in which case the entity names should probably not be null, but perhaps named according to the devices they control. (e.g. a physical DIM-02 where one dimmer channel might be e.g. "Spotlights" and another "Ceiling light"). However, I'm not sure whether there is much (or anything) to be gained from such an effort, in particular because Plejd already abstracts the physical devices and the control surfaces, so the actual hierarchy of physical devices is not that interesting inside HA. Furthermore, we likely wouldn't have much of a meaningful name for the physical device (other than perhaps the room it's in, e.g. "Dimmer in Kitchen"), so the user experience in HA might well be worse by making this more "correct".

To summarise, as I understand it the current state for modelling a DIM-02 looks something like this:

Device name: Spotlights
- Entity name: Spotlights
Device name: Ceiling light
- Entity name: Ceiling light

and, the proposed easy fix:

Device name: Spotlights
- Entity name: Null
Device name: Ceiling light
- Entity name: Null

Possibly more "correct" but more complicated fix:

Device name: Dimmer in [room name]
- Entity name: Spotlights
- Entity name: Ceiling light
SweVictor commented 1 year ago

However, I noted that technically the way devices/entities are modelled could possibly be made more "correct" according to the HA model ...

Absolutely true. And if we're really ambitious, looking at services in the Plejd app you see a quite complex data model.

Ex: one DIM-01 has

SweVictor commented 1 year ago

Alright - did a quick fix of this in the https://github.com/icanos/hassio-plejd/tree/feature/set-device-name-null

The branch is built on the upcoming 0.10.0 release. Feel free to test at your own risk 😄

I did a very quick sanity check - it does not immediately break my system.

ErikThorsell commented 1 year ago

I did a very quick sanity check - it does not immediately break my system.

Spoken like a true developer!

Thank you for the fix. 👍🏼 If nothing else, I'm sure it's a step in the right direction.

SweVictor commented 1 year ago

This fix is now in the https://github.com/icanos/hassio-plejd/tree/develop branch since the release of 0.10.0.

Planned for the next release.

timjackson commented 1 year ago

[...] if we're really ambitious, looking at services in the Plejd app you see a quite complex data model.

Ex: one DIM-01 has

  • Output
  • Input 1
  • Input 2
  • Rotary input
  • Accessory (showing the RTR-01)

True, although I'm not sure whether there's anything to be gained by trying to model it in that detail, or indeed if it's even possible (IIRC from a separate issue, the movement of a rotary input can't be determined separately from a command to dim the configured device, and similarly is it possible to get data about specific inputs?)

One possibly interesting entity to model (if it's possible) might be BAT-01, although even then I don't know if it's possible to fetch something useful to display in HA (e.g. battery level) from it.

Perhaps the discussion of modelling (vs stopping the HA complaints about entity name) should be moved to a separate issue?

Additionally, from the changelog, 2023.8.4 stops the complaints about entity names.

SweVictor commented 11 months ago

Fixed in 0.10.0