Refactor yeelight code #22547

zewelor · 2019-03-29T20:41:22Z

Breaking changes:

When starting HA, and some light is not available and doesn't have model declared. It won't be added until properties can be fetched. When model is declared in config, it will be added as before.

Description:

Split yeelight classes, for logic simplification, some nasty nested ifs are removed. Its for easier handling future component improvements. It fixes bugs when yeelights are exported, for example to homekit, before correct supported_features are set ( on first update from device ). If it gets exported earlier, light gets marked as brightness only. It fixes #24018 and #23871.

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

homeassistant/components/yeelight/light.py

homeassistant/components/yeelight/__init__.py

homeassistant/components/yeelight/light.py

homeassistant/components/yeelight/__init__.py

zewelor · 2019-04-07T10:28:47Z

Please don't merge before 0.93

codecov · 2019-04-08T06:36:11Z

Codecov Report

Merging #22547 into dev will decrease coverage by 0.29%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              dev   #22547     +/-   ##
=========================================
- Coverage   94.13%   93.83%   -0.3%     
=========================================
  Files         502      448     -54     
  Lines       39349    36528   -2821     
=========================================
- Hits        37040    34275   -2765     
+ Misses       2309     2253     -56

Impacted Files	Coverage Δ
homeassistant/components/owntracks/config_flow.py	`44.82% <0%> (-41.66%)`	⬇️
homeassistant/components/mqtt/fan.py	`74.33% <0%> (-23.64%)`	⬇️
homeassistant/components/mqtt/light/schema_json.py	`73.06% <0%> (-20.65%)`	⬇️
homeassistant/bootstrap.py	`58.08% <0%> (-17.21%)`	⬇️
homeassistant/components/config/script.py	`90% <0%> (-10%)`	⬇️
homeassistant/components/mqtt/lock.py	`91.83% <0%> (-8.17%)`	⬇️
homeassistant/components/axis/device.py	`93.02% <0%> (-6.98%)`	⬇️
homeassistant/components/hassio/ingress.py	`71.3% <0%> (-6.75%)`	⬇️
homeassistant/helpers/template.py	`95.54% <0%> (-4.46%)`	⬇️
homeassistant/components/html5/notify.py	`85.6% <0%> (-2.43%)`	⬇️
... and 264 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c928f82...2c24b9e. Read the comment docs.

zewelor · 2019-05-21T18:16:26Z

@rytilahti Seeing that you are active now, maybe you could take a look here ? Main idea was to simplify code and get rid of nasty ifs in update method, also be sure that all devices has defined supported features, from the start. It needs rebase, I will do it, if there is any hope to push this forward.

rytilahti · 2019-05-22T02:48:12Z

@zewelor I'll try to do a code review asap, unfortunately I won't have access to any bulbs until later this year, so I hope someone else chimes in to test the functionality.

zewelor · 2019-06-02T17:16:24Z

Looks like it fixes some issues with homekit and yeelight, #24018 (comment)

rytilahti

Sorry for taking my time, I added some comments and I think this can be merged after those are handled.

rytilahti · 2019-06-02T22:34:55Z

homeassistant/components/yeelight/light.py

+
+    # F821: https://github.com/PyCQA/pyflakes/issues/373
+    @property
+    def _bulb(self) -> 'yeelight.Bulb':  # noqa: F821


As imports can be done on the top level, this could be converted to simply Bulb, right?

rytilahti · 2019-06-02T22:35:44Z

homeassistant/components/yeelight/light.py

+        return self.device.bulb
+
+    @property
+    def _properties(self) -> dict:


I don't think dict is a correct type hint, is it? Or is using Dict from typing only necessary if you want to describe the contained types?

I believe dict type hint was introducend here by you rytilahti@cf5d9ce#diff-1395165791bf7671502a6e2e7bd7d4cbR173 . I'm not sure whats best here. Whats wrong with dict ? Maybe we can skip type hint ?

I'm not sure if there's something wrong with just using dict, the typing module has Dict, but maybe it is just for those cases where you want to define types of its keys and values. So I suppose this is fine..

homeassistant/components/yeelight/light.py

rytilahti · 2019-06-02T22:40:13Z

homeassistant/components/yeelight/light.py

+    def _lights_setup_helper(klass):
+        lights.append(klass(device, custom_effects=custom_effects))
+
+    if model in ('mono', 'mono1') or device.type == BulbType.White:


I don't really like mixing model and device.type comparison like this, as it makes reading hard. But it's not a showstopper.

Good point. I will work on splitting it into 2 methods, when model is declared or fetched via device.type

Its simplified now. Please take a look

rytilahti · 2019-06-02T22:41:27Z

homeassistant/components/yeelight/__init__.py

+
+    def setup(self):
+        """Initialize device and send initialized signal."""
+        if self._update_properties() or self.model:


Should the one above also check for self.model like this?

I'm not sure which one ? Can you link to code line ?

Line 289, in update(). I think this code structure is a bit confusing and could be made easier to follow by restructuring it somehow.

Model was only needed in setup, when update fails, but we have model declared, we can build correct Yeelight class. I've simplified this code, I hope it easier to follow now.

rytilahti · 2019-06-02T22:42:42Z

homeassistant/components/yeelight/__init__.py

    @property
    def is_nightlight_enabled(self) -> bool:
        """Return true / false if nightlight is currently enabled."""
        if self.bulb is None:
            return False

-        return self.bulb.last_properties.get('active_mode') == '1'
+        return self._active_mode == '1'


Maybe define ACTIVE_MODE_NIGHTLIGHT = '1' and use the constant here for readability. What are the other possible modes and should they also be handled somehow?

From yeelight docs:

0: daylight mode / 1: moonlight mode (ceiling light only) . I don't think we need to handle anything more now. Added suggested const.

Co-Authored-By: Teemu R. <[email protected]>

rytilahti

Looks much cleaner now, thanks! I added a very minor nitpick; you used a multi line comment for a single line commentary. Other than that let's get this merged :-)

rytilahti · 2019-06-12T02:30:34Z

homeassistant/components/yeelight/__init__.py

+        initial_update = self._update_properties()
+
+        if not initial_update and self.model:
+            """We can build correct class anyway."""


Use single line comment (#) here, other than that this looks good to go from my side :-)

Right, even lint complained about it. Fixed

zewelor · 2019-06-12T08:13:32Z

Great, thanks for review.

rytilahti · 2019-06-12T23:35:17Z

This fixes both #24018 and #23871? Are there other known issues this is fixing (just that they can be linked to the description).

zewelor · 2019-06-13T06:42:38Z

I've linked them from the description. Not that I'm aware of.

rytilahti · 2019-06-13T16:42:40Z

Great, let's get this merged 🎉

* Separate yeelight light classes * Removed not used variable * Allow to create device right away, when model is declared * Lint fixes * Use correct brightness, when nightlight mode is on * Pylint fix * Add power property * Fix imports * Update homeassistant/components/yeelight/light.py Co-Authored-By: Teemu R. <[email protected]> * Small PR fixes * Simplify device to yeelight class mapping * Simplify device initialization code * Fix comment

homeassistant added cla-signed integration: yeelight labels Mar 29, 2019

ghost added the in progress label Mar 29, 2019

houndci-bot reviewed Mar 29, 2019

View reviewed changes

homeassistant/components/yeelight/__init__.py Outdated Show resolved Hide resolved

houndci-bot reviewed Mar 29, 2019

View reviewed changes

homeassistant/components/yeelight/light.py Outdated Show resolved Hide resolved

houndci-bot reviewed Mar 30, 2019

View reviewed changes

homeassistant/components/yeelight/__init__.py Outdated Show resolved Hide resolved

zewelor marked this pull request as ready for review April 6, 2019 08:15

zewelor requested a review from rytilahti as a code owner April 6, 2019 08:15

syssi added cla-recheck and removed cla-signed labels Apr 7, 2019

homeassistant added cla-signed and removed cla-recheck labels Apr 7, 2019

zewelor mentioned this pull request Apr 10, 2019

Whitelist yeelight predefined effects per device type #22979

Closed

3 tasks

zewelor added 8 commits May 22, 2019 10:49

Separate yeelight light classes

960d777

Removed not used variable

0d6a9a5

Allow to create device right away, when model is declared

86c419f

Lint fixes

8a5f081

Use correct brightness, when nightlight mode is on

05ea47f

Pylint fix

7d6fd9b

Add power property

e2846b8

Fix imports

2c24b9e

rytilahti reviewed Jun 2, 2019

View reviewed changes

zewelor and others added 2 commits June 4, 2019 21:23

Update homeassistant/components/yeelight/light.py

7e1eb64

Co-Authored-By: Teemu R. <[email protected]>

Small PR fixes

568b8b4

rytilahti mentioned this pull request Jun 8, 2019

Add yeelight nighlight support with separate light entity #22936

Closed

4 tasks

zewelor added 2 commits June 11, 2019 22:30

Simplify device to yeelight class mapping

cdd6de6

Simplify device initialization code

c40ebf5

rytilahti approved these changes Jun 12, 2019

View reviewed changes

Fix comment

abdde06

rytilahti merged commit 6d3c3ce into home-assistant:dev Jun 13, 2019

rytilahti mentioned this pull request Jun 13, 2019

Yeelight RGB bulb YLDP02YL only support white in homekit #23871

Closed

zewelor deleted the refactor_yeelight_code branch June 13, 2019 20:36

posavasos mentioned this pull request Jun 20, 2019

Yeelight tunable white (YLDP05YL): "Cannot determine device type" #24643

Closed

balloob mentioned this pull request Jun 26, 2019

0.95.0 #24776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor yeelight code #22547

Refactor yeelight code #22547

zewelor commented Mar 29, 2019 •

edited

Loading

zewelor commented Apr 7, 2019

codecov bot commented Apr 8, 2019 •

edited

Loading

zewelor commented May 21, 2019 •

edited

Loading

rytilahti commented May 22, 2019

zewelor commented Jun 2, 2019

rytilahti left a comment

rytilahti Jun 2, 2019

rytilahti Jun 2, 2019

zewelor Jun 4, 2019

rytilahti Jun 8, 2019

rytilahti Jun 2, 2019

zewelor Jun 4, 2019

zewelor Jun 11, 2019

rytilahti Jun 2, 2019

zewelor Jun 4, 2019

rytilahti Jun 8, 2019

zewelor Jun 11, 2019

rytilahti Jun 2, 2019

zewelor Jun 4, 2019

rytilahti left a comment

rytilahti Jun 12, 2019

zewelor Jun 12, 2019

zewelor commented Jun 12, 2019

rytilahti commented Jun 12, 2019

zewelor commented Jun 13, 2019

rytilahti commented Jun 13, 2019

Refactor yeelight code #22547

Refactor yeelight code #22547

Conversation

zewelor commented Mar 29, 2019 • edited Loading

Breaking changes:

Description:

Checklist:

zewelor commented Apr 7, 2019

codecov bot commented Apr 8, 2019 • edited Loading

Codecov Report

zewelor commented May 21, 2019 • edited Loading

rytilahti commented May 22, 2019

zewelor commented Jun 2, 2019

rytilahti 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

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

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

rytilahti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zewelor commented Jun 12, 2019

rytilahti commented Jun 12, 2019

zewelor commented Jun 13, 2019

rytilahti commented Jun 13, 2019

zewelor commented Mar 29, 2019 •

edited

Loading

codecov bot commented Apr 8, 2019 •

edited

Loading

zewelor commented May 21, 2019 •

edited

Loading