Only compute homekit_controller accessory_info when entity is added or config changes #102145

bdraco · 2023-10-17T05:46:54Z

Proposed change

These values are never overridden by child classes but they have to be calculated each time the property
is called

Noticed this while trying to come up with a solution for #99088 (comment)

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)
Deprecation (breaking change to happen in the future)
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
I have followed the perfect PR recommendations
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.

To help with the load of incoming pull requests:

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

These values are never overridden by child classes but they have to be calculated each time the property is called

home-assistant · 2023-10-17T05:47:01Z

Hey there @Jc2k, 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!

Code owner commands

Code owners of homekit_controller can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign homekit_controller Removes the current integration label and assignees on the pull request, add the integration domain after the command.

Jc2k

Definitely want to avoid this yes.

So they were originally done like this for the same reason as the other dynamic props. Their "lifetime" is only until c# changes. (A new entity map is created rather than updating the existing one in place).

It does still feel weird to have old versions of these objects loaded. But I'm not sure that it will make the behaviour any worse:

If entities get removed from the accessory db, we suck either way
iids shouldn't change

I guess we wouldn't pick up any feature changes or min/max changes or any of that. I think I saw an arch issue for supporting those sorts of things now, but they should be static atm.

I guess ideally we'd have a callback from HKdevice that lets us update these attrs on a c# change. That would be where the other dynamic attributes could be set when the arch issue is resolved.

bdraco · 2023-10-17T07:13:36Z

I guess ideally we'd have a callback from HKdevice that lets us update these attrs on a c# change. That would be where the other dynamic attributes could be set when the arch issue is resolved.

I added that but just for the ones here to start

Jc2k

That was quick! Nice!

bdraco · 2023-10-17T07:22:31Z

manual testing looks good.

It looks like we already have a device_config_changed helper... I guess we could add a test for fan that gains swing mode after a config change is reflected

bdraco · 2023-10-17T07:23:09Z

I'll draft this and come up with a test tomorrow. Its getting late for me

bdraco · 2023-10-17T08:17:24Z

I think we could build on this in a future PR to remove entities when they are removed from a bridge similar to how esphome does it

Something to think about.

Gnight

bdraco · 2023-10-17T18:32:17Z

I tried to add a test for removing features at runtime but it fails on dev as well


async def test_fan_remove_feature_at_runtime(hass: HomeAssistant) -> None:
    """Test that features can be removed at runtime."""
    entity_registry = er.async_get(hass)

    # Set up a basic fan that does not support oscillation
    accessories = await setup_accessories_from_file(
        hass, "home_assistant_bridge_fan.json"
    )
    await setup_test_accessories(hass, accessories)

    fan = entity_registry.async_get("fan.living_room_fan")
    assert fan.unique_id == "00:00:00:00:00:00_1256851357_8"

    fan_state = hass.states.get("fan.living_room_fan")
    assert (
        fan_state.attributes[ATTR_SUPPORTED_FEATURES]
        is FanEntityFeature.SET_SPEED
        | FanEntityFeature.DIRECTION
        | FanEntityFeature.OSCILLATE
    )

    fan = entity_registry.async_get("fan.ceiling_fan")
    assert fan.unique_id == "00:00:00:00:00:00_766313939_8"

    fan_state = hass.states.get("fan.ceiling_fan")
    assert fan_state.attributes[ATTR_SUPPORTED_FEATURES] is FanEntityFeature.SET_SPEED

    # Now change the config to add oscillation
    accessories = await setup_accessories_from_file(
        hass, "home_assistant_bridge_basic_fan.json"
    )
    await device_config_changed(hass, accessories)

    fan_state = hass.states.get("fan.living_room_fan")
    assert (
        fan_state.attributes[ATTR_SUPPORTED_FEATURES]
        is FanEntityFeature.SET_SPEED | FanEntityFeature.DIRECTION
    )
    fan_state = hass.states.get("fan.ceiling_fan")
    assert fan_state.attributes[ATTR_SUPPORTED_FEATURES] is FanEntityFeature.SET_SPEED

homeassistant/components/homekit_controller/connection.py

bdraco · 2023-10-17T18:35:49Z

If the CI passes, I'll get this merged, and work on a new PR to see if I can make removes better

Jc2k · 2023-10-17T18:39:29Z

Sounds good. Ping me if you need a follow up explicit +1; but extra changes all look good. 👍

bdraco · 2023-10-17T18:41:43Z

I think I'll need to change aiohomekit's testing a bit for removes as the failure is there.. but it might be more than testing.py.. still digging

Only compute homekit_controller accessory_info and service once

6d02bc2

These values are never overridden by child classes but they have to be calculated each time the property is called

home-assistant bot added cla-signed code-quality Hacktoberfest integration: homekit_controller small-pr PRs with less than 30 lines. labels Oct 17, 2023

home-assistant bot assigned Jc2k Oct 17, 2023

home-assistant bot added the by-code-owner label Oct 17, 2023

home-assistant bot added the Quality Scale: No score label Oct 17, 2023

bdraco marked this pull request as ready for review October 17, 2023 05:59

bdraco requested a review from Jc2k as a code owner October 17, 2023 05:59

bdraco mentioned this pull request Oct 17, 2023

Only calculate homekit controller cover supported features once #99088

Closed

20 tasks

Jc2k reviewed Oct 17, 2023

View reviewed changes

notify entity on config changed

1fa53d7

notify entity on config changed

33d7ad7

Jc2k approved these changes Oct 17, 2023

View reviewed changes

bdraco marked this pull request as draft October 17, 2023 07:23

bdraco changed the title ~~Only compute homekit_controller accessory_info and service once~~ Only compute homekit_controller accessory_info when entity is added or config changes Oct 17, 2023

bdraco added 2 commits October 17, 2023 08:23

tests

c21a1e9

tests

2c18f3f

add test

fc64db1

bdraco commented Oct 17, 2023

View reviewed changes

homeassistant/components/homekit_controller/connection.py Show resolved Hide resolved

stale docstring

415d35f

bdraco mentioned this pull request Oct 17, 2023

Handle removal of accessories/services/chars in homekit_controller #102179

Merged

20 tasks

bdraco added 3 commits October 17, 2023 09:39

add snapshot for new fixture

9d236d6

Merge branch 'dev' into hkc_accessory_info_once

3766f3c

branch was out of date

1a3e7f9

bdraco marked this pull request as ready for review October 17, 2023 20:53

bdraco merged commit d8e541a into dev Oct 17, 2023
23 checks passed

bdraco deleted the hkc_accessory_info_once branch October 17, 2023 20:53

github-actions bot locked and limited conversation to collaborators Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only compute homekit_controller accessory_info when entity is added or config changes #102145

Only compute homekit_controller accessory_info when entity is added or config changes #102145

bdraco commented Oct 17, 2023 •

edited

Loading

home-assistant bot commented Oct 17, 2023

Jc2k left a comment

bdraco commented Oct 17, 2023

Jc2k left a comment

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

Jc2k commented Oct 17, 2023

bdraco commented Oct 17, 2023

Only compute homekit_controller accessory_info when entity is added or config changes #102145

Only compute homekit_controller accessory_info when entity is added or config changes #102145

Conversation

bdraco commented Oct 17, 2023 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Oct 17, 2023

Jc2k left a comment

Choose a reason for hiding this comment

bdraco commented Oct 17, 2023

Jc2k left a comment

Choose a reason for hiding this comment

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023

Jc2k commented Oct 17, 2023

bdraco commented Oct 17, 2023

bdraco commented Oct 17, 2023 •

edited

Loading