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.

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

Move yeelight into component #21593

Conversation

@zewelor
Copy link
Contributor

zewelor commented Mar 2, 2019

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:

Show resolved Hide resolved homeassistant/components/yeelight/switch.py Outdated
Show resolved Hide resolved homeassistant/components/yeelight/switch.py Outdated
Show resolved Hide resolved homeassistant/components/yeelight/switch.py Outdated
Show resolved Hide resolved homeassistant/components/yeelight/switch.py Outdated
Show resolved Hide resolved homeassistant/components/yeelight/switch.py Outdated
Show resolved Hide resolved homeassistant/components/yeelight/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/yeelight/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/yeelight/sensor.py Outdated

@zewelor zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch 3 times, most recently from ba39adf to e674750 Mar 2, 2019

@rytilahti

This comment has been minimized.

Copy link
Contributor

rytilahti commented Mar 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Mar 2, 2019

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

This comment has been minimized.

Copy link
Contributor

rytilahti commented Mar 2, 2019

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.

@zewelor zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch from e674750 to 3179689 Mar 3, 2019


_LOGGER = logging.getLogger(__name__)

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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 3, 2019

expected 2 blank lines, found 1

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Mar 3, 2019

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 zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch 2 times, most recently from 4169f5b to a47de39 Mar 3, 2019

@zewelor zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch 2 times, most recently from fab15ae to 9149895 Mar 3, 2019

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Mar 5, 2019

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

This comment has been minimized.

Copy link
Contributor

rytilahti commented Mar 5, 2019

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 zewelor referenced this pull request Mar 6, 2019

Merged

Update docs with yeelight component #8845

2 of 2 tasks complete

@zewelor zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch 2 times, most recently from 82308eb to 68f1afa Mar 10, 2019

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Mar 10, 2019

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

@zewelor zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch from 68f1afa to 0bd821b Mar 11, 2019



This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 17, 2019

blank line contains whitespace

return effects



This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 17, 2019

blank line contains whitespace

@zewelor zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch from b5e3dc2 to 4291f97 Mar 17, 2019

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

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Mar 17, 2019

@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

syssi approved these changes Mar 23, 2019

Copy link
Member

syssi left a comment

LGTM! Works fine with my Yeelight Color 1gen.

@syssi

This comment has been minimized.

Copy link
Member

syssi commented Mar 23, 2019

Could you fix the conflict? Thanks!

@zewelor zewelor force-pushed the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch from 4291f97 to ff53476 Mar 23, 2019

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Mar 23, 2019

@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

This comment has been minimized.

Copy link
Member

syssi commented Mar 23, 2019

@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.

@zewelor zewelor referenced this pull request Mar 24, 2019

Merged

Update python yeelight and add nightlight mode sensor #22345

3 of 3 tasks complete

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

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.3%) to 93.357%
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Mar 24, 2019

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

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

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

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/

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

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

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

async_track_time_interval(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

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


discovery.listen(hass, SERVICE_YEELIGHT, device_discovered)

def async_update(event):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

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

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

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.

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]

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 24, 2019

Member

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 zewelor referenced this pull request Mar 24, 2019

Merged

Improve yeelight component #22347

3 of 3 tasks complete
@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Mar 24, 2019

@MartinHjelmare PR with fixes here: #22347

@zewelor zewelor deleted the zewelor:move_yeelight_into_component_and_add_power_mode_switch branch Mar 24, 2019

@sergeymaysak

This comment has been minimized.

Copy link
Contributor

sergeymaysak commented Mar 27, 2019

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

  1. 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:
Screen Shot 2019-03-27 at 11 18 47 PM

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

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 27, 2019

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!

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

@balloob balloob removed the new-platform label Mar 28, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.