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

Fix eco preset for Wink Air Conditioner #25763

Merged
merged 1 commit into from Aug 10, 2019

Conversation

@cameronrmorris
Copy link
Contributor

commented Aug 7, 2019

Description:

  • Add preset support for device
  • Provide mappings between preset changes

Wink App:

image

After changes

HA card:

image

HA card details:

image

Related issue (if applicable): fixes #25762

Pull request with documentation for home-assistant.io (if applicable):

Example entry for configuration.yaml (if applicable):

wink:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@project-bot project-bot bot added this to Needs review in Dev Aug 7, 2019

@cameronrmorris cameronrmorris force-pushed the cameronrmorris:winkacecomode branch from f89b3b5 to 7c0ea10 Aug 8, 2019

@Santobert

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Since I am not a maintainer of this project, I can only give some hints:

  • As already mentioned here HVAC_MODE_AUTO is reserved for learned behavior, AI or any other related mechanism. I would suggest using HVAC_MODE_HEAT_COOL instead of HVAC_MODE_AUTO if no AI is included.
  • I don't know how Wink handles eco mode. If eco is another mode besides auto, I would suggest to make it a preset. If eco is the only mode that controls the temperature based on user input, it might be okay to map winks eco to HA's HVAC_MODE_HEAT_COOL.

As I said, I am not a maintainer, but I had the same issues with the integration of the zwave climate.

As mentioned in the documentation, they have split operation_mode into hvac_mode and preset_mode. Hvac_modes are a small subset of operation_modes such as heat, cool and heat/cool. All other modes (boost, eco, away, etc.) are now preset_modes.
https://www.home-assistant.io/components/climate/

@cameronrmorris

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Thanks for that!

I believe I now understand the expected integration. The Wink AC "auto_eco" state should be only reachable if HA is in a "HVAC_MODE_COOL" and "PRESET_ECO" state.

The user would do that by setting 'PRESET_ECO'.

Internally, the operation mode(HVAC_MODE_COOL) and preset(PRESET_ECO) and the wink api would receive a mode of "auto_eco".

So effectively, we get a state transition like this(apologies it's verbose):
image

@Santobert

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Looks good. But some transactions are not possible because you won't get a signal when the user selects sth. that is already selected. So when auto_eco is active, it is not possible to select HVAC_MODE_COOL, which is already active, to go to cool_only. PRESET_NONE is still possible in that situation.

homeassistant/components/wink/climate.py Outdated Show resolved Hide resolved

Dev automation moved this from Needs review to Review in progress Aug 9, 2019

Fix eco preset for Wink Air Conditioner
* Add preset support for device
* Provide mappings between preset changes

@cameronrmorris cameronrmorris force-pushed the cameronrmorris:winkacecomode branch from 7c0ea10 to 4bac457 Aug 9, 2019

@cameronrmorris cameronrmorris changed the title Fix 'eco' mode for Wink Air Conditioner Fix eco preset for Wink Air Conditioner Aug 9, 2019

@@ -436,7 +440,7 @@ def hvac_mode(self) -> str:

wink_mode = self.wink.current_mode()
if wink_mode == "auto_eco":
return HVAC_MODE_AUTO
return HVAC_MODE_COOL

This comment has been minimized.

Copy link
@balloob

balloob Aug 9, 2019

Member

I think that we should keep returning AUTO here. We could optionally check if AUTO is supported first (it's part of hvac_modes)

This comment has been minimized.

Copy link
@cameronrmorris

cameronrmorris Aug 10, 2019

Author Contributor

The idea here was to have the "auto_eco" wink mode displayed as "Cool - Eco". If this returns AUTO it will be displayed as "Auto - Eco". Which style is preferred?

I think the AUTO(in hvac_modes) state is causing confusion because the state map is currently shared with the Wink Thermostat which has a real auto mode(e.g. automatic temperate changes). Maybe it's better to separate the state maps for both the Thermostat and Air conditioner for clarity?

This comment has been minimized.

Copy link
@balloob

balloob Aug 10, 2019

Member

Oh, this is my bad. I did not realize that this was changing code inside the AC entity. This is ok .

@balloob balloob added this to the 0.97.2 milestone Aug 10, 2019

Dev automation moved this from Review in progress to Reviewer approved Aug 10, 2019

@balloob balloob merged commit f30e54f into home-assistant:dev Aug 10, 2019

11 checks passed

CI Build #20190809.59 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 60dfa38...4bac457
Details
codecov/project 94.01% (target 90%)
Details

Dev automation moved this from Reviewer approved to Done Aug 10, 2019

@lock lock bot locked and limited conversation to collaborators Aug 11, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
4 participants
You can’t perform that action at this time.