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

Home Assistant discovery improvements (including scenes) #19838

Merged
merged 9 commits into from
Nov 26, 2023

Conversation

mundschenk-at
Copy link
Contributor

@mundschenk-at mundschenk-at commented Nov 23, 2023

@mundschenk-at
Copy link
Contributor Author

@Koenkk: I can split the PR if you prefer these to be separate. I've also got some points that still need to be adressed before finalizing the PR:

  • One thing I'm not happy about in the scenes discovery is that I can't set the object_id separate from the unique_id for the scene entities. I'd prefer to have the slugified scene name (as well as the ID) in the object_id but only the scene ID in the unique_id field. Any way to achieve this without adding special logic to discover? Or could we make the name unique for a given group?
  • Are scenes for endpoints a thing? Should I also add support for that? I don't think it is possible to add such scenes via the frontend at least.

@mundschenk-at mundschenk-at force-pushed the homeassistant_discovery branch 2 times, most recently from 5bb6ab0 to cc022aa Compare November 23, 2023 21:44
@mundschenk-at
Copy link
Contributor Author

mundschenk-at commented Nov 23, 2023

I have fixed the tests for the group device info, but I'm not sure I understand Jest well enough to write a meaningful test to add onScenesChanged coverage. At least not today.

@mundschenk-at mundschenk-at changed the title Homeassistant discovery improvements Homeassistant discovery improvements (including scenes) Nov 24, 2023
@Koenkk
Copy link
Owner

Koenkk commented Nov 24, 2023

I'd prefer to have the slugified scene name (as well as the ID) in the object_id but only the scene ID in the unique_id field.

Why do you prefer this? It should also contain the settings.get().mqtt.base_topic

Are scenes for endpoints a thing? Should I also add support for that? I don't think it is possible to add such scenes via the frontend at least.

Scenes are also supported for endpoint, so we should add this

I have fixed the tests for the group device info, but I'm not sure I understand Jest well enough to write a meaningful test to add onScenesChanged coverage. At least not today.

You can emit the event form the eventBus directly, example

@mundschenk-at
Copy link
Contributor Author

I'd prefer to have the slugified scene name (as well as the ID) in the object_id but only the scene ID in the unique_id field.

Why do you prefer this? It should also contain the settings.get().mqtt.base_topic

Regarding the base_topic, you are talking about the unique_id, right? If so, yes, i would not want to change that part. What I am mainly concerned about is the object_id providing the base for HA's entity name. I think that for a given device My Device and a given scene My Scene, the generated entity name scene.my_device_my_scene_3 or (maybe scene.my_device_3_my_scene) is easier for humans to interact with than scene.my_device_3. The latter is also the form used by HA to differentiate duplicate entity names, so it is doubly unfortunate to my mind.

Whereas the scene name ought not to be part of the actual unique_id of the entity because it is not, well, unique.

Are scenes for endpoints a thing? Should I also add support for that? I don't think it is possible to add such scenes via the frontend at least.

Scenes are also supported for endpoint, so we should add this

OK.

I have fixed the tests for the group device info, but I'm not sure I understand Jest well enough to write a meaningful test to add onScenesChanged coverage. At least not today.

You can emit the event form the eventBus directly, example

Thanks, that helps, but my main stumbling block right now seems to be how to set up a state with scenes. Would I have to create them using the MQTT mock?

@mundschenk-at mundschenk-at force-pushed the homeassistant_discovery branch 2 times, most recently from abe6894 to 65f4795 Compare November 24, 2023 22:56
@mundschenk-at mundschenk-at changed the title Homeassistant discovery improvements (including scenes) Home Assistant discovery improvements (including scenes) Nov 25, 2023
test/homeassistant.test.js Outdated Show resolved Hide resolved
lib/util/utils.ts Outdated Show resolved Hide resolved
@Koenkk
Copy link
Owner

Koenkk commented Nov 25, 2023

I think that for a given device My Device and a given scene My Scene, the generated entity name scene.my_device_my_scene_3 or (maybe scene.my_device_3_my_scene) is easier for humans to interact with than scene.my_device_3.

I think we should include the name in the object_id, but to achieve this, we can set the name parameter.

Thanks, that helps, but my main stumbling block right now seems to be how to set up a state with scenes. Would I have to create them using the MQTT mock?

Yes, here is an example on how to create them:

