Hyperion: Add brightness, HDMI and effect support by starkillerOG · Pull Request #11543 · home-assistant/core

starkillerOG · 2018-01-09T09:00:39Z

Description:

added brightness support to dim the hyperion light
changed the "OFF" command to set the color to [0,0,0] after clearing all priorities.
This is neccesary to keep the light turned off when an HDMI grabber is used for ambilight with hyperion.
added HDMI ambilight mode recognition and control.
by setting the "hdmi_priority" in your "configuration.yaml" file (defaults to 880), home assistant will now be able to recognize when the hyperion light is in HDMI ambilight mode and will change its icon to an HDMI symbol and set the status to ON.
Switching the hyperion light to HDMI ambilight mode can be done through the effect option (clears all priorities such that the HDMI grabber remains).
added effect support for the default effects of hyperion, a custom list can be defined in the "configuration.yaml" file by using the "effect_list" option.

Related issue (if applicable): fixes #

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

Example entry for `configuration.yaml` (if applicable):

 - platform: hyperion
   host: IP_ADRESS
   port: 19444
   name: hyperion light
   priority: 100
   hdmi_priority: 880
   default_color: [0,0,255]
   effect_list: ['HDMI', 'Blue mood blobs']

Checklist:

The code change is tested and works locally.

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

Documentation added/updated in home-assistant.github.io

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.

- added brightness support to dim the hyperion light - changed the "OFF" command to set the color to [0,0,0] after clearing all priorities. This is neccesary to keep the light turned off when an HDMI grabber is used for ambilight with hyperion. - added HDMI ambilight mode recognition and control. by setting the "hdmi_priority" in your "configuration.yaml" file (defaults to 880), home assistant will now be able to recognize when the hyperion light is in HDMI ambilight mode and will change its icon to an HDMI symbol and set the status to ON. Switching the hyperion light to HDMI ambilight mode can be done through the effect option (clears all priorities such that the HDMI grabber remains). - added effect support for the default effects of hyperion, a custom list can be defined in the "configuration.yaml" file by using the "effect_list" option.

- added brightness support to dim the hyperion light - changed the "OFF" command to set the color to [0,0,0] after clearing all priorities. This is neccesary to keep the light turned off when an HDMI grabber is used for ambilight with hyperion. - added HDMI ambilight mode recognition and control. by setting the "hdmi_priority" in your "configuration.yaml" file (defaults to 880), home assistant will now be able to recognize when the hyperion light is in HDMI ambilight mode and will change its icon to an HDMI symbol and set the status to ON. Switching the hyperion light to HDMI ambilight mode can be done through the effect option (clears all priorities such that the HDMI grabber remains). - added effect support for the default effects of hyperion, a custom list can be defined in the "configuration.yaml" file by using the "effect_list" option. - fixed some style issues with too long lines

- fixed some more indentation style issues

- yet more fixed visuel indent issues

- more visuel indents

- fixed invalid variable "A"

- remove unnececary brackets - specify specific exceptions

starkillerOG · 2018-01-09T09:07:03Z

@tschmidty69 It seems like my code has passed all the checks.
Is it now ready to be merged, or do I need to do anything else?

starkillerOG · 2018-01-09T09:09:52Z

@tschmidty69
What does that "Local tests with tox run successfully" mean?
I have tested my code on my hyperion and home assistant and it works flauwless.

todschmidt · 2018-01-09T13:39:56Z

Read this page: https://home-assistant.io/developers/development_testing/

If you look at the CI testing check below you can see all it is doing is the same thing tox is doing but in the cloud as opposed to locally

As far as getting it accepted, it just needs to be reviewed by one of the core team. It may take some time. One of the things they may ask you to do is to write tests for your component.

starkillerOG · 2018-01-09T17:00:12Z

@tschmidty69 thanks for the info.
I will then wait untill I hear something or it is merged automatically.

pvizeli · 2018-01-11T22:31:11Z

+        self._hdmi_priority = hdmi_priority
        self._default_color = default_color
        self._rgb_color = [0, 0, 0]
+        self._rgb_mem = [0, 0, 0]


why you need this?

The "self._default_color" is used to set the color for the first time when hass is just booted. (after switching off again and switching on the color will be remembered and set to the previous value).
I changed this behaviour, in the previous (current) version the color is not remembered and after switching off and back on the color was reset to this default color.
I agree that this "self._default_color" is not perticularly nessesary because it is only used once after a reboot of hass, but I kept it because it was in the previous release and why not keep it.

The "self._rgb_color" and "self._rgb_mem" originate from the fact that Home Assistant and Hyperion use to diffrent definitions of RGB color. In Home Assistant an "RGB" color contains information about the Hue value (angle in the round color picker) and the saturation value (radius in the round color picker) but does not contain information about the brightness. Therefore in the HASS definition of "RGB" at least one of the values R, G or B is always 255 (exept when the light is off). The brightness is defined in a seperate value in home assistant.

However Hyperion uses the "true" RGB color definition: it contains the information about hue, saturation and brightness. Hyperion does not have any seprate brightness control. Therefore to set for instance a lower brighness pure blue color you simply sent [0,0,150] as RGB color.

It turns out Home Assistant misbehaved if i started to set self._rgb_color to values such as [0,0,150]. It wants a value of [0,0,255] for blue and then a brightness set to 150 to dim it. That is why I needed both values saved in memory. The "self._rgb_color" represents the HASS definition and "self._rgb_mem" is the Hyperion definition of RGB.

Furthermore when the light is turned off, the "self._rgb_color" is set to [0,0,0], therefore losing the information about the previous color. I wanted to store the previous color in memory "self._rgb_mem", so I can sent that color when the light is turend back on.

MartinHjelmare

Cache the argument in a local variable that you use when sending the json request. Don't change the instance attributes in the service methods. See below.

MartinHjelmare · 2018-01-12T08:04:42Z

            self._rgb_color = self._default_color
+            self._rgb_mem = self._default_color
+        else:
+            self._rgb_color = self._rgb_mem


You should not change any state holding instance attributes in the service methods. Those will be updated in update after the service call, since this is a polling entity.

I agree, I will update in a commit in a second

MartinHjelmare · 2018-01-12T08:04:52Z

+            self._rgb_color = self._rgb_mem
+
+        if ATTR_BRIGHTNESS in kwargs:
+            self._brightness = kwargs[ATTR_BRIGHTNESS]


Same as above.

I agree, I will update in a commit in a second

MartinHjelmare · 2018-01-12T08:07:34Z

+            self._brightness = kwargs[ATTR_BRIGHTNESS]

+        if ATTR_EFFECT in kwargs:
+            self._effect = kwargs[ATTR_EFFECT]


Same as above.

The Effect case is special, Hyperion takes to long to activate an effect and update its internal status. Therefore I experianced that when you activate an effect, Hyperion will start activating that effect, but imediatly HASS will ask for an update of the status, and because Hyperion is not yet done with activating the effect it will return a json staing that Hyperion is OFF. in the second time an update is requested by HASS the correct json response will be sent by Hyperion and the state will display correctly.

Therfore HASS behaved as followed: you turn on an effect, HASS will imediatly show the light is turned off, and a few seconds later HASS will show the light is turned on again and the correct effect is displayed.

This is super anying, therfore I have used "self._skip_check = True", this will cause the first update HASS requests imidiatly after you select an effect to be aborted. The second and following update requests will be done as usual. If some strange thing happed, HASS will always correct the state in the second update request, but I have not seen that this was necassary.

To display the correct Effect in HASS imediatly after you select an effect I do need to update the self._ properties in the special case of effects.

MartinHjelmare · 2018-01-12T08:08:04Z

+            self._effect = kwargs[ATTR_EFFECT]
+            if self._effect == 'HDMI':
+                self.json_request({'command': 'clearall'})
+                self._icon = 'mdi:video-input-hdmi'


Same as above.

I disagree, see above: effect special case.

MartinHjelmare · 2018-01-12T08:08:16Z

+            if self._effect == 'HDMI':
+                self.json_request({'command': 'clearall'})
+                self._icon = 'mdi:video-input-hdmi'
+                self._brightness = 255


Same as above.

I disagree, see above: effect special case.

MartinHjelmare · 2018-01-12T08:08:26Z

+                self.json_request({'command': 'clearall'})
+                self._icon = 'mdi:video-input-hdmi'
+                self._brightness = 255
+                self._rgb_color = [125, 125, 125]


Same as above.

I disagree, see above: effect special case.

MartinHjelmare · 2018-01-12T08:09:26Z

+            'priority': self._priority,
+            'color': [0, 0, 0]
+        })
        self._rgb_color = [0, 0, 0]


Same as above.

I agree, I will update in a commit in a second

@MartinHjelmare

Proccesed the comments of @MartinHjelmare: home-assistant#11543 (review)

houndci-bot · 2018-01-12T22:06:24Z

+        if ATTR_EFFECT in kwargs:
+            self._skip_check = True
+			self._effect = kwargs[ATTR_EFFECT]
+            if self._effect == 'HDMI':


indentation contains mixed spaces and tabs

houndci-bot · 2018-01-12T22:06:24Z


+        if ATTR_EFFECT in kwargs:
+            self._skip_check = True
+			self._effect = kwargs[ATTR_EFFECT]


indentation contains tabs
indentation contains mixed spaces and tabs
unexpected indentation
TabError: inconsistent use of tabs and spaces in indentation

corrected tab instead of 4 spaces

starkillerOG · 2018-01-12T22:17:10Z

@MartinHjelmare I have changed some of the things you requested, thanks for the feedback.
However as I explain below, setting an effect is a special case. Do you agree that in this case it is the only option to update the state properties in the service, to display what is happening to the light correctly?

The Effect case is special, Hyperion takes to long to activate an effect and update its internal status. Therefore I experianced that when you activate an effect, Hyperion will start activating that effect, but imediatly HASS will ask for an update of the status, and because Hyperion is not yet done with activating the effect it will return a json staing that Hyperion is OFF. in the second time an update is requested by HASS the correct json response will be sent by Hyperion and the state will display correctly.

Therfore HASS behaved as followed: you turn on an effect, HASS will imediatly show the light is turned off, and a few seconds later HASS will show the light is turned on again and the correct effect is displayed.

This is super anying, therfore I have used "self._skip_check = True", this will cause the first update HASS requests imidiatly after you select an effect to be aborted. The second and following update requests will be done as usual. If some strange thing happed, HASS will always correct the state in the second update request, but I have not seen that this was necassary.

To display the correct Effect in HASS imediatly after you select an effect I do need to update the self._ properties in the special case of effects. The light itself does react imidiatly so the state schould be set imediatly, however Hyperion takes some time to update its own state and sent the correct response, but it does control the lights without delay.

MartinHjelmare · 2018-01-12T22:28:34Z

Yeah, that sounds ok, to me. 👍

starkillerOG · 2018-01-12T22:37:11Z

@MartinHjelmare Could you then change the review such that the red "Changes requested" will disapear?

Can the pull request now be merged, or does something else need to happen?

MartinHjelmare · 2018-01-12T22:39:47Z

+        self._brightness = 255
+        self._icon = 'mdi:lightbulb'
+        self._effect_list = effect_list
+        self._effect = 'none'


This should be None.

Fixed in new commit

MartinHjelmare · 2018-01-12T22:40:26Z

+        self._icon = 'mdi:lightbulb'
+        self._effect_list = effect_list
+        self._effect = 'none'
+        self._skip_check = False


Rename this to self._skip_update.

Fixed in new commit

MartinHjelmare · 2018-01-12T22:44:57Z

            if response['info']['activeLedColor'] == []:
-                self._rgb_color = [0, 0, 0]
+                # Get the active effect
+                if response['info']['activeEffects'] != []:


What's the aim of this check? Checking if it's not a list?

the Json messages that HASS receives from Hyperion when the 'serverinfo' command is sent, contains a lot of information about the status of Hyperion in a dictonary.
It always contains the keys ['info']['activeEffects']. If Hyperion is not playing any effects like flashing the lights in all kind of ways or displaying rainbows etc. the ['info']['activeEffects'] contains an empty array: []. In that case the hyperion instance is doing something else like displaying a solid color (contained in the ['info']['activeLedColor'] key) or in ambilight mode (both ['info']['activeEffects'] and ['info']['activeLedColor'] are empty arrays and the ['info']['priorities'] key contains the specific priority for the HDMI grabber (or Kodi grabber)).

Therfore if the ['info']['activeEffects'] is not an empty array, it means that some kind of effect is playing. I will then try to retrive which effect is playing using the file-name of the script that is beeing played.

That check is thererefore ment to indicate if hyperion is playing an effect or not.

MartinHjelmare · 2018-01-12T22:45:52Z

+                        self._effect = [x for x in self._effect_list
+                                        if s_name.lower() in x.lower()][0]
+                    except (KeyError, IndexError):
+                        self._effect = 'none'


Fixed in new commit

MartinHjelmare · 2018-01-12T22:46:02Z

+                else:
+                    self._rgb_color = [0, 0, 0]
+                    self._icon = 'mdi:lightbulb'
+                    self._effect = 'none'


Same as above.

Fixed in new commit

MartinHjelmare · 2018-01-12T22:46:33Z

+                self._rgb_mem = [int(round(float(x)*255/self._brightness))
+                                 for x in self._rgb_color]
+                self._icon = 'mdi:lightbulb'
+                self._effect = 'none'


Same as above.

Fixed in new commit

- changed 'none' to None - renamed "self._skip_check" to "self._skip_update"

changed checking if a list is empty from "list == []" to "not list"

starkillerOG · 2018-01-13T13:55:24Z

@MartinHjelmare I adapted all the changes you requested

Could you againg change the review such that the red "Changes requested" will disapear?
Can the pull request now be merged, or does something else need to happen?

MartinHjelmare

Looks good!

pvizeli · 2018-01-14T08:09:02Z

The client library have no push function?

starkillerOG · 2018-01-18T20:29:51Z

I just noticed that I made a mistake in my last commit.
The self._brightness has been removed and named brightness.
However if the brightness is not in the current command this breaks the turn_on action because it is required for the cal_rgb.
I fixed this issue in my new pull request:
#11772

However I need help with merging this fix.
Can someone resolve the conflict and merge the fix.
It is only 2 aditional small lines of code.

In the last commit the self._brightness has been removed and named brightness. However if the brightness is not in the current command this breaks the turn_on action because it is required for the cal_rgb. This fixes that by using the self._brightness if it is not in the current command. My last commit is alreay merged in this pull request: home-assistant#11543

starkillerOG added 7 commits January 8, 2018 18:01

Hyperion: Add brightness, HDMI and effect support

3a975b2

- fixed some more indentation style issues

Hyperion: Add brightness, HDMI and effect support

b9c44fd

- yet more fixed visuel indent issues

Hyperion: Add brightness, HDMI and effect support

192f6b9

- more visuel indents

Hyperion: Add brightness, HDMI and effect support

b46bb39

- fixed invalid variable "A"

Hyperion: Add brightness, HDMI and effect support

4061922

- remove unnececary brackets - specify specific exceptions

homeassistant added platform: light.hyperion cla-signed labels Jan 9, 2018

balloobbot added platform via-github labels Jan 9, 2018

This was referenced Jan 9, 2018

Hyperion: Add brightness, HDMI and effect support #11532

Closed

Hyperion light, new features home-assistant/home-assistant.io#4376

Merged

pvizeli reviewed Jan 11, 2018

View reviewed changes

MartinHjelmare requested changes Jan 12, 2018

View reviewed changes

correct changing state holding attributes during a service method

d1e5b2a

Proccesed the comments of @MartinHjelmare: home-assistant#11543 (review)

homeassistant added the cla-signed label Jan 12, 2018

houndci-bot reviewed Jan 12, 2018

View reviewed changes

indent correction

6da5939

corrected tab instead of 4 spaces

home-assistant deleted a comment from starkillerOG Jan 12, 2018

MartinHjelmare requested changes Jan 12, 2018

View reviewed changes

Hyperion: Add brightness, HDMI and effect support

e0e39c7

- changed 'none' to None - renamed "self._skip_check" to "self._skip_update"

homeassistant added the cla-signed label Jan 13, 2018

Add brightness, HDMI and effect support

687fd67

changed checking if a list is empty from "list == []" to "not list"

MartinHjelmare approved these changes Jan 13, 2018

View reviewed changes

MartinHjelmare merged commit cdbf2f9 into home-assistant:dev Jan 13, 2018

starkillerOG mentioned this pull request Jan 18, 2018

Hyperion: Fix fatal error in last merge #11772

Closed

9 tasks

This was referenced Jan 18, 2018

Hyperion: Fix fatal error in last merge #11777

Closed

Hyperion: Fix fatal error in last merge #11793

Merged

balloob mentioned this pull request Jan 26, 2018

0.62 #11921

Merged

a-andre mentioned this pull request Mar 25, 2018

Hyperion raises error since 0.65 #13118

Closed

home-assistant locked and limited conversation to collaborators May 29, 2018

ghost added integration: hyperion and removed platform: light.hyperion labels Mar 21, 2019

Uh oh!

Conversation

starkillerOG commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

starkillerOG commented Jan 9, 2018

Uh oh!

starkillerOG commented Jan 9, 2018

Uh oh!

todschmidt commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

starkillerOG commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starkillerOG commented Jan 12, 2018

Uh oh!

MartinHjelmare commented Jan 12, 2018

Uh oh!

starkillerOG commented Jan 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starkillerOG commented Jan 9, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

todschmidt commented Jan 9, 2018 •

edited

Loading