[epsonprojector] Update epsonprojector binding for OH3 #9021

mlobstein · 2020-11-14T06:19:55Z

I have picked up where PR #7370 left off to bring the epsonprojector binding back for OH3. I have finished cleaning up the code & documentation and tested with two older projectors. Thanks to @ghys for starting this effort.

Test build:
https://github.com/mlobstein/mlobstein-beta-test/raw/master/org.openhab.binding.epsonprojector-3.0.0-SNAPSHOT.jar

https://github.com/mlobstein/mlobstein-beta-test/raw/master/org.openhab.binding.epsonprojector-3.0.0-SNAPSHOT-sources.jar

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

ghys · 2020-11-14T09:04:27Z

And thanks to you for finishing it @mlobstein! - it looks good and will surely be better maintained by you :) The implementation wasn't perfect, mostly straight from OH1, querying for unavailable data when off etc. But I had some stability issues when the connection was closed too frequently, after some time the projector wouldn't respond to any commands so I figure I didn't want to mess with it too much once it was stable enough for me. Nonetheless, glad to see that improved! I'll be sure to give it a try asap.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

bundles/org.openhab.binding.epsonprojector/.classpath

bundles/org.openhab.binding.epsonprojector/.project

...rojector/src/main/java/org/openhab/binding/epsonprojector/internal/EpsonProjectorDevice.java

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

bundles/org.openhab.binding.epsonprojector/src/main/resources/OH-INF/binding/binding.xml

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

...rojector/src/main/java/org/openhab/binding/epsonprojector/internal/EpsonProjectorDevice.java

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

ghys · 2020-11-22T09:32:34Z

Are you kidding? This is the field where the major contributors, present and past, are given credit. Author(s) != maintainer. Not that I care that much to have mine kept here, it's for the principle, much dissention has already happened because of things like that...

…

On Sun, Nov 22, 2020, 09:31 Fabian Wolter ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bundles/org.openhab.binding.epsonprojector/src/main/resources/OH-INF/binding/binding.xml <#9021 (comment)> : > @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="UTF-8"?> +<binding:binding id="epsonprojector" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xmlns:binding="https://openhab.org/schemas/binding/v1.0.0" + xsi:schemaLocation="https://openhab.org/schemas/binding/v1.0.0 https://openhab.org/schemas/binding-1.0.0.xsd"> + + <name>Epson Projector Binding</name> + <description>This binding is compatible with Epson projectors which support the ESC/VP21 protocol</description> + <author>Pauli Anttila, Yannick Schaus</author> If I understand correctly, you took over the binding and you are the maintainer now. So, I'd suggest that it's only your name in the author tag. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9021 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPJJMYQPYAML336HCWBNN3SRDD4RANCNFSM4TVLQU4Q> .

fwolter · 2020-11-22T09:37:53Z

It was only a suggestion. I'm fine if you keep all three authors.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

fwolter

LGTM

...va/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorDefaultConnector.java

...src/main/java/org/openhab/binding/epsonprojector/internal/handler/EpsonProjectorHandler.java

cpmeister · 2020-11-25T19:32:36Z

...src/main/java/org/openhab/binding/epsonprojector/internal/handler/EpsonProjectorHandler.java

+                pollingJob = scheduler.scheduleWithFixedDelay(() -> {
+                    for (Channel channel : channels) {
+                        // only query power & lamp time when projector is off
+                        if (isPowerOn || (channel.getUID().getId().equals(CHANNEL_TYPE_POWER)
+                                || channel.getUID().getId().equals(CHANNEL_TYPE_LAMPTIME))) {
+                            updateChannelState(channel);
+                        }
+                    }
+                }, 0, (pollingInterval > 0) ? pollingInterval : DEFAULT_POLLING_INTERVAL_SEC, TimeUnit.SECONDS);


You should avoid assigning class fields asynchronously since this handler might get disposed before you have a chance to assign it. This might lead to the case where this periodic task is never canceled.

Instead you should create this task during initialize and then inside the task just check if the device is connected or not before trying to update the channels.

This should be fixed now.

cpmeister · 2020-11-25T19:35:41Z

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

+                if (in.markSupported()) {
+                    in.reset();
+                }


At no point to you create any marks. What exactly are you resetting here?

I don't know. All the communication code is unchanged for the most part from #7370 and #6922.

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

...n/java/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorTcpConnector.java

cpmeister · 2020-11-25T19:38:51Z

...n/java/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorTcpConnector.java

+    private @Nullable InputStream in = null;
+    private @Nullable OutputStream out = null;


You only seem to write characters to these streams. Why not make these a Reader and Writer instead?

I had not considered this since the code was already written and had been scrutinized by the above mentioned PRs.

cpmeister · 2020-11-25T19:39:38Z

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

+    private @Nullable InputStream in = null;
+    private @Nullable OutputStream out = null;


same suggestion

cpmeister · 2020-11-25T19:43:32Z

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

+    public void serialEvent(SerialPortEvent arg0) {
+    }
+
+    private String sendMmsg(String data, int timeout) throws IOException, EpsonProjectorException {


How are you handling the case where multiple message are sent at the same time from multiple threads? Are you already preventing this from happening?

It is called by sendQuery() which is a synchronized method... Is that sufficient?

Looks sufficient, but it would be safer if you did the synchronization in this class instead. It looks safe enough as is though, so you don't have to change it.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

paulianttila · 2020-11-26T19:45:27Z

...src/main/java/org/openhab/binding/epsonprojector/internal/handler/EpsonProjectorHandler.java

+        if (THING_TYPE_PROJECTOR_SERIAL.equals(thingTypeUID)) {
+            device = new EpsonProjectorDevice(serialPortManager, getConfigAs(EpsonProjectorConfiguration.class));
+        } else {
+            device = new EpsonProjectorDevice(getConfigAs(EpsonProjectorConfiguration.class));
+        }


You should move this to initialize() method, otherwise user configuration changes are not taken in use.

Hmm, but then device still needs to be set to something here to satisfy the notnullbydefault. And EpsonProjectorDevice needs a connector in the constructor which is why I had done the EpsonProjectorDefaultConnector.

Well, notnullbydefault just forces you to think which field can be null and which not. In this case device can't be initialised or be final as it depends on the thing configuration an it can change during thing life time.

So it should be fine to be nullable

private @Nullable EpsonProjectorDevice device;

or if you want to avoid nulls, you could make it optional

private Optional<EpsonProjectorDevice> device = Optional.empty();

OK, fixed (using Optional).

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister · 2020-11-28T20:30:15Z

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java

+    public void serialEvent(SerialPortEvent arg0) {
+    }
+
+    private String sendMmsg(String data, int timeout) throws IOException, EpsonProjectorException {


Looks sufficient, but it would be safer if you did the synchronization in this class instead. It looks safe enough as is though, so you don't have to change it.

...src/main/java/org/openhab/binding/epsonprojector/internal/handler/EpsonProjectorHandler.java

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister · 2020-11-29T03:50:17Z

...rojector/src/main/java/org/openhab/binding/epsonprojector/internal/EpsonProjectorDevice.java

+                scheduler.scheduleWithFixedDelay(() -> {
+                    ready = true;
+                }, 0, 10000, TimeUnit.MILLISECONDS);


You need to keep track of the Future this returns so you can cancel it during disconnect.

I think this was put in to get rid of a 10 second thread.sleep(). It should only be a one time delay for 10 seconds before setting ready back to true, so I changed it to schedule() instead.

You still need to track the future so you can cancel it if the handler is disposed before 10 seconds expires.

I think there's a glitch in the Matrix #9021 (comment)

I made it cancel on disconnect.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister

Just some minor final tweaks.

...rojector/src/main/java/org/openhab/binding/epsonprojector/internal/EpsonProjectorDevice.java

...src/main/java/org/openhab/binding/epsonprojector/internal/handler/EpsonProjectorHandler.java

cpmeister · 2020-11-29T07:03:11Z

...src/main/java/org/openhab/binding/epsonprojector/internal/handler/EpsonProjectorHandler.java

+
+    private void updateChannelState(Channel channel) {
+        try {
+            if (!isLinked(channel.getUID()) && !channel.getUID().getId().equals(CHANNEL_TYPE_POWER)) {


power state will be queried even if the channel isn't linked?

Yes for two reasons:

If no channels are linked, power is still checked as a ping of the device to set the status to ONLINE and later OFFLINE if the connection is lost.

In the edge case of the user not linking the power channel, the power status still needs to be known to properly set the isPowerOn boolean (ie: only PWR? and LAMP? queries can be done when the power is off, because all others would fail).

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

* baseline EpsonProjector code from ysc * Improvements for OH3 * Finish epsonprojector binding for OH3 * improve exception logging * cleanup exception logging * Make connection specific thing types

* baseline EpsonProjector code from ysc * Improvements for OH3 * Finish epsonprojector binding for OH3 * improve exception logging * cleanup exception logging * Make connection specific thing types Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

mlobstein added 4 commits November 10, 2020 18:44

baseline EpsonProjector code from ysc

d913c6e

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

Improvements for OH3

59557e2

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

Improvements for OH3

4a08a94

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

Finish epsonprojector binding for OH3

b02daec

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister added new binding If someone has started to work on a binding. For a new binding PR. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2 labels Nov 16, 2020

Update a couple enums

5f4d660

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

fwolter self-assigned this Nov 20, 2020

fwolter requested changes Nov 20, 2020

View reviewed changes

review changes

446698c

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

mlobstein requested a review from fwolter November 21, 2020 03:03

fwolter requested changes Nov 22, 2020

View reviewed changes

improve exception logging

47cce9d

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

mlobstein requested a review from fwolter November 23, 2020 01:28

remove author tag

a6620a0

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

fwolter reviewed Nov 24, 2020

View reviewed changes

...ava/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorSerialConnector.java Outdated Show resolved Hide resolved

cleanup exception logging

82d573a

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

mlobstein requested a review from fwolter November 24, 2020 14:52

fwolter approved these changes Nov 24, 2020

View reviewed changes

fwolter added the cre Coordinated Review Effort label Nov 24, 2020

fwolter removed their assignment Nov 24, 2020

paulianttila reviewed Nov 24, 2020

View reviewed changes

...va/org/openhab/binding/epsonprojector/internal/connector/EpsonProjectorDefaultConnector.java Outdated Show resolved Hide resolved

cpmeister requested changes Nov 25, 2020

View reviewed changes

mlobstein added 3 commits November 25, 2020 17:48

Make connection specific thing types

49c93af

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

Review changes

f9154b0

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

Review changes

ebd4e49

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

paulianttila reviewed Nov 26, 2020

View reviewed changes

Review changes

478de5b

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister requested changes Nov 28, 2020

View reviewed changes

Review changes

116e25b

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister requested changes Nov 29, 2020

View reviewed changes

Review changes

18b4850

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister requested changes Nov 29, 2020

View reviewed changes

mlobstein added 2 commits November 29, 2020 10:07

Review changes

4519ee9

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

Review changes

bae2b80

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>

cpmeister approved these changes Dec 1, 2020

View reviewed changes

cpmeister added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 1, 2020

Hilbrand mentioned this pull request Dec 1, 2020

openHAB 1 addon compatibility overview with openHAB 3 #6179

Closed

cpmeister merged commit ffe252c into openhab:main Dec 4, 2020

cpmeister added this to the 3.0.0.M5 milestone Dec 4, 2020

mlobstein deleted the Epson_OH3 branch December 12, 2020 04:41

mlobstein mentioned this pull request Jan 4, 2021

[EpsonProjector] Add wireless and 3D support for Epson projector openhab/openhab1-addons#5772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[epsonprojector] Update epsonprojector binding for OH3 #9021

[epsonprojector] Update epsonprojector binding for OH3 #9021

mlobstein commented Nov 14, 2020 •

edited

Loading

ghys commented Nov 14, 2020

ghys commented Nov 22, 2020 via email

fwolter commented Nov 22, 2020

fwolter left a comment

cpmeister Nov 25, 2020 •

edited

Loading

mlobstein Nov 26, 2020

cpmeister Nov 25, 2020

mlobstein Nov 26, 2020 •

edited

Loading

cpmeister Nov 25, 2020

mlobstein Nov 26, 2020

cpmeister Nov 25, 2020

cpmeister Nov 25, 2020

mlobstein Nov 26, 2020

cpmeister Nov 28, 2020

paulianttila Nov 26, 2020

mlobstein Nov 26, 2020

paulianttila Nov 26, 2020 •

edited

Loading

mlobstein Nov 27, 2020

cpmeister Nov 28, 2020

cpmeister Nov 29, 2020

mlobstein Nov 29, 2020

cpmeister Nov 29, 2020

fwolter Nov 29, 2020

mlobstein Nov 29, 2020

cpmeister left a comment

cpmeister Nov 29, 2020

mlobstein Nov 29, 2020

		private @Nullable InputStream in = null;
		private @Nullable OutputStream out = null;

[epsonprojector] Update epsonprojector binding for OH3 #9021

[epsonprojector] Update epsonprojector binding for OH3 #9021

Conversation

mlobstein commented Nov 14, 2020 • edited Loading

ghys commented Nov 14, 2020

ghys commented Nov 22, 2020 via email

fwolter commented Nov 22, 2020

fwolter left a comment

Choose a reason for hiding this comment

cpmeister Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlobstein Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulianttila Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlobstein commented Nov 14, 2020 •

edited

Loading

cpmeister Nov 25, 2020 •

edited

Loading

mlobstein Nov 26, 2020 •

edited

Loading

paulianttila Nov 26, 2020 •

edited

Loading