Skip to content

Migrate homekit_controller light to color_mode #69261

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.

Sign up for GitHub

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

Merged
bdraco merged 6 commits into from
Apr 20, 2022
Merged

Migrate homekit_controller light to color_mode #69261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
660f971
Migrate homekit_controller light to color_mode
emontnemery Apr 4, 2022
5ee6331
Update according to discussion in PR
emontnemery Apr 4, 2022
298d529
Adjust tests
emontnemery Apr 4, 2022
325a994
Revert "Update according to discussion in PR"
emontnemery Apr 5, 2022
4e3a56a
Update according to discussion in PR
emontnemery Apr 5, 2022
4fef9f7
Adjust tests
emontnemery Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions homeassistant/components/homekit_controller/light.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
ATTR_BRIGHTNESS,
ATTR_COLOR_TEMP,
ATTR_HS_COLOR,
SUPPORT_BRIGHTNESS,
SUPPORT_COLOR,
SUPPORT_COLOR_TEMP,
COLOR_MODE_BRIGHTNESS,
COLOR_MODE_COLOR_TEMP,
COLOR_MODE_HS,
COLOR_MODE_ONOFF,
LightEntity,
)
from homeassistant.config_entries import ConfigEntry
Expand Down Expand Up @@ -79,23 +80,43 @@ def color_temp(self) -> int:
return self.service.value(CharacteristicsTypes.COLOR_TEMPERATURE)

@property
def supported_features(self) -> int:
"""Flag supported features."""
features = 0
def color_mode(self) -> str:
"""Return the color mode of the light."""
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

@emontnemery emontnemery Apr 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@Jc2k Jc2k Apr 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@emontnemery emontnemery Apr 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@bdraco bdraco Apr 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@bdraco bdraco Apr 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@Jc2k Jc2k Apr 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

@emontnemery emontnemery Apr 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@Jc2k Jc2k Apr 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@emontnemery emontnemery Apr 5, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, updated again.

@emontnemery emontnemery Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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


if self.service.has(CharacteristicsTypes.BRIGHTNESS):
features |= SUPPORT_BRIGHTNESS
return COLOR_MODE_BRIGHTNESS

return COLOR_MODE_ONOFF

@property
def supported_color_modes(self) -> set[str] | None:
"""Flag supported color modes."""
color_modes = set()

if self.service.has(CharacteristicsTypes.COLOR_TEMPERATURE):
features |= SUPPORT_COLOR_TEMP
color_modes.add(COLOR_MODE_COLOR_TEMP)

if self.service.has(CharacteristicsTypes.HUE):
features |= SUPPORT_COLOR
color_modes.add(COLOR_MODE_HS)

if self.service.has(CharacteristicsTypes.SATURATION):
features |= SUPPORT_COLOR
color_modes.add(COLOR_MODE_HS)

if not color_modes and self.service.has(CharacteristicsTypes.BRIGHTNESS):
color_modes.add(COLOR_MODE_BRIGHTNESS)

if not color_modes:
color_modes.add(COLOR_MODE_ONOFF)

return features
return color_modes

async def async_turn_on(self, **kwargs: Any) -> None:
"""Turn the specified light on."""
Expand Down
88 changes: 85 additions & 3 deletions tests/components/homekit_controller/test_light.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,14 @@
from aiohomekit.model.services import ServicesTypes

from homeassistant.components.homekit_controller.const import KNOWN_DEVICES
from homeassistant.const import STATE_UNAVAILABLE
from homeassistant.components.light import (
ATTR_COLOR_MODE,
ATTR_SUPPORTED_COLOR_MODES,
COLOR_MODE_BRIGHTNESS,
COLOR_MODE_COLOR_TEMP,
COLOR_MODE_HS,
)
from homeassistant.const import ATTR_SUPPORTED_FEATURES, STATE_UNAVAILABLE

from tests.components.homekit_controller.common import setup_test_component

Expand Down Expand Up @@ -98,13 +105,79 @@ async def test_switch_change_light_state_color_temp(hass, utcnow):
)


async def test_switch_read_light_state(hass, utcnow):
async def test_switch_read_light_state_dimmer(hass, utcnow):
"""Test that we can read the state of a HomeKit light accessory."""
helper = await setup_test_component(hass, create_lightbulb_service)

# Initial state is that the light is off
state = await helper.poll_and_get_state()
assert state.state == "off"
assert ATTR_COLOR_MODE not in state.attributes
assert state.attributes[ATTR_SUPPORTED_COLOR_MODES] == [COLOR_MODE_BRIGHTNESS]
assert state.attributes[ATTR_SUPPORTED_FEATURES] == 0

