search

Ole8700 / openhab

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

Epson projector binding #258

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Review request for Epson projector binding

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

Original issue reported on code.google.com by pauli.an...@gmail.com on 27 Apr 2013 at 9:45

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
I have a EH-TW3600 which has no ethernet-connection.

Will this binding be for ethernet or serial-connections?

Original comment by christop...@gmail.com on 13 Jul 2013 at 7:59

GoogleCodeExporter commented 9 years ago 
Binding is for serial port communication, but code stub for ethernet-serial 
port adapter is also generated but not implemented yet.

I have developed binding against EH-TW3200 which is pretty much the same 
projector.

Original comment by pauli.an...@gmail.com on 14 Jul 2013 at 7:48

GoogleCodeExporter commented 9 years ago 
That's really great!!! :)
I think I need an USB-to-serial-adapter. :)

Original comment by christop...@gmail.com on 14 Jul 2013 at 8:09

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

Original comment by teichsta on 6 Aug 2013 at 10:17

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

thanks for contributing this binding. Please find below my Review remarks:

# EpsonProjectorBindingProvider
* isInBinding() seems to be never called. Is this method superfluous probably?
* if isInBinding is superfluous getRefreshInterval is superfluous as well
* if isInBinding is superfluous getInBindingItemNames is superfluous as well

# EpsonProjectorCommandType
* since this enum seems to belong to the "internal" data structure it should be 
moved in the .internal package
* L84 ',' is superfluous
* L122-126 please use curly brackets although not really necessary but it 
supports better readability

# EpsonProjectBinding
* L114-135 please move code to a separat method disconnectAll() (name is just a 
proposal)
* L297-298, L342-343 please remove commented code
* L683-688, L708,709 since EpsonProjectorTcpConnector is not implemented and 
should be removed hence these parameters could be removed as well

# EpsonProjectorConnector
* this class seems to be better represented by an interface
* when changed to Interface, please remove 'public abstract' prefix

# EpsonProjectorDevice
* L361 please remove constructor for TcpConnector
* L365, L389 String comparison should be done with equals()
* L442, L489, L515, L531, L633, L759, L806, L822, L838, L858, L871, L895,  
which exception could be thrown/catched here?
* L923 ErrorStrings should be externalized (see novelanheatpump binding if you 
need details how to accomplish i18n)
* L990 IF board > If board

# EpsonProjectorGenericBindingProvider
* L108 remove inBinding member if isInBinding is superfluous
* L276 member seems not to be filled anywhere
* L280 remove inBinding if not necessary

# EpsonProjectSerialConnector
* L104, 107 you could better use commons.IOUtils to close handle Streams more 
comfortable. It contains also methods to open and administer streams

# EpsonProjectorTcpConnector
* please remove this class since it seems to be a place holder for future (when 
ever) developments

Best regards,

Thomas E.-E.

Original comment by teichsta on 7 Aug 2013 at 10:06

GoogleCodeExporter commented 9 years ago 
Thanks for the review Thomas.

# EpsonProjectorBindingProvider
* isInBinding() seems to be never called. Is this method superfluous probably?
* if isInBinding is superfluous getRefreshInterval is superfluous as well
* if isInBinding is superfluous getInBindingItemNames is superfluous as well
Binding provide both in and out functionality, so isInBinding() is not 
superfluous. Well isInBinding() method is not used, because interface provide 
also getInBindingItemNames() method.

# EpsonProjectorCommandType
* since this enum seems to belong to the "internal" data structure it should be 
moved in the .internal package
Done. I have copied this "idea" from other bindings, so your probaply should 
clean up other bindings on the main repo as well.

* L84 ',' is superfluous
Well, it's fully supported by the Java so I don't agree that it's superfluous 
:) There is good reasons why it's supported:
1) If use similar style as I use, diff only show newly added line(s).
2) Easier to add and comment out some enum values.
3) Make life easier to code generators.

