[samsungtv] Frame TV Fixes, Improvements and New Channels #11895

NickWaterton · 2021-12-30T17:03:14Z

This PR is to update the samsungtv binding to better support the Frame TV.
Fixes: #11285
Fixes: #11284
It adds 5 new channels specifically for Frame TV's.
These new channels allow the art brightness and colour to be adjusted, plus the current artwork is given and can be selected. A thumbnail image is downloaded (of the current art), and a string interface to the art websocket is provided (so that additional commands can be sent).

In addition it addresses the problem that >2020 TV's cannot launch apps because of an API being removed by Samsung. A work around for the problem has been implemented, restoring this functionality.
This restores the function of the sourceApp and url channels.

A new feature has also been added to allow connection to the Smartthings API, which restores the function of 4 channels which otherwise would not function on TV's >2016 (sourceName, sourceId, channel and channelName).

The keyCode channel has been upgraded to allow:

KeyPress as well as keyClick
Chaining of key commands
Variable delays and keyPress/Release timing
Mouse move and mouse click commands
Text entry

KeyCode timing has been changed to use the scheduler, in place of fixed delays, to make the binding more responsive.

The sourceApp channel has been changed to allow the setting of slideshows on frame TV's. This channel now reports if art is being displayed or a slideshow (plus duration) in ArtMode, or the current app in TV mode. Blank if there is no current App displayed in TV mode (or the TV is off).

The original functions continue to work as normal on non Frame TV's - there are no breaking changes to the interface.

The documentation has been updated to reflect these changes, and explain in more detail how the binding can be used with examples.

The binding has been tested on TV's ranging from 2012 to 2021, but only 7 models have been tested, so more testing would be useful, hence the [WIP] label.

2014 and 2015 (Models H and J) have encrypted remotecontrol websockets, so these models are still not supported.

Signed-off-by: Nick Waterton n.waterton@outlook.com

org.openhab.binding.samsungtv-4.2.0-SNAPSHOT.jar
org.openhab.binding.samsungtv-4.2.0-SNAPSHOT.kar

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

NickWaterton · 2024-05-06T18:24:56Z

@NickWaterton - I checked a few commits for the binding over the last year and found a clear trail of unintentional reverted/overwritten code. My best idea would be for you to check all commits for the binding since you either started the work or performed the action that lead to the overwrite (if you remember).

@lsiepel - you found a few random reverts also, but since it now appears to be more systematic, this is something to keep in mind during the review.

I think the problem here is that I have been working on this pull request for three years - and have completely re-written a lot of the binding.

In the mean time, minor changes have been made to the original code, which does not work well (which is why I started updating it in the first place).

I have reviewed the changes, as best I can, and have made changes where the code still exists.

lsiepel

Another review pass. Still need to look at the readme.md file and investigate some specific area's but at least i have now covered the full PR. If i look back at the history i think we are moving in the right direction. Stil some area's that can be improved:

