Implement Nikobus Binding

[GoogleCodeExporter]

Implement an openHAB binding for the Nikobus home automation system.
Should support buttons and switch modules.

Original issue reported on code.google.com by davy.van...@gmail.com on 24 Jul 2013 at 10:45

[GoogleCodeExporter]

Please assign issue to me.

Original comment by davy.van...@gmail.com on 24 Jul 2013 at 10:45

[GoogleCodeExporter]

Original comment by teichsta on 24 Jul 2013 at 11:54

[GoogleCodeExporter]

Work is in progress. Expected to be completed during August.

Original comment by davy.van...@gmail.com on 31 Jul 2013 at 9:12

• Changed state: Started

[GoogleCodeExporter]

Original comment by teichsta on 13 Aug 2013 at 8:11

• Added labels: Version-1.3.0
• Removed labels: Version-later

[GoogleCodeExporter]

I've added a wiki page (including demo video) here: 
https://code.google.com/p/openhab/wiki/NikobusBinding

I've pushed the latest changes into my clone and the binding is now ready for 
review.
Assigning to Thomas for code review.

Original comment by davy.van...@gmail.com on 24 Aug 2013 at 6:55

• Changed state: ReadyForReview

[GoogleCodeExporter]

HI Davy,

thank you very much for this contribution. Please find below some review 
comments:

# NikobusBinding
* please move this class to org.openhab.nikobus.internal

# NikobusBindingProvider
* a BindingProvider is only responsible for providing binding configuration 
details. It should not contain Binding functionality like sending/receiving 
events. Hence postUpdate(), sendCommand() and scheduleStatusUpdateRequest() 
should be moved to the Binding

# NikobusGenericBindingProvider
* see comment for #NikobusBindingProvider
* commandReceiver, commandSender, eventPublisher
* itemRegistry is not meant to be referenced by the BindingProvider
* from my understanding the NikobusAckMonitor appears to be a candidate to take 
over the functionality

#NikobusItemBinding
* since this class is rather meant to be a binding config than a binding it 
should be renamed
* since it is abstract class it is good practice to name the class something 
like "AbstractNikobus..."

Regards,

Thomas E.-E.

Original comment by teichsta on 25 Aug 2013 at 9:17

[GoogleCodeExporter]

Makes sense.  I'll do some refactoring and let you know when done.

Original comment by davy.van...@gmail.com on 26 Aug 2013 at 5:58

[GoogleCodeExporter]

Refactored binding available for review in my clone.

Original comment by davy.van...@gmail.com on 26 Aug 2013 at 9:33

[GoogleCodeExporter]

Thanks, davy! Will start with review asap …

Original comment by teichsta on 26 Aug 2013 at 9:34

[GoogleCodeExporter]

Hi Davy,

thanks a lot for incorporating my feedback. Anyhow i have some more comments 
that weren't so obvious  to me last time:

#NikobusGenericBindingProvider
* instead of the BindingProvider the NikobusBinding should implement 
ManagedService (and hence provide the upated() method)
* There seem to be a misunderstanding due to the misleading Name of the 
BaseClass. A BindingProvider shall provide a binding configuration rather than 
a Binding. Since you are not first one we will rename this BaseClass (and 
corresponding interface) in one of the next Refactorings. Feel free to enter an 
issue for that to keep track of that issue. See your MQTT binding as an example 
how this class is meant to be used.
* L107-126 bindingChanged() and allBindingChanged() are already implemented by 
NikobusBinding and should better be implemented there

# NikobusBinding
* member eventPublisher can be removed since AbstractBinding holds an 
appropriate reference already
* L89 Bindings should not refer to the ItemRegistry directly
* L101 you should better override internalReceiveCommand() since it is called 
only if bindingsAvailable()
* L146 a binding is not allowed to make any assumptions/expectations regarding 
an item state. Since there are many operations manipulating the state and the 
postUpdate() operation isn't atomar this could lead to unexpected behaviour. 
Furthermore if you prohibit events from being send to the internal event bus 
that way the "received update" triggers are annulled. In general the event 
subscribers should decide wether to react event tough the state doesn't change. 
What was your reason to implement the method such way?
* L259 since there could potentially be many status requests being scheduled i 
would prefer using a util concurrent ThreadPool rather creating single new 
threads. The amount of threads at startup in particular can be very high which 
could cause smaller exec environments to slow down.
* L404, 411 can be removed since 

# NikobusAckMonitor
* L133 please replace printStackTrace by an appropriate logging statement

What do you think?

Cheers,

Thomas E.-E.

Original comment by teichsta on 28 Aug 2013 at 6:53

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Note: This is a repost.  Google groups had mangled my previous email response..

Thanks for taking the time to do the review Thomas, see my comments below.

#NikobusGenericBindingProvider
> * instead of the BindingProvider the NikobusBinding should implement
> ManagedService (and hence provide the upated() method)
> * There seem to be a misunderstanding due to the misleading Name of the
> BaseClass. A BindingProvider shall provide a binding configuration rather
> than a Binding. Since you are not first one we will rename this BaseClass
> (and corresponding interface) in one of the next Refactorings. Feel free to
> enter an issue for that to keep track of that issue. See your MQTT binding
> as an example how this class is meant to be used.
> * L107-126 bindingChanged() and allBindingChanged() are already
> implemented by NikobusBinding and should better be implemented there
>
> The naming is indeed a bit confusing.  I guess there is also some room for
improvement in the javadoc of the base classes.
I will try to make the necessary changes today.

