Xiaomi MiIO Fan: Xiaomi Air Humidifier integration by syssi · Pull Request #12627 · home-assistant/core

syssi · 2018-02-23T16:46:12Z

Description:

Device support for the Xiaomi Air Humidifier.

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

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

fan:
  - platform: xiaomi_miio
    name: Xiaomi Air Humidifier
    host: 192.168.130.72
    token: 2b00042f7481c7b056c4b410d28f33cf
    model: zhimi.humidifier.v1

houndci-bot · 2018-02-23T16:46:19Z

homeassistant/components/fan/xiaomi_miio.py

continuation line under-indented for visual indent

houndci-bot · 2018-02-23T16:46:20Z

homeassistant/components/fan/xiaomi_miio.py

continuation line under-indented for visual indent

houndci-bot · 2018-02-23T16:46:20Z

homeassistant/components/fan/xiaomi_miio.py

continuation line under-indented for visual indent

houndci-bot · 2018-02-23T16:46:20Z

homeassistant/components/fan/xiaomi_miio.py

continuation line under-indented for visual indent

houndci-bot · 2018-02-23T16:46:20Z

homeassistant/components/fan/xiaomi_miio.py

continuation line under-indented for visual indent

houndci-bot · 2018-02-23T16:46:20Z

homeassistant/components/fan/xiaomi_miio.py

continuation line under-indented for visual indent

houndci-bot · 2018-02-23T19:55:22Z

homeassistant/components/fan/xiaomi_miio.py

line too long (88 > 79 characters)

houndci-bot · 2018-02-23T19:55:23Z

homeassistant/components/fan/xiaomi_miio.py

line too long (105 > 79 characters)

houndci-bot · 2018-02-23T19:55:23Z

homeassistant/components/fan/xiaomi_miio.py

line too long (104 > 79 characters)

houndci-bot · 2018-02-23T19:55:23Z

homeassistant/components/fan/xiaomi_miio.py

line too long (104 > 79 characters)

…here.

rytilahti

From a quick review it looks good! I left a couple of comments how it could be improved.

rytilahti · 2018-02-27T20:03:12Z

homeassistant/components/fan/services.yaml

+      description: Name of the xiaomi miio entity.
+      example: 'fan.xiaomi_miio_device'
+    humidity:
+      description: 	Target humidity. Allowed values are 30, 40, 50, 60, 70 and 80.


Unnecessary whitespace.

rytilahti · 2018-02-27T20:03:41Z

homeassistant/components/fan/services.yaml

+      example: 'fan.xiaomi_miio_device'
+
+xiaomi_miio_set_extra_features:
+  description: Set the extra features.


This could be more elaborate, what are the extra features and what are the known values below?

rytilahti · 2018-02-27T20:04:23Z

homeassistant/components/fan/services.yaml

      example: 1
+
+xiaomi_miio_set_auto_detect_on:
+  description: Turn the auto detect on.


Auto detect and brightness are relevant?

rytilahti · 2018-02-27T20:05:53Z

homeassistant/components/fan/xiaomi_miio.py

+
+SERVICE_SCHEMA_EXTRA_FEATURES = AIRPURIFIER_SERVICE_SCHEMA.extend({
+    vol.Required(ATTR_FEATURES):
+        vol.All(vol.Coerce(int), vol.Range(min=0))


Does range work when only the min value is given?

Yes. Clamp wouldn't work.

rytilahti · 2018-02-27T20:07:18Z

homeassistant/components/fan/xiaomi_miio.py

+    else:
+        _LOGGER.error(
+            'Unsupported device found! Please create an issue at '
+            'https://github.com/rytilahti/python.miio/issues '


Typo in the URL, should this be reported to homeassistant though (considering that the backend library may already support the device)? Just a thought though, this is fine for me either way.

It would be a feature request. That's why I don't want to mention homeassistant here. I added a link to my development repository now. So we can keep the python-miio repo clean for protocol issues.

rytilahti · 2018-02-27T20:09:38Z

homeassistant/components/fan/xiaomi_miio.py

+            ATTR_TURBO_MODE_SUPPORTED: None,
+            ATTR_SLEEP_MODE: None,
+            ATTR_AUTO_DETECT: None,
+        })


Would it make sense to have a list of these attrs and simply do _state_attrs = {attr: None for attr in available_attributes} and set the model attribute afterwards?

rytilahti · 2018-02-27T20:11:03Z

homeassistant/components/fan/xiaomi_miio.py

+                ATTR_EXTRA_FEATURES: state.extra_features,
+                ATTR_TURBO_MODE_SUPPORTED: state.turbo_mode_supported,
+                ATTR_AUTO_DETECT: state.auto_detect,
+            })


