Review Modbus-Binding

[GoogleCodeExporter]

Review the Modbus-Binding.

Doku is available here: http://code.google.com/p/openhab/wiki/ModbusTcpBinding
Code is available here: 
http://code.google.com/r/dbkrasn-khome/source/browse#hg/bundles/binding/org.open
hab.binding.modbus.tcp.master

Original issue reported on code.google.com by teichsta on 4 Dec 2012 at 10:40

[GoogleCodeExporter]

i am wondering about the name iof the binding.

Why didn't you simply choose binding.modbus? Is there more to come?

Original comment by teichsta on 4 Dec 2012 at 10:41

[GoogleCodeExporter]

Original comment by teichsta on 4 Dec 2012 at 10:41

• Changed title: Review Modbus-Binding

[GoogleCodeExporter]

I have no plans for other Modbus bindings at the moment - I believe tcp master 
is of the most demand.
On the naming.
Modbus device can act as master or slave, that's why there is "master" suffix. 
If one would like to create a slave impl it will fit existing naming. Also 
there are other flavors of Modbus protocol implementation like RTU and ASCII. 
Potential implementations will fit this naming too.

Original comment by dbkr...@gmail.com on 5 Dec 2012 at 6:05

[GoogleCodeExporter]

ok!

Here are some review comments:

# general
* since there are no plans to create other modbus bindings we should rename the 
binding to org.openhab.binding.modbus (yagni). If there will be different 
transport's available we could think about bundle-fragments instead of complete 
new binding-bundles.
* remove unused comm.jar, javax.comm.properties, win32com.dll or are they used 
somewhere?

# ModbusBindingProvider
* extract SLAVE_DATA_TYPES and change to an enum
* rename parameter name to "itemName"

# ModbusBinding
* rename modbusConfigCache to modbusSlaveChange since it seems to contain 
Slaves not Configs
* extract modbusConfigCache, ManagedService, updated() to new class 
ModbusConfiguration let ModbusBinding and ModbusPoll use this Configuration 
(see org.openhab.io.gcal.GCalConfiguration for an example)
* extract inner class ModbusSlave
* remove reference to ItemRegistry. Iterating through those items that have a 
modbus binding configured should be done by iterating over all ItemProviders 
and calling GenericBindingProvider.getItemNames().
* updateSlaves() should belong to ModbusPoll rather than ModbusBinding
* connect() (L423) - exception should be logged (at least on debug level) to 
make debugging possible
* setRegister() - use "command instanceof xxxType" instead of the classname 
equality to take class hierarchy into account
* setRegister() - use if + else since one value can either be INC or DEC but 
never both (same applies to UP/DOWN and ON/OFF
* update() - use if + else if instead of if-only since type can only have one 
type at a time

# ModbusPoll
* getName() should return a more meaningful name e.g. "Modbus Polling Service"?
* ModusPoll should only be started, when there devices configured. You could 
accomplish that by providing a more fine grained implementation of 
isProperlyConfigured()

# modbuspoll.xml
* both property configurations seem to be unnecessary

Original comment by teichsta on 5 Dec 2012 at 9:23

[GoogleCodeExporter]

how does the org.openhab.binding.khome.sensors bundle fits into the picture?

Original comment by teichsta on 5 Dec 2012 at 9:25

[GoogleCodeExporter]

org.openhab.binding.khome.**sensors is my persomal experimental code and is
unrelated to Modbus binding

Original comment by dbkr...@gmail.com on 6 Dec 2012 at 5:31

[GoogleCodeExporter]

ok, good to know ...

Original comment by teichsta on 6 Dec 2012 at 9:20

[GoogleCodeExporter]

could you manage to finish the binding until Friday (14.12) ?

Original comment by teichsta on 11 Dec 2012 at 10:04

[GoogleCodeExporter]

yes, it's done, now doing some testing on my home system

Original comment by dbkr...@gmail.com on 11 Dec 2012 at 4:33

[GoogleCodeExporter]

done, code committed. Please note that "modbustcpmaster" tags in openhab.cfg 
and items file became "modbus". Wike page updated accordingly

Original comment by dbkr...@gmail.com on 13 Dec 2012 at 11:38

[GoogleCodeExporter]

could you give me the default configuration input for the openhab_default.cfg?

Thanks in advance, Thomas E.-E.

Original comment by teichsta on 16 Dec 2012 at 1:37

[GoogleCodeExporter]

Something like this?

#modbus:slave.host=127.0.0.1
#modbus:slave.length=1
#modbus:slave.type=coil

Original comment by dbkr...@gmail.com on 16 Dec 2012 at 2:05

[GoogleCodeExporter]

yes, if you would add some explanation for each param :-) (see the given params 
as example)!

What is about "port, start, id"

Original comment by teichsta on 16 Dec 2012 at 2:08

[GoogleCodeExporter]

Added the documentation by myself!

Merged modbus-binding to default branch (see 
http://code.google.com/p/openhab/source/detail?r=05234f6e95c1d99cac11f04ef0676a0
b3257a2d1)

Original comment by teichsta on 16 Dec 2012 at 3:39

• Changed state: Implemented