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 homekit handling of 0 light brightness and fan speed #29962

Merged
merged 2 commits into from Dec 20, 2019

Conversation

@fuzzie360
Copy link
Contributor

fuzzie360 commented Dec 15, 2019

Breaking Change:

None

Description:

Replaces the fix in #27076.

Background:

When you turn on a HomeKit accessory it will try and restore the last brightness/rotation speed. But if it is set to 0%, HomeKit will update the brightness to 100% as it thinks 0% is off. This leads to unexpected behaviors such as:

  • HomeKit "forgets" the brightness setting when it is at the lowest setting.
  • On reboot, toggling on the device might cause the brightness to go to 100%.
  • The brightness in Home app does not match the actual device brightness.

I tried to do a screen recording to document this bug but it seems I need a proper camera to capture what is going on. It was more obvious on v100, but ever since I updated to v103 I cannot easily show it on the screen recording any more because the actual device brightness de-syncs from the Home app.

How is it fixed:

  • The initial value when the accessory is initialized is changed from 0% to 100%. update_state will immediately update it to the proper value after initialization.
  • If there is an update to the device brightness or rotation speed from home assistant and it is 0% but the device is reported to still be on, the brightness or rotation speed will be mapped to 1%. If the device is reported to be off, the update is ignored (off devices do not need to update Home app, Home app already knows to show 0%).

Please see code comments to see more details.

Example entry for configuration.yaml (if applicable):

None

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@project-bot project-bot bot added this to Needs review in Dev Dec 15, 2019
@fuzzie360 fuzzie360 mentioned this pull request Dec 15, 2019
9 of 9 tasks complete
@fuzzie360

This comment has been minimized.

Copy link
Contributor Author

fuzzie360 commented Dec 15, 2019

Stock component:

https://youtu.be/eGzqeLLD-5o

-Light already on at minimum brightness
-Turn off successfully
-Turn on successfully to minimum brightness
-Suddenly brightness changes to 100% but Home app still displays minimum brightness
-Change brightness to 5%, is less bright than previously, therefore it is obviously not at the minimum brightness. (Side note: notice the Home app UI at 0% brightness it displays the OFF symbol when trying to change the brightness.

With fix:

https://youtu.be/vOotsVOResc

-Light already on at minimum brightness (now mapped to 1%)
-Turn off successfully
-Light turns on successfully and stays at minimum bright (now mapped to 1%)

Sorry for the music, had to mute the audio but cant find the option on Youtube.

Of course there is countless other scenarios and devices not covered by these videos. But I just want to show you what I'm dealing with here. I believe the changes in this patch will result in the best experience for most devices. Devices should not report 0 brightness and rotation speed anyway so if it breaks for them technically they only work previously by accident.

@frenck

This comment has been minimized.

Copy link
Member

frenck commented Dec 15, 2019

Please rebase your PR onto the latest dev branch to get the build error fixed.

Dev automation moved this from Needs review to Reviewer approved Dec 16, 2019
Copy link
Member

balloob left a comment

Ok to merge when build passes 🎉

@fuzzie360 fuzzie360 force-pushed the fuzzie360:homekit-fix-zero-handling branch from fe4369c to 973dcbd Dec 17, 2019
@balloob balloob merged commit 92fd3e3 into home-assistant:dev Dec 20, 2019
9 checks passed
9 checks passed
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch No report found to compare against
Details
codecov/project 94.56% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Dec 20, 2019
@pvizeli pvizeli added this to the 0.103.3 milestone Dec 20, 2019
pvizeli added a commit that referenced this pull request Dec 20, 2019
* Fix homekit handling of 0 light brightness and fan speed

* Update homekit tests for new initial brightness/speed value
@pvizeli pvizeli mentioned this pull request Dec 20, 2019
@lock lock bot locked and limited conversation to collaborators Dec 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.