[WIP][lgwebos] Detection of power on/off with UPnP #7122

lolodomo · 2020-03-08T11:41:34Z

Also fix #7119

Signed-off-by: Laurent Garnier [email protected]

lolodomo · 2020-03-08T11:45:49Z

@sprehn : I let you check but it works very well for me.

I tested fully the following cases:

thing created from the discovery in Paper UI (deviceId included in the discovery)
thing created in Paper UI without the deviceId => thing configuration is then updated
thing created from a config file with the deviceId setting set

In these cases, I see the Power channel immediately updated when TV switched on or off.

sprehn · 2020-03-08T18:02:49Z

on my TV this version does not give any improvement yet.

the following method gets called with status false exactly when the websocket connection breakes, 20-30seconds after the screen goes dark.

 public void onStatusChanged(boolean status) {
        logger.debug("onStatusChanged status={}", status);
        postUpdate(CHANNEL_POWER, status ? OnOffType.ON : OnOffType.OFF);
    }

in thingDiscovered I think we should check for the MediaRenderer with same host as our TV.
not THING_TYPE_WEBOSTV, cause that will disappear in UPNP too late.
In that case update the thing properties should not happen, as it will be a different device.
if we use the udn of media renderer then it should probably not be stored in deviceid config. it could either be a property, or even a variable in the handler.

lolodomo · 2020-03-08T18:48:43Z

Do you have the UPnP Device Spy app on your PC ?
I would like to be sure that with your TV, the UPnP devices are disappearing not all at the same time.
In case the MediaRenderer device is disappearing earlier and fast, we can decide to rely on this one rather than the other, that is not a big problem.

As I told you, in my case, I see the 4 UPnP devices appearing/disappearing immediately when I use the standby button of the remote control. So it does not matter what UPnP device is considered by the discovery service.

In my case, the 4 UPnP devices have the same "friendly name". Note that I think we should rather use this "friendly name" to set the model name property.

As soon as you confirm that the MediaRenderer UPnP device is the one to consider because it appears/disappears faster, I update the discovery code. This is only a minor change to do in the class LGWebOSUpnpDiscoveryParticipant (mainly in the method getThingUID). All the other code I introduced will remain unchanged.

sprehn · 2020-03-08T21:29:58Z

Hi,

using a macbook and Upnp Analyser, but this does not seem to capture those events nicely so that it actually helps me. To debug the situation I use the shutdown detector code:

@Override
    public void remoteDeviceRemoved(@Nullable Registry registry, @Nullable RemoteDevice device) {
        if (device == null) {
            return;
        }
        String ip = device.getIdentity().getDescriptorURL().getHost();
        thingRegistry.getAll().stream().filter(thing -> THING_TYPE_WEBOSTV.equals(thing.getThingTypeUID()))
                .filter(thing -> ip.equals(thing.getConfiguration().get(CONFIG_HOST))).forEach(thing -> {
                    ThingHandler handler = thing.getHandler();
                    if (handler != null) {
                        logger.debug("Detected device shutdown: {}", device);
                        logger.debug("device type: {}", device.getType());
                        for (RemoteService s : device.getServices()) {
                            logger.debug("service id: {}", s.getServiceId());
                            logger.debug("service type: {}", s.getServiceType());
                        }
                        ((LGWebOSHandler) handler).postUpdate(CHANNEL_POWER, OnOffType.OFF);
                    }
                });
    }

I see device urn:schemas-upnp-org:device:MediaRenderer:1 and its services immediatly and only 24 seconds later the service urn:lge-com:service:webos-second-screen:1 go down.

