Migrate homekit_controller light to color_mode by emontnemery · Pull Request #69261 · home-assistant/core

emontnemery · 2022-04-04T12:52:12Z

Proposed change

Migrate homekit_controller light to color_mode

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

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2022-04-04T12:52:18Z

Hey there @Jc2k, @bdraco, mind taking a look at this pull request as it has been labeled with an integration (homekit_controller) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

emontnemery · 2022-04-04T12:54:18Z

homeassistant/components/homekit_controller/light.py

+        if self.service.has(CharacteristicsTypes.HUE):
+            return COLOR_MODE_HS
+
+        if self.service.has(CharacteristicsTypes.SATURATION):
+            return COLOR_MODE_HS
+
+        if self.service.has(CharacteristicsTypes.COLOR_TEMPERATURE):
+            return COLOR_MODE_COLOR_TEMP


This does not change the behavior compared to without this PR because it would always report a non-None hs color for lights supporting hue/saturation which is interpreted as color mode hs by the base entity.
However, for lights which support both hs and white mode it would be better to report according to the light's state.
I did not immediately find anything useful in the library though. Any idea, @Jc2k , @bdraco?

HomeKit (at least as defined in the public spec (https://developer.apple.com/file/?file=homekitspecification&agree=true)) doesn't have the concept of an active color mode. You can use h/s and you can use color temp. It has the characteristics you can see here, and thats it. There's no way to indicate which color mode is currently in use.

Sometimes you can figure out a bit more from the open source example code. There are some notes here:

https://github.com/apple/HomeKitADK/blob/b171c17dba6e593e243b5fdd61fb986e9fd3b5cb/HAP/HAPServiceTypes.h#L76

Again, theres no characteristic to indicate a color mode there.

#42807 (via @bdraco) says:

The HomeKit R2 spec explicitly states that the Color Temperature char must not be used for lamps which support color.

Does that help?

OK, I see. I'll add a comment in the code stating that's the case.

The situation has changed a bit in iOS 13+

The spec says that you're not supposed to have color and color temperature at the same time, but in practice even certified devices implement that now. Apple seems to be certifying them anyway, and the hope is that the spec will get updated in the next version to remove that restriction. iOS seems to handle it now so it seems like the documentation is behind the actual implementation

The way iOS seems to handle it is, whichever characteristic was the last to get a notify/event for is the color mode you're in

Hopefully we'll get a new spec or an ADK update... (doubt it).

It might be less sucky after I've done all the refactoring I want to in aiohomekit (I want the abstraction to be higher level than it is right now).

Based on your discussion I don't think the simplification I did in 5ee6331 was correct, it should instead be like this:

If the light reports support for both ct and hs, we should expose that as supported_color_modes = {COLOR_MODE_COLOR_TEMP, COLOR_MODE_HS} (no change compared to before this PR)

Because aiohomekit does not track which color mode the light is in, a light supporting both ct and hs should just report color_mode COLOR_MODE_HS (no change compared to before this PR)

Is that right?

To be more pedantic on the second bullet: the HAP protocol itself (rather than aiohomekit) doesn't really have a concept of a current colour mode to track and aiohomekit's abstraction doesn't let us work around that. And even if it did, it would be an incomplete work around (we would lose the "current colour mode" on a simple HA restart for example). I assume the official iOS implementation is subject to the same limitations too.

(The thing I'm being pedantic about is that despite the changes in iOS 13 it's still not the case there is a variable /to/ track, we would have to track multiple variables for multiple event types (push, put and poll) and then infer our own value from the event timestamps of a subset of event types, only to have a partially working implementation at the end of it).

In the future we could implement the heuristics described to make it closer to the ideal, but short of Apple adding a new colour mode characteristic there is a potential for our implementation to seem "glitchy". So we might not want to implement such heuristics at all.

But the ultimate conclusion is the same, I think.

OK, updated again.

@bdraco, @Jc2k Is this PR OK now, or are more changes needed?

This reverts commit 5ee6331.

Jc2k

Sorry this dropped off my radar, LGTM 👍

bdraco · 2022-04-20T19:21:53Z

Looks good. Testing now

bdraco

Testing passed 👍

Migrate homekit_controller light to color_mode

660f971

emontnemery requested review from Jc2k and bdraco as code owners April 4, 2022 12:52

homeassistant added the cla-signed label Apr 4, 2022

probot-home-assistant bot added has-tests integration: homekit_controller new-feature labels Apr 4, 2022

probot-home-assistant bot assigned bdraco and Jc2k Apr 4, 2022

emontnemery commented Apr 4, 2022

View reviewed changes

emontnemery marked this pull request as draft April 4, 2022 12:55

emontnemery marked this pull request as ready for review April 4, 2022 12:55

epenet mentioned this pull request Apr 4, 2022

Deprecate SUPPORT_* constants for color_mode #69269

Merged

90 tasks

emontnemery added 5 commits April 4, 2022 20:47

Update according to discussion in PR

5ee6331

Adjust tests

298d529

Revert "Update according to discussion in PR"

325a994

This reverts commit 5ee6331.

Update according to discussion in PR

4e3a56a

Adjust tests

4fef9f7

Jc2k approved these changes Apr 20, 2022

View reviewed changes

bdraco approved these changes Apr 20, 2022

View reviewed changes

bdraco merged commit 7c0b0f7 into dev Apr 20, 2022

bdraco deleted the homekit_controller_light_color_mode branch April 20, 2022 19:26

github-actions bot locked and limited conversation to collaborators Apr 21, 2022

Uh oh!

Conversation

emontnemery commented Apr 4, 2022

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant bot commented Apr 4, 2022

Uh oh!

emontnemery Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jc2k Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

emontnemery Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jc2k Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

emontnemery Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jc2k Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emontnemery Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

emontnemery Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

Jc2k left a comment

Choose a reason for hiding this comment

Uh oh!

bdraco commented Apr 20, 2022

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emontnemery Apr 4, 2022 •

edited

Loading

bdraco Apr 4, 2022 •

edited

Loading

bdraco Apr 4, 2022 •

edited

Loading

emontnemery Apr 5, 2022 •

edited

Loading

Jc2k Apr 5, 2022 •

edited

Loading