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

Add a new parameter for z2m integration to allow mqtt #109

Closed
xaviml opened this issue Aug 2, 2020 · 3 comments
Closed

Add a new parameter for z2m integration to allow mqtt #109

xaviml opened this issue Aug 2, 2020 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@xaviml
Copy link
Owner

xaviml commented Aug 2, 2020

Feature Request

Is your feature request related to a problem?

We recently added an mqtt integration that works together with zigbee2mqtt actions, however, it assumes that z2m fires a topic like: zigbee2mqtt/<friendly_name>/action or zigbee2mqtt/<friendly_name>/click. Although some people is able to see this topic on z2m logs, some others can not see it and it does not seem to be appearing in the official z2m documentation.

The documentation says that the official topic that will be fired is: zigbee2mqtt/<friendly_name> with a JSON object.

Describe the solution / feature you'd like

The solution for this problem is to add a new parameter for the z2m integration that will allow users to specify the friendly_name and the key that ControllerX will need to look for the action (it is normally action or click).

Describe alternatives you've considered

Additional context

Configuration for this request will look as follow:

example_app:
  module: controllerx
  class: E1810Controller
  integration:
    name: z2m
    listens_to: mqtt # options will be "ha" or "mqtt". Default "ha" as it is now
    action_key: action  # key that ControllerX will be taking for the action. It normally is "action" or "click". Default will be "action"
  controller: my_controller # for the topic zigbee2mqtt/my_controller
  light: light.my_light
@xaviml xaviml added the enhancement New feature or request label Aug 2, 2020
@xaviml xaviml self-assigned this Aug 2, 2020
@Paul-Vdp
Copy link

Paul-Vdp commented Aug 2, 2020

Hi Xavi,
I was a bit surprised to find that you would be enhancing the z2m integration this way. I had assumed you would on the contrary enhance the mqtt integration ...

Of course, it does not matter much, at least when z2m integration 'listening' to mqtt means you would be directly monitoring mqtt topics iso ha states, and therefore cutting an intermediate level in the flow and thus gaining the speed benefits.

On the other hand : would it not be simpler/clearer to just enhance the dedicated mqtt integration with a couple of parameters ? One indicating whether the topic given contains the direct state or a json containing an event key, the second (in the latter case) the 'action_key' as in your new z2m proposition ?

Edit: on second thoughts, I guess you would not even need a parameter to specify the type of the topic content, as it could be rather easily deduced from the actual content, or even by the mere presence of the 'action_key' parameter.

@xaviml
Copy link
Owner Author

xaviml commented Aug 3, 2020

Hi Paul,

I had assumed you would on the contrary enhance the mqtt integration

I try to apply the single responsibility principle. This feature request is required because z2m sends a JSON object through the MQTT topic payload, so it is z2m dependent. If I modify the MQTT integration I will be making the MQTT integration to have z2m functionality and that is not what I want. Although we can use MQTT integration for z2m devices, the only thing we use is a topic and the integration assumes that the topic carries the action. This can be also used by any other DYI device as long as it exposes the action in the payload. This is why the functionality will go to z2m integration.

Of course, it does not matter much, at least when z2m integration 'listening' to mqtt means you would be directly monitoring mqtt topics iso ha states, and therefore cutting an intermediate level in the flow and thus gaining the speed benefits.

Indeed, that is what I aimed with the MQTT integration. However, since I assumed that there was a topic that had the action in the payload, I did not find it necessary to modify the z2m.

On the other hand : would it not be simpler/clearer to just enhance the dedicated mqtt integration with a couple of parameters ? One indicating whether the topic given contains the direct state or a json containing an event key, the second (in the latter case) the 'action_key' as in your new z2m proposition ?

The same reason as before, we will assume that any MQTT topic will need a payload with a JSON object that will have in the root object a key like "action" or "click". But what if it's not a JSON object? What if it is a JSON object but the action_key is 2 levels down from the root? I might implement something similar for the MQTT in the future, but I need to think of the generic case of MQTT, not only z2m. Maybe the MQTT could have a parameter like type indicating the format of the payload (string, json, etc.) and is the type is json then a parameter action_key to indicate where the action is like data.action could imply that it is inside the action key that at the same time is inside the data object. But again, this is another topic, this feature request is a specific solution for z2m. This is why it will be like this and this is why the action_key can be action by default because it is z2m dependent.

Edit: on second thoughts, I guess you would not even need a parameter to specify the type of the topic content, as it could be rather easily deduced from the actual content, or even by the mere presence of the 'action_key' parameter.

This is true, but I prefer to be more explicit on what it is listening otherwise it would be hard to guess where is listening by just looking at the action_key attribute and it will be not readable. However, I agree that it is too verbose. I was thinking now on something like:

example_app:
  module: controllerx
  class: E1810Controller
  integration:
    name: z2m
    listens_to: mqtt # options will be "ha" or "mqtt". Default "ha" as it is now
  controller: action@my_controller # for the topic zigbee2mqtt/my_controller and the key together
  light: light.my_light

It is still readable (in my opinion) and shorter. The downside is that the friendly name can contain any character, included "@" and this could mess up the configuration, although I doubt anyone uses "@" in their friendly names... What do you think?

Cheers,
Xavi M.

xaviml added a commit that referenced this issue Aug 8, 2020
@xaviml
Copy link
Owner Author

xaviml commented Aug 17, 2020

This has been added in ControllerX v3.4.0

@xaviml xaviml closed this as completed Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants