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

Added effects to Yeelight bulbs #7152

Merged
merged 7 commits into from Jun 3, 2017

Conversation

Projects
None yet
10 participants
@Mister-Espria
Copy link
Contributor

commented Apr 17, 2017

Description:

This PR is now reopened, because there are now effects implemented in the yeelight module version 3.0.0.
I did update the original code of this PR to support the newly implemented effects.

Any suggetions/improvements are welcome of course.

PR text before closing:

Added some basic effects to Yeelight bulbs. Deleted requirement for the bulb to be in a specific colormode in order to use flash parameter.

There are three different type of effects added:

  1. Predefined colors and brightness;
  2. Predefined colors with current brightness;
  3. Random colors with current brightness.

I also did a "stop" effect which stops the effect and returns to the previous state.
I did add this to the effectlist i don't know if this is the best approach as it is not really an effect.

This is my first PR ever. So if i did something wrong please tell me. So i can improve it.
Also i would like to thank @armills for all his help!

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

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

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.
@mention-bot

This comment has been minimized.

Copy link

commented Apr 17, 2017

@Mister-Espria, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rytilahti, @jjensn and @HydrelioxGitHub to be potential reviewers.

@homeassistant

This comment has been minimized.

Copy link

commented Apr 17, 2017

Hi @Mister-Espria,

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!

@homeassistant

This comment has been minimized.

Copy link

commented Apr 17, 2017

Hi @Mister-Espria,

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!

homeassistant/components/light/yeelight.py Outdated
random.randrange(0, 255),
random.randrange(0, 255),
brightness=self.brightness,
duration=200))

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 17, 2017

Contributor

I think you mean duration=2000 here, not 200

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria Apr 17, 2017

Author Contributor

Yes typo i will fix it!

homeassistant/components/light/yeelight.py Outdated

transitions = list()
transitions.append(
RGBTransition(random.randrange(0, 255),

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 17, 2017

Contributor

You want randrange(0, 256) or randint(0, 255)

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria Apr 17, 2017

Author Contributor

random.randrange(0, 255) is used in the flux component. i did't give it any thought. So i should change it into random.randint(0, 255) right?

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 17, 2017

Contributor

Yes, if you want 255 as a possible outcome. Not that anyone will be able to tell the difference 😃

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria Apr 17, 2017

Author Contributor

Nope but then it is as random as it can get 😄 . So i will change it.

@balloobbot balloobbot added the platform label Apr 17, 2017

@Mister-Espria Mister-Espria referenced this pull request Apr 17, 2017

Merged

Flux_led: small fix random effects #7156

1 of 8 tasks complete
homeassistant/components/light/yeelight.py Outdated
from yeelight import (RGBTransition, TemperatureTransition,
SleepTransition, Flow, BulbException)
transition = int(self.config[CONF_TRANSITION])
if effect == EFFECT_ALARM:

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 22, 2017

Member

I think that the logic for the effects should be located in the yeelight module itself. This way the effects can be maintained at one place.

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria Apr 23, 2017

Author Contributor

Thanks for your response. I think you are right, that the best place for it is in the yeelight module. I have tried to make a start with adding it there. But with trial and error i'm getting nowhere. I have no clue on how to start the implementation there. I will still be trying, but i'm not certain i will get it done 😃
What do you want me to do with this PR?

This comment has been minimized.

Copy link
@rytilahti

rytilahti Apr 25, 2017

Contributor

This is being discussed here: https://gitlab.com/stavros/python-yeelight/issues/11 so I'd propose closing this PR for now and reopening/creating a new one after the implementation hits the upstream :-)

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria Apr 26, 2017

Author Contributor

Ok. I will do that now. Thank you.

Mister-Espria added some commits Apr 17, 2017

@Mister-Espria

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2017

I'm re-opening this PR, because there are now effects implemented in the yeelight module.
Will be adding changes shortly!
Edit: Done!

@Mister-Espria Mister-Espria reopened this May 14, 2017

@Mister-Espria Mister-Espria changed the title Added effects to Yeelight bulbs WIP:Added effects to Yeelight bulbs May 14, 2017

@Mister-Espria Mister-Espria changed the title WIP:Added effects to Yeelight bulbs Added effects to Yeelight bulbs May 14, 2017

EFFECT_TWITTER = "Twitter"
EFFECT_STOP = "Stop"

YEE_EFFECT_LIST = [

This comment has been minimized.

Copy link
@rytilahti

rytilahti May 14, 2017

Contributor

YEELIGHT_EFFECT_LIST to be consistent?

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria May 14, 2017

Author Contributor

Thanks for your quick review. This is something i thought of and then forgot it of course.
Thank you i will change it.

strobe_color, alarm, police,
police2, christmas, rgb,
randomloop, slowdown)
# transition = int(self.config[CONF_TRANSITION])

This comment has been minimized.

Copy link
@rytilahti

rytilahti May 14, 2017

Contributor

Remove commented out lines.

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria May 14, 2017

Author Contributor

removed.

randomloop, slowdown)
# transition = int(self.config[CONF_TRANSITION])
if effect == EFFECT_DISCO:
flow = Flow(count=0, transitions=disco(),)

This comment has been minimized.

Copy link
@rytilahti

rytilahti May 14, 2017

Contributor

Any reason these are passed as tuples? A list is expected? Maybe it'd make sense to do [disco()] to make it clear transitions expects an iterable? (Or in even better case, it should also allow single transitions...)

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria May 14, 2017

Author Contributor

Probably my incompetence. The reason i have this is because this is how i did it to get it to work. the disco function returns a list with the transitions. I just tried you example [disco()] but i can't get it to work. maybe i need to do something other then replace disco() with [disco()] ?

This comment has been minimized.

Copy link
@rytilahti

rytilahti May 16, 2017

Contributor

Hmm, and passing the list directly does not work? To my understanding it should..

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria May 16, 2017

Author Contributor

Yes just passing the list with transitions does work, but that is what i originally had.

@fabaff commented:

I think that the logic for the effects should be located in the yeelight module itself. This way the effects can be maintained at one place.

So this way we don't have to maintain the list for every transition, in case of changes in the yeelight module

This comment has been minimized.

Copy link
@rytilahti

rytilahti May 16, 2017

Contributor

Ah, I mean passing "disco()" as a parameter to transitions like:

Flow(count=0, transitions=disco())

should work. @fabaff is correct by saying that the implementation details belong to the module itself, where they are now thanks to you! :-)

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria May 17, 2017

Author Contributor

Ah i see, That works! thank you i have changed it. And i completely agree with having the effects over there.

if effect == EFFECT_TWITTER:
flow = Flow(count=2, transitions=pulse(0, 172, 237),)

if effect == EFFECT_STOP:

This comment has been minimized.

Copy link
@rytilahti

rytilahti May 14, 2017

Contributor

Should this be before other checks?

This comment has been minimized.

Copy link
@Mister-Espria

Mister-Espria May 14, 2017

Author Contributor

Yes i think that will make more sense. Changing it now.

@balloob

balloob approved these changes Jun 3, 2017

Copy link
Member

left a comment

Looks good. Sorry for this taking so long. 🐬

@balloob balloob merged commit 7d24efc into home-assistant:dev Jun 3, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 93.618%
Details
hound No violations found. Woof!

@balloob balloob referenced this pull request Jun 16, 2017

Merged

0.47 #8055

@bokub

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2017

Neat effects!
I just discovered them a few minutes ago by chance

Thanks for your work @Mister-Espria 👍

@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017

@Mister-Espria Mister-Espria deleted the Mister-Espria:yeelight-adding-effects branch Sep 24, 2018

@Mister-Espria Mister-Espria restored the Mister-Espria:yeelight-adding-effects branch Sep 24, 2018

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.