Yeelight spec files was added #1094

Kirmas · 2021-07-11T21:49:34Z

I add almost all function from the yeelight documentation except of set_scene, set_music, set_adjust, adjust_bright, adjust_ct, adjust_color :

for "adjust" functions it is not difficult to add but I really don`t understand why we need this.(maybe if there is some free time before someone looks, I will add)
for set_scene - I dont understand how it must work. What is the best parameters set, maybe I must to add one more .yaml file with scenes?
for set_music - How it must work? What server we can/must use for this?

I haven`t provide new tests for this because I want to have prufe of concept first.
@rytilahti could you take a look at this and give your opinion

rytilahti

Hi @Kirmas and thanks for the PR! I did a very brief review and added some comments inline.

The mid-term plan (i.e., for the 0.6 release) for this library is to start moving device integrations to be more plugin-like and self-contained. Considering that this PR introduces a new directory as well as new utility data files, I'd like to see this PR as a step towards that direction. Therefore I suggest that instead of introducing a new yeelight_specs data directory, please create a new yeelight directory to hold all the auxiliary files.

In order to make the code reviews related to functional changes easier, please do not move yeelight.py into that directory. Otherwise checking which changes are just refactoring and which are functional is hard and time-consuming. After this PR gets merged to the master, a new PR should be created to perform the move.

miio/yeelight.py

rytilahti · 2021-07-13T00:29:29Z

miio/yeelight.py

+                if mode == 2:
+                    raise YeelightException("RGB color not supported")
+                if mode == 3:
+                    raise YeelightException("HSV color not supported")


Please create an enum for the color mode mapping.

Looking at this again, is there some reason not to bail out early? Having something like the following in the beginning of the method would simplify the code:

if not self.model_specs.supports.color and mode > 0: raise YeelightException(f"The bulb does not support color mode {mode}")

rytilahti · 2021-07-13T00:31:47Z

