[flicbutton] Initial contribution FlicButton Binding

[pfink]

Ready for review. Binary is available for download here.

Signed-off-by: Patrick Fink mail@pfink.de

[pfink]

Mhm, PR build did not fail due to errors in the binding, could somebody may restart it?

[martinvw]

Mhm, PR build did not fail due to errors in the binding, could somebody may restart it?

I added the rebuild tag

[pfink]

Mhm, PR build did not fail due to errors in the binding, could somebody may restart it?

I added the rebuild tag

Failed again, this time due to StackOverflow exception :(

[fwolter]

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

EDIT: Is there a reason why you put some classes not into the internal package?

[openhab-bot]

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

https://community.openhab.org/t/how-to-integrate-flic-buttons/4468/66

[openhab-bot]

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

https://community.openhab.org/t/thinghandlerservice-background-discovery/118240/3

[fwolter]

@pfink What's the state of this PR? Is the only open topic about the ThingHandlerService?

[pfink]

@pfink What's the state of this PR? Is the only open topic about the ThingHandlerService?

Regarding ThingHandlerService, I tried to use it and it turned out not to be easy, as it raises quite some new challenges. As stated above, I currently don't see how I can refactor the current code in a way that it would be an overall improvement. So I'd recommend to merge as is.

The only other open topic was the setStatusBasedOnBridge method in ChildThingHandler. You asked me whether it's necessary. Unfortunately, I didn't remember why I introduced it and with the "real" binding, it was not so easy to test all cases. So to answer your question, I started to create a test binding to make some tests how the default behavior of openHAB is and how it differs from ChildThingHandler. Unfortunately, I lost track of it and cannot continue to work on it in the next days. I plan to continue working on this, just not sure when I have time, maybe mid of August. Generally, I agree to you that we should critically follow up on this, and in case setStatusBasedOnBridge is necessary, we should have a look why and whether there is an issue with the default behavior that we should fix in the core. But I don't think it's a very important point, so we could also merge now and if setStatusBasedOnBridge turns out not be necessary later, I could do a second PR removing it. Alternatively, I could just remove it now, it obviously wouldn't break anything important.

Long story short: The decision is up to you whether we merge now. From my point of view it would be fine.

[pfink]

Hi,
sorry that it took so long, but it needed quite a bit amount of time to do the requested changes. I think everything is done now. The remaining SAT warnings are false-positives. The DCO failure is due to merge commits.

That said, I want to say that I'm a little bit concerned regarding the enforcement of more and more concurrency usage in bindings. One example why: The default implementation of a configuration update is to dispose and reinitialize the handler. As I'm forced to use the provided scheduler, I only have control of the started threads via the Future API. So in my dispose() method, I can only use future.cancel() to abort my started threads, but this method is non-blocking and it's not guaranteed (even not likely) that the cancelled threads have actually ended before the dispose() method ends and before new threads are started again by the initialize method. In fact, threads canceled with future.cancel() will likely still do operations, as this method only flags the threads as interrupted and it's not checked after every line of code whether the thread was interrupted, so it will continue running for some time. Which means I may have quite a bunch of "ghost" threads running in parallel with new threads, doing things on the same device/object in parallel - and that happens even if I just do a simple thing like a configuration update. It's very hard to keep the overview here and be sure that nothing goes wrong if that happens, and you can never be sure whether your code is right even if you tested these cases.

I don't have the solution here, but just wanted to share my thoughts whether it's really so good when too much concurrency handling is put into the hands of binding developers - I would feel much better if I could prevent that old threads are still running when the handler is reinitialized, but currently I don't see any possibility to guarantee that. Do you see any, @fwolter? If you have any insights, I'm happy to get more input about that topic - maybe there were already discussions in the community about it?

Anyhow, I'm not aware of any existing issues with the current code, so from my point of view the binding is ready for merge.

[kaikreuzer]

Many thanks @pfink, great to see that we are getting close to merging it!

Code and documentation looks good to me - I just have a few small comments, see below.
Once addressed, I'll leave the merge to @fwolter, if he sees his comments being addressed.

[fwolter]

@pfink I fully agree on your concerns about concurrency. The majority of bindings doesn't handle the case when the Thing's lifecycle is executed too fast. I'm not aware this has been discussed in detail. Though, it doesn't seem to be a big deal in practice.

[pfink]

Thanks @kaikreuzer for your review, your comments are resolved now.

@fwolter it's good to hear that at least it doesn't seem to be a big issue in practice. Merge is up to you now!

[fwolter]

LGTM

Now, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website