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

T5800 support #4

Closed
rbebeau opened this issue Jan 15, 2018 · 6 comments
Closed

T5800 support #4

rbebeau opened this issue Jan 15, 2018 · 6 comments

Comments

@rbebeau
Copy link
Contributor

rbebeau commented Jan 15, 2018

I have an older Colortouch T5800 that I'd like to use with your code (for HomeAssistant). I noticed a few things that need to be somehow 'optional' for it to work with the T5800:

  1. In the update_info() function, both hum_setpoint and dehum_setpoint do not exist in the JSON returned by the device, making this call fail.

Example JSON:
$ curl "http://thermostatip/query/info"
{"name":"HOME","mode":3,"state":1,"fan":0,"fanstate":1,"tempunits":0,"schedule":1,"schedulepart":2,"away":0,"spacetemp":65.0,"heattemp":68.0,"cooltemp":80.0,"cooltempmin":65.0,"cooltempmax":99.0,"heattempmin":35.00,"heattempmax":80.0,"setpointdelta":4.0,"availablemodes":0}

  1. In the get_thermostat_sensor() function, trying to get the 'hum' sensor will also cause a problem on the T5800.

I'm attaching a diff that seems to fix these issues.

Thank you for your work!

--- venstarcolortouch.py.orig   2018-01-15 13:29:44.061887273 -0500
+++ venstarcolortouch.py        2018-01-15 14:24:49.799229048 -0500
@@ -138,8 +138,14 @@
         self.tempunits = self.get_info("tempunits")
         self.away = self.get_info("away")
         self.schedule = self.get_info("schedule")
-        self.hum_setpoint = self.get_info("hum_setpoint")
-        self.dehum_setpoint = self.get_info("dehum_setpoint")
+        if 'hum_setpoint' in self._info:
+          self.hum_setpoint = self.get_info("hum_setpoint")
+        else:
+          self.hum_setpoint = None
+        if 'dehum_setpoint' in self._info:
+          self.dehum_setpoint = self.get_info("dehum_setpoint")
+        else:
+          self.dehum_setpoint = None
         #
         return True
 
@@ -166,7 +172,10 @@
 
     def get_thermostat_sensor(self, attr):
         if self._sensors != None and self._sensors["sensors"] != None and len(self._sensors["sensors"]) > 0:
-            return self._sensors["sensors"][0][attr]
+            if attr in self._sensors["sensors"][0]:
+              return self._sensors["sensors"][0][attr]
+            else:
+              return None
         else:
             return None
@hpeyerl
Copy link
Owner

hpeyerl commented Jan 15, 2018

That looks like the right thing to do. Can you submit a pull request? It'd be good if there were a comment in there indicating it was due to the keys missing on a T5800.

@Cinntax probably cares about this for home-assistant.

@Cinntax
Copy link
Contributor

Cinntax commented Jan 16, 2018

Oh nice- the more the merrier.
@rbebeau - i already have a pending pull request for home-assistant's dev branch. Looks like i just missed the cut for 0.61.
home-assistant/core#11639

Also, we're discussing it here:
https://community.home-assistant.io/t/venstar-thermostat/20668/8

A couple of things:

  1. The hass reviewers don't want I/O in our constructor- i believe there is still I/O in the venstarColorTouch constructor (not in the hass component at least). Now that we're talking about another release, we may as well take that out (the call to login()) that is.

  2. Right now, the hass component assumes we can set the humidity. @rbebeau are you aware of anything else that doesn't apply to you with your model? I think maybe the easiest thing right now would be simply a configuration.yaml variable to disable humidity settings.

@rbebeau
Copy link
Contributor Author

rbebeau commented Jan 16, 2018

@Cinntax

As far as I know, anything humidity related is pretty much the only thing that is not supported for the T5800 model. A yaml config variable sounds good to me. I'm not sure what other thermostat models (if any) would be the same as mine.

@Cinntax
Copy link
Contributor

Cinntax commented Jan 16, 2018

Okay cool- actually, as I got to putting the "flag" in the hass component, I thought it might be cleaner if we have the library "tell" the component what it supports. So i'm thinking i can modify the library here to put a "supports humidity" or something of that nature, and it will return true if the thermostat supports it, and it's enabled.

@hpeyerl
Copy link
Owner

hpeyerl commented Jan 16, 2018

I've pushed 0.4 to pypi with changes from @rbebeau.

@hpeyerl
Copy link
Owner

hpeyerl commented Jan 16, 2018

I assume this issue has been resolved so I'm going to close it.

@hpeyerl hpeyerl closed this as completed Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants