Get room hints from areas

[Swamp-Ig]

Description:

When syncing devices, return room hints for area names if not configured in the config file.

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

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.

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

• Documentation added/updated in home-assistant.io

If the code does not interact with devices:

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

[balloob]

Let's restructure this code using guard clauses to be a bit more readable. If we move the for trt in traits code above the room hint figuring out, we can just return device as soon as a room hint is configured or any other if-check is falsey. We can then drop the else.

Now this line can be replaced with:

if not entity_entry:
    return device

[balloob]

same with the other 2 if statements below.

[Swamp-Ig]

Put it into an internal function so that we don't have stray returns in the procedure.

[Swamp-Ig]

See comment as below...

[balloob]

I wonder if we should get all registries at once, to avoid having to yield 3 times.

dev_reg, ent_reg, area_reg = asyncio.gather(
    self.hass.helpers.device_registry.async_get_registry(),
    self.hass.helpers.entity_registry.async_get_registry(),
    self.hass.helpers.area_registry.async_get_registry(),
)

[balloob]

Actually, we should move getting the registries into async_devices_sync so that we only have to get the registries once. Or maybe they should become part of config ?

[balloob]

Let's start with just putting it in async_devices_sync

[Swamp-Ig]

I have pushed an update that changes all this.

I was thinking however that it might be useful to have some helper methods:

in DeviceRegistry:

async def get_by_entity_id(entity_id) -> Optional[DeviceEntry]:

and in AreaRegistry:

async def get_by_entity_id(entity_id) -> Optional[AreaEntry]:
async def get_by_device_id(device_id) -> Optional[AreaEntry]:

I don't know that caching the registries here makes a lot of difference performance wise, they're cached anyhow so it's just a quick look-up. The await is only so that it can load it from storage if needed, and that's only the first call anyhow.

These helper methods would be in a different PR though.

[balloob]

I think that we should pass the registries as parameters to entity.sync_serialize() and only fetch them once inside async_devices_sync

When I talked about guard clauses, I didn't mean to extract it into a new method. You can move the for trt in traits for-loop above the room resolving, and then just return device when you're done processing

[Swamp-Ig]

I didn't mean to extract it into a new method. You can move the for trt in traits for-loop above the room resolving, and then just return device when you're done processing

I didn't like this, it seems messy that you're moving stuff around so you can sneak in a return. It's too much of a goto statement.

[balloob]

I don't agree, it's like the sole purpose of a return: return something from a function when you're done processing. With a guard clause you check if further processing is needed, and return otherwise.

Right now you still have the same returns, except that we also define a new function on each invocation of sync and do an extra yield to the event loop. Without the function, if a room hint is configured, no yielding will be done.

[Swamp-Ig]

OK changed it as requested.

NB: _GoogleEntity gets reconstructed with every invocation, and each call to sync_serialise is just a single call on that object, so there's not a lot of value in moving the registry lookup code anywhere else.

[awarecan]

Per the discussion in #21373 (comment), it will be very useful if we can have debug log for the response we sent to google, especially for SYNC command, since Google have a validation tool to verify those response JSON.

[awarecan]

I will suggest add a device_registry.async_get() method to replace devices.get(), and do the same enhancement in area_registry as well. Directly access devices and areas are not good design.

Moreover, it will be very helpful if we have a helper input is a entity_id, output are the entity_entry, device_entry, and area_entry

I actually have those in my another un-submitted pr.

[balloob]

That is a good point.

I've also been thinking about if we should get the registries squared out earlier and store it on hass, so we don't have to await whenever we use it. Downside is that we delay startup a little bit

[Swamp-Ig]

The startup delay isn't going to be that long, the asynchio thread is free to go do other stuff while it waits for the data to load from disk. I expect the registry loading is going to happen pretty early in the piece anyhow because at the very least the entity registry needs to load before a lot of the components, so it's not like you've really gained anything much startup time wise by pseudo lazy-loading it.

I guess at minimum you have to load the config files and the three registries from disk because basically everything else hangs off these. Would an early asyncio.gather hitting these bits of IO and parsing them be worthwhile? That way at least the loading-parsing steps get done in 'parallel', in whatever order they load from disk. Then you can just store the objects somewhere and not have to worry about awaiting any of their methods. It's probably not worth doing it threaded, although you could, the longest wait is for the IO, the parsing isn't going to be that CPU intensive.

OTOH, awaiting a method that doesn't actually do any IO isn't going to be very expensive so there's no major harm in occasional awaits, only it gets annoying because you have to propagate the async marker to everything that calls them. It does have the upside that you don't need to do a big refactor just to get this stuff sorted earlier.

[balloob]

Some of these files can grow pretty big, there are people with 1500 entities. If we delay start up, we cannot initialize any component until all three are loaded. In the future more data might be added, and we end up delaying startup even more.

[Swamp-Ig]

I just did a test run with the default config, and entity_registry.async_get_registry and device_registry.async_get_registry does get called on start up, it's hit as soon as any platform calls the async_add_entities method in async_setup_platform for the component which first happens with the yr platform, although it does get hit dozens more times.

This hits entity_platform.async_add_entities, so really no components can become available until at least the entity and device registries are loaded from disk. So the upshot is that startup gets delayed no matter what you do so concerns around start up time are a bit neither here nor there.

Of course this is aio enabled, so we can get on and do other work in the mean time, which might be a reasonable enough argument to leave it how it is.

I'm not really venturing an opinion on how it should be, just listing the pros and cons 😁

Anyhow this is getting quite off-topic from this PR. Happy to change it in the future if the registry code changes.

[balloob]

Agree with the off-topic and good point on it being used inside Entity Platform, so we might as well make it available right away.

[balloob]

Great 🎉