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

Add support for Dyson Hot+Cool Fan as a climate device #14598

Merged
merged 15 commits into from Oct 21, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
194 changes: 194 additions & 0 deletions homeassistant/components/climate/dyson.py
@@ -0,0 +1,194 @@
"""
Support for Dyson Pure Hot+Cool link fan.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/climate.dyson/
"""
import logging

from homeassistant.components.dyson import DYSON_DEVICES
from homeassistant.components.climate import (
ClimateDevice, STATE_HEAT, STATE_COOL, STATE_IDLE,
SUPPORT_TARGET_TEMPERATURE, SUPPORT_FAN_MODE, SUPPORT_OPERATION_MODE)
from homeassistant.const import TEMP_CELSIUS, ATTR_TEMPERATURE

_LOGGER = logging.getLogger(__name__)

STATE_DIFFUSE = "Diffuse Mode"
STATE_FOCUS = "Focus Mode"
FAN_LIST = [STATE_FOCUS, STATE_DIFFUSE]
OPERATION_LIST = [STATE_HEAT, STATE_COOL]

SUPPORT_FLAGS = (SUPPORT_TARGET_TEMPERATURE | SUPPORT_FAN_MODE
| SUPPORT_OPERATION_MODE)


def setup_platform(hass, config, add_devices, discovery_info=None):
"""Setup the Dyson fan components."""
if discovery_info is None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only want to setup the platform via discovery from the dyson component, not from platform config.

return

from libpurecoollink.dyson_pure_hotcool_link import DysonPureHotCoolLink
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is libpurecoollink? Should add DEPENDENCIES = ['dyson'] at top

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed since we setup via discovery only.

# Get Dyson Devices from parent component.
add_devices(
[DysonPureHotCoolLinkDevice(device)
for device in hass.data[DYSON_DEVICES]
if isinstance(device, DysonPureHotCoolLink)]
)


class DysonPureHotCoolLinkDevice(ClimateDevice):
"""Representation of a Dyson climate fan."""

def __init__(self, device):
"""Initialize the fan."""
self._device = device
self._current_temp = None
self._pending_target_temp = None

async def async_added_to_hass(self):
"""Callback when entity is added to hass."""
self.hass.async_add_job(self._device.add_message_listener,
self.on_message)

def on_message(self, message):
"""Called when new messages received from the climate."""
from libpurecoollink.dyson_pure_state import DysonPureHotCoolState

if isinstance(message, DysonPureHotCoolState):
from libpurecoollink.const import HeatMode
_LOGGER.debug("Message received for fan device %s : %s", self.name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for climate device, or for pure hot cool link

message)
self.schedule_update_ha_state()
if (self._device.state.heat_mode == HeatMode.HEAT_ON.value
and self._pending_target_temp is not None):
# Set any pending new target temperature on dyson device.
self._set_temperature_on_device()

@property
def should_poll(self):
"""No polling needed."""
return False

@property
def supported_features(self):
"""Return the list of supported features."""
return SUPPORT_FLAGS

@property
def name(self):
"""Return the display name of this climate."""
return self._device.name

@property
def temperature_unit(self):
"""Return the unit of measurement."""
return TEMP_CELSIUS

@property
def current_temperature(self):
"""Return the current temperature."""
if self._device.environmental_state:
Copy link
Contributor

@awarecan awarecan Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an option feature of device? Would it even possible be None? If so, change it to guard style, e.g.

if not self._device.environmental_state:
    return self._current_temp

temperature_kelvin = self._device.environmental_state.temperature
...

EDIT: sorry I missed not in my previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._device.environmental_state is not an option feature. It is an environmental state callback for whenever the devices feels like to report it. It is set to none when the device is first started (and hence no state available). It will then update to new values when the device reports new states.

temperature_kelvin = self._device.environmental_state.temperature
if temperature_kelvin != 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is problem about 0? Will upstream lib return 0 as failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream library reports whatever device reports. Device reports its value as 0 when the device is first started (or other reasons).

self._current_temp = float("{0:.1f}".format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should move it to util/temperature.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes maybe there should be a Kelvin conversion in util/temperature.py. But regardlessly, this should remains as the actual Kelvin conversion is something similar to Celsius - 273.15, but the device uses whole number substraction (i.e. C - 273 internally).

temperature_kelvin - 273))
return self._current_temp

