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

[kaleidescape] Kaleidescape Movie Player Binding - initial contribution #7568

Merged
merged 48 commits into from
Sep 15, 2020

Conversation

mlobstein
Copy link
Contributor

[kaleidescape] Initial contribution

This is the initial implementation of a Binding to control and retrieve information from a Kaleidescape movie player. All movie player components including the original K-Player series, M Class Players, Cinema One, Alto, and Strato are supported. As there are many good control options already available for these components, this binding focuses primarily on retrieving information for display purposes and to use in rules for controlling other Things such lighting, projector lens control, masking, etc. Any feedback or suggestions for improvement are welcome.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein mlobstein requested a review from a team as a code owner May 7, 2020 20:46
@mlobstein
Copy link
Contributor Author

mlobstein commented May 7, 2020

Test build available here:
https://github.com/mlobstein/mlobstein-beta-test/raw/master/org.openhab.binding.kaleidescape-2.5.8-SNAPSHOT.jar

If you receive an error about openhab-transport-serial, issue the following command in the openhab console:

feature:install openhab-transport-serial

@TravisBuddy
Copy link

Travis tests were successful

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

@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label May 8, 2020
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

@mlobstein
Copy link
Contributor Author

mlobstein commented Jun 3, 2020

@fwolter
Hold off on reviewing until #7371 is sorted out. This binding's Connector classes are of the same derivative, so I will need to make the requested changes here also.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
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 @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Everything looks pretty good. I was only able to find a couple of things worth pointing out.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
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.

Thanks for yet another contribution 😄 Looks good. Nice use of enum method overriding. Not really any big review comments.

new ArrayList<String>(Arrays.asList(GET_VIDEO_COLOR, GET_CONTENT_COLOR)));
}

initialCommands.forEach(command -> {
Copy link
Member

Choose a reason for hiding this comment

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

I have used ExpiringCache with a small timout, like 3 seconds. When the cache expires it will call the method passed. You could use this channels have the same command, such that when multiple of these channels are updated they are not all sending the command. And when using a cache you can probably iterate over the channels here, check if the channel is linked (i.e. if it actually make sense to update the channel) and then call handleRefresh with the channel instead of directly sending the command here. There is also an ExpiringCacheMap to handle multiple caches.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein
Copy link
Contributor Author

@Hilbrand I tried using ExpiringCache but it didn't really work with the framework of sending a command and getting one or more events back. And the constant refresh upon cache timeout is not needed because the device sends unsolicited updates. I implemented a similar scheme by saving events to a map and replaying them when refresh is called.

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 merged commit 197276b into openhab:2.5.x Sep 15, 2020
@Hilbrand Hilbrand added this to the 2.5.9 milestone Sep 15, 2020
@mlobstein mlobstein deleted the Kaleidescape branch September 15, 2020 13:36
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
…on (openhab#7568)

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Oct 8, 2020
…on (openhab#7568)

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
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