HA event entities way forward

[Koenkk]

Z2M 1.42.0 added support for the Home Assistant event entities. This was done in #24233. The current implementation has some limitations, it doesn't work for all actions atm. The current patterns are supported for now:

zigbee2mqtt/lib/extension/homeassistant.ts

Line 39 in 5b907f2

const ACTION_PATTERNS: string[] = [

My initial thought was to add a new action_data property for this (#24233 (comment)), but this will add a lot of duplication, so not a good option.

Therefore I'm now thinking to move the patterns that are now in homeassistant.ts into the e.action() exposes.

What do you think? @mundschenk-at @Nerivec @Drafteed

[Nerivec]

Whatever way is picked, I think it should follow:

• The less duplication of values/props the better, it needs to be easy to maintain...
• Avoid splitting payloads into several props if the user is going to always recombine them in automation triggers anyway (pointless)
• The "final behavior" should resemble the behavior of regular action (zigbee2mqtt-frontend) enough that it avoids too much confusion on usage (too many patterns...)
• Should have a nil-to-limited footprint when HA extension is disabled (non-HA users)

[mundschenk-at]

I can see the reasoning, but I disagree. The frontend just follows the current MQTT topic and should not be normative. The HA event API works differently and should embrace it. Having the action itself (event_type) and attributes separate also allows for proper localization in the HA frontend.

[mundschenk-at]

The current implementation should work on a basic level for almost all current actions, except for those like 511.344 which somehow adds endpoints to the actual payload string but not the exposes (I have to investigate this, just got notified a few hours ago), in that it passes the current action value through as the HA event_type. However, if we want to have nice even types and attributes in HA, the actions are currently parsed both on the Z2M side to define valid event_type values for the discovery topic and on the HA side to make a JSON value out of a plain string.

What exactly is your proposal? To do the parsing in e.action() and still generate a nice JSON payload (in its own topic or as part of state)? I'd be OK with that, though I think this should only be the codepath to deal with legacy converters and we should have some new API for the converters to specify the different parts without resorting to string parsing.

[Koenkk]

I do not intent do add something additional to the published payloads. My proposal is to remove the regexes out of homeassistant.ts and move them into the action exposes (e.action()). Also I think we should add new exposes for the other action_* values (like action_group)

[mundschenk-at]

Then to be honest I have no clue what you mean by moving the regexes. What are they supposed to do in e.action()? (And I don't see how a separate exposes for action_group could be integrated into the HA event logic.)

[mundschenk-at]

Or do you mean giving each action expose their own parsing regex, the default being something like ^(?:<action>.*)$? That could work, though I really would prefer having a proper JSON payload for HA instead of my fancy value_template. Of course the wildcard actions would have to go.

I still see no way to include action_group if it has its own separate expose.

[Koenkk]

Or do you mean giving each action expose their own parsing regex, the default being something like ^(?:.*)$?

Yes that's what I mean, I don't think that having it hardcoded in homeassistant.ts is future proof. Also sometimes the group is part of the action, e.g. {action: "click_1"}. We could then separate this out with the regex.

I still see no way to include action_group if it has its own separate expose.

Wouldn't that be as simple as adding value_json.action_group to

zigbee2mqtt/lib/extension/homeassistant.ts

Line 2271 in 5b907f2

const value_template =

?

[mundschenk-at]

But the action expose would have to know about the action_group, not sure what we would do with a separate expose? It would only be useful if we wanted to have separate HA entity for it. Each expose results in one (or more) entities, the design does not allow for several exposes to be combined into a single discovery topic (HA entity).

[Drafteed]

To be honest, I would do it like this: сompletely revert #24233 and all followups. Then add a new expose e.event() that would correspond to the new HA entity and gradually add them to existing devices/definitions. Also mark e.action() as deprecated, removing it in 3.x.x. In this case, the event entity will only be available if it is defined in the converter, otherwise users will have to use MQTT triggers until the event entity is added properly.

Regarding #24233, I think this implementation is a bit messy, IMO. All attempts to make workaround are hacky that will cause a lot of problems that will be difficult to solve later.

[Nerivec]

Actions cannot be removed, events are HA-only (would need a complete rework of actions otherwise, a huge breaking change for all setups, frontend, node-red, etc.). Hence the annoyance of duplicating code for converters with the introduction of new props... And it shouldn't impact performance for other users (HA extension disabled).
Also, parsing with regexes has to be simple enough, every time a new level of parsing is introduced, that already depends on a regex parsing, you potentially introduce a LOT of edge-cases that tests will undoubtedly fail to detect (i.e. 🐛 abound). Not to mention the difficulty increase for maintenance.

[Koenkk]

@Drafteed I guess your event proposal is similar to the action_data proposal from the OP?

My initial thought was to add a new action_data property for this (#24233 (comment)), but this will add a lot of duplication, so not a good option.

I think this would be too much work and duplication. The action should also not be removed, it works well for most of the cases (and also not everyone uses HA). Therefore I think it's the best to add the parsing regexes to the e.action() (for those that have more stuff in the action than just a simple single for example) and add exposes for e.g. action_group. Based on that the Home Assistant extension should be able to discover the events.

[mundschenk-at]

@Koenkk I still don't see how adding a separate exposes for action_group is supposed to work with the current discovery framework.

[Koenkk]

@mundschenk-at whenever we encounter an action, we check all the other exposes of that device, in case you find any action_, it's added to the discovery payload.

[mundschenk-at]

I don't think it's very efficient, but OK, it can be done that way.

I still don't see the benefit of not adding a new JSON state key (not a separate topic) and handling the parsing entirely on the TS side? Yes, it will duplicate data slightly, but it would remove duplicate parsing, which is much more error prone. This would also improve compatibility with consumers that don't have the exact same Jinja2 engine as HA.

[Drafteed]

We can also try asking @frenck opinion as the creator of the event component.