Review Modbus-Binding

[openhab-bot]

From teichsta on December 04, 2012 23:40:24

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.openhab.binding.modbus.tcp.master

Original issue: http://code.google.com/p/openhab/issues/detail?id=169

[openhab-bot]

From teichsta on December 04, 2012 14:41:37

i am wondering about the name iof the binding.

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

[openhab-bot]

From teichsta on December 04, 2012 14:41:56

Summary: Review Modbus-Binding (was: Modbus-Binding)

[openhab-bot]

From dbkr...@gmail.com on December 04, 2012 22:05:54

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.

[openhab-bot]

From teichsta on December 05, 2012 13:23:59

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

[openhab-bot]

From teichsta on December 05, 2012 13:25:53

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

[openhab-bot]

From dbkr...@gmail.com on December 05, 2012 21:31:05

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

[openhab-bot]

From teichsta on December 06, 2012 01:20:29

ok, good to know ...

[openhab-bot]

From teichsta on December 11, 2012 02:04:55

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

[openhab-bot]

From dbkr...@gmail.com on December 11, 2012 08:33:46

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

[openhab-bot]

From dbkr...@gmail.com on December 13, 2012 03:38:55

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

[openhab-bot]

From teichsta on December 16, 2012 05:37:30

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

Thanks in advance, Thomas E.-E.

[openhab-bot]

From dbkr...@gmail.com on December 16, 2012 06:05:34

Something like this?

modbus:slave.host=127.0.0.1

modbus:slave.length=1

modbus:slave.type=coil

[openhab-bot]

From teichsta on December 16, 2012 06:08:57

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

What is about "port, start, id"

[openhab-bot]

From teichsta on December 16, 2012 07:39:43

Added the documentation by myself!

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

Status: Implemented