Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lgwebos] Removal of connectsdk library and its dependencies #6060

Merged
merged 8 commits into from
Sep 26, 2019

Conversation

sprehn
Copy link
Contributor

@sprehn sprehn commented Sep 13, 2019

This is a major refactoring of the lgwebos binding.
This removes a lot of the connectsdk abstractions and allows us to use OH native json, websocket, UpnpDiscovery capabilities.

@sprehn sprehn requested a review from a team as a code owner September 13, 2019 21:08
@sprehn sprehn force-pushed the UpnpDiscoveryParticipant branch 7 times, most recently from 2a29aaf to ae4df49 Compare September 13, 2019 21:55
Replaced connectsdk with OH native json, websocket, UpnpDiscovery.
Signed-off-by: Sebastian Prehn <[email protected]>
@sprehn sprehn force-pushed the UpnpDiscoveryParticipant branch from ae4df49 to 32d13f7 Compare September 13, 2019 22:01
@TravisBuddy
Copy link

Travis tests were successful

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

@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 14, 2019
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I real appreciate removing those external dependencies. I left some comments in the code. Please pay special attention to the 3rdparty-content and the SAT report, I did not mention it everywhere.

@sprehn
Copy link
Contributor Author

sprehn commented Sep 15, 2019

where do I find 3rdparty-content?

@TravisBuddy
Copy link

Travis tests were successful

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

@J-N-K
Copy link
Member

J-N-K commented Sep 18, 2019

Regarding 3rdparty-content: At best look at the logreader binding. It's just a new Folder (with the same structure as /src/main which is named /src/3rdparty. To make this work in Eclipse you also need to adapt .classpath and for Maven the pom.xml (you can just copy the <build>...</build> part from logreader, no modification needed.

@J-N-K
Copy link
Member

J-N-K commented Sep 18, 2019

Otherwise looks good from my side.

…e call in LGWebOSHandler.

Signed-off-by: Sebastian Prehn <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @sprehn,
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
Copy link
Contributor Author

sprehn commented Sep 22, 2019

One user has reported a bug with TV: [LG] webOS TV OLED65E7P, webOS v5.80.35
I need to look into why it will not connect properly.

@Hilbrand
Copy link
Member

I did some testing and also had the bug. Here are 2 improvements:

  1. All classes that are parsed for gson parsing need to have a constructor without arguments.
  2. In LGWebOSTVSocket in the onMessage method: wrap the code in a try catch block to catch RuntimeException and just log as warning/error. The problem is. If something goes wrong with the code in onMessage and an exception is thrown this will result in the websocket connection being closed. This triggers the code to reopen the connection and a new call that will end up in onMessage. And if that causes the exception again. It will end up in a nasty loop. With lots of logging. This is what happens due to the problem mentioned in my point 1.

so that they do not disconnect the websocket only due to a misconfigured item, for example.
Signed-off-by: Sebastian Prehn <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @sprehn,
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
Copy link
Contributor Author

sprehn commented Sep 24, 2019

User reported that this fixed the issue.

@sprehn
Copy link
Contributor Author

sprehn commented Sep 24, 2019

@J-N-K do you still request any changes?

@Hilbrand
Copy link
Member

Hilbrand commented Sep 24, 2019

At least this needs to be fixed:

All classes that are parsed for gson parsing need to have a constructor without arguments

The catching doesn't fix the problem. It only blocks the binding from going into an endless loop due to it crashing. The crashing is caused (at least with my TV) by the exception thrown by gson due to the missing constructors.

@sprehn
Copy link
Contributor Author

sprehn commented Sep 24, 2019

sure will do, I actually also now see the other remarks.. had to click on "load more"

@TravisBuddy
Copy link

Travis tests were successful

Hey @sprehn,
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
Copy link

Travis tests were successful

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

@J-N-K J-N-K merged commit eef13d5 into openhab:master Sep 26, 2019
@J-N-K
Copy link
Member

J-N-K commented Sep 26, 2019

Thanks again.

@Hilbrand
Copy link
Member

@sprehn 2 questions:

  • Do you plan to add more features. Like dynamic state to get a list of all channels/apps?
  • Is it correct this changes the thing configuration? And users need to recreate the thing. In that case there need a comment in the change list on the openhab-distro repository.

@sprehn
Copy link
Contributor Author

sprehn commented Sep 26, 2019

@J-N-K thank you as well.

@Hilbrand, at the moment I am not planning to add more features, but you are right, the config parameter change, so I shall add it to the change list.

1 similar comment
@sprehn
Copy link
Contributor Author

sprehn commented Sep 26, 2019

@J-N-K thank you as well.

@Hilbrand, at the moment I am not planning to add more features, but you are right, the config parameter change, so I shall add it to the change list.

@sprehn
Copy link
Contributor Author

sprehn commented Sep 26, 2019

@Hilbrand I don't have a fork of the openhab distro repository. Could you review and add this comment for me, if you don't mind?

ALERT;LGWebOS Binding: The binding parameter 'localIP' was removed. The binding now uses system defaults for network communication. Thing type parameters 'deviceId' is no longer a parameter, but a property. Parameters 'host' and 'key' were added.

Hilbrand referenced this pull request in Hilbrand/openhab-distro Sep 27, 2019
@cweitkamp cweitkamp added this to the 2.5 milestone Sep 27, 2019
@cweitkamp cweitkamp added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible labels Sep 27, 2019
cweitkamp referenced this pull request in openhab/openhab-distro Sep 27, 2019
* [LGWebOS] Added alert about breaking change in 2.5

Related to https://github.com/openhab/openhab2-addons/pull/6060
Signed-off-by: Hilbrand Bouwkamp <[email protected]>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Oct 1, 2019
…b#6060)

* Removal of connectsdk library and its dependencies.
* Replaced connectsdk with OH native json, websocket, UpnpDiscovery.

Signed-off-by: Sebastian Prehn <[email protected]>
Signed-off-by: Rene Scherer <[email protected]>
thewiep pushed a commit to thewiep/openhab-addons that referenced this pull request Nov 23, 2019
…b#6060)

* Removal of connectsdk library and its dependencies.
* Replaced connectsdk with OH native json, websocket, UpnpDiscovery.

Signed-off-by: Sebastian Prehn <[email protected]>
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Dec 8, 2019
…b#6060)

* Removal of connectsdk library and its dependencies.
* Replaced connectsdk with OH native json, websocket, UpnpDiscovery.

Signed-off-by: Sebastian Prehn <[email protected]>
@wborn wborn changed the title [lgwebos] Removal of connectsdk library and its dependencies. [lgwebos] Removal of connectsdk library and its dependencies Dec 8, 2019
tmrobert8 pushed a commit to tmrobert8/openhab-addons that referenced this pull request Jan 21, 2020
…b#6060)

* Removal of connectsdk library and its dependencies.
* Replaced connectsdk with OH native json, websocket, UpnpDiscovery.

Signed-off-by: Sebastian Prehn <[email protected]>
Signed-off-by: Tim Roberts <[email protected]>
hww3 pushed a commit to hww3/openhab2-addons that referenced this pull request Feb 22, 2020
…b#6060)

* Removal of connectsdk library and its dependencies.
* Replaced connectsdk with OH native json, websocket, UpnpDiscovery.

Signed-off-by: Sebastian Prehn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants