DeebotUniverse / Deebot-4-Home-Assistant

Home Assistant integration for deebot vacuums
https://deebot.readthedocs.io/integrations/home-assistant/
GNU General Public License v3.0
180 stars 40 forks source link

Map refreshes cause progressive trace points drawing animation. #497

Closed lukakama closed 8 months ago

lukakama commented 9 months ago

Checks

The problem

Map refreshes, invoked from the map Image entity on async_update method: https://github.com/DeebotUniverse/Deebot-4-Home-Assistant/blob/3655b4204ddec720635191b461949be9cf939971/custom_components/deebot/image.py#L47

https://github.com/DeebotUniverse/Deebot-4-Home-Assistant/blob/3655b4204ddec720635191b461949be9cf939971/custom_components/deebot/image.py#L90-L96

causes the following animation on each Home Assistant polled update:

map_rendering

As far I was able to investigate, this behavior is a normal consequence of the map refresh performed on the client.py library, which triggers a complete re-download of trace points. Trace points however are downloaded in batch of 200 points at a time, so for maps with highly detailed traces with a lot of point (like it seems to be with my T20 Omni), it takes a while to download them all. During that time, the map is visually updated with partial trace points, causing the observed animation.

Is it really necessary to perform a full map refresh on every update polled from Home Assistant? Also, IMHO these updates could cause a bit of load on both local Home Assistant device and Ecovacs servers. Is there a way to optimize it? I will be happy to provide a PR to fix the issue, but before it, I would like to understand why the refresh is required in order to identify the best possible resolution.

Diagnostics information

n/a

Anything in the logs that might be useful for us?

No response

Additional information

No response

edenhaus commented 8 months ago

https://github.com/DeebotUniverse/Deebot-4-Home-Assistant/blob/3655b4204ddec720635191b461949be9cf939971/custom_components/deebot/image.py#L90-L96

Was added to deprecate the deboot.refresh service and no a full refresh it not required. Completely forgotten that the image entity is a poll entity. We should change that.

Are you willing to open a PR?

lukakama commented 8 months ago

Actually I investigated it much further and ended up on how device data, with an highlight on map traces, needs to be fetched and updated having to struggle with Ecovacs not properly reliable servers, worsen by possible local networking issues and random server-side sessions closure, that lead to random loss of pushed messages or commands responses. Even Ecovacs app seems to suffer them from time to time (sometime I have outdated bot status or position, partial map traces, missing map portions, etc) and an app restarts is required to fix them.

That said, a forced refresh from time to time is not be a bad idea, and probably it is needed to update/retrieve data lost somehow. However it could be relaxed a bit maybe raising the refresh interval to something like 1 or 2 minutes on needed platforms/entities (ref: https://developers.home-assistant.io/docs/integration_fetching_data/#separate-polling-for-each-individual-entity ). After that, it could be planned a long term improvement in order to perform data refreshes on needed capabilities/features each time a networking/session issue is detected by the client library. I will be happy to create a draft PR for it in order to define how it could be implemented.

Also, regarding map traces, I noticed that the client library clears all cached trace information every time a getMapTrace command returns a new starting point (that it is what happens when a map refresh is requested). This could be improved by using the tid parameter of getMapTrace command responses and ontMapTrace messages: it is a sort of "trace identifier" that changes every time the bot starts a new trace (I used it on my old integration for the XML model Ozmo 900 and it seems that Ecovacs kept it also for newer JSON models). I already drafted an improvement locally, with some other enhancement on trace point handling. Soon I will open a draft PR for it.

To recap:

  1. As now, refreshes are need to overcome random data losses, but they can be relaxed. I will open a PR on the Home Assistant component to reduce the refresh interval for the map entity.
  2. Map traces handling can be improved in the client library in order to use the trace id information to validate already received and cached trace points, with further improvements on handling of trace points pushed by the bot while it is moving around and an initial trace point fetch is in progress. I will open a PR on the client library for that.
  3. As a long term improvement, It will be nice to allows individual capability/feature to handle the need of a full/partial refresh due communication issues, as a sort of data recovery feature. I will open a draft PR to discuss it on the client library.

Points 1 and 2 will be required to close the current issue.

edenhaus commented 8 months ago

I don't have any data losses (at least I cannot see it on the map). I will disable polling as I forgot to remove it when introducing the MapChanged event. We can still improve the map with the points 2 and 3. For the data losses we can try to find a way to detect them but not sure how.

lukakama commented 8 months ago

I don't have any data losses (at least I cannot see it on the map)

While performing test locally, I randomly get "session closed" errors. I will report more info and logs on PR for point 3.

We can still improve the map with the points 2 and 3.

Ok, I will open their PRs as soon as possible.