MQTT Generic Binding #5101

sjsf · 2018-02-16T13:21:44Z

This is a rebased version of #3876 with commits on top which

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

For details & discussion about the binding please see #3876.

Also-by: David Gräff david.graeff@web.de
Also-by: Henning Treu henning.treu@telekom.de
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

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. The handlers and all value classes are backed by tests. Signed-off-by: David Gräff <david.graeff@web.de>

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

htreu

I did a quick review, no checkout but reading the code online. See inline comments.

htreu · 2018-02-16T13:46:28Z

...org/eclipse/smarthome/binding/mqttgeneric/handler/ThingHandlerTransformationPatternTest.java

+    private Thing thing;
+
+    @Mock
+    MqttBrokerConnectionHandler bridgeHandler;


htreu · 2018-02-16T13:46:32Z

...org/eclipse/smarthome/binding/mqttgeneric/handler/ThingHandlerTransformationPatternTest.java

+    MqttBrokerConnectionHandler bridgeHandler;
+
+    @Mock
+    MqttBrokerConnection connection;


htreu · 2018-02-16T13:46:53Z

...org/eclipse/smarthome/binding/mqttgeneric/handler/ThingHandlerTransformationPatternTest.java

+    @Mock
+    MqttBrokerConnection connection;
+
+    MqttThingHandler subject;


private & should be named thingHandler for better readability.

htreu · 2018-02-16T13:48:00Z

...org/eclipse/smarthome/binding/mqttgeneric/handler/ThingHandlerTransformationPatternTest.java

