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

Conversation

Projects
None yet
8 participants
@soraxas
Contributor

soraxas commented May 23, 2018

Description:

This PR added support for Dyson pure hot+cool fan as a climate device. In additional of the fan functionality, this component will expose the hot+cool fan as a climate device to control temperature, fan mode, etc.

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

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • Documentation added/updated in home-assistant.github.io
  • No new dependencies apart of those from the parent component.
  • Unit tests have been added to verify that the new code works.
  • New files were added to .coveragerc.
@homeassistant

This comment has been minimized.

homeassistant commented May 23, 2018

Hi @soraxas,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Removed decimal place in kelvin units conversion
Minor edits to be consistent with Dyson's internal conversion of temperature from kelvin to celsius. It does not include decimal place to convert between kelvin and celsius.
def setup_platform(hass, config, add_devices, discovery_info=None):
"""Setup the Dyson fan components."""
from libpurecoollink.dyson_pure_hotcool_link import DysonPureHotCoolLink

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Add a check if discovery_info is None. If so, return.

"""Setup the Dyson fan components."""
from libpurecoollink.dyson_pure_hotcool_link import DysonPureHotCoolLink
_LOGGER.debug("Creating new Dyson fans")

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Remove this.

if isinstance(d, DysonPureHotCoolLink)
]:
dyson_entity = DysonPureHotCoolLinkDevice(hass, device)
hass.data[DYSON_FAN_DEVICES].append(dyson_entity)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Why do we need to store the entities in hass.data?

class DysonPureHotCoolLinkDevice(ClimateDevice):
"""Representation of a Dyson climate fan."""
def __init__(self, hass, device):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Don't pass in hass. It will be set on the entity when the entity has been added to home assistant. This means you can access hass with self.hass, after the entity has been added to home assistant.

def __init__(self, hass, device):
"""Initialize the fan."""
_LOGGER.debug("Creating device %s", device.name)
self.hass = hass

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Remove this.

# Limit the target temperature into acceptable range.
target_temp = min(self.max_temp, target_temp)
target_temp = max(self.min_temp, target_temp)
self._target_temp = target_temp

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

This should preferably be done via the state update callback, if that information is available.

elif self.temp_unit == TEMP_FAHRENHEIT:
self._device.set_configuration(
heat_target=HeatTarget.fahrenheit(self._target_temp))
self._target_temp = None

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Why set it to None?

@property
def min_temp(self):
"""Return the minimum temperature."""
return convert_temperature(1, TEMP_CELSIUS, self.temp_unit)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Use self.temperature_unit here instead when converting.

@property
def max_temp(self):
"""Return the maximum temperature."""
return convert_temperature(37, TEMP_CELSIUS, self.temp_unit)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Same as above.

return kelvin - 273
@staticmethod
def kelvin_to_fahrenheit(kelvin):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

This can be removed as mentioned above.

@wafflebot wafflebot bot added the in progress label Jul 11, 2018

@MartinHjelmare

Don't change the access mode (644 -> 755) of the dyson component module.

Is it enough to only use Celsius when home assistant is interfacing with the device or do we need to use both Celsius and Fahrenheit to not confuse the user when the user is interfacing with the device outside of home assistant?

See further comments below and the unresolved comments above.

if self.temperature_unit == TEMP_CELSIUS:
self._device.set_configuration(
heat_target=HeatTarget.celsius(self._target_temp))
elif self.temperature_unit == TEMP_FAHRENHEIT:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

If we only need to use Celsius when interfacing with the device, we don't need this anymore.

Is the device showing the temperature outside home assistant to the user, and that's why we need both unit systems?

from libpurecoollink.dyson_pure_hotcool_link import DysonPureHotCoolLink
if DYSON_FAN_DEVICES not in hass.data:
hass.data[DYSON_FAN_DEVICES] = []

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

This isn't used now.

"""Return the target temperature."""
if self._target_temp is None:
heat_target = int(self._device.state.heat_target) / 10
return int(heat_target - 273)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

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

This comment has been minimized.

@soraxas

soraxas Jul 11, 2018

Contributor

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.

@property
def operation_list(self):
"""Return the list of available operation modes."""
return [STATE_HEAT, STATE_COOL]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

This can be extracted to a constant at the module level which we return here.

@property
def fan_list(self):
"""Return the list of available fan modes."""
return [STATE_FOCUS, STATE_DIFFUSE]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

Same as above.

elif self.temperature_unit == TEMP_FAHRENHEIT:
self._device.set_configuration(
heat_target=HeatTarget.fahrenheit(self._target_temp))
self._target_temp = None

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

Why set it to None?

This comment has been minimized.

@soraxas

soraxas Jul 11, 2018

Contributor

To clear the pending target temperature

@soraxas

This comment has been minimized.

Contributor

soraxas commented Jul 11, 2018

Hi @MartinHjelmare!
Thanks for your swift feedbacks. And apologies for the incomplete commit before, I was working it somewhere before and was thinking to push it to my repo and pull onto my server to confirm it's working as expected with a real device. Probably not the best workflow as it triggers your review in no time.

A note on the reason behind all the fuss of self._target_temp is because of the reason that dyson device ignores all request of changing temperature when the device is not in heating mode. The nature of the device is that it is a fan+heating device. So naturally, it can switch heating mode on & off. When the device is in cool mode, using mqtt request to change temperature (while in cool mode) will be ignored (this is how the library communicate with the device). It will remains to report the old temperature in the next callback.

Therefore, when I wrote the component the self._target_temp is used to remember the last temperature user want to set, and set it after the device is in heat mode. Perhepts a better name for it is self._pending_target_temp as it's a pending (or queued) value that the user wants to set. The property displays the pending value when the device is not in heating mode (i.e. in cooling mdoe), and set it to the desire temperature once the device is in heating mode (i.e. when the device will response to temp change request).

@soraxas

This comment has been minimized.

Contributor

soraxas commented Jul 11, 2018

Apology for the access mode, probably happens when I switched the working environment to windows at some point.

Also minor changes to all the enum instances. I shouldn't compare the state values (which are string instances because that's what dyson device reports via mqtt) to enum objects. Not sure why it works before (probably due to different python version? The equality now return false when comparing string to enum with the same value). But I should compare them with the enum's value instead.

@MartinHjelmare

Thanks for the clarification. It makes more sense now. Good renaming and refactoring too.

"""Test set climate temperature."""
device = _get_device_heat_on()
device.temp_unit = TEMP_CELSIUS
component = dyson.DysonPureHotCoolLinkDevice(device)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

This is not a component but an entity.

@property
def min_temp(self):
"""Return the minimum temperature."""
return convert_temperature(1, TEMP_CELSIUS, self.temperature_unit)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

Since temperature_unit is fixed to Celsius, we don't need the conversion.

@property
def max_temp(self):
"""Return the maximum temperature."""
return convert_temperature(37, TEMP_CELSIUS, self.temperature_unit)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 11, 2018

Member

Same as above.

@@ -187,9 +186,9 @@ def set_operation_mode(self, operation_mode):
@property
def min_temp(self):
"""Return the minimum temperature."""
return convert_temperature(1, TEMP_CELSIUS, self.temperature_unit)
return 1

This comment has been minimized.

@soraxas

soraxas Jul 12, 2018

Contributor

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.

@MartinHjelmare

Looks good!

The access mode is still changed though on the component. Please fix that before we merge.

@soraxas

This comment has been minimized.

Contributor

soraxas commented Jul 12, 2018

Sorry about that, should be fixed now!

@MartinHjelmare

Great!

@MartinHjelmare

I just looked at the coverage report, and the coverage is too low for the new module.

https://coveralls.io/builds/17951660/source?filename=homeassistant%2Fcomponents%2Fclimate%2Fdyson.py

Please either add tests to reach 95% or exclude the module from coverage calculation in .coveragerc.

@soraxas

This comment has been minimized.

Contributor

soraxas commented Jul 16, 2018

So apparently assert_called_once is only introduced after python 3.6, hence my tests failed on py35 but passed in py36 & py37. I changed to other assertation methods to achieve the same effect.

@soraxas

This comment has been minimized.

Contributor

soraxas commented Jul 24, 2018

Any updates on it? The coverage is now at 99.06%, and the only method that the test cases did not cover is async def async_added_to_hass(self): (for which I am not sure how to call it)

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Jul 24, 2018

Now we never test setting up the whole component and this platform. We just test calling setup_platform of this platform. Please add a test where we use setup_component imported from homeassistant.setup.py to set up the dyson component and this platform.

In general we try to write tests a bit more like integration tests, by using the home assistant core to set up the tests and also fetch state and assert state. Doing this usually simplifies testing, ie fewer tests will cover more code, and makes the tests more robust.

