search

openhab / openhab1-addons

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

[velux] Various bug fixes and enhancements #5833

Closed gs4711 closed 4 years ago

gs4711 commented 5 years ago

The following changes are provided:

9037568 commented 5 years ago

If the plan is to create an OH2 binding, that should be done in the OH2 repo.

gs4711 commented 5 years ago

If the plan is to create an OH2 binding, that should be done in the OH2 repo.

Yes, it will be fully OH2 compliant with a separate initialization module for dealing with things.

This bug fix is necessary as OH2 calls the configuration method with a null pointer instead of an empty Dictionary.

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/130

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/use-velux-binding-in-oh-1-x/71975/20

gs4711 commented 5 years ago

Thx, @wborn, for all the license adaptions! Now, this PR is adapted to the already approved #5843. Therefore, only minor changes have been left over.

openhab-bot commented 4 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/298

gs4711 commented 4 years ago

Hopefully one of the maintainers is reading this: the list of changes is mentioned/updated in the initial comment.

kaikreuzer commented 4 years ago

Thx, @wborn, for all the license adaptions!

Were those somehow lost again? I see a lot of non-standard license texts and EPLv1 mentioned in them. Could you check and possibly re-run a mvn license:format?

kaikreuzer commented 4 years ago

Yes, it will be fully OH2 compliant with a separate initialization module for dealing with things.

I am not fully clear on what that means. As it is a 1.x binding, it must not refer to Things etc., so I do nut understand why you have added things files here and removed the binding configuration.

This bug fix is necessary as OH2 calls the configuration method with a null pointer instead of an empty Dictionary.

This PR seems to do much more that this bugfix by now...

gs4711 commented 4 years ago

Could you check and possibly re-run a mvn license:format?

Unfortunatelly rhe re-run of a mvn license:format does not lead to any changes???

gs4711 commented 4 years ago

This PR seems to do much more that this bugfix by now...

Yes, of course: Due to the [feedback] (https://community.openhab.org/t/velux-new-openhab2-binding-feedback-welcome/32926/354) of users with OH1 and OH2, this PR contains nine bugfixes and ten extensions since 16/03/2019 in addition.

gs4711 commented 4 years ago

I see a lot of non-standard license texts and EPLv1

Thanks for this notice: Now, those files are manually adapted.

kaikreuzer commented 4 years ago

Thanks for the quick update, looks much better now. So my remaining question is this one from above:

As it is a 1.x binding, it must not refer to Things etc., so I do nut understand why you have added things files here and removed the binding configuration.

gs4711 commented 4 years ago

So my remaining question is this one from above:

As it is a 1.x binding, it must not refer to Things etc., so I do nut understand why you have added things files here and removed the binding configuration.

The separation of the binding configuration into things files originated in the strange behaviour of paperUI which does not handle this 1.x binding properly. This part of the change (which came from the adoptions towards a full 2.x binding) could be skipped as it did not resolve this issue completely.

kaikreuzer commented 4 years ago

This part of the change (which came from the adoptions towards a full 2.x binding) could be skipped

Yes, I would suggest this as Thing files must not be part of any 1.x binding and all configuration must be done on the binding level (as it is before this PR).

gs4711 commented 4 years ago

Yes, I would suggest this as Thing files must not be part of any 1.x binding and all configuration must be done on the binding level (as it is before this PR).

Adapted as suggested.

gs4711 commented 4 years ago

Thanks, @kaikreuzer, a lot!

openhab-bot commented 4 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/364

gs4711 commented 4 years ago

@kaikreuzer ,

first of all, thanks for the merge of the #5833.

I am not fully clear on what that means. As it is a 1.x binding, it must not refer to Things etc., so I do not understand why you have added things files here and removed the binding configuration.

Sorry to bother you but after removing the ESH/things stuff the PaperUI does not provide any thing information for this compat binding. Therefore, having eliminated the bridge and thing declaration, the config-file-based configuration is the only way to handle it with PaperUI.

An ideas?

Best regards, Guenther

kaikreuzer commented 4 years ago

That's exactly how 1.x bindings work. Note that the main feature of 2.x bindings is to bring thing & Paper UI support, which does not exist in 1.x.

andrewfg commented 4 years ago

Hi Guenter @gs4711 — How are you getting on with your Velux OH v2 binding? In the meantime, I have migrated the v1 version of the NeoHub binding to v2, and also written a completely new v2 binding for Siemens smart thermostats. And now the only equipment in my home which is not on OH v2 are the Velux roof windows. So I have some skills, some motivation, and the test equipment (KLF200 and windows), to help you with the v1 -> v2 migration. Do you already have a beta of the v2 binding somewhere that I can play with?

gs4711 commented 4 years ago

Hello Andrew,

the oh2 binding is about to be finished in the next days (PR#2531 has already run successfully through Jenkis).

The oh2 features like discovery are not yet stable so that I will dig deeper into it before announcing it to the list.

But, in fact, with PaperUI it already looks good to me ;-)

Regards, Guenther

Von: Andrew Fiddian-Green notifications@github.com Gesendet: Donnerstag, 8. August 2019 16:38 An: openhab/openhab1-addons openhab1-addons@noreply.github.com Cc: Guenther Schreiner guenther.schreiner@smile.de; Mention mention@noreply.github.com Betreff: Re: [openhab/openhab1-addons] [Velux] Bugfixes (Empty list of items, HTTP/JSON, sending error, binding shutdown/removal, channel values) plus Enhancements (documentation, new channels timestamp+reload, optional inversed actuator values, support of Somfy d

Hi Guenter @gs4711 https://github.com/gs4711 — How are you getting on with your Velux OH v2 binding? In the meantime, I have migrated the v1 version of the NeoHub binding to v2, and also written a completely new v2 binding for SIemens smart thermostats. And now the only equipment in my home which is not on OH v2 are the Velux roof windows. So I have some skills, some motivation, and the test equipment (KLF200 and windows), to help you with the v1 —v2 migration. Do you already have a beta of the v2 binding somewhere that I can play with?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openhab/openhab1-addons/pull/5833?email_source=notifications&email_token=ADDOI2FZJOWSIU6YZB2KFH3QDQVS7A5CNFSM4G657JTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD332MTA#issuecomment-519546444 , or mute the thread https://github.com/notifications/unsubscribe-auth/ADDOI2BAB5CP5FQ7I5YHW5LQDQVS7ANCNFSM4G657JTA . https://github.com/notifications/beacon/ADDOI2HVNEGIZO2HHPCNRODQDQVS7A5CNFSM4G657JTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD332MTA.gif

andrewfg commented 4 years ago

I notice that PR#4416 seems also to be an OH2 binding for Velux equipment ?? - is there some coordination with that?