Skip to content

Add color to light template#28141

Closed
tetienne wants to merge 16 commits into
home-assistant:devfrom
tetienne:Light_Template_Color
Closed

Add color to light template#28141
tetienne wants to merge 16 commits into
home-assistant:devfrom
tetienne:Light_Template_Color

Conversation

@tetienne
Copy link
Copy Markdown
Contributor

@tetienne tetienne commented Oct 23, 2019

Description:

Currently light template only support brightness. This PR adds colour and temperature support.

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

Example entry for configuration.yaml (if applicable):

input_boolean:
  foo_bool_input:

input_number:
  brightness_input:
    min: 0
    max: 255
  temperature_input:
    min: 0
    max: 500
  h_input:
    min: 0
    max: 360
  s_input:
    min: 0
    max: 360

light:
  - platform: template
    lights:
      foo:
        turn_on:
          service: input_boolean.turn_on
          entity_id: input_boolean.brightness_input
        turn_off:
          service: input_boolean.turn_off
          entity_id: input_boolean.brightness_input
        level_template: "{{states('input_number.brightness_input') | int }}"
        value_template: "{{states('input_number.brightness_input') | int > 0}}"
        set_level:
          service: input_number.set_value
          data_template:
            value: "{{ brightness }}"
            entity_id: input_number.brightness_input
        temperature_template: "{{states('input_number.temperature_input') | int}}"
        set_temperature:
          service: input_number.set_value
          data_template:
            value: "{{ color_temp }}"
            entity_id: input_number.temperature_input
        color_template: "({{states('input_number.h_input') | int}}, {{states('input_number.s_input') | int}})"
        set_color:
          - service: input_number.set_value
            data_template:
              value: "{{ h }}"
              entity_id: input_number.h_input
          - service: input_number.set_value
            data_template:
              value: "{{ s }}"
              entity_id: input_number.s_input

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.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@probot-home-assistant
Copy link
Copy Markdown

Hey there @PhracturedBlue, mind taking a look at this pull request as its been labeled with a integration (template) you are listed as a codeowner for? Thanks!

@tetienne
Copy link
Copy Markdown
Contributor Author

tetienne commented Oct 24, 2019

Checks failed because of black. When I run on it on my machine, everything is fine.

 black --fast homeassistant
All done! ✨ 🍰 ✨
2667 files left unchanged.

@frenck
Copy link
Copy Markdown
Member

frenck commented Oct 24, 2019

@tetienne Please rebase your PR onto the latest dev, that should fix the issue.

https://developers.home-assistant.io/docs/en/development_catching_up.html

@tetienne
Copy link
Copy Markdown
Contributor Author

tetienne commented Nov 5, 2019

Documentation added.
black step is now ok after a rebase with the dev branch.
About the test fail, I don't get it why it's ok on my machine with 3.7 but not o Azure :S

@tetienne tetienne force-pushed the Light_Template_Color branch from d10770b to 887dbf3 Compare November 6, 2019 21:04
@tetienne
Copy link
Copy Markdown
Contributor Author

tetienne commented Nov 9, 2019

Hi, this PR is ready for review.

@tetienne
Copy link
Copy Markdown
Contributor Author

@frenck Doc is not anymore missing.

@tetienne
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare Hi, can someone review my PR please?

@MartinHjelmare
Copy link
Copy Markdown
Member

When someone has time, someone will review this PR.

@tetienne tetienne force-pushed the Light_Template_Color branch from 44e0e5d to 009de07 Compare December 2, 2019 21:02
@tetienne
Copy link
Copy Markdown
Contributor Author

tetienne commented Dec 2, 2019

@PhracturedBlue @grillp What do you think about my enhancements?

@tetienne tetienne force-pushed the Light_Template_Color branch from 009de07 to f0d2cfe Compare December 9, 2019 09:53
@tetienne
Copy link
Copy Markdown
Contributor Author

tetienne commented Dec 9, 2019

@balloob @MartinHjelmare @frenck I'm really sorry to ping you again, but it become really difficult to maintain and rebase my PR. Can someone have a look please?

@tetienne tetienne force-pushed the Light_Template_Color branch from f0d2cfe to 7fac622 Compare December 9, 2019 10:04
Comment thread homeassistant/components/template/light.py Outdated

return 0
supported_features |= SUPPORT_BRIGHTNESS
if self._temperature_script is not None:
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.

Interesting that this is based off the existence of a script, not on a value template ?

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.

Indeed, I just follow the existing solution. But IMO it seems logic: you need an action to be able to change the brightness/color/temperature.

)
elif ATTR_HS_COLOR in kwargs and self._color_script:
hs_value = kwargs[ATTR_HS_COLOR]
await self._color_script.async_run(
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.

Why are all these different scripts? Since it is all done inside the async_turn_on method, shouldn't there just be a generic turn_on script ?

Copy link
Copy Markdown
Contributor Author

@tetienne tetienne Dec 10, 2019

Choose a reason for hiding this comment

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

All these scripts map the actions to call when the brightness/color/temperature is updated. See https://www.home-assistant.io/integrations/light.template

Comment thread homeassistant/components/template/light.py Outdated
Comment thread homeassistant/components/template/light.py Outdated
_LOGGER.error(ex)
self._state = None
"""Update from templates."""
await self.update_state()
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.

Instead of making these awaitable, you can mark them as @callback which means you don't need to yield.

Copy link
Copy Markdown
Contributor Author

@tetienne tetienne Dec 9, 2019

Choose a reason for hiding this comment

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

So, I can make update_state() not async also?

 @callback
  def update_state(self):
        """Update the state from the template."""
        if self._template is not None:
          ...
``

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 don't get the magic behind, but indeed it works well. Thx.

Comment thread homeassistant/components/template/light.py Outdated
@tetienne
Copy link
Copy Markdown
Contributor Author

@balloob I've fixed your remarks.

@tetienne
Copy link
Copy Markdown
Contributor Author

@balloob Can you pleare continue the review of my PR?

@tetienne tetienne requested a review from balloob December 30, 2019 07:36
@tetienne
Copy link
Copy Markdown
Contributor Author

tetienne commented Jan 3, 2020

I will split this PR in smaller ones. First one: #30455

try:
render = self._color_template.async_render()
h_str, s_str = map(
int, render.replace("(", "").replace(")", "").split(",", 1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should be float

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

From a hue lamp
FireShot Capture 032 - Home Assistant - hassio lan

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.

Oh I didn't know. I will change this.

@tetienne
Copy link
Copy Markdown
Contributor Author

2nd PR: #30595

@tetienne
Copy link
Copy Markdown
Contributor Author

tetienne commented Feb 3, 2020

3rd PR: #31435

@tetienne tetienne closed this Feb 3, 2020
@tetienne tetienne deleted the Light_Template_Color branch February 3, 2020 13:10
@lock lock Bot locked and limited conversation to collaborators Feb 4, 2020
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