* L122-126 please use curly brackets although not really necessary but it 
supports better readability
Done.

# EpsonProjectBinding
* L114-135 please move code to a separat method disconnectAll() (name is just a 
proposal)
Done.

* L297-298, L342-343 please remove commented code
Done.

* L683-688, L708,709 since EpsonProjectorTcpConnector is not implemented and 
should be removed hence these parameters could be removed as well
Are your really sure that you want me to remove TCP connector. 
It's much more easier to someone add TCP support, because all functionality is 
on already on place. 
My installation is currently so that I can use serial port connection to my 
projector, but I'm pretty sure that most of the users needs e.g. tcp-serial 
port adapter, because openhab is located to different place than projector 
itself. 

# EpsonProjectorConnector
* this class seems to be better represented by an interface
* when changed to Interface, please remove 'public abstract' prefix
Done.

# EpsonProjectorDevice
* L365, L389 String comparison should be done with equals()
Done. I do the same mistake every once in a while (C/C++ has been my primary).

* L442, L489, L515, L531, L633, L759, L806, L822, L838, L858, L871, L895,  
which exception could be thrown/catched here?
Fixed.

* L923 ErrorStrings should be externalized (see novelanheatpump binding if you 
need details how to accomplish i18n)
Done. That's really nice feature.

* L990 IF board > If board
"IF" is abbreviation which is copied directly from Epson spec. No clue what it 
means.

# EpsonProjectorGenericBindingProvider
* L276 member seems not to be filled anywhere
Fixed.

# EpsonProjectSerialConnector
* L104, 107 you could better use commons.IOUtils to close handle Streams more 
comfortable. It contains also methods to open and administer streams
Done.

Original comment by pauli.an...@gmail.com on 8 Aug 2013 at 6:38

GoogleCodeExporter commented 9 years ago 
I got a USB-to-Serial-Adapter, a long cable and an epson projector.
All I need to test the binding are a compiled binary and some time.
You can help me with the first one. ;)

Is there any other documentation than the list of commands in the source?
https://code.google.com/r/paulianttila-ihc-binding/source/browse/bundles/binding
/org.openhab.binding.epsonprojector/src/main/java/org/openhab/binding/epsonproje
ctor/internal/EpsonProjectorCommandType.java

Maybe you can post some parts of your items-file.

Original comment by christop...@gmail.com on 10 Aug 2013 at 8:26

GoogleCodeExporter commented 9 years ago

Original comment by kai.openhab on 11 Aug 2013 at 7:32

GoogleCodeExporter commented 9 years ago 
Preliminary Wiki page can be found here 
http://code.google.com/p/openhab/wiki/EpsonProjectorBinding

Original comment by pauli.an...@gmail.com on 11 Aug 2013 at 8:16

Attachments:

GoogleCodeExporter commented 9 years ago 
I was just a little playing with the new binding and got this:

18:24:07.343 DEBUG o.o.b.e.i.EpsonProjectorDevice[:358]- Query: 'SOURCE 20'
18:24:07.545 DEBUG o.o.b.e.i.EpsonProjectorDevice[:361]- Response: ':'
18:24:31.445 DEBUG o.o.b.e.i.EpsonProjectorDevice[:358]- Query: 'PWR?'
18:24:31.547 DEBUG o.o.b.e.i.EpsonProjectorDevice[:361]- Response: 'PWR=01'
18:24:31.547 DEBUG o.o.b.e.i.EpsonProjectorBinding[:199]- item 'epsonSource' is 
about to be refreshed now
18:24:31.547 DEBUG o.o.b.e.i.EpsonProjectorDevice[:358]- Query: 'SOURCE?'
18:24:31.648 DEBUG o.o.b.e.i.EpsonProjectorDevice[:361]- Response: 'SOURCE=21'
18:24:31.649 WARN  o.o.b.e.i.EpsonProjectorBinding[:372]- Couldn't execute 
command 'Source', 
org.openhab.binding.epsonprojector.internal.EpsonProjectorException: Can't 
convert value33 to Source
18:24:31.649 ERROR o.o.b.e.i.EpsonProjectorBinding[:213]- No response received 
from command 'Source'

Original comment by christop...@gmail.com on 11 Aug 2013 at 4:26

GoogleCodeExporter commented 9 years ago 
Currently only      

COMPONENT(0x14), PC_DSUB(0x20), HDMI1(0x30), HDMI2(0xA0), VIDEO(0x41), 
SVIDEO(0x42) are supported on source. 

You can use DirectSource (number item) and then introduce mappings on the 
sitemap.

Original comment by pauli.an...@gmail.com on 11 Aug 2013 at 5:03

GoogleCodeExporter commented 9 years ago 
Ok.

I get another error in the logs:

20:57:53.982 DEBUG o.o.b.e.i.EpsonProjectorBinding[:199]- item 
'epsonHorizontalReverse' is about to be refreshed now
20:57:53.983 DEBUG o.o.b.e.i.EpsonProjectorDevice[:358]- Query: 'HREVERSE?'
20:57:54.083 DEBUG o.o.b.e.i.EpsonProjectorDevice[:361]- Response: 
'HREVERSE=OFF'
20:57:54.084 WARN  o.o.b.e.i.EpsonProjectorBinding[:372]- Couldn't execute 
command 'HorizontalReverse', 
org.openhab.binding.epsonprojector.internal.EpsonProjectorException: Illegal 
response
20:57:54.084 ERROR o.o.b.e.i.EpsonProjectorBinding[:213]- No response received 
from command 'HorizontalReverse'

But everything works as expected.
So I don't know, what the error is.

  Switch epsonHorizontalReverse  { epsonprojector="hometheater:HorizontalReverse:ON,60000" }

  Switch  item=epsonHorizontalReverse label="Horizontal Reverse"

Original comment by christop...@gmail.com on 11 Aug 2013 at 7:00

GoogleCodeExporter commented 9 years ago 
I finidhed builduing a little demo.
https://code.google.com/p/openhab/wiki/EpsonProjectorBinding?ts=1376249007&updat
ed=EpsonProjectorBinding

Seems all good.
But I think the strings for the ErrMessages are missing. ;)

Original comment by christop...@gmail.com on 11 Aug 2013 at 7:24

GoogleCodeExporter commented 9 years ago 
Binding is merged into default branch (see 
http://code.google.com/p/openhab/source/detail?r=86aa9f32887c813c309567250f8dddc
edf89f486).

Thanks Pali for another cool contribution!

Original comment by teichsta on 11 Aug 2013 at 11:04

GoogleCodeExporter commented 9 years ago 
I think there is an issue with the "ERR"-command (or the response to it).

16:28:05.973 DEBUG o.o.b.e.i.EpsonProjectorDevice[:358] - Query: 'ERR?'
16:28:06.075 DEBUG o.o.b.e.i.EpsonProjectorDevice[:361] - Response: 'ERR'
16:28:06.075 WARN  o.o.b.e.i.EpsonProjectorBinding[:372] - Couldn't execute 
command 'ErrCode', 
org.openhab.binding.epsonprojector.internal.EpsonProjectorException: Error 
response received
16:28:06.076 ERROR o.o.b.e.i.EpsonProjectorBinding[:213] - No response received 
from command 'ErrCode'

Frankly, I'm not sure if the Err-commands is supported by my TW3600.

Original comment by christop...@gmail.com on 17 Aug 2013 at 2:55

GoogleCodeExporter commented 9 years ago 
Since this issue is closed could you open another issue if there are issue with 
the epson binding? Best, Thomas E.-E.

Original comment by teichsta on 18 Aug 2013 at 7:02

GoogleCodeExporter commented 9 years ago 
https://code.google.com/p/openhab/issues/detail?id=406

Original comment by christop...@gmail.com on 18 Aug 2013 at 10:39