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

[gridbox] Initial contribution #16664

Merged
merged 18 commits into from
May 19, 2024
Merged

[gridbox] Initial contribution #16664

merged 18 commits into from
May 19, 2024

Conversation

benediktkuntz
Copy link
Contributor

Viessmann GridBox Binding

Initial contribution for a new binding for connecting to the Viessmann GridBox, see README.MD for information about it

Description

My first contribution, be merciful ;) Only tested with my private setup, any comments or testing highly appreciated...

@benediktkuntz
[gridbox] Add binding for Viessmann GridBox - Initial contribution
e7923e8 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz benediktkuntz requested a review from a team as a code owner April 19, 2024 12:43
@jlaur jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Apr 19, 2024
@benediktkuntz
Only start update scheduler if initialization worked
bf18ca1 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
jlaur
jlaur reviewed Apr 20, 2024
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 your contribution!

Just one quick comment: The CI build might work better if you add your binding to this POM:
https://github.com/openhab/openhab-addons/blob/main/bom/openhab-addons/pom.xml

.gitattributes Outdated Show resolved Hide resolved
@benediktkuntz
Revert .gitattributes change
7696b83 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
Add gridbox binding to bom pom.xml
6189dbb 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
Fixed formatting in bundles pom.xml
861605c 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
Copy link
Contributor Author

Should be done now 👍 I think the build error was because I messed up the bundles/pom.xml formatting... But I added the binding to the bom pom.xml as well. Maybe this would be an improvement to the create_openhab_binding_skeleton skripts?

@benediktkuntz
Add missing newline
7ab9288 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
lsiepel
lsiepel requested changes Apr 27, 2024
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment •
edited

Choose a reason for hiding this comment

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

Nice contribution, thanks. Many comments are the same issue, so it is allready in a good shape.
You could also consider to support i18n with the updatestatus detail text. But that is up to you, updateStatus example here: https://www.openhab.org/docs/developer/utils/i18n.html#using-custom-keys

Also unit tests are highly appreciated (not mandatory). See other bindings for some examples.

bundles/org.openhab.binding.gridbox/src/main/resources/OH-INF/addon/addon.xml Show resolved Hide resolved
bundles/org.openhab.binding.gridbox/README.md Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/api/GridBoxApi.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/api/GridBoxApi.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/api/GridBoxApi.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/api/GridBoxApi.java Outdated Show resolved Hide resolved
...ding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxBindingConstants.java Outdated Show resolved Hide resolved
@lsiepel lsiepel changed the title [gridbox] Add binding for Viessmann GridBox - Initial contribution [gridbox] Initial contribution Apr 27, 2024
@benediktkuntz
[gridbox] Add connection tag to addon.xml
41df293 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] rename channel ids to lowercase-hyphen
3a64c59 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] Apply PR comments for logging etc.
bbb2fc5 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] Externalize updateStatus messages
fc2f2f3 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] apply spotless
e49e6e5 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
lsiepel
lsiepel requested changes Apr 28, 2024
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment •
edited

Choose a reason for hiding this comment

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

Some new comments, things i missed earlier. Thanks for addressing this quick. Very nice you also adapted to the i18n labels.

...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/GridBoxHandler.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/api/GridBoxApi.java Outdated Show resolved Hide resolved
...enhab.binding.gridbox/src/main/java/org/openhab/binding/gridbox/internal/api/GridBoxApi.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.gridbox/src/main/resources/OH-INF/i18n/gridbox.properties Outdated Show resolved Hide resolved
@benediktkuntz
[gridbox] Apply PR Comments
9d67927 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] Rename channel type id to hyphen
2066888 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] Fix comment formatting
8b57942 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] Add tests for GridBoxApi
7ff6c24 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] Add maximum reconnect handling
7f1678a 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
lsiepel
lsiepel approved these changes May 5, 2024
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment •
edited

Choose a reason for hiding this comment

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

Thanks again for the contribution. LGTM. as many changes have been made , let me know that it was tested enough to merge.

@benediktkuntz
[gridbox] Fix typo "@test"
2e13029 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
[gridbox] Add check to not handle an empty response
386a39c 
Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
Copy link
Contributor Author

It runs quite smoothly on my openhab instance since some days. Only thing i noticed is that the API sometimes returns an "empty" data response which results in all channels set to 0, so I added some code to ignore those responses.

@benediktkuntz
Copy link
Contributor Author

@jlaur or @lsiepel , do you know why the build failed?

@lsiepel lsiepel added the rebuild Triggers Jenkins PR build label May 19, 2024
@lsiepel lsiepel removed the rebuild Triggers Jenkins PR build label May 19, 2024
@lsiepel lsiepel merged commit 9e75a49 into openhab:main May 19, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.2 milestone May 19, 2024
@lsiepel
Copy link
Contributor

lsiepel commented May 20, 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

PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
@benediktkuntz @PRosenb
[gridbox] Initial contribution (openhab#16664)
3e80097 
* [gridbox] Add binding for Viessmann GridBox - Initial contribution

Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
@benediktkuntz @PRosenb
[gridbox] Initial contribution (openhab#16664)
c094e94 
* [gridbox] Add binding for Viessmann GridBox - Initial contribution

Signed-off-by: Benedikt Kuntz <benkuntz@web.de>
@benediktkuntz
Copy link
Contributor Author

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

Is it allowed to add a image, e.g. from the Viessmann homepage, which could be copyrighted?

@lsiepel
Copy link
Contributor

lsiepel commented May 25, 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

Is it allowed to add a image, e.g. from the Viessmann homepage, which could be copyrighted?

You can always ask them, most company's have some kind of policy on their website about the terms.

@jlaur
Copy link
Contributor

jlaur commented May 25, 2024

Is it allowed to add a image, e.g. from the Viessmann homepage, which could be copyrighted?

See #16017 (comment)

@jlaur
Copy link
Contributor

jlaur commented May 25, 2024

Is it allowed to add a image, e.g. from the Viessmann homepage, which could be copyrighted?

See #16017 (comment)

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

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

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.

None yet
3 participants
@benediktkuntz @lsiepel @jlaur