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

[roku] binding - initial implementation #9571

Merged
merged 6 commits into from
Jan 18, 2021
Merged

Conversation

mlobstein
Copy link
Contributor

@mlobstein mlobstein requested a review from a team as a code owner December 29, 2020 02:01
@Skinah Skinah added the new binding If someone has started to work on a binding. For a new binding PR. label Dec 29, 2020
@Skinah
Copy link
Contributor

Skinah commented Dec 29, 2020

I'll post some tips whilst you wait for someone to do a review.

In your roku.xml file you have channels that are not camelCase.

active_app should become activeApp which will mean multiple files including the readme need to be updated. Apply this to all your channels.

@Skinah
Copy link
Contributor

Skinah commented Dec 29, 2020

catch (RokuHttpException e) {
logger.debug("Unable to launch app on Roku, appId: {}, Exception: {}", command.toString(),
e.getMessage());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
}

Remove the logger.debug line and instead just put the message into the ThingStatus

updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, "Enter a nice friendly string of text here.");

Signed-off-by: Michael Lobstein <[email protected]>
@mlobstein
Copy link
Contributor Author

updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, "Enter a nice friendly string of text here.");

Thanks, I updated the channel names. I think it is OK to leave off the exception message from the ThingStatusDetail. This exception is caused by a communication failure and in that situation the exception message will not provide anything useful to the user.

@morph166955
Copy link
Contributor

I'm going to play devils advocate for a second (because I can totally make a case in both directions). Why should the Roku get a binding? The new HTTP binding works really well with the Roku's with very minimal effort. I posted an example at https://community.openhab.org/t/roku-support-on-oh3-using-http-binding/110533 a few days back. What will this binding bring that the HTTP binding is unable to provide?

@mlobstein
Copy link
Contributor Author

What will this binding bring that the HTTP binding is unable to provide?

Generally speaking when compared to using the HTTP binding to scrape an endpoint, a dedicated binding will provide a more robust implementation. For roku, this binding provides device discovery, a dynamically updated drop-down to launch installed apps, status channels that update when a stream is playing (play state, elapsed time & total time) and a drop down with available remote button commands (filtered based on standalone roku vs roku tv). Also to speed the development of future improvements, all available xml data elements from the api are mapped in the dto objects.

Signed-off-by: Michael Lobstein <[email protected]>
@mlobstein mlobstein requested a review from Hilbrand December 30, 2020 00:37
Signed-off-by: Michael Lobstein <[email protected]>
@Skinah
Copy link
Contributor

Skinah commented Dec 30, 2020

What will this binding bring that the HTTP binding is unable to provide?

A) If someone is willing to build and maintain (and review it) it that is not "you" then why care? OP, Thank you for your work.
B) I always apricate it when I don't have to open up yet another API or search a forum where half the posts have half baked solutions to sort through.
C) Other projects have it so why not us? https://www.home-assistant.io/integrations/roku/ I don't like openHAB slipping behind even if it is only in 1 area, so lets encourage new contributors and make them feel valued.

Sorry I know you were playing Devils Advocate but that hits a raw nerve for me.

For the record I don't use Roku and have no intension of doing so, but good to know it is there.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM

@Hilbrand Hilbrand added the cre Coordinated Review Effort label Jan 4, 2021
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

LGTM

@cpmeister cpmeister merged commit 08833c7 into openhab:main Jan 18, 2021
@cpmeister cpmeister added this to the 3.1 milestone Jan 18, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/roku-support-on-oh3-using-http-binding/110533/11

@mlobstein mlobstein deleted the Roku_OH3 branch January 30, 2021 22:03
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
* Roku binding - initial implementation
* update channel names to camelCase
* review changes
* spelling
* update README.md

Signed-off-by: Michael Lobstein <[email protected]>
Signed-off-by: John Marshall <[email protected]>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
* Roku binding - initial implementation
* update channel names to camelCase
* review changes
* spelling
* update README.md

Signed-off-by: Michael Lobstein <[email protected]>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
* Roku binding - initial implementation
* update channel names to camelCase
* review changes
* spelling
* update README.md

Signed-off-by: Michael Lobstein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants