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

[mqtt][WIP] Make Homeassistant discovery work for Tasmota #4927

Closed
wants to merge 1 commit into from

Conversation

jochen314
Copy link
Contributor

@jochen314 jochen314 commented Feb 18, 2019

Impovement of Homeassistent discovery.

I have a couple of tasmota devices and I wanted to have the homeassistant discovery work for them.
I found several problems.
Tasmota is using

  • abbreviations in json config
  • ~ replacement in topics
  • jinja templates for value extraction.

All these issues are adressed by this PR.

There were several external libraries needed for the jinja templates. To my knowledge, their licences are OK to be used.

There were also several features added to the different HA component types. They should be feature complete, but

  • light is missing channels.
  • alarm control pannel: should we allow disarming without checking the pin code?
  • the availablity_topic is not used for any component

The PR groups HA components by the device (substructure in the config), they are in.
Therefore there is a thing for each device.
Each HA component is a group having one or more channels,
For this a dynamic thing type provider is implemented.

@jochen314 jochen314 requested review from davidgraeff and a team as code owners February 18, 2019 22:04
@davidgraeff
Copy link
Member

davidgraeff commented Feb 18, 2019

Thanks a lot. I had a brief look and I suggest to split your PR for effectively a faster merge.

  • You are breaking configuration compatibility and that has to be discussed and documented. That process can take some time and I want that to happen only for a small part of the PR.
  • You are touching multiple bundles.
  • I need some time to verify that tasmota still complies to the HA discovery documents. If they are not (and HA is also not), we first need to fix the HA documentation as that should be the reference for this implementation (so that no fellow developer is wondering what's going on).

Therefore I suggest that you:

  • split the transformation out of this PR. I see no problems with the license. It is Apache 2, very compatible to Eclipse 2.
  • That you add HA fixes and additions in small portions (about 300 lines max). Leaving the "abbreviations" and "~ replacement" to the very last.

That way we can get your 3k lines PR merged within a few days.

Cheers

@wborn wborn added the work in progress A PR that is not yet ready to be merged label Feb 19, 2019
@wborn wborn added the mqtt label Feb 20, 2019
@wborn wborn changed the title [MQTT][WIP] Make Homeassistant discovery work for Tasmota [mqtt][WIP] Make Homeassistant discovery work for Tasmota Feb 20, 2019
@davidgraeff davidgraeff added awaiting other PR Depends on another PR and removed awaiting other PR Depends on another PR labels Feb 22, 2019
@davidgraeff
Copy link
Member

davidgraeff commented Feb 23, 2019

Now you can flood me with small to medium changes to the MQTT HA subsystem, preferably first structural changes that you require, then changes to each HA MQTT component as single PRs.

Thanks again for the effort and hopefully OH 2.5 will have a tremendous MQTT HA discovery support.

@jochen314
Copy link
Contributor Author

@davidgraeff
I have a question: I have working solutions to support the abbreviations for the home assistant configs.

  1. Add @SerializedName to each and every config member
  2. Write a Gson TypeAdapterFactory, which will translate the incoming names.

Both have their pros and cons.

  1. is easier to understand for any future maintainer of the code, but the config classes are to easy to read because of annotations blocking the view to the important stuff. E.g.
        @SerializedName(value = "brightness_scale", alternate = "bri_scl")
        protected int brightness_scale = 255;
        @SerializedName(value = "brightness_state_topic", alternate = "bri_stat_t")
        protected @Nullable String brightness_state_topic;
        @SerializedName(value = "brightness_command_topic", alternate = "bri_cmd_t")
        protected @Nullable String brightness_command_topic;
        @SerializedName(value = "brightness_value_template", alternate = "bri_val_tpl")
        protected @Nullable String brightness_value_template;
  1. keeps the translation in one place. We only need to specify the needed translation in one file. Using some simple reflection one could also to the ~ replacement in that adaptor.
    But the code needs to derive/wrap a class, which is not coded to be used that way (JsonReader)

  2. is used in this PR
    For 2, you can have a look at
    https://github.com/jochen314/openhab2-addons/blob/TypeAdapterFactory/addons/binding/org.openhab.binding.mqtt.generic/src/main/java/org/openhab/binding/mqtt/generic/internal/convention/homeassistant/HAConfigTypeAdapterFactory.java
    in bac0bbd

Which one to preceed?

@davidgraeff
Copy link
Member

I tend to option 2, but I'll make up my mind tomorrow.

@davidgraeff
Copy link
Member

I'm pretty sure now that I prefer option 2. Abbreviations are one feature, that IMO should be kept in one place (separation of concerns).

@jochen314
Copy link
Contributor Author

I have been looking at the homae assistant implementation to cross check, what we are doing.
I will add anothe PR soon, because HA uses a default state topic, which is <base>/<component>/[<node_id>/]<object_id>/state

But we should talk about the node_id: I am not sure, what the intended way to use this is.
Can there by 2 configs with the same object_id but with different node_id?
Currently the code basically ignores the node_id. It is only used to put it into the group ID. And there it causes problems when we try to split object_id and node_id again, because the ids may contain _. SO there is no save way to split them. We would have to encode/escape every '_'....

If we want to keep the node_id then shouldn't it be part of the HandlerConfiguration?

In general, what is the mapping:
..../config maps to a home assistant 'entity', which maps to an openhab 'channelGroup' (AbstractComponent)
An openhab 'thing' is an 'object_id'

I would like to see an openhab 'thing' that corresponds to a home assistant 'device'. That is probably a new openab mqtt thing type to keep the compability. That thing would need to be configured with a list of mqtt topics (basically base topic, node_id and object_id). Each of which will create a channelGroup.
Each channelGroup would have a single topic configured as well as the componentConfigurationJson.
If we put that into the config, we do not need to encode anything in the groupId. It could be derived from the componentConfigurationJson.unique_id or the name as fallback, or - as last resort - a hash over the topic. But maybe the object_id is unique?

Does that make any sense?

Is there a way to enter a multiple strings value in the configuration parameters?
Just declaring a multi value paramter did not work

@davidgraeff
Copy link
Member

That is probably a new openab mqtt thing type to keep the compability.

Please don't care about compatibility. The HomeAssistant support was implemented by someone (me) who has never used HA and tried to map HA specific entities like components, objects and nodes to openHAB jargon. If you think, that a different design makes more sense, I will support that.

Is there a way to enter a multiple strings value in the configuration parameters?
Just declaring a multi value paramter did not work

That is supported via "multiple". If multiple is set on a configuration description parameter, it will be represented as a List internally and marshaled to a comma separated string for string representation.
Paper UI does not have superb support unfortunately, it will just render a normal text input.

But we should talk about the node_id: I am not sure, what the intended way to use this is.
Can there by 2 configs with the same object_id but with different node_id?

node_id seem to be rarely used. I thought it is a good idea to just use it as a suffix to object_id. As far as I understood, it is like an instance of an object, so yes, multiple same object ids with different node ids.

We would have to encode/escape every '_'....

Nothing wrong with that.

I would like to see an openhab 'thing' that corresponds to a home assistant 'device'.

That sentence and paragraph puzzled me. HA does not have the notion of a "device" afaik. They have objects, nodes, components. I think you are overestimating node_ids importance. We can as well just disallow all node_ids that contain an underbar, if you are worried of escaping. External projects will soon stop to use those characters if a major implementation like OH is not supporting them, I guess.

@jochen314
Copy link
Contributor Author

I was looking into this: https://developers.home-assistant.io/docs/en/device_registry_index.html
HA has the notion of a device and entities belong to devices.

From that it feel natural to me, that a OH thing is such device. So my Sonoff TH (with tasmota) with a switch and 2 sensors (temperature and humidity) would be respresented as one thing.

So a thing would represent multiple <node_id>/<object_id> instances.

BTW: compability was an issue you brought up in https://github.com/openhab/openhab2-addons/pull/4927#issuecomment-464905084

I think I will go forward to remove the explicit node_id and implicitly allow object_id to be <node_id>/<object_id>. And remove any explicit values from the groupId and put those values into the group configuration.

@davidgraeff
Copy link
Member

I think I will go forward to remove the explicit node_id and implicitly allow object_id to be <node_id>/<object_id>. And remove any explicit values from the groupId and put those values into the group configuration.

Sounds right.

From that it feel natural to me, that a OH thing is such device. So my Sonoff TH (with tasmota) with a switch and 2 sensors (temperature and humidity) would be respresented as one thing.

I agree.

@davidgraeff
Copy link
Member

@jochen314 Could you describe what is still left to do from this PR after the merge of #4990? Thanks

@jochen314
Copy link
Contributor Author

jochen314 commented Mar 18, 2019 via email

@davidgraeff
Copy link
Member

davidgraeff commented Mar 18, 2019

The new build system will take some time to get used to, so I like to avoid the migration until you have finished your PR. There are some caveats like a broken auto re-build in Eclipse. Unit tests would work though.

I have written a small howto in the migration issue: https://github.com/openhab/openhab2-addons/issues/5005. If you think that this will also work for you, then I'll do the migration, otherwise I'll just wait, that's fine as well.

@davidgraeff davidgraeff added this to the 2.5 milestone Mar 21, 2019
@jochen314
Copy link
Contributor Author

I do not plan to stop contributing after these topics.

  • I thing we need to consider the availability_topic.
  • We should also think about the optimistic mode(value.update should not change the state in non optimistic mode)
  • ...

I think, after #5156 we have a good point to think about the migration.
I am not dependent on the auto-rebuild in Eclipse. I prefere to trigger rebuilding explicitly, so that would not be a problem.

@davidgraeff
Copy link
Member

Alright I merge the other PR as soon as travis is satisfied and do the migration

@davidgraeff
Copy link
Member

I have created #5240.
That will change the buildsystem, but also the package structure a bit. You basically have your own "homeassistant" bundle now. In parallel I will work on the Homie part next month.

@davidgraeff davidgraeff added stale As soon as a PR is marked stale, it can be removed 6 months later. and removed discussion work in progress A PR that is not yet ready to be merged labels Jul 13, 2019
@davidgraeff davidgraeff removed this from the 2.5 milestone Jul 13, 2019
@J-N-K
Copy link
Member

J-N-K commented Aug 15, 2019

@jochen314 What is the state of this PR? I see a lot of merge
Conflicts.

@jochen314
Copy link
Contributor Author

I think, the most important parts of this have gone to other PRs by now (the last one #5978 ).
I close this now

@jochen314 jochen314 closed this Aug 25, 2019
@jochen314 jochen314 deleted the improveHAdiscovery branch October 18, 2020 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale As soon as a PR is marked stale, it can be removed 6 months later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants