Add support to Dyson 360 Eye robot vacuum using new vacuum platform #8852

CharlesBlonde · 2017-08-05T20:02:08Z

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:

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.

MartinHjelmare

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

MartinHjelmare · 2017-08-05T22:35:41Z

homeassistant/components/vacuum/dyson.py

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


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

MartinHjelmare · 2017-08-05T22:40:28Z

homeassistant/components/vacuum/dyson.py

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


return dyson_labels.get(self._device.state.state, self._device.state.state)

MartinHjelmare · 2017-08-05T22:54:59Z

homeassistant/components/dyson.py

-                    _LOGGER.warning("Unable to connect to device %s",
-                                    dyson_device)
+                try:
+                    connected = dyson_device.connect(device["device_ip"])


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?

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 · 2017-08-05T23:26:53Z

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

MartinHjelmare · 2017-08-06T09:26:15Z

tests/components/vacuum/test_dyson.py

+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 should go above home assistant internal imports separated with a blank line.
https://www.python.org/dev/peps/pep-0008/#imports

MartinHjelmare · 2017-08-06T09:32:53Z

homeassistant/components/vacuum/dyson.py

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


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

MartinHjelmare · 2017-08-06T09:57:03Z

tests/components/vacuum/test_dyson.py

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


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.

CharlesBlonde · 2017-08-06T10:31:12Z

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.

MartinHjelmare · 2017-08-06T10:40:11Z

homeassistant/components/vacuum/__init__.py

@@ -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 should not be here, so it's good to remove. It would hinder discovery as stated already.

Ok, perfect.

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

MartinHjelmare · 2017-08-06T10:43:13Z

homeassistant/components/vacuum/dyson.py

@@ -7,14 +7,14 @@
 import asyncio
 import logging

+from homeassistant.util.icon import icon_for_battery_level


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

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

azogue · 2017-08-06T11:06:14Z

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 · 2017-08-06T11:09:43Z

@CharlesBlonde Please link to a docs PR asap.

azogue · 2017-08-06T11:12:57Z

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 · 2017-08-06T21:19:17Z

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

…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 · 2017-09-25T03:51:59Z

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 · 2017-09-25T05:35:43Z

Please use the forum or chat for support questions.

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

homeassistant added cla-signed integration: dyson integration: vacuum new-platform platform: fan.dyson labels Aug 5, 2017

balloobbot added component platform requirement labels Aug 5, 2017

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

49615ff

CharlesBlonde force-pushed the dyson_vacuum branch from fcfe3b1 to 49615ff Compare August 5, 2017 20:59

homeassistant added the cla-signed label Aug 5, 2017

Fix tests with Python 3.5

e98bf0f

MartinHjelmare reviewed Aug 5, 2017

View reviewed changes

Code review

1ddf0b0

MartinHjelmare requested changes Aug 6, 2017

View reviewed changes

MartinHjelmare self-assigned this Aug 6, 2017

Code review - v2

8217f63

homeassistant added the cla-signed label Aug 6, 2017

MartinHjelmare reviewed Aug 6, 2017

View reviewed changes

MartinHjelmare requested changes Aug 6, 2017

View reviewed changes

Code review - v3

417c0ef

homeassistant added the cla-signed label Aug 6, 2017

MartinHjelmare approved these changes Aug 6, 2017

View reviewed changes

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

CharlesBlonde mentioned this pull request Aug 6, 2017

Add documentation for Dyson 360 Eye robot vacuum home-assistant/home-assistant.io#3148

Merged

fabaff mentioned this pull request Aug 12, 2017

0.51 #8919

Merged

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

ghost removed platform: fan.dyson labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to Dyson 360 Eye robot vacuum using new vacuum platform #8852

Add support to Dyson 360 Eye robot vacuum using new vacuum platform #8852

CharlesBlonde commented Aug 5, 2017 •

edited

Loading

MartinHjelmare left a comment

MartinHjelmare Aug 5, 2017

MartinHjelmare Aug 5, 2017

MartinHjelmare Aug 5, 2017 •

edited

Loading

CharlesBlonde Aug 5, 2017

CharlesBlonde commented Aug 5, 2017

MartinHjelmare left a comment

MartinHjelmare Aug 6, 2017

MartinHjelmare Aug 6, 2017

MartinHjelmare Aug 6, 2017

CharlesBlonde commented Aug 6, 2017

MartinHjelmare Aug 6, 2017 •

edited

Loading

CharlesBlonde Aug 6, 2017

azogue Aug 6, 2017

MartinHjelmare Aug 6, 2017

CharlesBlonde Aug 6, 2017

azogue commented Aug 6, 2017

MartinHjelmare commented Aug 6, 2017

azogue commented Aug 6, 2017

CharlesBlonde commented Aug 6, 2017

Au8ust commented Sep 25, 2017

MartinHjelmare commented Sep 25, 2017

Add support to Dyson 360 Eye robot vacuum using new vacuum platform #8852

Add support to Dyson 360 Eye robot vacuum using new vacuum platform #8852

Conversation

CharlesBlonde commented Aug 5, 2017 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesBlonde commented Aug 5, 2017

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesBlonde commented Aug 6, 2017

MartinHjelmare Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azogue commented Aug 6, 2017

MartinHjelmare commented Aug 6, 2017

azogue commented Aug 6, 2017

CharlesBlonde commented Aug 6, 2017

Au8ust commented Sep 25, 2017

MartinHjelmare commented Sep 25, 2017

CharlesBlonde commented Aug 5, 2017 •

edited

Loading

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

MartinHjelmare Aug 5, 2017 •

edited

Loading

MartinHjelmare Aug 6, 2017 •

edited

Loading