Use homekit service callbacks for lights to resolve out of sync states by bdraco · Pull Request #32348 · home-assistant/core

bdraco · 2020-02-29T14:03:17Z

Proposed change

Service callbacks allow us to get the on/off, brightness, etc all in one call so we remove all the complexity that was previously needed to handle the out of sync states

We now get the on event and brightness event at the same time which allows us to prevent lights from flashing up to 100% before the requested brightness.

Type of change

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

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes Dimmers turned on via HomeKit always set to 100% #13695
This PR is related to issue:
Link to documentation pull request:

Multiple issues #32278 and #13768 and #13695

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 homeassistant tests)
Tests have been added to verify that the new code works.

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

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.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

codecov · 2020-02-29T14:57:40Z

Codecov Report

❗ No coverage uploaded for pull request base (dev@f2dad79). Click here to learn what that means.
The diff coverage is 28.57%.

@@          Coverage Diff           @@
##             dev   #32348   +/-   ##
======================================
  Coverage       ?   94.73%           
======================================
  Files          ?      775           
  Lines          ?    56191           
  Branches       ?        0           
======================================
  Hits           ?    53230           
  Misses         ?     2961           
  Partials       ?        0

Impacted Files	Coverage Δ
homeassistant/components/homekit/accessories.py	`89.51% <28.57%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2dad79...5383bd6. Read the comment docs.

bdraco · 2020-03-03T08:13:31Z

Test failure seems unrelated. Happening in other PRs as well

Jc2k · 2020-03-18T22:16:35Z

Nice.

Sorry for the delay getting to this.

I was worried if some less polished integrations might set their brightness in memory but not actually turn on, but the interface seems to be pretty clear that this should be OK.

My main itch is that it feels weird to put this light specific logic in HomeDriver. I can see why, the upstream interface doesn't group the updates so theres no way to implement this on the Accessory as it stands. I wonder if we should propose an amendment upstream to AccessoryDriver.set_characteristics to group on aid and let each accessory handle the value setting part self? (I.e. have an Accessory.set_characteristics that gets batches of chars from AccessoryDriver).

Actually that would be a cool refactor because you could coalesce the characteristics writes. E.g. if you received a scene update to hue, saturation, brightness and on you could turn that into a single call to turn_on. The current code has to call turn_on for each characteristic write seperately.

bdraco · 2020-03-18T22:37:22Z

(I.e. have an Accessory.set_characteristics that gets batches of chars from AccessoryDriver).

Actually that would be a cool refactor because you could coalesce the characteristics writes. E.g. if you received a scene update to hue, saturation, brightness and on you could turn that into a single call to turn_on. The current code has to call turn_on for each characteristic write seperately.

@Jc2k Thanks for taking a look!

This is a case where AccessoryDriver is providing a bit too much abstraction by only feeding in one change at a time. My first thought was to change it so you get all the characteristic changes at once from the iOS client and then consume them in batches. The downside is it adds complexity to devices on the Home Assistant side that have to consume the data.

I looked for other special cases where we would want set some characteristic before a device turned on but I didn't find anything besides brightness so I ended up with this solution. I think it is worth refactoring the interface if there turn out to be more. You are more familiar with the range of devices than I am so I'll need some guidance on that.

Jc2k · 2020-03-18T23:14:15Z

I think we are on the same wavelength re: the abstraction being not quite right.

I think we can change the interface while preserving the current interface, so the other accessories don't have to change. AccessoryDriver.set_characteristics would batch the changes and call Accessory.set_characteristics. The default impl of Accessory.set_characteristics would do what happens now - get the characteristic and update the value. If upstream made that change we could pull it in wihout changing a line of code. But then this PR could target the light accessory class specifically.

I actually think this change could make things less complex on the HA side. Consider this bit of code here. 5 entry points that invoke turn_on. set_saturation and set_hue in particular are weird because HA expects them to be provided as a pair, so it has a weird state machine involving self._flag so that it doesnt send a hue without a saturation and vice versa. With the change i'm proposing the self._flag stuff might go away completely.

I think there are other cases lurking around where this refactor would be a win for sure, even if not exactly the same as your original bug:

The original bug may well happen for all the other light characteristics - e.g. turn the light on and then change its color, rather than making a single call to turn_on.
One might be climate. E.g. here. You can see there are 2 largely identical calls to SERVICE_SET_TEMPERATURE_THERMOSTAT. The API I suggested could coalesce those service calls together at the very least.

bdraco · 2020-03-19T02:59:23Z

After digging in, I think the callbacks should go to the service instead of the characteristic. I think we could keep the existing api working and register callbacks on the service instead of the characteristic. That way we could process them all in one transaction.

bdraco · 2020-03-19T03:18:02Z

Might be able to do it without upstream changes

1.Remove the callbacks for lights

Create a super class for services to allow a setter callback
Keep the existing super class.

        # TODO: Add support for chars that do no support notifications.
        for cq in chars_query[HAP_REPR_CHARS]:
            aid, iid = cq[HAP_REPR_AID], cq[HAP_REPR_IID]
            char = self.accessory.get_characteristic(aid, iid)

Read all the chars.
Iterate though self.accessory.services
Iterate though service.characteristics

If char == service.characteristics[...] and service.setter_callback:
append to our list of chars to send to the service setter callback

then for every service fire service.setter_callback with. (char_name, value)

bdraco · 2020-03-19T03:51:16Z

Might be able to do it without upstream changes

1.Remove the callbacks for lights

Create a super class for services to allow a setter callback

Keep the existing super class.
        # TODO: Add support for chars that do no support notifications.
        for cq in chars_query[HAP_REPR_CHARS]:
            aid, iid = cq[HAP_REPR_AID], cq[HAP_REPR_IID]
            char = self.accessory.get_characteristic(aid, iid)
Read all the chars.
Iterate though self.accessory.services
Iterate though service.characteristics

If char == service.characteristics[...] and service.setter_callback:
append to our list of chars to send to the service setter callback

then for every service fire service.setter_callback with. (char_name, value)

It would be a lot patching to make that work though so better to see if upstream can accept a patch

bdraco · 2020-03-19T05:14:44Z

@Jc2k

It looks like I can actually do everything in HomeAssistant without patching HAP-python if need be by making a super class for Service.

Of course it would be much cleaner to do it in HAP-python. Concept is here with a test to show how it works
bdraco/ha-HAP-python@2e33481

and then the change to HA would be something like
https://github.com/bdraco/home-assistant/blob/homekit_service_callbacks/homeassistant/components/homekit/type_lights.py#L119-L143

It seems work pretty well

2020-03-19 01:47:06 DEBUG (Thread-6) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 53}

bdraco · 2020-03-19T07:16:31Z

@Jc2k Thanks for pointing me at the 5 light entry points.

The per service callback seems to fit a lot better for this use case

2020-03-19 02:14:14 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 77}
2020-03-19 02:14:17 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 36}
2020-03-19 02:14:17 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'Brightness': 37}
2020-03-19 02:14:19 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 73}
2020-03-19 02:14:21 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 29}
2020-03-19 02:14:21 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'Brightness': 31}
2020-03-19 02:14:23 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 70}
2020-03-19 02:14:24 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 88}
2020-03-19 02:14:28 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 44}
2020-03-19 02:14:28 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'Brightness': 45}
2020-03-19 02:14:29 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 25}
2020-03-19 02:14:30 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 66}
2020-03-19 02:14:30 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'Brightness': 67}
2020-03-19 02:14:34 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 0}
2020-03-19 02:14:36 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1}
2020-03-19 02:14:38 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 0}
2020-03-19 02:14:40 DEBUG (Thread-48) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 50}

Jc2k · 2020-03-19T07:37:29Z

Nice! I was going to suggest doing it on the service originally (HomeKit controller is very service centred). Thought it would be hard to do with the way this integration is written, but adding a service callback is a great idea.

bdraco · 2020-03-22T02:10:32Z

@Jc2k. Bought a LIFX bulb to test this with and its working just as well as the native homekit control now. Thanks again for getting me down this path. If @ikalchev is ok with ikalchev/HAP-python#229 I'll get this cleaned up and ready for merge

2020-03-22 02:07:11 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'Saturation': 67, 'Hue': 30}
2020-03-22 02:07:38 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 68}
2020-03-22 02:07:38 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 72}
2020-03-22 02:07:42 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 0}
2020-03-22 02:07:45 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'Saturation': 23, 'Hue': 28}
2020-03-22 02:08:01 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 0}
2020-03-22 02:08:04 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'Saturation': 67, 'Hue': 30}
2020-03-22 02:08:14 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 68}
2020-03-22 02:08:17 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 78}
2020-03-22 02:08:18 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 97}
2020-03-22 02:08:18 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 100}
2020-03-22 02:08:22 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 56}
2020-03-22 02:08:25 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 34}
2020-03-22 02:08:27 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'On': 1, 'Brightness': 69}
2020-03-22 02:08:29 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'Saturation': 20, 'Hue': 222}
2020-03-22 02:08:30 DEBUG (Thread-8) [custom_components.homekit.type_lights] _set_chars: {'Saturation': 23, 'Hue': 28}

Jc2k · 2020-03-22T14:01:42Z

Nice! And thank you for getting stuck in with homekit. I've barely had time to peer over from homekit_controller and help review stuff, so it's nice to see progress again.

Jc2k

Nice! This looks great!

bdraco · 2020-04-02T21:52:05Z

ci failure is unrelated and should be fixed in #33553

***** ERROR
Tests are leaving files behind. Please update the tests to avoid writing any files:
tests/testing_config/.storage/core.config_entries

bdraco · 2020-04-02T22:15:32Z

rebasing to get a clean ci run to be extra sure this is ok

bdraco · 2020-04-02T23:05:04Z

tts test flapped on this run

            ] == "{}/api/tts_proxy/42f18378fd4393d18c8dd11d03fa9563c1e54491_de_{}_demo.mp3".format(

Service callbacks allow us to get the on/off, brightness, etc all in one call so we remove all the complexity that was previously needed to handle the out of sync states We now get the on event and brightness event at the same time which allows us to prevent lights from flashing up to 100% before the requested brightness.

bdraco · 2020-04-02T23:05:52Z

3rd times a charm on running this through CI?

bdraco · 2020-04-02T23:53:19Z

Flapped on tts again

This has had enough runs for me to be comfortable with it. Going to recheck coverage before merging though

bdraco · 2020-04-02T23:54:03Z

b4

homeassistant/components/homekit/type_lights.py               106      2    98%

after

homeassistant/components/homekit/type_lights.py                85      2    98%

bdraco · 2020-04-02T23:57:59Z

Missing cover here. Looking into this

bdraco · 2020-04-03T00:13:07Z

Missing cover here. Looking into this

Definitely needs coverage for this because it doesn't work as expected if you go from STATE_OFF to STATE_ON with a brightness of 0

bdraco · 2020-04-03T00:14:56Z

ok better

homeassistant/components/homekit/type_lights.py                84      0   100%

bdraco · 2020-04-03T01:06:52Z

Ideally this gets fixed in 0.108 since python-HAP was updated to do so. If someone thinks its too risky, please remove

FidgetyRat · 2020-04-03T14:58:56Z

Really awesome to see the work done on this issue. Thanks to everyone involved, looking forward to seeing it in the main release.

My wife's eyes will also thank you in the middle of the night.

#32348) * Switch homekit lights to use service callbacks Service callbacks allow us to get the on/off, brightness, etc all in one call so we remove all the complexity that was previously needed to handle the out of sync states We now get the on event and brightness event at the same time which allows us to prevent lights from flashing up to 100% before the requested brightness. * Fix STATE_OFF -> STATE_ON,brightness:0

homeassistant added the cla-signed label Feb 29, 2020

probot-home-assistant Bot added the integration: homekit label Feb 29, 2020

MartinHjelmare changed the title ~~homekit: Suppress the ON event when brightness is adjusted on the same event~~ Suppress the homekit ON event when brightness is adjusted on the same event Feb 29, 2020

bdraco mentioned this pull request Mar 2, 2020

Global_setter for accessories ikalchev/HAP-python#91

Open

bdraco changed the title ~~Suppress the homekit ON event when brightness is adjusted on the same event~~ Suppress the homekit ON event when BRIGHTNESS is adjusted on the same event Mar 3, 2020

bdraco requested review from Jc2k and cdce8p March 10, 2020 05:45

cdce8p removed their request for review March 13, 2020 19:23

bdraco mentioned this pull request Mar 19, 2020

Add support for service callbacks ikalchev/HAP-python#229

Merged

bdraco added the waiting-for-upstream We're waiting for a change upstream label Mar 19, 2020

bdraco force-pushed the homekit_suppress_on_and_brightness_same_event branch 2 times, most recently from b0f658d to ae52b54 Compare April 2, 2020 17:43

bdraco removed the waiting-for-upstream We're waiting for a change upstream label Apr 2, 2020

bdraco changed the title ~~Suppress the homekit ON event when BRIGHTNESS is adjusted on the same event~~ Switch homekit lights to use service callbacks Apr 2, 2020

bdraco linked an issue Apr 2, 2020 that may be closed by this pull request

HomeKit Light Dimming Starts at 100% Brightness #32278

Closed

bdraco force-pushed the homekit_suppress_on_and_brightness_same_event branch from c8adc6f to faa843a Compare April 2, 2020 19:42

Jc2k approved these changes Apr 2, 2020

View reviewed changes

bdraco force-pushed the homekit_suppress_on_and_brightness_same_event branch from faa843a to 1088b2a Compare April 2, 2020 22:15

bdraco changed the title ~~Switch homekit lights to use service callbacks~~ Switch homekit lights to use service callbacks to resolve out of sync states Apr 2, 2020

bdraco changed the title ~~Switch homekit lights to use service callbacks to resolve out of sync states~~ Use homekit service callbacks for lights to resolve out of sync states Apr 2, 2020

bdraco force-pushed the homekit_suppress_on_and_brightness_same_event branch from 1088b2a to a06a94a Compare April 2, 2020 23:05

Fix STATE_OFF -> STATE_ON,brightness:0

5383bd6

bdraco merged commit f25321e into home-assistant:dev Apr 3, 2020

bdraco added this to the 0.108.0 milestone Apr 3, 2020

balloob added the cherry-picked label Apr 3, 2020

lock Bot locked and limited conversation to collaborators Apr 4, 2020

Uh oh!

Conversation

bdraco commented Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

codecov Bot commented Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bdraco commented Mar 3, 2020

Uh oh!

Jc2k commented Mar 18, 2020

Uh oh!

bdraco commented Mar 18, 2020

Uh oh!

Jc2k commented Mar 18, 2020

Uh oh!

bdraco commented Mar 19, 2020

Uh oh!

bdraco commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Mar 19, 2020

Uh oh!

bdraco commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Mar 19, 2020

Uh oh!

Jc2k commented Mar 19, 2020

Uh oh!

bdraco commented Mar 22, 2020

Uh oh!

Jc2k commented Mar 22, 2020

Uh oh!

Jc2k left a comment

Choose a reason for hiding this comment

Uh oh!

bdraco commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Apr 2, 2020

Uh oh!

bdraco commented Apr 2, 2020

Uh oh!

bdraco commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Apr 2, 2020

Uh oh!

bdraco commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco commented Apr 2, 2020

Uh oh!

bdraco commented Apr 3, 2020

Uh oh!

bdraco commented Apr 3, 2020

Uh oh!

bdraco commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FidgetyRat commented Apr 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bdraco commented Feb 29, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

codecov Bot commented Feb 29, 2020 •

edited

Loading

bdraco commented Mar 19, 2020 •

edited

Loading

bdraco commented Mar 19, 2020 •

edited

Loading

bdraco commented Apr 2, 2020 •

edited

Loading

bdraco commented Apr 2, 2020 •

edited

Loading

bdraco commented Apr 2, 2020 •

edited

Loading

bdraco commented Apr 3, 2020 •

edited

Loading