# Simulate that someone switched on the device in the real world not via HA
state = await helper.async_update(
ServicesTypes.LIGHTBULB,
{
CharacteristicsTypes.ON: True,
CharacteristicsTypes.BRIGHTNESS: 100,
},
)
assert state.state == "on"
assert state.attributes["brightness"] == 255
assert state.attributes[ATTR_COLOR_MODE] == COLOR_MODE_BRIGHTNESS
assert state.attributes[ATTR_SUPPORTED_COLOR_MODES] == [COLOR_MODE_BRIGHTNESS]
assert state.attributes[ATTR_SUPPORTED_FEATURES] == 0

# Simulate that device switched off in the real world not via HA
state = await helper.async_update(
ServicesTypes.LIGHTBULB,
{
CharacteristicsTypes.ON: False,
},
)
assert state.state == "off"


async def test_switch_push_light_state_dimmer(hass, utcnow):
"""Test that we can read the state of a HomeKit light accessory."""
helper = await setup_test_component(hass, create_lightbulb_service)

# Initial state is that the light is off
state = hass.states.get(LIGHT_BULB_ENTITY_ID)
assert state.state == "off"

state = await helper.async_update(
ServicesTypes.LIGHTBULB,
{
CharacteristicsTypes.ON: True,
CharacteristicsTypes.BRIGHTNESS: 100,
},
)
assert state.state == "on"
assert state.attributes["brightness"] == 255

# Simulate that device switched off in the real world not via HA
state = await helper.async_update(
ServicesTypes.LIGHTBULB,
{
CharacteristicsTypes.ON: False,
},
)
assert state.state == "off"


async def test_switch_read_light_state_hs(hass, utcnow):
"""Test that we can read the state of a HomeKit light accessory."""
helper = await setup_test_component(hass, create_lightbulb_service_with_hs)

# Initial state is that the light is off
state = await helper.poll_and_get_state()
assert state.state == "off"
assert ATTR_COLOR_MODE not in state.attributes
assert state.attributes[ATTR_SUPPORTED_COLOR_MODES] == [COLOR_MODE_HS]
assert state.attributes[ATTR_SUPPORTED_FEATURES] == 0

# Simulate that someone switched on the device in the real world not via HA
state = await helper.async_update(
Expand All @@ -119,6 +192,9 @@ async def test_switch_read_light_state(hass, utcnow):
assert state.state == "on"
assert state.attributes["brightness"] == 255
assert state.attributes["hs_color"] == (4, 5)
assert state.attributes[ATTR_COLOR_MODE] == COLOR_MODE_HS
assert state.attributes[ATTR_SUPPORTED_COLOR_MODES] == [COLOR_MODE_HS]
assert state.attributes[ATTR_SUPPORTED_FEATURES] == 0

# Simulate that device switched off in the real world not via HA
state = await helper.async_update(
Expand All @@ -130,7 +206,7 @@ async def test_switch_read_light_state(hass, utcnow):
assert state.state == "off"


async def test_switch_push_light_state(hass, utcnow):
async def test_switch_push_light_state_hs(hass, utcnow):
"""Test that we can read the state of a HomeKit light accessory."""
helper = await setup_test_component(hass, create_lightbulb_service_with_hs)

Expand Down Expand Up @@ -168,6 +244,9 @@ async def test_switch_read_light_state_color_temp(hass, utcnow):
# Initial state is that the light is off
state = await helper.poll_and_get_state()
assert state.state == "off"
assert ATTR_COLOR_MODE not in state.attributes
assert state.attributes[ATTR_SUPPORTED_COLOR_MODES] == [COLOR_MODE_COLOR_TEMP]
assert state.attributes[ATTR_SUPPORTED_FEATURES] == 0

# Simulate that someone switched on the device in the real world not via HA
state = await helper.async_update(
Expand All @@ -181,6 +260,9 @@ async def test_switch_read_light_state_color_temp(hass, utcnow):
assert state.state == "on"
assert state.attributes["brightness"] == 255
assert state.attributes["color_temp"] == 400
assert state.attributes[ATTR_COLOR_MODE] == COLOR_MODE_COLOR_TEMP
assert state.attributes[ATTR_SUPPORTED_COLOR_MODES] == [COLOR_MODE_COLOR_TEMP]
assert state.attributes[ATTR_SUPPORTED_FEATURES] == 0


async def test_switch_push_light_state_color_temp(hass, utcnow):
Expand Down