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

Add start_flow service to yeelight with an ability to declare custom effects #20107

Merged
merged 1 commit into from Jan 24, 2019

Conversation

Projects
None yet
5 participants
@zewelor
Copy link
Contributor

zewelor commented Jan 14, 2019

Description:

Added support for declaring custom effects for yeelight platform. I wanted to recreate yeelight app effects, like fire flicker. Didn't want to hardcore anything in python-yeelight, as probably everyone will want to tweak some effects etc. Also exposed service to start flow with custom transitions. Use case is to control brightness / duration etc using some templates / dynamic values.

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

Example entry for configuration.yaml (if applicable):

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

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:

@zewelor zewelor requested a review from rytilahti as a code owner Jan 14, 2019

@homeassistant

This comment has been minimized.

Copy link

homeassistant commented Jan 14, 2019

Hi @zewelor,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@bbbenji

This comment has been minimized.

Copy link

bbbenji commented Jan 17, 2019

Great PR! Thanks.

@rytilahti
Copy link
Contributor

rytilahti left a comment

Looks good to me, I didn't test on a real device.

Show resolved Hide resolved homeassistant/components/light/services.yaml Outdated
Show resolved Hide resolved homeassistant/components/light/services.yaml Outdated

@zewelor zewelor force-pushed the zewelor:add_yeelight_start_flow_service branch from 19fe82c to f69587b Jan 17, 2019

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Jan 17, 2019

Thanks for Review. Updated with comments addressed.

@zewelor zewelor force-pushed the zewelor:add_yeelight_start_flow_service branch 3 times, most recently from 07b2c40 to 18f3d15 Jan 17, 2019

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Jan 17, 2019

Tests fixed and rebased to single commit.

@rytilahti rytilahti changed the title Add yeelight start flow service and ability to declare custom effects Add start_flow service to yeelight with an ability to declare custom effects Jan 19, 2019

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Jan 21, 2019

@rytilahti Is there anything more I should do, to PR get approved ? It's my first contribution, not sure about the flow.

@rytilahti
Copy link
Contributor

rytilahti left a comment

I have now tested this briefly and it seemed to work fine, I think this is a really good addition! :-)

One nitpicky thing that I don't like that much is how the parameters are passed -- it took reading the code to know what the parameters are, e.g., for TemperatureTransition (temperature, time_in_milliseconds, brightness). This could be alleviated by using by passing keyword arguments as a dict, but it may not be worth the effort and could be fixed by improving the documentation.

In my opinion this is ready to be merged as soon as the documentation PR is approved.

Show resolved Hide resolved homeassistant/components/light/services.yaml Outdated

@zewelor zewelor force-pushed the zewelor:add_yeelight_start_flow_service branch from 18f3d15 to ac6e507 Jan 21, 2019

@zewelor

This comment has been minimized.

Copy link
Contributor Author

zewelor commented Jan 21, 2019

I was thinking about dict params, but after looking at config with it, it was long and another level of nesting. Also they don't have default values, so all of them are needs to be specified. After writing some test examples, they were quite easy to guess, as value ranges differs. That was my reasoning, maybe we could make support for both. Or wait for more feedback to design it better ? I don't have strong opinions about any of the solution, so I've choosen simpler / shorter. Thanks for approval !

@rytilahti

This comment has been minimized.

Copy link
Contributor

rytilahti commented Jan 24, 2019

Ok, let's keep this as it is, it is not so crucial. I think this is ready to be merged after the documentation update is ready! 💯

@rytilahti rytilahti merged commit 2559bc4 into home-assistant:dev Jan 24, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0006%) to 92.927%
Details

@balloob balloob referenced this pull request Feb 6, 2019

Merged

0.87.0 #20794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment