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

Add support for green LEDs to API #4556

Merged
merged 3 commits into from
Sep 14, 2023
Merged

Add support for green LEDs to API #4556

merged 3 commits into from
Sep 14, 2023

Conversation

mdegat01
Copy link
Contributor

@mdegat01 mdegat01 commented Sep 12, 2023

Proposed change

Add APIs for changing Green LEDs per its DBus interface (home-assistant/os-agent#164)

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

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

If API endpoints of add-on configuration are added/changed:

@mdegat01 mdegat01 added the new-feature A new feature label Sep 12, 2023
@mdegat01 mdegat01 requested a review from agners September 12, 2023 20:22
@mdegat01 mdegat01 added missing-documentation Added to pull requests that needs a docs, but none is linked need-cli Added to pull requests that need cli changes but none is linked labels Sep 12, 2023
@mdegat01 mdegat01 removed the missing-documentation Added to pull requests that needs a docs, but none is linked label Sep 12, 2023
@mdegat01 mdegat01 removed the need-cli Added to pull requests that need cli changes but none is linked label Sep 12, 2023
@pvizeli
Copy link
Member

pvizeli commented Sep 13, 2023

I guess @agners changed a bit how LED would work in future. The LED are now get restored from Supervisor on first start and the dbus command directly control the state. Like I understand it, the Yellow will get the same changes for LED. All other options still would need a reboot

@mdegat01
Copy link
Contributor Author

@pvizeli can you clarify what you mean by "restored from Supervisor on first start"? Supervisor isn't running when OS starts up so how can this be?

I can change supervisor so its stores the expected values for the LEDs in its file config and posts that value on startup. But that seems very strange to me. What will the LEDs do between the time of system startup and supervisor startup, just revert to default behavior? I do like that they change immediately without a reboot though, that's nice.

@mdegat01 mdegat01 force-pushed the homeassistant-green-leds branch from 3e3c8bd to fb6d5a4 Compare September 13, 2023 20:08
@mdegat01
Copy link
Contributor Author

@pvizeli ok I changed it. Supervisor now stores changes to LEDs on disk like the others. It then posts the expected value for LEDs on startup when we connect to the dbus interface for the board.

Although all boards have a different config only one can be in use at the same time so I had them all share board.json. The one potential consequence of this is if a user migrates to a different board it will keep options which share the same name (like power_led for yellow and green) and drop the rest. This seemed like a good thing for the most part (keep what we can) but if you forsee an unexpected consequence I can adjust to make sure that doesn't happen.

I updated yellow with this PR as well in anticipation of the change you said. I kept the REBOOT_REQUIRED issue there for now and only dropped it from green.

@agners
Copy link
Member

agners commented Sep 14, 2023

What will the LEDs do between the time of system startup and supervisor startup, just revert to default behavior? I do like that they change immediately without a reboot though, that's nice.

Yes, they have the default behavior during that time.

It is a bit a trade-off of simplicity on implementation side vs. how early we can disable them. For Yellow disabling them early was quite easy since overlay parameters have been available. It would be a bit more complex for Green. But in any case, it requires to store the requested state in some boot time configuration, which essentially made the reboot necessary.

So now reconsidering the implementation with Green, I've chosen to have them be on a bit longer, but make the implementation simpler. As an added benefit with that implementation it will apply the state immediately. The lights are now on longer, but at very early boot, they did would light up also in the other implementation variant 🤷‍♂️

@mdegat01 mdegat01 merged commit e1232bc into main Sep 14, 2023
22 checks passed
@mdegat01 mdegat01 deleted the homeassistant-green-leds branch September 14, 2023 13:27
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants