Add support for unloading entries to device trackers#20237
Add support for unloading entries to device trackers#20237rohankapoorcom wants to merge 6 commits into
Conversation
…th the gpslogger platform.
ghost
assigned
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Do we also need to ask the DeviceTracker instance to remove the entities of the platform when unloading it?
I'm not sure what you mean by this. Any suggestions on additional tests to add? Should I test the manager methods? |
|
The |
|
Yes, we should test the manager. But let's wait for input from @home-assistant/core. They need to approve before we continue with this approach. |
|
I think that it's not a good idea to create a new approach to solve a problem that has been previously solved. However, given how device tracker "merges" devices (Mac/dev_id), it would not be possible to create a method that would allow to leverage entity component. However, there is hope! There is a proposal for a "person" component, which will be responsible to do merging of devices. Once that is added, we can drop merging from device tracker (as it's wonky at best anyway) and migrate it to EntityComponent. |
|
All of that makes sense for the final state that we want to be in. Are you okay with introducing this new logic as a stopgap until the Person component is created and Device Trackers and migrated over to EntityComponent? I think this approach makes a good stop gap because it adds very little code and is easily testable. It's also very unintuitive that Device Trackers don't support being unloaded. |
| return | ||
| if platform_name in self.platforms: | ||
| _LOGGER.warning( | ||
| 'Cannot add duplicate platform %s to platform manager', |
There was a problem hiding this comment.
A person can have duplicates of a platform in their config.
There was a problem hiding this comment.
Yep - I totally blanked on that. Switching to storing a namedtuple with the count of loading those platforms, when the last once is unloaded, remove the platform from the manager.
There was a problem hiding this comment.
But that means we don't unload entities until they are all unloaded? That seems weird. It's better if we can distinguish where the entities came from.
I really think that it's not simple to do this, it brings a lot of complexities and we're introducing a bunch of more bugs probably. I rather not have the stopgap. There is also a person component PR #20290
There was a problem hiding this comment.
No, the entity would still be unloaded. The platform manager would still keep a reference to the platform, allowing other entities from the platform to be unloaded up until the last one was unloaded.
If you feel strongly about not pursuing the stopgap, I could just mark the appropriate tests as xfail until the Person component is merged and we've switched to EntityComponent.
|
I suggested a PR like this, but maybe we should focus on getting to What do you think? @rohankapoorcom I'm sorry if it turns out we wasted some time here. I hope you will want to continue getting entry unloading to device trackers even if it means we have to take the long route. |
|
Sounds good @MartinHjelmare. Let's discuss further on discord and see what would be needed to get this working incrementally. I'm certainly happy to help. I'm going to close this out and make a new PR for the associated platform loading/unloading work and tests. |
|
Ok 👍 FYI, I have a WIP branch here with my attempt at getting device tracker to EntityComponent. As soon as I get all tests passing, I'll make a WIP PR for discussion. |
4 participants
Description:
Device tracker platforms did not support having their entries unloaded. This PR adds support for that and demonstrates how it works with the gpslogger platform. This came out of a couple of discussions with @MartinHjelmare #20083 (comment) and #20079 (comment).
Checklist:
tox. Your PR cannot be merged unless tests passIf the code does not interact with devices: