Skip to content
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.

Sign up for GitHub

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

Xiaomi Ceiling Lamp: Some refactoring and fault tolerance if a philips light ball is used #45

Merged
merged 6 commits into from
Aug 30, 2017
Merged

Xiaomi Ceiling Lamp: Some refactoring and fault tolerance if a philips light ball is used #45

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
edcd9e4
Some refactoring and fault tolerance if a philips light ball is used.
syssi Aug 29, 2017
a77a06a
Stick to the same naming.
syssi Aug 29, 2017
0f7dd10
Code reformatted.
syssi Aug 29, 2017
15717c6
Debug statement removed.
syssi Aug 29, 2017
06f090a
Suggested improvements added.
syssi Aug 30, 2017
91a9acd
Some missing parts.
syssi Aug 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions mirobo/ceil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import logging
from .device import Device
from typing import Any, Dict

_LOGGER = logging.getLogger(__name__)

class CeilStatus:
"""Container for status reports from Xiaomi Philips LED Ceiling Lamp"""

Expand All @@ -20,19 +23,19 @@ def is_on(self) -> bool:
return self.power == "on"

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kuduka kuduka Aug 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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}

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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


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

@property
Expand All @@ -44,9 +47,11 @@ def ac(self) -> int:
return self.data["ac"]

def __str__(self) -> str:
s = "<CeilStatus power=%s, bright=%s, cct=%s, snm=%s, dv=%s, " \
s = "<CeilStatus power=%s, brightness=%s, " \
"color_temperature=%s, scene=%s, dv=%s, " \
"bl=%s, ac=%, >" % \
(self.power, self.bright, self.cct, self.snm, self.dv,
(self.power, self.brightness,
self.color_temperature, self.scene, self.dv,
self.bl, self.ac)
return s

Expand All @@ -65,11 +70,11 @@ def off(self):
"""Power off."""
return self.send("set_power", ["off"])

def set_bright(self, level: int):
def set_brightness(self, level: int):
"""Set brightness level."""
return self.send("set_bright", [level])

def set_cct(self, level: int):
def set_color_temperature(self, level: int):
"""Set Correlated Color Temperature."""
return self.send("set_cct", [level])

Expand Down Expand Up @@ -104,4 +109,22 @@ def status(self) -> CeilStatus:
"get_prop",
properties
)
properties_count = len(properties)
values_count = len(values)
if properties_count != values_count:
Copy link

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)

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.",
properties_count, values_count)
Copy link

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent


values.extend((None, None))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent


print(values)
return CeilStatus(dict(zip(properties, values)))
21 changes: 11 additions & 10 deletions mirobo/ceil_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
pass_dev = click.make_pass_decorator(mirobo.Ceil)


def validate_bright(ctx, param, value):
def validate_percentage(ctx, param, value):
value = int(value)
if value < 1 or value > 100:
raise click.BadParameter('Should be a positive int between 1-100.')
Expand Down Expand Up @@ -97,9 +97,9 @@ def status(dev: mirobo.Ceil):
return # bail out

click.echo(click.style("Power: %s" % res.power, bold=True))
click.echo("Brightness: %s" % res.bright)
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)
Copy link
Owner

Choose a reason for hiding this comment

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

temperature :-)

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)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Expand All @@ -120,19 +120,20 @@ def off(dev: mirobo.Ceil):


@cli.command()
@click.argument('level', callback=validate_bright, required=True,)
@click.argument('level', callback=validate_percentage, required=True,)
@pass_dev
def set_bright(dev: mirobo.Ceil, level):
def set_brightness(dev: mirobo.Ceil, level):
"""Set brightness level."""
click.echo("Brightness: %s" % dev.set_bright(level))
click.echo("Brightness: %s" % dev.set_brightness(level))


@cli.command()
@click.argument('level', callback=validate_bright, required=True,)
@click.argument('level', callback=validate_percentage, required=True,)
@pass_dev
def set_cct(dev: mirobo.Ceil, level):
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" %
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

dev.set_color_temperature(level))


@cli.command()
Expand Down