Initial contribution of the KNX 2.0 (refactored) binding #1438
Conversation
| } | ||
|
|
||
| link = new KNXNetworkLinkIP(ipConnectionType, localEndPoint, | ||
| new InetSocketAddress(ip, ((BigDecimal) getConfig().get(PORT_NUMBER)).intValue()), false, |
There was a problem hiding this comment.
Suggestion: use default port number, config field is not required, this will crate null pointer exception if user forgets to set it. As default multitasking address is set, one can assume the default port alia set as well.
There was a problem hiding this comment.
I need to traverse the code again and insert some NPE checks left and right
|
|
||
| protected Logger logger = LoggerFactory.getLogger(KNXBridgeBaseThingHandler.class); | ||
|
|
||
| public IndividualAddressDiscoveryService(KNXBridgeBaseThingHandler bridgeHandler) throws IllegalArgumentException { |
There was a problem hiding this comment.
Suggestion: for Knx traffic sensitive environments it could be usable to have config parameter on bridge to enable/disable scanning
There was a problem hiding this comment.
Fair point. device discovery part, and certainly the part for reading out device parameters, is quite experimental. Sometimes it works fine, but it times out a lot on a busy KNX bus
Please keep in mind, that knx does work this way, there are some installations that need more than one GA per channel, for example: An actuator does not provide it's state but does react to more than one GA, in this case, you will have to configure the second GA in each switch to always get the correct state. |
|
@kgoderis I agree with @udo1toni, this would be a constraint in comparison to the KNX1 binding, where we can add any number of "listeningGAs" (by adding them with "+"). I think it should be fairly easy to support this in your code as well, because you could simply declare the statusGA config parameters to be |
|
@kaikreuzer @udo1toni Thru enough, indeed, you can configure more that one listening GA on an actor. But then, from the perspective of ESH/OH the question is if this holds true as well? IMO in the Thing definition you want to listen to the actual state of Actor (by the same KNX specs/constraints, the Actors can only send to a single Group Address), so, you would only have to listen to a single GA. Since we ruled out the strategy to build virtual actors in ESH/OH, are there use cases whereby a Thing would listen to multiple GA's (e.g. thus sent by different Actors, or, originating from different Communication Objects on the same Actor)? I could imagine something like a Switch that is ON when at least one switch actor of a group of actors (or channels on an actor if a multi-channel device) is switched on, but apart from that.... Maybe the OH1 KNX binding is too long in the past for me, I do not recall having multiple GA's in an item definition in own installation. Just to be clear on this, as it might be utterly confusing, the objective here is not to replicate an KNX Actor in ESH/OH, but rather provide a way/method/interface to operate them, right? That being said, there is no issue to implement multiple="true" of course. Do you rather see this limited to the listening GA's, or also to the "action" GA's, and thus enable things like? [pls neglect exact syntax here]
so that a ON command on the someSwitch channel of the Thing, will switch on for example two different switch actors on the bus, and that in reverse a status sent back by any of the two actors, could change the state of the Item linked to someSwitch? To further complicate things, I presume that in such a case, it is not expected that the channel acts as a bridge between the GA's? (e.g. a 1.001 DPT received on 1/5/1 will set the state of the linked Item to ON, but will also send out a new 1.001 DPT to both 1/5/0 and 1/4/0) |
|
@kaikreuzer the usage of constructs, no? I understand how that can be done from a UI perspective, e.g user making the selection from a list, but how is that dealt with in the DSL? |
|
No, "multiple" does not require any options, it can be any kind of entries. |
I checked as well, and they are not at the time being. Once that is in, I will add support for them |
Yes, you are. I will think about it.
@kgoderis I'll let you know. But I guess you will notice the PR yourself anyway. |
| <scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" immediate="false" name="org.openhab.binding.knx.internal.factory.KNXHandlerFactory"> | ||
|
|
||
| <implementation class="org.openhab.binding.knx.internal.factory.KNXHandlerFactory"/> | ||
| <reference bind="setItemRegistry" cardinality="1..1" interface="org.eclipse.smarthome.core.items.ItemRegistry" name="ItemRegistry" policy="static" unbind="unsetItemRegistry"/> |
There was a problem hiding this comment.
Bindings should not need access to the item registry. And as a matter of fact you are not using it 👍
Just remove the reference here.
| THING_TYPE_IP_BRIDGE, THING_TYPE_SERIAL_BRIDGE); | ||
|
|
||
| private Map<ThingUID, ServiceRegistration<?>> discoveryServiceRegs = new HashMap<>(); | ||
| protected ItemRegistry itemRegistry; |
There was a problem hiding this comment.
the itemRegistry here is dead code (which is good, see my comment above...)
| private Logger logger = LoggerFactory.getLogger(IPBridgeThingHandler.class); | ||
|
|
||
| // the ip connection type for connecting to the KNX bus. Could be either TUNNEL or ROUTING | ||
| private static int ipConnectionType; |
| private static int ipConnectionType; | ||
|
|
||
| // the ip address to use for connecting to the KNX bus | ||
| private static String ip; |
| private static String ip; | ||
|
|
||
| // the group address used within the KNX bus | ||
| private static String localsource; |
| link = new KNXNetworkLinkFT12(serialPort, new TPSettings()); | ||
|
|
||
| } catch (NoClassDefFoundError e) { | ||
| throw new KNXException( |
There was a problem hiding this comment.
please add e as the second argument (cause)
| */ | ||
| public class SerialBridgeThingHandler extends KNXBridgeBaseThingHandler { | ||
|
|
||
| // List of all Configuration parameters |
| @@ -0,0 +1,15 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
registering "dumb" instances just to get the dependencies injected looks like a pretty awkward hack to me
There was a problem hiding this comment.
How else should this be done with Declarative Services? in KNXHandlerFactory? and what if then new KNXTypeMappers would be registered....?
| @@ -0,0 +1,15 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
registering "dumb" instances just to get the dependencies injected looks like a pretty awkward hack to me
| private LinkedBlockingQueue<RetryDatapoint> readDatapoints = new LinkedBlockingQueue<RetryDatapoint>(); | ||
| protected ConcurrentHashMap<IndividualAddress, Destination> destinations = new ConcurrentHashMap<IndividualAddress, Destination>(); | ||
|
|
||
| protected ItemChannelLinkRegistry itemChannelLinkRegistry; |
There was a problem hiding this comment.
you shouldn't need to depend in it in a binding. Also, I cannot see any usage.
There was a problem hiding this comment.
In fact, for the KNX binding you might. This was discussed several times in the past, whereby one has to decide what the binding will do wrt "bridging" values between Items.
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
this implementation of equal is not null safe. And this class is lacking a hashcode() method. Which is especially a problem as it is used within a set!
|
|
||
| if (pollingJob == null || pollingJob.isCancelled()) { | ||
| logger.trace("'{}' will be polled every {} ms", getThing().getUID(), pollingInterval); | ||
| pollingJob = scheduler.scheduleWithFixedDelay(pollingRunnable, Math.round(pollingInterval / 4), |
There was a problem hiding this comment.
pollingInterval / 4 already results in an int, so rounding is useless
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| return (o instanceof RetryDatapoint) |
There was a problem hiding this comment.
again, not null-safe and hashCode() is missing!
PS: let eclipse generate it... ;-)
|
@SJKA You professional coders are way too professional ;-) ;-) ;-) |
8ef3876 to
365d033
Compare
|
@SJKA Processed the bulk, some stuff we need to further discuss |
|
Quick poll : who is using ETS3, ETS4 or ETS5? I am looking into adding the possibility to OH2 to automatically read-out the knxproj ETS files and create Things from that. The XSD of the ETS project files have evolved over time, I would like to find out to what extent the various versions of ETS have to be supported |
|
ETS 5
…On Sat, Dec 24, 2016, 13:32 Karel Goderis ***@***.***> wrote:
Quick poll : who is using ETS3, ETS4 or ETS5?
I am looking into adding the possibility to OH2 to automatically read-out
the knxproj ETS files and create Things from that. The XSD of the ETS
project files have evolved over time, I would like to find out to what
extent the various versions of ETS have to be supported
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<https://github.com/openhab/openhab2-addons/pull/1438#issuecomment-269080795>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AV-t2gRp4Sg3hgDPHZeomwHcF8CxEQ6cks5rLQKvgaJpZM4K3SjL>
.
|
|
ETS 5 |
|
ETS 3 |
|
ETS 5 |
|
Have a look at https://github.com/tuxedo0801/KnxProjParser |
|
no, please don't - it's GPL code. So unless the author agrees to contribute a version of that code under the EPL or the like, it's really not a good source to copy from... |
|
ETS 4. In my opinion KNX could be a bit more succeful if ETS wasn't so expensive, even the upgrade to ETS 5 is relatively pricey :-/ |
|
ETS 4 |
|
ETS5 btw.: Thanks for the effort you've put into KNX2 binding @kgoderis. I'm looking forward to see it merged... |
|
ETS 4 |
|
I did not know that https://github.com/tuxedo0801/KnxProjParser was out there, but then, I have not intention whatsoever to use or even look at the code. Well, I looked at it and do not like parts of it anyways ;-) I probed knx.org to get the official XSD files but got no answer from them (anyone got them?). Since my own ETS5-driven installed base is rather large, I derived an XSD from the final XML files, and I hope they are rather complete. The XSD will serve as input for xjc (Java XML Binding Compiler). If someone wants to contribute a XSD for ETS4, pls feel free to do so (http://www.freeformatter.com/xsd-generator.html) |
|
ETS5 |
|
ETS4
Let me know if that might help you, and if so, how could I send them you? |
|
I have been thinking over the last days about the troubles you have with the As a matter of fact, the KNX binding is the only binding that we have in openHAB which implements this method at all. All others don't need it. And that's for a good reason: This method only makes sense for use-cases where the state is not "owned" somewhere in any real device (e.g. a Hue lamp knows whether it is ON or OFF), but where the state resides on openHAB side (may it the item itself or openHAB acting as a proxy to another device outside in the KNX bus). So, for ChannelTypes representing a KNX switch/dimmer/rollershutter actor it does not make sense to do anything in The interesting part will be if e.g. a KNX wall-button should be included in openHAB. It sends "commands" via on GA, but also needs to know via another GA the current state of the GA in order to reflect this with its LED and send the correct ON/OFF command if pushed. A good example would be a Hue lamp which should be switched ON and OFF from a KNX wall-button. However, such a device only sends commands, it does not receive any. There is no So I'm wondering: if we would define the ChannelTypes right, wouldn't this whole problem suddenly vanish? And in fact there correctly already is a "statusswitch" type. So we'd only need to use this information to decide whether the communication is meant to go one way or the other (in the sense of Or am I missing anything? |
|
@SJKA Maybe the best thing to explain the problem is with an example. Assume I want to control a Shutter with a wall-mounted control panel. The shutter is defined as follows: [This comes from the knxproj parsing] In the runtime these channels are linked to an Item The Item is defined as follows:
I now control the shutter, e.g. I open it, via the wall mounted control panel, and what follows is a bunch of KNX Telegrams on the bus to Group Address 3/3/14 (e.g. the shutter is reporting the actual position) In the logs you will thus have:
So, the binding is receiving the telegram on the appropriate Channel, and will process it. It does this by updating the In doing so, the Runtime will now issue a handleUpdate for all the other Channels linked to that Item. The other Channels, see above, include for example GA 3/3/13 to steer the shutter to a desired/defined position. We do not want that to happen, because the shutter is actually moving down, so, we definitely do not want the handleUpdate() to happen for those other Channels linked to the I solve this by tracking the itemChannelLinkRegistry, and thus can do the following: What are the solutions at hand?
To me the binding has to have a handleUpdate(). I see the KNX bus not as a "real" bus, bus rather as a kind of shared memory between actors. they all use a memory address, i.e. GA, to write and read from that memory. So, you not only have to send commands, but also have to deal with other actors writing to that memory, and listen for changes on GA's you are interested in. What you describe (actor sending a command, and a status update coming via an another GA) is not necessarily true: it all depends on the actual configuration of the actor, there is no obligation whatsoever to respond with a status update. Also, status updates are sent without a previous command send by someone else, it is just a device reporting a state at given interval or conditions. So, handleCommand is for stuff coming from the ESH runtime, handleUpdate is for stuff coming from the bus, but unfortunately also from the ThingManager doing the bridging between Channels linked to an Item |
|
@SJKA Just to follow up on this, consider the example you give with the HUE lamp. Well, in a scenario whereby the ThingManager would be the disc jokey of handleUpdate() and handleCommands(), using solution 2. (the Auto-bridge binding) you would instruct the ThingHandler to allow "inter"-binding bridging of updates, but disallow "intra"-binding updates. That way the HUE sourced updates can be passed on to the KNX-binding linked Channels, but updates between KNX-linked Channels would not be allowed. |
Ah... well, there is no distinction between commands and updates on the KNX bus. It boils down to his question : "where do you want to place the functional KNX logic in the runtime"?
Why would that be? In the current implementation, a GA is a GA, they are all the same type. Am I right if you in fact suggest to define different ChannelTypes based on the actual configuration/characteristics of the GA, whether that is defined in the DSL or coming from the knxproj parsing? IMHO that will be messy, because part of that is configurable in ETS. For example, you can easily change the Read/Write/Transmit/Update flags of a GA in an actor. |
Yes, I'm very well aware of how it works. Still, the whole problem arises only because you are trying to model the way how KNX works into ESH directly in a generic fashion. As I said, I do not understand the need for it and I also don't think it is the right approach.
No, that was exactly my point: When you are using such a functional abstraction, there is no need for parsing or guessing. You already know by the functional type if the state/logic is kept on KNX side, no need for
You didn't read my comment closely. I agree with you, it needs to have it. But it does not make sense to to anything in there all of the time. It's actually rather an exceptional case where this really is needed.
Really? Where? AFAIR, the suggestion was about moving the semantic level from ThingTypes down to ChannelTypes (see https://github.com/openhab/openhab2-addons/pull/21#issuecomment-255045806), not to drop the functional level completely! And btw. that's what you still have here in generic.xml and what you also decribed as the main purpose of this PR. So why not use this information?
I really can't find it. Who suggested that?
Yes. As you said yourself, the ETS file format is very limited wrt figuring out the functional abstraction. And I agree, this sucks. However, as much as I like the idea - and I have to admit that you got pretty far with it despite of its limitations - I'd rather sacrifice the knxproj parsing than implementing a special mechanism in the framework which allows the KNX binding to guess a little better but still it's only a guess.
If I configure my actor to not have a status GA, then my wall-buttons won't light up. Same with openHAB: If I don't have a status GA, then I can't use the current value from the bus. What is your point? No doubt, you will always be able to abuse the system to a limit that it does not interact well with its environment anymore. But do you have a common use-case example where this is required? |
Signed-off-by: Karel Goderis <karel.goderis@me.com>
That is not in general! Older (switch-)actors in general and some modern simple actors use Main-GA as Status-GA. Then the Main-GA then is set to write AND read.
KNX wall-button can switch its LED by its Main-GA as Status-GA too, it does not necessarily need two GAs.
No, this is not abuse the system to a limit. As described above, many Actors need/must not handle status-GA! Some times it can help to have a look back, where most parts work very well in KNX1. Switch: RollershutterItem or Dimmer: General: The underlying logic, should be easily transferred into Things / Channels? Just as a stimulus! ;-) |
|
Does the current Source (knx-refactor) work with current Nightly Build 2.1.0? I have compiled KNX2 Binding source from knx-refactor branch with current openhab:master well. It connects and no Error or Warning is shown. It does not discover. It finds knxproj, but does not parse. I can send from KNX-Bus but it ends at:
I can switch Channel over Paper UI - Control, but nothing is sent to KNX-Bus: In my despair, I tried to compile the branch directly, with very old Sources with hole knx-refactor branch (This branch is 3 commits ahead, 573 commits behind openhab:master. ). As expected, this does not work. How do you get a working Binding at the moment. @kgoderis, it would help, if you can copy/update your knx-refactor branch to openhab:master. knx-refactor branch is no longer compatible with current KNX2 source (Java 8)! |
Maybe we are talking sideways here, but how would you deal with Updates sent by an Actor on the KNX bus? I am not sure I understand what you try to explain here
https://github.com/openhab/openhab2-addons/pull/21#issuecomment-257531370 Btw, as is stands today, it is still at the Channel level except that there is a 1-GA-1-Channel idiom because of the knxproj parsing.
I think It was something discussed with Kai over email, but that is a long, very long time ago.
Because I wanted to go for the automatic parsing route
This is a big no-no for me. I can see the benefits of this in a small installation, but with now >130 actors and close to 3000 GA's in production at my home, this is impossible. Keeping the KNX binding configuration in sync with what is in ETS5 is a big nightmare. In reverse, I see a lot of benefits of being able to read-out knxproj files (no more configuration copy-over mistakes for one), and therefore the fact you have to let the binding use the itemChannelLinkRegistry (because that's all there is to it) is a minor thing. Yes it is guessing to some extend, but one can overcome some of the errors through a definition via the DSL
I am afraid this does not hold true 100%. Many actors, not only wall-buttons, do not have a loop-back status GA mechanism. What other solutions are there? |
I have no idea - I tested the current source in Eclipse and it ran perfectly.
Not sure I understood this. I though the complete openhab:master went Java 8? |
|
@SJKA I grabbed an older configuration file of mine, based on the generic.xml syntax, and in that one I have about 400 "multi-GA" Channels, and about 1200 single-GA channels. Even as you can solve the "circular Telegram" problem by grouping GA's in Channels, still, in the real world, there are bunch of "orphan" Channels that will be linked to Items as well, and thus you can not except the problem to go away automagically. Unless it is accepted that users misconfigure the whole binding, of course. |
|
@SJKA Simon, after some thinking I came to the conclusion that we suffer badly from a lack of context with respect to configuring KNX in ESH/OH, and there is nothing we can do about it. It is not in the ETS configuration files, the only place it basically resides is in the mind of the user. Getting that out of that mind will require manual coding/configuration, it can not be done automatically. That configuration can be done at the level of .items or .things (hence the discussion were are currently in). For me the logical place to do that is the .things context, as you suggest and already implemented, but it will still enable faulty configurations. However, I think we still can have good use of the information stored in the ETS configuration files. We could think of a kind of KNXConfigurationProvider API that can basically provide information on Individual Addresses and Group Addresses. For GA's, it can provide the DPT, and the R/W/T/U flags. This configuration is now explicitly set through the ChannelType, and provided by means of the DSL configuration and KNXChannelSelectorProxy class. In fact, the big issue in configuring KNX in ESH comes from the need to back and forward to ETS to get specific kind of information, like DPT, flags, descriptions and so forth. Secondly, there is currently no easy way to multiply/copy&paste configurations in OH, e.g. configuring similar rooms in the house. Lastly, we what we have at hand, we do not solve the inherent problem of related GA's that are not of the same ItemType or can not be linked. For example, you can have a presence sensor that reports back a Start of Motion, Stop of Motion and Status. This are in fact 3 separate Channels, you can not put them together in a logical block, e.g. 1 Channel with 3 defined GAs. For example, switches with energy read-outs: the switch is a Channel of given Type, but the energy part is another Type, yet, ideally one would like to have these GA's in a logical block. (The only solution is at the Thing level in this case, but that is not the route we want to go). This will lead to a massive amount of 1-GA-1-Channel channels, and thus a lot of administration. |
I'm right you have imported KNX2 Project from your knx-refactor to eclipse. In your eclipse, there resists the current openhab:master? Then you can run in eclipse, this works in mine too. To get a knx2.jar and for using code-analysis, I have copied knx2 Project manually into openhab:master Branch folders an ran mvn clean install in KNX2 folder. This compiles perfect, but the jar does not run perfect in a openHAB 2.1.0 karaf nightly Installation (no IDE). I'm wondering why it runs in Eclipse but not karaf. hmmm...
Your current Brunch knx-refactor is not compatible to java 8 , because it is too old: The only Source which is java 8 compatible in Banch knx-refactor is the source of KNX2. There I told you: https://github.com/openhab/openhab2-addons/pull/1438#issuecomment-302376240 As I can see you only testing in IDE. So it os on me, I will try to find the reason why in karaf it is not working... |
Why 3 Channels?
Why not shorthand? same as: What about Simons (same Kai) Examples: |
Ok in that sense. What I have to do is rebase it against the HEAD of the master, but then we will potentially loose the discussions/comments here above. I did that with another binding this week, and Jenkins screwed up after the forced push. |
Because a "motion start" is not the same kind of information as a "status". It was just an example, not all GA's of an actor lead to the same kind of information, even if their Type happens to be same. Alternative example: A roller shutter can have GA's for the position, but also boolean indicators to indicate if it is in the uppermost position for example |
What Type will be used to update the Channel? a Switch (the switch) or a Number (the energy read out in Wh)? AFAIK a Channel is only associated with a single Type. |
Maybe I was not clear. Even as you can group together GA's as you describe, next to that you will still have a large amount of GA's that cannot be coupled together, and thus, these ones are mapped on a single Channel so to speak |
OH NO!!!!! 👎 EDIT: There is a way you add a new Branch maybe "knx-refactor-java8" and push from local the whole openhab:master including current org.openhab.binding.knx folder from knx-refactor. So I did first Time with my changes for generic mapper and code-analysis. Did it with eclipse directly from current openhab:master to my github fork: Team->Remote->Push... there you can Push to another repository and what branch. |
OK, thats right! We are back at: To show the togetherness, what about this: |
IMO the problem is still the same. What kind of Types will you push on this Channel? What ItemType will this channel be linked to? |
Naive as I am, I would make three channels out of it: It's about naming, right. We need unique names like a,b,c. I can live with it! |
* Bulk edit linter errors. Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
This is PR that contains a refactoring of the KNX 2.0 binding. In short, it will allow a DSL syntax similar as follows:
I have made some design decisions as well, but these are up for discussion:
[ GroupAddresses="1/5/0, 1/5/1"]configuration constructs for the channels, but just one GA per configuration parameter, just to keep things simpelThe rest you will have to discover in the code itself. Enjoy ;-)
Signed-off-by: Karel Goderis karel.goderis@me.com