it('Scenes', async () => {

@mundschenk-at
Copy link
Contributor Author

mundschenk-at commented Nov 25, 2023

I think that for a given device My Device and a given scene My Scene, the generated entity name scene.my_device_my_scene_3 or (maybe scene.my_device_3_my_scene) is easier for humans to interact with than scene.my_device_3.

I think we should include the name in the object_id, but to achieve this, we can set the name parameter.

No, we can't. When the object_id parameter is set in a discover payload, name is only used for the Friendly Name in the UI and not for the entity ID (slug).

After thinking about it, the easiest way to achieve this would be to provide an optional override parameter unique_id in DiscoveryEntry that when set is used instead of the object_id in. generating the unique_id payload (i.e. don't change the general logic for unique_id, but allow discovery entries to specify their own string instead of re-using the object_id).

@Koenkk
Copy link
Owner

Koenkk commented Nov 25, 2023

After thinking about it, the easiest way to achieve this would be to provide an optional override parameter unique_id in DiscoveryEntry that when set is used instead of the object_id in. generating the unique_id payload (i.e. don't change the general logic for unique_id, but allow discovery entries to specify their own string instead of re-using the object_id).

Agree!

@mundschenk-at
Copy link
Contributor Author

OK, so it's more complicated than I initially thought. We currently have several different usages of the config.objectID:

  • In homeassistant.ts as a key for this.discovered[].objectIDs (unrelated to HA, but should not change when a scene is renamed or it need to be separately purged in onScenesChanged).
  • As part of the discoveryTopic for HA - this should be unique for the HA entity, but does not have to be the same as object_id key in the actual payload. Indeed, HA now recommends to dispense with the nodeID/objectID part of the discovery topic and just use the actual unique_id from the payload instead. This should probably not change on a rename either.
  • As part of the discovery payload - that's what I'm concerned with above, i.e. the value used to derive the HA entity_id.

@mundschenk-at
Copy link
Contributor Author

I think adding an optional postfix to the payload.object_id but not changing the config.object_id is the less invasive solution. If you like, I can also add a prefix for symmetry with state_topic.

@mundschenk-at mundschenk-at force-pushed the homeassistant_discovery branch 2 times, most recently from 8d400cc to 540bf49 Compare November 25, 2023 21:05
@Koenkk Koenkk merged commit 838629c into Koenkk:dev Nov 26, 2023
11 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Nov 26, 2023

All good, many thanks!

@miani
Copy link

miani commented Nov 29, 2023

Hi @Koenkk,
I am new to HA and zigbee2mqtt and directly run into the question of scene support.
When will this function become available / get released?

@Koenkk
Copy link
Owner

Koenkk commented Nov 30, 2023

Next release = tomorrow!

@jd1900
Copy link
Contributor

jd1900 commented Dec 1, 2023

Thank you @mundschenk-at !
Any documentation about how to use this?

@mundschenk-at
Copy link
Contributor Author

mundschenk-at commented Dec 1, 2023

@jd1900 You get scene entities in HA.

@ukmgranger Not sure why they would appear unavailable for you, availability is tied to the standard Z2M mechanism (i.e. both the bridge topic and the specific device/group topic must be available). Are you on a recent-ish HA version? If so, maybe try reloading the MQTT integration.

@ukmgranger
Copy link

@mundschenk-at Sorry - my bad - I deleted my post just after I wrote it. It was just a full restart of HA that fixed things (should have known).
It's working great from my very limited time playing around with it!

@mundschenk-at
Copy link
Contributor Author

If it happens again (say with future discovery improvements 😅), a reload of the MQTT config entry should normally be enough to get things on track again.

@theRealAJR
Copy link

I don't know if it is related to this feature, but after I updated I can not add scenes via the z2m web interface to a single light any more: Adding a scene triggers error (red pop-up) "Publish 'set' 'scene_store' to 'Licht Esszimmer Mitte' failed: 'Error: Not a group'".
The single light scenes, which existed before the update, can still be used.
For groups adding new scenes works fine.

@mundschenk-at
Copy link
Contributor Author

mundschenk-at commented Dec 1, 2023

That's an error message from zigbee-herdsman-converters, so not related. I think I also noticed during testing of the PR that deleting all scenes did not work from the frontend on dev.

@mundschenk-at mundschenk-at deleted the homeassistant_discovery branch December 1, 2023 21:45
@ukmgranger
Copy link

I'm having a bit of a nightmare with the scenes at the moment! They seem to randomly change their name (discovered scenes) in HA to either add or remove a suffix. So for example I have a scene called 'scene.kitchen_0_kitchen_bright'. This then becomes 'scene.kitchen_0_kitchen_bright_2'. There are other scenes which have the '_2' at the end, which then gets removed. This obviously throws out all my automatons in NR and my many scene buttons on my Dashboard.
Anyone else seeing this?

@mundschenk-at
Copy link
Contributor Author

@ukmgranger I don't see this (but I'm on dev). Do you perhaps still have some scenes configured manually via YAML? HA suffixes entities when an entity of the same name but a different unique_id is already in its database.

@theRealAJR
Copy link

I have also experienced the name changes in scenes and was wondering why may HA automations stopped working.
They names seemed to change when I edited scenes in the same group in the z2m GUI.

@ukmgranger
Copy link

@mundschenk-at No I have removed all other HA created scenes for the kitchen. It's almost like it's discovering the scene from Z2M more than once causing the scene to change names. It's happening on many of my scenes.

@mundschenk-at
Copy link
Contributor Author

Huh, can you check the MQTT info whether there are multiple discovery payloads?

@mundschenk-at
Copy link
Contributor Author

@ukmgranger @theRealAJR Do you remember what you did before those duplicate entities showed up (editing scenes etc.)? I'll try to reproduce the issue, but I need some starting points.

@mundschenk-at
Copy link
Contributor Author

You know, I think this is the same core issue as #12610. I'll add a delay before republishing the scene data there too.

@tebra-jl
Copy link

tebra-jl commented Dec 8, 2023

I have also experienced the name changes. The patch to prevent scene entities from being duplicated is going to stable branch soon?
Thanks

@mundschenk-at
Copy link
Contributor Author

It's not merged into dev yet and and from what I've seen, @Koenkk normally makes releases once a month, so probably early January?

@theRealAJR
Copy link

I played with it again yesterday. Scenes in the same group were renamed when other scenes were added. There are scenes with suffix _2 now i.e. in the same group some scenes have that suffix and some don't.
I also noticed that scene names have changed in groups which I didn't touch at all. I noticed that only some time after I was done adding scenes and hence don't know what caused that - I also added new devices and restarted z2m a few times.

@mundschenk-at
Copy link
Contributor Author

@theRealAJR Are you on the most recent dev commit?

@theRealAJR
Copy link

no, 1.34.0 commit: aae7312

@mundschenk-at
Copy link
Contributor Author

Ah, OK. This has already been fixed via #20097.

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.

7 participants