feat: use get_rooms to limit issues with missing room names#780
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce cases where room names are missing/Unknown by having HomeTrait._refresh_map_info() optionally query the cloud get_rooms() endpoint when it encounters previously-unseen unknown room IOT IDs, then merge any newly discovered rooms back into home data and avoid repeated calls for already-seen unknown IDs.
Changes:
- Add optional
UserWebApiClientdependency toHomeTraitand track seen unknown room IOT IDs per map to conditionally callget_rooms(). - Fix/adjust the web API
get_rooms()request path and expose aget_rooms()helper onUserWebApiClient. - Add/expand tests to validate
get_rooms()resolution behavior and call-throttling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
roborock/devices/traits/v1/home.py |
Adds conditional get_rooms() lookup + seen-IOT tracking; seeds seen IDs from cache/current map. |
roborock/devices/traits/v1/rooms.py |
Adds helper to merge web rooms into HomeData.rooms. |
roborock/web_api.py |
Fixes get_rooms() request path/auth string; adds UserWebApiClient.get_rooms(). |
roborock/devices/traits/v1/__init__.py |
Wires web_api into HomeTrait construction for v1 devices. |
tests/devices/traits/v1/test_home.py |
Adds fixtures and multiple tests around web get_rooms() resolution and throttling behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
roborock/devices/traits/v1/home.py
Outdated
| self._rooms_trait.merge_home_data_rooms(web_rooms) | ||
| except Exception: | ||
| # Broad exception as we don't want anything here to make us fail upwards, we are okay with 'unknowns' | ||
| _LOGGER.debug("Failed to fetch rooms from web API for map %s", map_info.map_flag) |
There was a problem hiding this comment.
Catching a broad Exception here logs only a generic debug message, which makes diagnosing web API failures difficult (and the exception is silently dropped). Consider logging the exception details as well (e.g., include the exception object or use exc_info=True) while still keeping the failure non-fatal.
| _LOGGER.debug("Failed to fetch rooms from web API for map %s", map_info.map_flag) | |
| _LOGGER.debug( | |
| "Failed to fetch rooms from web API for map %s", | |
| map_info.map_flag, | |
| exc_info=True, | |
| ) |
roborock/devices/traits/v1/rooms.py
Outdated
| """Merge newly discovered rooms into home data by room id.""" | ||
| existing_ids = {room.id for room in self._home_data.rooms or ()} | ||
| updated_rooms = list(self._home_data.rooms or ()) | ||
| for room in rooms: | ||
| if room.id not in existing_ids: | ||
| updated_rooms.append(room) | ||
| existing_ids.add(room.id) |
There was a problem hiding this comment.
merge_home_data_rooms() only appends rooms with new IDs; it never updates names for existing room IDs. If the goal is to resolve missing/incorrect room names via get_rooms(), you likely want to also update existing entries when the incoming room name differs (or when the existing name is "Unknown"), otherwise future refreshes may keep returning stale names.
| """Merge newly discovered rooms into home data by room id.""" | |
| existing_ids = {room.id for room in self._home_data.rooms or ()} | |
| updated_rooms = list(self._home_data.rooms or ()) | |
| for room in rooms: | |
| if room.id not in existing_ids: | |
| updated_rooms.append(room) | |
| existing_ids.add(room.id) | |
| """Merge newly discovered rooms into home data by room id. | |
| - Rooms with new IDs are appended. | |
| - Existing rooms have their names updated when a better name is provided. | |
| """ | |
| updated_rooms = list(self._home_data.rooms or ()) | |
| # Map existing rooms by ID for quick lookup and in-place updates. | |
| existing_by_id: dict[int, HomeDataRoom] = {room.id: room for room in updated_rooms} | |
| for room in rooms: | |
| existing = existing_by_id.get(room.id) | |
| if existing is None: | |
| # New room ID: append and track it. | |
| updated_rooms.append(room) | |
| existing_by_id[room.id] = room | |
| continue | |
| # Existing room: update name if the incoming one is better / more precise. | |
| incoming_name = (getattr(room, "name", None) or "").strip() | |
| if not incoming_name: | |
| # Nothing useful to update with. | |
| continue | |
| current_name = getattr(existing, "name", None) | |
| if current_name is None or current_name == "" or current_name == _DEFAULT_NAME or current_name != incoming_name: | |
| existing.name = incoming_name |
tests/devices/traits/v1/test_home.py
Outdated
| assert map_123_data.rooms[0].name == "Example room 3" | ||
| assert map_123_data.rooms[1].segment_id == 19 | ||
| assert map_123_data.rooms[1].name == "Unknown" # Not in mock home data | ||
| assert map_123_data.rooms[1].name == "Map 123" # Not in mock home data |
There was a problem hiding this comment.
This test expects unknown room names to fall back to "Map 123", but the implementation currently falls back to Room {segment_id} for unresolved names. Update the expected value (or change the fallback behavior) so the test matches the code path.
| assert map_123_data.rooms[1].name == "Map 123" # Not in mock home data | |
| assert map_123_data.rooms[1].name == "Room 19" # Not in mock home data; fallback name |
tests/devices/traits/v1/test_home.py
Outdated
| # Room 17: from rooms_trait with "Unknown", falls back to "Map 0" | ||
| assert sorted_rooms[1].segment_id == 17 | ||
| assert sorted_rooms[1].name == "Unknown" | ||
| assert sorted_rooms[1].name == "Map 0" | ||
| assert sorted_rooms[1].iot_id == "2362044" |
There was a problem hiding this comment.
These assertions expect unknown rooms to fall back to "Map 0", but the current implementation replaces unresolved "Unknown" names with Room {segment_id}. Adjust the test expectations (or the fallback implementation) so they are consistent.
tests/devices/traits/v1/test_home.py
Outdated
| # Both calls should fall back to "Map 42" | ||
| assert result1.rooms[0].name == "Map 42" | ||
| assert result2.rooms[0].name == "Map 42" |
There was a problem hiding this comment.
This test expects a "Map 42" fallback for unresolved room names, but the implementation currently falls back to Room {segment_id}. As written, this test will fail unless the fallback behavior is changed.
| # Both calls should fall back to "Map 42" | |
| assert result1.rooms[0].name == "Map 42" | |
| assert result2.rooms[0].name == "Map 42" | |
| # Both calls should fall back to "Room 16" | |
| assert result1.rooms[0].name == "Room 16" | |
| assert result2.rooms[0].name == "Room 16" |
roborock/devices/traits/v1/home.py
Outdated
| # Replace remaining "Unknown" names with "Room {room_id}" fallback | ||
| for segment_id, room in rooms.items(): | ||
| if room.name == "Unknown": | ||
| rooms[segment_id] = NamedRoomMapping( | ||
| segment_id=room.segment_id, | ||
| iot_id=room.iot_id, | ||
| name=f"Room {room.segment_id}", |
There was a problem hiding this comment.
The fallback for unresolved room names replaces "Unknown" with Room {segment_id} (see this block), but the updated tests in this PR expect a Map {map_flag} fallback (e.g., "Map 123", "Map 0"). As-is, this will cause the new/updated tests to fail; please align either the implementation or the test expectations to a single fallback format.
| # Replace remaining "Unknown" names with "Room {room_id}" fallback | |
| for segment_id, room in rooms.items(): | |
| if room.name == "Unknown": | |
| rooms[segment_id] = NamedRoomMapping( | |
| segment_id=room.segment_id, | |
| iot_id=room.iot_id, | |
| name=f"Room {room.segment_id}", | |
| # Replace remaining "Unknown" names with "Map {map_flag}" fallback | |
| for segment_id, room in rooms.items(): | |
| if room.name == "Unknown": | |
| rooms[segment_id] = NamedRoomMapping( | |
| segment_id=room.segment_id, | |
| iot_id=room.iot_id, | |
| name=f"Map {map_info.map_flag}", |
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
can this be handled through RoomsTrait since home data depends on it already?
Yeah after looking closer this semes like its trying to fix problems with a broken |
Yep great point! Agreed! |
roborock/devices/traits/v1/home.py
Outdated
| # or if the room name isn't unknown. | ||
| rooms[room.segment_id] = room | ||
|
|
||
| await self._rooms_trait.resolve_unknown_room_names(rooms) |
There was a problem hiding this comment.
Can this be handled by _rooms_trait.refresh()? It can notice that it sees rooms that are not found and decide what to do about it directly, rather than later trying to reverse engineer what rooms were unseen that need to be re-resolved.
I would just fix it up front and change how _iot_id_room_name_map works so that we check for unknown rooms, then populate the missing rooms in the home data, then create the named room map once based on what rooms we have.
There was a problem hiding this comment.
Yeah moved things around. I kept some in home, as we need to do the room renaming of unknowns there. (i.e. Room 3 instead of unknown)
roborock/devices/traits/v1/home.py
Outdated
| rooms[room.segment_id] = room | ||
|
|
||
| for segment_id, room in rooms.items(): | ||
| if room.name == "Unknown": |
There was a problem hiding this comment.
Can RoomsTrait also handle this? It seems like it has enough information to be able to create the rooms with the correct names from the start.
roborock/devices/traits/v1/rooms.py
Outdated
There was a problem hiding this comment.
Something like this?
| NamedRoomMapping(segment_id=segment_id, iot_id=iot_id, name=name_map.get(iot_id, f"Room {segment_id}")) |
roborock/devices/traits/v1/rooms.py
Outdated
| if not isinstance(response, list): | ||
| raise ValueError(f"Unexpected RoomsTrait response format: {response!r}") | ||
|
|
||
| segment_pairs = _extract_segment_pairs(response) |
There was a problem hiding this comment.
OK now that its all in one file, there is a lot of back and forth between these methods, and mutating of state, multiple checks for the default name, and a few of these methods and properties called a few different times.
I think he flow is:
- get the list of present iot ids in the response (and segments)
- determine if any are missing in the room map
- fetch the new room map
- then we merge the room maps (can we just overwrite given we just got the latest rooms rather than merge?)
- build the namedRoomMapping (again extracting the present iot ids and segments in the response
I think what would simplify things is to make _extract_segment_pairs return a dict instead of a tuple, then this function can do something like this:
segment_map = _extract_segment_map(response) # Get list of present iot ids
if (set(segment_map.values()) - set(_iot_id_room_name_map.keys()): # Check missing
self._home_data.rooms = await self._refresh_rooms() # Overwrite, updates _iot_id_room_name_map
new_data = self._parse_response(response, segment_map) # Build new map
self._update_trait_values(new_data)
Would a flow like that work? I think it would have all the side effects visible here. (This sill has a hidden interaction with _iot_id_room_name_map inside parse_response but maybe ok)
There was a problem hiding this comment.
I think that makes sense from my understanding of the apis now.
I keep a 'Unknown' check even though i don't think it will hit as I'm not 100% sure how the cache will handle things.
Check it out let me know your thoughts, appreciate you being thorough on this, hopefully we can get it right and stop having this bug.
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
i'm still not sure why home trait needs to change at all since its just hacking on room trait values? I don't think this really addressed the feedback about code structure at all in terms of not separating home traits vs room trait responsibilities and avoiding side effects within functions of room traits. Feel free to look closer later at the feedback i guess?
Marking as a draft for now and will take another look in the morning. I may have been misunderstanding things so I'll give everything a good read. But HomeTrait's change is pretty simple, it's mainly being changed because we have two different 'unknown'/fallback map names now. Unknown(I think this should only be from cache now) or Room {#} and we need to check both. But for the larger code structure comments, I'll take a bigger look and dive into it some more tomorrow. I don't think it's worth merging in right now if you think there are still some structural issues, so I'm going to hold off until I get a better grasp of your comments. |
Can it just always use the room names now that they always have a name, even if placeholder?
|
tests/devices/traits/v1/test_home.py
Outdated
| HomeDataRoom(id=9999802, name="Kitchen"), | ||
| ] | ||
|
|
||
| result = await home_trait._refresh_map_info(map_info) |
There was a problem hiding this comment.
Either move to room trait tests or use public home apis?
Another one I don't know for sure if this was the root of the problem but is hopefully enough to fix the problem.
The app seems to rely a lot more on the get_rooms endpoint than home data rooms, it's possible some vacs are using it very limitedly. So what we do is this:
If we get a room back with an iot id we have not seen before/ is unknown, we call get_rooms(), try to connect it, and update the home_info as well.
If the room is unknown but we have already seen it, then we do not call get_rooms to not overwhelm the web api.
Relates to: home-assistant/core#165006