[gree] Initial contribution

[markus7017]

This binding integrates GREE air conditioners using the WiFi remote control interface. Depending on the model it provides are functions of the regular remote (air control, temp, power, swing etc.)

I took over this binding from John Cunha, who stopped supporting the binding. Code has been re-factored, discovery and localization added. README is also updated.

Community discussions: https://community.openhab.org/t/new-gree-air-conditioner-binding/36429/123?u=markus7017

[wborn]

You can make Travis CI happier by adding <module>org.openhab.binding.gree</module> to:

openhab-addons/bundles/pom.xml

Lines 101 to 102 in 15f2e78

	<module>org.openhab.binding.gpstracker</module>
	<module>org.openhab.binding.groheondus</module>

🙂

[markus7017]

done :-)

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[markus7017]

@alexander-po any other comments?

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[markus7017]

@alexander-po any other comments?

[alexander-po]

@alexander-po any other comments?

For me looks good

[markus7017]

@cpmeister Who could perform the review?

[markus7017]

@openhab/maintainers How could do the review?
I expect this to be straight forward. I already re-facorted the code to match the coding style.
Function has been verified by various users. @alexander-po already did a first review

[fwolter]

Sorry for the many findings, but most of them are only minor improvements. I known the code base is not yours, but this is the chance to get a clean code when migrating to OH2.

There are several locations where you use complex types, where int would be sufficient. I didn't mark all of them.

There are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

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

[markus7017]

@fwolter No worries, a challenge for both of us :-)

The code was created by a different developer. I'm going to install a A/C and checked what options are supported by OH (one of my primary decision criteria, of course :-)). I found this binding, which was outdated, but community members were responding so I decided take over the code base, did a lot of re-factoring and quickly we have an up-to-date implementation, which was also tested with various models.

I already did a lot of re-factoring to get rid of many things and make the code more efficient. Nevertheless you touch the primitives topic. I was already on the way to replace all of the Integer's, but had to learn that at some points the author uses the fact that they can be null. I didn't developed the code and without a unit to test with I decided to revert the changes and keep it for now as is. This could lead into many tiny issues impacting functionality and stability. Again I'll check and try to understand how that can be changed, but...

[fwolter]

I was already on the way to replace all of the Integer's, but had to learn that at some points the author uses the fact that they can be null.

You could replace all complex Integers with primitive ones that don't lead to compiler errors. And leave those, which can be null, untouched. If there's a reason for using complex types, you can simply stick to it.

[markus7017]

@fwolter Done with most of the changes
please reply to the open topics

[TravisBuddy]

Travis tests have failed

Hey @markus7017,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[cpmeister]

These are the final-final changes. The code looks good so I'm just double checking all the other stuff.
I'm just making sure I'm not missing anything since with @fwolter becoming a maintainer I've become the second maintainer to review this binding, thus making me responsible for merging it as well.

Sorry for all this back and forth, I can only review so much during a single sitting. Thanks for your patience.

[cpmeister]

Jenkins reported a build failure:

[INFO] There is 1 error reported by Checkstyle 8.12 with jar:file:/home/jenkins/jenkins-agent1/maven-repositories/1/org/openhab/tools/sat/sat-plugin/0.9.0/sat-plugin-0.9.0.jar!/rulesets/checkstyle/rules.xml ruleset.

Any ideas?

[markus7017]

Jenkins reported a build failure:

[INFO] There is 1 error reported by Checkstyle 8.12 with jar:file:/home/jenkins/jenkins-agent1/maven-repositories/1/org/openhab/tools/sat/sat-plugin/0.9.0/sat-plugin-0.9.0.jar!/rulesets/checkstyle/rules.xml ruleset.

Any ideas?

no, is there any kind of log output to identify the error?
Why does the Jenkins status doesn't show the GREE binding? problem with bundles/pom.xml?

[markus7017]

Sorry for all this back and forth, I can only review so much during a single sitting. Thanks for your patience.

@cpmeister No problem, we all have and 2nd job and need to manage our time. I'm fine if we are making progress and once we are done with this one we could go back to MagentaTV :-)

[TravisBuddy]

Travis tests were successful

Hey @markus7017,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[markus7017]

@cpmeister anything open before you could proceed with the merge?

[markus7017]

@cpmeister Hi, your "final review changes" were requested almost 2 weeks ago. Is there anything open? I want to complete this work here and start over with the next PR. @fwolter Maybe you could complete the review?

[fwolter]

You can start making changes by creating a branch based on this PR's branch gree_snapshot, if you haven't already. When this PR is merged you could immediately file a new PR with your new changes done. There need to be approvals by two reviewers, that's why I can't finish your PR.

[cpmeister]

Sorry about the delay, I took some time off.
LGTM now.

[cpmeister]

Manually checked DTO

[markus7017]

@cpmeister @fwolter thanks for your support

@cpmeister could you please check the status of the MagentaTV bindjng, we are also almost done there