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.

Sign up for GitHub

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

Jump to bottom

[roku] Improve TV discovery model name and add timeout #16210

Merged
merged 5 commits into from Jan 8, 2024
Merged

[roku] Improve TV discovery model name and add timeout #16210

merged 5 commits into from Jan 8, 2024

Conversation

mlobstein
Copy link
Contributor

@mlobstein mlobstein commented Jan 5, 2024 •
edited

Improve the label that is given to the thing by the discovery process. Previously TV things were not easily identified as a Roku device by the model name provided by the API. The friendly-model-name and user-device-location fields are now combined to help the user identify the thing in case of duplicate devices in different rooms. The strings are formatted to remove any extraneous characters as seen in the friendly-model-name example.

image

Addresses #15797 by setting a timeout on http requests sent to the roku device. Previously if the device was to crash while an http request was in progress it could cause the binding to hang.

Addresses #16164 by setting the uuid property to prevent manually added things from appearing in the inbox.

@mlobstein
Improve discovery TV model name and add timeout
fa1e095 
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein mlobstein added the enhancement An enhancement or new feature for an existing add-on label Jan 5, 2024
@mlobstein mlobstein linked an issue Jan 5, 2024 that may be closed by this pull request
[roku] Handler warning when TV crashes #15797
Closed
@mlobstein
additional readme edit
aa6877c 
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein mlobstein changed the title [roku] Improve discovery TV model name and add timeout [roku] Improve TV discovery model name and add timeout Jan 5, 2024
lsiepel
lsiepel approved these changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

LGTM

@morph166955
Copy link
Contributor

morph166955 commented Jan 5, 2024 •
edited

It may be better to use friendly-device-name or default-device-name to avoid duplicates and/or make it more easily identified in the inbox. Example:

<friendly-device-name>50" onn. Roku TV Guest Room</friendly-device-name>
<friendly-model-name>onn. Roku TV</friendly-model-name>
<default-device-name>onn. Roku TV - X01200ECK0EW</default-device-name>

<friendly-device-name>50" onn. Roku TV Girls</friendly-device-name>
<friendly-model-name>onn. Roku TV</friendly-model-name>
<default-device-name>onn. Roku TV - X01200G883DJ</default-device-name>

@mlobstein
Copy link
Contributor Author

mlobstein commented Jan 5, 2024 •
edited

It may be better to use friendly-device-name or default-device-name to avoid duplicates and/or make it more easily identified in the inbox.

It seems redundant because the serial number (as thing id) is seen below the thing label:
image

Also friendly-device-name has the inches/quote character in it that does not play nice when saving the thing. I could filter it out but thought 50 onn. Roku TV.... would look wierd.

@morph166955
Copy link
Contributor

That's fair. The friendly name then may make the most sense as it seems to be tied to what the user set when they configured the device. I have multiple of the same model in the house so this would make it obvious which is which.

@morph166955
Copy link
Contributor

morph166955 commented Jan 5, 2024 •
edited

Sorry I didn't see your edit about the quotes. Could you deliberately swap the quote for "in."? If not, perhaps concatenate in user-device-location? Just trying to figure out some way to make it easily identifiable. I have 6 of the same devices in the house, having something to go "oh that's that one" is very helpful.

<user-device-location>Guest bedroom</user-device-location>

@mlobstein
Copy link
Contributor Author

I have 6 of the same devices in the house, having something to go "oh that's that one" is very helpful.

<user-device-location>Guest bedroom</user-device-location>

What about friendly-model-name - user-device-location? I think that would accomplish "oh that's that one"

@morph166955
Copy link
Contributor

I agree. That should resolve everything. May still want to strip a quote in the event a manufacturer doesn't play nice and includes it.

@mlobstein
review changes
881a233 
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein
improve regex for label
66144aa 
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein
set uuid property with serial number
15aa391 
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein mlobstein linked an issue Jan 7, 2024 that may be closed by this pull request
[roku] representation-property seems to be tied to an unused field #16164
Closed
morph166955
morph166955 approved these changes Jan 7, 2024
View reviewed changes
Copy link
Contributor

@morph166955 morph166955 left a comment

Choose a reason for hiding this comment

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

LGTM and Thank You!

jlaur
jlaur approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

bundles/org.openhab.binding.roku/README.md
@@ -109,17 +109,19 @@ roku:roku_tv:mytv1 "My Roku TV" [ hostName="192.168.10.1", refresh=10 ]

String Player_ActiveApp "Current App: [%s]" { channel="roku:roku_player:myplayer1:activeApp" }
String Player_ActiveAppName "Current App Name: [%s]" { channel="roku:roku_player:myplayer1:activeAppName" }
String Player_Button "Send Command to Roku" { channel="roku:roku_player:myplayer1:button" }
String Player_Button "Send Command to Roku" { channel="roku:roku_player:myplayer1:button", autoupdate="false" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this reminded me of #12141. Do you think a similar solution would be relevant here as well? Not for this PR obviously, just a side note.

Copy link
Contributor Author

@mlobstein mlobstein Jan 9, 2024 •
edited

Choose a reason for hiding this comment

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

Seeing this reminded me of #12141. Do you think a similar solution would be relevant here as well? Not for this PR obviously, just a side note.

I can address that in a future PR. Is this just a substitute/implicit way of setting autoupdate="false"?

Copy link
Contributor

Choose a reason for hiding this comment

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

The veto policy will make sure that the command is sent, but without automatically updating the item state:
https://www.openhab.org/docs/developer/bindings/thing-xml.html#auto-update-policies

@jlaur jlaur merged commit c858e05 into openhab:main Jan 8, 2024
3 checks passed
@jlaur jlaur added this to the 4.2 milestone Jan 8, 2024
@mlobstein mlobstein deleted the roku_modelname branch January 9, 2024 02:03
@morph166955 morph166955 mentioned this pull request Jan 11, 2024
Merged
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Jan 27, 2024
@mlobstein @andrasU
[roku] Improve TV discovery model name and add timeout (openhab#16210)
015f29a 
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
@mlobstein @austvik
[roku] Improve TV discovery model name and add timeout (openhab#16210)
990ad74 
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur approved these changes

@lsiepel lsiepel lsiepel approved these changes

@morph166955 morph166955 morph166955 approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

[roku] representation-property seems to be tied to an unused field [roku] Handler warning when TV crashes
4 participants
@mlobstein @morph166955 @lsiepel @jlaur