Xiaomi Ceiling Lamp: Some refactoring and fault tolerance if a philips light ball is used by syssi · Pull Request #45 · rytilahti/python-miio

syssi · 2017-08-29T20:18:20Z

Fixes #43.

houndci-bot · 2017-08-29T20:18:30Z

mirobo/ceil.py

+            else:
+                _LOGGER.error("Count (%s) of requested properties does not "
+                    "match the count (%s) of received values.",
+                    properties_count, values_count)


continuation line under-indented for visual indent

houndci-bot · 2017-08-29T20:18:30Z

mirobo/ceil.py

+                values.extend((None, None))
+            else:
+                _LOGGER.error("Count (%s) of requested properties does not "
+                    "match the count (%s) of received values.",


continuation line under-indented for visual indent

houndci-bot · 2017-08-29T20:18:30Z

mirobo/ceil.py

+                _LOGGER.info("The values of two properties are missing. "
+                    "Assumption: A Xiaomi Philips Light LED Ball is "
+                    "used which doesn't provide the property bl and ac.",
+                    properties_count, values_count)


continuation line under-indented for visual indent

houndci-bot · 2017-08-29T20:18:30Z

mirobo/ceil.py

+            if properties_count == values_count+2:
+                _LOGGER.info("The values of two properties are missing. "
+                    "Assumption: A Xiaomi Philips Light LED Ball is "
+                    "used which doesn't provide the property bl and ac.",


continuation line under-indented for visual indent

houndci-bot · 2017-08-29T20:18:30Z

mirobo/ceil.py

+        if properties_count != values_count:
+            if properties_count == values_count+2:
+                _LOGGER.info("The values of two properties are missing. "
+                    "Assumption: A Xiaomi Philips Light LED Ball is "


continuation line under-indented for visual indent

houndci-bot · 2017-08-29T20:18:30Z

mirobo/ceil.py

+        values_count = len(values)
+
+
+        if properties_count != values_count:


too many blank lines (2)

syssi · 2017-08-29T20:31:07Z

mirobo/ceil.py

+                    "provide the property bl and ac.",
+                    properties_count, values_count)
+
+                values.extend((None, None))


I admit inheritance of two devices (led ball and ceiling lamp) would be the better approach.

This feels way too complicated and unpythonic. How about using defaultdict just below

defaultdict(lambda: None, zip(properties, values))

and having a warning() or debug() here if the amount of returned values is different from what was asked?

rytilahti

Looks good in general, but I think the property handling should be done in a more straight forward manner.

rytilahti · 2017-08-29T23:02:17Z

mirobo/ceil.py


    @property
    def dv(self) -> int:
        return self.data["dv"]


what could dv mean? It would be nice to have a real name for it too.

Unfortunately I don't know the meaning. Google didn't help as well.

Fair enough, it can be corrected later on. Btw, looking at the list of properties (with the comment of limitation of 8), is any of those left out properties interesting to include considering that currently 7 properties is being fetched?

I just own the the Philips LED Ball. This device just provides the first 5 properties. Maybe @kuduka can provide some more informations.

dv property represents seconds remaining until turn off is activated.

0 means it's deactivated and it won't turn off itself.

At least in ceiling light maximum value is 6 hours.

### 192.168.1.219 => 192.168.1.227 (b8:27:eb:34:b0:bb => 34:ce:00:86:db:44) {"id":9056,"method":"delay_off","params":[21599]} ### 192.168.1.219 => 192.168.1.227 (b8:27:eb:34:b0:bb => 34:ce:00:86:db:44) {"id":9057,"method":"get_prop","params":["power","bright","snm","dv","cct","sw","bl","mb","ac","ms"]} ### 192.168.1.227 => 192.168.1.219 (34:ce:00:86:db:44 => b8:27:eb:34:b0:bb) {"result":["on",35,0,21597,30,[[0,3],[0,2],[0,1]],1,1,1,1],"id":9057}

@kuduka do you also have descriptions for the rest of the undocumented properties?

Which ones are left undocumented? I think that was the last one on this code.

From product side there are quite a few that are not included on this code related to miband/wall switch/remote controller pairing and its functionality.

'mb', 'ms', 'cctsw' and 'sw' are shown in the example query, but are not currently fetched and there are no property getters for them.

yes, those are related to miband/wall switch integration and they are not supported yet :)

I need to spend some time deciphering them and with current limitation of 8 props I don't know how to do it, see this --> #35 (comment)

Although some times using more than 8 props as shown on this previous PR works fine, so it's kind of a mystery this behavior.

Any ideas how I could query more than 8 props? Doing 2 calls and merging its results? That would be acceptable? Any other ideas out there?

Ahh, I had forgotten that, my fault.

I think it's just a limitation one has to live with, probably also related to https://gitlab.com/stavros/python-yeelight/issues/17 . The "developer mode" is just a wrapper to allow accessing the API over HTTP without a token, or so it seems at least.

Doing two calls could probably be fine, the question is more whether it is worth it or not. However, it would make sense to document those findings (and create corresponding properties which just do their thing / raise NotImplementedError where not possible) into the source code for anyone reading the code and wondering why those are not used :-)

rytilahti · 2017-08-29T23:04:55Z

mirobo/ceil.py

+                    "provide the property bl and ac.",
+                    properties_count, values_count)
+
+                values.extend((None, None))


This feels way too complicated and unpythonic. How about using defaultdict just below

defaultdict(lambda: None, zip(properties, values))

and having a warning() or debug() here if the amount of returned values is different from what was asked?

rytilahti · 2017-08-29T23:06:42Z

mirobo/ceil_cli.py

-    click.echo("CCT: %s" % res.cct)
-    click.echo("Scene Number: %s" % res.snm)
+    click.echo("Brightness: %s" % res.brightness)
+    click.echo("Correlated color temperatur: %s" % res.color_temperature)


temperature :-)

rytilahti · 2017-08-29T23:07:52Z

mirobo/ceil_cli.py

+    click.echo("Scene: %s" % res.scene)
    click.echo("dv: %s" % res.dv)
    click.echo("Smart Midnight Light: %s" % res.bl)
    click.echo("Auto CCT: %s" % res.ac)


Could you also rename ac and bl, maybe auto(matic)_color_temperature and smart_midnight_mode or similar.

rytilahti · 2017-08-29T23:08:26Z

mirobo/ceil_cli.py

+def set_color_temperature(dev: mirobo.Ceil, level):
    """Set CCT level."""
-    click.echo("CCT level: %s" % dev.set_cct(level))
+    click.echo("Correlated color temperatur level: %s" %


temperature. Any ideas what "correlated" means and does it bring anything for the user what simply calling it "color temperature" would not?

I removed "correlated". It won't help anybody and isn't correct because a percentage is delivered.

rytilahti

Looks good, let's get it merged!

Some refactoring and fault tolerance if a philips light ball is used.

edcd9e4

Fixes rytilahti#43.

houndci-bot reviewed Aug 29, 2017

View reviewed changes

syssi added 3 commits August 29, 2017 22:19

Stick to the same naming.

a77a06a

Code reformatted.

0f7dd10

Debug statement removed.

15717c6

syssi commented Aug 29, 2017

View reviewed changes

rytilahti reviewed Aug 29, 2017

View reviewed changes

syssi added 2 commits August 30, 2017 07:28

Suggested improvements added.

06f090a

Some missing parts.

91a9acd

rytilahti approved these changes Aug 30, 2017

View reviewed changes

rytilahti merged commit 516c692 into rytilahti:master Aug 30, 2017

		values_count = len(values)


		if properties_count != values_count:

Uh oh!

Conversation

syssi commented Aug 29, 2017

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!

kuduka Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kuduka Aug 30, 2017 •

edited

Loading