Coroutines can be tested by standalone pytest test functions that are defined as coroutines. Though async_added_to_hass will be called by the core during platform setup, so it could possibly be validated in that test.

@mock.patch('libpurecoollink.dyson.DysonAccount.devices',
return_value=[_get_device_heat_on(), _get_device_cool()])
@mock.patch('libpurecoollink.dyson.DysonAccount.login', return_value=True)
def test_setup_component_with_parent_discovery(self, mocked_login, mocked_devices):

This comment has been minimized.

@houndci-bot

houndci-bot Jul 25, 2018

line too long (87 > 79 characters)

}
})
self.assertEqual(len(self.hass.data[dyson.DYSON_DEVICES]), 2)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 25, 2018

Member

Can we assert here that device._device.add_message_listener was called?

This comment has been minimized.

@soraxas

soraxas Jul 27, 2018

Contributor

Yes!

@awarecan

I am confusing about self._pending_target_temp, seems unnecessary,

if discovery_info is None:
return
from libpurecoollink.dyson_pure_hotcool_link import DysonPureHotCoolLink

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

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

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 27, 2018

Member

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

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

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

Why?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 27, 2018

Member

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

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

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

for climate device, or for pure hot cool link

self.schedule_update_ha_state()
def _set_temperature_on_device(self):
from libpurecoollink.const import HeatTarget

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

missing docstring

def _set_temperature_on_device(self):
from libpurecoollink.const import HeatTarget
self._device.set_configuration(

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

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

This comment has been minimized.

@soraxas

soraxas Jul 27, 2018

Contributor

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.

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

OK then.

"""Return the current temperature."""
if self._device.environmental_state:
temperature_kelvin = self._device.environmental_state.temperature
if temperature_kelvin != 0:

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

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

This comment has been minimized.

@soraxas

soraxas Jul 27, 2018

Contributor

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

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

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

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

This comment has been minimized.

@soraxas

soraxas Jul 27, 2018

Contributor

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.

if self._device.environmental_state:
temperature_kelvin = self._device.environmental_state.temperature
if temperature_kelvin != 0:
self._current_temp = float("{0:.1f}".format(

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

Maybe we should move it to util/temperature.py

This comment has been minimized.

@soraxas

soraxas Jul 27, 2018

Contributor

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).

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

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

Same question as current_temperature

This comment has been minimized.

@soraxas

soraxas Jul 27, 2018

Contributor

Same as above

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:

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor

We should first check this condition, and failed method fast

This comment has been minimized.

@soraxas

soraxas Jul 27, 2018

Contributor

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.

This comment has been minimized.

@awarecan

awarecan Jul 27, 2018

Contributor
    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)

This comment has been minimized.

@soraxas

soraxas Jul 28, 2018

Contributor

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.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 12, 2018

Member

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

This comment has been minimized.

@balloob

balloob Aug 13, 2018

Member

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.

This comment has been minimized.

@soraxas

soraxas Aug 25, 2018

Contributor

Sure!

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

This comment has been minimized.

@houndci-bot

houndci-bot Jul 27, 2018

line too long (83 > 79 characters)

@soraxas

This comment has been minimized.

Contributor

soraxas commented Aug 25, 2018

Removed all of the pending target temperature thingy, and now it sends a heat on request along with the new target temperature directly on user request. Let me know if it looks okay!

@ankushg

This comment has been minimized.

ankushg commented Oct 15, 2018

Hi! Is there anything blocking this from getting merged?

Was looking into writing something similar myself when I saw this PR, but it looks like it hasn’t been updated for a couple months…

@Oskoss

This comment has been minimized.

Oskoss commented Oct 21, 2018

Wondering the same as above...Don't mind pitching in to help here, but seems like tests coverage is up, the requested changes have been explained documentation is ready, and there are no merge conflicts. Please let us know what can be done!

@MartinHjelmare MartinHjelmare changed the title from Added support for Dyson Hot+Cool Fan as a climate device to Add support for Dyson Hot+Cool Fan as a climate device Oct 21, 2018

@MartinHjelmare MartinHjelmare merged commit b6d3a19 into home-assistant:dev Oct 21, 2018

5 checks passed

Hound No violations found. Woof!
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 increased (+0.01%) to 93.778%
Details

@wafflebot wafflebot bot removed the almost-done label Oct 21, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment