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

New venstar climate component #11639

Merged
merged 6 commits into from Jan 25, 2018
Merged

New venstar climate component #11639

merged 6 commits into from Jan 25, 2018

Conversation

@Cinntax
Copy link
Contributor

Cinntax commented Jan 14, 2018

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:

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.

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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

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

password=password,
proto=proto)

add_devices([VenstarThermostat(client)])

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

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.

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]

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

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?

This comment has been minimized.

Copy link
@Cinntax

Cinntax Jan 14, 2018

Author Contributor

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.

return 60

# Commands
def set_temperature(self, **kwargs):

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

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.

temperature = kwargs.get(ATTR_TEMPERATURE)

if self._client.mode == self._client.MODE_HEAT:
_LOGGER.info("Currently operating in heat mode. "

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

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.


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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

Again, I think that's too much logging.


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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

See above.


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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 14, 2018

Contributor

See above.

@Cinntax

This comment has been minimized.

Copy link
Contributor Author

Cinntax commented Jan 14, 2018

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

@Cinntax Cinntax mentioned this pull request Jan 14, 2018
2 of 2 tasks complete
@Cinntax Cinntax mentioned this pull request Jan 16, 2018
@Cinntax

This comment has been minimized.

Copy link
Contributor Author

Cinntax commented Jan 18, 2018

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!

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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 19, 2018

Contributor

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.

This comment has been minimized.

Copy link
@Cinntax

Cinntax Jan 19, 2018

Author Contributor

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


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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 19, 2018

Contributor

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

This comment has been minimized.

Copy link
@Cinntax

Cinntax Jan 19, 2018

Author Contributor

okay- missed that one. Will remove.

Copy link
Contributor

tinloaf left a comment

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

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

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 19, 2018

Contributor

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?

This comment has been minimized.

Copy link
@Cinntax

Cinntax Jan 19, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@tinloaf

tinloaf Jan 19, 2018

Contributor

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

This comment has been minimized.

Copy link
@Cinntax

Cinntax Jan 20, 2018

Author Contributor

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

Copy link
Contributor

tinloaf left a comment

Looks good to me!

@fabaff fabaff merged commit 72bb0e9 into home-assistant:dev Jan 25, 2018
5 checks passed
5 checks passed
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 93.83%
Details
hound No violations found. Woof!
@balloob balloob mentioned this pull request Jan 26, 2018
hmn added a commit to hmn/home-assistant-dev that referenced this pull request Jan 26, 2018
* dev: (28 commits)
  Update frontend to 20180126.0
  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)
  Snips - (fix) removed endSession mqtt response on error and unknown intents (home-assistant#11908)
  Pushbullet (fix) invalid keyword, added unittests (home-assistant#11804)
  Add android option for linux_battery.py (home-assistant#11852)
  Bme680 (home-assistant#11892)
  device tracker - tomato https support (home-assistant#11566)
  Handle Daikin AC adapters without fan mode and swing mode support (home-assistant#11840)
  [SMALL-PR] Don't make climate calls if feature is not supported (home-assistant#11841)
  ...
matemaciek pushed a commit to matemaciek/home-assistant that referenced this pull request Jan 27, 2018
* 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)
  ...
@Cinntax Cinntax deleted the Cinntax:venstar branch Jan 28, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.