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

[Broadlinkthermostat] Initial contribution #9260

Merged
merged 18 commits into from Feb 8, 2021
Merged

[Broadlinkthermostat] Initial contribution #9260

merged 18 commits into from Feb 8, 2021

Conversation

flo-02-mu
Copy link
Contributor

This is a new binding to support thermostats that are based on a broadlink chips and sold with different brand names (see https://community.openhab.org/t/electronic-heating-thermostat-reverse-engineering-beok-floureon-decdeal/39251).

While there is already another broadlink binding in development (https://community.openhab.org/t/broadlink-binding-for-rmx-a1-spx-and-mp-any-interest/22768) they both focus on different things, that's why I decided to create a separate binding.

Unfortunately the binding includes a library in the lib folder which is not an osgi bundle. The distributed version is a jdk11 patched version of https://github.com/mob41/broadlink-java-api/

Myself and other users have been using this binding for quite a while, that's why I'd like to get this into the official distribution.
Discussion for this binding takes place at https://community.openhab.org/t/broadlink-floureon-hysen-thermostat-binding/81921

@flo-02-mu flo-02-mu requested a review from a team as a code owner December 6, 2020 16:24
@fwolter fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Dec 6, 2020
@fwolter fwolter self-assigned this Dec 13, 2020
Copy link
Member

@fwolter fwolter 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! I reviewed your code and here is my feedback.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

@flo-02-mu
Copy link
Contributor Author

Thanks for your contribution! I reviewed your code and here is my feedback.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

Thank You for your fast feedback. I incorporated the quick wins and will take care about the dateformatter in the next days.
The only issue that remains is about the external library: It does exist on maven central, but with a non-java-11 compatible version, and there is not much activity on the repo. But duplicating the functionality of that library would be pretty sad, too.

@fwolter
Copy link
Member

fwolter commented Dec 15, 2020

You're welcome. Shouldn't run any much older JAR version with the current Java version?

@flo-02-mu
Copy link
Contributor Author

You're welcome. Shouldn't run any much older JAR version with the current Java version?

You're completely right, I was mistaken on that. I thought it would be missing the jaxb lib, but it seems to not miss that one "transitively". Also the handling in the feature.xml file seems to be not needed anymore... so the lib is not bundled anymore, but just referenced as compile dependency in the pom.
All other changes are done, let me know if there is anything else that I should change.

@fwolter
Copy link
Member

fwolter commented Dec 18, 2020

That the transitive dependency isn't missing is a coincidence as it is propably used somewhere else in OH. The transitive dependencies need to be added to the pom.xml.

@flo-02-mu
Copy link
Contributor Author

That the transitive dependency isn't missing is a coincidence as it is propably used somewhere else in OH. The transitive dependencies need to be added to the pom.xml.
I added the jaxb dependency via <feature dependency="true">openhab.tp-jackson</feature> in feature.xml

@fwolter
Copy link
Member

fwolter commented Dec 28, 2020

Just to avoid confusion, did you click on "Load more ..." in my review comments? Because you fixed all visible but none of the hidden comments.

@flo-02-mu
Copy link
Contributor Author

Just to avoid confusion, did you click on "Load more ..." in my review comments? Because you fixed all visible but none of the hidden comments.

🤦 indeed, I missed them in the first place. They are now processed.

flo-02-mu and others added 12 commits December 29, 2020 23:42
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
…org/openhab/binding/broadlinkthermostat/internal/discovery/BroadlinkThermostatDiscoveryService.java

Accept review suggestion.
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>

Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
…org/openhab/binding/broadlinkthermostat/internal/handler/FloureonThermostatHandler.java

Accept review suggestion.
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>

Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
…org/openhab/binding/broadlinkthermostat/internal/BroadlinkThermostatHandlerFactory.java

Accept review suggestion.
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>

Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Simplify timestamp method
Format fixes

Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

There is one checkstyle warning left. You could take a look at target/code-analysis/report.html.

There are some compiler warnings about null access left. To fix these, you need to store the field to a local variable and do the null check on that.

Fix regex
Use QuantityType for temperatures.

Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
@flo-02-mu
Copy link
Contributor Author

There is one checkstyle warning left. You could take a look at target/code-analysis/report.html.

There are some compiler warnings about null access left. To fix these, you need to store the field to a local variable and do the null check on that.

Cool, I was always wondering how to deal with the null checks -> Should all be solved. I did not have a checkstyle warning - is it still present?

Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
@fwolter
Copy link
Member

fwolter commented Jan 9, 2021

Did you finished making all changes?

@flo-02-mu
Copy link
Contributor Author

Did you finished making all changes?

As far as I can see, yes, everything is pushed.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

As this is a new binding, another maintainer needs to review your code before it can be merged.

@fwolter fwolter added the cre Coordinated Review Effort label Jan 13, 2021
@scrakcho
Copy link

Sorry but how time ussualy take to merge an addon?

@fwolter
Copy link
Member

fwolter commented Jan 25, 2021

It depends on when another maintainer finds time to do the second review. But the forum link might bring you to a JAR file, so that you can install the add-on in advance.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Here is the second review. One additional comment. You need to update the copyright year. You can do this by running mvn license:format on your binding.

@Hilbrand Hilbrand self-assigned this Feb 1, 2021
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
* introduce cache for AdvanceStatusInfo
* Move authentication outside of initialize step

Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM

@Hilbrand Hilbrand merged commit a58676d into openhab:main Feb 8, 2021
@Hilbrand Hilbrand added this to the 3.1 milestone Feb 8, 2021
@Hilbrand Hilbrand removed the cre Coordinated Review Effort label Feb 8, 2021
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: Florian Mueller <f.l.o.mueller@web.de>
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

4 participants