2020-03-08 21:58:40.384 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - device type: urn:schemas-upnp-org:device:MediaRenderer:1
2020-03-08 21:58:40.384 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service id: urn:upnp-org:serviceId:AVTransport
2020-03-08 21:58:40.384 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service type: urn:schemas-upnp-org:service:AVTransport:1
2020-03-08 21:58:40.385 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service id: urn:upnp-org:serviceId:ConnectionManager
2020-03-08 21:58:40.385 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service type: urn:schemas-upnp-org:service:ConnectionManager:1
2020-03-08 21:58:40.385 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service id: urn:upnp-org:serviceId:RenderingControl
2020-03-08 21:58:40.385 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service type: urn:schemas-upnp-org:service:RenderingControl:1
2020-03-08 21:59:04.359 [DEBUG] [very.LGWebOSUpnpDiscoveryParticipant] - Found LG WebOS TV: (RemoteDevice) Identity: (RemoteDeviceIdentity) UDN: uuid:6da62662-1369-a82d-95c7-353c235ed503, Descriptor: http://192.168.2.119:1523/, Root: true
2020-03-08 21:59:04.359 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - Detected device shutdown: (RemoteDevice) Identity: (RemoteDeviceIdentity) UDN: uuid:6da62662-1369-a82d-95c7-353c235ed503, Descriptor: http://192.168.2.119:1523/, Root: true
2020-03-08 21:59:04.359 [DEBUG] [ebos.internal.handler.LGWebOSHandler] - Received thingRemoved event: lgwebos:WebOSTV:6da62662-1369-a82d-95c7-353c235ed503
2020-03-08 21:59:04.359 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - device type: urn:schemas-upnp-org:device:Basic:1
2020-03-08 21:59:04.360 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service id: urn:lge-com:serviceId:webos-second-screen-3000-3001
2020-03-08 21:59:04.360 [DEBUG] [iscovery.LGWebOSUpnpShutdownDetector] - service type: urn:lge-com:service:webos-second-screen:1
2020-03-08 21:59:04.397 [DEBUG] [bos.internal.handler.LGWebOSTVSocket] - WebSocket Closed - Code: 1001, Reason: server shutting down

If we changed what we listen for in LGWebOSUpnpDiscoveryParticipant, then it would change the deviceId for all current users, and we know that listening for urn:lge-com:service:webos-second-screen:1 works well for discovery.
Users won't like such a change, as they will have to relink all channels to a new Thing.

btw. thingRemoved already got called without registering as UpnpIOParticipant or my ShutdownDetector when webos-second-screen disappears - only too late.

I thought the whole point was to use UpnpIOParticipant to react on a different device.

But question would remain how we can discover it's UDN.

sprehn · 2020-03-08T22:35:54Z

Would it be ok to go through all devices in the registry once connected and attempt to find the one which matches MediaRenderer on the same host:

this.upnpService.getRegistry().getDevices()

it would be sufficient to store that device's UDN in a property and we could then register the UpnpIOParticipant based on it.

lolodomo · 2020-03-09T07:25:01Z

If we change the UPnP device you are relying on in the discovery service, this will be fully transparent for any user and they will not have to relink channels. The deviceId is only a property of the current thing. It is also used as in the thing UID as default thing id but any user could have changed it.

I will update the code this evening, so that at least you can then confirm that it is working better for you.

Addtionnaly I will think if the discovery service can rely on the two UPnP devices. This could require to change the representation property to something else.

lolodomo · 2020-03-11T22:45:29Z

@sprehn : can you please make a new try after my last change ?
Note that it is possible that you need to clear your inbox using the console command smarthome:inbox clear

It is possible that implementing UpnpIOParticipant is finally useless and that implementing DiscoveryListener could be sufficient.
First, I am waiting for your positive feedback and then I will simplify my PR.

lolodomo · 2020-03-12T07:26:21Z

Additionally I will evaluate if we can keep a simple property to store the UDN, It would avoid the edition of the thing configuration. I have the feeling it could work.

sprehn · 2020-03-13T20:53:21Z

Hi,
have to pause the work for the moment, as I don't have access to my TV to run tests.
This is due to day-care closure in Germany. Have moved temporary ...

lolodomo · 2020-03-14T11:15:29Z

I rebased my PR to have in my branch all the recently merged changes.

TravisBuddy · 2020-03-14T11:24:53Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

TravisBuddy · 2020-03-15T10:50:07Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-03-15T10:52:15Z

@sprehn : I have now terribly simplified my proposal. Everything is based on the discovery service. No need for additional JUPnP stuff, no need to transform the deviceId property into a configuration setting.
For me, it is working perfectly. I will wait for your feedback.
To summarize, the discovery is now based on the MediaRender device and the thing handler is considering the changes in the thing discovery to update the power channel.

TravisBuddy · 2020-03-21T12:41:53Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

TravisBuddy · 2020-03-28T08:27:50Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-03-28T16:21:25Z

@sprehn : I integrated two fix:

update the key in the thing configuration only if the key is new. This avoids the thing update when the user has already set the right key.
abort the init process in case the thing handler is disposed while the init thread is still sending and receiving messages. This is done in the method handling the response.

TravisBuddy · 2020-03-28T16:22:00Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

sprehn · 2020-03-29T21:38:02Z

Hi @lolodomo

Re "Ignore power off command when the TV is already off"