> # NikobusBinding
> * member eventPublisher can be removed since AbstractBinding holds an
> appropriate reference already
>
OK, I will update..

> * L89 Bindings should not refer to the ItemRegistry directly
> * L101 you should better override internalReceiveCommand() since it is
> called only if bindingsAvailable()
>

OK, I will update..

> * L146 a binding is not allowed to make any assumptions/expectations
> regarding an item state. Since there are many operations manipulating the
> state and the postUpdate() operation isn't atomar this could lead to
> unexpected behaviour. Furthermore if you prohibit events from being send to
> the internal event bus that way the "received update" triggers are
> annulled. In general the event subscribers should decide wether to react
> event tough the state doesn't change. What was your reason to implement the
> method such way?
>

Yes, I was aware that this annuls the received update.  I did this to work
around the behaviour of the nikobus.  When communicating with the switch
module, you can only set/get the status of 6 channels/items at the same
time.  If the postUpdate is not filtered, then whenever a single channel is
switched on/off, it will always result in a status update for 6 items, even
though only one has changed.  This makes the logs difficult to read and
generates unwanted/unnecessary events.

A cleaner solution where the binding does not depend on the itemregistry
would be having a eventPublisher.postUpdateIfChanged(item, state) method.
Or we can go for the not so nice solution and send the unnecessary events...

What do you suggest here?

> * L259 since there could potentially be many status requests being
> scheduled i would prefer using a util concurrent ThreadPool rather creating
> single new threads. The amount of threads at startup in particular can be
> very high which could cause smaller exec environments to slow down.
>

Performance is important, since I run this binding on a raspberry pi myself.
Now, a status request is only scheduled in 2 scenarios:
1) when a physical button is pressed
2) one every 10 minutes for a scheduled status refresh
The threads themselves should take no longer than 1 second to
complete,which means that in practice, there is rarely ever going to be
more than 2-3 threads running at the same time and most of the time there
will even be no threads running.

Anyway, I'll change it to a cachedThreadPool, that will work fine too.

> * L404, 411 can be removed since

OK, I will update..

> # NikobusAckMonitor
> * L133 please replace printStackTrace by an appropriate logging statement
>

Oops, this should definitely not have been there...

Kind regards,

Davy

Original comment by davy.van...@gmail.com on 28 Aug 2013 at 11:24

[GoogleCodeExporter]

Hi Thomas,

A new version has been pushed to my clone.  All comments are incorporated.
The item registry link has been removed (I went for a third option ...) and the 
status update request is now using a SingleThreadExecutor.

Kind regards,

Davy

Original comment by davy.van...@gmail.com on 28 Aug 2013 at 6:01

[GoogleCodeExporter]

Hi Davy,

many thanks for your changes!

One issue with the GenericBindingProvider is still open. But i must admit i 
didn't understand that from my own comment today. What i meant by:

> A BindingProvider shall provide a binding configuration rather than a 
Binding. Since you are not first 
> one we will rename this BaseClass (and corresponding interface) in one of the 
next Refactorings. Feel
> free to enter an issue for that to keep track of that issue. See your MQTT 
binding as an example how
>  this class is meant to be used.

That the BindingProvider should simply return a BindingConfiguration which can 
than be processed by the Binding when its needed. You used that pattern in your 
MQTT Binding very well (and that is the pattern all other Bindings works, too).

I would expect a method getItemConfig(String itemName) in 
NikobusBindingProvider. NikobusGenericBindingProvider should parse the configs 
and caches them appropriately.

Does it become clearer what i've meant with my comment?

Original comment by teichsta on 31 Aug 2013 at 7:13

[GoogleCodeExporter]

Hi Thomas,

> That the BindingProvider should simply return a BindingConfiguration which 
can than be processed by the Binding when its needed.

I thought this was already the case?  I've now:
* renamed NikobusBindingProvider.getBindingConfig(String itemName) to 
NikobusBindingProvider.getItemConfig(String itemName)
* renamed AbstractNikobusItemBindingConfig to AbstractNikobusItemConfig
* moved the config storage for modules into the genericbindingprovider

If I've misunderstood again, please let me know...

Original comment by davy.van...@gmail.com on 31 Aug 2013 at 8:08

[GoogleCodeExporter]

thanks!

What belongs into that topic, too is the setting the Binding into the 
BindingConfigReader which i didn't mention explicitly before. All providers are 
injected into the binding so the NikobusBinding should not be set into the 
GenricBindingProvider.

Original comment by teichsta on 31 Aug 2013 at 9:29

[GoogleCodeExporter]

OK, this last one was a bit trickier to do, but I think it should be good now. 
Can you have a look at the code in my clone?

Original comment by davy.van...@gmail.com on 2 Sep 2013 at 10:13

[GoogleCodeExporter]

Merged binding to the default branch (see 
http://code.google.com/p/openhab/source/detail?r=c2b00ce26ddf012998cd7fb23bcffe9
8d524381c).

MANY thanks for this contribution and your patience :-)

Original comment by teichsta on 3 Sep 2013 at 12:45

• Changed state: Implemented