New venstar climate component by Cinntax · Pull Request #11639 · home-assistant/core

Cinntax · 2018-01-14T16:24:37Z

Description:

New climate platform for venstar wifi enabled thermostats.

Related issue (if applicable): fixes # None

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4412

Example entry for `configuration.yaml` (if applicable):

climate:
  - platform: venstar
    host: IP_OR_HOSTNAME_OF_THERMOSTAT
    ssl: True/False
    username: OPTIONAL_AUTH_USER_HERE
    password: OPTIONAL_AUTH_PASS_HERE
    timeout: 5

Checklist:

The code change is tested and works locally.

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

Documentation added/updated in home-assistant.github.io

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

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

tinloaf · 2018-01-14T17:25:55Z

+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Required(CONF_HOST): cv.string,
+    vol.Optional(CONF_USERNAME, default=None): cv.string,


I don't think you need to set default=None. That's the default's default.

tinloaf · 2018-01-14T17:26:57Z

+                                                 password=password,
+                                                 proto=proto)
+
+    add_devices([VenstarThermostat(client)])


If you call this with True as second parameter, it will schedule an update right after adding the device. That might be desirable?

tinloaf · 2018-01-14T17:28:04Z

+    def __init__(self, client):
+        """Initialize the thermostat."""
+        self._client = client
+        self._client.update_info()


I guess this does I/O? Please don't do that in the constructor. Instead, see above: call add_devices(…, True), and HA will call your update() method right after adding the entity.

tinloaf · 2018-01-14T17:28:52Z

+        self._client.update_info()
+        self._client.update_sensors()
+        self._fan_list = [STATE_ON, STATE_AUTO]
+        self._operation_list = [STATE_HEAT, STATE_COOL, STATE_IDLE, STATE_AUTO]


For extra cleanliness, you could extract those two lists into a tuple constant somewhere at the start of this file.

tinloaf · 2018-01-14T17:30:26Z

+    @property
+    def supported_features(self):
+        """Return the list of supported features."""
+        if self._client.mode == self._client.MODE_AUTO:


What does this mean? When the device is in auto mode, it tries to stay between a min and max temperature? And otherwise… it doesn't?

That's correct, yes - the thermostat physically has 4 modes:

heat- you set a single temperature, and the thermostat will attempt to heat the house UP TO this temp.

-cool- you set a single temperature, and the thermostat will attempt to cool the house DOWN TO this temp

-auto- you set 2 temperatures, and the thermostat will automatically swap between heating and cooling depending upon the temperature of the house. It's essentially the heat/cool mode combined.

-off/idle - no hold at all, but reporting on temp

I don't expect to use the auto setting, but felt i needed to do something for it in case someone else did.

tinloaf · 2018-01-14T17:36:40Z

+        return 60
+
+# Commands
+    def set_temperature(self, **kwargs):


The set_temperature method can also be called with an ATTR_OPERATION_MODE in the kwargs. The idea is that the thermostat should then first set the requested operation mode, and then the requested temperature. Please check here whether ATTR_OPERATION_MODE is in the kwargs and set the operation mode accordingly.

tinloaf · 2018-01-14T17:39:13Z

