Allow HomeKit name to be customized

[schmittx]

Description:

HomeKit accessory information can be customized by specifying name within entity_config.

Related issue (if applicable): N/A

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

Example entry for configuration.yaml (if applicable):

homekit:
  port: 51825
  filter:
    include_domains:
      - lock
  entity_config:
    lock.front_door:
      name: Front Door Lock

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 communicates with devices, web services, or third-party tools:

• New dependencies have been added to the REQUIREMENTS variable (example).
• New dependencies are only imported inside functions that use them (example).
• New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
• New files were added to .coveragerc.

If the code does not interact with devices:

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

[schmittx]

@cdce8p This is my first draft, take a look and let me know your feedback. As discussed in #14114, this doesn't provide much direct functionality but just gives a cleaner experience for those that care. It's a similar concept to that of the Alexa cloud component.

[cdce8p]

I have to think about it, but at the moment I'm not convinced this should be added.

[cdce8p]

There have been a few changes with #14278
If you rebase it, I can take a new look at it next week

[schmittx]

@cdce8p Rebased with the latest updates

[cdce8p]

Although I personally don't see much value in it, we can move forward with it.
Can you add tests and update the doc?

[cdce8p]

Move that call to get_accessory

[cdce8p]

The empty lines between these four config opions (name - serial_number) can be removed

[cdce8p]

Regarding the config dictionary: I was thinking about how we can improve it since I don't
Iike adding it to every super call.

Why don't we use the same custruct as for hass and the entity_id? Remove the keyword in get_accessory and pass it through args. Than assign it to self.config in HomeAccessory.

[schmittx]

@cdce8p I updated per your comments, updated the doc, and updated test_util.py. I'm not sure what other tests need to be added or updated, can you give a little advice?

[cdce8p]

@schmittx As a starting point take a look at the failing tests: https://travis-ci.org/home-assistant/home-assistant/jobs/375445342#L808. Since config is no longer a keyword argument, most of the accessory tests are failing. As an example:

acc = GarageDoorOpener(self.hass, 'Cover', garage_door, 2, config=None)

# Would need to change to this:
acc = GarageDoorOpener(self.hass, 'Cover', garage_door, 2)

Basically config can be removed, except for security_systems.

---

Another thing would be to add checks for the newly added vars to test_home_accessory: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/homekit/test_accessories.py#L91-L97

---

And for the name config var a test in get_accessories, here: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/homekit/test_get_accessories.py#L33

[schmittx]

@cdce8p Thanks for the help, I updated/added most of the tests without problem. But, I keep getting a KeyError with test_get_accessory_customize_name and test_alarm_control_panel as they're currently written. Can you take a look and give me some suggestions?

[cdce8p]

I added a few comments that should resolve the failing tests.

Although my initial doubts, this is coming together quite nicely.
Just a few notes FYI:

• Most likely a PR will be merged soon that changes one of the homekit tests, which will require a small rebase #14314
• Even after all changes I would like to wait a few more days (the next weekend, maybe earlier) before I merge this. That way I'm still able to push bug fixes if I need to.

[cdce8p]

Can you change the name to test_customize_name? That should be enough.
Also for the first test test_invalid_aid might be better as well. Just noticed it.

[schmittx]

I agree, I cleaned up both names.

[cdce8p]

Regarding the actual test:
The type modules are imported dynamically, but not for this test module. Therefore the Light type has to be mocked. The following should work:

def test_customize_name():
    """Test with customized name."""
    mock_type = Mock()
    with patch.dict(TYPES, {'Light': mock_type}):
        config = {CONF_NAME: 'Customize Name'}
        acc = get_accessory(None, State('light.demo', 'on'), 2, config)
        mock_type.assert_called_with(
            None, 'Customize Name', 'light.demo', 2, config)

Instead of validation the the display_name is set correctly, we just check that the right name was passed.

[cdce8p]

For the alarm_control_panel test:
If you replace this lines: https://github.com/schmittx/home-assistant/blob/b41effa8fad337c7868e1e831835e34420a18916/tests/components/homekit/test_get_accessories.py#L173-L175 with:

self.assertEqual(self.mock_type.call_args[0][-1][ATTR_CODE], '1234')

that should solve it.

The reason for this failure was the change from a keyword to a normal argument.

[schmittx]

Awesome, thanks for the assist!

[cdce8p]

What do you think of:

        for key in (CONF_NAME, CONF_MANUFACTURER,
                    CONF_MODEL, CONF_SERIAL_NUMBER):
            value = config.get(key)
            if value:
                params[key] = value

That way we only add dict entries if a value exists. It also fixes test_validate_entity_config.

[schmittx]

Agreed, much cleaner.

[cdce8p]

Instead of initializing a new HomeAccessory, just combine it with the previous one.

[schmittx]

Agreed.

[schmittx]

@cdce8p Sounds good to me, this should be good to go now once the tests pass.

FYI, I'm also working on adding support for Fan entities. I should have that PR ready later this week or next weekend.

[cdce8p]

I would prefer to use key instead.. index is usually used for an int counter through lists, so it might lead to confusion here.

[schmittx]

key is already used in the top level at line 20, so I went with char in reference to the fact that they're used for characteristic customize.

[cdce8p]

Missed that. What do you think of changing the top level one to entity_id and using key here? (At least if I haven't missed something again.)

[cdce8p]

I just realized that we wouldn't/can't cover this case otherwise with the accessory below.
We might have to add it back. Sorry for the confusion.

[schmittx]

Added back and cleaned up test_home_accessory, hopefully you agree with my changes.

[cdce8p]

@schmittx Regarding fan support. I think someone else was already working on it, although I haven't seen an open PR yet. Take a look at #13805.

We should't talk to much about it in this PR though, to keep it clean. Find me on discord.

[schmittx]

@cdce8p I read through #14330 as well as the repos that it mentioned. I understand the benefit of being able to see the history for accessories and persistent storage but I'd be hesitant to implement it due to the convoluted method required and the need to use the Eve app exclusively (if history is desired). I also think there's a high risk of regular maintenance being needed in the event that Elgato changes their services/characteristics.

I think we should focus on using the native Apple HomeKit services/characteristics in order to guarantee a consistent experience for all users, regardless of what app is used (Home, Eve, etc.).

For now, I'd vote to focus on improving the current HomeKit component (i.e. add support for more entities like media_player or show switches as valve/sprinklers, adding optional characteristics to existing entities, battery tracking, etc.).

If we run out of things to improve for this component and want to revisit Fakegato in the future then let's worry about the accessory information at that time (Maybe we'd make Fakegato a config option, which would then overwrite any accessory information with unique serial numbers, etc.?)

[cdce8p]

I agree with almost all of your points. The native services and chars are just easier to maintain at the moment and the focus should be an improving support for other components (eg. Media Player, Fan, ...).

What I'm worried about however is that it's quite difficult to take a feature away once it's implemented. If we really decide to implement fakegate, it shouldn't come at the cost of overriding the serial number if it's activated (maybe through a config option).

Therefore I would prefer if we limit the configurable parameters to the name only and assign the others as explained before. We can always add features later, but removing them is really difficult.

[schmittx]

I agree that it's difficult to remove a feature once it's implemented, I just struggle with limiting functionality now in the hopes that we might add something else in future.

I'll defer to you in the end though. If we limit the confutable parameters to just name, then I'd propose to close this PR and create a new one. Then we can always come back to this in the future if necessary.

[cdce8p]

I agree that it's difficult to remove a feature once it's implemented, I just struggle with limiting functionality now in the hopes that we might add something else in future.

Take in mind that this doesn't add any new functionality, just the option to customize some additional parts.

If we limit the confutable parameters to just name, then I'd propose to close this PR and create a new one. Then we can always come back to this in the future if necessary.

We shouldn't do that. This PR has some great foundations in place to implement this feature. A new PR would just copy most of it. A better way might be to create a new PR once this is merge that adds the additional parameters as well and close that for the time being.

[cdce8p]

Can you change key to entity_id in line 19 and 20, the for loop? That way we won't have this problem in the future.

[schmittx]

No problem, done.

[schmittx]

@cdce8p OK, everything should be good now. Rebased with #14377 and a small cleanup afterwards.

[cdce8p]

@schmittx Will take a closer look tomorrow. Thanks for taking the time!

[cdce8p]

@schmittx BTW: I have started looking into the complete async conversion for the homekit component. homekit-async if you want to take a look.

Are you in the HA discord channel? Discussing it there or in direct msg might be the easiest.