Closed GoogleCodeExporter closed 8 years ago
Hi Pauli,
thanks for contributing your binding code für Samsung TVs. Please find my
first review comments below:
* all classes should contain Javadoc (at least the function of the Class should
be explained in detail)
* Classes in package should be refered to as lib not as Code-Copy
* Base64 should by used from Apache's commons.codec project
* Class-Names should start with a capital-letter
* The port 55000 seems to be a default value which should be set by
SamsungTvActiveBinding.update() if no port has been configured
Regards,
Thomas E.-E.
Original comment by teichsta
on 3 Dec 2012 at 4:31
Thanks for the review Thomas.
* all classes should contain Javadoc (at least the function of the Class should
be explained in detail)
Should be OK, or do you mean also for "remocon" classes written by Tom Quist?
* Classes in package should be refered to as lib not as Code-Copy
No clue how to do it.
* Base64 should by used from Apache's commons.codec project
I want to avoid make any changes to remocon package, because maintaining will
then be much more harder.
* Class-Names should start with a capital-letter
Fixed.
* The port 55000 seems to be a default value which should be set by
SamsungTvActiveBinding.update() if no port has been configured
Fixed.
Original comment by pauli.an...@gmail.com
on 3 Dec 2012 at 7:43
where did get the lib-code from?
Original comment by teichsta
on 4 Dec 2012 at 10:07
http://quist.de/2011/02/remote-control-samsung-tv-with-android-phone/?lang=en
Code were on https://github.com/tomquist/SamyGo-Android-Remote, but pag is not
available anymore. But there are several copies on github. E.g.
https://github.com/keremkusmezer/SamyGo-Android-Remote/tree/master/src/de/quist/
samy/remocon
Original comment by pauli.an...@gmail.com
on 5 Dec 2012 at 6:43
looks good so far :-)
Nevertheless some remarks below:
It would be more convenient if the bindingConfig accepts more than one
configuration. The most ItemTypes accept different command that should be
applyable to the samsung binding. A config for a switch item should look like
this:
samsungtv="OFF:Kitchen:KEY_POWEROFF ON:Kitchen:KEY_POWERON" or
samsungtv="INC:Kitchen:KEY_PANNEL_VOLUP DEC:Kitchen.:KEY_PANNEL_VOLDOW"
etc ...
For an example how to implement this please have a look at the mpd-binding
Original comment by teichsta
on 5 Dec 2012 at 10:31
Actually, I already think that before I made my decision to not implement it.
There is only few use cases where that could be useful. Inc/dec is only usable
for volume and maybe channel selection, and number support for direct channel
selection. Samsung TV's don't support power ON command, which is really shame.
Because normal remote control is still needed for powering TV on, I didn't see
volume and channel selection very useful. Normal remote control is still pretty
handy for those purposes :) Because I currently need only power off and mute
commands, I don't have urgent desire to implement that (too many bindings to do
and too little time). But feel free to implement it ;)
Original comment by pauli.an...@gmail.com
on 6 Dec 2012 at 2:59
Now bindingConfig accepts more than one configuration.
Number directChannel {samsungtv="1:Livingroom:KEY_1, 2:Livingroom:KEY_2,
3:Livingroom:KEY_3"}
Dimmer channel {samsungtv="INCREASE:Livingroom:KEY_CHUP,
DECREASE:Livingroom:KEY_CHDOWN"}
Switch mute {samsungtv="ON:Livingroom:KEY_MUTE, OFF:Livingroom:KEY_MUTE"}
Rollershutter volume {samsungtv="UP:Livingroom:KEY_VOLUP,
DOWN:Livingroom:KEY_VOLDOWN"}
String tvtest {samsungtv="volumeup:Livingroom:KEY_VOLUP,
volumedown:Livingroom:KEY_VOLDOWN"}
Original comment by pauli.an...@gmail.com
on 28 Mar 2013 at 9:51
so you decided to implement the binding anyway?
Good to know :-) Do you think you could finish it by the end of this week? This
would make a review feasible.
Original comment by teichsta
on 1 Apr 2013 at 8:36
Binding should already be ok, at least from my point of view.
Original comment by pauli.an...@gmail.com
on 2 Apr 2013 at 6:29
Binding looks good and i will merge into default soon! Some questions though:
* there is no license header or author mentioned in class RemoteReader - did
you create it?
* currently this binding is layed out as "out"binding does RemoteSession (or at
least the TV) support a biderctional connection?
* could you create the corresponding wiki page? Would be great to have it in
place until the 10th
Thanks for your efforts!!
Thomas E.-E.
Original comment by teichsta
on 3 Apr 2013 at 8:07
RemoteReader is also copied directly from github. Actually all files on remocon
package is directly from SamyGo android remote project. I assume that
RemoteReader is also written by Tom Quist.
Protocol on port 55000 simulates the remote control, so we can just send key
codes. Protocol is described on page
http://sc0ty.pl/2012/02/samsung-tv-network-remote-control-protocol/.
Yes, I will create wiki page.
Original comment by pauli.an...@gmail.com
on 3 Apr 2013 at 2:54
thanks for this update!
(and the upcoming wiki-page ;-) )
Original comment by teichsta
on 3 Apr 2013 at 2:55
[deleted comment]
binding is checked in to default branch (see
http://code.google.com/p/openhab/source/detail?r=1ffcc67eef9e57bd08cbdacdf15b6bb
b973e0d72) - let's resolve this issue once the Wiki page is written!
Thanks Pauli for this distribution!
Original comment by teichsta
on 4 Apr 2013 at 10:22
Thanks Pauli for adding the Wiki-Page!
Original comment by teichsta
on 10 Apr 2013 at 8:14
Configuration is missing on openhab_default.cfg
Original comment by pauli.an...@gmail.com
on 7 May 2013 at 8:17
Original comment by pauli.an...@gmail.com
on 7 May 2013 at 8:18
openhab_default.cfg has been updated (see
http://code.google.com/p/openhab/source/detail?r=f33ad39e4ed051afae263a8d559c066
735ab23a7)
Original comment by teichsta
on 7 May 2013 at 7:59
Few improvements can be pulled from my repo:
- Fixed RemoteSession logging and set socket timeout to 5000ms.
- Give warning to log when connection details cannot be found.
Original comment by pauli.an...@gmail.com
on 13 May 2013 at 9:36
Could you reopened this issue (I cannot do it), so that you don't forgot to
pull my improvements to main.
Original comment by pauli.an...@gmail.com
on 22 May 2013 at 7:55
could you send the link to the appropriate changeset, please?
Original comment by teichsta
on 22 May 2013 at 9:46
http://code.google.com/r/paulianttila-ihc-binding/source/detail?r=91dc811e1fe162
51bd8d4fe971221ef1774d612d
http://code.google.com/r/paulianttila-ihc-binding/source/detail?r=a7d905b27867f9
fe63e662487ab56182ab017829
Original comment by pauli.an...@gmail.com
on 23 May 2013 at 5:06
pushed your changes - thanks Pali (see e79acba6c274, 16a1fe4cfcc2 and
9d912dd2bdcf)
Original comment by teichsta
on 23 May 2013 at 4:36
I don't know if this is the best place to publish this...
I have tried the poweron command on my TV, but it doesn't work (UE46C7000). I
have done a workaround. I've attached the TV to my raspberry pi with HDMI, and
I use cec-client to check the status and to power on.
With the exec binding I launch "echo 'on 0' | cec-client -s" to power on, and
every minute with cron, i update the item status trough rest with the command
"echo pow 0 | cec-client -d 1 -s | grep "power status:" | awk '{ print $3; }'".
Original comment by angel.bu...@gmail.com
on 7 Sep 2013 at 2:55
Thanks Angel, for this sharing this. An even better place to share would be the
samples wiki (https://code.google.com/p/openhab-samples/wiki/Sidebar?tm=6),
which i added you as committer to. Would be great if you could create a small
wiki page or simply enhance an existing one.
Original comment by teichsta
on 8 Sep 2013 at 10:49
Original issue reported on code.google.com by
kai.openhab
on 20 Nov 2012 at 10:06