@property
def target_temperature(self):
"""Return the target temperature."""
if self._pending_target_temp is None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should always return _device.sate.heat_target, not this _pending_target_temp

I am very confusing about this _pending_target_temp, is it any necessary? Since your device supports target_temperature feature, you don't need cache your input target value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because the device ignores set target temp request when it is in cool mode. Without it, setting target temp when the device is not in heat mode will render the user interface and actual device target temp out of sync. It also won't have a callback update (as none of the internal state changes) so this out of sync remains indefinitely. The user set values will be rendered useless as a result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, if device ignores set target temp in some condition, you should simply log error and do nothing. as long as you always return device's target temp as entity's target temp, I cannot see the reason for out of sync.

heat_target = int(self._device.state.heat_target) / 10
return int(heat_target - 273)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not update self._target_temp here and return that consistently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because dyson device does not response to set temp message when it is not in heating mode. This creates a pending value to set it when it is in heating mode later.

return self._pending_target_temp

@property
def current_humidity(self):
"""Return the current humidity."""
if self._device.environmental_state:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as current_temperature

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

if self._device.environmental_state.humidity == 0:
return None
return self._device.environmental_state.humidity
return None

@property
def current_operation(self):
"""Return current operation ie. heat, cool, idle."""
from libpurecoollink.const import HeatMode, HeatState
if self._device.state.heat_mode == HeatMode.HEAT_ON.value:
if self._device.state.heat_state == HeatState.HEAT_STATE_ON.value:
return STATE_HEAT
return STATE_IDLE
return STATE_COOL

@property
def operation_list(self):
"""Return the list of available operation modes."""
return OPERATION_LIST

@property
def current_fan_mode(self):
"""Return the fan setting."""
from libpurecoollink.const import FocusMode
if self._device.state.focus_mode == FocusMode.FOCUS_ON.value:
return STATE_FOCUS
return STATE_DIFFUSE

@property
def fan_list(self):
"""Return the list of available fan modes."""
return FAN_LIST

def set_temperature(self, **kwargs):
"""Set new target temperature."""
target_temp = kwargs.get(ATTR_TEMPERATURE)
if target_temp is None:
return
target_temp = int(target_temp)
_LOGGER.debug("Set %s temperature %s", self.name, target_temp)
# Limit the target temperature into acceptable range.
target_temp = min(self.max_temp, target_temp)
target_temp = max(self.min_temp, target_temp)
self._pending_target_temp = target_temp
# Dyson only response when it is in heat mode.
from libpurecoollink.const import HeatMode
if self._device.state.heat_mode == HeatMode.HEAT_ON.value:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should first check this condition, and failed method fast

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand how to fail this method faster? Assuming you agree with the pending value method, I will still need to do all the checking and store the value regardlessly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    def set_temperature(self, **kwargs):
        """Set new target temperature."""
        target_temp = kwargs.get(ATTR_TEMPERATURE)
        if target_temp is None:
            return

        # Dyson only response when it is in heat mode.
        from libpurecoollink.const import HeatMode
        if self._device.state.heat_mode != HeatMode.HEAT_ON.value:
            _LOGGER.error("Cannot set target temperature, bla bla")
            return

        target_temp = int(target_temp)
        # Limit the target temperature into acceptable range.
        target_temp = min(self.max_temp, target_temp)
        target_temp = max(self.min_temp, target_temp)
        _LOGGER.debug("Set %s temperature %s", self.name, target_temp)
        self._set_temperature_on_device(target_temp)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with this approach, it will really confuse the user in what they expect (as with I did when I first started developing this component.)
The limitations of this approach are:

  1. The discrepancy from the physical interface.
    If you look at the physical remote:
    remote
    ...there really isn't an on/off switch for cool/hot mode. The top right button always switches to cool mode, and there are temperature control buttons for up and down. Those two buttons control the temperature, but also always will affect the target temperature regardless of heat/cool mode. In fact, it switches to heat mode if it was previously in cool mode. (Internally, I think it makes a call to switch to heat mode if it is in cool mode, then makes a call to increase/decrease temperature; which is different than our current approach of controlling the heat mode on/off and temp separately. This leads to my approach 3 later on).
  2. Within the front end interface, changing the target temp via up/down arrowhead instantly changes the temperature value (even though its state remains the same). It would be out of sync (and not what the user expects) where the user would think they successfully changed the temperature but in reality, the request was simply ignored.
  • Logging an error doesn't really make sense because the upstream lib nor device will generate an error or any messages. It just simply has no effect.