+            temperature = kwargs.get(ATTR_TEMPERATURE)
+
+        if self._client.mode == self._client.MODE_HEAT:
+            _LOGGER.info("Currently operating in heat mode. "


That is a lot of logging… HA will automatically print a log line when your entity changes its state. So, I'd suggest removing this.

tinloaf · 2018-01-14T17:40:22Z

+
+    def set_fan_mode(self, fan):
+        """Set new target fan mode."""
+        _LOGGER.info("Contacting your thermostat to "


Again, I think that's too much logging.

tinloaf · 2018-01-14T17:40:49Z

+
+    def set_operation_mode(self, operation_mode):
+        """Set new target operation mode."""
+        _LOGGER.info("Contacting your thermostat to set "


tinloaf · 2018-01-14T17:41:23Z

+
+    def set_humidity(self, humidity):
+        """Set new target humidity."""
+        _LOGGER.info("Contacting your thermostat to set it's "


Cinntax · 2018-01-14T19:01:39Z

Thanks! I've made a few comments- otherwise i'll make the recommended changes and resubmit.

Cinntax · 2018-01-18T03:03:01Z

Ok I think we’re about ready for another look when you get a chance- we had another tester come in and highlight a few additional items to resolve. Should be good to go!

tinloaf · 2018-01-18T17:59:44Z

+    def __init__(self, client):
+        """Initialize the thermostat."""
+        self._client = client
+        self._fan_list = VALID_FAN_STATES


These variables are never modified, so you actually don't need them as members, do you? Just make the methods return VALID_FAN_STATES / VALID_THERMOSTAT_MODES.

Heh- yeah you're right :). I'll get rid of the _fan_list and _operation_list and adjust the methods.

tinloaf · 2018-01-18T18:00:17Z

+
+    def update(self):
+        """Update the data from the thermostat."""
+        _LOGGER.info("Refreshing data from your Venstar Thermostat.")


HA will automatically log whenever the state of your device changes. I'd recommend to remove this logging to reduce the logspam.

okay- missed that one. Will remove.

tinloaf

Still have some minor comments, but looks good to me!

tinloaf · 2018-01-19T10:18:56Z

+        elif self._client.mode == self._client.MODE_AUTO:
+            return STATE_AUTO
+        else:
+            return STATE_IDLE


I'm not really sure what STATE_IDLE should mean for an operation mode. I guess STATE_OFF would be better here? Just so I understand this correctly: If the device is in this state, it won't start automatically because some temperature was over/undershot, right? So, basically, it's switched off?

It's a little confusing because the thermostat is still "on". It's screen is on, it's still monitoring/reporting temperature/humidity, still responding to API calls, etc.

But your assumption is correct there- it will not turn on the air conditioner, nor the furnace to attempt to keep a certain temperature.

It looks like in the honeywell component they're referring to this state as "idle" (see line 286 from honeywel.py). Looks like nuheat is also.

Okay… we should probably clean that up at some point, but for now this is probably fine. :)

Oh- no problem if you'd rather use the "off" state to represent this. I'll go ahead and do that.

tinloaf

Looks good to me!

* upstream/master: (465 commits) Update pyhomematic to 0.1.38 (home-assistant#11936) Implement Alexa temperature sensors (home-assistant#11930) Fixed rfxtrx binary_sensor KeyError on missing optional device_class (home-assistant#11925) Allow setting climate devices to AUTO mode via Google Assistant (home-assistant#11923) fixes home-assistant#11848 (home-assistant#11915) Add "write" service to system_log (home-assistant#11901) Update frontend to 20180126.0 Version 0.62 Allow separate command and state OIDs and payloads in SNMP switch (home-assistant#11075) Add ERC20 tokens to etherscan.io sensor (home-assistant#11916) Report scripts and groups as scenes to Alexa (home-assistant#11900) Minor fix to configuration validation and related log line. (home-assistant#11898) Multi-Room Support for Greenwave Reality (home-assistant#11364) Added Xeoma camera platform (home-assistant#11619) Improve foscam library exception support (home-assistant#11701) Iota wallet (home-assistant#11398) New venstar climate component (home-assistant#11639) Update python-wink version and multiple wink fixes/updates. (home-assistant#11833) Use API to discover Hue if no bridges specified (home-assistant#11909) Clarify emulated hue warning (home-assistant#11910) ...

New venstar climate component

138f08f

Cinntax requested a review from andrey-git as a code owner January 14, 2018 16:24

homeassistant added the cla-signed label Jan 14, 2018

Cinntax mentioned this pull request Jan 14, 2018

Resolved validation issue on set temperature point method. hpeyerl/venstar_colortouch#3

Merged

tinloaf reviewed Jan 14, 2018

View reviewed changes

Cinntax mentioned this pull request Jan 14, 2018

New venstar climate platform home-assistant/home-assistant.io#4412

Merged

2 tasks

Cinntax added 2 commits January 14, 2018 13:59

Corrected items raised during the PR process.

b3981fa

Corrected flak8 issues.

8dc5e9f

Cinntax mentioned this pull request Jan 16, 2018

T5800 support hpeyerl/venstar_colortouch#4

Closed

Add support for venstar models without humidity control.

126b4b2

tinloaf reviewed Jan 19, 2018

View reviewed changes

Changed the idle operation mode to off. Few other adjustments.

ad5f55e

tinloaf approved these changes Jan 20, 2018

View reviewed changes

Minor changes (log messages, sorting, etc.)

340b52f

fabaff merged commit 72bb0e9 into home-assistant:dev Jan 25, 2018

balloob mentioned this pull request Jan 26, 2018

0.62 #11921

Merged

Cinntax deleted the venstar branch January 28, 2018 23:23

home-assistant locked and limited conversation to collaborators May 29, 2018

Uh oh!

Conversation

Cinntax commented Jan 14, 2018

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cinntax commented Jan 14, 2018

Uh oh!

Cinntax commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinloaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinloaf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Example entry for `configuration.yaml` (if applicable):