Add HomeKit support for media players

[schmittx]

Description:

Adds support for media players in HomeKit. Entities will be shown as switches within HomeKit. Available modes are on_off, play_pause, play_stop, and toggle_mute. Uses supported_features to determine the default mode but can be customized in entity_config.

Related issue (if applicable): N/A

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5378

Example entry for configuration.yaml (if applicable):

homekit:
  entity_config:
    media_player.living_room:
      mode:
        - on_off
        - play_pause
        - play_stop
        - toggle_mute

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass

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

• Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

• Tests have been added to verify that the new code works.

[cdce8p]

Would be best if you create a separate type for it, either in the type_switch or even in a new module.

[quthla]

This is already possible with templates. Why would we hack it into the HomeKit component?

[cdce8p]

@quthla It might be, however official support would make it easier for inexperienced users.

[houndci-bot]

'homeassistant.components.homekit.const.ON_OFF' imported but unused
'homeassistant.components.homekit.const.PLAY_PAUSE' imported but unused
'homeassistant.components.homekit.const.PLAY_STOP' imported but unused
'homeassistant.components.homekit.const.TOGGLE_MUTE' imported but unused
line too long (93 > 79 characters)

[houndci-bot]

'homeassistant.core.split_entity_id' imported but unused

[cdce8p]

Left a few comments.
How do you plan to cover the case the switches for all modes should be added? IMO this should be the default as well. Can be excluded via the config.

[cdce8p]

You should be able to use: if features & (SUPPORT_TURN_ON | SUPPORT_TURN_OFF).

[cdce8p]

Maybe Named Tuples instead of the list?

[schmittx]

I'm new to namedtuple, you're proposing something like this?

Mode = namedtuple('Mode', ['on_state', 'on_service', 'off_service'])

STATE_SERVICE_MAP = {
    ON_OFF: Mode(STATE_ON, SERVICE_TURN_ON, SERVICE_TURN_OFF),
    PLAY_PAUSE: Mode(STATE_PLAYING, SERVICE_MEDIA_PLAY, SERVICE_MEDIA_PAUSE),
    PLAY_STOP: Mode(STATE_PLAYING, SERVICE_MEDIA_PLAY, SERVICE_MEDIA_STOP),
    TOGGLE_MUTE: Mode(True, SERVICE_VOLUME_MUTE, SERVICE_VOLUME_MUTE)
}

then

self.state_service_map = STATE_SERVICE_MAP[self.mode]

finally

service = self.state_service_map.on_service if value \
        else self.state_service_map.off_service

[cdce8p]

Yea, exactly.

[cdce8p]

Instead of assigning those with the named tuple assigning it just to one var and accessing it later through self.mode_map.on_service (mode_map might not be the best name).

[cdce8p]

Split the line before the else. Would improve readability.

[cdce8p]

I don't like those three lines, and they currently don't work either.
For hk_state: What if it changes from off to playing (just an example that doesn't work yet)?
For current_state: Maybe use an explicit if instead. Currently it's quite complicated and wouldn't get any easier.

current_state = new_state.state
if self.mode == TOGGLE_MUTE:
    current_state = new_state.attributes.get(ATTR_MEDIA_VOLUME_MUTED)

[schmittx]

For hk_state: Valid point, perhaps for on_off mode its best to compare with state_off; and for the other modes compare with state_on? This is the same approach that homebridge-homeassistant uses.
For current_state: Agreed, if statement is good.

[schmittx]

@cdce8p

How do you plan to cover the case the switches for all modes should be added? IMO this should be the default as well. Can be excluded via the config.

Sorry, I don’t understand the question. Can you clarify?

[cdce8p]

Most media_player entites support more then just one mode. I don't think it's far fetched to think that users also want to be able to control all of those from the Home App. Currently it's limited to just on swich per entity or am I getting something wrong?

[schmittx]

OK, I thought that’s what you meant. Valid point, I agree that we should support multiple modes for one entity if possible. I’ll look into it.

[cdce8p]

A tuple would be better here: (self.state_service_map.off_state, STATE_UNKNOWN, 'None').
I'm not sure though if 'None' is really necessary here, but wouldn't be a big deal to leave it in either.

[cdce8p]

Move the validation to the MediaPlayer class. get_accessory is designed to handle choosing the right type only.

[schmittx]

How should we handle the case if none of the config_modes are supported? Or if entity has no supported_modes? Both would result in validated_modes being empty. That’s why I initially put it in get_accessory.

[cdce8p]

Good question. I haven't tested it, but you should be able to do these checks before the super call in the type. If the setup should be aborted, a return should do the trick.

[schmittx]

If we do before the super call, how to get supported_features since self.hass isn't initialized yet?

[cdce8p]

You should be able to access all necessary attributes through args. However this would be pretty ugly.
What do think of only moving it to a separate function in util instead, so it doesn't clutter __init__.py and get_accessory that much?

[cdce8p]

The Accessory class does that for you, but the parameter is called self.display_name

[cdce8p]

Please sort all imports alphabetically. I'm working on doing the same for all other modules. That applies to the tests as well.

[cdce8p]

A dictionary for the flag vars might be a better solution. Take a look at the Light class type.

[cdce8p]

A dict might be better here as well.

self._chars = {}

[schmittx]

I used self.chars so that we can check it in the tests.

[cdce8p]

If you remove the keyword, it fits on one line. Also for the other cases.

[cdce8p]

A tuple is more efficient for immutable data structures.

hk_state = current_state not in (STATE_OFF, STATE_UNKNOWN, 'None')

[cdce8p]

Please sort the import alphabetically.

[cdce8p]

Move them belove STRING CONSTANTS and maybe create a new section Media_player Modes.

[cdce8p]

Left you another round of comments, some really small ones. What I'm really thinking about is in which case, given a valid config, the type won't be added. I couldn't came up with an example. Nevertheless validate_media_player_modes can stay in util.

[cdce8p]

Since you already pass config to validate_media_player_modes, why not override config[CONF_MODE] there? Also the is not None check can be abbreviated.

validate_media_player_modes(state, config)
if config[CONF_MODE]:

[cdce8p]

We don't need this check. cv.ensure_list(value) returns an empty list if the value is None. Additionally I think changing the for loop mode to key might be a bit better.

[cdce8p]

Since you only check if a mode is in the list, it isn't necessary to filter out duplicates with and mode not in validated_modes. Through the change in vec we guaranty that config[CONF_MODE] is always a list at the time of execution for this function, so we can directly iterate over it. Furthermore I think we should raise vol.Invalid if a mode is not supported.

if not config[CONF_MODE]:
    config[CONF_MODE] = supported_modes
    return

for mode in config[CONF_MODE]:
    if mode not in supported_modes:
        raise vol.Invalid('......')

[schmittx]

With this in validate_entity_config

        if domain == 'media_player':
            mode = config.get(CONF_MODE)
            params[CONF_MODE] = cv.ensure_list(mode)
            for key in params[CONF_MODE]:
                if key not in MEDIA_PLAYER_MODES:
                    raise vol.Invalid(
                        'Invalid mode: "{}", valid modes are: "{}".'
                        .format(key, MEDIA_PLAYER_MODES))

And this in validate_media_player_modes

    if not config[CONF_MODE]:
        config[CONF_MODE] = supported_modes
        return

    for mode in config[CONF_MODE]:
        if mode not in supported_modes:
            raise vol.Invalid('"{}" does not support mode: "{}".'
                              .format(state.entity_id, mode))

An error is produced if mode is not given in entity_config
KeyError: 'mode'
I know that the change below works, but is there a better solution?

if not config.get(CONF_MODE):
    ...

[cdce8p]

I couldn't reproduce the error. Can you push your changes to Github, so I can try them?

[cdce8p]

self.chars should be private. (The empty line above can be deleted.)

[schmittx]

I think it cannot be private in order for the tests to function? Correct?

All other components (type_lights, etc.) use non-private self.chars.

assert acc.chars[ON_OFF].value == 0

[cdce8p]

Yea, you're right. Please ignore that comment.

[cdce8p]

Through the change CONF_MODE will always be set modes = self.config[CONF_MODE]. (The empty line above can be deleted as well.)

[cdce8p]

You can just use current_state below.

[cdce8p]

We should just remove the empty lines in between. Also done by #14556

[cdce8p]

We need an extra test case for when the media_player should not be added. While thinking about it: What are the use cases for this / the user input (for entity_config/mode) to reach it.

[schmittx]

You're right, now that we added this to validate_media_player_modes

    if not config[CONF_MODE]:
        config[CONF_MODE] = supported_modes
        return

    for mode in config[CONF_MODE]:
        if mode not in supported_modes:
            raise vol.Invalid('"{}" does not support mode: "{}".'
                              .format(state.entity_id, mode))

Then this in get_accessory

    elif state.domain == 'media_player':
        validate_media_player_modes(state, config)
        if config[CONF_MODE]:
            a_type = 'MediaPlayer'

Can be re-written as

    elif state.domain == 'media_player':
        validate_media_player_modes(state, config)
        a_type = 'MediaPlayer'

I also added a test in test_not_supported in the event that the media_player entity has no supported modes (i.e. Universal) .

    with pytest.raises(vol.Invalid):
        attrs = {ATTR_SUPPORTED_FEATURES: SUPPORT_PAUSE | SUPPORT_SEEK}
        entity_state = State('media_player.demo', 'on', attrs)
        get_accessory(None, entity_state, 2, {CONF_MODE: [ON_OFF]})

[cdce8p]

I would need to see the final version for this. If there are entities that don't match any supported modes, we might as well need the change if config[CONF_MODE]: a_type = 'MediaPlayer'. Could be possible that I'm missing something.

[schmittx]

If an entity has no supported modes, then supported_modes will be empty and a vol.Invalid will be raised from if mode not in supported_modes:. Anyway, I pushed my latest version so you can review.

[cdce8p]

This import should be above homeassistant.const

[cdce8p]

Some last style changes and test improvements

[cdce8p]

'invalid_mode' instead of 'on_on'

[cdce8p]

Add an assert statement to make sure config is as expected:

    config = {}
    attrs = {ATTR_SUPPORTED_FEATURES: 20873}
    entity_state = State('media_player.demo', 'on', attrs)
    validate_media_player_modes(entity_state, config)
    assert config == {CONF_MODE: [ON_OFF, PLAY_PAUSE, PLAY_STOP, TOGGLE_MUTE]}

[cdce8p]

    entity_state = State('media_player.demo', 'on')
    config = {CONF_MODE: [ON_OFF]}
    with pytest.raises(vol.Invalid):
        validate_media_player_modes(entity_state, config)

[cdce8p]

media players

[cdce8p]

import MediaPlayer (without parentheses)

[cdce8p]

STATE_IDLE before STATE_OFF

[cdce8p]

Looks good. Merging this till next weekend. Thanks 🥇