Closed GoogleCodeExporter closed 9 years ago
Hi Till, first of all: thumbs up! very cool work! Find below my very first
rough comments (merely formal issues) on your binding:
* if possible remove cul4java classes and add them as binary lib. If you added
functionality a fork of https://github.com/tobiaswegner/CUL4Java should do the
trick
* please add licence header to each class file
* please add javadoc to each class of your binding
* please add javadoc for (at least all public) methods especially abstract and
interface methods
* HandlerType should be an Enumeration rather than a String/Char
* please remove //TODOs
* there seem to be unused dependencies declared in manifest.mf
* please add test-fragment bundle (see http, exec, dmx as example) to proof (at
least) the parsing functionality
* please add wiki page for your binding (your README seems to be a very good
starting point for that page)
Again many thanks for effort! Keep coding for openHAB ;-)
Thomas E.-E.
Original comment by teichsta
on 3 Apr 2013 at 8:42
is #106 somehow related to this issue?
Original comment by teichsta
on 4 Apr 2013 at 7:05
If it is sufficient to bind the FHZ system via the CUL hardware, then this is
connected. Implementing the SHT and EM devices is still on my todo list.
Original comment by till.klo...@gmail.com
on 4 Apr 2013 at 8:00
* The Wiki page is added and needs review/adding to the TOC.
* Test cases development is started but not finished.
* As is the writing of the Javadoc.
Support for environment sensors based on FS20 is nearly finished and should be
ready within one or two weeks (depending on my work load)
Original comment by till.klo...@gmail.com
on 12 Apr 2013 at 9:47
Does your CUL binding also support the MAX! protocol for thermostats and open
window sensors etc?
Original comment by ABrand...@gmail.com
on 23 Apr 2013 at 11:49
No, unfortunately not, since it is not possible to drive the CUL in both modes
(SlowRF/FS20/FHT/SHT and MAX!) at the same time. This would require a new
binding which binds to another CUL running in the MORITZ mode. The CUL binding
could work as a blue print since some things should be pretty similiar.
Probably some things can be outsources to a library.
Original comment by till.klo...@gmail.com
on 23 Apr 2013 at 11:54
Original comment by teichsta
on 7 May 2013 at 8:09
Hi. Are there any plan for binding the CUL with homematic support ?
Peter
Original comment by petersau...@hotmail.de
on 8 Jun 2013 at 7:46
Currently there are no plans to support Homematic with the CUL. But I see there
is a demand to implement other protocols than the SlowRF protocols.
As soon as my Android client has release quality I will see to create a service
from my binding which handles low level comminication with the CUL (or similar
devices). This should ease the development of other protocols.
Original comment by till.klo...@gmail.com
on 9 Jun 2013 at 6:42
[deleted comment]
will review asap
Original comment by teichsta
on 6 Aug 2013 at 4:26
[deleted comment]
Hi Till,
please find below my review comments:
#general
* cul4java classes by tobias wegner should be removed and added as binary lib.
If you added functionality a fork of https://github.com/tobiaswegner/CUL4Java
should do the trick
* please add test-fragment bundle (see http, exec, dmx as example) to proof (at
least) the parsing functionality
* please update version tag to 1.3.0
* please add licence header to each class file
* please add javadoc to each class of your binding
* please add javadoc for (at least all public) methods especially abstract and
interface methods
# CULBindingProvider
* L46, L55 "Retrieve a binding [config]"
* L76 javadoc is missing
# FHTMode
*is this class part of the tobiaswegner-lib? Then it should be moved to a
lib-jar if not, please document
# CULBinding
* please adopt to new handling of isProperlyConfigured (see
http://code.google.com/p/openhab/source/detail?r=6d2ba578d8a0fbe92849430e9dd0d81
45dc3f5fc)
* L171 although this method is private could you please explain what this
TimeTimer is for?
* L190, L203 please use logging rather than printStackTrace()
* L304 please remove internalReceiveUpdate since this method isn't used
# CULGenericBindingProvider
* please add some valid configuration examples in javadoc on class level
* L85 this method is meant to validate the type of the configured item. It
should check wether the given item type is generally valid.
# UpdateFHTTimeJob
*is this class part of the tobiaswegner-lib? Then it should be moved to a
lib-jar if not, please document
# AbstractCulBindingConfig
* please add javadoc on class level
* storing the variants of configurations in different caches feels a bit
unintended. I would rather prefer having just one (the one given by
AbstractGenericBindingProvider and haven probably some attributes in
AbstractCulBindingConfig to differentiate.
* shouldn't the members allowedItems and allowedCommands be moved to this
abstract class?
* allowedItems should be renamed to allowedItemTypes
* allowedCommands should be renamed allowedCommandTypes
* since this class is not only about hosting/parsing the configuration but
rather is responsible for executing commands for the different protocols it
should be better be renamed to something like AbstractCulProtocolDriver with
it's special implementations?
# FHTBindingConfig
* please add javadoc on class level
# FS20BindingConfig
* please add javadoc on class level
# FS20CommandHelper
* please add licence header to each class file
* please add javadoc on class level
# IntertechnoAddressHelper
* please add licence header to each class file
* please add javadoc on class level
* please add javadoc on method level
# IntertechnoBindingConfig
* please add licence header to each class file
* please add javadoc on class level
Regards,
Thomas E.-E.
Original comment by teichsta
on 11 Aug 2013 at 10:12
Till, will you be able to incorporate the review by the mid of next week? Best,
Thomas E.-E.
Original comment by teichsta
on 25 Aug 2013 at 9:18
The missing documentation will be fixed soon. Also I will now be adding license
headers via the license plugin.
The change for isProperlyConfigured is already adpopted ( I have this binding
running with 1.3).
For the current binding I would rather not start bigger refactorings since I
would like to create a CUL transport (analog to the serial transport) which
then can be used by other bindings to use the CUL device. This would not only
allow to create a binding for the slow rf protocols, but also for the Max! and
Homematic protocols (and others supported by culfw).
But the other issues should be fixed.
Original comment by till.klo...@gmail.com
on 26 Aug 2013 at 7:37
Original comment by till.klo...@gmail.com
on 26 Aug 2013 at 7:38
ok, please let me know when you are ready.
Original comment by teichsta
on 26 Aug 2013 at 7:46
Ok, due to some sick days I was able to start a complete rewrite. I have now a
CUL transport which can be used by bindings accessing the CUL. The transport
abstracts everything hardware related, so it should be easy to add support for
the CUN and CUNO devices later on.
Every protocol will now get its own binding which makes the bindings much
smaller (and hopefully easier to maintain).
In the future it should be also possible to support other device modes of the
CUL so we can implement bindings for MAX! and BidCos (aka Homematic).
@teichsta
Could you please review my current rewrite and check if I'm on the right track?
If everything checks out, I will add JavaDoc and documentation. Technically the
rewrite works fine so far.
Original comment by till.klo...@gmail.com
on 28 Aug 2013 at 11:57
in the culmode.java of cul transport it should be: ASK_SIN("Ar") for HomeMatic.
the closing is not done properly for other modes.
in culserialhandlimpl.java it sends always a "X00", which is only for the
slowrf mode. Closing the asksin mode is done by sending "Ax". For closing MAX
send "Zx". But i don't know what you have to send to start the MAX mode, but it
is something starting with "Z"
see cul_00.pm of fhem.
unfortunately max is not described in the culfw as far as i see
Original comment by pgsh.tudo
on 29 Aug 2013 at 12:50
Hi,
> Could you please review my current rewrite and check if I'm on the right
track? If everything checks out, I will
> add JavaDoc and documentation. Technically the rewrite works fine so far.
yes, the binding seems to be on the right track … BUT … there seems to be
a lot of work to tidy up the code? Would it be possible to release the version
i've reviewed with some minor changes and continue development the new path? It
would be a pity to not have the CUL binding on board.
What do you think?
Original comment by teichsta
on 1 Sep 2013 at 7:11
Most issues should be fixed. I added JavaDoc to most classes and methods
(hopefully all relevant). As far as I can see I didn't moved the CUL4Java
classes to their own lib, everything else should be fixed.
@pgsh.tudo
The enum CULMode is currently just a prototype. I realized that opening and
closing the device is much more protocol specific. Therefore I will probably
change the way this handled. But I'm not certain how at this moment.
The target is to support all modes and protocols of the CUL, probably even the
raw mode.
Original comment by till.klo...@gmail.com
on 1 Sep 2013 at 7:35
What is the roadmap for the CUL Binding? Is it planned to get it pushed to
openhab release branch or do i have to checkout your source (1.4.0 SNAPSHOT)
and build it from source if i want to use the CUL with openhab?
Original comment by m.schult...@googlemail.com
on 18 Oct 2013 at 9:31
A pull request including the CUL transport, FHT-, FS20- and Intertechno-Binding
is waiting for review since some time. A S300TH-Binding is in development. You
can get snapshot builds of the CUL transport and the bindings using it from
here http://download.akuz.de/openHAB/
Original comment by till.klo...@gmail.com
on 21 Oct 2013 at 7:53
Original comment by kai.openhab
on 21 Oct 2013 at 7:55
Thanks for your quick answer, i checked out your fork and built the
1.4.0-SNAPSHOT from source because i couldn't wait to get my CUL running with
openHAB. It works very fine without any issues so far - thanks for your efforts.
Hopefully the pull request gets reviewed soon and it will pulled into the main
branch for release 1.4.0.
Original comment by m.schult...@googlemail.com
on 22 Oct 2013 at 9:01
What are the settings for cul fs20 binding.
Original comment by rainersc...@gmail.com
on 24 Oct 2013 at 8:21
Do the following:
- sudo apt-get install librxtx-java
- add -Djava.library.path=/usr/lib/jni -Dgnu.io.rxtx.SerialPorts=/dev/ttyACM0
to your start.sh
where /dev/ttyACM0 is your CUL
- add openhab user to dialout group
- edit openhab.cfg and add fs20:device=serial:/dev/ttyACM0
where /dev/ttyACM0 is you CUL device
- edit your *.items file
Switch wz_Licht_Sofa (Lights) { fs20="AAAA00" }
Where AAAA is your house code and 00 the FS20 address.
Original comment by waitz.sebastian@gmail.com
on 24 Oct 2013 at 8:39
Great binding! Did you also Test this already on windows? How would i then
configure it?
Original comment by haukeern...@gmail.com
on 1 Nov 2013 at 3:38
Original comment by teichsta
on 5 Nov 2013 at 10:47
This issue has been migrated to Github. If this issue id is greater than103 its
id has been preserved on Github. You can open your issue by calling the URL
https://github.com/openhab/openhab/issues/<issueid>. Issues with ids less or
equal 103 new ids were created.
Original comment by teichsta
on 17 Nov 2013 at 8:08
see above!
Issue has been migrated to Github and should be discussed there.
Original comment by teichsta
on 21 Nov 2013 at 1:51
Original issue reported on code.google.com by
kai.openhab
on 3 Apr 2013 at 7:59