Closed GoogleCodeExporter closed 9 years ago
Original comment by teichsta
on 7 May 2013 at 4:00
Original comment by kai.openhab
on 7 May 2013 at 4:58
Hi Oliver,
Here are my review comments:
1. Please see our coding conventions at
https://code.google.com/p/openhab/wiki/HowToContribute - it would be nice if
you could adjust your code accordingly (mainly adding missing Javadocs, @author
and @since tags)
2. Does your binding really require Java7? If not, please set the required
execution environment in the manifest to Java5 or Java6.
3. change binding version from 1.2.0 to 1.3.0 (sorry, that's because I was too
slow ;-))
4. Your binding MUST NOT reference the item registry - sorry that this is not
yet clearly stated in the documentation, but it is an architectural principal
that we want to enforce. Bindings should only communicate through the event bus
and thus be stateless (see
http://wiki.openhab.googlecode.com/hg/images/events.png). For your case (the
method internalReceiveCommand), you should instead simply use
"eventPublisher.postUpdate(itemName, command)"
5. The binding configuration would be nicer if you'd move the configuration of
the serial port to the openhab.cfg file - you would then only specify the
channel in the items file. This is how other bindings handle it as well (when
requiring a serial port or a host ip).
6. UrsiDevice.xtend, l. 152: Wouldn't you need a Thread::sleep in this while
loop?
7. What is AbstractUrtsiBinding good for? I guess it can be removed
8. ArrayHelper should be removed - the one method is only used in one place and
has only one line - this does not really qualify for a class of its own.
That's all - looking forward to the next update!
Original comment by kai.openhab
on 8 May 2013 at 9:09
Hi Kai,
thanks for reviewing the binding. My comments:
1. I'm going to extend the code accordingly.
2. The code doesn't require Java 7, I'm going to switch to Java 5 or 6.
3. sure
4. I see
5. In theory it's possible to use two or more devices. Each of them offers 16
channels. Putting the port into the global config file restricts the binding to
one device. Anyway, it's your decision... for me (and most of the others users)
one device is sufficient. Please give me feedback concerning this one.
6. yes
7. I just needed this due to a Bug in the Xtend Maven Plugin which compiles the
Xtend file. I guess this bug has been solved in Xtend 2.4.1, but openhab uses
Xtext and Xtend 2.3.1. It's ugly I know...
8. I created this class because of an Xtend limitation:
https://groups.google.com/forum/#!msg/xtend-lang/p33jDUNh0Hg/z4u7B7BHhJgJ
With Xtend 2.4.1 AbstractUrtsiBinding and ArrayHelper can be deleted.
Do you have any plans concerning an Xtext/Xtend update?
Kind regards
Oliver
Original comment by oliver_l...@gmx.de
on 9 May 2013 at 11:19
Hi Kai,
I have moved the binding to a googlecode clone:
http://code.google.com/r/oliver-libutzki-urtsi-binding/
I fixed 1, 2, 3, 4 and 6.
For 5 I expect your decision.
7 and 8 cannot be fixed without an Xtext/Xtend update, but I documented this in
the code.
Kind regards
Oliver
Original comment by oliver_l...@gmx.de
on 11 May 2013 at 11:29
Thanks for addressing 1,2,3,4 and 6 so quickly!
Regarding 5: I would say that supporting a single one is enough - we do it
similarly for other bindings like KNX, where you could have multiple
connections in theory, but in practice, nobody ever asked for it.
If you would still refer to support multiple devices, you could also do it
similarly to https://code.google.com/p/openhab/wiki/SamsungTVBinding, i.e.
introduce an id for each device and configure it in openhab.cfg.
7+8: How do you assess the effort for an upgrade? Will existing things break
and need to be adapted or can we expect it to be rather smooth?
Original comment by kai.openhab
on 13 May 2013 at 5:51
Regarding 5: Ok, I'm going to refactor this accordingly.
Regarding 7+8: I recently updated a project for a customer without having any
bigger issues (but we don't use Xbase). Xtend's performance increases
massively, so I highly recommend this update... right after the release this is
the best moment to do such a step I think.
I can offer you to perform the upgrade on a clone, but we should file another
issue for this work as it is not related to this one directly.
Kind regards
Oliver
Original comment by oliver_l...@gmx.de
on 14 May 2013 at 11:54
As I plan an overall upgrade to Kepler end of June, I think we can wait with
the Xtext upgrade until then (Kepler will include Xtext 2.5.0, if I saw it
right). So let's keep your code for 7+8 as it is for the moment.
Apparently Xtext 2.4.0 brought major changes to Xbase, so I could imagine that
many things will break - if you like, you could already do some tests on this,
so that we are prepared for the big upgrade end of June then :-)
Original comment by kai.openhab
on 14 May 2013 at 7:49
Kepler will include Xtext/Xtend 2.4.2:
https://twitter.com/xtendlang/statuses/333677692035690496/
It might be useful to switch to Xtext/Xtend 2.4.1 prior to the Kepler update;
just to avoid a big bang update ;-)
Original comment by oliver_l...@gmx.de
on 15 May 2013 at 9:55
Ok, I was talking about Xtext version:
http://projects.eclipse.org/projects/modeling.tmf.xtext/releases/2.5.0
So please feel free to open a new issue fot Xtend 2.4.1 upgrade and assign it
to yourself :-)
Original comment by kai.openhab
on 15 May 2013 at 11:44
Regarding 5: Change is pushed.
I have rewritten the UrtsiBinding class in Java due to the Xtend 2.3.0 problems.
Anyway, there are still a couple of Xtend files and this binding can be used as
reference implementation, especailly concerning the Maven part.
Kind regards
Oliver
Original comment by oliver_l...@gmx.de
on 20 May 2013 at 4:25
stumbled upon the protocol specification:
http://www.somfysmarthome.co.uk/pdfs/RS485%20RTS%20Transmitter.pdf
Original comment by teichsta
on 5 Jun 2013 at 4:19
This is the correct specification:
http://www.avoutlet.com/images/product/additional/s/1810872-ins.pdf
Currently the openhab binding supports the RS232 connection.
Original comment by oliver_l...@gmx.de
on 17 Jun 2013 at 1:19
Original comment by kai.openhab
on 18 Jun 2013 at 9:21
Original issue reported on code.google.com by
kai.openhab
on 5 May 2013 at 7:02