I have given a thought when I first developed this component. There are possibly 3 approaches to this:

  1. Simple and naively just send the request without any checking. It was how I did it when I first started, but it really confuses me due to the inconsistent interface from its homeassistant and physical representation. And it renders the user input useless (without a concrete way of knowing the request was ignored because the updates of device states need to wait for the mqtt callback)
  2. My current approach, which simply stores the value for now, and re-deploy later on. This keep track of user wanted temp value and apply it when appropriate. Whether it's best or not it is debatable, but it feels most natural to me.
  3. Simply mimic the physical interface where we check if it is in heat mode when user changes target temp. If it is not, then we first make a call to switch heatmode on, then make a call to change the target temp (Technically we can combine both calls to one because mqtt request can set multiple configurations together). This will result in changing temperature will switch heat mode on (which is, in fact, the most consistent with the physical interface as it is how Dyson does it in its physical remote and Android/iPhone app). However, it is a bit different from how normal homeassistant climate device works I assume?

Please let me know which you think is more appropriate thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awarecan or @balloob please give you're final opinion so we can move this PR along.

Copy link
Member

@balloob balloob Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the front end interface, changing the target temp via up/down arrowhead instantly changes the temperature value (even though its state remains the same). It would be out of sync (and not what the user expects) where the user would think they successfully changed the temperature but in reality, the request was simply ignored.

The frontend will wait a few seconds before calling set temperature. After setting, it will reset the frontend to the current state of the climate. So it should be clear to the user that the operation was not successful.

The moment the MQTT callback comes in the UI will updated immediately.

That should just work then right?

I am not a fan of trying to workaround quirks in our user interface, as that is not how we should write code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

self._set_temperature_on_device()
else:
# Update the display of pending target temperature to user.
self.schedule_update_ha_state()

def _set_temperature_on_device(self):
from libpurecoollink.const import HeatTarget
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing docstring

self._device.set_configuration(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't check upstream lib, but will this call trigger a message, then re-entry self.on_message()

Copy link
Contributor Author

@soraxas soraxas Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This set a mqtt message to device. The device will then (usually) send its new state to its mqtt subscriber when it's internal state changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK then.

heat_target=HeatTarget.celsius(self._pending_target_temp))
self._pending_target_temp = None

def set_fan_mode(self, fan_mode):
"""Set new fan mode."""
_LOGGER.debug("Set %s focus mode %s", self.name, fan_mode)
from libpurecoollink.const import FocusMode
if fan_mode == STATE_FOCUS:
self._device.set_configuration(focus_mode=FocusMode.FOCUS_ON)
elif fan_mode == STATE_DIFFUSE:
self._device.set_configuration(focus_mode=FocusMode.FOCUS_OFF)

def set_operation_mode(self, operation_mode):
"""Set operation mode."""
_LOGGER.debug("Set %s heat mode %s", self.name, operation_mode)
from libpurecoollink.const import HeatMode
if operation_mode == STATE_HEAT:
self._device.set_configuration(heat_mode=HeatMode.HEAT_ON)
elif operation_mode == STATE_COOL:
self._device.set_configuration(heat_mode=HeatMode.HEAT_OFF)

@property
def min_temp(self):
"""Return the minimum temperature."""
return 1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I should make these as constants or not. This weren't defined in the library as constants (but enforced with a check against if temperature < 1 or temperature > 37) so I cannot import them. It just seems redundant to define them as constants in this component alone as they are only used once.


@property
def max_temp(self):
"""Return the maximum temperature."""
return 37
1 change: 1 addition & 0 deletions homeassistant/components/dyson.py
Expand Up @@ -102,5 +102,6 @@ def setup(hass, config):
discovery.load_platform(hass, "sensor", DOMAIN, {}, config)
discovery.load_platform(hass, "fan", DOMAIN, {}, config)
discovery.load_platform(hass, "vacuum", DOMAIN, {}, config)
discovery.load_platform(hass, "climate", DOMAIN, {}, config)

return True