feat: Add map content to the Home trait #572
Conversation
This manages caching map content for the whole home, including performing discovery similar to metadata.
There was a problem hiding this comment.
Pull Request Overview
This PR extends the Home trait to manage map content caching alongside existing map metadata caching. The implementation adds support for storing and retrieving raw map bytes from the API, parsing them on startup, and refreshing them when needed.
Key Changes:
- Added map content caching infrastructure to store raw API responses
- Integrated map content discovery and refresh into existing home discovery flow
- Enhanced cache data structure to include map content alongside map info
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| roborock/devices/cache.py | Added home_map_content field to store raw map bytes per map |
| roborock/devices/traits/v1/map_content.py | Exposed parse_map_content method and added raw_api_response field to MapContent |
| roborock/devices/traits/v1/home.py | Integrated map content trait, added methods for fetching/caching map content, and updated discovery/refresh logic |
| roborock/devices/traits/v1/init.py | Updated HomeTrait initialization to include map_content parameter |
| tests/devices/traits/v1/test_home.py | Added comprehensive test coverage for map content caching and parsing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
roborock/devices/traits/v1/home.py
Outdated
| except RoborockException as ex: | ||
| _LOGGER.warning("Failed to parse cached home map content, will re-discover: %s", ex) | ||
| self._home_map_content = {} |
There was a problem hiding this comment.
When parsing cached map content fails, the code sets self._home_map_content = {} but then returns early on line 93, leaving self._home_map_info populated from cache while self._home_map_content is empty. This creates an inconsistent state where map info exists without corresponding map content. The return statement should be inside the try block's else clause, not outside the except block.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| # Refresh the map content to ensure we have the latest image and object positions | ||
| new_map_content = await self._refresh_map_content() | ||
| # Refresh the current map's room data | ||
| current_map_data = self._home_map_info.get(map_flag) | ||
| if current_map_data: | ||
| map_data = await self._refresh_map_info(current_map_info) | ||
| if map_data != current_map_data: | ||
| await self._update_home_map_info({**self._home_map_info, map_flag: map_data}) | ||
|
|
||
| combined_map_info = await self._refresh_map_info(current_map_info) | ||
| await self._update_current_map_cache(map_flag, combined_map_info, new_map_content) |
There was a problem hiding this comment.
So I guess in our HA implementation, this is going to be checked on the HA side if we are in cleaning/ how frequently we update - that is not a responsibility here, right?
There was a problem hiding this comment.
Yes, the caller is responsible for frequency of calling refresh. One difference is we're combing map info and map content and room information, rather than treating just the map bytes/image as a separate call. If that turns out to be a problem for some reason, we could separate them.
2 participants
This manages caching map content for the whole home, including performing discovery similar to metadata. This caches the raw map bytes API response and parses it on startup for each map.