Xiaomi Ceiling Lamp: Some refactoring and fault tolerance if a philips light ball is used #45
Conversation
mirobo/ceil.py
Outdated
| else: | ||
| _LOGGER.error("Count (%s) of requested properties does not " | ||
| "match the count (%s) of received values.", | ||
| properties_count, values_count) |
There was a problem hiding this comment.
continuation line under-indented for visual indent
mirobo/ceil.py
Outdated
| values.extend((None, None)) | ||
| else: | ||
| _LOGGER.error("Count (%s) of requested properties does not " | ||
| "match the count (%s) of received values.", |
There was a problem hiding this comment.
continuation line under-indented for visual indent
mirobo/ceil.py
Outdated
| _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) |
There was a problem hiding this comment.
continuation line under-indented for visual indent
mirobo/ceil.py
Outdated
| 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.", |
There was a problem hiding this comment.
continuation line under-indented for visual indent
mirobo/ceil.py
Outdated
| 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 " |
There was a problem hiding this comment.
continuation line under-indented for visual indent
mirobo/ceil.py
Outdated
| values_count = len(values) | ||
|
|
||
|
|
||
| if properties_count != values_count: |
mirobo/ceil.py
Outdated
| "provide the property bl and ac.", | ||
| properties_count, values_count) | ||
|
|
||
| values.extend((None, None)) |
There was a problem hiding this comment.
I admit inheritance of two devices (led ball and ceiling lamp) would be the better approach.
There was a problem hiding this comment.
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?
| 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"] |
There was a problem hiding this comment.
what could dv mean? It would be nice to have a real name for it too.
There was a problem hiding this comment.
Unfortunately I don't know the meaning. Google didn't help as well.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I just own the the Philips LED Ball. This device just provides the first 5 properties. Maybe @kuduka can provide some more informations.
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
@kuduka do you also have descriptions for the rest of the undocumented properties?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
'mb', 'ms', 'cctsw' and 'sw' are shown in the example query, but are not currently fetched and there are no property getters for them.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :-)
mirobo/ceil.py
Outdated
| "provide the property bl and ac.", | ||
| properties_count, values_count) | ||
|
|
||
| values.extend((None, None)) |
There was a problem hiding this comment.
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?
mirobo/ceil_cli.py
Outdated
| 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) |
mirobo/ceil_cli.py
Outdated
| click.echo("Scene Number: %s" % res.snm) | ||
| click.echo("Brightness: %s" % res.brightness) | ||
| click.echo("Correlated color temperatur: %s" % res.color_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) |
There was a problem hiding this comment.
Could you also rename ac and bl, maybe auto(matic)_color_temperature and smart_midnight_mode or similar.
| """Set CCT level.""" | ||
| click.echo("CCT level: %s" % dev.set_cct(level)) | ||
| click.echo("Correlated color temperatur level: %s" % |
There was a problem hiding this comment.
temperature. Any ideas what "correlated" means and does it bring anything for the user what simply calling it "color temperature" would not?
There was a problem hiding this comment.
I removed "correlated". It won't help anybody and isn't correct because a percentage is delivered.
Fixes #43.