-
Notifications
You must be signed in to change notification settings - Fork 784
Conversation
d66a980
to
14bfc4d
Compare
ba1ecc9
to
dbf3eff
Compare
@htreu Can you have a look at this PR? It is a simpler and smaller PR than the Mqtt Broker configuration PR and there are less subjects that need to be discussed architecture-wise. |
@davidgraeff Also: Don't you think the binding should support the other noteworthy MQTT message features retained flag and qos? Who knows what the user might need them for... |
I want to keep this PR simple for the start. Additional features can be added later. But it is more likely to get this merged faster if the PR is smaller. @ThomDietrich : Can you elaborate on QoS and retained messages? As far as I know, those are client connection and last-will options and not per-topic options. You can choose those for the mqtt client connection already right now. |
Reasonable and I'd also root for a faster merge. QoS and Retained are both properties on per message basis actually, even though they are often identical amongst all messages per topic. Even though they might not be important in the beginning they are one of a few basic properties. I wonder if the initial version of the binding should support them to facilitate further ambitions. As you see fit! Thanks for working on this crucial feature. |
As mentioned in #3812 (comment), there is no direct dependency on Paper UI work for merging this PR.
This is the outstanding part that should be addressed before the PR can be reviewed & merged. |
9f5ce95
to
23a2d77
Compare
Adapted to dynamic channels. Is the current implementation how it should look like? (Especially the ThingHandler?) |
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>
23a2d77
to
1b40122
Compare
Is this PR going to be accepted? Would like to use this. |
Hi @davidgraeff, this one also seems to be nearly finished. Here Paper UI support is also ready for extending Channels on Things. Would be nice to see your engagement again :-) |
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 didn't do an in-depth review yet, as there are still some major inconsistencies to solve. Nevertheless, I left a few comments on stuff that caught my eyes while browsing through the productive (in the sense of non-test) code.
@@ -0,0 +1,202 @@ | |||
<?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.
The current state of this file doesn't correlate with the code (topic
-ThingType, different names for config parameters, different ChannelTypeUIDs, configuration on channels instead of things, extensible-attribute,...). Could it be that you forgot to commit the updated version?
config.transformationPattern = config.transformationPattern.substring(index + 1); | ||
config.transformationService = getTransformationService(type); | ||
if (config.transformationService == null) { | ||
throw new IllegalArgumentException("The transformation type is unknown: " + 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.
This is not so much of a good idea...
- Don't keep references on OSGi services (which is essentially what you are doing here, although
TransformationHelper
unfortunately hides this fact pretty well) - The transformation service implementation which is configured for this channel might not/currently be available but will be later on. If you let initialization fail because of this, the handler won't get a chance to recover from this.
Therefore, please only remember the transformation name and rather grab a fresh reference every time.
|
||
@Override | ||
public State update(String updatedValue) throws IllegalArgumentException { | ||
numberValue = PercentType.valueOf(updatedValue); |
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.
Should be DecimalType
here I assume?
v = numberValue.doubleValue() + step; | ||
} | ||
numberValue = new PercentType(new BigDecimal(v)); | ||
} else { |
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.
DecimalType
is missing here
@Override | ||
public String update(Command command) throws IllegalArgumentException { | ||
if (command instanceof StringType) { | ||
numberValue = PercentType.valueOf(((StringType) command).toString()); |
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.
DecimalType
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/light-color-rgb-rgbw-rgbcw-to-and-from-hsb-dimmer-conversion/39132/3 |
I took the freedom to rebase this PR and add some commits on-top, filed as #5101. This is not meant to be a hostile take-over, just meant as an offer to push this binding forward. You are more than welcome to continue on this PR here and either implement the review feedback and other amendments yourself or take them from #5101. In the meantime I'm going to close this PR in order to prevent confusion. |
Haha all good, no offense taken. Good to see progress :) |
Implementation of the second idea mentioned in #3812.
This has a strong dependency on #4146 and a soft dependency on #4142 for graphical configuration.
Abstract
At this very moment there exist no means to actually connect mqtt topics to ESH things. There are a lot of commercial and non-commercial (Owntracks, EasyESP, Homie) Mqtt products out there who deserve their own binding.
But for a start I propose a simple generic Mqtt binding where you can bind topics to things (very much like the Openhab 1.x Mqtt binding, but modern) with the following features:
JSONPATH:$.device.status.temperature
for an incoming message of{device: {status: { temperature: 23.2 }}}
.Discussion
I have added four things with one channel each (the value channel). The following item types can be used with the channel:
I'm not sure if we need more things to cover the basic needs, but I might have overlooked use cases.
Cheers,
David