Fahrenheit Conversion Inconsistencies #73
Conversation
The current limits of the temperature table are not sufficient. Given that the temperature sensor of the AC unit is reading higher than 30C, or 86F, the temperature value will not be reported correctly. Simply extending this range solves this problem. This is specifically to address the temperature sensor in the unit, not its climate-addressable range.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #73 +/- ##
==========================================
- Coverage 92.18% 91.98% -0.21%
==========================================
Files 5 5
Lines 678 686 +8
==========================================
+ Hits 625 631 +6
- Misses 53 55 +2 ☔ View full report in Codecov by Sentry. |
greeclimate/device.py
Outdated
| @@ -105,7 +105,11 @@ def generate_temperature_record(temp_f): | |||
| TEMP_OFFSET = 40 | |||
| TEMP_MIN_F = 46 | |||
| TEMP_MAX_F = 86 | |||
| TEMP_TABLE = [generate_temperature_record(x) for x in range(TEMP_MIN_F, TEMP_MAX_F + 1)] | |||
| TEMP_MIN_TABLE = 8 | |||
There was a problem hiding this comment.
Since the goal here is to allow the device to report temperatures outside of the settable range, MIN should also be lowered.... maybe -50, then max to 60... just to give some headroom of conceivable reasonable values.
There was a problem hiding this comment.
I understand your point, but if the unit inside a room is reading less than 8C, or 46F, there's a bigger issue at hand than the temperature it's reading (that is really cold, haha). Fair point, should be fine to adjust limits.
Edit: Didn't realize you suggested a limit, I'll make changes for that.
There was a problem hiding this comment.
Extended the minimum to -60 C (-76 F), and maximum to 60 C (140 F).
| try: | ||
| f = next(t for t in matching_temSet if t["temRec"] == bit) | ||
| except StopIteration: | ||
| f = matching_temSet[0] |
There was a problem hiding this comment.
Please add test cases covering these lines.
There was a problem hiding this comment.
I haven't gotten around to that, I'll take a look at this shortly.
There was a problem hiding this comment.
Also, do you have any clue why some units have discrepancies between the temSets and temRecs? Since this solution has worked fine for me on HomeAssistant, it isn't very important to actually implement, but addressing this problem would fix the root of this hacky solution.
There was a problem hiding this comment.
No, not sure. This whole approach to supporting F feels like Gree added a feature that while trying not to break C support on existing hardware. It's seems likely that the conversion is not computed on need, I could never find a functional formula that just worked and so had to pre-compute the table for the lookup to work at all.
Have to remove some of the now valid ranges :) |
|
Wasn't sure if I should have touched those test cases - Thanks. |
No worries, looks good. I'll merge soon. Thanks for the updates. |
## [1.4.2](v1.4.1...v1.4.2) (2024-06-26) ### Bug Fixes * Fahrenheit conversion inconsistencies ([#73](#73)) ([f09f3f1](f09f3f1))
|
🎉 This PR is included in version 1.4.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
I understand that this is a hacky way of resolving the StopIteration problems, but this resolves the issue I have in #72. Unless someone can dedicate significant time into determining how the conversion process differs, this is enough to allow HomeAssistant to function as expected with the Gree units that I have, and this PR should not cause any issues for those who don't have these problems.