Move yeelight into component #21593

zewelor · 2019-03-02T13:54:41Z

Breaking Changes

Move yeelight from light platform to component. As more yeelights are being released, with more features. It will make possible to support them. For example adding sensors for ceiling light, to get current power mode ( daylight / nightlight ). Or switches to support turning on off moonlight ( without having to use service call ). Also switch to turn off both main light and ambilight ( ceiling 650 ). Probably something more, as we will see what ideas we can make for better UX and support.

If you have following configuration:

light:
  - platform: yeelight
    devices:
      192.168.1.13:
        name: Wall light
    custom_effects:
      - name: 'Fire Flicker'
        flow_params:
          count: 0
          transitions:
            - TemperatureTransition: [1900, 1000, 80]
            - TemperatureTransition: [1900, 2000, 60]
            - SleepTransition:       [1000]

It needs to be migrated to:

yeelight:
  devices:
    192.168.1.13:
      name: Wall light
  custom_effects:
    - name: 'Fire Flicker'
      flow_params:
        count: 0
        transitions:
          - TemperatureTransition: [1900, 1000, 80]
          - TemperatureTransition: [1900, 2000, 60]
          - SleepTransition:       [1000]

Just move everything to yeelight namespace, and remove 2 spaces vertical.

Description:

Moved yeelight into component to allow adding switches / sensors etc. Now only option to change yeelight power mode ( daylight / nightlight in yeelight ceiling lamps ), is to call service. Then its not possible to see current power mode. Added nightlight switch to turn on / off nightlight mode and get feedback about currently enabled power mode. I'm not sure about many things, regarding component -> platform communication and setup. I've looked at other components and tried to come up with something works. Whole config is identical as it was for light platform. Looking to early feedback if its done right, then I will add docs update if its ok.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8845

Example entry for `configuration.yaml` (if applicable):

yeelight:
  devices:
    192.168.1.2:
      name: Yeelight

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.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.io

homeassistant/components/yeelight/switch.py

homeassistant/components/yeelight/sensor.py

rytilahti · 2019-03-02T16:05:54Z

I am going to try this out later on, but would it make sense to convert yeelight to use their custom protocol alongside with this conversion? This way we would gain the unique ids for the devices, allowing renames in the UI: https://yeelight.readthedocs.io/en/latest/#usage

If this will introduce a breaking change, I would also say that we should drop the LEGACY_DEVICE_TYPE_MAP in favor of using the device provided types.

edit: another note, components need to have unit tests available, so that needs to be taken care of.

zewelor · 2019-03-02T16:19:06Z

Thanks for initial feedback. I will work on tests and fix lint issues.

You mean id presented in discover_bulbs example ? Not sure where it should be used, I assume to somehow register devices with it ?

I was also wondering, how to approach light / switch synchonization of update. For example, when light is off, switching switch will turn it on, but then light waits up to 30s to update status. Is there any guideline how to synchonize between different platforms ? I was thinking light object could be passed to switch, but it looks like a bad coupling.

rytilahti · 2019-03-02T17:21:05Z

Yeah, I'm talking about using that id as unique_id inside homeassistant's entity registry (https://developers.home-assistant.io/docs/en/entity_registry_index.html). This will allow homeassistant to identify the bulb uniquely even if its IP address changes and will enable the "rename device" option in the UI (so no need to manually use friendly_name mappings). As this information is not available with mDNS discovery (what is currently used), it has not been possible earlier.

Wrt syncing, I think you can use the schedule_update (https://developers.home-assistant.io/docs/en/entity_index.html#subscribing-to-updates) even when polling is used, but I hope someone will confirm that or propose another approache.

Wrt coupling, I think it's fine to pass a reference of the light to the switch as they are coupled anyway.

houndci-bot · 2019-03-03T09:37:27Z

homeassistant/components/yeelight/switch.py

+
+_LOGGER = logging.getLogger(__name__)
+
+def setup_platform(hass, config, add_entities, discovery_info=None):


expected 2 blank lines, found 1

zewelor · 2019-03-03T09:38:47Z

~~I think I've figures out how to synchronize state between entities. Code taken from KNX integration.~~

Synchonization done using homeassistant.helpers.dispatcher

Also I've put some fixes, regarding nightlight:
d85bc75#diff-6b4ac627753ee21cde162f000848ba3bR326

d85bc75#diff-6b4ac627753ee21cde162f000848ba3bR312

zewelor · 2019-03-05T19:19:05Z

Fixed lint errors.

I've also asked on discord about help with writing tests. Response I've got was that requirements for test is not written in checklist, so maybe they are not needed ( https://developers.home-assistant.io/docs/en/creating_component_code_review.html ). I've tried looking at some similar platform, to get idea how to approach / what to tests, but still not sure how to write them correctly.

As for unique id, not sure how to approach that ? Should discovery_bulb be done in component setup ? Or register some background job, and when response come, update all bulb devices to put received id ? Also is it needed to be done here. Maybe putting too large PR will make it harder to merge and somehow blocks other PR regarding yeelight. There is ongoing effort to add ambilight support for ceiling 650. Also I was thinking about adding some more config options, in separated PRs. Or its better to make one big ?

rytilahti · 2019-03-05T19:31:40Z

I cannot say about the test requirements, but I had to create some when porting the tplink into a component.. I have not yet found time to test these changes and cannot currently promise when I'm able to do that, so I hope someone else chimes in.

Anyway, on the unique_ids and other improvements, I think it's better to done it in phases to keep it simple so please ignore that for now. That conversion will be a bit more involved, basically requiring creating a config entry flow (https://developers.home-assistant.io/docs/en/config_entries_index.html) to do the discovery and initializing the platforms based on the discovery results.

zewelor · 2019-03-10T21:40:57Z

Moved services into component domain. I think this is ready.

houndci-bot · 2019-03-17T15:17:51Z

homeassistant/components/yeelight/light.py

+

blank line contains whitespace

houndci-bot · 2019-03-17T15:17:51Z

homeassistant/components/yeelight/light.py

-    return effects
-
-
+


blank line contains whitespace

zewelor · 2019-03-17T18:17:48Z

@rytilahti Its ready, rebased on newest code. Can you take a look at it ? I've removed switch as I'm not sure about its UX ( What it should do on off etc). Will add sensor to get current working mode for daylight / nightlight for ceiling lights.

syssi

LGTM! Works fine with my Yeelight Color 1gen.

syssi · 2019-03-23T16:00:09Z

Could you fix the conflict? Thanks!

zewelor · 2019-03-23T16:55:55Z

@syssi Thanks, rebased on new code. Are you planning on adding ceiling4 ambilight support ? I saw some post about it on community forum. I was planning to work on it, don't want to duplicate work. After merging this I wanted to make PR with python-yeelight bump and some cleaning, that will be possible with new lib version. And then I can work on ambilight support. Also I want to add power mode sensor, for better UX with moonlight mode for ceilings.

syssi · 2019-03-23T17:09:51Z

@zewelor I would be happy if you do most of the job. I don't own the device. I'd like to provide some support.

MartinHjelmare

Please open a new PR where we can address the comments.

MartinHjelmare · 2019-03-24T12:25:15Z

homeassistant/components/yeelight/__init__.py

+Support for Xiaomi Yeelight Wifi color bulb.
+
+For more details about this platform, please refer to the documentation at
+https://home-assistant.io/components/yeelight/


We don't add the url in the docstring anymore.

MartinHjelmare · 2019-03-24T12:27:29Z

homeassistant/components/yeelight/__init__.py

+        for device in yeelight_data[CONF_DEVICES].values():
+            device.update()
+
+    async_track_time_interval(


We can't call an async function from sync context or vice versa. Use track_time_interval instead.

MartinHjelmare · 2019-03-24T12:28:00Z

homeassistant/components/yeelight/__init__.py

+
+    discovery.listen(hass, SERVICE_YEELIGHT, device_discovered)
+
+    def async_update(event):


Why name this async_update when it's not a coroutine or callback decorated function?

MartinHjelmare · 2019-03-24T12:32:39Z

homeassistant/components/yeelight/light.py

-    hass.services.register(
-        DOMAIN, SERVICE_START_FLOW, service_handler,
-        schema=service_schema_start_flow)
+    yeelight_data[CONF_LIGHTS][ipaddr] = light


We're not allowed to store the entity itself in a shared container. The entity is a platform concern. It's ok to store a reference to the entity, eg the entity_id.

MartinHjelmare · 2019-03-24T12:34:41Z

homeassistant/components/yeelight/__init__.py

+        entity_ids = extract_entity_ids(hass, service)
+        target_devices = [dev.device for dev in
+                          yeelight_data[CONF_LIGHTS].values()
+                          if dev.entity_id in entity_ids]


We don't need to access the entity here. It's enough to check for the correct entity_ids. See comment below about storing entities vs entity_ids.

zewelor · 2019-03-24T13:18:37Z

@MartinHjelmare PR with fixes here: #22347

sergeymaysak · 2019-03-27T21:22:29Z

@zewelor First, I really appreciate this effort which significantly improves usage of my yeelight 650. I've tested today in merged to dev branch. Yes - since now it allows me to control ambient lights as well. Great job, really! However there are several issues which I believe important to depict and possibly to fix maybe in next releases of this great contribution:

the most significant issue imo - if my configuration.yaml does not have any notions of yeelight platform it still discovered by hass (Bonjour/mDNS is here ;) for good) and before this contribution I had just auto-discovered yeelight light representing main bulb (yes, ambient lighs missed). With this contribution I have exception while hass load:

2019-03-27 20:12:15 ERROR (MainThread) [homeassistant.setup] Error during setup of component yeelight
Traceback (most recent call last):
  File "/Users/sam/0.homeassistant/home-assistant/homeassistant/setup.py", line 154, in _async_setup_component
    component.setup, hass, processed_config)  # type: ignore
  File "/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/sam/0.homeassistant/home-assistant/homeassistant/components/yeelight/__init__.py", line 147, in setup
    conf = config[DOMAIN]
KeyError: 'yeelight'

and no lights at all.
So yes, after addition of yeelight: into configuration.yaml its autodiscovered my lights and now its shows two lights for device - main and ambient! great! as expected.
But the point is that overall user experience is completely suffered - instead of magical auto-discovered device which is really shows the power and elegance and magic of HA to end users - users would have to specify platform explicitly. Pitty. If hovewer KeyError whould be just suppressed in code it would provide significant improvement for user experience. Please consider. And yes - I do not specify my yeelight ip addresses - yes I want Bonjour and auto-discovery do it for me. Yes I do really dont want to modify my configuration.yaml for cases that do not introduce anything specific - I love when my new devices are auto-discovered ...
2. issue number two but less significant ):
with this contribution main light lost ability to change temperature and specify effect from UI:

So only brightness can be changed. Well, hmmm - presence of ability to control ambient light is really more important for me, but sounds as defect imo...

Thanks again for overall great contribution - hope you would share my vision on UX here ) Cheers!

MartinHjelmare · 2019-03-27T21:46:50Z

Please open an issue if you suspect a bug.

If you want to suggest an enhancement please open a feature request in the Feature Requests section of our community forum.

Merged PRs should not be used for enhancement discussion or bug reports. If you've found a bug it's ok to make a review with inline comments and link to an issue that reports the bug.

Thanks!

zewelor requested a review from rytilahti as a code owner March 2, 2019 13:54

homeassistant added cla-signed integration: discovery new-platform integration: yeelight labels Mar 2, 2019

ghost added the in progress label Mar 2, 2019

houndci-bot reviewed Mar 2, 2019

View reviewed changes

rytilahti added the breaking-change label Mar 2, 2019

houndci-bot reviewed Mar 3, 2019

View reviewed changes

zewelor mentioned this pull request Mar 3, 2019

Yeelight Moonlight mode brightness slider glitch #18313

Closed

zewelor mentioned this pull request Mar 6, 2019

Update docs with yeelight component home-assistant/home-assistant.io#8845

Merged

2 tasks

houndci-bot reviewed Mar 17, 2019

View reviewed changes

zewelor changed the title ~~Move yeelight into component and add power mode switch~~ Move yeelight into component Mar 17, 2019

syssi approved these changes Mar 23, 2019

View reviewed changes

zewelor added 5 commits March 23, 2019 17:43

Move yeelight into component and add power mode switch

aeae0fe

Synchronize state between entities, fixes for nightlight

124f047

Lint fixes

f1e212f

Move yeelight services to component domain

1101c18

Remove switch from PR

ff53476

zewelor mentioned this pull request Mar 24, 2019

Update python yeelight and add nightlight mode sensor #22345

Merged

3 tasks

syssi merged commit 9214934 into home-assistant:dev Mar 24, 2019

ghost removed the in progress label Mar 24, 2019

home-assistant deleted a comment from houndci-bot Mar 24, 2019

MartinHjelmare reviewed Mar 24, 2019

View reviewed changes

zewelor mentioned this pull request Mar 24, 2019

Improve yeelight component #22347

Merged

3 tasks

zewelor deleted the move_yeelight_into_component_and_add_power_mode_switch branch March 24, 2019 20:01

home-assistant locked as resolved and limited conversation to collaborators Mar 27, 2019

balloob removed the new-platform label Mar 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move yeelight into component #21593

Move yeelight into component #21593

zewelor commented Mar 2, 2019 •

edited

Loading

rytilahti commented Mar 2, 2019 •

edited

Loading

zewelor commented Mar 2, 2019

rytilahti commented Mar 2, 2019

houndci-bot Mar 3, 2019

zewelor commented Mar 3, 2019 •

edited

Loading

zewelor commented Mar 5, 2019

rytilahti commented Mar 5, 2019

zewelor commented Mar 10, 2019

houndci-bot Mar 17, 2019

houndci-bot Mar 17, 2019

zewelor commented Mar 17, 2019

syssi left a comment

syssi commented Mar 23, 2019

zewelor commented Mar 23, 2019

syssi commented Mar 23, 2019

MartinHjelmare left a comment

MartinHjelmare Mar 24, 2019

MartinHjelmare Mar 24, 2019

MartinHjelmare Mar 24, 2019

MartinHjelmare Mar 24, 2019

MartinHjelmare Mar 24, 2019

zewelor commented Mar 24, 2019

sergeymaysak commented Mar 27, 2019 •

edited

Loading

MartinHjelmare commented Mar 27, 2019


		_LOGGER = logging.getLogger(__name__)

		def setup_platform(hass, config, add_entities, discovery_info=None):


		discovery.listen(hass, SERVICE_YEELIGHT, device_discovered)

		def async_update(event):

Move yeelight into component #21593

Move yeelight into component #21593

Conversation

zewelor commented Mar 2, 2019 • edited Loading

Breaking Changes

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

rytilahti commented Mar 2, 2019 • edited Loading

zewelor commented Mar 2, 2019

rytilahti commented Mar 2, 2019

Choose a reason for hiding this comment

zewelor commented Mar 3, 2019 • edited Loading

zewelor commented Mar 5, 2019

rytilahti commented Mar 5, 2019

zewelor commented Mar 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zewelor commented Mar 17, 2019

syssi left a comment

Choose a reason for hiding this comment

syssi commented Mar 23, 2019

zewelor commented Mar 23, 2019

syssi commented Mar 23, 2019

MartinHjelmare 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

zewelor commented Mar 24, 2019

sergeymaysak commented Mar 27, 2019 • edited Loading

MartinHjelmare commented Mar 27, 2019

zewelor commented Mar 2, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

rytilahti commented Mar 2, 2019 •

edited

Loading

zewelor commented Mar 3, 2019 •

edited

Loading

sergeymaysak commented Mar 27, 2019 •

edited

Loading