initial contribution of the KNX Basic binding #2323
Conversation
|
Some things to notice:
|
|
@SJKA The only main feedback I have so far is that I think we should generalise the Read/Write concept and make that configurable per Group Address by using flags, and to make the DPT also configurable. If these two elements are not configured then we can revert to standard values. Also, I would copy the R/W meaning from the KNX bus and thus step away from the connotation we have given to it, which is in fact copied from the KNX 1.0 binding. In doing so we can simplify even more the Basic handler, and have "richer" extensions afterwards, for example such as virtual KNX devices residing in the runtime that are participating on the bus as if they were real actors. This generalisation will also allow for a better re-use of the knxproj parsing code (which I would really like to re-use as I have spent a lot of hours on that) I will give this a go and propose a commit to your repo |
|
@SJKA Simon, I have created sjsf#1 As it was, your initial PR did not resolve the issue of "circular" updates that are "inter"-binding, e.g. when no other binding like Hue is involved, but only KNX channels. In the above PR to your repo this is solved by generalising the configuration of the Group Addresses that make up a Channel Configuration (see the src code for doc), and by doing away with handleUpdate() altogether (which is perfectly possible, since all relevant Types are both implementing the Command and State interfaces) |
Could you please sketch up a concrete example here? I would really like to understand what use-cases you have in mind. |
Sure. Just consider the simple example of a Channel defined like
The second major use case is when a user couples together generic/"primitive" channels through an Item, for example, because there is no advanced channel like rollershutter available. Imagine that you couple together 2 separate "switch" channels, one using GA 2/2/155 and the other 2/2/156, then in the flow here above you end up with an endless loop. This use case can not be solved with the current architecture, and requires one of the solutions sketched before (auto-bridge binding,....) Some comments ago you mentioned that the KNX binding was the only binding implementing handleUpdate(), so I sought after a solution that could do away with handleUpdate(), so that the binding would behave like any other binding on the runtime. That was very easy in fact, since almost all Types are both Command and State, so, everything can be handled with just handleCommand(). I presume that this is the same reason why no-one else ever bothered implementing handleUpdate() in bindings. In the example above, you then get :
|
|
@SJKA Below is a bit of logging of how thing flow for a simple statusswitch (See example above) |
|
@SJKA To illustrate the "looping problem", consider the following example: 9/5/100 and /101 are non-existing on my KNX bus, it is just for illustration purposes Here 2 KNX channels are bound to the same item. This is what can be observed when toggling the Item to ON: Because handleUpdate() is gone, the loop is not endless anymore, but it still loops 1 time (which needs to be fixed) |
|
@SJKA I updated the commit to your repo with a possible solution for the "loop problem". It brings back handleUpdate() with a twist so that your use case (HUE lamp, wall button) is supported |
| knxScheduler.schedule(new ReadRunnable(groupAddress, dpt), 0, TimeUnit.SECONDS); | ||
| } | ||
|
|
||
| if (recurring && readInterval != null) { |
There was a problem hiding this comment.
the truthiness of recurring implies readInterval != null.
There was a problem hiding this comment.
well spotted! I just put it there in order to shut up the IDE, as Eclipse seems to be not a believer of boolean logic.
There was a problem hiding this comment.
yes, eclipse is ignorant to the concept of truthiness ;-)
No! Not on in basic handler. Let's do this in the generic one - this is what we have it for.
I agree here, dpt configuration is yet missing. However, I wouldn't want to squeeze this into specially formatted strings, but would spend separated config parameters for it. Also, I would copy the R/W meaning from the KNX bus and thus step away from the connotation we have given to it, which is in fact copied from the KNX 1.0 binding.
No, again - the knxproj parsing (as well as the automatic discovery) should be based on the generic thing handler, not on the basic one. |
|
@SJKA Do I read you well that we then will have (at least) 3 different bundles:
|
|
@kgoderis regarding your examples: In the first one, the following assumption is wrong:
A statusswitch does nothing in handleUpdate() -> no cycle
Correct - this would require a new ChannelType in the basic ThingHandler. I would leave it to the generic ThingHandler to cover such scenarios.
Not quite. You have to keep in mind the different meanings of commands and state update events. The commands are an intention to change something in reality (e.g. make the light brighter). The state update events only notify that an item (which reflects the reality) changed its value (e.g. the light was dimmed, therefore the item now got an update to a lower value). It usually is not meant for a binding to again change the real world because of that. So I did not want to imply that it is generally wrong to implement handleUpdate(). I just wanted to point out that it is not common practice to enable a binding to "follow" another Thing, just because both their channels are bound to the very same item. Usually, if there are two things which both carry their own state, rules are used to "connect" them to each other. There are some exceptions to this, which are not due to the KNX protocol, but rather because some types of devices are pretty common in the KNX world which are not in other environments, like e.g. a wall-button which has a LED reflecting the current state (very bad idea on battery powered devices, easy on KNX). However, I could imagine some other scenarios where it would make sense to "follow" another device, e.g. a thermostat following another "smart" thermostat like NEST. There it would also be feasible to take the shortcut and make one be the "slave" of another, without the need to write a rule for it. Therefore I could imagine that a more generic solution like the profiles as anticipated in #2226 could be used in order to make item-channel-links as slaves and handle some of the magic in |
Maybe... For sure we need another ThingHandler, which can handle "generic" channels and does all the guessing magic. And then there is the project parsing and bus scanning stuff, which would create instances of such a generic thing and therefore relies on them. I don't mind if these parts end up in one bundle or in two. That actually depends on how entangled they are and if one makes sense without the other. |
I think that anything more than 2 bundles is overkill and will confuse people. Via the commit proposed the basic Thing can still be basic without a lot of configuration information present, falling back to logical default values. I would not put the DPT in separate channel configuration elements because the number of configuration elements is variable with each Channel Type. Maybe you can go through the commit to better structure our discussion here? |
Of course it has to make sense somehow - architecturally and from the user's point of view. Still, if in the end we think it makes sense, we can do so. As you might have seen I currently only factored out one additional bundle, which contains both the generic handler and the project parsing.
Yes, they actually have to be even more variable than what they are currently, as we will also have to support multiple GAs for one configuration parameter (e.g. for actors not announcing a dedicated status GA but reacting on several GAs). Still, I would assume that the multiples will all be of the same type, as it also was in the OH1 binding because KNX requires it. So with
Sure, I will. However, general discussions are better suited here. |
Ok, it does nothing indeed in what you provided in the PR, but on a general note, handleCommand() are followed by the handleUpdate() afterwards, and if we need to solve this bit for basic, and/or generic and/or extended handlers, we better fix it once. [Btw, after that post I made changes to the commit, so some comments are not 100% in line anymore]
Ah... I do not agree here unfortunately. I have had different variations of the KNX binding running over the last 2 year, with and without all the ad hoc solutions likes the auto-bridging and all that. The basic version as you propose it will be fine until the first person comes across a use case where it doesn't work anymore. IMHO we need to have firm plumbing that works for basic and extended scenarios
That part of the post is superseded by the altered commit, so I take that back. (I agree on what you say however, but it feels to me that this different meaning of command and states have been used/abused in the very same way in the past for other bindings)
I think we have no time to wait for that, frankly. The KNX binding has been in limbo for 2 years without a clear direction. It is time to come conclusions, at least that is my personal opinion. On the slave concept: with the commit proposed there is no need for such a new concept anymore. just my 2 cents. |
That sounds very much as if you want to replicate KNX actors in software. I thought we left that route |
|
[Mental Note] We could imagine something similar to http://www.gira.com/en/gebaeudetechnik/systeme/knx-eib_system/knx-produkte/systemgeraete/logikmodul/logikbausteine.html into the "KNX Advanced" binding |
|
@kgoderis Isn´t this what rule templates are all about? To provide logic templates which can be "filled in" by items and conditions? |
|
@htreu yes Indeed, but for some of the Gira logic functions we would have to code some Actions in the KNX binding. E.g. some of these involve sending a "crafted" Telegram on the KNX bus. It is to be investigated once we get the basics right. |
| } | ||
|
|
||
| try { | ||
| if (!useNAT) { |
There was a problem hiding this comment.
Maybe a good reason to make it a boolean primitive?
|
|
||
| <config-description> | ||
| <parameter name="address" type="text"> | ||
| <label>Address</label> |
There was a problem hiding this comment.
Layout looks broken (most likely a combination of tabs and spaces), please format using eclipse
| <required>true</required> | ||
| <default>0</default> | ||
| </parameter> | ||
| <parameter name="enableDiscovery" type="boolean"> |
| Service-Component: OSGI-INF/*.xml | ||
| Export-Package: org.openhab.binding.knx, | ||
| org.openhab.binding.knx.handler, | ||
| tuwien.auto.calimero, |
There was a problem hiding this comment.
Are these exports expected, is this both a binding and bundle to provide others things?
|
|
||
| The IP Gateway is the most commonly used way to connect to the KNX bus. At its base, the *ip* bridge accepts the following configuration parameters: | ||
|
|
||
| |Name|Required|Description|Default value| |
There was a problem hiding this comment.
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 👍
| <p><em> | ||
| <strong>Calimero 2 - A library for KNX network access</strong> <br/><br/> | ||
| Calimero 2 is free software; you can redistribute it and/or modify | ||
| it under the terms of the GNU General Public License as published by |
There was a problem hiding this comment.
There was a problem hiding this comment.
I guess the clean way would be switching to EPL 2.0 and add GPL 2.0 as a secondary license to this binding. But I'm not that much into the details yet.
@kaikreuzer any advice from your side?
There was a problem hiding this comment.
No need to put any code under GPL. Long time ago, I went through a tedious discussion to have the Calimero team to move the code from GPL to GPL w/ classpath exception, see https://github.com/openhab/openhab2-addons/pull/2323/files#diff-bff3d9a909c2a99471fde09efef41e45R54.
As long as we merely include the calimero binary as is, we can happily distribute the KNX binding under EPL.
There was a problem hiding this comment.
Oh..., I never read that far in license texts 😉
Thanks for the clarification!
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
This method is almost 200 lines, there is no valid reason for such a big method
|
|
||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class KNXThreadPoolFactory { |
| */ | ||
| public class DeviceConstants { | ||
|
|
||
| private DeviceConstants() { |
| } | ||
| return null; | ||
| } | ||
| } |
addons/binding/pom.xml
Outdated
| @@ -41,6 +41,7 @@ | |||
| <module>org.openhab.binding.globalcache</module> | |||
| <module>org.openhab.binding.ipp</module> | |||
| <module>org.openhab.binding.keba</module> | |||
| <module>org.openhab.binding.knx</module> | |||
There was a problem hiding this comment.
To have the binding being picked up by the distro, you furthermore need to add it to the feature.xml, again at the alphabetically correct position. If you have a dependency on some transport bundle (e.g. upnp, mdns or serial), make sure to add a line for this dependency as well (see the other bindings as an example).
|
hey, |
|
Yes, it's on my stack! There are some changes required in the framework in order to prevent the infinite loops of state updates properly. I'm working on those right now. And then, of course, I will incorporate all the feedback immediately in order to push this PR forward. I'm desperately waiting for it myself 😄 |
|
Any news? The double telegram problem is a total blocker for me as those telegrams interfere with my infrared switches causing to switch the light off immediately after them turning it on. |
|
Maybe it would help if this behaviour (one thing for all up to one thing for one channel/item) is more explicit described in the documentation, at least to me this was confusing and I was at the beginning really not shure what's the right way until I noticed both ways (or something in between) is possible. |
|
Yes, of course! The current wording "They have no specific function" indeed is not very elaborate. Would you like giving it a shot here? |
|
New day, updated instruction.
Please keep in mind that I have exactly 10 days of experience with OpenHAB so I'm learning along the way and I really appreciate the feed back. |
|
When I define an item the same way (or so I think) in Paper UI and .items config, they are shown differently in paper UI Control: the [%s] is not replaced by the value. .items config Paper UI Control Paper UI item |
|
Hi, |
|
@nibblerrick @SJKA The item can only be linked to 1 knx device while there are usually more than one knx devices participating in a channel. I.e. push button and actuator for a light switch. So which of these two devices needs to be polled to show the thing as online or offline. The best is to go for the actuator since it contains the state and does the actual switch of the light. If the pushbutton is down, ather device can still send the telegram switch on. 2. All the readable group addresses (i.e. <) in all channels it contains Looking at the knx 1 binding, each GA had an individual autorefresh to reread the GA every seconds Could it be that there are 2 separate use cases for the item
If my assumptions are correct, "technically it doesn't make any difference" is correct if you just want to use the channels in the ui, rules,... Feel free to give feedback on this. |
|
I do not recommend to use readInterval at all but only if this is the only way to get the state. There are some (old) devices which do not provide a regular send interval, so this might be a solution for these devices, but in a proper setup there should be no need for frequently read requests. I have setup my 40 devices as individual devices, naming is like manufacturerTypeofdevice_a_b_c where a_b_c is the bus address a.b.c So I have several Devices without any channel, but I don't bother as my tests worked great so far. in question of |
|
@udo1toni, Your description I would classify as best practice! In my Tests, the |
|
Just a quick question about the best approach to use the new KNX binding. Thanks for the clarification |
|
@Astrakan27 It only works on 2.3 snapshot. |
|
@ArdescoConsulting I think the main confusion is here with the (hardware)address you can specify and the GAs were normally more than one (hardware)address/devices. The configuration of @udo1toni seems to be a good way but my problem is one time I read the text and I think I'm getting it, next time I am confused. I don't think I understood it completely, maybe I have to play with the options in PaperUI a bit. |
|
Hello, Maybe I'm missing a configuration something elsewhere in OH2 but I tried to follow the general guidelines for multi-channel configuration. My goal is to use OH2 to send current datetime to the KNX bus. I see the CurrentDateTime object changing every minute. But nothing is sent to the KNX bus. I'm running OH2 2.2 with the latest daily package from https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/binding/org.openhab.binding.knx/2.3.0-SNAPSHOT/org.openhab.binding.knx-2.3.0-SNAPSHOT.jar Regards |
|
@oblaise your item configuration looks okay. What is the channel-type of |
|
And which DPT is ch_CurrentDateTime?
|
|
Hello, how to use a ga with DPT 9.007 (humidity)? In OH1 I used In OH2 This also works with OH2 binding, but instead of 51% I get 0.51%. Edit: same issue, but reversed order for DPT 14.017 OH2 gives 701 g/m³ With or without DPT give same values. |
|
@pdcemulator valid point! |
|
Separate issue sounds like a good idea. :-) |
|
@SJKA In my test I used datetime. Just tried with datetime-control but without more success: @udo1toni this was explictly defined as 19.001 but I tested also the two other DPT type without more success. |
|
Hi all, |
|
@FredericMa Indeed, the binding currently does not have a "color" channel type. Therefore, please create a separate issue for it 😄 |
|
@SJKA So I continued my tests and observed (Sorry for not doing that before) that after OH2 restart it worked with datetime-control. If I'm putting datetime it doesn't work it doesn't work after restart. If I change to datetime-control during running it doesn't work (bug?). So my question: why do I need to use datetime-control? From my understanding the difference between XXX and XXX-control is that XXX-control doesn't directly match to an existing device on the KNX bus (GA not associated to a KNX communication object that can be read) but instead the state remains owned by OH2 and thus OH2 never issue read request to XXX-control GA. But from my understanding this shouldn't prevent OH2 to send telegram to GA when using the standard XXX control type. |
Looking at from a technical perspective: Not commands, but state updates. And this is effectively what happens here: The NTP binding doesn't send commands but only issues state updates for the item. Standard channels don't react on those because otherwise we would end up in loops. Your assumption to use the
is for sure correct but a little too restrictive. Rather think of it like who "owns" the truth, and this clearly is the NTP binding on openHAB side in your case. |
|
I'm not sure if I understand this parts of your best practice
With my understanding of it I would do the sample like below. Is that correct? BTW sorry for the late reply. A bad bios update resulted in a complete reinstall :( @nibblerrick I'll update the tutorial document once I understand the best practice myself |
|
No, I used the Things for monitoring and channel definition. |
* New translations DefaultSystemChannels.properties (Hungarian) * New translations SystemProfiles.properties (Dutch) * New translations SystemProfiles.properties (Hungarian) * New translations tags.properties (French) * New translations tags.properties (French)
As discussed in #2294, this is a PR which contains the factored-out part regarding "basic" KNX features.
It uses strongly typed channels, i.e. neither contains the generic magic, nor does any bus scanning or KNX project imports. These parts will be taken care of in separate PRs as their own dedicated bundles, extending this basic part.
There are some details which are still open (see below), but I hope we can address them quickly with your feedback.
Also-by: Kai Kreuzer kai@openhab.org
Also-by: Karel Goderis karel.goderis@me.com
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com