So it seams the TV actually starts up again, if it receives a second "ssap://system/turnOff" message, during a specific time window while shutting down. Your proposed fix should prevent that we sent "ssap://system/turnOff" twice, if the user sends two OFF events. Seems to be a counter intuitive response to a "turnOff" request, so maybe it means it aborts shutdown.

Now I wonder, will this be the same, if the user sends any another event, e.g. "ssap://tv/channelUp" while shutting down? If so, we should handle this further down in the socket implementation's state. I actually had a state DISCONNECTING at some point, which I later removed. We could use this to ignore any events being sent to the TV during shutdown.

lolodomo · 2020-04-05T10:11:22Z

@sprehn : will you review and validate at least few of my changes if I cut this PR into several separate PRs, even if you have no way to test ?

lolodomo · 2020-04-05T11:24:02Z

Now I wonder, will this be the same, if the user sends any another event, e.g. "ssap://tv/channelUp" while shutting down? If so, we should handle this further down in the socket implementation's state. I actually had a state DISCONNECTING at some point, which I later removed. We could use this to ignore any events being sent to the TV during shutdown.

I updated my rule that was triggering the issue. I now increase the volume, mute it and set a new channel. I will see what happens after few days but I guess these commands will be just ignored and will not turn on the TV. like "ssap://system/turnOff" can do.

lolodomo · 2020-04-05T12:05:40Z

@sprehn : I created separate PRs with the hope we can merge few fixes quickly without your testing, while the main part of this PR will require your testing.

TravisBuddy · 2020-04-05T22:09:38Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-04-05T22:09:47Z

New rebase done to fix the conflict.

TravisBuddy · 2020-04-09T09:27:26Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-04-09T09:39:53Z

Now I wonder, will this be the same, if the user sends any another event, e.g. "ssap://tv/channelUp" while shutting down? If so, we should handle this further down in the socket implementation's state. I actually had a state DISCONNECTING at some point, which I later removed. We could use this to ignore any events being sent to the TV during shutdown.

I updated my rule that was triggering the issue. I now increase the volume, mute it and set a new channel. I will see what happens after few days but I guess these commands will be just ignored and will not turn on the TV. like "ssap://system/turnOff" can do.

@sprehn : I can confirm that I did not encounter any problem since the change. I thing the problem is really solved now and was only a problem with the "ssap://system/turnOff" command.

...lgwebos/src/main/java/org/openhab/binding/lgwebos/internal/handler/LGWebOSConfiguration.java

sprehn · 2020-03-08T21:33:48Z

...nding.lgwebos/src/main/java/org/openhab/binding/lgwebos/internal/handler/LGWebOSHandler.java