miio/yeelight.py


    @command(
        click.option("--transition", type=int, required=False, default=0),
+        click.option(
+            "--lamp", type=int, required=False, default=YeelightSubLightType.Main


Use EnumType from click_common.py module for the type, this will allow using the string representation in the cli.

rytilahti · 2021-07-13T00:34:31Z

miio/yeelight.py

+            miio_param += ["smooth", transition]
        else:
            # Bedside lamp requires transition
-            return self.send("set_ct_abx", [level, "sudden", 0])
+            miio_param += ["sudden", 0]


As the transition handling seems to be common for plenty of methods, maybe create a small helper _handle_transition(query, transition) to perform this check in one place?

rytilahti · 2021-07-13T00:35:56Z

miio/yeelight.py

        """Set color in RGB."""
+        if (
+            lamp == YeelightSubLightType.Background
+            and not self.model_specs.has_background_light


Can the combination of both of these ever happen? I'm thinking if it would be enough to check if the sublight is of type background to make the code more readable.

rytilahti · 2021-07-13T00:38:00Z

miio/yeelight.py

+        if transition > 0:
+            miio_param += ["smooth", transition]
+
+        return self.send(miio_command, miio_param)


This has no "sudden" mode, should it? See also my comment above wrt. transition handling.

rytilahti · 2021-07-13T00:40:05Z

miio/yeelight.py

+        if (
+            lamp == YeelightSubLightType.Background
+            and not self.model_specs.has_background_light
+        ):


See my above comment on this. If this is necessary, I'd suggest creating a helper method to handle the pre-checks to avoid code duplication across methods.

rytilahti · 2021-07-13T00:55:54Z

miio/yeelight.py

+        click.option(
+            "--lamp", type=int, required=False, default=YeelightSubLightType.Main
+        ),


Instead of repeating the --lamp for each command separately, I think it makes more sense to add this as a parameter to the module (see vacuum.py and its get_device_group -- it's not very pretty, but avoids this repetition and makes the command to be miiocli yeelight --lamp <type> <command> <command parameters>).

Hello @rytilahti sorry for a while, I was trying to figure out some good solution but failed.
Main problem that --lamp <type> in get_device_group pass to constructor but not a function property. And this is ruining using this functions not from command line. (Recreate a Yeelight device or pass target property every time I want to change lamp status looks ugly).

As for me: better idea can be use some thing like sub device for every sub lights.
I see this like main lamp device(general function like dump_ble_debug and general property like save_state_on_change) with one or two sub light device(Main and Background light).
Can I know device model during constructor to create list of sub device? I need this not only for get status function.
But get status for this 3(or 2) device must use one single request to physical lamp.
If some one call status function on the sub light - system must check when was be last request and use cashed data or send new one using general lamp device.

But every my tried was failed. (because command-line part doesn't wont to work with sub light class. Only with main light.(through an error Managed to invoke callback without a context).

Could you provide me some work around about this.

PS I have a lot of work at my main place of work for at least the next three months, but I am not forgetting about this pull request.
PS2 If my english not good enough ask any kinda question to understand what i mean.

Hi @Kirmas and sorry for the delayed answer!

As for me: better idea can be use some thing like sub device for every sub lights.

Agreed. A regular light and a background light should implement the same interface to allow easy interfacing.

I see this like main lamp device(general function like dump_ble_debug and general property like save_state_on_change) with one or two sub light device(Main and Background light).

The main light (and its data) is always available so I think the sublight should re-use these.

Can I know device model during constructor to create list of sub device?

The model information is now available by just accessing self.model.

If some one call status function on the sub light - system must check when was be last request and use cashed data or send new one using general lamp device.

I think that trying to access the status of the secondary light should just return the information from the main query which returns all necessary data already.

rytilahti · 2021-07-13T00:56:43Z

miio/yeelight_specs/__init__.py

+import logging
+import os
+from typing import List, Optional, Tuple
+
+import yaml


Rename from __init__.py to containers.py or something similar?

rytilahti · 2021-07-13T01:28:57Z

To your question in the description:

for "adjust" functions it is not difficult to add but I really don`t understand why we need this.(maybe if there is some free time before someone looks, I will add)

So the idea of these functions is to allow adjusting the state of the bulb relative to its current state, making it possible to execute commands like "increase brightness by 10%" or "increase color temperature by 500k".

for set_scene - I dont understand how it must work. What is the best parameters set, maybe I must to add one more .yaml file with scenes?

I'm not sure about how the API should look like for this, but the idea of these scenes is to allow setting multiple variables at once. For example, set_hsv allows just controlling the 'h' and 's' of the hsv, but using set_scene allows adjusting the v, too. Same applies to the color temperature, allowing to change from 7000k with 10% to 2500k with 50% using a single call.

for set_music - How it must work? What server we can/must use for this?

python-yeelight implements this mode. Basically, the idea is that instead of polling the device for updates, you open a listening socket and let the bulb connect to you. This allows receiving immediate updates when the light state changes (e.g., when being controlled by the official app).

Personally, I don't think that this library should implement this feature (at least not at the moment) as it will make the code more complicated.

… yeelight

Kirmas · 2021-07-13T07:14:59Z

Therefore I suggest that instead of introducing a new data directory, please create a new directory to hold all the auxiliary files.yeelight_specs``yeelight

I have tried to rename yeelight_specs folder to yeelight and after this could not resolve import (Two module with the same name), as I understand this is task for next PR or I`ll must use some work around in the this PR?

rytilahti · 2021-07-13T12:12:42Z

I have tried to rename yeelight_specs folder to yeelight and after this could not resolve import (Two module with the same name), as I understand this is task for next PR or I`ll must use some work around in the this PR?

Ah, indeed.. Okay, let's keep the directory as it is currently and do the rename in another PR after this gets done.

rytilahti · 2021-10-03T22:04:14Z

miio/yeelight.py

+        if (
+            lamp == YeelightSubLightType.Background
+            and not self.model_specs.has_background_light
+        ):
+            raise YeelightException("Background lamp not supported")


This is used often enough that it warrants its own private check method, e.g., calling self._error_on_non_background(lamp) should suffice.

Or more preferably, these checks should be done in the subclass that implements the background light handling.

rytilahti · 2021-10-03T22:05:06Z

miio/yeelight.py

+        if transition > 0:
+            miio_param += ["smooth", transition]
+        else:
+            miio_param += ["sudden", 0]


Transition seems to be used in several places, so it'd be better to add an internal method that performs its handling.

rytilahti

Hi @Kirmas and sorry for the delay. I had some time to look into this, and I think we should split this PR to make it easier to manage, review and test:

First PR should just involve adding the spec files & add necessary properties for accessing the device properties on the main device. This PR should also restructure the codebase to create a new yeelight package that will host all the files relevant to the yeelight "integration".
Second PR would be adding the support for color flows. Although this could perhaps be merged with the first one, if it looks that it doesn't require many changes.
Third PR would concern only about extending the integration to support night light / background light (assuming those are separate?). This will involve some thinking about the API design, but I think the sublights should implement the same interface as the main one while being backed by the data from the main light. The interfaces would check for the prerequisites (e.g., if trying to perform a non-supported call) and handle the query building (i.e., handling the method call prefixing).

What do you think about such approach?

rytilahti · 2021-10-03T22:24:43Z

miio/yeelight.py

+        click.option(
+            "--lamp", type=int, required=False, default=YeelightSubLightType.Main
+        ),


Hi @Kirmas and sorry for the delayed answer!

As for me: better idea can be use some thing like sub device for every sub lights.

Agreed. A regular light and a background light should implement the same interface to allow easy interfacing.

I see this like main lamp device(general function like dump_ble_debug and general property like save_state_on_change) with one or two sub light device(Main and Background light).

The main light (and its data) is always available so I think the sublight should re-use these.

Can I know device model during constructor to create list of sub device?

The model information is now available by just accessing self.model.

If some one call status function on the sub light - system must check when was be last request and use cashed data or send new one using general lamp device.

I think that trying to access the status of the secondary light should just return the information from the main query which returns all necessary data already.

rytilahti · 2021-10-03T22:30:35Z

miio/yeelight.py

+        if (
+            lamp == YeelightSubLightType.Background
+            and not self.model_specs.has_background_light
+        ):
+            raise YeelightException("Background lamp not supported")
+        return self.send(SUBLIGHT_PROP_PREFIX[lamp] + "stop_cf")


Are you sure both the main and the background light have the same features? Colors, color flows etc.? I had the idea that the background one was just a color temperature controllable, or maybe dimmable, light?

Yes bg light can do all what simple color light can. I create the table using yeelight documentation and test its with my lamp:

Method value Parameters Count Param 1 Param 2 Param 3 Param 4

get_prop 1 ~ N * * * *

set_ct_abx/bg_set_ct_abx 3 int (ct_value) string(effect) int(duration)

set_rgb/bg_set_rgb 3 int(rgb_value) string(effect) int(duration)

set_hsv/bg_set_hsv 4 int(hue) int(sat) string(effect) int(duration)

set_bright/bg_set_bright 3 int(brightness) string(effect) int(duration)

set_power/bg_set_power 3 string(power) string(effect) int(duration) int(mode)

toggle/bg_toggle/dev_toggle 0

set_default/bg_set_default 0

set_name 1 string(name)

start_cf / bg_start_cf 3 int(count) int(action) string(flow_expression)

stop_cf / bg_stop_cf 0

set_scene / bg_set_scene 3 ~ 4 string(class) int(val1) int(val2) *int(val3)

cron_add 2 int(type) int(value)

cron_get 1 int(type)

cron_del 1 int(type)

set_adjust / bg_set_adjust 2 string(action) string(prop)

set_music 1 ~ 3 int(action) string(host) int(port)

adjust_bright / bg_adjust_bright 2 int(percentage) int(duration)

adjust_ct / bg_adjust_ct 2 int(percentage) int(duration)

adjust_color / bg_adjust_color 2 int(percentage) int(duration)

All bg_ is for background light.

PS @rytilahti I answer this for story. When I do new PR for this I'll look to this.

rytilahti · 2021-10-03T22:34:44Z

miio/yeelight.py

+                if mode == 2:
+                    raise YeelightException("RGB color not supported")
+                if mode == 3:
+                    raise YeelightException("HSV color not supported")


Looking at this again, is there some reason not to bail out early? Having something like the following in the beginning of the method would simplify the code:

if not self.model_specs.supports.color and mode > 0: raise YeelightException(f"The bulb does not support color mode {mode}")

Kirmas added 6 commits May 23, 2021 22:30

sub light support for standard functions

599c682

test fix

665757c

Merge branch 'rytilahti:master' into yeelight

d71d3be

Merge branch 'rytilahti:master' into yeelight

99b0903

Add lamp specific file and color flows

244b8db

Merge branch 'rytilahti:master' into yeelight

7c521c4

rytilahti reviewed Jul 13, 2021

View reviewed changes

Kirmas added 2 commits July 13, 2021 10:06

renaming __init__.py to containers.py

f9dee3c

Merge branch 'yeelight' of https://github.com/Kirmas/python-miio into…

e39a707

… yeelight

rytilahti mentioned this pull request Aug 13, 2021

[meta] Restructure integrations to their own directories #1115

Closed

1 task

rytilahti reviewed Oct 3, 2021

View reviewed changes

This was referenced Oct 16, 2021

create separate directory for yeelight #1160

Merged

Add light specs for yeelight #1163

Merged

Kirmas mentioned this pull request Dec 13, 2021

yeelight: use and expose the color temp range from specs #1247

Merged

Kirmas closed this Oct 25, 2022

Method value	Parameters Count	Param 1	Param 2	Param 3	Param 4
get_prop	1 ~ N	*	*	*	*
set_ct_abx/bg_set_ct_abx	3	int (ct_value)	string(effect)	int(duration)
set_rgb/bg_set_rgb	3	int(rgb_value)	string(effect)	int(duration)
set_hsv/bg_set_hsv	4	int(hue)	int(sat)	string(effect)	int(duration)
set_bright/bg_set_bright	3	int(brightness)	string(effect)	int(duration)
set_power/bg_set_power	3	string(power)	string(effect)	int(duration)	int(mode)
toggle/bg_toggle/dev_toggle	0
set_default/bg_set_default	0
set_name	1	string(name)
start_cf / bg_start_cf	3	int(count)	int(action)	string(flow_expression)
stop_cf / bg_stop_cf	0
set_scene / bg_set_scene	3 ~ 4	string(class)	int(val1)	int(val2)	*int(val3)
cron_add	2	int(type)	int(value)
cron_get	1	int(type)
cron_del	1	int(type)
set_adjust / bg_set_adjust	2	string(action)	string(prop)
set_music	1 ~ 3	int(action)	string(host)	int(port)
adjust_bright / bg_adjust_bright	2	int(percentage)	int(duration)
adjust_ct / bg_adjust_ct	2	int(percentage)	int(duration)
adjust_color / bg_adjust_color	2	int(percentage)	int(duration)

Uh oh!

Yeelight spec files was added #1094

Yeelight spec files was added #1094

Uh oh!

Conversation

Kirmas commented Jul 11, 2021

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Kirmas Aug 17, 2021 • 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!

rytilahti commented Jul 13, 2021

Uh oh!

Kirmas commented Jul 13, 2021

Uh oh!

rytilahti commented Jul 13, 2021

Uh oh!

rytilahti Oct 3, 2021 • 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!

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!

Uh oh!

Kirmas Aug 17, 2021 •

edited

Loading

rytilahti Oct 3, 2021 •

edited

Loading