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

[emotiva] Initial contribution #16499

Merged
merged 15 commits into from
Jun 13, 2024
Merged

[emotiva] Initial contribution #16499

merged 15 commits into from
Jun 13, 2024

Conversation

espenaf
Copy link
Contributor

@espenaf espenaf commented Mar 7, 2024 •
edited
Loading

This PR adds support for Emotiva Audio Corporation AV processors based on the Emotiva Network Remote Control protocol. Discovery is supported.

Binding has only been tested with an XMC-2, so please test and give feedback for other devices.

Has support for all Emotiva notification types in version 2.0 and 3.0 of the protocol, and adds support for almost all commands, has support for Zone 2 controls, browsing and controlling the menu and using the radio tuner.

@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/emotiva-av-processor-binding/154406/1

@mlobstein mlobstein added the new binding If someone has started to work on a binding. For a new binding PR. label Mar 9, 2024
@mlobstein
Copy link
Contributor

The build is failing because your commit it not signed. See here for instructions to fix it: https://github.com/openhab/openhab-addons/pull/16499/checks?check_run_id=22412921872

@espenaf espenaf force-pushed the emotiva branch 3 times, most recently from eca824b to e3162e3 Compare March 12, 2024 21:52
@espenaf espenaf force-pushed the emotiva branch 2 times, most recently from 3793c64 to 969b223 Compare March 31, 2024 20:43
austvik
austvik reviewed Apr 1, 2024
View reviewed changes
...inding.emotiva/src/main/java/org/openhab/binding/emotiva/internal/EmotivaCommandHandler.java Outdated Show resolved Hide resolved
...inding.emotiva/src/main/java/org/openhab/binding/emotiva/internal/EmotivaCommandHandler.java Outdated Show resolved Hide resolved
...ding.emotiva/src/main/java/org/openhab/binding/emotiva/internal/EmotivaProcessorHandler.java Show resolved Hide resolved
...ding.emotiva/src/main/java/org/openhab/binding/emotiva/internal/EmotivaProcessorHandler.java Show resolved Hide resolved
...tiva/src/main/java/org/openhab/binding/emotiva/internal/protocol/EmotivaControlCommands.java Show resolved Hide resolved
...ing.emotiva/src/main/java/org/openhab/binding/emotiva/internal/protocol/EmotivaDataType.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/config/config.xml Show resolved Hide resolved
@espenaf espenaf force-pushed the emotiva branch 3 times, most recently from bf10246 to 612fde5 Compare April 30, 2024 16:47
@espenaf espenaf changed the title [emotiva][WIP] Initial contribution [emotiva] Initial contribution Apr 30, 2024
@espenaf espenaf marked this pull request as ready for review April 30, 2024 16:53
@espenaf espenaf requested a review from a team as a code owner April 30, 2024 16:53
@espenaf espenaf force-pushed the emotiva branch 2 times, most recently from 10d3e3c to ec88727 Compare April 30, 2024 20:16
lsiepel
lsiepel reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this contribution. Looked at all except the key controller/handlers, will do in a next round.
For each comment, try to check if it can also apply to other parts of the code.

Please check that al SAT errors are fxied and the build comes without any warnings.

bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/addon/addon.xml Show resolved Hide resolved
...otiva/src/main/java/org/openhab/binding/emotiva/internal/protocol/EmotivaControlRequest.java Outdated Show resolved Hide resolved
...otiva/src/main/java/org/openhab/binding/emotiva/internal/protocol/EmotivaControlRequest.java Outdated Show resolved Hide resolved
...otiva/src/main/java/org/openhab/binding/emotiva/internal/protocol/EmotivaControlRequest.java Outdated Show resolved Hide resolved
...va/src/main/java/org/openhab/binding/emotiva/internal/discovery/EmotivaDiscoveryService.java Outdated Show resolved Hide resolved
lsiepel
lsiepel requested changes May 20, 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.

^^^

espenaf added 3 commits May 24, 2024 23:23
@espenaf
[emotiva] Initial contribution
6aa47bb 
Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
[emotiva] Code review round 1:
6fc0ef4 
* Renamed all channels to use - instead of _
* Adding state for a number of channels
* Start using i18n translations
* Cleanups and fixes

Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
Code review round 2:
97179d6 
* Wildcard for static imports.
* Removed Norwegian translation.
* Removed empty lines.
* Small fixes.

Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
[emotiva] Code review round 3:
69f0643 
* Discovery method
* Model and revisions as properties
* Rename keep-alive to last-seen

Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
[emotiva] Remove keep-alive/last-seen channel.
1fdacb8 
Signed-off-by: Espen Fossen <espenaf@junta.no>
@lsiepel
Copy link
Contributor

lsiepel commented May 26, 2024

While waiting for emotiva to respons to the question about an oiption list, there are some issues to fix.
Can you fix the SAT issues and build warnings. (see
https://github.com/openhab/openhab-addons/actions/runs/9237265388/artifacts/1537689743 )
Some comment about previous review is open: #16499 (comment)

After these are fixed i need one last review round.

espenaf added 2 commits May 26, 2024 23:03
@espenaf
[emotiva] Code review round 4
bad5b9f 
* Fix SAT issues
* Add config check and set to UNKNOWN on initialize

Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
[emotiva] Code review round 5
535e962 
* Rewrite tests from assertj to junit/mockito.

Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
Copy link
Contributor Author

espenaf commented May 26, 2024

While waiting for emotiva to respons to the question about an oiption list, there are some issues to fix. Can you fix the SAT issues and build warnings. (see https://github.com/openhab/openhab-addons/actions/runs/9237265388/artifacts/1537689743 ) Some comment about previous review is open: #16499 (comment)

After these are fixed i need one last review round.

Pushed new changes that fixes all the SAT issues.

espenaf added 2 commits May 27, 2024 23:45
@espenaf
[emotiva] Fix correct channel to step frequencies
7c5c3a8 
Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
[emotiva] Fixes after going through review checklist
a006dfc 
* Added catch of InterruptedIOException
* Move logging of message content to trace

Signed-off-by: Espen Fossen <espenaf@junta.no>
@lsiepel
Copy link
Contributor

lsiepel commented May 28, 2024

While waiting for emotiva to respons to the question about an oiption list, there are some issues to fix. Can you fix the SAT issues and build warnings. (see https://github.com/openhab/openhab-addons/actions/runs/9237265388/artifacts/1537689743 ) Some comment about previous review is open: #16499 (comment)
After these are fixed i need one last review round.

Pushed new changes that fixes all the SAT issues.

Thanks, Another step forward. There are 35 compile warnings, can you also look into those.

@espenaf
[emotiva] Fixed all compiler warnings.
29896c4 
Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
Copy link
Contributor Author

espenaf commented May 28, 2024

While waiting for emotiva to respons to the question about an oiption list, there are some issues to fix. Can you fix the SAT issues and build warnings. (see https://github.com/openhab/openhab-addons/actions/runs/9237265388/artifacts/1537689743 ) Some comment about previous review is open: #16499 (comment)
After these are fixed i need one last review round.

Pushed new changes that fixes all the SAT issues.

Thanks, Another step forward. There are 35 compile warnings, can you also look into those.

Now those have also been fixed.

Communication is established with Emotiva, just waiting on word from the people at Engineering.

@espenaf
[emotiva] Update tuner-program with the RDS Programme Types with i18n…
55d25b0 
… support.

Signed-off-by: Espen Fossen <espenaf@junta.no>
lsiepel
lsiepel requested changes May 31, 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.

Thanks for fixing the warnings and SAT errors. Looks very clean now. These are my final comments.

...ding.emotiva/src/main/java/org/openhab/binding/emotiva/internal/EmotivaProcessorHandler.java Outdated Show resolved Hide resolved
...ding.emotiva/src/main/java/org/openhab/binding/emotiva/internal/EmotivaProcessorHandler.java Show resolved Hide resolved
...ab.binding.emotiva/src/main/java/org/openhab/binding/emotiva/internal/dto/EmotivaAckDTO.java Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.emotiva/src/main/resources/OH-INF/thing/thing-types.xml Show resolved Hide resolved
@espenaf
[emotiva] Fixes after code review.
5809206 
Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
Copy link
Contributor Author

espenaf commented Jun 1, 2024 •
edited
Loading

Thanks for fixing the warnings and SAT errors. Looks very clean now. These are my final comments.

Fixed it all, added a few more translations and cleanup some descriptions.

@espenaf
[emotiva] Cleanup some description strings.
395cdcf 
Signed-off-by: Espen Fossen <espenaf@junta.no>
espenaf added 2 commits June 1, 2024 21:16
@espenaf
[emotiva] Better label for selected-movie-music.
35f9432 
Signed-off-by: Espen Fossen <espenaf@junta.no>
@espenaf
[emotiva] Fixed state option translation.
70834bf 
Signed-off-by: Espen Fossen <espenaf@junta.no>
@lsiepel lsiepel merged commit 67e6f8b into openhab:main Jun 13, 2024
5 checks passed
@lsiepel
Copy link
Contributor

lsiepel commented Jun 13, 2024

Now, you could add your binding's logo to the openHAB website. `
See https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website

Please also see the updated documentation here - SVG is now preferred:
https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui

@lsiepel lsiepel added this to the 4.2 milestone Jun 13, 2024
@espenaf
Copy link
Contributor Author

espenaf commented Jun 14, 2024

Now, you could add your binding's logo to the openHAB website. ` See https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website

Please also see the updated documentation here - SVG is now preferred: https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui

Thanks for the good work in reviewing it. I always like this attention to detail.

Opened openhab/openhab-docs#2310 for the logo in SVG format.

psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Jun 29, 2024
@espenaf @psmedley
[emotiva] Initial contribution (openhab#16499)
155d2d6 
* [emotiva] Initial contribution

Signed-off-by: Espen Fossen <espenaf@junta.no>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
@espenaf @pgfeller
[emotiva] Initial contribution (openhab#16499)
cab86ba 
* [emotiva] Initial contribution

Signed-off-by: Espen Fossen <espenaf@junta.no>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
@espenaf @joni1993
[emotiva] Initial contribution (openhab#16499)
852cf31 
* [emotiva] Initial contribution

Signed-off-by: Espen Fossen <espenaf@junta.no>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
@espenaf @matchews
[emotiva] Initial contribution (openhab#16499)
5584a1c 
* [emotiva] Initial contribution

Signed-off-by: Espen Fossen <espenaf@junta.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur left review comments

@lsiepel lsiepel lsiepel approved these changes

@austvik austvik austvik left review comments

Assignees
No one assigned
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
6 participants
@espenaf @openhab-bot @mlobstein @lsiepel @austvik @jlaur