[Velux] Add support for KLF200 firmware v2 #5690

gs4711 · 2018-10-31T18:42:23Z

The Velux 2.x firmware provide full access to any IO-Home compatible devices not limited to Velux and includes an important feature: It is now possible to manipulate the settings of any IO-Home controled device explicitely without the previous scene definition.

openhab-bot · 2018-12-01T09:26:50Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/velux-oh1-binding-with-full-control-of-any-io-home-controled-device-beta-tester-wanted/55434/2

openhab-bot · 2019-01-02T15:03:56Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/velux-new-openhab2-binding-feedback-welcome/32926/90

mira2000 · 2019-01-02T18:16:49Z

Binding updated:

functionality: full support and both Velux firmware versions v1 and v2
update: support for openhab1-addons 1.14.0-SNAPSHOT
update: copyright adapted for new year
fixed: memory leak within product refresh
fixed: length of serial numbers

9037568 · 2019-01-04T04:53:28Z

A few brief comments while I continue to review...

All classes should have @author and @since attributes
public code units can't be changed to private
public code units can't be removed, but need to be marked as deprecated
all code needs to be run through the Eclipse formatter

gs4711 · 2019-01-04T08:00:00Z

thanks for the quick response. Some questions regarding your comment:

(a) Refering to discussion Usage of @since tags in JavaDocs I was not aware about the @since attribute. Shall I re-introduce it within any file?

(b)+(c) question: due to the new functionality of the Velux bridge, the code has got many changes due to restructuring for supporting two different protocols. Is your comment regarding compatibility to the latest version - or do I miss your point?

(d) Yes, every code segment has gone through the Eclipse formatter. But there are special settings (i.e. 4 spaces instead of tabs) coming from the OH2 guideline.

9037568 · 2019-01-05T02:51:23Z

I wasn't aware of the change in recommendation surrounding @since. Thanks for the pointer.

Many changes: no problem. Destroying public code: problem. You need to preserve the existing public units and mark them deprecated.

The formatting is improved with your latest push. But there are still a number of changes when the Eclipse formatter is run on the code base.

gs4711 · 2019-01-05T07:28:47Z

Regarding "Destroying public code: problem": will have to check the refactoring against thr recent version.

Regarding formatting: sorry for the inconvenience - could you please give me a clue (any pointer appreciated!) how to check it on my own?

BTW there is a problem (dangling constraints) with the Eclipse openHAB installer with the lastest version - do you know where to mention?

9037568 · 2019-01-08T00:25:15Z

Regarding formatting: sorry for the inconvenience - could you please give me a clue (any pointer appreciated!) how to check it on my own?

All this should take is a Source -> Format in Eclipse.

BTW there is a problem (dangling constraints) with the Eclipse openHAB installer with the lastest version - do you know where to mention?

I'd start in the forum, probably.

9037568 · 2019-01-16T04:09:26Z

Looks like you committed some files outside of the Velux binding. You'll need to remove those changes.
I hope to finish this review in the next couple of days...

gs4711 · 2019-01-20T18:25:37Z

Looks like you committed some files outside of the Velux binding. You'll need to remove those changes.

Hopefully fixed. In addition, the new year is set again within the copyrights.
But, now, some new issues with the OSGI-INF occur....

gs4711 · 2019-01-20T18:33:00Z

But, now, some new issues with the OSGI-INF occur....

due to tabulator instead of spaces... fixed.

gs4711 · 2019-01-20T19:02:24Z

sigh, now, an error occurs within the official part of the openHAB Weather Binding....

9037568 · 2019-01-20T22:22:29Z

sigh, now, an error occurs within the official part of the openHAB Weather Binding....

You don't need to worry about those.

gs4711 · 2019-01-21T06:26:53Z

What about the new copyright statements (EPLv1 towards EPLv2)? Is this considerable as well?

9037568 · 2019-01-22T00:23:47Z

As far as I know, there's no need to do anything with EPL either.

9037568

Please don't forget to remove the extraneous changes to files outside the Velux binding.

bundles/binding/org.openhab.binding.velux/README.md

...enhab.binding.velux/src/main/java/org/openhab/binding/velux/bridge/VeluxBridgeActuators.java

...nhab.binding.velux/src/main/java/org/openhab/binding/velux/bridge/slip/io/SSLconnection.java

...enhab.binding.velux/src/main/java/org/openhab/binding/velux/bridge/slip/SlipVeluxBridge.java

gs4711 · 2019-01-24T15:07:26Z

Thank you, Chris, @9037568, for the intensive review and feedback. I do really appreciated your heavy work on it!

All but six comments are resolved now: Next commit/push will still include the trailing three comment lines in most of the file like

/**
 * end-of-VeluxProductReference.java
 */

which will be eliminated in all files in the additional push following.

gs4711 · 2019-01-24T15:20:06Z

As last time, the openHAB Weather Binding prevented the full run of Jenkins:

16:14:37 [INFO] openHAB Weather Binding ............................ FAILURE [ 0.824 s]

Shall I eliminate this binding for the time being from pom.xml ?

@9037568

