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

Improve z-wave thermostat support #27040

Merged
merged 4 commits into from Nov 26, 2019
Merged

Conversation

@oandrew
Copy link
Contributor

oandrew commented Sep 28, 2019

Discover thermostats using COMMAND_CLASS_THERMOSTAT_MODE so that it is a single entity when there are multiple setpoints.
z-wave docs mention this command class is required to be present.
Add support for single/dual target temperature depending
on the current thermostat mode.
I also verified thermostat is correctly exposed through Amazon Alexa/Google Assistant and single/dual setpoints are dynamically updating based on the current mode.

Breaking Change:

This changes the primary command class for z-wave thermostats in discovery schemas from COMMAND_CLASS_THERMOSTAT_SETPOINT to COMMAND_CLASS_THERMOSTAT_MODE.
This will cause a typical dual setpoint thermostat to be correctly represented as a single entity.

Description:

Previously z-wave thermostats were discovered using COMMAND_CLASS_THERMOSTAT_SETPOINT as a primary value.
That caused a single thermostat to be exposed multiple times - once for each setpoint.
Also this PR adds dynamic single/dual setpoint support based on the current thermostat mode.
e.g. if current mode is HVAC_MODE_HEAT or HVAC_MODE_COOL - expose single target temp ATTR_TEMPERATURE, if current mode is HVAC_MODE_HEAT_COOL - expose target range attributes ATTR_TARGET_TEMP_LOW and ATTR_TARGET_TEMP_HIGH

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
I will create PR for docs once I get feedback on this PR

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][dev-checklist]

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Discover thermostat using COMMAND_CLASS_THERMOSTAT_MODE
so that it is a single entitiy in case there are multiple
setpoints. z-wave docs mention it is always present.
Add support for single/range target temperature depending
on the current thermostat mode.
@oandrew oandrew requested a review from home-assistant/z-wave as a code owner Sep 28, 2019
@project-bot project-bot bot added this to Needs review in Dev Sep 28, 2019
@cgarwood

This comment has been minimized.

Copy link
Member

cgarwood commented Sep 30, 2019

Z-Wave side looks fine to me, but I'd like to have someone with more knowledge of the climate platforms take a look

@project-bot project-bot bot moved this from Needs review to Second opinion wanted in Dev Sep 30, 2019
@frenck

This comment has been minimized.

Copy link
Member

frenck commented Sep 30, 2019

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

support = SUPPORT_TARGET_TEMPERATURE
support = 0
current_setpoints = self._current_mode_setpoints()
if len(current_setpoints) == 1:

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 1, 2019

Member

Can current_setpoints change during entity lifetime? Supported features are not allowed to change during entity lifetime.

This comment has been minimized.

Copy link
@oandrew

oandrew Oct 2, 2019

Author Contributor

Yep. It depends on the current mode currently.
Let me check how UI/alexa/google components interact with climate component.
My main concern is exposing the correct number of setpoints based on the current mode.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2019

Member

The target temperature properties should return None if the current mode isn't using a corresponding target temperature. See smartthings climate platform eg:

@property
def supported_features(self):
"""Return the supported features."""
return self._supported_features
@property
def target_temperature(self):
"""Return the temperature we try to reach."""
if self.hvac_mode == HVAC_MODE_COOL:
return self._device.status.cooling_setpoint
if self.hvac_mode == HVAC_MODE_HEAT:
return self._device.status.heating_setpoint
return None
@property
def target_temperature_high(self):
"""Return the highbound target temperature we try to reach."""
if self.hvac_mode == HVAC_MODE_HEAT_COOL:
return self._device.status.cooling_setpoint
return None
@property
def target_temperature_low(self):
"""Return the lowbound target temperature we try to reach."""
if self.hvac_mode == HVAC_MODE_HEAT_COOL:
return self._device.status.heating_setpoint
return None

This comment has been minimized.

Copy link
@oandrew

oandrew Oct 2, 2019

Author Contributor

Makes sense. I'll update the PR. Thanks!

This comment has been minimized.

Copy link
@adrianschroeter

adrianschroeter Oct 6, 2019

Contributor

There are also thermostats (like Eurotronic Spirit-Z) which have two heating setpoints. One for normal and one for Eco mode. So this code should maybe not be limited to cooling?

This comment has been minimized.

Copy link
@oandrew

oandrew Oct 13, 2019

Author Contributor

I've updated the PR to address the comments

return
operation_mode = self._hvac_mapping.get(hvac_mode)
_LOGGER.debug("Set operation_mode to %s", operation_mode)
self.values.mode.data = operation_mode
self.values.primary.data = operation_mode

This comment has been minimized.

Copy link
@Santobert

Santobert Oct 5, 2019

Member

Please ignore that if it's a bad idea or out of scope of this PR. We can prevent the setting of a mode that is already active with something like the following function:

    def update_mode(self, operation_mode):
        """Update the operation mode only if it is not already set."""
        if self.values.primary.data != operation_mode:
            self.values.primary.data == operation_mode

I noticed that my zwave thermostats move when an active mode is activated again.

- use explicit mapping between modes and setpoints as defined in
Z-Wave specs
- add tests for away (2 setpoints) and heat eco (1 setpoint) modes
"dry air": ("setpoint_dry_air",),
"moist air": ("setpoint_moist_air",),
"auto changeover": ("setpoint_auto_changeover",),
"heat econ": ("setpoint_eco_heating",),

This comment has been minimized.

Copy link
@Santobert

Santobert Oct 12, 2019

Member

The eco mode is called "Heat Eco" in case of Eurotronic Spirit-Z thermostats. I'm not sure if this makes a difference.

This comment has been minimized.

Copy link
@oandrew

oandrew Oct 12, 2019

Author Contributor

Thanks for noticing that.
The labels I used are from https://github.com/OpenZWave/open-zwave/blob/85ca5769a57d13f328e65b415d95e95647994366/cpp/src/command_classes/ThermostatMode.cpp#L56-L94 .
But it turns out open-zwave let's you override the labels in xml as well.
In your case here: https://github.com/OpenZWave/open-zwave/blob/85ca5769a57d13f328e65b415d95e95647994366/config/eurotronic/eur_spiritz.xml#L71
So indexes are the same but labels are different :(
I just ran grep across all XML configs and found various confusing labels for same indexes e.g.
Energy Heat = Energy Saving = Heat Eco = Heat Econ and so on.

I will add the aliases I've found.
Sometime later it makes sense to switch to indexes instead of aliases since they seem to be more stable

@oandrew

This comment has been minimized.

Copy link
Contributor Author

oandrew commented Oct 16, 2019

Could someone review this when you have a chance? Thanks

@MartinHjelmare MartinHjelmare moved this from Second opinion wanted to Review in progress in Dev Oct 27, 2019
@Santobert

This comment has been minimized.

Copy link
Member

Santobert commented Nov 3, 2019

It'd be great if we could continue here. I'm really looking forward to seeing these changes in the next release.
@oandrew Could you please improve the code coverage by writing some more tests. Besides some single lines only the aux_heat methods are untested.

Dev automation moved this from Review in progress to Reviewer approved Nov 26, 2019
Copy link
Member

balloob left a comment

This looks great! Thanks

@balloob balloob merged commit f5c01cc into home-assistant:dev Nov 26, 2019
10 of 11 checks passed
10 of 11 checks passed
codecov/patch 93.75% of diff hit (target 94.3%)
Details
CI Build #20191012.70 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/project 94.35% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Nov 26, 2019
@lock lock bot locked and limited conversation to collaborators Nov 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.