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

[anthem] Initial contribution of binding for Anthem AV preamp/processors #14311

Merged
merged 11 commits into from
Mar 26, 2023
Merged

[anthem] Initial contribution of binding for Anthem AV preamp/processors #14311

merged 11 commits into from
Mar 26, 2023

Conversation

mhilbush
Copy link
Contributor

@mhilbush mhilbush commented Feb 1, 2023

This is a binding that can control Anthem AV preamp/processors using the Anthem IP control protocol. It has been tested on model AVM-60.

Signed-off-by: Mark Hilbush mark@hilbush.com

@mhilbush mhilbush added the new binding If someone has started to work on a binding. For a new binding PR. label Feb 1, 2023
@mhilbush mhilbush requested a review from a team as a code owner February 1, 2023 15:00
@mhilbush mhilbush self-assigned this Feb 1, 2023
@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/binding-for-anthem-av-processors/143960/2

@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/anthem-binding/144408/1

jlaur
jlaur requested changes Feb 15, 2023
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.

Thanks for providing this new binding. It looks good in general. I have added a few comments and questions from first review iteration.

bundles/org.openhab.binding.anthem/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/README.md Outdated Show resolved Hide resolved
...binding.anthem/src/main/java/org/openhab/binding/anthem/internal/AnthemBindingConstants.java Outdated Show resolved Hide resolved
...nthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemConnectionManager.java Outdated Show resolved Hide resolved
...nthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemConnectionManager.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/README.md Outdated Show resolved Hide resolved
...nthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemConnectionManager.java Outdated Show resolved Hide resolved
jlaur
jlaur reviewed Feb 15, 2023
View reviewed changes
bom/openhab-addons/pom.xml Outdated Show resolved Hide resolved
mhilbush
mhilbush commented Feb 16, 2023
View reviewed changes
Copy link
Contributor Author

@mhilbush mhilbush left a comment

Choose a reason for hiding this comment

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

I'm addressed almost all of your review feedback with the exception of a couple questions, as well as the refactoring of the AnthemConnectionManager.

bundles/org.openhab.binding.anthem/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/README.md Outdated Show resolved Hide resolved
...binding.anthem/src/main/java/org/openhab/binding/anthem/internal/AnthemBindingConstants.java Outdated Show resolved Hide resolved
...nthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemConnectionManager.java Outdated Show resolved Hide resolved
...nthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemConnectionManager.java Outdated Show resolved Hide resolved
....binding.anthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemHandler.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/addon/addon.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
@mhilbush
Copy link
Contributor Author

I believe I've addressed everything. There were a lot of changes but I'm pretty sure I haven't introduced any new issues.

jlaur
jlaur requested changes Mar 26, 2023
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.

Almost there - sorry for the delay.

bundles/org.openhab.binding.anthem/src/main/resources/OH-INF/addon/addon.xml Show resolved Hide resolved
...nthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemConnectionManager.java Outdated Show resolved Hide resolved
mhilbush added 10 commits March 26, 2023 14:27
@mhilbush
Initial contribution
6750f11 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Fix xml tags
784b22c 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Add to pom
8b6296e 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Address review feedback
d6b4611 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Address review comments. Refactor out AnthemCommunicationManager
6a3755e 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Generate i18n properties. Update README
f2adb46 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Remove required=true and fix spelling mistakes
b907db7 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Regenerate i18n properties
d97e943 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Change null annotation
f2ed3d4 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush
Declare connection in addon.xml
8571f33 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
jlaur
jlaur requested changes Mar 26, 2023
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.

It seems that I didn't previously review the handler. At least I had it marked as "not viewed" and I found some issues with status updates and logging.

....binding.anthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemHandler.java Outdated Show resolved Hide resolved
....binding.anthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemHandler.java Outdated Show resolved Hide resolved
....binding.anthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemHandler.java Outdated Show resolved Hide resolved
....binding.anthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemHandler.java Outdated Show resolved Hide resolved
....binding.anthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemHandler.java Outdated Show resolved Hide resolved
....binding.anthem/src/main/java/org/openhab/binding/anthem/internal/handler/AnthemHandler.java Outdated Show resolved Hide resolved
@mhilbush
Address review comments
91b3df7 
Signed-off-by: Mark Hilbush <mark@hilbush.com>
jlaur
jlaur approved these changes Mar 26, 2023
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

@jlaur jlaur merged commit 749cf58 into openhab:main Mar 26, 2023
@jlaur jlaur added this to the 4.0 milestone Mar 26, 2023
@morph166955
Copy link
Contributor

@jlaur As the base of this binding was from the androidtv binding which is pending review, I'm assuming it would be prudent to go through the changes here and adapt them into the androidtv PR?

@jlaur
Copy link
Contributor

jlaur commented Mar 26, 2023

@morph166955 - do you mean that the Android TV binding was based on the Anthem binding? Sure, if we resolved stuff here that you have copied, it would probably be a good idea to go through that and adapt to avoid similar comments in your PR. 🙂

@mhilbush mhilbush deleted the anthem-binding branch March 26, 2023 19:49
@morph166955
Copy link
Contributor

morph166955 commented Mar 26, 2023 •
edited
Loading

No, @mhilbush pulled from the androidtv binding to build this one. Either way the changes should apply so I'll make sure to pull them over.

#14282

@morph166955 morph166955 mentioned this pull request Mar 30, 2023
Merged
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
@mhilbush @FordPrfkt
[anthem] Initial contribution of binding for Anthem AV preamp/process…
f20c731 
…ors (openhab#14311)

* Initial contribution

Signed-off-by: Mark Hilbush <mark@hilbush.com>
@jlaur
Copy link
Contributor

jlaur commented Jul 20, 2023

@mhilbush - I forgot the classic reminder about adding a logo. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website

@mhilbush
Copy link
Contributor Author

Thanks for the reminder.

Done.

openhab/openhab-docs#2103

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

Assignees

@mhilbush mhilbush

Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mhilbush @openhab-bot @morph166955 @jlaur