…ub: gs4711). Integrated code harmonisation (thanks to @9037568 for the intensive review and feedback). Removal of any trailing three comment lines.

9037568

A few final changes.
Also, please run the Eclipse formatter again after changes are completed. Thanks!

...enhab.binding.velux/src/main/java/org/openhab/binding/velux/bridge/slip/SlipVeluxBridge.java

...ng/org.openhab.binding.velux/src/main/java/org/openhab/binding/velux/bridge/VeluxBridge.java

...hab.binding.velux/src/main/java/org/openhab/binding/velux/handler/VeluxBridgeHandlerOH1.java

...org.openhab.binding.velux/src/main/java/org/openhab/binding/velux/internal/VeluxBinding.java

gs4711 · 2019-01-28T08:22:28Z

And completely run through Eclipse 2018-09 formatter. Thanks!

gs4711 · 2019-02-04T14:45:32Z

A few final changes.

Hello Chris, is there anything I still should address?

openhab-bot · 2019-02-10T21:15:49Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/velux-oh1-binding-with-full-control-of-any-io-home-controled-device-beta-tester-wanted/55434/6

9037568 · 2019-02-11T00:22:40Z

Hello Chris, is there anything I still should address?

I need to make one more pass through to review your latest changes.

9037568 · 2019-02-16T14:22:03Z

Thanks, @gs4711 !

gs4711 mentioned this pull request Oct 31, 2018

[velux] Support for new firmware v2 added #5689

Closed

9037568 changed the title ~~[velux] Support for KLF200 firmware v2 added~~ [Velux] Add support for KLF200 firmware v2 Nov 25, 2018

9037568 added enhancement review-in-progress labels Nov 25, 2018

9037568 suggested changes Jan 22, 2019

View reviewed changes

9037568 added awaiting-feedback and removed review-in-progress labels Jan 22, 2019

gs4711 closed this Jan 25, 2019

Signed-off-by: Guenther Schreiner <[email protected]> (gith…

a886792

…ub: gs4711). Integrated code harmonisation (thanks to @9037568 for the intensive review and feedback). Removal of any trailing three comment lines.

gs4711 reopened this Jan 25, 2019

gs4711 added 2 commits January 27, 2019 15:51

recent review remarks about coding style and documentation

4ed9de5

using LOGGER as static logger

68f475b

9037568 suggested changes Jan 28, 2019

View reviewed changes

changes regarding loglevel

788bb3f

fix: clean handling of unrare situation where bridge goes into idle mode

9b9a344

9037568 merged commit 9b9a344 into openhab:master Feb 16, 2019

9037568 removed the awaiting-feedback label Feb 16, 2019

9037568 added this to the 1.14.0 milestone Feb 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Velux] Add support for KLF200 firmware v2 #5690

[Velux] Add support for KLF200 firmware v2 #5690

gs4711 commented Oct 31, 2018

openhab-bot commented Dec 1, 2018

openhab-bot commented Jan 2, 2019

mira2000 commented Jan 2, 2019

9037568 commented Jan 4, 2019

gs4711 commented Jan 4, 2019 •

edited

Loading

9037568 commented Jan 5, 2019

gs4711 commented Jan 5, 2019

9037568 commented Jan 8, 2019

9037568 commented Jan 16, 2019

gs4711 commented Jan 20, 2019

gs4711 commented Jan 20, 2019

gs4711 commented Jan 20, 2019

9037568 commented Jan 20, 2019

gs4711 commented Jan 21, 2019

9037568 commented Jan 22, 2019

9037568 left a comment

gs4711 commented Jan 24, 2019

gs4711 commented Jan 24, 2019

9037568 left a comment

gs4711 commented Jan 28, 2019

gs4711 commented Feb 4, 2019

openhab-bot commented Feb 10, 2019

9037568 commented Feb 11, 2019

9037568 commented Feb 16, 2019

[Velux] Add support for KLF200 firmware v2 #5690

[Velux] Add support for KLF200 firmware v2 #5690

Conversation

gs4711 commented Oct 31, 2018

openhab-bot commented Dec 1, 2018

openhab-bot commented Jan 2, 2019

mira2000 commented Jan 2, 2019

9037568 commented Jan 4, 2019

gs4711 commented Jan 4, 2019 • edited Loading

9037568 commented Jan 5, 2019

gs4711 commented Jan 5, 2019

9037568 commented Jan 8, 2019

9037568 commented Jan 16, 2019

gs4711 commented Jan 20, 2019

gs4711 commented Jan 20, 2019

gs4711 commented Jan 20, 2019

9037568 commented Jan 20, 2019

gs4711 commented Jan 21, 2019

9037568 commented Jan 22, 2019

9037568 left a comment

Choose a reason for hiding this comment

gs4711 commented Jan 24, 2019

gs4711 commented Jan 24, 2019

9037568 left a comment

Choose a reason for hiding this comment

gs4711 commented Jan 28, 2019

gs4711 commented Feb 4, 2019

openhab-bot commented Feb 10, 2019

9037568 commented Feb 11, 2019

9037568 commented Feb 16, 2019

gs4711 commented Jan 4, 2019 •

edited

Loading