+    // JSonpathservice implementation. Unfortunately, we can't use the real class, because
+    // it is not exported. An OSGI test is to heavy and unnecessary to just have those few lines
+    // of code for a jsonPath transformation.
+    TransformationService jsonPathService = (jsonPathExpression, source) -> {


Why does this implementation exist at all? It could be a simple mock. Otherwise only JsonPath#read would silently be tested here, too.

No idea - would think so too.

htreu · 2018-02-16T13:48:37Z

...org/eclipse/smarthome/binding/mqttgeneric/handler/ThingHandlerTransformationPatternTest.java

+    @Test
+    public void processMessageWithJSONPath() {
+        subject.initialize();
+        ChannelConfig c = subject.channelDataByChannelUID.get(textChannelUID);


would be nicer to read if c would be channelConfig.

htreu · 2018-02-16T15:08:33Z

...mqttgeneric/src/main/java/org/eclipse/smarthome/binding/mqttgeneric/internal/OnOffValue.java

+ * @author David Graeff - Initial contribution
+ */
+public class OnOffValue implements AbstractMqttThingValue {
+    OnOffType boolValue;


htreu · 2018-02-16T15:08:37Z

...mqttgeneric/src/main/java/org/eclipse/smarthome/binding/mqttgeneric/internal/OnOffValue.java

+public class OnOffValue implements AbstractMqttThingValue {
+    OnOffType boolValue;
+    boolean isInversedOnOff = false;
+    String onValue;


htreu · 2018-02-16T15:08:44Z

...mqttgeneric/src/main/java/org/eclipse/smarthome/binding/mqttgeneric/internal/OnOffValue.java

+    OnOffType boolValue;
+    boolean isInversedOnOff = false;
+    String onValue;
+    String offValue;


htreu · 2018-02-16T15:09:01Z

....mqttgeneric/src/main/java/org/eclipse/smarthome/binding/mqttgeneric/internal/TextValue.java

+ * @author David Graeff - Initial contribution
+ */
+public class TextValue implements AbstractMqttThingValue {
+    StringType strValue;


htreu · 2018-02-16T15:09:37Z

...rg/eclipse/smarthome/binding/mqttgeneric/internal/discovery/MqttServiceDiscoveryService.java

+@Component(service = DiscoveryService.class, immediate = true, name = "MqttServiceDiscoveryService")
+public class MqttServiceDiscoveryService extends AbstractDiscoveryService implements MqttBrokersObserver {
+    private final Logger logger = LoggerFactory.getLogger(MqttServiceDiscoveryService.class);
+    MqttService mqttService;


* Review comments Signed-off-by: Henning Treu <henning.treu@telekom.de>

maggu2810 · 2018-02-20T14:54:45Z

...rg/eclipse/smarthome/binding/mqttgeneric/internal/discovery/MqttServiceDiscoveryService.java

+        super.modified(configProperties);
+    }
+
+    @Reference(cardinality = ReferenceCardinality.MANDATORY)


No need to set the cardinality explicit to mandatory as this is the implicit one.

maggu2810 · 2018-03-05T18:42:16Z

@SJKA Is this code ready to be merged from your side? Then I will try to find some time for a detailed review.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf · 2018-03-06T07:50:41Z

Sure, that would be great. As you probably have noticed I'm not the original author either, so I'd happily accept PRs to this branch as well, to keep the overall effort as low as possible for all of us.

davidgraeff · 2018-03-19T15:42:56Z

I have finished all other Mqtt PRs of mine (Mqtt embedded broker, MqttService, Mqtt broker connections) and like to continue on this one. Where do we stand currently? I guess support for newer PaperUI features like #4146 and #4142 need this binding to be adapted?

htreu · 2018-03-20T07:29:18Z

@davidgraeff this adoption already happened, see: https://github.com/eclipse/smarthome/pull/5101/files#diff-1000633721a6b82ab70752c8c3124aefR22

davidgraeff · 2018-03-20T09:07:09Z

Perfect. There is one thing missing. I have asked the related question here: https://community.openhab.org/t/development-is-it-possible-in-a-thing-types-xml-for-a-thing-to-refer-to-bridges-defined-in-some-other-binding/42126

sjsf · 2018-03-28T12:03:56Z

...and with #5301 being rejected I think this should be ready for @maggu2810's review.

Afterwards it'll need to be run through the Eclipse IP review (a.k.a. CQ) as it exceeds the 1000 LoC.

davidgraeff · 2018-03-28T12:49:31Z

You need to set the bindingid to mqtt though. As I understood Kai, this binding, a homie mqtt binding later on, and the mqtt broker connection binding basically need the same bindingid.

sjsf · 2018-03-28T13:02:45Z

Argh, sure!

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

davidgraeff · 2018-03-28T13:31:32Z

Are dots not allowed in binding directory names? The "mqtt.generic" name wasn't too bad.

sjsf · 2018-03-28T13:36:39Z

No, dots aren't the problem.

I just had a second look at how it was done in the bluetooth case - and realized that in the end one bundle has to be the "main" MQTT one, providing e.g. the binding.xml file and the main frame for the documentation. And therefore my assumption was that this is the best candidate. Isn't it?

davidgraeff · 2018-03-28T13:40:40Z

It depends on the bundle of #3839, but as Kai mentioned somewhere, those two bundles can be grouped together in one feature. So yes it is a good candidate.

* eclipse-archived#4173 changes some APIs, adapt to those. * Remove the broker connection thing, we are going to use the one defined in eclipse-archived#3839 * Remove all discovery stuff Signed-off-by: David Gräff <david.graeff@web.de>

sjsf · 2018-03-28T13:54:34Z

Yes, they will be both included in the same karaf feature.

As #3839 actually contains even more "basic" features of the binding, it might in fact be the even better candidate for the "o.e.sh.binding.mqtt" namespace. Would you rename the folders of #3839? Then I will add the .generic here to the bundle path and the packages again. Okay?

Let's move/fuse binding.xml, README and the like later on. It's already chaotic enough...

davidgraeff · 2018-03-28T13:59:29Z

Haha oh gosh. I just reimported the updated mqtt.generic without generic into eclipse. But yeah will do. There are two more things in this PR:

I broke the tests by removing the discovery stuff and the bridge.
The thing-types.xml need a change: The parent bridge types (there are two!) are : "broker" and "systemBroker".

sjsf · 2018-03-28T14:04:04Z

I just reimported the updated mqtt.generic without generic into eclipse. But yeah will do.

Sorry & thanks - you were just too quick for me 😉

I will have a look.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

maggu2810

Will have a look at the code the next days. Only a few comments.

maggu2810 · 2018-03-28T18:37:18Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/ESH-INF/thing/thing-types.xml

+		<label>Text value</label>
+		<config-description>
+			<parameter name="stateTopic" type="text">
+				<label>Mqtt state topic</label>


Add this comment only once, but will be true for all other occurrences: It should be written MQTT in labels and descriptions.

maggu2810 · 2018-03-28T18:39:14Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/ESH-INF/thing/thing-types.xml

+			</parameter>
+			<parameter name="isfloat" type="boolean">
+				<label>Is Decimal?</label>
+				<description>Any supported transformation can be used. An example for a received JSON from a Mqtt state topic would be a pattern of JSONPATH:$.device.status.temperature for a json {device: {status: { temperature: 23.2 }}}.</description>


wrong description

maggu2810 · 2018-03-28T18:41:19Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/META-INF/MANIFEST.MF

+ org.eclipse.smarthome.io.transport.mqtt,
+ org.osgi.framework,
+ org.osgi.service.component,
+ org.osgi.service.component.annotations;resolution:=optional,


That line has been removed on another PR from all bundles

maggu2810 · 2018-03-28T18:41:58Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/META-INF/MANIFEST.MF

+Service-Component: OSGI-INF/*.xml
+Export-Package: org.eclipse.smarthome.binding.mqtt.generic,
+ org.eclipse.smarthome.binding.mqtt.generic.handler
+Bundle-ActivationPolicy: lazy


Hm, as it is only necessary for the Eclipse IDE and not pure OSGi, I don't think we need it.
Is there already an agreement, if this should be used or not?

maggu2810 · 2018-03-28T18:43:12Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/README.md

+
+## Supported Channels
+
+* **text**: This channel can show the received text on the given topic and can send text to a given topic.


What about "percentage"

maggu2810 · 2018-03-28T18:49:17Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/README.md

+
+## Discovery
+
+Broker connections are automatically discovered and presented as bridges. MQTT provides no means to enumerate available topics though, therefore things representing topics cannot be automatically discovered.


Because broker connections are not managed by this binding, ...

maggu2810 · 2018-03-28T18:50:41Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/README.md

+### Common channel configuration parameters
+
+* __mqttstate__: The MQTT topic that represents the state of the thing. This can be empty, the thing channel will be a state-less trigger then. 
+* __mqttcommand__: The MQTT topic that commands are send to. This can be empty, the thing channel will be read-only then.


mqttcommand => commandTopic?

maggu2810 · 2018-03-28T18:50:43Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/README.md

+
+* __mqttstate__: The MQTT topic that represents the state of the thing. This can be empty, the thing channel will be a state-less trigger then. 
+* __mqttcommand__: The MQTT topic that commands are send to. This can be empty, the thing channel will be read-only then.
+* __transformpattern__: An optional transformation pattern like [JSONPath](http://goessner.net/articles/JsonPath/index.html#e2). Use http://jsonpath.com/ to verify your pattern for the latter case. An example would be JSONPATH:$.device.status.temperature for a received json input of `{device: {status: { temperature: 23.2 }}}` to extract the temperature value.


transformpattern => transformationPattern?

maggu2810 · 2018-03-28T18:50:44Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/README.md

+
+### Common channel configuration parameters
+
+* __mqttstate__: The MQTT topic that represents the state of the thing. This can be empty, the thing channel will be a state-less trigger then. 


mqttstate => stateTopic?

maggu2810 · 2018-03-28T18:51:33Z

extensions/binding/org.eclipse.smarthome.binding.mqtt.generic/pom.xml

@@ -0,0 +1,18 @@
+<?xml version="1.0" encoding="UTF-8"?>


two spaces for one indentation in pom files

The formatter turns them into tabs 😕

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

maggu2810

Shouldn't new classes be annotated with NonNullByDefault?

maggu2810 · 2018-03-29T07:25:00Z

...ric/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/MqttBrokerBindingConstants.java

+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.eclipse.smarthome.binding.mqtt.generic;


I know that the archetype generates a constant class in that package, but are this constants exported by intention?

I guess the initial intention was to have access to the BINDING_ID final string and all defined thing/bridge types. All my submissions so far have the constants class public.

maggu2810 · 2018-03-29T07:26:34Z

....generic/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/ChannelConfig.java

+    @Override
+    public void processMessage(String topic, byte[] payload) {
+        if (!topic.equals(stateTopic)) {
+            logger.trace("Received Mqtt data on {}. Does not match configured thing {}", topic, stateTopic);


Mqtt => MQTT
Please have a look at the other files, too.

maggu2810 · 2018-03-29T07:27:46Z

....generic/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/ChannelConfig.java

+                    .getTransformationService(transformationServiceName);
+            if (service == null) {
+                logger.warn("Transformation service '{}' not found", transformationServiceName);
+                return;


Should the state update on a missing but defined transformation service be ignored or should the state set to UNDEF?

[EDIT] right answer, wrong line. Here the updated one:

Yes, in this case I'd agree 😄

maggu2810 · 2018-03-29T07:28:16Z

....generic/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/ChannelConfig.java

+            try {
+                str = service.transform(transformationPattern, str);
+            } catch (TransformationException e) {
+                logger.error("Error executing the transformation {}", str);


Doesn't UNDEF make sense for a failed transformation?

No, I don't think so. I know several devices/services which send partial updates only. That would mean the state would stay in UNDEF most of the time.

Of course, this heavily depends on the semantics of a "missing" json attribute. Normally that would be the equivalent of null, but my feeling is that this is different in practice.

Can't we cache the transformation service like it was done before? If we receive a message per second, we need to resolve the service 60 times a minute. That sounds very inefficient and contradicts my original design to be honest.

maggu2810 · 2018-03-29T07:32:01Z

....generic/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/ChannelConfig.java

+        return stateTopic;
+    }
+
+    void dispose() {


I assume the naming has been chosen because it is called by the handlers dispose method.
But a name should not chose the naming because of the context it is used only. Wouldn't it make sense to use a consistent naming (initialize and dispose or start and stop or open and close, ...).

maggu2810 · 2018-03-29T08:33:11Z

...neric/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/MqttThingHandler.java

+     * We make this an explicit function for test mocks.
+     */
+    protected MqttBrokerConnectionHandler getBridgeHandler() {
+        return (MqttBrokerConnectionHandler) getBridge().getHandler();


getBridge() can return null, can't it?

maggu2810 · 2018-03-29T08:34:00Z

...neric/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/MqttThingHandler.java

+                    config.value = new OnOffValue(config.on, config.off, config.inverse);
+                    break;
+                default:
+                    throw new IllegalArgumentException("ThingTypeUID not recognised");


Isn't it about a ChannelTypeUID?

maggu2810 · 2018-03-29T08:35:28Z

...rc/main/java/org/eclipse/smarthome/binding/mqtt/generic/internal/AbstractMqttThingValue.java

+    /**
+     * Returns the current value state.
+     */
+    public State getValue();


interface methods are pulic implicit

maggu2810 · 2018-03-29T08:35:42Z

...rc/main/java/org/eclipse/smarthome/binding/mqtt/generic/internal/AbstractMqttThingValue.java

+     * Updates the internal value state with the given command.
+     *
+     * @param command The command to update the internal value.
+     * @return A string representation of the value to be send to Mqtt.


Missing doc for exception

maggu2810 · 2018-03-29T08:35:54Z

...rc/main/java/org/eclipse/smarthome/binding/mqtt/generic/internal/AbstractMqttThingValue.java

+    /**
+     * Updates the internal value with the received Mqtt value.
+     *
+     * @param updatedValue


Missing doc for return value and exception

davidgraeff · 2018-03-29T16:30:35Z

....generic/src/main/java/org/eclipse/smarthome/binding/mqtt/generic/handler/ChannelConfig.java

+ *
+ * @author David Graeff - Initial contribution
+ */
+public class ChannelConfig implements MqttMessageSubscriber {


Oh yes I remember, that was a proof of concept class. It works, but configuration holder classes should be pure DTOs. It should read like this: MessageSubscriber extends ChannelConfig implements MqttMessageSubscriber

davidgraeff · 2018-04-23T18:04:22Z

@maggu2810 : I have squashed the PR and incorporated your requested changes in a second commit in #5450

David Gräff and others added 3 commits February 16, 2018 11:06

adaptations & fixes by sjka

065b725

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

added MQTT to docu & features

07aeff6

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf added Binding New Contribution labels Feb 16, 2018

sjsf changed the title ~~Mqtt things rebased~~ MQTT Generic Binding Feb 16, 2018

sjsf mentioned this pull request Feb 16, 2018

Mqtt things #3876

Closed

fixed static code analysis errors

e75aa62

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

htreu reviewed Feb 16, 2018

View reviewed changes

Mqtt things (#3)

3ee2ff0

* Review comments Signed-off-by: Henning Treu <henning.treu@telekom.de>

maggu2810 reviewed Mar 5, 2018

View reviewed changes

removed superfluous mandatory attribute

c5d5cde

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

renamed mqttgeneric to mqtt

392e887

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf force-pushed the mqttThingsRebased branch from ccee8b1 to 392e887 Compare March 28, 2018 13:28

Adapt to new mqttservice API

51ec9a4

* eclipse-archived#4173 changes some APIs, adapt to those. * Remove the broker connection thing, we are going to use the one defined in eclipse-archived#3839 * Remove all discovery stuff Signed-off-by: David Gräff <david.graeff@web.de>

move to o.e.sh.binding.mqtt.generic folder

b937c33

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

fixed bridge ref names

e401445

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

davidgraeff mentioned this pull request Mar 28, 2018

Mqtt Broker configuration binding #3839

Merged

Simon Kaufmann added 2 commits March 28, 2018 16:29

fix launch config

c9af05e

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

adapt tests to changed transport APIs

97b277f

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

maggu2810 reviewed Mar 28, 2018

View reviewed changes

review comments

de49c91

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

maggu2810 reviewed Mar 29, 2018

View reviewed changes

davidgraeff reviewed Mar 29, 2018

View reviewed changes

davidgraeff mentioned this pull request Apr 23, 2018

Mqtt generic things and Homie 3.x things #5450

Closed

sjsf closed this Apr 24, 2018

davidgraeff mentioned this pull request Jul 9, 2018

[MQTT Generic Things] with Homie 3.x and HomeAssistant thing discovery #5880

Merged

lukasvermeer mentioned this pull request Jun 2, 2020

[mqtt][homeassistant] ComponentSensors (/BinarySensors) - Allow forced_update true openhab/openhab-addons#7063

Closed


		## Supported Channels

		* text: This channel can show the received text on the given topic and can send text to a given topic.


		## Discovery

		Broker connections are automatically discovered and presented as bridges. MQTT provides no means to enumerate available topics though, therefore things representing topics cannot be automatically discovered.


		### Common channel configuration parameters

		* __mqttstate__: The MQTT topic that represents the state of the thing. This can be empty, the thing channel will be a state-less trigger then.

MQTT Generic Binding #5101

MQTT Generic Binding #5101

Conversation

sjsf commented Feb 16, 2018 • edited Loading

htreu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 commented Mar 5, 2018

sjsf commented Mar 6, 2018

davidgraeff commented Mar 19, 2018

htreu commented Mar 20, 2018

davidgraeff commented Mar 20, 2018

sjsf commented Mar 28, 2018

davidgraeff commented Mar 28, 2018

sjsf commented Mar 28, 2018

davidgraeff commented Mar 28, 2018

sjsf commented Mar 28, 2018

davidgraeff commented Mar 28, 2018

sjsf commented Mar 28, 2018

davidgraeff commented Mar 28, 2018

sjsf commented Mar 28, 2018

maggu2810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjsf Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Apr 23, 2018

sjsf commented Feb 16, 2018 •

edited

Loading

sjsf Mar 29, 2018 •

edited

Loading