Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Mqtt generic things and Homie 3.x things #5450

Closed
wants to merge 1 commit into from

Conversation

davidgraeff
Copy link
Contributor

@davidgraeff davidgraeff commented Apr 23, 2018

This is a friendly re-take over of #5101, originally #3876 with changes from @SJKA:

  • incorporate (my...) review feedback (see Mqtt things #3876 (review))
  • update the license headers
  • adaptation to mockito 2
  • adding the binding to the docu structure
  • Eclipse & Karaf features

Abstract

At this very moment there exist no means to actually connect mqtt topics to ESH things. Specialized bindings for all those commercial and non-commercial MQTT products out there makes sense.

But for a start this PR provides a simple generic MQTT topics to thing/channel mapping.

  • Use bridges of the main MQTT bundle.
  • Manually add a thing and an arbitrary number of channels (Text, Number, OnOff, Color, Dimmer) and bind those to MQTT states and/or command topic.
  • Allow to use a transformation pattern to extract a state from a formatted response like JSON. An example would be the pattern JSONPATH:$.device.status.temperature for an incoming message of {device: {status: { temperature: 23.2 }}}.
  • The number as well as dimmer channel can have a configured min, max and step.
  • The OnOff channel takes a configuration for the ON and OFF state. Some people use "1" and "0", some "true" and "false", we support all variants.

Channels

  • TextChannel: String
  • NumberChannel: Number, Rollershutter, Dimmer
  • OnOffChannel: Contact, Switch
  • ColorChannel: Color

Homie 3.x convention

Discovery of Things/Channels is possible by supporting the Homie convention.

  • I'm not too keen to support node instances at that moment in time. The specification is not entirely clear to me in this regard.
  • Device statistics are made available as thing properties.
  • Some attributes like the localip or mac attribute do not make sense in the standard and will not be used or required from peer devices.

Cheers,
David

@htreu
Copy link
Contributor

htreu commented Apr 24, 2018

Your daily Travis report ;-)

[ERROR] Cannot resolve project dependencies:
[ERROR]   Software being installed: org.eclipse.smarthome.binding.mqtt.generic 0.10.0.qualifier
[ERROR]   Missing requirement: org.eclipse.smarthome.binding.mqtt.generic 0.10.0.qualifier requires 'package org.eclipse.smarthome.binding.mqtt.handler 0.0.0' but it could not be found

@davidgraeff
Copy link
Contributor Author

davidgraeff commented Apr 24, 2018

That is expected. The bridges of the mqtt bundle #3839 are used

@davidgraeff davidgraeff changed the title Mqtt things Mqtt generic things and Homie 3.x things Apr 26, 2018
@htreu
Copy link
Contributor

htreu commented Apr 30, 2018

@davidgraeff: #3839 is merged, you may proceed here.

@kaikreuzer
Copy link
Contributor

@davidgraeff I would strongly advice NOT to put homie stuff into the mqtt.generic bundle, but rather create a separate bundle (&PR) mqtt.homie for it.

@kaikreuzer
Copy link
Contributor

Sorry - I just noticed that I confused "homie" with "homee" - so as "homie" is a generic protocol for MQTT IoT integration, it indeed makes sense to support it in here. So forget my previous comment!

@ThomDietrich
Copy link

@davidgraeff
Copy link
Contributor Author

davidgraeff commented Apr 30, 2018

Hm, there's one point that might make this complicated. The Eclipse foundation usually doesn't tend to like any other license then Eclipse or Apache.

The Homie MIT license requires a copyright and license notice. Where should I put it, so that it is visible to the ESH user during runtime, to comply to the used license. Maybe in the bundles "binding.xml"?

@kaikreuzer
Copy link
Contributor

NOTICE file.
But if you include content from Homie, we will also require a CQ for it.

@davidgraeff
Copy link
Contributor Author

Yes, NOTICE file will probably do it. I'm not using anything external except the name Homie.

@kaikreuzer
Copy link
Contributor

I doubt that referencing "homie" requires any kind of license inclusion or CQ.

@sjsf
Copy link
Contributor

sjsf commented May 3, 2018

