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

Fix light groups not restoring off_brightness #78

Merged

Conversation

TheJulianJES
Copy link
Contributor

Background info

At the moment, we do not restore light group state at all, which is fine generally.
However, off_brightness and off_with_transition are not calculated from the member state at the moment.
So, we need to restore them to make sure that a group light turned off with a transition still turns on to the correct brightness after a restart (and not completely dim).

(yet to this PR in the "real-world", but it should be fine and is unit-tested at least)

Change

For that reason, this PR restores off_brightness and off_with_transition for light groups.
The test is also modified to test for the restored off_brightness and verifies off_with_transition is still False after the restore.

@@ -837,6 +838,8 @@ async def test_zha_group_light_entity(
)

assert bool(entity.state["on"]) is False
assert bool(entity.state["off_with_transition"]) is False
Copy link
Contributor Author

@TheJulianJES TheJulianJES Jul 11, 2024

Choose a reason for hiding this comment

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

off_with_transition is False is also tested here, but it's False by default anyway, even if not restored.
If we'd want to fully verify that it also restores correctly too, we'd need to modify the test further (or split it out into a separate one), as the code below expects the on cluster command on the OnOff cluster. That doesn't happen if off_with_transition is True though.
Still, the modified test checks that off_with_transition stays False. (and the off_brightness check below is fully correct and would fail if it weren't restored. It would be None before this PR.)

IMO, it's fine like this, but can change if wanted.

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.38%. Comparing base (a880209) to head (672d94c).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev      #78   +/-   ##
=======================================
  Coverage   95.38%   95.38%           
=======================================
  Files          61       61           
  Lines        9228     9232    +4     
=======================================
+ Hits         8802     8806    +4     
  Misses        426      426           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dmulcahey dmulcahey merged commit 7681fb0 into zigpy:dev Jul 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants