sabriallani / openhab

Automatically exported from code.google.com/p/openhab
GNU General Public License v3.0
0 stars 0 forks source link

Binding: OpenSprinkler Pi #392

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
I have developed a binding for OpenSprinkler Pi. This allows for water valves 
to be controlled from OpenHAB. I posted a summary of my work in this forum 
thread:

https://groups.google.com/d/msg/openhab/alISfEmI5Cg/EMmSJBSjv6AJ

And I have created my own clone of the repo for the code I have developed here:
http://code.google.com/r/jonathan-opensprinklerpi/

Any questions feel free to ask.

Original issue reported on code.google.com by jonathan@jonathangiles.net on 4 Aug 2013 at 8:55

GoogleCodeExporter commented 9 years ago
Hi, i've just created the blank Wiki-Page 
http://code.google.com/p/openhab/wiki/OpenSprinkler for the documentation. 
Write-Access should be granted now.

Original comment by teichsta on 4 Aug 2013 at 9:01

GoogleCodeExporter commented 9 years ago
I've just published a first draft wiki content at the page you created. 

I should note two things about the link that you created:

1) The name should be OpenSprinklerPi, not OpenSprinkler. This is because 
OpenSprinkler also exists and is not what this binding controls. This binding 
is specifically for the OpenSprinkler Pi edition.

2) The wiki link is not consistent with the other binding links, but it isn't a 
big deal. For example, rather than OpenSprinkler being the link, to be 
consistent it would be named OpenSprinklerPiBinding.

Original comment by jonathan@jonathangiles.net on 4 Aug 2013 at 9:13

GoogleCodeExporter commented 9 years ago
I've just quickly taken a (very poor quality) video of the binding in action, 
to prove that it does actually work and has been tested. Here is the YouTube 
video to prove it:

https://www.youtube.com/watch?v=RgZo8Rg0yEs

Original comment by jonathan@jonathangiles.net on 4 Aug 2013 at 9:34

GoogleCodeExporter commented 9 years ago
Cool, thanks! I have added it to the openHAB YouTube playlist :-)
Btw, you should better hold your phone in landscape mode when producing videos!

Original comment by kai.openhab on 4 Aug 2013 at 9:41

GoogleCodeExporter commented 9 years ago
Yeah, perhaps I should refilm the video....?

Original comment by jonathan@jonathangiles.net on 4 Aug 2013 at 9:43

GoogleCodeExporter commented 9 years ago
At least as soon as you have real sprinklers attached :-)

Original comment by kai.openhab on 4 Aug 2013 at 9:47

GoogleCodeExporter commented 9 years ago
Ok, updated video: https://www.youtube.com/watch?v=lT0uxFlwu9s :-)

Original comment by jonathan@jonathangiles.net on 4 Aug 2013 at 9:56

GoogleCodeExporter commented 9 years ago
Thanks, I have replaced it in the playlist!

Original comment by kai.openhab on 5 Aug 2013 at 8:41

GoogleCodeExporter commented 9 years ago
Great work!
I have been using the standard opensprinkler (non RPi) for two seasons now, 
using the http binding, rules, and direct http commands:
http://rayshobby.net/?page_id=730#httpget
I take it this only works if openhab is installed on the opensprinkler pi?
Any chance of extending this enable the standard opensprinkler?

Original comment by g.g.r...@gmail.com on 5 Aug 2013 at 7:38

GoogleCodeExporter commented 9 years ago
Yes, this binding is only for OpenSprinkler Pi, and only when OpenHAB is 
running on the Raspberry Pi it is attached to. The binding communicates 
directly over the Raspberry Pi GPIO pins, rather than via http. I would be 
quite happy to write a OpenSprinkler binding that can communicate via http, but 
I don't have the standard OpenSprinkler to test with. Without this it is 
largely an exercise in futility :-)

Original comment by jonathan@jonathangiles.net on 5 Aug 2013 at 7:49

GoogleCodeExporter commented 9 years ago
Hi Jonathan,

please find attached my review comments:

# lib
* please enhance lib name with it's version

# OSGI-INF
* the given name (tag) of both the binding and genericbindingprovider is both 
'org.openhab.binding.opensprinklerpi' please rename the genericbindingprovider 
to something more reasonable

# OpenSprinklerPiBindingProvider
* javadoc is missing on both Class- and Methodlevel

# OpenSprinklerPi
* please move description of numberOfStations (currently contained in 
classlevel javadoc) to javadoc of that specific member
* please add javadoc for at least every public and protected constructor and 
method
* please add CR/LF at EOF

# OpenSprinklerPiBinding
* please add javadoc on class level
* please remove template inline comments
* L62 better use numberOfStations as parameter here
* L82 insert log statement which explains the else case

# OpenSprinklerPiGenericBindingProvider
* please add some valid configurations to classlevel javadoc

# OpenSprinklerPiTest
* please add some javadoc on classlevel to explain the purpose of this somewhat 
'special' class
* please move class to another package to explicitly express it's special 
purpose

Original comment by teichsta on 5 Aug 2013 at 8:15

GoogleCodeExporter commented 9 years ago
Thanks - I'll get onto these ASAP. Just a few comments / questions:

1) I didn't realise that the JavaDoc was published anywhere - is there a URL of 
the current release so I can see what is expected?

2) I don't follow your OSGI-INF comment. I don't know what a more reasonable 
name would be?

3) The OpenSprinklerPi and OpenSprinklerPiTest classes will probably be removed 
at some point, replaced by a jar file containing the .class files. This was 
actually how I originally architected it but then simplified the changeset. 
This way the OpenSprinkler Pi code can continue to be developed in a neutral 
way and included in various other projects (as well as expanded perhaps to also 
support the http approach available in the original OpenSprinkler device).

Original comment by jonathan@jonathangiles.net on 5 Aug 2013 at 8:24

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Hi,

> 1) is there a URL of the current release so I can see what is expected?

no, the javadoc isn't published (yet). A good documented example binding is the 
DMX binding by Davy or Kai's KNX binding.

> 2) I don't follow your OSGI-INF comment. I don't know what a more reasonable 
name would be?

Sorry, this was a very unspecific comment. Both of the components described in 
both files do have a name which should be unique (attribute "name" of element 
"scr") . Currently both names are equal and should be changed thus. A good name 
for the genericbinding provider could be 
"org.openhab.binding.opensprinklerpi.genericbindingprovider". Does that makes 
sense?

> 3) This way the OpenSprinkler Pi code can continue to be developed in a 
neutral way and included in various other projects (as well as expanded perhaps 
to also support the http approach available in the original OpenSprinkler 
device).

ok, understand. Then probably all files potentially moved to that external lib 
should be moved to another package.

Original comment by teichsta on 5 Aug 2013 at 8:44

GoogleCodeExporter commented 9 years ago
I believe I have made most (if not all) changes discussed above. I have also 
changed the binding to be called openSprinkler, rather than openSprinklerPi, 
given the possibility that the binding may one day support connecting to an 
OpenSprinkler device (which is not dependent on a Pi), and may also support 
connecting via HTTP rather than GPIO.

I am just compiling and testing the changes, and will then push them all to my 
clone shortly.

Original comment by jonathan@jonathangiles.net on 6 Aug 2013 at 3:05

GoogleCodeExporter commented 9 years ago
I have just now implemented support for OpenSprinkler devices that use the 
interval program that is available for OpenSprinkler and OpenSprinkler Pi. That 
means that the OpenSprinkler binding for OpenHAB should now be able to support 
both (although the configuration will be slightly different). I'll update the 
binding to support both styles of communication (http and gpio).

Original comment by jonathan@jonathangiles.net on 6 Aug 2013 at 4:39

GoogleCodeExporter commented 9 years ago
I've just pushed the changes to 
http://code.google.com/r/jonathan-opensprinklerpi/source/detail?r=979fc397a1a09b
3d1bf7cd6a735c615f97745c3b

Changeset comment:
Big cleanup based on review feedback, as well as extracting out OpenSprinkler
code into a separate library jar and introducing support for the original
OpenSprinkler (connecting over http rather than gpio pins)

I need to go update the wiki, but in short here are the new config details:

################################ OpenSprinkler Binding 
################################
# The type of OpenSprinkler connection to make. There are two valid options, 
and the 
# default mode is gpio.
#
# gpio: this mode is only applicable when running OpenHAB on a Raspberry Pi, 
which 
#       is connected directly to an OpenSprinkler Pi. In this mode the 
communication
#       is directly over the GPIO pins of the Raspberry Pi
# http: this mode is applicable to both OpenSprinkler and OpenSprinkler Pi, as 
long as
#       they are running the interval program. Realistically though if you have 
an
#       OpenSprinkler Pi, it makes more sense to directly connect via gpio mode.
# openSprinkler:mode=gpio

# If the http mode is used, you need to specify the url of the internal program,
# and the password to access it. By default the password is 'opendoor'.
# openSprinkler:httpUrl=http://localhost:8080/
# openSprinkler:httpPassword=opendoor

# The number of stations available. By default this is 8, but for each 
expansion board
# installed this number will can be incremented by 8. Default value is 8.
# openSprinkler:numberOfStations=8

Original comment by jonathan@jonathangiles.net on 6 Aug 2013 at 6:26

GoogleCodeExporter commented 9 years ago
I have now updated the wiki page to detail the new functionality:
http://code.google.com/p/openhab/wiki/OpenSprinkler

Original comment by jonathan@jonathangiles.net on 6 Aug 2013 at 6:34

GoogleCodeExporter commented 9 years ago
Will re-review and merge into default!

Thanks Jonathan!

Original comment by teichsta on 6 Aug 2013 at 4:21

GoogleCodeExporter commented 9 years ago
Merged binding to default repo (see 
http://code.google.com/p/openhab/source/detail?r=ec963bf17b9469504c8832753e55634
ed6f441c5).

Finally i made some small changes which i hope you don't mind:

* changed OS mode to Enum
* made parsing of the OS mode more restrictive in updated()
* made (theoretic) default case in updateBinding() more restrictive

Thank you very much for this contribution! I am looking to all your upcoming 
bindings (Ninjablock, etc.)

Original comment by teichsta on 6 Aug 2013 at 7:52

GoogleCodeExporter commented 9 years ago
Thanks for merging, and the changes are fine. 

Regarding other bindings: my wife is reminding me that I have a lot of other 
projects I need to get sorted before the baby arrives, so maybe this is the 
only binding for now.....but we'll see :-)

Original comment by jonathan@jonathangiles.net on 6 Aug 2013 at 7:56

GoogleCodeExporter commented 9 years ago
Take your time :-)

Original comment by teichsta on 6 Aug 2013 at 7:57

GoogleCodeExporter commented 9 years ago
You will soon have many sleepless nights and thus time for coding :-D

Original comment by kai.openhab on 6 Aug 2013 at 7:58