Could you please have a look at the Travis build log?

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.1.0:compile (default-compile) on project org.eclipse.smarthome.binding.mqtt.generic: Compilation failure: Compilation failure: 
[ERROR] /home/travis/build/eclipse/smarthome/extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/HomieThingHandler.java:[178] 
[ERROR] 	device.startChannels(connection, this).thenRun(() -> {
[ERROR] 	                     ^^^^^^^^^^
[ERROR] Null type mismatch (type annotations): required '@NonNull MqttBrokerConnection' but this expression has type '@Nullable MqttBrokerConnection'

There are several other warnings regarding potential null pointers, but with this one the tooling is confident enough in its own capabilities so that it breaks the build.

@davidgraeff
Copy link
Contributor Author

The full code part is:

        if (connection != null) {
            try {
                device.startChannels(connection, this).thenRun(() -> {
                    logger.trace("Homie device {} fully attached", device.attributes.name);
                });
            } catch (MqttException e) {
                logger.trace("Subscribing to channels failed", e);
            }
        }

But yeah a local variable should do.

@kaikreuzer
Copy link
Contributor

@davidgraeff Just fyi: I would like to schedule the openHAB 2.3 release for end of May with a code freeze on May 21 - it would be cool to have this PR merged soon as it will improve MQTT support in openHAB tremendously! Btw, many thanks for pushing that topic, I am sure many users will be grateful 😎 .

@kaikreuzer
Copy link
Contributor

@davidgraeff I see that your last commit does an "Adapt to future API" - which means that this PR now depends on all your other new PRs, which will make it impossible to get it merged within the next week (which would be necessary for getting it into the openHAB 2.3 release). Note that once this PR is ready, we will also require a CQ which can take a few days for being processed & approved.

What is your opinion on this? Would it be possible to get this PR in a mergeable state on the current API? Imho, the internal refactorings won't break the binding from a user perspective, so it should be fine to do this as a separate step afterwards.

@davidgraeff
Copy link
Contributor Author

@kaikreuzer When you revealed the release planning I was already secretly thinking that's a tough plan. I always wanted a full implementation test with all MQTT components involved and a real non-mocked broker running. I'm happy that I finally had time to implement it. That final piece uncovered some crucial shortcomings in the MqttBrokerConnection API that leads to unpredictable behavior (like silently failing subscriptions).

To still make it for the release, one idea is to just review, discuss and merge the CompletableFuture API for MqttBrokerConnection. That's about 500 LOC and no CQ is required. After the release someone need to tidy the class up though.

@kaikreuzer
Copy link
Contributor

Thanks @davidgraeff. I am not myself in the MQTT code base, so it is difficult for me to assess - I'd leave that to @htreu as he is imho deepest into the code.

one idea is to just review, discuss and merge the CompletableFuture API for MqttBrokerConnection

Which PR is that exactly? And I assume you mean that PR AND the this here, right? (because the MQTT Things are the most important part from a user perspective).

@htreu
Copy link
Contributor

htreu commented May 14, 2018

@davidgraeff lets make a concrete plan for the openHAB release: When I understand you right your preferred option is to strip down the "CompletableFuture API" PR to only have the new MqttBrokerConnection API, w/o the MqttService refactoring and other stuff. This would be then a smaller PR which we might get reviewed and merged on time.
The alternative would be to postpone the new API and get this PR through the door "as is".
I'm open for both variants and and available for review. We should then have all this done during this week.
wdyt?

@htreu
Copy link
Contributor

htreu commented May 15, 2018

With @maggu2810's comment #5535 (comment) its up to you @davidgraeff whether we wait with this MQTT things/homie PR until the new API is approved and merged or geht this one merged first.

Bind Mqtt topics to things. Broker connections are listed as brigdes. A generic thing
can be added. The thing can host one or multiple channels, each linked to a mqtt state / command
topic and an own optional transformation. A channel is one of the following types:

* TextChannel
* NumberChannel
* PercentageChannel
* OnOffChannel

A channel can be configured with a transformation pattern
for an incoming message from a Mqtt state topic. This allows to extract a value from
JSON/XML/etc, as it seems to get popular on some Mqtt topics to encode multiple values into json.

Homie 3.x MQTT convention also supported (except node instances).

The handlers and all value classes are backed by tests.

Also-By: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: David Graeff <david.graeff@web.de>
Signed-off-by: David Gräff <david.graeff@web.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants