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

[nobohub] Initial contribution #12937

Merged
merged 12 commits into from
Aug 22, 2022
Merged

[nobohub] Initial contribution #12937

merged 12 commits into from
Aug 22, 2022

Conversation

espenaf
Copy link
Contributor

@espenaf espenaf commented Jun 14, 2022

[nobohub] Initial contribution

This binding adds support for the Glen Dimplex Nobø Hub (also know as Nobø ecoHub). Nobø Hub supports a range of electrical panel heaters, thermostats and switches from Glen Dimplex and Nobø.

README.md has information about configuration and usage.

The code should both be using the code guidelines and have run spotless on it.

Pre-built jar
Nobø Hub API Documentation 1.1
Community Discussion @ community.openhab.org
Community Discussion @ hjemmeautomasjon.no
Supported Devices

@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/nobo-hub-binding/136653/1

@jlaur jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Jun 14, 2022
Signed-off-by: Espen Fossen <espenaf@junta.no>
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! And sorry for the delay. I've added some feedback for you. There might be some inconsistency as I started the review some time ago and picked it up again today. Please also run:

mvn org.openhab.core.tools:i18n-maven-plugin:3.2.0:generate-default-translations

to generate default translations.

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.

Some minor comments. Posted from my phone - I'll be fully back sometime next week. :)

@espenaf espenaf force-pushed the nobohub branch 3 times, most recently from 7abaf37 to 0aeda8a Compare July 17, 2022 08:38
* Added English and Norwegian translation for error messages
* Added support for QuantityType for temperatures
* Added consistent usage of property constants
* Changed most logging to DEBUG or TRACE to avoid spamming logs
* Changed Override to OverridePlan to avoid import clash with java.lang.Import
* Lots of other small changes

Signed-off-by: Espen Fossen <espenaf@junta.no>
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.

I hope you don't mind incremental review? It allows me to push some comments sooner.

@espenaf
Copy link
Contributor Author

espenaf commented Jul 18, 2022

I hope you don't mind incremental review? It allows me to push some comments sooner.

No problem. I love this attention to detail, and I am learning a few tricks as we iterate on the 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.

Commenting on null annotation issue as the castToNonNull pattern is used in many places and could be avoided.

@jlaur
Copy link
Contributor

jlaur commented Jul 19, 2022

I hope you don't mind incremental review? It allows me to push some comments sooner.

No problem. I love this attention to detail, and I am learning a few tricks as we iterate on the changes.

Glad to hear. Please feel free to resolve all comments which you have provided fixes for, so we can easily track any pending issues/discussions. I will make sure to verify and reopen if needed.

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

austvik commented Aug 17, 2022

@jlaur: I believe Espen has addressed all comments. What are the next steps here?

@espenaf
Copy link
Contributor Author

espenaf commented Aug 17, 2022

@austvik: From my perspective there are two comments I have not addressed:

  1. Remove use of Helpers.castToNonNull().
  2. Introducing a retry mechanism in NoboHubBridgeHandler,initialize, like the use of a ScheduledFuture polling job.

I already have code for issue 1, this is running locally, but I wanted to test it a bit before pushing. Can do that shortly to get some feedback. I have not done any changes for issue 2 yet, but hopefully this might actually improve some of the disconnects it get once or twice a month.

@austvik
Copy link
Contributor

austvik commented Aug 17, 2022

Thanks @espenaf! Looked to me like everything had been closed for a good while. Just me itching after using it.

Retries in NoboHubBridgeHandler.initialize: Do you mean like running the scheduler that creates the connection and thread with a rate? Any stack traces or logs?

I tried to hide networking issues and reconnections in the HubCommunicationThread.

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

espenaf commented Aug 17, 2022

(I have tried to use the marketplace, but get "Thing factory (class org.openhab.binding.nobohub.internal.NoboHubHandlerFactory) returned null on create thing when it reports to support the thing type (nobohub:nobohub)." I'm running 3.3.0 and was thinking the only way this could fail was if it was using different ThingTypeUID versions.)

Strange, just tested with installation of the last version from marketplace, and no similar issue.

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.

After recent activity I went through the changed files again, and added some additional findings. It looks good, and we should be ready to merge soon. :)

Signed-off-by: Espen Fossen <espenaf@junta.no>
Signed-off-by: Espen Fossen <espenaf@junta.no>
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.

Just a few comments after looking through latest iteration of changed files. Some previously unnoticed, sorry for that.

Signed-off-by: Espen Fossen <espenaf@junta.no>
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.

Quick feedback on new code.

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

jlaur commented Aug 21, 2022

@espenaf - commit c48a597 recently introduced some status update changes for which I posted some comments. Do you need more time for testing these late changes, or are you ready for merging when the last comments regarding this are resolved?

@espenaf
Copy link
Contributor Author

espenaf commented Aug 21, 2022

@espenaf - commit c48a597 recently introduced some status update changes for which I posted some comments. Do you need more time for testing these late changes, or are you ready for merging when the last comments regarding this are resolved?

Me and @austvik have done some more testing for the last couple of days so I think we are getting there pretty soon. Take a look at the last commit, and see if there are anything more to discuss.

Signed-off-by: Espen Fossen <espenaf@junta.no>
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 again for your contribution!

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

@jlaur jlaur merged commit bc9cf8e into openhab:main Aug 22, 2022
@jlaur jlaur added this to the 3.4 milestone Aug 22, 2022
leifbladt pushed a commit to leifbladt/openhab-addons that referenced this pull request Oct 15, 2022
* Added NoboHub binding.

Signed-off-by: Espen Fossen <espenaf@junta.no>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* Added NoboHub binding.

Signed-off-by: Espen Fossen <espenaf@junta.no>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* Added NoboHub binding.

Signed-off-by: Espen Fossen <espenaf@junta.no>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
marcelGoerentz pushed a commit to marcelGoerentz/openhab-addons that referenced this pull request Nov 14, 2022
* Added NoboHub binding.

Signed-off-by: Espen Fossen <espenaf@junta.no>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* Added NoboHub binding.

Signed-off-by: Espen Fossen <espenaf@junta.no>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* Added NoboHub binding.

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
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.

5 participants