search

openhab / openhab1-addons

Add-ons for openHAB 1.x
Eclipse Public License 2.0
3.43k stars 1.7k forks source link

[Velux] Add support for KLF200 firmware v2 #5690

Closed gs4711 closed 5 years ago

gs4711 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Binding updated:

9037568 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

due to tabulator instead of spaces... fixed.

gs4711 commented 5 years ago

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

9037568 commented 5 years ago

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

You don't need to worry about those.

gs4711 commented 5 years ago

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

9037568 commented 5 years ago

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

gs4711 commented 5 years ago

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 commented 5 years ago

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 ?

gs4711 commented 5 years ago

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

gs4711 commented 5 years ago

A few final changes.

Hello Chris, is there anything I still should address?

openhab-bot commented 5 years ago

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 commented 5 years ago

Hello Chris, is there anything I still should address?

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

9037568 commented 5 years ago

Thanks, @gs4711 !