The same idea that I elaborated above could be use here too, maybe the available_attributes could be a map of (attribute_name, attribute_getter)` or similar?

Cool! I will care about.

rytilahti · 2018-02-27T20:11:48Z

homeassistant/components/fan/xiaomi_miio.py

+            return
+
+        yield from self._try_command(
+            "Turning the learn mode of the miio device off failed.",


Auto detect versus learn mode.

Attribute "volume" added.

rytilahti · 2018-03-05T06:44:26Z

homeassistant/components/fan/xiaomi_miio.py

+    ATTR_BUZZER: 'buzzer',
+    ATTR_LED_BRIGHTNESS: 'led_brightness',
+    ATTR_SLEEP_MODE: 'sleep_mode',
+})


Considering minimum python version being 3.5 now, I think the following would also work to simplify it a bit more:

AVAILABLE_ATTRIBUTES_AIRPURIFIER = {**AVAILABLE_ATTRIBUTES_AIRPURIFIER_COMMON, ATTR_BUZZER: 'buzzer', }

Determine support features and attributes at the constructor. Use starred expressions at dicts instead of copies.

houndci-bot · 2018-03-05T07:03:53Z

homeassistant/components/fan/xiaomi_miio.py

+                                      AVAILABLE_ATTRIBUTES_AIRPURIFIER})
+
+
+    @property


too many blank lines (2)

houndci-bot · 2018-03-16T08:59:48Z

homeassistant/components/fan/xiaomi_miio.py

-            ATTR_PURIFY_VOLUME: None,
+            ATTR_MODEL: self._model,
        }
+        self._device_features = DEVICE_FEATURE_FLAGS_GENERIC


undefined name 'DEVICE_FEATURE_FLAGS_GENERIC'

rytilahti

Looks great! I added some more comments, other than that I feel this can be merged. Maybe someone else could also do a quick review on this?

Generally speaking it is visible here that the amount of boilerplate required to expose extra services will become a pain to maintain in the future...

rytilahti · 2018-03-19T21:26:18Z

homeassistant/components/fan/xiaomi_miio.py

-        """Call an air purifier command handling error messages."""
+    @staticmethod
+    def _extract_value_from_attribute(state, attribute):
+        from enum import Enum


Normal imports (not related to backend libraries) should be done on top of the file.

rytilahti · 2018-03-19T21:30:27Z

homeassistant/components/fan/xiaomi_miio.py

+            return [mode.name for mode in OperationMode if mode.name != 'Auto']
+
+        # Air Humidifier CA
+        return [mode.name for mode in OperationMode]


Is this also valid for non-CA humidifiers? If yes, the comment should be updated.

I'm using the V1 as new default now. The CA is a very specific device version. My approach was to provide all known operation modes to reduce it later on for specific devices.

rytilahti · 2018-03-19T21:33:01Z

homeassistant/components/fan/xiaomi_miio.py

+                self._state_attrs.update(
+                    {key: self._extract_value_from_attribute(state, value)
+                     for key, value in
+                     AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER.items()})


This looks like repetition from above (in __init__), I think it'd make sense to have a self._available_attrs or something initialized in there.

Good catch!

rytilahti · 2018-03-19T21:33:40Z

homeassistant/components/fan/xiaomi_miio.py


        if result:
            self._state = True
            self._skip_update = True


Worth adding a comment wrt _skip_update here also?

The skip_update behavior is commented at the async_update already. I don't want to introduce a second comment here.

…rd version doesn't

This reverts commit 40b443e.

rytilahti · 2018-03-24T22:04:36Z

As there has been no further requests for changes, let's get this merged for the next release 👍

syssi requested a review from rytilahti as a code owner February 23, 2018 16:46

homeassistant added platform: fan.xiaomi_miio cla-signed labels Feb 23, 2018

houndci-bot reviewed Feb 23, 2018

View reviewed changes

syssi added 5 commits February 24, 2018 08:57

Device support for the Xiaomi Air Humidifier.

512a967

Requirements updated.

ca94512

"continuation line under-indented for visual indent" fixed.

13c70ab

Make hound happy.

5671446

Inadvertently added light.xiaomi_miio component removed from PR.

be70837

syssi force-pushed the feature/xiaomi-miio-airhumidifier branch from 0be7d02 to be70837 Compare February 24, 2018 08:00

homeassistant added the cla-signed label Feb 24, 2018

syssi added 5 commits February 24, 2018 09:04

Service descriptions added.

c139e22

One of the pylint errors fixed.

2d692d6

Redundancy removed.

4177c7c

pylint: disable=no-self-use added. The method signature is important …

a92f68e

…here.

Pylint fixed.

04096cd

syssi changed the title ~~Xiaomi MiIO Fan: Device support for the Xiaomi Air Humidifier~~ Xiaomi MiIO Fan: Xiaomi Air Humidifier integration Feb 26, 2018

Use a unique data key per domain.

dbba32f

homeassistant added the cla-signed label Feb 27, 2018

rytilahti reviewed Feb 27, 2018

View reviewed changes

syssi added 2 commits February 28, 2018 08:26

Review incorporated.

ff25070

Map of available attributes added.

a6421b9

syssi mentioned this pull request Mar 4, 2018

Xiaomi MiIO Fan: Xiaomi Air Humidifier integration home-assistant/home-assistant.io#4829

Merged

Pylint fixed.

c194182

Attribute "volume" added.

rytilahti reviewed Mar 5, 2018

View reviewed changes

Don't use the support flag bit mask as model identifier.

6701b72

Determine support features and attributes at the constructor. Use starred expressions at dicts instead of copies.

houndci-bot reviewed Mar 5, 2018

View reviewed changes

homeassistant/components/fan/xiaomi_miio.py Outdated

AVAILABLE_ATTRIBUTES_AIRPURIFIER})

@property

Copy link
Copy Markdown

houndci-bot Mar 5, 2018

Choose a reason for hiding this comment

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

too many blank lines (2)

syssi added 2 commits March 5, 2018 08:16

Blank line removed.

e34e9cb

Use Async / await syntax.

0a7c69e

syssi requested a review from andrey-git as a code owner March 10, 2018 17:01

homeassistant added the cla-signed label Mar 10, 2018

syssi and others added 7 commits March 10, 2018 21:09

Clean-up.

319abc9

Additional supported features refactoring completed.

865ee3e

Merge branch 'dev' into feature/xiaomi-miio-airhumidifier

bdf7ea1

Additional supported features renamed properly.

dbb03e1

Unique id added.

967f8ad

Device unavailable handling improved.

d366411

Refactoring.

4703e15

houndci-bot reviewed Mar 16, 2018

View reviewed changes

Missed const updated.

95676af

syssi changed the title ~~Xiaomi MiIO Fan: Xiaomi Air Humidifier integration~~ [WIP] Xiaomi MiIO Fan: Xiaomi Air Humidifier integration Mar 18, 2018

Incomplete Air Humidifier CA support fixed.

c17482b

homeassistant added the cla-signed label Mar 19, 2018

syssi changed the title ~~[WIP] Xiaomi MiIO Fan: Xiaomi Air Humidifier integration~~ Xiaomi MiIO Fan: Xiaomi Air Humidifier integration Mar 19, 2018

rytilahti approved these changes Mar 19, 2018

View reviewed changes

Review incorporated

37ab4a5

homeassistant added the cla-signed label Mar 20, 2018

syssi added 6 commits March 21, 2018 08:27

The Air Humidifier CA supports the operation mode "auto" - the standa…

4e393ba

…rd version doesn't

Attributes are part of the common set already

40b443e

Revert "Attributes are part of the common set already"

6660c54

This reverts commit 40b443e.

Comment added

378f5ac

Service description of the set_dry_{on,off} service added

53511cb

Typo fixed

099645d

rytilahti merged commit e36f27d into home-assistant:dev Mar 24, 2018

balloob mentioned this pull request Apr 13, 2018

0.67.0 #13856

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: xiaomi_miio and removed platform: fan.xiaomi_miio labels Mar 21, 2019

Uh oh!

Conversation

syssi commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rytilahti commented Mar 24, 2018

Uh oh!

Reviewers

syssi commented Feb 23, 2018 •

edited

Loading

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