core Decouple Hyperion entitites and clear source when light is off

Breaking change

Be aware that with this update to the Hyperion integration, turning off the light entity will only clear the priority you have configured during integration setup. Other HA-independent light sources in Hyperion might still be active and cause light to be emitted. In order to turn the light output off entirely regardless of active light sources, you can enable the switch.[instance]_led_device entity that acts as a global switch. See the updated documentation for more details.

Proposed change

The current state of Hyperion light entities makes custom configurations very hard because there is inbuilt strong coupling between the different components:

the default light entity will actually control 3 different things:
- when turning on:
  - the overall Hyperion instance state (switch.my_instance_all)
  - the LED device state (switch.my_instance_led_device)
  - create a light source at the configured priority
- when turning off:
  - it will only switch off the LED device state (switch.my_instance_led_device)
  - it will not clear the opened light source at the configured priority - it will stay active and there is absolutely no way of clearing it from HA. I consider this a bug.
- this behaviour has multiple disadvantages:
  - other light sources, e.g. a constant background "ambient" light will not be shown anymore. Another example is you want to watch a movie on your HTPC but the light from the HTPC screen grabber's configured priority will not be shown as your LED device has been turned off.
  - if you turn the LED device back on because you want other light sources being able to connect and light up, the previous HA priority will still be there and can take priority over other sources.

Screenshot 2022-10-17 at 19 23 28 Screenshot 2022-10-17 at 17 18 57

Proposed Change ✅:

remove strong coupling between light entity, LED device and overall Hyperion state
light entity will create a priority channel light source in Hyperion when turned on and will clear that channel when turned off - nothing else
full customization: the previous behaviour can now be partially or entirely reconstructed by using automations or a template light (if you want even the state of the entity to represent multiple entities' states)
all the entities are decoupled and can be individually controlled

Note: during https://github.com/home-assistant/core/pull/45410 an additional "Priority Light" was introduced that would send a black color when being turned off. As this results in the priority being ignored by Hyperion it had the same effect as clearing the priority but would maintain an active connection on that priority channel. Clearing the priority is the proper thing to do, therefore this light is not required any longer. The priority light entity has therefore be unified with the main entity.

As a positive side effect, the code of this integration is simplified significantly.

@dermotduffy your feedback is welcome and highly appreciated!

Type of change

[ ] Dependency upgrade
[x] Bugfix (non-breaking change which fixes an issue)
[ ] New integration (thank you!)
[ ] New feature (which adds functionality to an existing integration)
[ ] Deprecation (breaking change to happen in the future)
[x] Breaking change (fix/feature causing existing functionality to break)
[ ] Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes https://github.com/home-assistant/core/issues/77020
This PR is related to issue: https://github.com/home-assistant/core/issues/41892
Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/24604

Checklist

[x] The code change is tested and works locally.
[x] Local tests pass. Your PR cannot be merged unless tests pass
[x] There is no commented out code in this PR.
[x] I have followed the development checklist
[x] The code has been formatted using Black (black --fast homeassistant tests)
[x] Tests have been added to verify that the new code works.

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

[x] Documentation added/updated for www.home-assistant.io

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

[ ] The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
[ ] New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
[ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
[ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

[ ] I have reviewed two other open pull requests in this repository.

Oct 17 '22 17:10 Sab44

Hi Sab44

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!

Oct 17 '22 17:10 home-assistant[bot]

Hey there @dermotduffy, mind taking a look at this pull request as it has been labeled with an integration (hyperion) you are listed as a code owner for? Thanks!

Oct 17 '22 17:10 home-assistant[bot]

Hi @Sab44 ,

Thanks for the contribution! As I think this is may be your first HA core contribution, please note that my approval is neither necessary nor sufficient for this PR to be accepted, so the ultimate conclusion will come from someone with merging privileges.

Firstly, I agree with your analysis here -- but am unconvinced this PR constitutes an improvement for many, or most, users. To me, the situation stems from multiple equally valid ways of interpreting off. For the HA Hyperion integration off could mean any of these things:

The Hyperion device outputs nothing no matter what (arguably the traditional definition of 'off').
The Hyperion device outputs 'black' to let HA (if it's the winning priority) set the LED device to black to give the impression of being 'off'.
The Hyperion device takes no particular input from HA at all (so the light could be 'on' for some other input).

The integration today has (1) as the default, and (2) as the functionality offered by the extra priority light the integration supports. This PR proposes (3).

My concerns with (3):

I suspect this is a breaking change for a significant majority of users. Previously when they turn their Hyperion light off, it goes off -- no matter what. With this PR, it very likely will not go off (i.e. any other active priority in Hyperion will keep the light on, including system effects that are default enabled). Unless they are an advanced Hyperion user, this is probably not what they expect as a HA user.
Because of the former, it seems like an unreasonable burden to ask these users to create template lights or automations to get off to mean off like it already does today.
If the user sets an input like 'USB Capture' to sync the lighting with TV input (probably a common use of Hyperion?), it will come on at a stock Hyperion priority (default: 240). With this PR when the user attempts to turn off that same light, it will have no effect since the default HA priority (128) will be cleared.
This issue was extensively litigated in https://github.com/home-assistant/core/issues/41892 where there was pushback from the HA team on adding a 'non-absolute' definition of 'off' -- which this PR steers directly into.

If you think I've missed anything in the above let me know. My alternative suggestion is simple (although still runs afoul of concern 4 above):

Add a HyperionPurePriorityLight entity, that is disabled by default but which implements (3) optionally, for advanced Hyperion users who have multiple inputs and know what they're doing. This comes with all the benefits you rightly list in this PR (except code simplicity), but also does not break the integration for the average HA user.

Oct 18 '22 03:10 dermotduffy

Hi @dermotduffy

first of all thank you for your swift and in-depth reply.

I understand your argumentation and in a previous iteration I did consider following your proposal of adding another entity. Let me explain why I decided against this.

I suspect this is a breaking change for a significant majority of users. Previously when they turn their Hyperion light off, it goes off -- no matter what. With this PR, it very likely will not go off (i.e. any other active priority in Hyperion will keep the light on, including system effects that are default enabled). Unless they are an advanced Hyperion user, this is probably not what they expect as a HA user.

We do have the switch.my_instance_led_device entity already available which will turn off the LED device. Just like previously the light entity did. I do agree it will be a change in behaviour and I'm willing to write a "Breaking Change" text if this change qualifies for that designation.

This is just my opinion, but shying away from introducing change at the cost of a convoluted code base should not be the driver of our development. Having now another disabled-by-default entity makes this integration somewhat unintuitive to get into and is (again, my opinion) a band-aid to tackle the strong coupling issue we have.

Because of the former, it seems like an unreasonable burden to ask these users to create template lights or automations to get off to mean off like it already does today.

Why do you think so? The inner workings of the integration are now decoupled from each other and should be easier to understand and customize via automations according to users' preferences. In order to simplify this I'm also willing to expand the documentation to provide better description of each entity and example automations on how to combine them. Creating such a simple automation nowadays should no longer qualify as advanced HA usage I suppose.

If the user sets an input like 'USB Capture' to sync the lighting with TV input (probably a common use of Hyperion?), it will come on at a stock Hyperion priority (default: 240). With this PR when the user attempts to turn off that same light, it will have no effect since the default HA priority (128) will be cleared.

That is kind of the point of this PR though. The 'USB Capture' should be decoupled from the light input that HA as a client can provide. It is a different input source and in my opinion should not be mixed into the light entity. It can still be turned off via the switch.my_instance_led_device just like before. We also have switch.my_instance_component_usb_capture which should do the job, although I haven't tested this myself.

This issue was extensively litigated in Turning off Hyperion light via Home Assistant turns off Hyperion for other sources #41892 where there was pushback from the HA team on adding a 'non-absolute' definition of 'off' -- which this PR steers directly into.

I see, maybe that's also a point where I would like to get some more feedback. The difficulty we face here is interfacing HA components (light & switch entities) with the Hyperion components (priorities, toggles for different components). If we can agree on merging this PR I could imagine further developing this integration to allow for better usability. For example, I'm thinking about an entity that fetches all active priorities and allows the user to manage them from HA directly. But I think this is out-of-scope here.

Again, thank you for your feedback, I could see us having different opinions here and I definitely don't want to create a back-and-forth argument. Maybe someone else can provide their feedback as well? If the majority opinion is to go the conservative route and implement an additional light entity as proposed, I will not object and will implement this myself.

Oct 20 '22 08:10 Sab44

This is just my opinion, but shying away from introducing change at the cost of a convoluted code base should not be the driver of our development.

Completely agree! I think a better principle could be: Our codebase should be just complicated enough to meet the needs of the majority of our target users, and no more.

Why do you think so? The inner workings of the integration are now decoupled from each other and should be easier to understand and customize via automations according to users' preferences. In order to simplify this I'm also willing to expand the documentation to provide better description of each entity and example automations on how to combine them. Creating such a simple automation nowadays should no longer qualify as advanced HA usage I suppose.

To me, the question is: Should we expect a new "vanilla" Hyperion user / "vanilla" HA user to need to create an automation to meet a behavior they likely expect to work out of the box? I think the answer is "No" and that this is in keeping with the spirit of trying to lower the barrier to entry to Home Assistant. I don't know how we could -- with a straight face -- explain to a new user that turning off the light in HA leaves their Hyperion light visibly on, but that we did this because it allowed us to decouple the implementation (my argument: they don't care).

That is kind of the point of this PR though. The 'USB Capture' should be decoupled from the light input that HA as a client can provide. It is a different input source and in my opinion should not be mixed into the light entity. It can still be turned off via the switch.my_instance_led_device just like before. We also have switch.my_instance_component_usb_capture which should do the job, although I haven't tested this myself.

I can see this argument, yes. It sort of makes the case to the user that the USB Capture isn't a light at all, and so the integration does not treat it as one. It's just a little awkward, since it does result in visible light... :-)

I see, maybe that's also a point where I would like to get some more feedback. The difficulty we face here is interfacing HA components (light & switch entities) with the Hyperion components (priorities, toggles for different components). If we can agree on merging this PR I could imagine further developing this integration to allow for better usability. For example, I'm thinking about an entity that fetches all active priorities and allows the user to manage them from HA directly. But I think this is out-of-scope here.

Good ideas! Any path that simplifies the code and doesn't violate the "Principle of least surprise" for a vanilla user, will have my vote. It's pretty awkward though for the reasons you highlight -- there's just a different model in HA and Hyperion, and so where they meet may always be challenging.

Again, thank you for your feedback, I could see us having different opinions here and I definitely don't want to create a back-and-forth argument. Maybe someone else can provide their feedback as well? If the majority opinion is to go the conservative route and implement an additional light entity as proposed, I will not object and will implement this myself.

Absolutely, thank you for the great discussion and contribution. As stated at the outset, my opinion is actually much less relevant than the repository maintainers who have the right to merge or close PRs. We can also invite more Hyperion users to give their feedback, but I'd suggest to save time and keep focused on what matters (the opinion of someone who can merge!) we wait for a maintainer to weigh in first since they may have a strong opinion in one direction or the other (i.e. see my prior linked issue).

Oct 25 '22 01:10 dermotduffy

For people reading this, I would just like to add a few notes:

for me as a new user to the Hyperion integration, it was very confusing in the beginning that turning off the light would turn off the LED device entirely - I expected it to clear its priority that I defined when setting up the integration
turning off the LED device is still entirely possible for users who want to do so, simply by using the available switch entity - we could consider enabling this entity by default to increase user friendliness
I feel like with this MR we are both creating an intuitive to use integration (although this is a matter of opinion) and prepare the codebase for further development to improve integration with Hyperion's control system

Regarding automations for people who would like to couple the LED device toggle to their light entity, these would be very straightforward, similar to the following:

alias: LED Device turn on with light
description: ""
trigger:
  - platform: state
    entity_id:
      - light.my_instance
    from: "off"
    to: "on"
condition:
  - condition: state
    entity_id: switch.my_instance_component_led_device
    state: "off"
action:
  - service: switch.turn_on
    target:
      entity_id: switch.my_instance_component_led_device
mode: single

and vice versa for turning the entity off. As said in my previous comment, I could add example cases like this automation to the integration's documentation just to showcase what is possible.

Nov 04 '22 12:11 Sab44

Please correct my if i'm wrong, but there used to be an old Hyperion integration only compatible with Hyperion Xlassic (not Hyperion NG), which somewhat used to work like described in this PR. Turning off Hyperion in HA just turned off / cleared the Home Assistant Instance. That's the reason I created #41892 back in the day.

Nov 04 '22 16:11 7h30n3

Please correct my if i'm wrong, but there used to be an old Hyperion integration only compatible with Hyperion Xlassic (not Hyperion NG), which somewhat used to work like described in this PR.

The old integration used 'off method' #2, which is still supported today in the priority light. This PR is 'off method' #3 which is not supported at all today. It could be added separately, but this PR entirely removes 1 & 2 in favor of 3.

Nov 05 '22 01:11 dermotduffy

Please correct my if i'm wrong, but there used to be an old Hyperion integration only compatible with Hyperion Xlassic (not Hyperion NG), which somewhat used to work like described in this PR. Turning off Hyperion in HA just turned off / cleared the Home Assistant Instance. That's the reason I created #41892 back in the day.

I agree, this PR is related with your issue #41892 (and also the issue I created #77020). We came to the same conclusion that something is not right that when you turn of your HA light entity the LED device is turned off and any other non-HA input is ignored. I think the response to the issue you created aimed to resolve this, but it did so in a way that ended up somewhere in the middle:

The priority light entity was created: this basically solved the underlying issue, but it used a workaround of sending a black color instead of clearing it's priority. You still have no way of ever clearing the HA priority from Hyperion.
Next, among others, the switch entity for the LED device was added. This kind of made coupling the default light entity to the LED device obsolete, since now you have the switch entity for the LED device.

This PR aims to resolve and simplify all this. As another example of Hyperion input, we can take a look at the Hyperion Free app on the Google Play Store https://play.google.com/store/apps/details?id=nl.hyperion.hyperionfree which handles it exactly this way: create priority when activated and send color / effect. When turning off in the app, clear priority channel.

Nov 05 '22 09:11 Sab44

As another example of Hyperion input, we can take a look at the Hyperion Free app on the Google Play Store https://play.google.com/store/apps/details?id=nl.hyperion.hyperionfree which handles it exactly this way: create priority when activated and send color / effect. When turning off in the app, clear priority channel.

To me this is apples vs oranges -- a Hyperion specific app can do whatever it wants. I'd be more interested in a comparison statement like "Here are X, Y, Z other HA integrations where turning the light off in HA will not cause the device to stop actually emitting light in the default case". Which is why I think this maybe boils down to a Home Assistant product question -- i.e. is the consistency of the entity model in HA intended to get consistency of behavior across integrations? If there's no particular expectation of that, then "light off" can mean whatever we want it to mean.

As is, my hunch is that this PR will increase the support load ("I turned my Hyperion light off, but it's still on!") and that for the (I'll suspect?) minority of HA hyperion users who have other inputs that are going direct to Hyperion -- we can support them by simply using the existing priority light entity in place today (or if there are cases where this is not working, adding a new entity that supports off mode 3).

Nov 05 '22 15:11 dermotduffy

Hey @dermotduffy I understand your concern and I absolutely agree that the experience for the newbie HA + Hyperion user should be intuitive and Hyperion should stop emitting light when the light entity is turned off.

"... turning the light off in HA will not cause the device to stop actually emitting light in the default case".

So I'm not sure if our understanding of the "default case" is the same. After a clean installation of Hyperion (tested with current version 2.0.13) no light sources are preconfigured, nor is the background effect/color enabled. Therefore for the newbie user the flow would be as follows:

set up Hyperion and the HA integration with a certain priority
at this point no light is emitted from the device in default configuration
turn the light entity on in HA -> Hyperion receives light source at configured priority -> device starts emitting light
turn the light entity off in HA -> (with this PR merged) light source will be cleared from Hyperion -> as there is no other source, the device stops emitting light

Therefore I believe we have this case covered or did I miss something here?

minority of HA hyperion users who have other inputs that are going direct to Hyperion

So assuming the newbie user enters the Hyperion interface and thinks to himself it would be nice to enable the background light as a fallback when no other source is active - in the current state of the integration, he will turn off the light in HA and the device will stop emitting light even though the background light was configured. So the user needs to go through documentation, GitHub etc. to figure out there is an existing disabled-by-default priority light that would allow him to make use of that functionality. Instead of a background light this could of course be any other intentionally configured source, for example a Kodi installation that sends ambilight data to Hyperion. This is the case I ran into, and apparently regarding issue #41892 I was not alone with this.

As is, my hunch is that this PR will increase the support load

I agree that this is a valid concern, but only for the change in behaviour that might create some confusion to existing users. Therefore I'd be more than willing to add some context to the release notes similar to e.g.

Be aware that with this update to the Hyperion integration, turning off the light entity will only clear the priority you have configured during integration setup. Other HA-independent light sources in Hyperion might still be active and cause light to be emitted. In order to turn the light output off entirely regardless of active light sources, you can enable the switch.my_instance_led_device entity that acts as a global switch.

There could be a transition phase where some existing users will notice the entity behaving differently which might require them to adapt some settings in their setup, but it will be minor adjustments and will overall end up in a cleaner, more lightweight and more flexible integration.

Nov 06 '22 13:11 Sab44

So I'm not sure if our understanding of the "default case" is the same. [...] Therefore I believe we have this case covered or did I miss something here?

I think our understanding is the same. I had thought the 'Background Effect/Color' was previously enabled by default. A fresh Hyperion install confirms your statement -- there are no priorities enabled by default. The 'Boot Effect/Color' is enabled by default though, so with this PR we'd still have users wondering why their 'off' light emits light on bootup, but I think that's quite minor.

To return to my original concerns:

Concern 1: I think this now addressed, as it will work with the default out-of-the-box installation. It will only break users, in practice, who have manually configured their Hyperion instance / use multiple priorities.
Concern 2: I think this goes away, as the assumption per above is that the burden is on a smaller set of users. It's becomes just a regular breaking change, unpleasant as they are.
Concern 3: I think this still remains, although is more of a 'product choice'. Today, users have one dashboard control (the light more-info control) that can change background colors/effects and choose inputs like USB capture. With this PR, they will lose that convenience. This is definitely more natural to an experienced Hyperion user who understands the priority scheme, but for some users (myself included), having a single control to set colors/effects/inputs is pretty handy and we lose that. Instead, users would presumably need to add separate controls to their dashboards to enable/disable USB capture.
Concern 4: Remains, but is non-deterministic and you're the one doing the hard work with the PR review process :-)

I agree that this is a valid concern, but only for the change in behaviour that might create some confusion to existing users.

Yeah. I'm not too worried about that. I think we'd want:

Release notes similar to what you suggest. I'd suggest it also include a link to ...
A discussion in the documentation, with a copy and paste automation that will restore the prior behavior (i.e. toggle the global state off when the HA priority goes to off).

In all, since this does not break default users I'm coming around to this (and it's 600 lines less code to maintain). Lets get some more feedback from other users who helped with the original issue @koying @Joeboyc2 @jes1417 @RomRider .

Nov 06 '22 17:11 dermotduffy

Hey @dermotduffy sorry for coming back to you so late, thank you for your response and happy to hear that we are aligning more and more here 🙂

Concern 3: I think this still remains, although is more of a 'product choice'. Today, users have one dashboard control (the light more-info control) that can change background colors/effects and choose inputs like USB capture. With this PR, they will lose that convenience. This is definitely more natural to an experienced Hyperion user who understands the priority scheme, but for some users (myself included), having a single control to set colors/effects/inputs is pretty handy and we lose that. Instead, users would presumably need to add separate controls to their dashboards to enable/disable USB capture.

You are correct, the functionality for setting external sources would be transferred over to the respective component switches. Although the code did not reflect that yet, so I added a couple more commits to do this (previously you would have still been able to set the external inputs, but the light entity would turn off once you pick one of these options which is certainly not a good user experience). Just to be clear, (normal) effects are still possible just as before as they're being sent on the same priority channel as static colors.

Additionally, I overhauled the documentation in the respective PR: https://github.com/home-assistant/home-assistant.io/pull/24604 with the following:

basic explanation of Hyperion's priority system
add more info about the light entity behaviour
add the proposed automation blueprint to couple the LED device to the light entity
added a "Control over external sources" section

Regarding your call for more feedback, absolutely agree. Would be great if others could provide their feedback, so if you're reading this let us know your thoughts!

I updated this PR to include the Breaking Change label and added the notes discussed earlier.

Nov 18 '22 15:11 Sab44

Hi @dermotduffy! 👋

Sorry for the ping, I hope you don't mind. This PR is currently awaiting code-owner review/approval. Could you take a look?

Thanks! 👍

../Frenck

Dec 20 '22 23:12 frenck

Approved.

Dec 21 '22 01:12 dermotduffy

Hi @frenck please let me know if there's anything still left to do that's preventing this MR from being merged. Thanks for taking care. 👍

Feb 02 '23 13:02 Sab44

Thanks @Sab44 for your great work.

As someone who previously used the hyperion-remote cli tool, it was very confusing at first to have HomeAssistant turn off the entire LED device.

On the other hand, it was also handy that it takes all Hyperion light sources into account. However, this implementation with the strict coupling, has problems with unknown source types. For example, if the redirection from one Hyperion instance to another is active, HomeAssistant thinks that a Solid color is set, although the Ambilight is active. (it apparently didn't consider the forwarded flattbuffer source) The same applies to the new audio grabber source. This slightly broke the controlling for instances that get the signal forwarded instead of using the V4L2 grabber. However, that is another topic.

So, while testing your pull request, I noticed that the controlling of the effects does not work reliably:

It can happen that the light turns off after switching from a solid color to an effect.
Sometimes turning on the light is not possible (or only after several attempts), if an effect was previously selected. (Turning on the light with a solid color worked always without any problems).
Turning on the light directly by selecting an effect is also often not possible.
The dimming of the effects often causes the light to turn off

Controlling solid colors works completely flawlessly and without any problems.

I also noticed that the integration options still show "boblightserver", "grabber" and "v4l" in the "effects to hide" dropdown. I think these entries should be removed as they are no longer relevant.

Apr 27 '23 19:04 hampoelz

Thanks @Sab44 for your great work.

As someone who previously used the hyperion-remote cli tool, it was very confusing at first to have HomeAssistant turn off the entire LED device.

On the other hand, it was also handy that it takes all Hyperion light sources into account. However, this implementation with the strict coupling, has problems with unknown source types. For example, if the redirection from one Hyperion instance to another is active, HomeAssistant thinks that a Solid color is set, although the Ambilight is active. (it apparently didn't consider the forwarded flattbuffer source) The same applies to the new audio grabber source. This slightly broke the controlling for instances that get the signal forwarded instead of using the V4L2 grabber. However, that is another topic.

So, while testing your pull request, I noticed that the controlling of the effects does not work reliably:
* It can happen that the light turns off after switching from a solid color to an effect.

* Sometimes turning on the light is not possible _(or only after several attempts)_, if an effect was previously selected. (Turning on the light with a solid color worked always without any problems).

* Turning on the light directly by selecting an effect is also often not possible.

* The dimming of the effects often causes the light to turn off
Controlling solid colors works completely flawlessly and without any problems.

Hi @hampoelz and thanks for joining the discussion. Unfortunately this PR has been stuck in limbo for so long that in the meantime I moved and do not have an ambilight setup available for testing right now. I don't know whether or when this PR is planned to be merged.

Consequently, this also means updates to the Hyperion software that happened in the meantime, such as the new audio grabber source for example, could potentially produce unexpected behaviour as you seem to have observed. I can therefore only provide some general information.

At the time of finishing this PR, Hyperion effects were working fine and as intended. There are also unit tests that should ensure this. It is of course possible that your specific setup creates those issues you observed. Feel free to explore this a bit further and if you're able to find bugs in the code, of course you're welcome to submit changes. In any case, like I said, I won't be able to test anything at the moment and for the forseeable future, so someone else would have to confirm your issues and help test potential fixes.

I also noticed that the integration options still show "boblightserver", "grabber" and "v4l" in the "effects to hide" dropdown. I think these entries should be removed as they are no longer relevant.

This might be a valid oversight, I will check it when I have time and come back to this. Thanks for letting me know!

Apr 29 '23 09:04 Sab44

Thanks @Sab44 for the quick reply!

I'll definitely do some more testing to identify the bugs that are currently occurring. Unfortunately I'm not familiar with the code at the moment. Maybe there is some more time in the next weeks/months.

However, I just noticed that when setting or changing an effect, it is cleared first. (light.py#L270-L275) According to the code comment due to the issue hyperion-project/hyperion.ng#992. However, this seems to be already fixed.

After removing the workaround, it seems that now almost all the issues I mentioned are fixed. And that even makes sense somehow, because sometimes when setting and changing the effect, the light was turned off. In other words, it seems that the effect was cleared, but not set again or something. And it also explains why these issues only occur when using effects.

The only thing I noticed is that turning off the light with an active effect didn't work the first time sometimes.

But there are a few things that I still don't quite understand:

The issue was apparently already fixed in Hyperion alpha.9, and although the minimum recommended version since the first commit in this history is Hyperion alpha.9, this workaround exists.
But what's even more interesting to me is that the current upstream integration didn't cause any problems, but the integration using the pull request did, even though they both use the same workaround.

Note: Maybe hyperion-project/hyperion.ng#1354 is related, as it was shipped with Hyperion 2.0.12, which was released a week or two after your last effective commit in this pr. That would explain why it worked for you, but no longer for me. And this is the only api change/fix I have found so far in the changelog since alpha.9

Apr 29 '23 12:04 hampoelz

Hey @hampoelz thanks to your excellent analysis, I updated the code accordingly and remove the workaround for https://github.com/hyperion-project/hyperion.ng/issues/992. I don't know when this workaround started to interfere with the functionality, but in any case you have clearly identified it as the culprit and I agree that it is no longer necessary now. Also, I removed the external effects from the "effects to hide" list as they do not belong there any longer, thanks again for noticing.

I have tested the changes with a local Hyperion v2.0.15 instance without any real LED hardware by observing the "Source Selection" in the "Remote Control" section of the Hyperion web interface and everything seems to work so far.

The only thing I noticed is that turning off the light with an active effect didn't work the first time sometimes.

I couldn't reproduce this in my limited testing, could you check whether the "Source Selection" still shows the effect as on i.e. the priority as active when this happens? If it is not showing there, it might hint at a possible bug in Hyperion itself but that is just speculation at this point.

May 01 '23 10:05 Sab44

Hey @Sab44, thanks for the updated integration. I was able to test everything the entire last week and there were no real problems. I couldn't reproduce the "power off" issue I mentioned either - seems it was just temporary.

I only noticed two more minor issues:

Turning off the light does not reset the brightness in the Hyperion instance. So if I lower the brightness and turn off the light, the Ambilight also has a lower brightness. I'd have to turn the light back on in HA, reset the brightness, and then turn them off again to get the Ambilight back to full brightness. Of course it's not a big deal, maybe it's a bug in Hyperion - I just wanted to let you know.
The setup page, which asks for the hyperion auth token, doesn't display its text. This doesn't seem to be caching related.

May 16 '23 18:05 hampoelz

This PR wasn't labeled as breaking change but there is a breaking change section in the PR description. Should we label it a breaking change and update the release notes of core-2023.7.0?

Jul 06 '23 06:07 MartinHjelmare

This PR wasn't labeled as breaking change but there is a breaking change section in the PR description. Should we label it a breaking change and update the release notes of core-2023.7.0?

Yes we should. Also the documentation PR needs to be merged.

Sorry about the missing labels, initially I didn't consider this PR a breaking change but in the discussion we later concluded that it should be.

Jul 06 '23 06:07 Sab44