-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Initial contribution of Homie Binding #1741
Conversation
Hey @Kwave, the README is missing the tables for channels and the "full example". These would be needed for manual configuration. |
@@ -0,0 +1,9 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file from the changeset.
I think we should have them in global .gitignore
file, but for now
the best thing we can do is to create an ./git/info/exclude
file with the following entries:
.classpath
.project
/target/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with you. The classpath is necessary especially for projects that are using additional libraries. If you remove it you will have to recreate it each time you add this project to an Eclipse workspace.
@@ -0,0 +1,39 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file from the changeset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is necessary too when this project is added to an Eclipse workspace. If you look into the existing projects, you will see that all contain a .project and .classpath.
@@ -0,0 +1,2 @@ | |||
eclipse.preferences.version=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if whole .settings
folder should be even committed here :)
</buildCommand> | ||
</buildSpec> | ||
<natures> | ||
<nature>org.eclipse.m2e.core.maven2Nature</nature> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this nature.
This binding requires one MQTT broker installed or it has one embedded broker? |
The binding connects to an existing MQTT broker (in the binding configuration you need to provide the hostname and port where the broker is running), so you need to install one (Mosquitto, for example) previously. |
Hey guys, any news about this binding? When will this pull be closed? |
Opening and closing to trigger a new build |
I heard if you put "[WIP]" in the title it will be built automatically. |
It should always do so, [WIP] is just for humans to know that we do not yet have to review :-) |
Some errors are reported by the static code analysis:
I also started reviewing but that might take a while :-) |
Signed-off-by: Michael Kolb <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some very early comments, you are still in line for a full review, put these have been pending for so long that I can better send them out now :-)
</parameter> | ||
|
||
<parameter name="basetopic" type="text" required="true"> | ||
<label>MQTT base topic</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting looks incorrect. XML files should be indented using tabs according to our code formatter. Can you please reformat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -0,0 +1,11 @@ | |||
# binding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file since you did not change anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
Bundle-Vendor: openHAB | ||
Bundle-Version: 2.0.0.qualifier | ||
Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
Bundle-ClassPath: .,lib/org.eclipse.paho.client.mqttv3-1.1.0.jar,lib/commons-lang3-3.5.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the build in commons lang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the mqtt io.transport bundle from https://github.com/eclipse/smarthome/tree/master/bundles/io/org.eclipse.smarthome.io.transport.mqtt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- As far as I know, there's only commons.lang build in, and not commons.lang3.
- This would mean a complete reimplementation of the connection class and its architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I opened an issue for that, but are you using anything specific in this case? Upgrading commons.lang to commons.lang3 eclipse-archived/smarthome#3522
- I did not check the exact impact but the idea of the transport bundle is that not all bindings have to duplicate the transport handling and drag along there own jar's. if you require changes in the transport bundle you can of course discuss/suggest them and create a PR
Bundle-Name: Homie Binding | ||
Bundle-SymbolicName: org.openhab.binding.homie;singleton:=true | ||
Bundle-Vendor: openHAB | ||
Bundle-Version: 2.0.0.qualifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version bump to 2.1.0-SNAPSHOT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
Bundle-ClassPath: .,lib/org.eclipse.paho.client.mqttv3-1.1.0.jar,lib/commons-lang3-3.5.jar | ||
Import-Package: | ||
com.google.common.collect;version="10.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not enforce versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
org.eclipse.smarthome.core.types, | ||
org.openhab.binding.homie, | ||
org.openhab.binding.homie.handler, | ||
org.osgi.framework;version="1.8.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
<?xml version="1.0" encoding="UTF-8"?> | ||
<!-- | ||
|
||
Copyright (c) 2014-2016 by the respective copyright holders. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this file has a missing or invalid copyright header, can you fix all of them by running
mvn license:format
Please commit only the ones for your binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
If you want openHAB to render your nodes properly, you have to provide the following topics. | ||
|
||
| Property | Required | Message Format | Description | Example (Setable heater temperature) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would make me happy by formatting the tables in the MD file as well :-D
http://www.tablesgenerator.com/markdown_tables
Select 'File' > 'Past table data ...' and past the unformatted mark down table, you can then copy paste the formatted table 👍
### Topic `value` (inbound) | ||
The inbound messages (sent from a Homie device to the binding via the `value` topic) are automatically mapped to corresponding ESH command types, depending on the `itemtype` you have choosen. Auto mapping supports the following values: | ||
|
||
| inbound message | ESH command type | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would make me happy by formatting the tables in the MD file as well :-D
http://www.tablesgenerator.com/markdown_tables
Select 'File' > 'Past table data ...' and past the unformatted mark down table, you can then copy paste the formatted table 👍
<dependencies> | ||
<dependency> | ||
<groupId>org.eclipse.paho</groupId> | ||
<artifactId>mqtt-client</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels double besides including it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, removed it.
Signed-off-by: Michael Kolb <[email protected]>
I cannot get it working. My Homie-Node is discovered, but just with "advanced"-Channels like online, name, localip, ... I use homie-python on a raspberry. My script and config is here: https://gist.github.com/sja/0ce0cc5fa4dc45483c29c79a08004269 |
Ping! Any idea where I'm wrong? |
Hi @sja! The problem you are seeing is that the plugin developed by kwave is only compatible with the second version of the Homie specification (Homie 2.0: https://github.com/marvinroger/homie), whereas the python-homie library hasn't been updated yet and it's still using the 1.0 convention (https://github.com/jalmeroth/homie-python/projects). Did you have a look at the log files to see what's not working correctly? Good luck and many thanks to @Kwave for developing the plugin, looking forward to seeing it officially merged. Best regards, Aitor |
@bodiroga Thanks for your reply. I'm confused: The title of jalmeroth's repo is 'A Python-implementation of the homie v2 convention.' You say, that is wrong? |
Hi again @sja! You are right about the repo's title, so I'm also confused about the Homie implementation used in the project. I thought that @jalmeroth was using the first implementation after seeing this issue and this project:
What about the openhab log files? Anything there? |
Hello, @bodiroga thanks for mentioning me. You're right it's a bit confusing as there's only one repo title and it was already updated. Actually, the master branch adheres the Homie v1 convention whereas the dev branch has the v2 already. The dev branch will become the master once homie-esp8266 is out of beta. As I don't expect to much changes, it is probably safe to use the dev branch already. If you find any bugs or have a feature request, please feel free to open an issue. I hope this helps. |
Hi @bodiroga and @jalmeroth , thanks for your reply. I just saw it too late. I already submitted a PR for my v2 approach ... It was a good thing to get warm with python. :-/ I will have a look at the dev branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally found the time to do the rest of the review, thanks again for all your work already.
OSGI-INF/,\ | ||
ESH-INF/,\ | ||
lib/org.eclipse.paho.client.mqttv3-1.1.0.jar,\ | ||
lib/commons-lang3-3.5.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please rewrite your code to depend on (I known how old it is) version 2.6 which is provided by the framework.
.,\ | ||
OSGI-INF/,\ | ||
ESH-INF/,\ | ||
lib/org.eclipse.paho.client.mqttv3-1.1.0.jar,\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned before:
Please use the mqtt io.transport bundle from https://github.com/eclipse/smarthome/tree/master/bundles/io/org.eclipse.smarthome.io.transport.mqtt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also be interested in eclipse-archived/smarthome#3695
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, do you have an example of how to use this service? I'm having troubles creating a binding that relies on this service and i can't seem to find it used in any other OH2 binding, nor can I find docs on how to use it.
Please see https://www.eclipse.org/forums/index.php/t/1088038/ with my experience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aisebastian This service is not used in OH2, because there are no Mqtt related bindings ported to OH2. Did you see my response to your question in the eclipse forum? Look at the PRs of the eclipse smarthome repository and you will find some API consumers :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Yes, I managed to achieve my tasks, Thanks!
|
||
All nodes that do not use the `ESH:` prefix in their `$type` topic will be made available as plain string items. They will be not writable, no matter if the `:settable` attribute is present or not. | ||
|
||
## Manual configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this paragraph deliberately empty? Can you work on that?
import org.eclipse.smarthome.core.thing.type.ChannelTypeProvider; | ||
import org.eclipse.smarthome.core.thing.type.ChannelTypeUID; | ||
|
||
public interface HomieChannelTypeProvider extends ChannelTypeProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an author tag and initial contribution and (at least minimal) information about the purpose / goal of this class.
*/ | ||
public class HomieDeviceHandler extends BaseThingHandler implements IMqttMessageListener { | ||
|
||
private abstract class NodeHandler implements IMqttMessageListener { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add authors / docblocks to all classes including inner classes.
public class NodePropertiesListAnnouncementParser { | ||
|
||
protected final static String MATCHGROUP_PROPERTYNAME_NAME = "propname"; | ||
protected final static String MATCHGROUP_SETTABLE_NAME = "settable"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean, can it have a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the names of regex pattern match groups. I'll add a comment.
protected final static String MATCHGROUP_INTERNAL_SUBPROPERTY_NAME = "intsubproperty"; | ||
protected final static String MATCHGROUP_PROPERTY_NAME = "property"; | ||
protected final static String MATCHGROUP_NODEID_NAME = "nodeid"; | ||
protected final static String MATCHGROUP_DEVICEID_NAME = "deviceid"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho it should be DEVICE_ID instead can you agree? same holds for NODE_ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware that this violates the common java constant naming pattern, but for a good reason:
Currently it is clear that the ID is related to the device. If we change it to MATCHGROUP_DEVICE_ID_NAME: Is it now a Device-Id or a Device with an ID-Name?
public HomieTopic parse(String topic) throws ParseException { | ||
Matcher m = pattern.matcher(topic); | ||
if (!m.matches()) { | ||
throw new ParseException("Topic '" + topic + "' is does not complie to homie specification 2.0.0", 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this message can be optimized:
"Topic ... does not comply to homie specification 2.0.0"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -0,0 +1,75 @@ | |||
var basetopic="homie/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always hard to decide whether this should be part of the repo and in what form, I'm doubting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes into a separate test bundle, Kai mentioned that when I had my tests included in the Yamaha binding PR.
.,\ | ||
OSGI-INF/,\ | ||
ESH-INF/,\ | ||
lib/org.eclipse.paho.client.mqttv3-1.1.0.jar,\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an about.html and make sure it is referred to from this file.
<description>MQTT Broker URL in the format "tcp://myBrokerHost:1883"</description> | ||
<default>tcp://broker:1883</default> | ||
</parameter> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be awesome to use the eclipse smarthome io.transport.mqtt bundle and the MqttService instead
Bundle-Vendor: openHAB | ||
Bundle-Version: 2.1.0.qualifier | ||
Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
Bundle-ClassPath: .,lib/org.eclipse.paho.client.mqttv3-1.1.0.jar,lib/commons-lang3-3.5.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be awesome to use the eclipse smarthome io.transport.mqtt bundle and the MqttService instead
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class MqttConnection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need more functionality in the smarthome io.transport.mqtt bundle and the MqttBrokerConnection, let me know.
@@ -0,0 +1,50 @@ | |||
# Homie Binding | |||
|
|||
This is the binding for devices that complie with the [Homie MQTT Convention](https://github.com/marvinroger/homie). This binding allows you to integrate all devices, as long as they complie with the specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling: comply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
## Additional convention | ||
|
||
If you want openHAB to render your nodes properly, you have to provide the following topics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a good idea. I've seen that you talked to the homie author already, but there is hopefully a way to solve it with the new 2.0 convention instead of adding something on top that only works for OH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Check homieiot/convention#12 (comment) for an ongoing discussion and add whatever might be missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To implement such a binding, it is essential to define additional conventions. There is no way to solve this without specifying additional stuff, or changing the convention itself. The later one is more of a long term process, that's why the binding uses the current convention and defines additional things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought Homie 2.0 is not released yet and still being discussed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. My feeling is, that Marvin is currently preoccupied with other stuff. However the changes discussed in the issue are more or less settled and @Kwave 's PR goes a long way in making them final. homieiot/convention#27
According to GitHub there are some issues not yet addressed, can you make sure that you covered all of them or respond to the comment and tell me why I might be wrong 🙂 |
I think it would be alright to merge this binding with the internal MqttBrokerConnection solution (because it works) and remove the bundled jars in a follow up PR and before OH2.2 is released. A contributing binding developer cannot be responsible to adapt a PR all the time when the framework changes (like this time with the Mqtt transport). |
@kaikreuzer wdyt? |
Afaics, the way the binding is to be configured will change when it starts using the ESH MQTT infrastructure. So imho it does not make much sense to introduce it now and have a breaking change just a few weeks after. |
I have no idea how busy @davidgraeff is but maybe you can collaborate on this with @Kwave it would be great to have this as a first binding based on the MQTT infrastructure. |
Sure, but there is one MQTT PR that needs to be merged first. |
any news? |
@Kwave disappeared. Someone needs to take over this PR I guess |
I have some homie nodes running in my home automation. As the mapping of all topics is very annoying, it would be great to have this binding running on openhab. I want to help but I'm not really sure what is the current state. Can someone please give a short summary, what is missing? What is the state of the referenced Mqtt pr? |
@kaikreuzer in retrospect I believe it was a mistake to delay this binding in August. Potential breaking changes in a Binding are imho better than not having it to begin with. Besides, the Homie ought to change as the Homie is still evolving... |
@ThomDietrich Can you or someone else help here? If the homie related classes are separated from this PR, I would create a new PR, starting fresh with a new version of an addon, where I integrate those classes. But I don't have time to do everything here unfortunately. |
@davidgraeff imho if you want to help it would be best to continue with the new MQTT Binding (and the systems around it). After that I'd hope it's possible to introduce two-three MQTT conventions, either as individual bindings or right into the MQTT binding. I wonder if the latter would be cleaner and easier to maintain. That is totally up to the developer, hopefully you ;) I am not a Binding developer myself and have only little experience in the area. Besides that I apreoccupied with my thesis the next couple of months and will not be able to help :-/ |
@davidgraeff I agree with @ThomDietrich: The best way to drive this forward is to get the ESH MQTT PRs merged. Afaik, there shouldn't be any blocking issues on them anymore, right? |
Are all these merged by now? Is there anything we can or should do to unblock this PR? |
Hi @martinvw! @davidgraeff also has a PR to support Homie 3.0 in ESH (eclipse-archived/smarthome#5450), so IMO this PR can be closed 😉 Many thanks for your work! |
That sounds awesome, thanks for the heads-up and @gorootde thanks for your great effort. |
…enhab#1741) Signed-off-by: Flole <[email protected]>
Inital Contribution: Binding for Homie Devices