Questions about best practices implementing bindings

codestyle
code
binding
Tags: #<Tag:0x00007f1837e3c0a0> #<Tag:0x00007f182635fe68> #<Tag:0x00007f182635fd00>

(Hilbrand Bouwkamp) #1

I’ve been building some bindings and review code recently and looked at other reviews and have some questions about best practices:

Threads

It is in general not allowed to create your own threads (Point 2 of http://docs.openhab.org/developers/development/guidelines.html#d-runtime-behavior). The point has an exemption clause to start a discussion on the forum. For binding developers and reviewers it would great if this is made more specific. Because I don’t see a lot of discussions but see people creating their own threads. Meaning threads are simply created anyway. Therefore I also want to start the discussion and see if the usage can be made more specific. Here are 2 recent cases I encountered:

  • In the Rotel PR there is a thread needed that just loops forever, blocks until input from the serial bus is received, handles it and blocks again. The suggested solution was not to use schedule or threadpool but: Executors.newSingleThreadScheduledExecutor(). It this ok? and why? and how should it be implemented to not have dangling threads?
  • In the Blebox PR in discovery the local network is scanned. If done sequential this would take 8 minutes, therefore the implementation does this concurrently with 50 threads so it only takes 10 seconds. Here also the Executors.newFixedThreadPool(THREADS_NUMER) is used. Is this the right approach? and if not how should it be implemented?

Catching Exception

Very regular I see bindings implement catch(Exception e). This is considered bad. In general these catches are done in background threads (for example to poll the state of a device) to make sure the thread doesn’t get killed. One way is to only catch checked exceptions, but that leaves out the RuntimeExceptions. What is the preferred solution: Only catch checked exceptions and let the RuntimeExceptions stop the thread or also catch RuntimeExceptions?

Logging to error.

According to the coding style logging to error should only be done when something is wrong with the setup. My interpretation of this is that a binding should simply never log to error, as a single binding failing isn’t great but not a severe bug. Is this correct?

Checking configuration parameters

If a binding configuration parameter has a default and is required the value will never be null when the initialize of the binding is called because otherwise a configuration validator will already have thrown an exception. Is it therefore necessary to tests these values on null values? Or can these checks be omitted? (There are other check to think of, like min) I would say they can be omitted making the code simpler.


(namraccr) #2

This is correct. Bindings should not generally log to error, as stated in the logging guidelines.


(Hilbrand Bouwkamp) #3

I would really love to have some feedback on these questions. Maybe our BDFL @Kai can spare some of his precious time and provide some guidance.


(Kai Kreuzer) #4

Hi @hilbrand,

The short answer about best practices is: Bindings should follow all ESH APIs and guidelines, the developer docs can be found here: https://www.eclipse.org/smarthome/documentation/development/bindings/how-to.html

As with all documentation: It is never complete and might be outdated - so questions around it should generally be asked at https://www.eclipse.org/forums/index.php/f/271/ - so may I ask you to repeat your (still open) questions there (Luckily there is such a thing as copy&paste :wink: ).
And please note: On ESH I am no BDFL, but just one equal committer besides the others that all follow a clear governance process.

Thanks!
Kai


(Hilbrand Bouwkamp) #5

I’ve posted the message on the Eclipse Smart Home forum. I had thought about it before, but because I assumed most binding developers hang around this forum and would also be served by the answers, I posted it here.

I did look up the documentation but found this information lacking or not clear. If I get some useful answers I’ll will try to add them to the documentation…