Add support for GPCV and GECO series heaters. #28
Conversation
make-all
changed the title
|
Just letting you know I haven't forgotten about this. I'll be in to review it soon. Meanwhile I've made a few more tweaks and also added Dev Containers so you can easily set up a local environment for testing. If you could rebase to fix the conflicts I'll do a review and we can look at getting it merged. |
These are different than the GPPH series heaters that are already supported. See Issue #27 Based on changes in KiLLeRRat/homeassistant-goldair-climate and the manual for GECO/GPEH heaters, the DPS arrangement for GECO seems the same as GPCV, but this model does not have the High and Low modes which we exposed as PRESET_MODE in the GPCV so may generate errors if configured as GPCV. This heater appears cosmetically and functionally the same as GEPH, so hopefully it works with these as well.
There was a problem hiding this comment.
Thanks for the awesome contribution! I only have one tweak that I'd like to discuss.
With regards to the potential "smell" of duplicating all of this code (as discussed in #27), I'm of the opinion that it's OK (and even beneficial) to duplicate code in the short term, as long as we are using the same pattern. This prevents premature abstraction and allows you to see the full picture of shared functionality, with the short-term maintenance pain helping guide the refactor of parts that can be shared.
So now that we have three different types of heater, I can see a good path forward to reducing duplicated code and I'll start working on a refactor that will also benefit the other device types at the same time. But first, let's get support for these heaters merged.
Also don't worry about the failing hacs check, that's unreleated to this repository and I'm tracking it in #33.
| else: | ||
| return CONF_TYPE_GECO_HEATER | ||
| else: | ||
| return CONF_TYPE_DEHUMIDIFIER |
There was a problem hiding this comment.
This is a logical ay to extend it, but I feel like from an understandability point of view this is a bit confusing - we have heaters and the dehumidifier in the same statement. I wonder if it could be more like this:
if "5" in cached_state and "3" in cached_state:
return CONF_TYPE_GPCV_HEATER if "7" in cached_state else CONF_TYPE_GECO_HEATER
if "5" in cached_state and "3" not in cached_state:
return CONF_TYPE_DEHUMIDIFIER
if "8" in cached_state:
# etcAvoids nesting and intermixing heaters with dehumidifier in the logic. List was already order dependent, so have not included previous conditions, to simplify the logic.
These are different than the GPPH series heaters that are already supported.
See Issue #27