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 to Dyson 360 Eye robot vacuum using new vacuum platform #8852

Merged
merged 5 commits into from Aug 6, 2017

Conversation

@CharlesBlonde
Contributor

CharlesBlonde commented Aug 5, 2017

Description:

Add support to Dyson 360 Eye robot vacuum implementing the new vacuum platform added in PR #8623

@azogue You did an amazing job with this platform! In less than 2 hours, I had a first fully working Dyson implementation.

Nevertheless, I had to remove a test in the vacuum platform:

if not config[DOMAIN]:
  return False

Because I'm calling the setup programaticly, without adding any

vacuum:

I have to do that because I'm using the parent Dyson hub used also by fan/purifier. Do you see any issue by removing this test ?

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

Example entry for configuration.yaml (if applicable):

It's using the already existing Dyson hub configuration

dyson:
  username: <email>
  password: <password>
  language: <language_code>
  devices:
    - device_id: <device_id> # eg: Dyson fan
      device_ip: <device_ip>
    - device_id: <device_id> # eg: Dyson robot vacuum
      device_ip: <device_ip>

Checklist:

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.
@MartinHjelmare

I assume there's no I/O going on in the entity properties? Three comments. I didn't look at the tests.

class Dyson360EyeDevice(VacuumDevice):
"""Dyson 360 Eye robot vacuum device."""
def __init__(self, hass, device):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 5, 2017

Member

Don't pass in hass. It will be set on the entity when it has been added to home assistant.

Dyson360EyeMode.FULL_CLEAN_FINISHED: "Finished",
Dyson360EyeMode.FULL_CLEAN_NEEDS_CHARGE: "Need charging"
}
if self._device.state.state in dyson_labels:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 5, 2017

Member
return dyson_labels.get(self._device.state.state, self._device.state.state)
_LOGGER.warning("Unable to connect to device %s",
dyson_device)
try:
connected = dyson_device.connect(device["device_ip"])

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 5, 2017

Member

What's the difference between this part of the connection logic and the one at line 92-97? Would it be possible to make a connect function that is called wherever it is needed?

This comment has been minimized.

@CharlesBlonde

CharlesBlonde Aug 5, 2017

Contributor

The first block is for manual connection (specifying device id/ip). The second one try to use mDNS protocol to discover fan/purifier devices (not working with robot vacuum yet). https://home-assistant.io/components/dyson/
It was the same method before but it was quite painful to use so I updated the underlying library and create 2 different methods.

@CharlesBlonde

This comment has been minimized.

Contributor

CharlesBlonde commented Aug 5, 2017

Thanks for your review.
I confirm there is no IO in the entity properties. Values are only updated with MQTT events and stored in self._device object.

@MartinHjelmare

Fix the two import comments and I think this is good to go. 👍

from homeassistant.components.vacuum import dyson
from homeassistant.components.vacuum.dyson import Dyson360EyeDevice
from tests.common import get_test_home_assistant
from libpurecoollink.dyson_360_eye import Dyson360Eye

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 6, 2017

Member

This should go above home assistant internal imports separated with a blank line.
https://www.python.org/dev/peps/pep-0008/#imports

SUPPORT_TURN_OFF, SUPPORT_TURN_ON,
VacuumDevice)
from homeassistant.util.icon import icon_for_battery_level
from homeassistant.components.dyson import DYSON_DEVICES

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 6, 2017

Member

Put this line above from homeassistant.components.vacuum import ..., ie 🔡

"""Test setup component with no devices."""
self.hass.data[dyson.DYSON_DEVICES] = []
add_devices = mock.MagicMock()
dyson.setup_platform(self.hass, None, add_devices)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 6, 2017

Member

It might not matter for this test but the config argument, the second argument, will always be at least an empty dict {}. When setting up a platform via discovery it will always be an empty dict.

@MartinHjelmare MartinHjelmare self-assigned this Aug 6, 2017

@CharlesBlonde

This comment has been minimized.

Contributor

CharlesBlonde commented Aug 6, 2017

I just updated the import orders (and also use an empty dict instead of none, it's better). Tell me if it's OK for you.

But before merging, I would like to have @azogue point of view because I updated the vacuum platform and I'm unable to check if it can break something. I don't think so but I can't be sure.

@@ -168,9 +168,6 @@ def send_command(hass, command, params=None, entity_id=None):
@asyncio.coroutine
def async_setup(hass, config):
"""Set up the vacuum component."""
if not config[DOMAIN]:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 6, 2017

Member

This should not be here, so it's good to remove. It would hinder discovery as stated already.

This comment has been minimized.

@CharlesBlonde

CharlesBlonde Aug 6, 2017

Contributor

Ok, perfect.

This comment has been minimized.

@azogue

azogue Aug 6, 2017

Contributor

Yeah, I think I copied it from some other component, but I'm not sure why exactly, so better out...

@@ -7,14 +7,14 @@
import asyncio
import logging
from homeassistant.util.icon import icon_for_battery_level

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 6, 2017

Member

No, please put this back below. Imports should be alphabetically sorted but following the greater rules of PEP8 for imports.

This comment has been minimized.

@CharlesBlonde

CharlesBlonde Aug 6, 2017

Contributor

Ok, this time it should be fine. Next time I'll try to do it in the first commit :)

@azogue

This comment has been minimized.

Contributor

azogue commented Aug 6, 2017

Hi @CharlesBlonde, congrats for this PR!

It is great to see how the community works and how HA grows better and better each release!

@MartinHjelmare MartinHjelmare merged commit 83afd12 into home-assistant:dev Aug 6, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 93.787%
Details
hound No violations found. Woof!
@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Aug 6, 2017

@CharlesBlonde Please link to a docs PR asap.

@azogue

This comment has been minimized.

Contributor

azogue commented Aug 6, 2017

Yes, to include all three vacuum cleaners in the next release! (0.50.3 or 0.51.0)

... suggesting subtitle alias for the release: The botvac revolution hehehe ...

@CharlesBlonde

This comment has been minimized.

Contributor

CharlesBlonde commented Aug 6, 2017

I just submitted the PR for the documentation (home-assistant/home-assistant.io#3148).
I added a comment on the ha_category

Ready for release 0.51.0 :)

@fabaff fabaff referenced this pull request Aug 12, 2017

Merged

0.51 #8919

dethpickle added a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017

Add support to Dyson 360 Eye robot vacuum using new vacuum platform (h…
…ome-assistant#8852)

* Add support to Dyson 360 Eye robot vacuum using new vacuum platform

* Fix tests with Python 3.5

* Code review

* Code review - v2

* Code review - v3
@Au8ust

This comment has been minimized.

Au8ust commented Sep 25, 2017

I try to use Dyson hub to add my 360 eye. But I am not sure which should be the device ID. I only see serial number in my settings page. I put it in the device ID but logs shows can't find the device. Could you please advice on?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 25, 2017

Please use the forum or chat for support questions.

https://home-assistant.io/help/#communication-channels

@home-assistant home-assistant locked and limited conversation to collaborators Sep 25, 2017

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