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

Turning off Hyperion light via Home Assistant turns off Hyperion for other sources #41892

Closed
7h30n3 opened this issue Oct 15, 2020 · 73 comments · Fixed by #45410
Closed

Turning off Hyperion light via Home Assistant turns off Hyperion for other sources #41892

7h30n3 opened this issue Oct 15, 2020 · 73 comments · Fixed by #45410

Comments

@7h30n3
Copy link

7h30n3 commented Oct 15, 2020

The problem

I have Hyperion NG running on an RPi3 (LibreELEC) (priority: 250). On another PC I have Home Assistant 0.116.2 (priority: 128). And on a 3rd PC I use Hyperion Screen Capture (Protocol Buffers Server) to get Ambient Light while gaming (priority: 100).
(With the update to Home Assistant 0.116 I also had to update from Hyperion Classic to NG.)

Home Assistant is controlling Hyperion NG via a bunch of automations.
However when I turn off the light in Home Assistant and start the Screen Capture Software the light keeps off although the Screen Capture Software has lower priority.
If I keep the Screen Capture Software running and turn the light back on via Home Assistant (with a solid color or an effect) the lights also turns on, but with colors according to the caputred screen.

If I remember correctly this didn't used to be the case with Hyperion Classic. There, as soon as a source with a lower priority as Home Assistant was active, settings in Home Assistant where completly ignored.

I don't know how the "turn off action" is implemented in the hyperion integration and as I don't know if this is a problem with Hyperion NG or its integration in home assistant I also posted the issue here:
hyperion-project/hyperion.ng#1048

Environment

  • Home Assistant Core release with the issue: 0.116.2
  • Last working Home Assistant Core release (if known): 0.115 (with Hyperion Classic not NG)
  • Operating environment (OS/Container/Supervised/Core):

arch | aarch64
dev | false
docker | false
hassio | false
installation_type | Unknown
os_name | Linux
os_version | 5.9.0-rc4-1-ARCH
python_version | 3.8.6

Problem-relevant configuration.yaml

Traceback/Error logs

Additional information

@probot-home-assistant
Copy link

hyperion documentation
hyperion source
(message by IssueLinks)

@7h30n3
Copy link
Author

7h30n3 commented Oct 16, 2020

As I didn't know what's cause of the problem was I also posted an issue in the Hyperion-NG repo. There @Joeboyc2 wrote:

it actually toggles the LED Device switch in the Hyperion Remote Panel

So maybe this information helps to fix the issue.

@dermotduffy
Copy link
Contributor

I will work on this bug. Adding the contributors to the related bug on the Hyperion side for their thoughts.

@7h30n3
@Joeboyc2
@jes1417

Fundamentally, the issue is that the Hyperion Home Assistant component turns off LEDDEVICE, rather than setting a "black" color as a "fake off" at a given configured priority. To me, actually turning it off felt like the least surprising behavior -- wouldn't one expect turning the light off, to actually turn it off in all circumstances? Why is the alternative preferred? (i.e. where Home Assistant only "turns it off" at a given priority, where other priorities will keep it on).

@jes1417
Copy link

jes1417 commented Oct 22, 2020

I prefer it to be off as I use UDP or rather the WLED configuration in Hyperion and when the LED device is turned off in Hyperion I can use WLED standalone. Also I think if it were to just send black it would also keep sending black causing unnecessary traffic on my network.

@dermotduffy
Copy link
Contributor

@jes1417 That's exactly my usecase too.

I also found this bug where someone is complaining that (with the old 'fake black') setup, there was no way to actually turn the thing off.

@7h30n3
Copy link
Author

7h30n3 commented Oct 22, 2020

I understand your point, but doesn't this contradicts the Hyperion design philosophy with its multiple sources and priorities?
So turning off the light in Home Assistant I expected that it turns black and is removed as a source (as it would happen by clicking on the red button in the Hyperion Remote Control Dashboard, which just calls

function onclick(event) {
  requestPriorityClear(128);
}

As I use Hyperion as TV ambi light, which automatically turns off when the AV receiver turns off, the light turns off in a hard transition and not gradually as it used to be, due to the fact that the LEDDEVICE is turned off.

@Joeboyc2
Copy link
Contributor

I will work on this bug. Adding the contributors to the related bug on the Hyperion side for their thoughts.

@7h30n3
@Joeboyc2
@jes1417

Fundamentally, the issue is that the Hyperion Home Assistant component turns off LEDDEVICE, rather than setting a "black" color as a "fake off" at a given configured priority. To me, actually turning it off felt like the least surprising behavior -- wouldn't one expect turning the light off, to actually turn it off in all circumstances? Why is the alternative preferred? (i.e. where Home Assistant only "turns it off" at a given priority, where other priorities will keep it on).

Hi @dermotduffy,
I dont think anyone is knocking the work you have done on this component, as you said it was for sure in need of an update and if your willing to maintain it and move the development forward that benefits all us users.

I think the issues myself and other are seeing now the component has had its functionality changed could be a combination of many things, i think there may also be a bug in Hyperion which I think this pull request will fix: hyperion-project/hyperion.ng#1008

I think the other issues people are having are because of the removal of the support for the old Hyperion instance, I'm not sure why this decision was made? I know it is has been put in to an archived state in github, but many people will still be using it as it is stable and people may not have the need to upgrade, the version themselves are separate, Hyperion and Hyperion.NG, why could this new component not have been specifically for the new version and the old left intact?

I think it would have avoided some of these bad feelings and complaints about the functions no longer working, i mean for me the old component worked just fine with ng and i had no issues, now with the new component i have constant issues with turning lights off and none of my automations work, its a pain for me

I'd be more than happy to test any changes being made to help progress this towards an end state

Joe

@dermotduffy
Copy link
Contributor

dermotduffy commented Oct 22, 2020

Thanks for the thorough reply Joe!

I'm don't think keeping support for deprecated systems in the Home Assistant code base is a viable strategy:

  • It's twice the work to maintain 2 codebases. Who will maintain "hyperion_old"? What happens when the Home Assistant APIs change and "old" needs to be updated? What happens when there's a "Hyperion_v3" and "Hyperion_v4"? If this approach was applied over the full set of components in HA, we'd have a codebase covered in "v1", "v2", "v3" components all of which need to be maintained. Won't this be more confusing for new users like me? (etc)
  • I'm not a Home Assistant Core developer, but the project appears to have a fairly aggressive stance on this topic in general (e.g. only keeping support for the last 2 versions of Python, deprecating options after 2 sub-releases, etc). From a very quick scan, I cannot see other components (list) which appear to have multiple versions supported in the way described.

In short: Painful as it may be for that population, I think those Hyperion users need to update to a current Hyperion version. I just don't see a realistic alternative long-term that isn't just putting off the inevitable.

To move back to the current situation: Can your automations be updated to work with the new component? Is it the inconvenience of needing to update things to work with this "really-turning-off" (as described above), or is there a technical gap in the new component that should be closed that automations cannot work around? (For example, I could imagine an option for 'off' to be either 'real off' or 'fake off', but I can't tell if there is something here that cannot be worked around with a change in automations/workflow)

@Joeboyc2
Copy link
Contributor

Hi @dermotduffy,

Thank you for your response, I understand the point your making and I think it probably is a difficult one to say one way or the other, in theory, the old Hyperion component hasn't been maintained for some time, and as the project has been archived it is very unlikely it will be modified any time soon resulting in a breakage of what is now the removed component.

Hyperion.ng has also been under development for many year too - i have been following it for some time so know it history, so its unlikely they will iterate through versions in a fast manor, breaking any maintained component

But, having said all that, I do fully understand your position, and users of a deprecated service should be looking to move to the latest version, but taking the option away from them feel a little harsh, as they will be forced to roll back to the previous HA and not upgrade further.

But in regard to my specific issue, it not that i don't want to update my automation, i have updated them, they do run as expected, but the result is that the light gets switched of in HA, the led device toggle is flipped in Hyperion but the leds stay on the last colour / frame they where displaying.

Its annoying as the automation I have, detects when my TV is turned off and switches the lights off, but as they stay on, i then have to pull my phone out and log in to the hyperion interface and flip the leddevice switch back on, for them to turn off.

As mentioned before, this error may be more to do with that bug i have linked so i do need to test the pull request that has been made to see if that resolve the issue.

If it does I will be happy, but I think you might find that others with multiple instances are still going to have an issue as turning off the led device switch disables the ability to control all instances

@dermotduffy
Copy link
Contributor

Thanks @Joeboyc2 , really appreciate your thoughts here (and agree with counterpoints such that the old discontinued codebase is quite unlikely to dramatically change, etc).

but taking the option away from them feel a little harsh, as they will be forced to roll back to the previous HA and not upgrade further.

I suppose there are two cases here:

  1. Those who are unwilling to, for whatever reason, upgrade their Hyperion instance.
  2. Those who are missing some functionality that makes the new component less attractive or useful to them.

For case 1 above, my position is that this is their choice -- but that Home Assistant is better moving forwards rather than stay with the old codebase essentially decaying unmaintained/ununittested. I absolutely empathize, and agree this may be an unwelcome position (and who am I to decide, anyway?). If someone else feels strongly about this, they are more than welcome to re-add the now-removed code and to make the argument to the Core team for why that is the better path.

For case 2 on the other hand, I strongly want to know what this functionality is, so I can make the new component a satisfactory choice for all users (who are prepared to upgrade to Hyperion-NG).

So far, from this thread, this latter point is a little unclear -- it sounds like your issue may be resolved[*], but that you suspect others may wish the 'fake off' functionality to be returned? I see @7h30n3 making an argument that off should just mean 'remove priority', which means "off" could actually do any of these three different things:

  • Actually turn off LEDDEVICE [status quo].
  • Just remove the current priority (this would require changes to how V4L currently works, as it is "selected" by just having the other priorities removed and USBCAPTURE turned on).
  • Turn "on" a black color.

[* If you don't have means to do a build, I am happy to do a build for you if you tell me your arch so you could test]

@RomRider
Copy link
Contributor

RomRider commented Oct 25, 2020

Setting a black color is the way to go I think when you turn it off from home assistant.

The problem currently is that turning off "ALL" or "LEDDEVICE" has a bug and (at least for me) it never comes back online after (running alpha-8) and I have to reboot the complete system (not only restart hyperion...)
So what I have done is that I created a custom effect that sets black (because you can't set rgb 0,0,0 with home assistant...) and apply that effect through a template light which "replaces" the hyperion light (the turn off action does this)

As you can imagine this is not very convenient but it works.

@dermotduffy
Copy link
Contributor

dermotduffy commented Oct 25, 2020

Eugh. Thanks @RomRider , sorry to hear that, although that's a separate problem. Can you clarify: Does this happen in the Hyperion UI? (i.e. you turn off LEDDEVICE on the "Remote Control" and it never comes back on line again?). If yes, this should definitely be filed as a Hyperion bug. The dev team are super responsive in my experience, so definitely file something if you're having a problem with the underlying Hyperion instance. If it's a problem in HA only, but not in Hyperion directly, then it's a separate bug that should be filed here in HA (and add me into it).

With regards to "real off" vs "priority off", this thread is highlighting there's merit in both directions. So, I plan to support both. Right now, because Hyperion in HA is still on the old YAML structure, this is not possible (as YAML modifications are banned). However, once my config flow PR is approved, I can work on adding this alternative option.

@RomRider
Copy link
Contributor

It doesn't happen through the UI but I didn't manage to get to the end of it and find exactly what makes it happen. I feel there's a bug in the API on hyperion-ng's side. It only happens with home assistant turning off the LEDDEVICE or ALL I think but I need more time to reproduce the exact way to create the situation (also usually it crashes my hyperion-ng instance).

Once this "black color" virtual off is implemented, I guess it will solve my problem at the same time. 😊

@dermotduffy
Copy link
Contributor

@RomRider OK! There's been a tonne of small-but-important changes on the Hyperion-NG side, I'm going to see if I can convince them to do a new build once this bug is resolved, then perhaps upgrade Hyperion NG and give it another go.

And yes, once "black off" is implemented, sounds like it wouldn't bother you anyway.

@RomRider
Copy link
Contributor

Sorry for hijacking the thread but I thought also that instead of having V4L or BOBLIGHT as effects, maybe they should be switches.

The light would only monitor real effects and solid colors only on the priority that HA sets (currently 128) meaning it could be on but some other stuff could have a higher priority without being reflected on the light itself. To have V4L work you'd have to turn off the light, and have the V4L switch turned on. This would better replicate the hyperion-ng model.

Also I'll open a separate issue because it never reconnects when the hyperion-ng goes down (unless you act on the light in HA)

@dermotduffy
Copy link
Contributor

@RomRider Separate switches for V4L/BOBLIGHT doesn't sound terribly user friendly -- right now everything is controllable in a single HA UI screen. But perhaps we can chat about it in a new issue.

@RomRider That last one would be a definite problem, although one that absolutely I do not experience on my side (as I restart Hyperion & HA continually and it works!). File a bug and lets chat!

@dermotduffy
Copy link
Contributor

dermotduffy commented Oct 25, 2020

Feedback sought. I'm thinking about two modes:

"Absolute mode" [This is how the new component works today]

  • On: Turn LEDDEVICE on (if not on), then sets a color/effect at a certain priority.
  • Off: Turn LEDDEVICE off.
  • State shown in HA: Color, effect (etc) in HA reflecting the state of the lowest (winning) priority.

"Priority mode":

  • On: Turn LEDDEVICE on (if not on), then sets a color/effect at a priority.
  • Off: Clear effects/colors at that priority, set black at that priority [New]
  • State shown in HA: Shows whatever is running at priority X: If X is black, report 'off'. If X is anything else, report that. If there's nothing running at priority X at all, show the state of the lowest priority. [New]

So I have at least one concrete usecase, perhaps @7h30n3 (who started this bug) can comment on whether or not the above "Priority mode" would work for them?

@dermotduffy
Copy link
Contributor

@RomRider @7h30n3 Hoping one of you will tell me if the described "Priority mode" would actually work correctly for your usecases?

@7h30n3
Copy link
Author

7h30n3 commented Oct 31, 2020

Sorry, somehow I missed your second last comment.

I use Hyperion NG as TV Ambilight (LibreELEC) with Priority: 250 and Home Assistant has Priority: 128.
Turning off the Light in HA, would set it black and the Ambilight would not be visable, right?
However setting the effect in HA to "HDMI" (or "Grabber" as it's now called) would show me the Ambilight, right?

If these two questions can be answered with yes, I would say the "Priority Mode" fits my use case.
(It "just" needs to work as the old Hyperion Integration.)

@dermotduffy
Copy link
Contributor

Thank you @7h30n3 . The answer to both of those questions can be "Yes". I will add that this is not actually precisely the behavior of the old component as far as I can tell from the code, as it also more aggressively cleared other priorities when turning off.

I've coded the described mode -- it was a relatively modest change in the new component (unittests took most of the effort). Code: dermotduffy@ef6abe8

There's a bit of a PR train right now that is necessary before I can submit this:

Stay tuned. Once the Getting rid of YAML and options flow are submitted, I'll be hoping for a volunteer from here to test the new priority mode in a custom_component to make sure it actually works for your usecase. Hope someone is up for that when the time comes.

@Joeboyc2
Copy link
Contributor

Joeboyc2 commented Nov 1, 2020

Great work @dermotduffy , from the looks of the PR it coming along nicely, and hopefully, all being well will be pushed out in one of the upcoming updates 👍
I'd be more than happy to test a 'custom_component' when its ready

@RomRider
Copy link
Contributor

RomRider commented Nov 1, 2020

@dermotduffy, I think the priority mode should work well for my use case. I'd suggest that you add an option to define the priority (would work for both modes).

In the priority mode, setting V4L would then remove altogether what is running at priority X and any other priority then? (including black)

State shown in HA: Shows whatever is running at priority X: If X is black, report 'off'. If X is anything else, report that. If there's nothing running at priority X at all, show the state of the lowest priority. [New]

For me that is the confusing part for the end user. You can hace 2 approaches for setting it off (ha setting priority 128, something else setting priority 100)

  • use case 1:

    • ha running a color with priority 128, external colors et with priority 100
    • you'd report the priority 100 color.
    • what happens when you turn off the light in HA? Based on what you say, it would remove priority 100 and set black on 128
  • use case 2:

    • ha is off, black at 128. I turn on something external that sets priority 100. HA would now report what is running on priority 100
    • what happens when you set a color now in HA? Does it remove priority 100?

I think even with the priority mode there should be a sub-mode where you only consider you own and unique priority and reflect that and a mode where you can override anything else.

@dermotduffy
Copy link
Contributor

dermotduffy commented Nov 1, 2020

Thank you @Joeboyc2 and @RomRider for the great feedback.

@RomRider:

  • Good news: An option for priority is actually already written, it's part of the options flow change I mentioned above as the first implemented option.
  • With regards to your 2 usecases, let me explain what will happen with the proposed code in each case.
    • Case 1: No. If HA is running a color with priority 128, HA will report that priority=128 color (even if Hyperion is actually showing a different color if there's lower winning priority). If you turn off the light in HA, it will set the priority=128 color to black and show itself as 'off'. This will make the LEDs go dark only if there's no lower priority in Hyperion.
    • Case 2: No. If there's black @ 128, HA will show 'off' no matter what. If you set a color, it will set that color at priority 128. That may make no difference to what Hyperion actually shows on the LEDs, unless 128 is the lowest/winning priority.

So: I think what you describe as a 'sub-mode' is actually what I have implemented. The only time HA will do anything with a different non-HA priority, is when literally the HA 128 priority doesn't exist on Hyperion. Due to how Hyperion implements grabbing/V4L there are concrete cases where that will happen:

  • Case 3: HA is priority 128, and currently 'off' / black. V4L is priority 240, something else (non-HA) is priority 100, showing a red color and currently visible on the LEDs.
    1. User chooses 'V4L' in HA.
    2. In priority mode, HA will remove the black @ priority 128, and ensure the V4L component is enabled.
    3. However, as V4L is @ priority 240, and there is a red color @ priority 100, Hyperion will continue to show red.
    4. HA will notice that there's nothing at priority 128, but there is something at a lower priority, and so will change its HA state to show a SOLID color that is red.

That last part is a bit awkward (as the user chose 'V4L', only to have it move to 'SOLID' a second later). The only way around that would be to remove not just the priority 128 when you select 'V4l', but all priorities -- and that would appear to defeat the whole idea of respecting priorities.

Very much want to hear if the answers to 1, 2 and the new 3 are still useful to you.

@dermotduffy
Copy link
Contributor

dermotduffy commented Nov 2, 2020

As this is a somewhat confusing topic, I've created what I think are the options available to us:

HA/Hyperion Mode Options

I think my question is whether or not the items with the 'blue stars' are sufficient to meet the usecases of @Joeboyc2 and @RomRider (they are what is discussed so far in this thread), or whether the 'gold star' is also required.

@RomRider
Copy link
Contributor

RomRider commented Nov 2, 2020

Thanks @dermotduffy!

1 and 2 are perfect from my point of view.

For the 3rd one however it breaks your point 1 and 2 logic I think on:

iv. HA will notice that there's nothing at priority 128, but there is something at a lower priority, and so will change its HA state to show a SOLID color that is red.

From my understanding this goes against only showing what is at your own priority. If there's nothing at your own priority, showing what has a lower number priority (eg: 100) can be confusing as you said.

This is why in this mode I'd have had a switch that represents V4L/boblight instead of a light effect. And the light only representing the specific priority.
You could also introduce a service to clear all the priorities if a user wants to force V4L/boblight etc...

But this 3rd point is an edge case and maybe it shouldn't introduce this kind of complexity. I think the way you've implemented it in the new version is great and it's going to be good for 99,99% of the use cases!

Edit: regarding your document, for me only the blue stars are important. I believe people choosing the "hard" mode (which should be the default one) won't have any other random priorities set outside of HA 😊

@dermotduffy
Copy link
Contributor

From my understanding this goes against only showing what is at your own priority. If there's nothing at your own priority, showing what has a lower number priority (eg: 100) can be confusing as you said.

For case #3, there just aren't great options, nor is the problem avoidable without more complex arrangements (like the switches idea). So, I think, based on this thread I'll stick with the priority mode as implemented, but perhaps rename the option to "off mode" (absolute vs priority), to allow for different "on modes" later if someone wants them.

Thanks for the feedback, all. Unless there are other ideas, I'll thus wait for the YAML PR + options PR, to get into the dev branch, and then pick this back up.

@RomRider
Copy link
Contributor

RomRider commented Jan 4, 2021

@MartinHjelmare, what is your take on the above?
#41892 (comment)

@MartinHjelmare
Copy link
Member

I think we should try to solve this with a scene. We shouldn't fake state.

@dermotduffy
Copy link
Contributor

dermotduffy commented Jan 5, 2021

@MartinHjelmare

  • Sorry, but a scene does not solve the problem at all. The issue is one of interoperability between Hyperion users (of which HA is one). HA's inability to set black is a distraction from Home Assistant breaking the internal Hyperion model & priority system.
  • There is no fake state in any of these proposals. Hyperion has a much richer state than Home Assistant -- 100% of the state continues to live in the Hyperion server with any of these proposals, this is all about how that that richer state is "downsampled" into HA entities.

@RomRider
Copy link
Contributor

RomRider commented Jan 5, 2021

@MartinHjelmare, hyperion has 254 priorities. Each priority should be seen as a unique light entity by itself. Each priority has a color, a brightness and supports effects.
On top of that there is global switch to turn everything on or off.

The proposal addresses everything:

  • Simple mode: 1 priority only (= 1 light) with the ability to control everything at once (on/off) + color + and effect on that specific priority
  • Advanced mode: The ability to control individual priorities (= multiple light entities would be generated) with color, effects etc...
    On top of that, HA doesn't support [0, 0, 0] as a color, but it is required in the Hyperion model thus the other part of the proposal (treat [0, 0, 0] as off).

FWIW, this is the crap I need to build in order to be able to achieve that (and I'm loosing support for effects because the template platform doesn't support it):

light:
  - platform: hyperion
    host: !secret hyperion_raspi_ip
    name: Hyperion TV
  - platform: template
    lights:
      hyperion_tv_template:
        friendly_name: "TV Backlight"
        icon_template: mdi:television-ambient-light
        level_template: "{{ state_attr('light.hyperion_tv', 'brightness')|int }}"
        value_template: >          # Had to match specifically if black was the currently running effect to say the light is off
          {{ 
            is_state('light.hyperion_tv', 'on')
            and not state_attr('light.hyperion_tv', 'effect') == "Black"
          }}
        color_template: "({{state_attr('light.hyperion_tv', 'hs_color')}})"
        turn_on:
          service: light.turn_on
          data:
            entity_id: light.hyperion_tv
            brightness: 255
            rgb_color: [255, 255, 255]
            effect: Solid
        turn_off:
          service: light.turn_on
          data:
            entity_id: light.hyperion_tv
            effect: Black                        ## Had to create a special black effect to turn_off the light
        set_level:
          service: light.turn_on
          data:
            entity_id: light.hyperion_tv
            brightness: "{{ brightness }}"
        set_color:
          service: light.turn_on
          data:
            hs_color: ['{{ h }}', '{{ s }}']
            entity_id: light.hyperion_tv

@dermotduffy
Copy link
Contributor

Good morning all -- looks like we're still stuck here.

I am the lone codeowner for this component, and I would like to implement and support the proposed "disabled-by-default" entity functionality. I think this is a reasonable compromise that can make the APIs of Home Assistant & Hyperion work well together.

@MartinHjelmare : Can you confirm if you will consider a PR that adds these advanced entities (see my and RomRider's thoughts on why these are necessary and why this is not fixed with scenes or custom services).

If you will not be convinced, then please close this bug as "Will not fix" so we can disagree but move on.

Thank you.

@MartinHjelmare
Copy link
Member

I haven't changed my mind based on the above. No need to ping me with the same suggestion.

@dermotduffy
Copy link
Contributor

Thank you. Would you mind closing out this bug? It does not seem that further discussion is going to be fruitful in changing the decision.

As a small piece of sincerely well-intended feedback: I would recommend considering accompanying such decisions with the motivation / "the Why". Without it, it causes guesswork on behalf of everyone else, which is time expensive. Single line (ref, ref, ref, ref, ref) responses will rarely convey sufficient signal to know what middle-ground / suggestions may actually be acceptable to both parties.

Thanks again.

@MartinHjelmare
Copy link
Member

In my opinion it's better to be short and concise. That's often more clear than filling out a reply with empty words.

This is clear in my mind:

We can't change the meaning of light off in home assistant with an option.

You're always welcome to ask for a second opinion from other members. Maybe they have another view or are better at explaining.

@Joeboyc2
Copy link
Contributor

Joeboyc2 commented Jan 10, 2021 via email

@7h30n3
Copy link
Author

7h30n3 commented Jan 10, 2021

Thanks @dermotduffy for all your effort.
Is it possible to release theis as custom components via HACS?

@dermotduffy
Copy link
Contributor

@7h30n3 Yes. We could use the Lutron style model, leaving "Hyperion" in the base Home Assistant, but have a "Hyperion Pro" as a HACS component that offers more advanced features. However that is considerably more maintenance workload in the long-term as the cost of an effective fork will be borne ~forever.

@MartinHjelmare Thank you -- It's a good idea to get a second opinion before going down the expensive fork road. Would you mind suggesting a peer who you think might be open to weighing-in and see if they agree?

I think they'd start by reading from this comment onwards as it contains both the proposal for advanced entities and the subsequent replies contain the explanation for why this is necessary. Thanks!

@MartinHjelmare
Copy link
Member

Sorry, you'll have to find someone and ask yourself. Our help channels are open. And this issue and all our PRs are also open for anyone that is interested.

Please don't ping me on this issue.

@balloob
Copy link
Member

balloob commented Jan 12, 2021

I think it's okay to have an entity per API part. So 1 light to turn all of Hyperion on/off and 1 light for the HA priority.

@dermotduffy
Copy link
Contributor

Very happy there might be an agreeable compromise here. My plan is to break this into 2 PRs:

PR 1:

  • Add a new (disabled by default) light entity that behaves just like the current entity, except 'off' is a black color only (i.e. LEDDEVICE component state is not considered/modified).

PR 2:

  • Add new (disabled by default) switch entities that expose the Hyperion "component" controls, e.g.:
    • smoothing
    • blackbar_detection
    • forwarder
    • boblight_server
    • platform_capture
    • usb_capture
    • led_device
    • hyperion (overall)

Via automation, users can combine the entities from PR 1 and PR 2 to do all sorts of flexible combinations.

Starting work on PR 1 right now, but feedback welcome from anyone on tweaks to the above.

@koying
Copy link
Contributor

koying commented Jan 13, 2021

@dermotduffy Great!

FWIW, not sure it makes sense to split in 2 PR, as to have a functional PR1, you'd have to add code to handle separately effects vs. the other sources, and that code would disappear in PR2 which, I assume, would make the light only handle effects, similarly to the POC I made above...

@dermotduffy
Copy link
Contributor

dermotduffy commented Jan 14, 2021

Thanks @koying. To clarify, I'm not planning to remove the support for external sources as HA effects in the base (enabled by default) light entity. I want to keep that so basic users have a single light UI to make all the changes they want -- as such there should be no loss of functionality between PRs. In fact, when these PRs are complete, the basic enabled-by-default light entity will behave identically to how it behaves today.

@koying
Copy link
Contributor

koying commented Jan 14, 2021

@dermotduffy Ah ok. Fair and square.

@dermotduffy
Copy link
Contributor

dermotduffy commented Jan 22, 2021

Hello all,

I have prepared a single combined PR for testing purposes. Given the amount of debate and time you all have spent on this topic already, I really want to be sure this new PR will work for you.

Anyone up for testing the PR to be sure it delivers what you were expecting? It will add a disabled-by-default light entity that supports off-is-black and does NOT support external sources as effects, and will add a series of switches that map to the Hyperion component of the same name. This is a "pure" exposing of the underlying API.

(You will need a very fresh dev install, or to manually install the new version of the underlying library which required minor new features to support this PR).

Thanks in advance to anyone with the know-how & time to test.

@Joeboyc2
Copy link
Contributor

Hey @dermotduffy ,
I've install the new version, and updated the python remote. all looked to be working fine, after i removed the entities, restarted and re-added then, but i no longer have the option to set black on turn off, or find the "disabled-by-default" entity to use.
Am i doing something wrong?

@dermotduffy
Copy link
Contributor

Hi @Joeboyc2 -- thanks for testing!

If you installed this PR, and installed the latest version of the underlying library (which is now the version specified by the HA dev branch), then you should see the following:

  • The option will be gone. Rather than an option, it's a different entity entirely.
  • You'll see many more entities associated with Hyperion.
  • The entity you want is the light entity that ends with priority. It will implement black as off.

Let me know if it still doesn't look right.

Screenshots below:

Lots of entities (in this case I have 2 instances, which unfold to 20 entities):
Screen Shot 2021-01-25 at 7 48 08 PM

If you click in, you'll see most of the entities are disabled by default:
Screen Shot 2021-01-25 at 7 48 27 PM

Each one can be enabled:
Screen Shot 2021-01-25 at 7 48 41 PM

@Joeboyc2
Copy link
Contributor

Hey @dermotduffy,
Thanks for getting back to me.
So, it seems i had an older version of the component and have since resolved that by copying the data over manually from the latest pull request.

I now see all the entities and have enabled the priority ones :)

@Joeboyc2
Copy link
Contributor

However, am i missing something as there is no HDMI / V4l effect to select any longer.
How would i go about getting the light in to HDMI mode, and back to colour (as i have an automation that only turns on the V4L effect when the HDMI input is switched, and outside of that a colour is set)

@dermotduffy
Copy link
Contributor

Hi @Joeboyc2 . This is an important point as what you describe is working as intended in the proposed design (see this comment or this comment). In Hyperion NG, the color/effect at a given priority is a different concept entirely than V4L on/off at a different static priority. In the main light entity, these are combined to make a workable 'average' experience -- in these new raw entities color/effect is kept separate from these external components to better represent the underlying Hyperion-NG model to allow all sorts of combinations. People can use automation to do whatever they want...

I think what you want is V4L to 'win' in some cases, if not those cases you want a light color to 'win', unless the whole thing is turned off when you want nothing to show.

To get that, you would:

  • In Hyperion, set V4L to always use a priority lower than your HA Hyperion priority (Configuration -> Capturing Hardware -> Priority Channel).
  • Use the new switch entity switch.<instance>_component_usb_capture to instantly enable/disable V4L.
  • Use the new priority light entity to set the "fallback" color that will be disabled when V4L is off.
  • ... and when that priority light entity is off, 'black' will be shown (i.e. nothing).

I will add that for this use case, I don't see why the normal light entity won't just work for you (when you select your HDMI input, set the 'effect' to V4L, otherwise just set a color/effect). Unless there's more going on here that you didn't describe (e.g. a non-HA device controlling Hyperion).

Does this make sense? Will this address your use case?

Thanks for helping to test!

@Joeboyc2
Copy link
Contributor

Joeboyc2 commented Jan 27, 2021

Hi @Joeboyc2 . This is an important point as what you describe is working as intended in the proposed design (see this comment or this comment). In Hyperion NG, the color/effect at a given priority is a different concept entirely than V4L on/off at a different static priority. In the main light entity, these are combined to make a workable 'average' experience -- in these new raw entities color/effect is kept separate from these external components to better represent the underlying Hyperion-NG model to allow all sorts of combinations. People can use automation to do whatever they want...

I think what you want is V4L to 'win' in some cases, if not those cases you want a light color to 'win', unless the whole thing is turned off when you want nothing to show.

To get that, you would:

  • In Hyperion, set V4L to always use a priority lower than your HA Hyperion priority (Configuration -> Capturing Hardware -> Priority Channel).
  • Use the new switch entity switch.<instance>_component_usb_capture to instantly enable/disable V4L.
  • Use the new priority light entity to set the "fallback" color that will be disabled when V4L is off.
  • ... and when that priority light entity is off, 'black' will be shown (i.e. nothing).

I will add that for this use case, I don't see why the normal light entity won't just work for you (when you select your HDMI input, set the 'effect' to V4L, otherwise just set a color/effect). Unless there's more going on here that you didn't describe (e.g. a non-HA device controlling Hyperion).

Does this make sense? Will this address your use case?

Thanks for helping to test!

Hi @dermotduffy,

Ok, so i have re-read the linked comments, and I understand the concept now.
Priority light, as is, controlling only the colour and effects, and switched to control the other aspects.

I will re-work my automations to take this new concept in to account.

I will add that for this use case, I don't see why the normal light entity won't just work for you (when you select your HDMI input, set the 'effect' to V4L, otherwise just set a color/effect). Unless there's more going on here that you didn't describe (e.g. a non-HA device controlling Hyperion).

The standard light mechanism doesn't actually work for my setup for some reason, I feel it might be more related to Hyperion itself as my TV lights (APA102's) are connected directly to a Raspberry Pi , and when switching "LED Device" off, The lights actually stay on and static
My secondary instance is controlled by the same hyperion server and the LEDs are controlled via a Wled Controller and switching "LED Device" off actually dose turn the lights off there.....

I'll get a ticket logged over on the Hyperion repo for my particular issue looks like its already been logged hyperion-project/hyperion.ng#1132 and work on amending my automations in the meantime

@dermotduffy
Copy link
Contributor

The standard light mechanism doesn't actually work for my setup for some reason, I feel it might be more related to Hyperion itself as my TV lights (APA102's) are connected directly to a Raspberry Pi , and when switching "LED Device" off, The lights actually stay on and static

Yeah ... that definitely sounds like a Hyperion-NG bug. Thanks.

@dermotduffy
Copy link
Contributor

Hello all. I'm finding it a little hard to believe, but perhaps this bug is now properly closed given that #45410 has been merged in! I hope it (or automation built using those primitives) works for your respective usecases that otherwise weren't met by the default entity. Thanks for coming on this adventure! If you find problems with the new entities, please open new bugs and we'll talk there.

Thanks again!

@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants