RFXCOM Binding

[GoogleCodeExporter]

Clone repo: http://code.google.com/r/paulianttila-ihc-binding/

TemperatureHumidity and Lightning2 packet types are currently implemented. 
Community support is needed to add more packet types.

Python scripts which support many other packets:
http://code.google.com/p/rfxcmd/source/browse/trunk/rfxcmd.py

Original issue reported on code.google.com by pauli.an...@gmail.com on 8 Jan 2013 at 4:04

[GoogleCodeExporter]

Original comment by kai.openhab on 14 Jan 2013 at 10:17

• Changed state: Accepted

[GoogleCodeExporter]

Hi Pali, thanks for this work, here are my review comments - let me know, if 
you have any questions!

OSGI-INF:
- both connection.xml and unbinding.xml define the same service pid - only one 
of them will have the updated() method called

MANIFEST.MF:
- Should export package org.openhab.binding.rfxcom
- Version should be 1.2.0.qualifier

RFXComConnection:
- line 93: Should catch IllegalArgumentException and wrap it in a 
ConfigurationException
-line 96: Should throw a ConfigurationException here
- line 113: connect() should throw a more specific exception
- line 129: This error log is most likely not helpful to the user - either 
completely remove it or give some context of what went wrong here
- line 131: This can throw IllegalArgumentException as well
- line 139: Why is this necessary? If really, add a comment
- line 143: IAException again

RFXComDataConverter:
- line 72: Should mention in which case an emtpy string is returned as id 
(wouldn't null be more appropriate?)

RFXComGenericBindingProvider:
- line 54: Not clear from the examples whether "Command" is a static string to 
put here, or if it just stands for "any command", such as "ON" or "OFF".
- line 88ff: Why is this in comment?
-line 173: Class should start with a capital letter

RFXComInBinding:
- As you do not use the execute() method at all, you do not need to extend 
AbstractActiveServiceBinding at all. My suggestion would be to move the code 
into RFXComOutBinding and rename that to RFXComBinding, so you only have one 
binding class, handling both cases at once. This will also directly solve the 
duplicate service.pid issue.
- Bindings should NOT make use of the item registry, i.e. methods like 
getItemFromItemName are not permitted - this breaks the "statelessness" of 
bindings, which is not nice. So the only place where you can get hold of the 
item type is the method 
RFXComGenericBindingProvider.processBindingConfiguration - in consequence, you 
should store this information in your BindingConfig for later use.

RFXComOutBinding:
- line 112: Are you sure that you want to treat status updates just like 
commands? I would assume that it is better to completely remove the method 
internalReceiveUpdate here
- line 161: Message should give some context - something like "cannot execute 
command because no binding provider was found for itemname 'xx'"
- line 177: Warning should be more user-focused like "RFXCom controller is not 
initialized"
- line 232: Again, more helpful messages for the user in case of an error 
(maybe catch different exceptions than just "Exception" to be able to provide 
more details)
- line 280: ditto

RFXComUtils:
- why is this in a different namespace? Would expect it under 
org.openhab.binding.rfxcom.internal. (same for rfxcom.connector/messages 
classes)

RFXComMessageReceivedEvent:
- Javadocs missing

RFXComSerialConnector:
- line 170: do proper logging here

RFXComTcpConnector:
- Replace System.out by proper logging

RFXComBaseMessage:
- shouldn't this class be abstract?
- line 111: shouldn't this method be abstract?
- line 127+130: Replace System.out by proper logging

Original comment by kai.openhab on 2 Feb 2013 at 10:43

[GoogleCodeExporter]

Original comment by kai.openhab on 11 Feb 2013 at 9:30

• Changed title: RFXCOM Binding
• Changed state: Started
• Added labels: Version-1.2.0

[GoogleCodeExporter]

Original comment by kai.openhab on 11 Feb 2013 at 9:30

• Added labels: Type-Feature
• Removed labels: Type-Review

[GoogleCodeExporter]

All other remarks should now be fixed except...

"Bindings should NOT make use of the item registry, i.e. methods like 
getItemFromItemName are not permitted - this breaks the "statelessness" of 
bindings, which is not nice. So the only place where you can get hold of the 
item type is the method 
RFXComGenericBindingProvider.processBindingConfiguration - in consequence, you 
should store this information in your BindingConfig for later use."

Do you really think that every binding should store item type information 
locally? I have used same method on other bindings as well. Actually I copied 
this method from http binding and it seems to be used on other bindings as well.

Original comment by pauli.an...@gmail.com on 9 Mar 2013 at 10:24

[GoogleCodeExporter]

Binding does not use ItemRegistry anymore.

Original comment by pauli.an...@gmail.com on 16 Mar 2013 at 3:31

[GoogleCodeExporter]

Thanks! You are right that other bindings are doing it wrong - we have created 
issue 211 to fix them as well.

Original comment by kai.openhab on 21 Mar 2013 at 10:01

[GoogleCodeExporter]

Original comment by kai.openhab on 21 Mar 2013 at 10:01

[GoogleCodeExporter]

I have just taken over the code to the central repository, thanks for your 
efforts!
May I now just ask you to fill the wiki page about the binding, which explains, 
what you can do with it and how you have to configure it?
https://code.google.com/p/openhab/wiki/RFXCOMBinding

Cheers,
Kai

Original comment by kai.openhab on 22 Mar 2013 at 9:53

• Changed state: Implemented