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

[dolbycp] Initial Contribution #16216

Merged
merged 26 commits into from Feb 11, 2024
Merged

[dolbycp] Initial Contribution #16216

merged 26 commits into from Feb 11, 2024

Conversation

Cybso
Copy link
Contributor

@Cybso Cybso commented Jan 5, 2024

Added new binding cp750 for Dolby Digital Cinema Processor CP750

I have written a new binding to communicate with a CP750, a common sound processor used in cinemas which can be controlled using a TCP socket.

The binding is based on the java library https://github.com/Cybso/cp750-java/ that I wrote earlier.

The release file is available at https://github.com/Cybso/openhab-addons/releases/tag/release-0.1

Signed-off-by: Roland Tapken <dev@cybso.de>
@Cybso Cybso requested a review from a team as a code owner January 5, 2024 21:45
@jlaur jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Jan 5, 2024
@jlaur jlaur changed the title Added new binding cp750 for Dolby Digital Cinema Processor CP750 [cp750] Add new binding for Dolby Digital Cinema Processor CP750 Jan 5, 2024
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.

Thanks for your contribution. Now i want this device too :-/
Left some comments to check, first pass, mainly looked at documentation I don't see a linked issues or community, so not sure if it was discussed; is cp750 the right name for this binding? On the website i also see that an CP850 exists, and maybe more will follow. If possible a more generic /brand / family / protocol level would be great so that in future it can support multiple devices, does that make sense?

bundles/pom.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.cp750/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.cp750/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.cp750/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.cp750/README.md Outdated Show resolved Hide resolved
@Cybso
Copy link
Contributor Author

Cybso commented Jan 6, 2024

Thank you for your review. I will apply the changes, but I'm busy the next two days, so I will have to wait until Monday or Tuesday.

I don't see a linked issues or community, so not sure if it was discussed

I saw no reason to discuss this because there wasn't a binding to speak with this device, and I needed one :-)

is cp750 the right name for this binding?

I have checked other sound processors from Dolby like CP650 or CP950. The CP650 does not seem to speak any TCP protocol at all, and the CP950 has a similar protocol, but only allows to set fader value and mute, no input channel. I also wanted to avoid the brand name "dolby".

@lsiepel

This comment was marked as resolved.

@jlaur

This comment was marked as resolved.

@lsiepel

This comment was marked as resolved.

@jlaur

This comment was marked as resolved.

@lsiepel

This comment was marked as resolved.

…sion

Signed-off-by: Roland Tapken <dev@cybso.de>
@jlaur jlaur changed the title [cp750] Add new binding for Dolby Digital Cinema Processor CP750 [dolbycp] Add new binding for Dolby Digital Cinema Processor CP750 Jan 7, 2024
Cybso and others added 2 commits January 8, 2024 13:45
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
@Cybso Cybso requested a review from lsiepel January 8, 2024 12:47
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
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.

Some more comments, not looked at the actual code yet.

Signed-off-by: Roland Tapken <dev@cybso.de>
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.

There are some open comments from a previous review, reviewed all files except DolbyCPHandler.java. That needs some further checks i'll do in the coming days.

Cybso and others added 2 commits January 16, 2024 18:43
Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
@Cybso
Copy link
Contributor Author

Cybso commented Jan 16, 2024

There are some open comments from a previous review, reviewed all files except DolbyCPHandler.java. That needs some further checks i'll do in the coming days.

Thank you very much for your comments.

Unless I have overlooked something, I have asked for feedback on the outstanding points. Especially with the textual Things definition I would need some support, because I have seen contradictory spellings in the README files of the existing bindings (see pending note on README.md in https://github.com/openhab/openhab-addons/pull/16216/files/888c0195f20902151c01c4526907534782be656f ).

@lsiepel
Copy link
Contributor

lsiepel commented Jan 16, 2024

There are some open comments from a previous review, reviewed all files except DolbyCPHandler.java. That needs some further checks i'll do in the coming days.

Thank you very much for your comments.

Unless I have overlooked something, I have asked for feedback on the outstanding points. Especially with the textual Things definition I would need some support, because I have seen contradictory spellings in the README files of the existing bindings (see pending note on README.md in https://github.com/openhab/openhab-addons/pull/16216/files/888c0195f20902151c01c4526907534782be656f ).

Strange:
os-version comment: #16216 (comment)
things example comment: #16216 (comment)

One aditional comment while looking at it: The dependency for de.cybso.cp750 needs to be listed in the notice file

@Cybso
Copy link
Contributor Author

Cybso commented Jan 21, 2024

I have implemented the trivial changes. The suggestions to DolbyCPHandler are a bit more complex, I have to think about that (and test them, of course).

Unless I have overlooked something, I have asked for feedback on the outstanding points. Especially with the textual Things definition I would need some support, because I have seen contradictory spellings in the README files of the existing bindings (see pending note on README.md in https://github.com/openhab/openhab-addons/pull/16216/files/888c0195f20902151c01c4526907534782be656f ).

Strange: os-version comment: #16216 (comment) things example comment: #16216 (comment)

I somehow have the feeling that we're talking at cross purposes. What do you mean by "strange" in this context? These are the pending questions I have referred to.

@lsiepel
Copy link
Contributor

lsiepel commented Jan 21, 2024

mehow have the feeling that we're talking at cross purposes. What do you mean by "strange" in this context? These are the pending questions I have referred to.

Take your time, no need to rush.

We might cross-talk there :-) I meant strange, as in I don't see your reply or change. For example, this one is left open:
#16216 (comment)
image

@Cybso
Copy link
Contributor Author

Cybso commented Jan 26, 2024

We might cross-talk there :-) I meant strange, as in I don't see your reply or change. For example, this one is left open: #16216 (comment)

I have commented to this one:

I wanted to show this version string on the control panel when the device comes online. Is this possible with thing properties?

But the comment shows a "pending" flag. How can I "commit" this comment? The button "Resolve Conversionen" marks the issue as resolved, doesn't it?

Edit: Finally found it. Thanks to https://github.com/orgs/community/discussions/10369

@lsiepel
Copy link
Contributor

lsiepel commented Jan 26, 2024

I wanted to show this version string on the control panel when the device comes online. Is this possible with thing properties?

Really think this os-version should be modelled as Thing property. It will then be shown in the Thing details page.

Cybso and others added 10 commits February 7, 2024 20:39
…rified README.md

Signed-off-by: Roland Tapken <dev@cybso.de>
…binding/dolbycp/internal/DolbyCPConfiguration.java

Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
…binding/dolbycp/internal/DolbyCPConfiguration.java

Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
…binding/dolbycp/internal/DolbyCPHandler.java

Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
…binding/dolbycp/internal/DolbyCPHandler.java

Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Roland Tapken <dev@cybso.de>
@Cybso
Copy link
Contributor Author

Cybso commented Feb 7, 2024

For personal reasons, I unfortunately had to leave this a little longer than I wanted.

I have now restructured the code and converted "osversion" into a property.

As far as I can see, the last open question is the example configuration, right?

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.

Very minor last comments before merger

Cybso and others added 3 commits February 10, 2024 19:39
Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
…thing/thing-types.xml

Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
…binding/dolbycp/internal/DolbyCPBindingConstants.java

Co-authored-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: Roland Tapken <github@tmp.dau-sicher.de>
@Cybso
Copy link
Contributor Author

Cybso commented Feb 10, 2024

Thank you very much! Please wait a moment before merge. I want to do a full test in my production environment but I have to wait until tomorrow morning before I can do a test in our cinema :-)

@lsiepel lsiepel added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Feb 10, 2024
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

@Cybso
Copy link
Contributor Author

Cybso commented Feb 11, 2024

I have done a full check in our theater using the new version and everything works fine.

Thank you very much for your help, @lsiepel!

@lsiepel lsiepel merged commit 0745cdb into openhab:main Feb 11, 2024
3 checks passed
@lsiepel lsiepel added this to the 4.2 milestone Feb 11, 2024
@lsiepel lsiepel changed the title [dolbycp] Add new binding for Dolby Digital Cinema Processor CP750 [dolbycp] Initial Contribution Feb 11, 2024
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* Added new binding dolbycp

Signed-off-by: Roland Tapken <dev@cybso.de>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@lsiepel lsiepel removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

3 participants