+            if (host != null && host.equals(getLGWebOSConfig().getHost())) {
+                // Thing matching the discovery
+
+                // Update the thing properties from the discovery result


why update properties from discovery result. Isn't this already handled in UpnpDiscoveryParticipant?

so the media renderer will not have those additional properties I assume... ok, fine for me.

If your thing is declared in a config file, it will have no properties set after startup. And it will not be set by the thing discovery.
Of course, the framework core code will check first if there are any change.

And of course, we absolutely need to change (if necessary) the deviceId property.
This allows an automatic update of all existing things when you move from the old code to the new code.

sprehn · 2020-03-13T20:57:45Z

...ain/java/org/openhab/binding/lgwebos/internal/discovery/LGWebOSUpnpDiscoveryParticipant.java

+                && device.getDetails().getManufacturerDetails().getManufacturer().toUpperCase()
+                        .contains("LG ELECTRONICS")
+                && device.getDetails().getModelDetails().getModelDescription() != null
+                && device.getDetails().getModelDetails().getModelDescription().toUpperCase().contains("WEBOSTV")) {
            logger.debug("Found LG WebOS TV: {}", device);
            return new ThingUID(THING_TYPE_WEBOSTV, device.getIdentity().getUdn().getIdentifierString());


I do agree that this will work if in case manufacturer and model descriptor match. I have certainty if all devices will consistently report values this way. If so, it should work.

This will be a different ID compared to the second screen device. and the representational property will be different. So I do think all users will see a new discovery result, next to their previously discovered TV.

I'll need to test this

I am not fully sure whether the previous discovery will be kept. Maybe just replaced.

But you certainly understood that the users will have nothing to change on their things. Their deviceId property will be updated automatically by the binding.

sprehn · 2020-03-15T18:26:40Z

...ain/java/org/openhab/binding/lgwebos/internal/discovery/LGWebOSUpnpDiscoveryParticipant.java

+                // .withProperty(PROPERTY_MODEL_NAME,
+                // device.getDetails().getModelDetails().getModelName() + " "
+                // + device.getDetails().getModelDetails().getModelNumber())
+                .withProperty(PROPERTY_MODEL_NAME, device.getDetails().getFriendlyName())


sure you want friendly name in label and model?
why not use modelname + model number as you proposed earlier?

Because they are not what we expect on this device.
For example, in my case:

modelname = LG TV

modelNumber = 1.0

friendlyName remains the same on all devices.

Friendly name is what the user sets as the name in the TV settings. It is not necessarily the model.

...nding.lgwebos/src/main/java/org/openhab/binding/lgwebos/internal/handler/LGWebOSHandler.java

sprehn · 2020-04-09T22:14:23Z

...nding.lgwebos/src/main/java/org/openhab/binding/lgwebos/internal/handler/LGWebOSHandler.java

+                logger.debug("thingDiscovered: discovery with UID {} matching thing {}", result.getThingUID(),
+                        getThing().getUID());
+
+                channelHandlers.get(CHANNEL_POWER).onDeviceReady(CHANNEL_POWER, this);


websocket connected iff power on

for users, which don't use upnp this wont work.
it may create a situation, when power channel is on, but we are not connected yet.

I don't understand this part of your comment "for users, which don't use upnp this wont work.". The binding uses UPnP discovery, this is not a user's choice.
And for the few users having UPnP not working in their network, of course this PR will not help them as it is based on UPnP. But it probably concerns only a very small minority.

You are right, it can happen, at startup, that the channel is set to ON just before the websocket is connected and TV is considered as registered, as this is done in parallel by 2 different threads.

But this is required to have a fast update of the channel when you power on the TV while the websocket was still alive and connected.
In fact, the goal of this PR is a fast update of the power channel (ON or OFF), not based on websocket connection status because the websocket remains connected during a certain time after the TV is switched to OFF.

But I agree that this channel represents whether the TV is ON or OFF, not whether the websocket is connected and TV is registered. This is an essential point.

Considering the PR #7355, I think that state update to ON will become unnecessary.

sprehn · 2020-04-09T22:18:58Z

...nding.lgwebos/src/main/java/org/openhab/binding/lgwebos/internal/handler/LGWebOSHandler.java

+    public void thingRemoved(DiscoveryService source, ThingUID thingUID) {
+        if (!udn.isEmpty() && thingUID.getId().equals(udn)) {
+            logger.debug("thingRemoved: discovery with UID {} matching thing {}", thingUID, getThing().getUID());
+            channelHandlers.get(CHANNEL_POWER).onDeviceRemoved(CHANNEL_POWER, this);


same comment as in thing discovered.

This line is the most important line of this PR. It does what you expect since a long time, that is updating the power channel to OFF as fast as possible, even when the websocket is still connected.

Considering the PR #7355, I think that state update to OFF will become unnecessary.

TravisBuddy · 2020-04-10T10:38:36Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Also fix openhab#7119 Signed-off-by: Laurent Garnier <[email protected]>

lolodomo · 2020-04-10T19:30:00Z

New rebase.
Here is a new jar for testing:
org.openhab.binding.lgwebos-2.5.4-SNAPSHOT.zip

TravisBuddy · 2020-04-10T19:31:00Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-04-16T12:24:08Z

In fact, with PR #7355 handling the update of the power channel state, I think this PR could become simply useless.

lolodomo · 2020-04-16T12:37:40Z

The only interest will be the update of few properties we get only through UPnP discovery.

sprehn

In fact, with PR #7355 handling the update of the power channel state, I think this PR could become simply useless.

Agree, and the method in #7355 is working for every user, not only those who are able to use Upnp from a network perspective.

sprehn · 2020-04-10T08:53:14Z

...ain/java/org/openhab/binding/lgwebos/internal/discovery/LGWebOSUpnpDiscoveryParticipant.java

+                // .withProperty(PROPERTY_MODEL_NAME,
+                // device.getDetails().getModelDetails().getModelName() + " "
+                // + device.getDetails().getModelDetails().getModelNumber())
+                .withProperty(PROPERTY_MODEL_NAME, device.getDetails().getFriendlyName())


Friendly name is what the user sets as the name in the TV settings. It is not necessarily the model.

lolodomo · 2020-04-16T18:40:08Z

The main goal of this PR has disappeared after the merge of the PR #7355
PR no more necessary.

lolodomo requested a review from sprehn as a code owner March 8, 2020 11:41

lolodomo force-pushed the lgwebos_channel branch from 2675a80 to 6eeb0f1 Compare March 8, 2020 11:51

openhab deleted a comment from TravisBuddy Mar 8, 2020

sprehn closed this Mar 8, 2020

sprehn reopened this Mar 8, 2020

lolodomo force-pushed the lgwebos_channel branch 2 times, most recently from 6d84479 to 69adaed Compare March 11, 2020 18:12

openhab deleted a comment from TravisBuddy Mar 11, 2020

lolodomo added the work in progress A PR that is not yet ready to be merged label Mar 11, 2020

lolodomo changed the title ~~[lgwebos] Detection of power on/off with UPnP~~ [WIP][lgwebos] Detection of power on/off with UPnP Mar 11, 2020

lolodomo force-pushed the lgwebos_channel branch from bc6c02b to d84ae46 Compare March 14, 2020 11:14

openhab deleted a comment from TravisBuddy Mar 14, 2020

lolodomo force-pushed the lgwebos_channel branch from d84ae46 to 24c0893 Compare March 15, 2020 10:45

lolodomo force-pushed the lgwebos_channel branch from 24c0893 to 8bde76d Compare March 21, 2020 09:15

lolodomo force-pushed the lgwebos_channel branch from 929988e to b82002c Compare April 5, 2020 22:07

sprehn requested changes Apr 9, 2020

View reviewed changes

lolodomo force-pushed the lgwebos_channel branch 2 times, most recently from b82002c to 6c1d7df Compare April 10, 2020 10:36

[lgwebos] Detection of power on/off with UPnP

c884e1b

Also fix openhab#7119 Signed-off-by: Laurent Garnier <[email protected]>

lolodomo force-pushed the lgwebos_channel branch from 6c1d7df to c884e1b Compare April 10, 2020 19:28

sprehn reviewed Apr 16, 2020

View reviewed changes

lolodomo closed this Apr 16, 2020

lolodomo deleted the lgwebos_channel branch October 10, 2020 10:00

[WIP][lgwebos] Detection of power on/off with UPnP #7122

[WIP][lgwebos] Detection of power on/off with UPnP #7122

Conversation

lolodomo commented Mar 8, 2020 • edited Loading

lolodomo commented Mar 8, 2020

sprehn commented Mar 8, 2020

lolodomo commented Mar 8, 2020 • edited Loading

sprehn commented Mar 8, 2020 • edited Loading

sprehn commented Mar 8, 2020

lolodomo commented Mar 9, 2020 • edited Loading

lolodomo commented Mar 11, 2020

lolodomo commented Mar 12, 2020

sprehn commented Mar 13, 2020

lolodomo commented Mar 14, 2020

TravisBuddy commented Mar 14, 2020

Travis tests were successful

TravisBuddy commented Mar 15, 2020

Travis tests were successful

lolodomo commented Mar 15, 2020

TravisBuddy commented Mar 21, 2020

Travis tests were successful

TravisBuddy commented Mar 28, 2020

Travis tests were successful

lolodomo commented Mar 28, 2020 • edited Loading

TravisBuddy commented Mar 28, 2020

Travis tests were successful

sprehn commented Mar 29, 2020 • edited Loading

lolodomo commented Apr 5, 2020 • edited Loading

lolodomo commented Apr 5, 2020

lolodomo commented Apr 5, 2020

TravisBuddy commented Apr 5, 2020

Travis tests were successful

lolodomo commented Apr 5, 2020 • edited Loading

TravisBuddy commented Apr 9, 2020

Travis tests were successful

lolodomo commented Apr 9, 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

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

lolodomo Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

lolodomo Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

TravisBuddy commented Apr 10, 2020

Travis tests were successful

lolodomo commented Apr 10, 2020

TravisBuddy commented Apr 10, 2020

Travis tests were successful

lolodomo commented Apr 16, 2020

lolodomo commented Apr 16, 2020

sprehn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo commented Apr 16, 2020

lolodomo commented Mar 8, 2020 •

edited

Loading

lolodomo commented Mar 8, 2020 •

edited

Loading

sprehn commented Mar 8, 2020 •

edited

Loading

lolodomo commented Mar 9, 2020 •

edited

Loading

lolodomo commented Mar 28, 2020 •

edited

Loading

sprehn commented Mar 29, 2020 •

edited

Loading

lolodomo commented Apr 5, 2020 •

edited

Loading

lolodomo commented Apr 5, 2020 •

edited

Loading

lolodomo commented Apr 9, 2020 •

edited

Loading

lolodomo Apr 10, 2020 •

edited

Loading

lolodomo Apr 16, 2020 •

edited

Loading