[gree] Add channel for temperature sensor

[taboneclayton]

New Feature/Improvement

Added a new channel called "currentTemperature" which maps to the temperature sensor which is present on GREE AC devices. This should allow for the addition of rules which are based on the actual room temperature, Eg: switch on AC in heating/cooling mode if room temperature is higher/lower than X. This temperature sensing channel can also be used to control other features like ventilation, etc by comparing the indoor and outdoor temperature. This usually requires a dedicated temperature sensor for reading the indoor temperature and such temperature is readily available in GREE AC devices and readable over this binding with the help of this PR.

### Breaking Change!
This binding which was only recently introduced already contained a temperature channel called "temperature". This was meant to be able to set the desired/target temperature on the AC device. In order to avoid any confusion between the desired/target and actual/current temperature channels, the old existing channel has been renamed from "temperature" to "targetTemperature". The temperature channel name has been reverted to the original naming so no breaking changes.

Additional Notes:

This is my first contribution to openHAB so please make sure that everything is in order. I am not 100% sure if I messed up any character encoding in the il8n strings. The character encoding has been modified from UTF8 to ISO 8859-1. Also, my proficiency in the German language is very limited so certain translation strings might require some improvements.
Static code analysis passes successfully and the changes have been tested locally on openHAB versions 2.5.7 and 2.5.8 running on Rasbian Buster on a Raspberry Pi 3.

[taboneclayton]

Attaching sources and binary JARs as per PR guidelines hoping that these might help anyone reviewing this PR. Thanks :)
plugin-jar.zip

[TravisBuddy]

Travis tests were successful

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

[fwolter]

Is it worth the breaking change? We are going from 2.5.7 to 2.5.8. The user doesn't expect a breaking change. Why not keep the Channel name the same and clarify it in the label/description?

[taboneclayton]

Is it worth the breaking change? We are going from 2.5.7 to 2.5.8. The user doesn't expect a breaking change. Why not keep the Channel name the same and clarify it in the label/description?

I originally retained the original channel name in order to minimize the changes required and remain backwards compatible.

Below are the reasons for which I ended up reconsidering to rename the existing temperature channel to targetTemperature:

1. the temperature channel name was too generic. Whilst the label/description could further clarify the meaning of this channel, any references from rules would only contain the channel name and might become confusing and result in unwanted consequences if the temperature channel gets used by mistake instead of the currentTemperature

2. Any examples/documentation for openHAB with similar use-cases seemed to suggest that a device which can behave like a thermostat would have a currentTemperature and a targetTemperature for example here or temperatureAmbient and temperatureSetpoint for example here. So I was trying to make this binding consistent with this concept.

3. the GREE binding was only recently introduced in version 2.5.7 so I thought that there might not have been a huge uptake in usage till now and that maybe it was worth making the channel names clearer before the binding gets used by a lot more people. Having said that I understand that there must have been a lot of people who were using this plugin before as the old unofficial greeair plugin and were only too happy (like I was) to migrate to the official GREE binding once this was released with openHAB 2.5.7. I understand that it might not be cool to ask these people to go through their item mappings or rules to update to the new name.

Despite all of the above I understand that 2.5.8 is a patch version bump and therefore should contain no breaking changes. I have a local commit which reverts to the temperature channel name and can push this if it is preferred. Just waiting for a confirmation first and then I'll push the temperature channel name revert.

[markus7017]

I appreciate your contribution, it was also on my list.
And yes, please revert the name change. There is always a reason why to change names etc.m but at the end there is also a README and a channel description.

[TravisBuddy]

Travis tests were successful

Hey @taboneclayton,
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 @taboneclayton,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[taboneclayton]

Thanks for reviewing and for your comments @fwolter , @markus7017. Agree with both your comments and don't see the need to break backwards compatibility. The name of the temperature channel has been reverted to the original name temperature.

[markus7017]

@fwolter how do we proceed?

[fwolter]

If you approve the change, I'd do so, too.

[TravisBuddy]

Travis tests were successful

Hey @taboneclayton,
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 @taboneclayton,
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 @taboneclayton,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[taboneclayton]

Thanks for your feedback and comments. This should now be ready for another round of reviews. From my end I just need an answer on the two questions which I presented here and here

[taboneclayton]

Also updated PR description to match the most current list of changes.

[markus7017]

@taboneclayton Please sign you code updates
add a line like

description

Signed-off-by: Markus Michels <markus7017@gmail.com> (github: markus7017) 

as comment to the push

[fwolter]

I don't think it's intuitive if configuring an offset of 0, would result in an offset of +40°C. Can you subtract the internal offset in the code, so an offset of 0 is really 0?

[Hilbrand]

I think the idea here is that the device returns the data with a 40 degree offset. But that value 40 is not guaranteed. So therefor this parameter is there correct that. If it would always be 40 this offset would not be needed or maybe could be a boolean. To switch between 40 and 0 in case there is no offset. So it very much depends if the 40 is fixed or not.

[taboneclayton]

Yes this was the exact reasoning behind it. On the Gree devices which I have installed at home, the offset is that of +40°C. So the offset required for calibration for this particular device is actually -40 and not 0.

Judging from online research this seems to be the case with the majority of devices but I cannot vouch for that since new models might behave differently.

The idea for this configuration parameter was that other devices might come with an offset which is different from +40°C. So the idea was that the binding would offset exactly by the amount which is specified in this parameter and not make any assumptions other than setting the default at -40 which seems to be the most common at the time.

If for some other Gree device, the offset is +30°C, the required value here would be -30 and not +10.

[fwolter]

@markus7017 you as the codeowner, WDYT? Are you aware of any device type that has a different offset than -40°C?

[markus7017]

Hmm, as you said it seems that all devices have an offset of 40°C. Having this offset configurable would allow the user to do adjustment when the temp is really 1°C off etc., which is a useful feature.
We have no proof that there are other offsets than 40°. Therefore I would make this assumption and do the math in the binding. In case a device has something != 40, this is adjustable as a work around. We could then decide how to model this (e.g,. by a different thing type), but so far I don't see this scenario.

[markus7017]

I favor

• keeping the parameter, but as normalized offset
• hard code the -40 as constant in the code

So 0 means 0 for the user, but -40 for the binding, 1=-39, -2=-42 etc.

@taboneclayton it might me that 0.5 steps are supported

[markus7017]

@taboneclayton From my point of view this is the only open issue. Could you change it that way?

[taboneclayton]

Updated as suggested. I think this should address all requests for changes. However there is a merge conflict in thing-types.xml due to the introduction of spotless. Don't want to merge from upstream to avoid problems. Should I rebase to resolve the conflict?

[Hilbrand]

Yes do rebase and git push --force-with-lease back to GitHub.

Also do run mvn spotless:apply on your binding to make sure your changes also comply with the style, otherwise it might break the build of your binding.

[taboneclayton]

Done

[TravisBuddy]

Travis tests were successful

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

[fwolter]

LGTM

It might be sufficient to only run mvn spotless:apply.

[markus7017]

looks good for me

[markus7017]

@taboneclayton Good job! :-)
Please join us here: https://community.openhab.org/t/new-gree-air-conditioner-binding/36429/358

[markus7017]

@taboneclayton I tried the version with the merged PR. currentTemperatur shows -40°C for my unit.
I opened this issue: #8452