Thing status handling. Not all error situations are routed to Thingsatus + detail, but logged. From a user perspective it is best to set at least the thingstatus. Also, these can have i18n, but maybe that is outside of the scope of this PR.
Logging. This binding is loud, very loud. Three suggestions for improvement: 1. choose the right log level. 2. choose the right place (log inside the method responsible for the action and not before and after each time calling the method. 3. Ask yourself if the logging is really needed.
Optionals. There is no rule or guideline that says this cannot be used, so it is more a personal note. They are used a lot and many times they have no use or just obfuscate.
For example, i did not add review notes on this, but i really hope we can just use plain java without these wrappers.

public String getHostName() {
        return Optional.ofNullable(hostName).orElse("");
    }

public String getHostName() {
        return hostName != null hostName : "";
    }

....openhab.binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WolSend.java

...ng.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/protocol/WebSocketArt.java

...src/main/java/org/openhab/binding/samsungtv/internal/protocol/RemoteControllerWebSocket.java

...samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/handler/SamsungTvHandler.java

lsiepel · 2024-05-06T21:24:18Z

have reviewed the changes, as best I can, and have made changes where the

I fully understand, i think we got all of them now. I admire your determination to finish this PR. Let's try to do some review passes and finish this PR rather sooner then later.

lsiepel · 2024-05-06T21:29:11Z

ppears to be more systematic, this is something to keep in mind during the review.

Will do, any suggestions on a more systematic way of detecting?

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel

Thanks for providing a mooit so quick. While looking at the changes that have been made, i found some new. I also noticed that not all comments are addressed, possibly because they are behind the 'Load more' button. (exactly those are missing)

Some comments from this and the other review round can also be applied to different areas of this PR, would be nice if you can try to apply those suggestions on other places too.

....openhab.binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WolSend.java

...binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WakeOnLanUtility.java

...ngtv/src/main/java/org/openhab/binding/samsungtv/internal/service/SmartThingsApiService.java

NickWaterton · 2024-05-08T16:42:48Z

Thanks for providing a mooit so quick. While looking at the changes that have been made, i found some new. I also noticed that not all comments are addressed, possibly because they are behind the 'Load more' button. (exactly those are missing)

Some comments from this and the other review round can also be applied to different areas of this PR, would be nice if you can try to apply those suggestions on other places too.

Yes, missed those. Will try to fix them now.

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel

I have to come back to the readme, but here are some first comments. Very nice to see all the detailed documentation, really usefull for users. We only need to restructure it a bit and present it in a way that it is an easy read and in line with other bindings.

bundles/org.openhab.binding.samsungtv/README.md

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel · 2024-05-20T21:26:46Z

There will probably allways be some area's to improve, especially with these many changes, tests and PR age. I like to move forward and merge this. Regressions, new improvements can (and should) have a smaller scope and will certainly be merged faster :-)
@NickWaterton can you respond to the last open comments?

NickWaterton · 2024-05-21T12:46:28Z

There will probably allways be some area's to improve, especially with these many changes, tests and PR age. I like to move forward and merge this. Regressions, new improvements can (and should) have a smaller scope and will certainly be merged faster :-) @NickWaterton can you respond to the last open comments?

i think I have addressed all comments now.

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel

Thanks for this PR! LGTM. Let’s get some mileage on this with the snapshots. We have some time left before this gets into stable.

lsiepel · 2024-05-21T17:50:42Z

@NickWaterton As you have gained deep knowledge of this binding, you might want to create a follow up PR to add yourself to the codeowners file for this binding.

NickWaterton · 2024-05-21T18:01:21Z

@NickWaterton As you have gained deep knowledge of this binding, you might want to create a follow up PR to add yourself to the codeowners file for this binding.

Thanks for merging this!

How do I add myself to the codeowners file, and do a follow up PR?

lsiepel · 2024-05-21T20:02:55Z

Just create a new PR like any other and only edit the codeowners file:
https://github.com/openhab/openhab-addons/blob/main/CODEOWNERS

NickWaterton · 2024-05-21T20:11:35Z

Just create a new PR like any other and only edit the codeowners file: https://github.com/openhab/openhab-addons/blob/main/CODEOWNERS

A PR against the Main branch? or a new branch?

lsiepel · 2024-05-21T20:24:11Z

ust create a new PR like any othe

Just like how you created this PR. Create a feature branch agains main, chagne the file, publish the branch and push the changes to your github, create a pr here.

NickWaterton · 2024-05-22T12:56:35Z

ust create a new PR like any othe

Just like how you created this PR. Create a feature branch agains main, chagne the file, publish the branch and push the changes to your github, create a pr here.

OK, PR #16784 created.

Also @jlaur found a bug in the legacy code. (#16783) I have a fix for it. Do I do the same, branch, fix, push, PR etc to fix it?

Thanks.

jlaur · 2024-05-22T13:30:30Z

Also @jlaur found a bug in the legacy code. (#16783) I have a fix for it. Do I do the same, branch, fix, push, PR etc to fix it?

Yes, please.

) * [samsungtv] add certificate trust Signed-off-by: Nick Waterton <n.waterton@outlook.com> Signed-off-by: Paul Smedley <paul@smedley.id.au>

NickWaterton requested a review from paulianttila as a code owner December 30, 2021 17:03

NickWaterton force-pushed the samsungtv-framefixes branch from 3f66888 to 5260859 Compare December 30, 2021 17:23

[samsungtv] made requested changes

07e37f1

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel requested changes May 6, 2024

View reviewed changes

[samsungtv] reduced logging level and made suggested changes

43c133c

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel requested changes May 7, 2024

View reviewed changes

[samsungtv] improved websocket reconnect, and made requested changes

66924f0

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel requested changes May 9, 2024

View reviewed changes

NickWaterton added 2 commits May 9, 2024 16:04

[samsungtv] updated README.md

a9e22ed

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

[samsungtv] updated README.md

e0b25df

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel removed the request for review from kaikreuzer May 20, 2024 21:18

[samsungtv] updated README

f1baa2e

Signed-off-by: Nick Waterton <n.waterton@outlook.com>

lsiepel approved these changes May 21, 2024

View reviewed changes

lsiepel merged commit 20ace64 into openhab:main May 21, 2024
5 checks passed

lsiepel added this to the 4.2 milestone May 21, 2024

jlaur mentioned this pull request May 21, 2024

[samsungtv] Exception thrown when changing input source on legacy TV #16783

Closed

NickWaterton deleted the samsungtv-framefixes branch May 22, 2024 12:43

jlaur mentioned this pull request May 22, 2024

[samsungtv] Change codeowner #16784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[samsungtv] Frame TV Fixes, Improvements and New Channels #11895

[samsungtv] Frame TV Fixes, Improvements and New Channels #11895

NickWaterton commented Dec 30, 2021 •

edited

Loading

NickWaterton commented May 6, 2024

lsiepel left a comment

lsiepel commented May 6, 2024

lsiepel commented May 6, 2024

lsiepel left a comment

NickWaterton commented May 8, 2024

lsiepel left a comment

lsiepel commented May 20, 2024

NickWaterton commented May 21, 2024

lsiepel left a comment

lsiepel commented May 21, 2024

NickWaterton commented May 21, 2024

lsiepel commented May 21, 2024

NickWaterton commented May 21, 2024

lsiepel commented May 21, 2024

NickWaterton commented May 22, 2024

jlaur commented May 22, 2024

[samsungtv] Frame TV Fixes, Improvements and New Channels #11895

[samsungtv] Frame TV Fixes, Improvements and New Channels #11895

Conversation

NickWaterton commented Dec 30, 2021 • edited Loading

NickWaterton commented May 6, 2024

lsiepel left a comment

Choose a reason for hiding this comment

lsiepel commented May 6, 2024

lsiepel commented May 6, 2024

lsiepel left a comment

Choose a reason for hiding this comment

NickWaterton commented May 8, 2024

lsiepel left a comment

Choose a reason for hiding this comment

lsiepel commented May 20, 2024

NickWaterton commented May 21, 2024

lsiepel left a comment

Choose a reason for hiding this comment

lsiepel commented May 21, 2024

NickWaterton commented May 21, 2024

lsiepel commented May 21, 2024

NickWaterton commented May 21, 2024

lsiepel commented May 21, 2024

NickWaterton commented May 22, 2024

jlaur commented May 22, 2024

NickWaterton commented Dec 30, 2021 •

edited

Loading