Skip to content

LIFX: add lifx_set_state service call#7552

Merged
balloob merged 4 commits into
home-assistant:devfrom
amelchio:lifx-set_color
May 12, 2017
Merged

LIFX: add lifx_set_state service call#7552
balloob merged 4 commits into
home-assistant:devfrom
amelchio:lifx-set_color

Conversation

@amelchio
Copy link
Copy Markdown
Contributor

@amelchio amelchio commented May 11, 2017

Description:

As suggested in #7477, this is a LIFX-specific service that is just like light.turn_on except it does not actually turn on the light (unless asked to).

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

Example entry for configuration.yaml (if applicable):

automation:
  # Reset all lights from our evening fun (but do not light up the whole house).
  - alias: Bright morning light
    trigger:
      platform: time
      after: '06:00'
    action:
      - service: light.lifx_set_state
        data:
          color_temp: 200
          brightness: 200
          transition: 180

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.

@mention-bot
Copy link
Copy Markdown

@amelchio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @shmuelzon and @BillyNate to be potential reviewers.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined name 'get_descriptions'

amelchio added 2 commits May 11, 2017 23:32
This is a synonym for light.turn_on except it does not actually turn on the
light unless asked to.

Thus, turn_on can be implemented just by asking.

CONF_SERVER = 'server'

SERVICE_LIFX_SET_COLOR = 'lifx_set_color'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't something like lifx_set_state make more sense ? You're not just adjusting the color, also brightness etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think lifx_set_state carried much meaning, seeing that every service call is about changing the state. I changed it anyway, at least it does not give the wrong impression like lifx_set_color did.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can think of other names, set_attributes, set_properties… guess all of them are slightly confusing.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's in LIFX and does not interfere with normal running of hass, I'm fine with it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined name 'changed_power'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comparison to False should be 'if cond is False:' or 'if not cond:'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comparison to True should be 'if cond is True:' or 'if cond:'

@amelchio amelchio changed the title LIFX: add lifx_set_color service call LIFX: add lifx_set_state service call May 12, 2017
@amelchio
Copy link
Copy Markdown
Contributor Author

I changed the service call into lifx_set_state and gave it the capability of also turning off the light. It now matches the LIFX HTTP Set State pretty well and is also what @keatontaylor suggested in issue 4005 (except it is not global and thus does not help Flux).

@balloob balloob merged commit 1ab7103 into home-assistant:dev May 12, 2017
@keatontaylor
Copy link
Copy Markdown
Contributor

I really hate to stay this, especially considering that personally I really want to see this functionality, but I think these types of changes that are to a single platform but drastically change the method in which automations will interact with lights is the wrong direction in an effort to unify components into a single platform.

If each of the light components offered bundled technology outside the scope of the core light component in HA we lose the ability to control lights across platforms in a unified way. This type of fragmentation is something I would highly wish to avoid, especially in a component such as lights that I strongly feel needs to be reworked in order to provide some consistency among the different type of lights.

Overall I'd prefer the unification and abstraction of the light component so that we can prevent code duplication and provide a highly coherent and cohesive platform for further development.

@amelchio, @balloob I would like to hear your thoughts if you have time.

@amelchio
Copy link
Copy Markdown
Contributor Author

@keatontaylor I do agree with your vision, which is why I started out with #7477. Read the "Alternatives" and "Further work" sections to see how I think this should preferably be done (it matches your thoughts).

However, I honestly do not see that goal being reached any time soon. For a unified solution, we need motivated non-LIFX developers to step up as well.

I think it's a shame that Hue etc. owners are missing out but at the same time I am not going to forfeit functionality just to promote some higher purpose. Hence this PR.

@balloob
Copy link
Copy Markdown
Member

balloob commented May 13, 2017

There is currently no common ground for set_state functionality outside of LIFX, as they are the only ones that implement it. Once a couple of light systems come available that support this, we can reconsider.

For now, it's the only way any of this functionality can be made available.

If each of the light components offered bundled technology outside the scope of the core light component in HA we lose the ability to control lights across platforms in a unified way.

I think that this is a tad dramatic. All functionality that is shared across multiple platforms will still be available. Just not edge cases like changing the state on a device that's off. We'll be fine.

@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants