-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[nikobus] Nikobus Binding initial contribution #6021
Conversation
Travis tests were successfulHey @crnjan, |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Just a quick comment: your binding is missing in the |
Thank you @Hilbrand for super fast check - my bad, did use skeleton to create the project (on MacOS) and |
@crnjan ok no problem. I saw this on other new bindings too. So just wanted to check if it wasn't something with the skeletonscript. Also check the travis-ci build. It shows the messages from the automatic checks (Those will also be shown when doing a local maven build on the binding) |
@crnjan I plan to do a more detailed review. But I was looking at the serial connection implementation. In openHAB the preferred way to implement serial is via the |
Thank you @Hilbrand, will check it and adjust the code. |
@Hilbrand - your suggestion allowed me to simplify the code quite a bit (and when connected via serial connection - it works perfectly) - thx! But seems as I'm not using the correct format for tcp/ip connection - it's giving me Mainly changes were done here. |
@crnjan The format looks correct. When do you get this message? At startup? or also when you add a new thing? Btw. I would recommend to keep the serial part in a separate class (and maybe split also some other code) to make the code more readable. The handler class has now become very big, making it more difficult to understand what's going on. |
I get it when calling |
I was interested in the specific context. Like when you debug or when the system starts up, or when you add a thing? |
Binding will try to connect each time there is a need to send a command (if not connected already), i.e. when |
Moved serial connection related code to separate class. |
Updated readme as well to reflect binding (bridge) changes. |
@Hilbrand - updated the binding and now using |
@crnjan It tried it in locally in Eclipse and then (and by modifying to get the connection in initialized) I get the serial over ip connector, but doesn't continue after that because I don't have a device. So it fails on connecting. I have the service It is a problem that the ip-address can't be entered into PaperUI. I've think I've seen a discussion about this, but can't remember where. It's possible to use it in a things file configuration as you mentioned. |
...nikobus/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusPcLinkHandler.java
Show resolved
Hide resolved
bundles/org.openhab.binding.nikobus/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
@Hilbrand - yes, that was it - missing service - now I can connect via tcp/ip. Thank you! |
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 think I've completed my review. Mostly comments related to logging and exceptions.
...s/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusDimmerModuleHandler.java
Outdated
Show resolved
Hide resolved
...s/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusDimmerModuleHandler.java
Outdated
Show resolved
Hide resolved
...nikobus/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusModuleHandler.java
Show resolved
Hide resolved
...nikobus/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusPcLinkHandler.java
Outdated
Show resolved
Hide resolved
...nikobus/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusPcLinkHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/nikobus/internal/protocol/SwitchModuleCommandFactory.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/nikobus/internal/protocol/SwitchModuleCommandFactory.java
Outdated
Show resolved
Hide resolved
...g.nikobus/src/main/java/org/openhab/binding/nikobus/internal/protocol/SwitchModuleGroup.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.nikobus/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.nikobus/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Travis tests have failedHey @crnjan, 2nd BuildExpand here
|
Travis tests were successfulHey @crnjan, |
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Travis tests were successfulHey @crnjan, |
Travis tests have failedHey @crnjan, 1st BuildExpand here
|
Travis tests have failedHey @crnjan, 1st BuildExpand here
|
Travis tests were successfulHey @crnjan, |
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Travis tests were successfulHey @crnjan, |
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'll had a final look. Just 2 comments. I'll ping the other @openhab/2-x-add-ons-maintainers for the second eyes-review.
bundles/org.openhab.binding.nikobus/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...obus/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusBaseThingHandler.java
Show resolved
Hide resolved
…nds all the traffic through the serial interface. Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Travis tests have failedHey @crnjan, 2nd BuildExpand here
|
Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Travis tests have failedHey @crnjan, 2nd BuildExpand here
|
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.
LGTM. To the other @openhab/2-x-add-ons-maintainers This new binding is up for a 2nd opinion review.
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.
Thanks looks great!
Sorry, it seems that I am a bit too late with my comments, so I have created https://github.com/openhab/openhab2-addons/pull/6181 with some documentation fixes and added a couple of comments to it. Please especially note that I think there is a flaw in the Thing modelling, so I would not advice anyone to start using the binding the way it is now. |
@kaikreuzer It seems @crnjan isn't on the contributors list. Can you add him? |
Certainly, done! |
* Port of the Nikobus v1 binding to v2. Signed-off-by: Boris Krivonog <boris.krivonog@inova.si> Signed-off-by: Roie Geron <roie.geron@satixfy.com>
* Port of the Nikobus v1 binding to v2. Signed-off-by: Boris Krivonog <boris.krivonog@inova.si> Signed-off-by: Roie Geron <roie.geron@satixfy.com>
* Port of the Nikobus v1 binding to v2. Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
* Port of the Nikobus v1 binding to v2. Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
* Port of the Nikobus v1 binding to v2. Signed-off-by: Boris Krivonog <boris.krivonog@inova.si> Signed-off-by: Tim Roberts <timmarkroberts@gmail.com>
* Port of the Nikobus v1 binding to v2. Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
Signed-off-by: Boris Krivonog boris.krivonog@gmail.com (github: crnjan)