home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
74.07k stars 31.09k forks source link

Missing operating state for hvac via vera zwave #29220

Closed skarfacegc closed 3 years ago

skarfacegc commented 5 years ago

Home Assistant release with the issue:

0.96.5

Last working Home Assistant release (if known): NA

Operating environment (Hass.io/Docker/Windows/etc.): PyEnv on Linux VM

Description of problem: HVAC state isn't getting pulled from Vera zwave climate device. It's visible via the API but I'm not seeing it as an attribute of the climate entity

<state id="3425" service="urn:micasaverde-com:serviceId:HVAC_OperatingState1" variable="ModeState" value="PendingHeat"/>
<state id="3426" service="urn:micasaverde-com:serviceId:HVAC_OperatingState1" variable="ModeStateForEnergy" value="1"/>

Current attribute list for thermostat in question

fan_mode: auto
max_temp: 95
temperature: 68
fan_modes: on,auto
current_temperature: 69
supported_features: 9
friendly_name: Upstairs_Thermostat
min_temp: 45
hvac_modes: cool,heat,heat_cool,off
Vera Device Id: 18
battery_level: 93

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

vera:
  vera_controller_url: !secret vera_url
  lights: [14, 15, 16, 17]
probot-home-assistant[bot] commented 5 years ago

Hey there @home-assistant/z-wave, mind taking a look at this issue as its been labeled with a integration (zwave) you are listed as a codeowner for? Thanks!

springstan commented 5 years ago
<state id="3425" service="urn:micasaverde-com:serviceId:HVAC_OperatingState1" variable="ModeState" value="PendingHeat"/>
<state id="3426" service="urn:micasaverde-com:serviceId:HVAC_OperatingState1" variable="ModeStateForEnergy" value="1"/>

@skarfacegc are you using the PyVera library to get this information or are you using a different one?

springstan commented 5 years ago

Just as a note, we are calling get_hvac_mode from PyVera to get the current mode of a thermostat. Its implementation looks like this:

    def get_hvac_mode(self, refresh=False):
        """Get the hvac mode"""
        if refresh:
            self.refresh()
        return self.get_value("mode")

However, looking at your API output for your device it seems that the value for the key variable is not mode. Therefore, we are searching for mode cannot find it and because of that return hvac mode as off.

@skarfacegc btw hvac_mode is not an attribute of a climate entity, it is the state of the entity. The state of your device is off correct?

skarfacegc commented 4 years ago

I think I just hit the API endpoint and looked through the XML.

State is always Heat (or Cool) regardless of thermostat run mode. Should I be opening this against pyvera?

springstan commented 4 years ago

Could you take another look at the API endpoint and look for anything that includes mode? Trying to figure out why its state is always heat or cool regardless what mode it is actually on.

Yeah I would open an issue on PyVera as well, looks like your device is not properly implemented.

awooganl commented 4 years ago

The vera component is missing the hvac_action() property. Pyvera exposes this through the get_hvac_state() method.

When I have a little time tomorrow I will try it out, but I think we need a section like this

def hvac_action(self):
   mode = self.vera_device.get_hvac_state()
   if mode == "Idle":
      return CURRENT_HVAC_IDLE
   if mode == "Heating":
      return CURRENT_HVAC_HEAT
   if mode == "Cooling":
      return CURRENT_HVAC_COOL
   return CURRENT_HVAC_OFF

And add the relevant constants to the import

CURRENT_HVAC_HEAT,
CURRENT_HVAC_COOL,
CURRENT_HVAC_IDLE,
CURRENT_HVAC_OFF,

Here's the relevant part from Pyvera

quinten@hp:~$ python Python 2.7.15+ (default, Oct 7 2019, 17:39:04) [GCC 7.4.0] on linux2 Type "help", "copyright", "credits" or "license" for more information.

import pyvera controller = pyvera.VeraController('http://192.168.0.39:3480') device = controller.get_device_by_id(52) device

VeraThermostat (id=52 category=Thermostat name=heatmiser entrance)

device.get_hvac_state() u'Idle'

skarfacegc commented 4 years ago

So this is an HA side issue?

awooganl commented 4 years ago

So this is an HA side issue?

In my opinion it is the vera component that needs a small addition, yes

awooganl commented 4 years ago

I've just tested this using a custom component in my Hassio config/custom_components folder and it now shows the hvac_action: idle property of my climate component (a Heatmiser Wifi thermostat that is exposed by a Vera Lite as if it was a Zwave device).

skarfacegc commented 4 years ago

if you want to give me your custom i can test as well.

awooganl commented 4 years ago

climate.py.zip

Download the current vera component (https://github.com/home-assistant/home-assistant/tree/dev/homeassistant/components/vera) and put the above climate.py in place of the downloaded version.

A diff for those who can read it.

quinten@hp:~/vera$ diff -u orig/climate.py custom/climate.py 
--- orig/climate.py 2019-12-06 09:37:48.561150172 +0000
+++ custom/climate.py   2019-12-08 14:03:07.680991450 +0000
@@ -5,6 +5,11 @@
 from homeassistant.components.climate.const import (
     FAN_AUTO,
     FAN_ON,
+    CURRENT_HVAC_OFF,
+    CURRENT_HVAC_HEAT,
+    CURRENT_HVAC_COOL,
+    CURRENT_HVAC_DRY,
+    CURRENT_HVAC_IDLE,
     HVAC_MODE_COOL,
     HVAC_MODE_HEAT,
     HVAC_MODE_HEAT_COOL,
@@ -73,6 +78,20 @@
         return SUPPORT_HVAC

     @property
+    def hvac_action(self):
+        """The current HVAC action (heating, cooling)"""
+        mode = self.vera_device.get_hvac_state()
+        if mode == "Idle":
+            return CURRENT_HVAC_IDLE
+        if mode == "Heating":
+            return CURRENT_HVAC_HEAT
+        if mode == "Cooling":
+            return CURRENT_HVAC_COOL
+        if mode == "Drying":
+            return CURRENT_HVAC_DRY
+        return CURRENT_HVAC_OFF
+
+    @property
     def fan_mode(self):
         """Return the fan setting."""
         mode = self.vera_device.get_fan_mode()
skarfacegc commented 4 years ago

Process (to ensure I did the right thing)

Seems to work (Heat/Idle appear to be reported correctly, can't check cooling at the moment)

awooganl commented 4 years ago

After the update to 103.0 I've redone the changes to the latest vera component. There is no difference in my change, but the underlying climate.py has been updated with this release.

How can we get this to be considered for implementation by the maintainer?

springstan commented 4 years ago

@awooganl I would recommend that you open a PR for the changes you made.

stale[bot] commented 4 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue now has been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

skarfacegc commented 4 years ago

don't know if this has been merged yet, probably still worth fixing

awooganl commented 4 years ago

Yes sorry guys, I've got no idea how to do the PR stuff to make this eligible for inclusion.

springstan commented 4 years ago

@awooganl I would recommend that you setup a local repository as described here: https://developers.home-assistant.io/docs/development_environment#setup-local-repository

And then follow this guide on how to include your changes and submit your work: https://developers.home-assistant.io/docs/development_submitting

stale[bot] commented 4 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue now has been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

awooganl commented 4 years ago

I'm going to give it a go at some point... seeing as no-one else is willing/able to do so

stale[bot] commented 4 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue now has been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

awooganl commented 4 years ago

I still want to give it a go at some point

github-actions[bot] commented 3 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

awooganl commented 3 years ago

I've still not given up on this. I've re-done my custom component changes as per https://github.com/home-assistant/core/issues/29220#issuecomment-562953090 and it is still the solution to this issue.

I'm just not in a position to do this the official way. Someone who is an experienced developer can maybe do this on my behalf? @vangorra maybe?

awooganl commented 3 years ago

@pavoni are you able to help?

pavoni commented 3 years ago

I don’t have a climate device connected to my Vera - so I’m cautious about making changes I can’t test (particularly as they might break existing devices).

This needs to be raised as a PR by someone who can test it.

Happy to review the PR far as I can if you tag me.

awooganl commented 3 years ago

I personally do not see how an additional read-only property could break existing devices, but if you are unable to do it, then that's fine.

I'll maybe try my hand at raising a PR again...

awooganl commented 3 years ago

Sorry to bother you @pavoni , but I've managed to set up a development environment and it looks to me that the latest updates you've added to pyvera to deal with communication errors has broken the climate entity of vera. Both my thermostats are reported as unavailable, and if I compare the other entities, I can see that they started to call the super().update() method? I'm by no means a python developer, so I'm not sure how that hangs together, but simply adding a

 def update(self) -> None:
     """Update the state."""
     super().update()

to the climate.py did not fix it, so I've no idea what else I can do.

If I replace all the .py's in the vera component with the one from my HassIO install (pyvera=0.3.11) then the thermostats are available and operating as normal

pavoni commented 3 years ago

Thanks for alerting me. The change I made will mark devices that have a comms error as unavailable.

Not sure why this isn't working for the thermostat, but love to have some help finding out.

You can see the code here https://github.com/pavoni/pyvera/blob/master/pyvera/__init__.py#L783

This is then used in HA here https://github.com/home-assistant/core/blob/dev/homeassistant/components/vera/__init__.py#L287

The code you highlighted will be inherited from the base class anyway - and is to deal with a vera bug where recovery from a failed state isn't always sent as an event. So the code forces HA to poll failed devices to make sure it sees the recovery.

So my guess is that for some reason your thermostats are reporting a comm status that I'm not expecting.

Could you change your code to log the comm status after the update?

Something like

_LOGGER.info("%s - fail=%s comms=%s"), self._name, self.vera_device.comm_failure, self.vera_device.get_strict_value("commFailure"))

In an emergency we can just add this to the VeraThermostat class to override the base class and always mark them as available.

    @property
    def available(self):
        """If device communications have failed return false."""
        return True

If you can help me diagnose what will work - I'll do a PR to sort it out.

awooganl commented 3 years ago

This is what I get with the LOGGER output

2021-02-25 22:27:20 INFO (SyncWorker_2) [homeassistant.loader] Loaded climate from homeassistant.components.climate 2021-02-25 22:27:20 INFO (MainThread) [homeassistant.setup] Setting up climate 2021-02-25 22:27:20 INFO (MainThread) [homeassistant.setup] Setup of domain climate took 0.0 seconds 2021-02-25 22:27:20 INFO (MainThread) [homeassistant.components.climate] Setting up climate.vera 2021-02-25 22:27:20 INFO (SyncWorker_1) [homeassistant.components.vera.climate] heatmiser entrance - fail=True comms=6 2021-02-25 22:27:20 INFO (SyncWorker_2) [homeassistant.components.vera.climate] heatmiser bedroom - fail=True comms=11 2021-02-25 22:27:21 INFO (SyncWorker_0) [homeassistant.components.vera.climate] heatmiser entrance - fail=True comms=6 2021-02-25 22:27:21 INFO (SyncWorker_3) [homeassistant.components.vera.climate] heatmiser bedroom - fail=True comms=11

It is probably worth mentioning that these are not actual Zwave devices, but Wifi thermostats that have a .Lua app written that enables them to be controlled and visible as if they are a Zwave device. So it is certainly possible that these do not have the same properties as a normal Zwave device.

awooganl commented 3 years ago

In fact, I've just checked and note

Screenshot 2021-02-25 at 22 32 00

This is the device 'heatmiser bedroom' and the 11 matches the CommFailure value. The CommFailure value for 'heatmiser entrance' is 6. So if I reset those values to 0, it should maybe work?

awooganl commented 3 years ago

Yup, sure as rain!

Screenshot 2021-02-25 at 22 35 38
pavoni commented 3 years ago

Perfect! Thanks for the help.

And hope this is an unusual case and the new release doesn't cause chaos!

awooganl commented 3 years ago

I think the underlying error here is in the .Lua code as I can see that it sets the initial CommFailure value to 0 only when the Luup engine reloads. Each time an error occurs involving the communication to the wifi unit, it increases the CommFailure value, so by that logic, it will always report it as unavailable once your changes make it to the "production" HassIO.

My Lua is only slightly better than my Python (I'm a C developer though, so I like a challenge ;)) but I will see if I can amend the .Lua to set the CommFailure value back to 0 once comms have been re-established. That way, it works even more like a Zwave device...

awooganl commented 3 years ago

Yes, have found the right part to insert the CommFailure reset

    -- Reset 'force database update' flag after first good data following comms recovery
    cs_change = 0

I'll be back to the actual issue of the missing hvac_action tomorrow!

awooganl commented 3 years ago

@pavoni I hope I've done this right... what a lot of work ;)

awooganl commented 3 years ago

So what happens now? It just gets ignored? What can I do to get this reviewed and approved?

github-actions[bot] commented 3 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.