From 543ada09b79812a6c55167f599239b290e6f7f07 Mon Sep 17 00:00:00 2001 From: Luke Date: Sat, 7 Mar 2026 10:31:44 -0500 Subject: [PATCH 1/8] feat: use get_rooms to limit issues with missing room names --- roborock/devices/traits/v1/__init__.py | 2 +- roborock/devices/traits/v1/home.py | 46 +++++++ roborock/devices/traits/v1/rooms.py | 12 +- roborock/web_api.py | 8 +- tests/devices/traits/v1/test_home.py | 183 ++++++++++++++++++++++++- 5 files changed, 242 insertions(+), 9 deletions(-) diff --git a/roborock/devices/traits/v1/__init__.py b/roborock/devices/traits/v1/__init__.py index 65e7588b..c11aa46a 100644 --- a/roborock/devices/traits/v1/__init__.py +++ b/roborock/devices/traits/v1/__init__.py @@ -196,7 +196,7 @@ def __init__( self.rooms = RoomsTrait(home_data) self.maps = MapsTrait(self.status) self.map_content = MapContentTrait(map_parser_config) - self.home = HomeTrait(self.status, self.maps, self.map_content, self.rooms, self._device_cache) + self.home = HomeTrait(self.status, self.maps, self.map_content, self.rooms, self._device_cache, web_api) self.network_info = NetworkInfoTrait(device_uid, self._device_cache) self.routines = RoutinesTrait(device_uid, web_api) diff --git a/roborock/devices/traits/v1/home.py b/roborock/devices/traits/v1/home.py index 16c669a5..6df55b61 100644 --- a/roborock/devices/traits/v1/home.py +++ b/roborock/devices/traits/v1/home.py @@ -26,6 +26,7 @@ from roborock.devices.traits.v1 import common from roborock.exceptions import RoborockDeviceBusy, RoborockException, RoborockInvalidStatus from roborock.roborock_typing import RoborockCommand +from roborock.web_api import UserWebApiClient from .map_content import MapContent, MapContentTrait from .maps import MapsTrait @@ -49,6 +50,7 @@ def __init__( map_content: MapContentTrait, rooms_trait: RoomsTrait, device_cache: DeviceCache, + web_api: UserWebApiClient | None = None, ) -> None: """Initialize the HomeTrait. @@ -71,6 +73,8 @@ def __init__( self._map_content = map_content self._rooms_trait = rooms_trait self._device_cache = device_cache + self._web_api = web_api + self._seen_room_iot_ids_by_map: dict[int, set[str]] = {} self._discovery_completed = False self._home_map_info: dict[int, CombinedMapInfo] | None = None self._home_map_content: dict[int, MapContent] | None = None @@ -90,6 +94,7 @@ async def discover_home(self) -> None: if device_cache_data and device_cache_data.home_map_info: _LOGGER.debug("Home cache already populated, skipping discovery") self._home_map_info = device_cache_data.home_map_info + self._seed_seen_room_iot_ids(device_cache_data.home_map_info) self._discovery_completed = True try: self._home_map_content = { @@ -138,6 +143,40 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo: # or if the room name isn't unknown. rooms[room.segment_id] = room + # If we discovered at least one new unknown room iot_id for this map, fetch names from web API. + seen_room_iot_ids = self._seen_room_iot_ids_by_map.setdefault(map_info.map_flag, set()) + has_new_unknown_room_iot_id = any( + room.name == "Unknown" and room.iot_id not in seen_room_iot_ids for room in rooms.values() + ) + if self._web_api and has_new_unknown_room_iot_id: + try: + web_rooms = await self._web_api.get_rooms() + web_room_names = {str(r.id): r.name for r in web_rooms} + for segment_id, room in rooms.items(): + if room.name == "Unknown" and room.iot_id in web_room_names: + rooms[segment_id] = NamedRoomMapping( + segment_id=room.segment_id, + iot_id=room.iot_id, + name=web_room_names[room.iot_id], + ) + # Merge new rooms into home_data so future refreshes benefit + if web_rooms: + 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) + + # 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}", + ) + + self._seen_room_iot_ids_by_map[map_info.map_flag].update(room.iot_id for room in rooms.values()) + return CombinedMapInfo( map_flag=map_info.map_flag, name=map_info.name, @@ -254,6 +293,7 @@ async def _update_home_cache( await self._device_cache.set(device_cache_data) self._home_map_info = home_map_info self._home_map_content = home_map_content + self._seed_seen_room_iot_ids(home_map_info) async def _update_current_map( self, @@ -282,7 +322,13 @@ async def _update_current_map( if self._home_map_info is None: self._home_map_info = {} self._home_map_info[map_flag] = map_info + self._seed_seen_room_iot_ids({map_flag: map_info}) if self._home_map_content is None: self._home_map_content = {} self._home_map_content[map_flag] = map_content + + def _seed_seen_room_iot_ids(self, home_map_info: dict[int, CombinedMapInfo]) -> None: + """Seed known room iot_ids from cached/current map information.""" + for map_flag, map_info in home_map_info.items(): + self._seen_room_iot_ids_by_map.setdefault(map_flag, set()).update(room.iot_id for room in map_info.rooms) diff --git a/roborock/devices/traits/v1/rooms.py b/roborock/devices/traits/v1/rooms.py index bdf4467c..401858db 100644 --- a/roborock/devices/traits/v1/rooms.py +++ b/roborock/devices/traits/v1/rooms.py @@ -3,7 +3,7 @@ import logging from dataclasses import dataclass -from roborock.data import HomeData, NamedRoomMapping, RoborockBase +from roborock.data import HomeData, HomeDataRoom, NamedRoomMapping, RoborockBase from roborock.devices.traits.v1 import common from roborock.roborock_typing import RoborockCommand @@ -55,6 +55,16 @@ def _parse_response(self, response: common.V1ResponseData) -> Rooms: ] ) + def merge_home_data_rooms(self, rooms: list[HomeDataRoom]) -> None: + """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) + self._home_data.rooms = updated_rooms + def _extract_segment_pairs(response: list) -> list[tuple[int, str]]: """Extract segment_id and iot_id pairs from the response. diff --git a/roborock/web_api.py b/roborock/web_api.py index 578e6e72..141cbeb5 100644 --- a/roborock/web_api.py +++ b/roborock/web_api.py @@ -544,10 +544,10 @@ async def get_rooms(self, user_data: UserData, home_id: int | None = None) -> li rriot.r.a, self.session, { - "Authorization": _get_hawk_authentication(rriot, "/v2/user/homes/" + str(home_id)), + "Authorization": _get_hawk_authentication(rriot, f"/user/homes/{home_id}/rooms"), }, ) - room_response = await room_request.request("get", f"/user/homes/{str(home_id)}/rooms" + str(home_id)) + room_response = await room_request.request("get", f"/user/homes/{home_id}/rooms") if not room_response.get("success"): raise RoborockException(room_response) rooms = room_response.get("result") @@ -752,6 +752,10 @@ async def get_routines(self, device_id: str) -> list[HomeDataScene]: """Fetch routines (scenes) for a specific device.""" return await self._web_api.get_scenes(self._user_data, device_id) + async def get_rooms(self) -> list[HomeDataRoom]: + """Fetch rooms using the API client.""" + return await self._web_api.get_rooms(self._user_data) + async def execute_routine(self, scene_id: int) -> None: """Execute a specific routine (scene) by its ID.""" await self._web_api.execute_scene(self._user_data, scene_id) diff --git a/tests/devices/traits/v1/test_home.py b/tests/devices/traits/v1/test_home.py index 38c9e581..762a534c 100644 --- a/tests/devices/traits/v1/test_home.py +++ b/tests/devices/traits/v1/test_home.py @@ -6,7 +6,7 @@ import pytest -from roborock.data.containers import CombinedMapInfo, NamedRoomMapping +from roborock.data.containers import CombinedMapInfo, HomeDataRoom, NamedRoomMapping from roborock.data.v1.v1_code_mappings import RoborockStateCode from roborock.data.v1.v1_containers import MultiMapsListMapInfo, MultiMapsListRoom from roborock.devices.cache import DeviceCache, DeviceCacheData, InMemoryCache @@ -139,6 +139,14 @@ def rooms_trait(device: RoborockDevice) -> RoomsTrait: return device.v1_properties.rooms +@pytest.fixture +def mock_web_api() -> AsyncMock: + """Create a mock web API client.""" + mock = AsyncMock() + mock.get_rooms.return_value = [] + return mock + + @pytest.fixture def home_trait( status_trait: StatusTrait, @@ -146,9 +154,10 @@ def home_trait( map_content_trait: MapContentTrait, rooms_trait: RoomsTrait, device_cache: DeviceCache, + mock_web_api: AsyncMock, ) -> HomeTrait: """Create a HomeTrait instance with mocked dependencies.""" - return HomeTrait(status_trait, maps_trait, map_content_trait, rooms_trait, device_cache) + return HomeTrait(status_trait, maps_trait, map_content_trait, rooms_trait, device_cache, mock_web_api) @pytest.fixture(autouse=True) @@ -227,7 +236,7 @@ async def test_discover_home_empty_cache( assert map_123_data.rooms[0].segment_id == 18 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 map_123_content = home_trait.home_map_content[123] assert map_123_content is not None @@ -620,6 +629,7 @@ async def test_single_map_no_switching( async def test_refresh_map_info_room_override_and_addition_logic( home_trait: HomeTrait, rooms_trait: RoomsTrait, + mock_web_api: AsyncMock, ) -> None: """Test the room override and addition logic in _refresh_map_info. @@ -660,6 +670,9 @@ async def test_refresh_map_info_room_override_and_addition_logic( NamedRoomMapping(segment_id=18, iot_id="2362041", name="Example room 3"), # Not in map_info, should be added ] + # Mock web API to return empty rooms (no resolution) + mock_web_api.get_rooms.return_value = [] + # Mock rooms_trait.refresh to prevent actual device calls with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): result = await home_trait._refresh_map_info(map_info) @@ -676,9 +689,9 @@ async def test_refresh_map_info_room_override_and_addition_logic( assert sorted_rooms[0].name == "Kitchen from map_info" assert sorted_rooms[0].iot_id == "2362048" - # Room 17: from rooms_trait with "Unknown", added because not in map_info + # 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" # Room 18: from rooms_trait with valid name, added because not in map_info @@ -690,3 +703,163 @@ async def test_refresh_map_info_room_override_and_addition_logic( assert sorted_rooms[3].segment_id == 19 assert sorted_rooms[3].name == "Updated Bedroom Name" assert sorted_rooms[3].iot_id == "2362042" + + +async def test_get_rooms_resolves_unknown_rooms( + home_trait: HomeTrait, + rooms_trait: RoomsTrait, + mock_web_api: AsyncMock, +) -> None: + """Test that get_rooms from web API resolves unknown room names.""" + map_info = MultiMapsListMapInfo( + map_flag=0, + name="Test Map", + rooms=[ + MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), + MultiMapsListRoom(id=17, iot_name_id="2362044", iot_name=None), + ], + ) + + rooms_trait.rooms = [ + NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + NamedRoomMapping(segment_id=17, iot_id="2362044", name="Unknown"), + ] + + # Web API returns fresh room names + mock_web_api.get_rooms.return_value = [ + HomeDataRoom(id=2362048, name="Living Room"), + HomeDataRoom(id=2362044, name="Kitchen"), + ] + + with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): + result = await home_trait._refresh_map_info(map_info) + + sorted_rooms = sorted(result.rooms, key=lambda r: r.segment_id) + assert sorted_rooms[0].name == "Living Room" + assert sorted_rooms[1].name == "Kitchen" + mock_web_api.get_rooms.assert_called_once() + + +async def test_get_rooms_called_once_for_same_unknown_room( + home_trait: HomeTrait, + rooms_trait: RoomsTrait, + mock_web_api: AsyncMock, +) -> None: + """Test that get_rooms is not re-called for an already-seen unknown room.""" + map_info = MultiMapsListMapInfo( + map_flag=42, + name="Test Map", + rooms=[ + MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), + ], + ) + + rooms_trait.rooms = [ + NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + ] + + # Web API returns empty (no resolution) + mock_web_api.get_rooms.return_value = [] + + with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): + result1 = await home_trait._refresh_map_info(map_info) + result2 = await home_trait._refresh_map_info(map_info) + + # Both calls should fall back to "Map 42" + assert result1.rooms[0].name == "Map 42" + assert result2.rooms[0].name == "Map 42" + # get_rooms should only be called once for the same unknown room + mock_web_api.get_rooms.assert_called_once() + + +async def test_get_rooms_called_again_for_new_unknown_room( + home_trait: HomeTrait, + rooms_trait: RoomsTrait, + mock_web_api: AsyncMock, +) -> None: + """Test that get_rooms is called again when a new unknown room appears.""" + map_info = MultiMapsListMapInfo( + map_flag=42, + name="Test Map", + rooms=[ + MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), + ], + ) + + rooms_trait.rooms = [ + NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + ] + + # Web API returns empty (no resolution) + mock_web_api.get_rooms.return_value = [] + + with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): + result1 = await home_trait._refresh_map_info(map_info) + + # Add a brand-new unknown room for the same map + rooms_trait.rooms = [ + NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + NamedRoomMapping(segment_id=17, iot_id="2362044", name="Unknown"), + ] + result2 = await home_trait._refresh_map_info(map_info) + + assert sorted(room.name for room in result1.rooms) == ["Map 42"] + assert sorted(room.name for room in result2.rooms) == ["Map 42", "Map 42"] + assert mock_web_api.get_rooms.call_count == 2 + + +async def test_get_rooms_called_again_for_new_unknown_iot_id_same_segment( + home_trait: HomeTrait, + rooms_trait: RoomsTrait, + mock_web_api: AsyncMock, +) -> None: + """Test that get_rooms is called again for a new unknown iot_id on the same segment.""" + map_info = MultiMapsListMapInfo( + map_flag=42, + name="Test Map", + ) + + rooms_trait.rooms = [ + NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + ] + + mock_web_api.get_rooms.return_value = [] + + with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): + await home_trait._refresh_map_info(map_info) + + # Same segment, but now with a different iot_id. + rooms_trait.rooms = [ + NamedRoomMapping(segment_id=16, iot_id="2362999", name="Unknown"), + ] + await home_trait._refresh_map_info(map_info) + + assert mock_web_api.get_rooms.call_count == 2 + + +async def test_get_rooms_failure_falls_back_to_map_flag( + home_trait: HomeTrait, + rooms_trait: RoomsTrait, + mock_web_api: AsyncMock, +) -> None: + """Test that get_rooms failure gracefully falls back to 'Map {flag}'.""" + map_info = MultiMapsListMapInfo( + map_flag=7, + name="Test Map", + rooms=[ + MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), + ], + ) + + rooms_trait.rooms = [ + NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + ] + + # Web API raises an exception + mock_web_api.get_rooms.side_effect = Exception("API error") + + with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): + result = await home_trait._refresh_map_info(map_info) + + assert result.rooms[0].name == "Map 7" + mock_web_api.get_rooms.assert_called_once() From 8b80865bd0d9996e19e3aaa04afcd05dec60ad67 Mon Sep 17 00:00:00 2001 From: Luke Date: Sat, 7 Mar 2026 10:57:05 -0500 Subject: [PATCH 2/8] fix: tests --- roborock/devices/traits/v1/home.py | 2 +- tests/devices/traits/v1/test_home.py | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/roborock/devices/traits/v1/home.py b/roborock/devices/traits/v1/home.py index 6df55b61..99c79f84 100644 --- a/roborock/devices/traits/v1/home.py +++ b/roborock/devices/traits/v1/home.py @@ -166,7 +166,7 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo: # 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) - # Replace remaining "Unknown" names with "Room {room_id}" fallback + # Replace remaining "Unknown" names with "Room {segment_id}" fallback. for segment_id, room in rooms.items(): if room.name == "Unknown": rooms[segment_id] = NamedRoomMapping( diff --git a/tests/devices/traits/v1/test_home.py b/tests/devices/traits/v1/test_home.py index 762a534c..182d5176 100644 --- a/tests/devices/traits/v1/test_home.py +++ b/tests/devices/traits/v1/test_home.py @@ -236,7 +236,7 @@ async def test_discover_home_empty_cache( assert map_123_data.rooms[0].segment_id == 18 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 == "Map 123" # Not in mock home data + assert map_123_data.rooms[1].name == "Room 19" # Not in mock home data map_123_content = home_trait.home_map_content[123] assert map_123_content is not None @@ -689,9 +689,9 @@ async def test_refresh_map_info_room_override_and_addition_logic( assert sorted_rooms[0].name == "Kitchen from map_info" assert sorted_rooms[0].iot_id == "2362048" - # Room 17: from rooms_trait with "Unknown", falls back to "Map 0" + # Room 17: from rooms_trait with "Unknown", falls back to "Room 17" assert sorted_rooms[1].segment_id == 17 - assert sorted_rooms[1].name == "Map 0" + assert sorted_rooms[1].name == "Room 17" assert sorted_rooms[1].iot_id == "2362044" # Room 18: from rooms_trait with valid name, added because not in map_info @@ -765,9 +765,9 @@ async def test_get_rooms_called_once_for_same_unknown_room( result1 = await home_trait._refresh_map_info(map_info) result2 = await home_trait._refresh_map_info(map_info) - # 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" # get_rooms should only be called once for the same unknown room mock_web_api.get_rooms.assert_called_once() @@ -803,8 +803,8 @@ async def test_get_rooms_called_again_for_new_unknown_room( ] result2 = await home_trait._refresh_map_info(map_info) - assert sorted(room.name for room in result1.rooms) == ["Map 42"] - assert sorted(room.name for room in result2.rooms) == ["Map 42", "Map 42"] + assert sorted(room.name for room in result1.rooms) == ["Room 16"] + assert sorted(room.name for room in result2.rooms) == ["Room 16", "Room 17"] assert mock_web_api.get_rooms.call_count == 2 @@ -837,12 +837,12 @@ async def test_get_rooms_called_again_for_new_unknown_iot_id_same_segment( assert mock_web_api.get_rooms.call_count == 2 -async def test_get_rooms_failure_falls_back_to_map_flag( +async def test_get_rooms_failure_falls_back_to_room_segment_id( home_trait: HomeTrait, rooms_trait: RoomsTrait, mock_web_api: AsyncMock, ) -> None: - """Test that get_rooms failure gracefully falls back to 'Map {flag}'.""" + """Test that get_rooms failure gracefully falls back to 'Room {segment_id}'.""" map_info = MultiMapsListMapInfo( map_flag=7, name="Test Map", @@ -861,5 +861,5 @@ async def test_get_rooms_failure_falls_back_to_map_flag( with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): result = await home_trait._refresh_map_info(map_info) - assert result.rooms[0].name == "Map 7" + assert result.rooms[0].name == "Room 16" mock_web_api.get_rooms.assert_called_once() From 000b5107d55494d22431c9aa6e6860b2d29f5a1c Mon Sep 17 00:00:00 2001 From: Luke Date: Sat, 7 Mar 2026 13:30:07 -0500 Subject: [PATCH 3/8] chore: address comments --- roborock/devices/traits/v1/__init__.py | 4 +- roborock/devices/traits/v1/home.py | 46 +--------------------- roborock/devices/traits/v1/rooms.py | 43 ++++++++++++++++++-- tests/devices/traits/v1/test_home.py | 3 +- tests/devices/traits/v1/test_rooms.py | 54 ++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 52 deletions(-) diff --git a/roborock/devices/traits/v1/__init__.py b/roborock/devices/traits/v1/__init__.py index c11aa46a..0f830774 100644 --- a/roborock/devices/traits/v1/__init__.py +++ b/roborock/devices/traits/v1/__init__.py @@ -193,10 +193,10 @@ def __init__( self.device_features = DeviceFeaturesTrait(product, self._device_cache) self.status = StatusTrait(self.device_features, region=self._region) self.consumables = ConsumableTrait() - self.rooms = RoomsTrait(home_data) + self.rooms = RoomsTrait(home_data, web_api) self.maps = MapsTrait(self.status) self.map_content = MapContentTrait(map_parser_config) - self.home = HomeTrait(self.status, self.maps, self.map_content, self.rooms, self._device_cache, web_api) + self.home = HomeTrait(self.status, self.maps, self.map_content, self.rooms, self._device_cache) self.network_info = NetworkInfoTrait(device_uid, self._device_cache) self.routines = RoutinesTrait(device_uid, web_api) diff --git a/roborock/devices/traits/v1/home.py b/roborock/devices/traits/v1/home.py index 99c79f84..53103581 100644 --- a/roborock/devices/traits/v1/home.py +++ b/roborock/devices/traits/v1/home.py @@ -26,7 +26,6 @@ from roborock.devices.traits.v1 import common from roborock.exceptions import RoborockDeviceBusy, RoborockException, RoborockInvalidStatus from roborock.roborock_typing import RoborockCommand -from roborock.web_api import UserWebApiClient from .map_content import MapContent, MapContentTrait from .maps import MapsTrait @@ -50,7 +49,6 @@ def __init__( map_content: MapContentTrait, rooms_trait: RoomsTrait, device_cache: DeviceCache, - web_api: UserWebApiClient | None = None, ) -> None: """Initialize the HomeTrait. @@ -73,8 +71,6 @@ def __init__( self._map_content = map_content self._rooms_trait = rooms_trait self._device_cache = device_cache - self._web_api = web_api - self._seen_room_iot_ids_by_map: dict[int, set[str]] = {} self._discovery_completed = False self._home_map_info: dict[int, CombinedMapInfo] | None = None self._home_map_content: dict[int, MapContent] | None = None @@ -94,7 +90,6 @@ async def discover_home(self) -> None: if device_cache_data and device_cache_data.home_map_info: _LOGGER.debug("Home cache already populated, skipping discovery") self._home_map_info = device_cache_data.home_map_info - self._seed_seen_room_iot_ids(device_cache_data.home_map_info) self._discovery_completed = True try: self._home_map_content = { @@ -143,39 +138,7 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo: # or if the room name isn't unknown. rooms[room.segment_id] = room - # If we discovered at least one new unknown room iot_id for this map, fetch names from web API. - seen_room_iot_ids = self._seen_room_iot_ids_by_map.setdefault(map_info.map_flag, set()) - has_new_unknown_room_iot_id = any( - room.name == "Unknown" and room.iot_id not in seen_room_iot_ids for room in rooms.values() - ) - if self._web_api and has_new_unknown_room_iot_id: - try: - web_rooms = await self._web_api.get_rooms() - web_room_names = {str(r.id): r.name for r in web_rooms} - for segment_id, room in rooms.items(): - if room.name == "Unknown" and room.iot_id in web_room_names: - rooms[segment_id] = NamedRoomMapping( - segment_id=room.segment_id, - iot_id=room.iot_id, - name=web_room_names[room.iot_id], - ) - # Merge new rooms into home_data so future refreshes benefit - if web_rooms: - 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) - - # Replace remaining "Unknown" names with "Room {segment_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}", - ) - - self._seen_room_iot_ids_by_map[map_info.map_flag].update(room.iot_id for room in rooms.values()) + await self._rooms_trait.resolve_unknown_room_names(rooms) return CombinedMapInfo( map_flag=map_info.map_flag, @@ -293,7 +256,6 @@ async def _update_home_cache( await self._device_cache.set(device_cache_data) self._home_map_info = home_map_info self._home_map_content = home_map_content - self._seed_seen_room_iot_ids(home_map_info) async def _update_current_map( self, @@ -322,13 +284,7 @@ async def _update_current_map( if self._home_map_info is None: self._home_map_info = {} self._home_map_info[map_flag] = map_info - self._seed_seen_room_iot_ids({map_flag: map_info}) if self._home_map_content is None: self._home_map_content = {} self._home_map_content[map_flag] = map_content - - def _seed_seen_room_iot_ids(self, home_map_info: dict[int, CombinedMapInfo]) -> None: - """Seed known room iot_ids from cached/current map information.""" - for map_flag, map_info in home_map_info.items(): - self._seen_room_iot_ids_by_map.setdefault(map_flag, set()).update(room.iot_id for room in map_info.rooms) diff --git a/roborock/devices/traits/v1/rooms.py b/roborock/devices/traits/v1/rooms.py index 401858db..7fe3a192 100644 --- a/roborock/devices/traits/v1/rooms.py +++ b/roborock/devices/traits/v1/rooms.py @@ -6,6 +6,7 @@ from roborock.data import HomeData, HomeDataRoom, NamedRoomMapping, RoborockBase from roborock.devices.traits.v1 import common from roborock.roborock_typing import RoborockCommand +from roborock.web_api import UserWebApiClient _LOGGER = logging.getLogger(__name__) @@ -32,10 +33,12 @@ class RoomsTrait(Rooms, common.V1TraitMixin): command = RoborockCommand.GET_ROOM_MAPPING - def __init__(self, home_data: HomeData) -> None: + def __init__(self, home_data: HomeData, web_api: UserWebApiClient | None = None) -> None: """Initialize the RoomsTrait.""" super().__init__() self._home_data = home_data + self._web_api = web_api + self._seen_unknown_room_iot_ids: set[str] = set() @property def _iot_id_room_name_map(self) -> dict[str, str]: @@ -57,14 +60,46 @@ def _parse_response(self, response: common.V1ResponseData) -> Rooms: def merge_home_data_rooms(self, rooms: list[HomeDataRoom]) -> None: """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 ()) + existing_by_id = {room.id: room for room in updated_rooms} + for room in rooms: - if room.id not in existing_ids: + existing_room = existing_by_id.get(room.id) + if existing_room is None: updated_rooms.append(room) - existing_ids.add(room.id) + existing_by_id[room.id] = room + elif room.name and existing_room.name in ("", _DEFAULT_NAME): + existing_room.name = room.name + self._home_data.rooms = updated_rooms + async def resolve_unknown_room_names(self, rooms: dict[int, NamedRoomMapping]) -> None: + """Resolve unknown room names using home data and web API fallbacks.""" + unknown_room_iot_ids = {room.iot_id for room in rooms.values() if room.name == _DEFAULT_NAME} + new_unknown_room_iot_ids = unknown_room_iot_ids - self._seen_unknown_room_iot_ids + web_room_names: dict[str, str] = {} + + if self._web_api and new_unknown_room_iot_ids: + try: + web_rooms = await self._web_api.get_rooms() + except Exception: + _LOGGER.debug("Failed to fetch rooms from web API", exc_info=True) + else: + if web_rooms: + web_room_names = {str(room.id): room.name for room in web_rooms} + self.merge_home_data_rooms(web_rooms) + + for segment_id, room in rooms.items(): + if room.name != _DEFAULT_NAME: + continue + rooms[segment_id] = NamedRoomMapping( + segment_id=room.segment_id, + iot_id=room.iot_id, + name=web_room_names.get(room.iot_id, f"Room {room.segment_id}"), + ) + + self._seen_unknown_room_iot_ids.update(unknown_room_iot_ids) + def _extract_segment_pairs(response: list) -> list[tuple[int, str]]: """Extract segment_id and iot_id pairs from the response. diff --git a/tests/devices/traits/v1/test_home.py b/tests/devices/traits/v1/test_home.py index 182d5176..13f488b4 100644 --- a/tests/devices/traits/v1/test_home.py +++ b/tests/devices/traits/v1/test_home.py @@ -157,7 +157,8 @@ def home_trait( mock_web_api: AsyncMock, ) -> HomeTrait: """Create a HomeTrait instance with mocked dependencies.""" - return HomeTrait(status_trait, maps_trait, map_content_trait, rooms_trait, device_cache, mock_web_api) + rooms_trait._web_api = mock_web_api + return HomeTrait(status_trait, maps_trait, map_content_trait, rooms_trait, device_cache) @pytest.fixture(autouse=True) diff --git a/tests/devices/traits/v1/test_rooms.py b/tests/devices/traits/v1/test_rooms.py index e5fd7e66..492712c3 100644 --- a/tests/devices/traits/v1/test_rooms.py +++ b/tests/devices/traits/v1/test_rooms.py @@ -5,6 +5,7 @@ import pytest +from roborock.data.containers import HomeDataRoom, NamedRoomMapping from roborock.devices.device import RoborockDevice from roborock.devices.traits.v1.rooms import RoomsTrait from roborock.devices.traits.v1.status import StatusTrait @@ -70,3 +71,56 @@ async def test_refresh_rooms_trait( # Verify the RPC call was made correctly assert mock_rpc_channel.send_command.call_count == 1 mock_rpc_channel.send_command.assert_any_call(RoborockCommand.GET_ROOM_MAPPING) + + +def test_merge_home_data_rooms_appends_new_rooms(rooms_trait: RoomsTrait) -> None: + """Test merge_home_data_rooms appends new rooms without replacing known names.""" + rooms_trait.merge_home_data_rooms( + [ + HomeDataRoom(id=2362048, name="Living Room"), + HomeDataRoom(id=9999999, name="Office"), + ] + ) + + home_data_rooms = {str(room.id): room.name for room in rooms_trait._home_data.rooms} + assert home_data_rooms["2362048"] == "Example room 1" + assert home_data_rooms["9999999"] == "Office" + + +async def test_resolve_unknown_room_names_web_api_called_once( + rooms_trait: RoomsTrait, + web_api_client: AsyncMock, +) -> None: + """Test unknown room IDs trigger one web lookup per iot_id.""" + web_api_client.get_rooms.return_value = [ + HomeDataRoom(id=2362048, name="Living Room"), + ] + + room_map = { + 16: NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + } + await rooms_trait.resolve_unknown_room_names(room_map) + assert room_map[16].name == "Living Room" + + second_room_map = { + 16: NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), + } + await rooms_trait.resolve_unknown_room_names(second_room_map) + assert second_room_map[16].name == "Room 16" + web_api_client.get_rooms.assert_called_once() + + +async def test_resolve_unknown_room_names_falls_back_to_segment_id( + rooms_trait: RoomsTrait, + web_api_client: AsyncMock, +) -> None: + """Test unresolved unknown names use Room {segment_id} fallback.""" + web_api_client.get_rooms.return_value = [] + room_map = { + 33: NamedRoomMapping(segment_id=33, iot_id="9999911", name="Unknown"), + } + + await rooms_trait.resolve_unknown_room_names(room_map) + + assert room_map[33].name == "Room 33" + web_api_client.get_rooms.assert_called_once() From d11a52ca3a03ca8ac49f7b9cf8df1b1cb52ab583 Mon Sep 17 00:00:00 2001 From: Luke Date: Sat, 7 Mar 2026 15:25:41 -0500 Subject: [PATCH 4/8] chore: address comments --- roborock/devices/traits/v1/home.py | 8 +- roborock/devices/traits/v1/rooms.py | 54 +++++++------ tests/devices/traits/v1/test_home.py | 109 ++++++++++---------------- tests/devices/traits/v1/test_rooms.py | 44 ++++++----- 4 files changed, 102 insertions(+), 113 deletions(-) diff --git a/roborock/devices/traits/v1/home.py b/roborock/devices/traits/v1/home.py index 53103581..9b874814 100644 --- a/roborock/devices/traits/v1/home.py +++ b/roborock/devices/traits/v1/home.py @@ -138,7 +138,13 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo: # or if the room name isn't unknown. rooms[room.segment_id] = room - await self._rooms_trait.resolve_unknown_room_names(rooms) + 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}", + ) return CombinedMapInfo( map_flag=map_info.map_flag, diff --git a/roborock/devices/traits/v1/rooms.py b/roborock/devices/traits/v1/rooms.py index 7fe3a192..06e11867 100644 --- a/roborock/devices/traits/v1/rooms.py +++ b/roborock/devices/traits/v1/rooms.py @@ -33,13 +33,26 @@ class RoomsTrait(Rooms, common.V1TraitMixin): command = RoborockCommand.GET_ROOM_MAPPING - def __init__(self, home_data: HomeData, web_api: UserWebApiClient | None = None) -> None: + def __init__(self, home_data: HomeData, web_api: UserWebApiClient) -> None: """Initialize the RoomsTrait.""" super().__init__() self._home_data = home_data self._web_api = web_api self._seen_unknown_room_iot_ids: set[str] = set() + async def refresh(self) -> None: + """Refresh room mappings and backfill unknown room names from the web API.""" + response = await self.rpc_channel.send_command(self.command) + if not isinstance(response, list): + raise ValueError(f"Unexpected RoomsTrait response format: {response!r}") + + segment_pairs = _extract_segment_pairs(response) + await self._populate_missing_home_data_rooms(segment_pairs) + + new_data = self._parse_response(response) + self._update_trait_values(new_data) + _LOGGER.debug("Refreshed %s: %s", self.__class__.__name__, new_data) + @property def _iot_id_room_name_map(self) -> dict[str, str]: """Returns a dictionary of Room IOT IDs to room names.""" @@ -73,30 +86,23 @@ def merge_home_data_rooms(self, rooms: list[HomeDataRoom]) -> None: self._home_data.rooms = updated_rooms - async def resolve_unknown_room_names(self, rooms: dict[int, NamedRoomMapping]) -> None: - """Resolve unknown room names using home data and web API fallbacks.""" - unknown_room_iot_ids = {room.iot_id for room in rooms.values() if room.name == _DEFAULT_NAME} + async def _populate_missing_home_data_rooms(self, segment_pairs: list[tuple[int, str]]) -> None: + """Load missing room names into home data for newly-seen unknown room ids.""" + name_map = self._iot_id_room_name_map + unknown_room_iot_ids = { + iot_id for _, iot_id in segment_pairs if name_map.get(iot_id, _DEFAULT_NAME) == _DEFAULT_NAME + } new_unknown_room_iot_ids = unknown_room_iot_ids - self._seen_unknown_room_iot_ids - web_room_names: dict[str, str] = {} - - if self._web_api and new_unknown_room_iot_ids: - try: - web_rooms = await self._web_api.get_rooms() - except Exception: - _LOGGER.debug("Failed to fetch rooms from web API", exc_info=True) - else: - if web_rooms: - web_room_names = {str(room.id): room.name for room in web_rooms} - self.merge_home_data_rooms(web_rooms) - - for segment_id, room in rooms.items(): - if room.name != _DEFAULT_NAME: - continue - rooms[segment_id] = NamedRoomMapping( - segment_id=room.segment_id, - iot_id=room.iot_id, - name=web_room_names.get(room.iot_id, f"Room {room.segment_id}"), - ) + if not new_unknown_room_iot_ids: + return + + try: + web_rooms = await self._web_api.get_rooms() + except Exception: + _LOGGER.debug("Failed to fetch rooms from web API", exc_info=True) + else: + if web_rooms: + self.merge_home_data_rooms(web_rooms) self._seen_unknown_room_iot_ids.update(unknown_room_iot_ids) diff --git a/tests/devices/traits/v1/test_home.py b/tests/devices/traits/v1/test_home.py index 13f488b4..476e4de4 100644 --- a/tests/devices/traits/v1/test_home.py +++ b/tests/devices/traits/v1/test_home.py @@ -140,11 +140,10 @@ def rooms_trait(device: RoborockDevice) -> RoomsTrait: @pytest.fixture -def mock_web_api() -> AsyncMock: - """Create a mock web API client.""" - mock = AsyncMock() - mock.get_rooms.return_value = [] - return mock +def mock_web_api(web_api_client: AsyncMock) -> AsyncMock: + """Alias the shared web API fixture for readability in this module.""" + web_api_client.get_rooms.return_value = [] + return web_api_client @pytest.fixture @@ -154,10 +153,8 @@ def home_trait( map_content_trait: MapContentTrait, rooms_trait: RoomsTrait, device_cache: DeviceCache, - mock_web_api: AsyncMock, ) -> HomeTrait: """Create a HomeTrait instance with mocked dependencies.""" - rooms_trait._web_api = mock_web_api return HomeTrait(status_trait, maps_trait, map_content_trait, rooms_trait, device_cache) @@ -630,7 +627,6 @@ async def test_single_map_no_switching( async def test_refresh_map_info_room_override_and_addition_logic( home_trait: HomeTrait, rooms_trait: RoomsTrait, - mock_web_api: AsyncMock, ) -> None: """Test the room override and addition logic in _refresh_map_info. @@ -671,9 +667,6 @@ async def test_refresh_map_info_room_override_and_addition_logic( NamedRoomMapping(segment_id=18, iot_id="2362041", name="Example room 3"), # Not in map_info, should be added ] - # Mock web API to return empty rooms (no resolution) - mock_web_api.get_rooms.return_value = [] - # Mock rooms_trait.refresh to prevent actual device calls with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): result = await home_trait._refresh_map_info(map_info) @@ -708,32 +701,29 @@ async def test_refresh_map_info_room_override_and_addition_logic( async def test_get_rooms_resolves_unknown_rooms( home_trait: HomeTrait, - rooms_trait: RoomsTrait, + mock_rpc_channel: AsyncMock, mock_web_api: AsyncMock, ) -> None: """Test that get_rooms from web API resolves unknown room names.""" + room_mapping_data = [[16, "9999801"], [17, "9999802"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data] + map_info = MultiMapsListMapInfo( map_flag=0, name="Test Map", rooms=[ - MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), - MultiMapsListRoom(id=17, iot_name_id="2362044", iot_name=None), + MultiMapsListRoom(id=16, iot_name_id="9999801", iot_name=None), + MultiMapsListRoom(id=17, iot_name_id="9999802", iot_name=None), ], ) - rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - NamedRoomMapping(segment_id=17, iot_id="2362044", name="Unknown"), - ] - # Web API returns fresh room names mock_web_api.get_rooms.return_value = [ - HomeDataRoom(id=2362048, name="Living Room"), - HomeDataRoom(id=2362044, name="Kitchen"), + HomeDataRoom(id=9999801, name="Living Room"), + HomeDataRoom(id=9999802, name="Kitchen"), ] - with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): - result = await home_trait._refresh_map_info(map_info) + result = await home_trait._refresh_map_info(map_info) sorted_rooms = sorted(result.rooms, key=lambda r: r.segment_id) assert sorted_rooms[0].name == "Living Room" @@ -743,28 +733,26 @@ async def test_get_rooms_resolves_unknown_rooms( async def test_get_rooms_called_once_for_same_unknown_room( home_trait: HomeTrait, - rooms_trait: RoomsTrait, + mock_rpc_channel: AsyncMock, mock_web_api: AsyncMock, ) -> None: """Test that get_rooms is not re-called for an already-seen unknown room.""" + room_mapping_data = [[16, "9999701"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data] + map_info = MultiMapsListMapInfo( map_flag=42, name="Test Map", rooms=[ - MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), + MultiMapsListRoom(id=16, iot_name_id="9999701", iot_name=None), ], ) - rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - ] - # Web API returns empty (no resolution) mock_web_api.get_rooms.return_value = [] - with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): - result1 = await home_trait._refresh_map_info(map_info) - result2 = await home_trait._refresh_map_info(map_info) + result1 = await home_trait._refresh_map_info(map_info) + result2 = await home_trait._refresh_map_info(map_info) # Both calls should fall back to "Room 16" assert result1.rooms[0].name == "Room 16" @@ -775,34 +763,27 @@ async def test_get_rooms_called_once_for_same_unknown_room( async def test_get_rooms_called_again_for_new_unknown_room( home_trait: HomeTrait, - rooms_trait: RoomsTrait, + mock_rpc_channel: AsyncMock, mock_web_api: AsyncMock, ) -> None: """Test that get_rooms is called again when a new unknown room appears.""" + room_mapping_data_1 = [[16, "9999601"]] + room_mapping_data_2 = [[16, "9999601"], [17, "9999602"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2] + map_info = MultiMapsListMapInfo( map_flag=42, name="Test Map", rooms=[ - MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), + MultiMapsListRoom(id=16, iot_name_id="9999601", iot_name=None), ], ) - rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - ] - # Web API returns empty (no resolution) mock_web_api.get_rooms.return_value = [] - with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): - result1 = await home_trait._refresh_map_info(map_info) - - # Add a brand-new unknown room for the same map - rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - NamedRoomMapping(segment_id=17, iot_id="2362044", name="Unknown"), - ] - result2 = await home_trait._refresh_map_info(map_info) + result1 = await home_trait._refresh_map_info(map_info) + result2 = await home_trait._refresh_map_info(map_info) assert sorted(room.name for room in result1.rooms) == ["Room 16"] assert sorted(room.name for room in result2.rooms) == ["Room 16", "Room 17"] @@ -811,56 +792,48 @@ async def test_get_rooms_called_again_for_new_unknown_room( async def test_get_rooms_called_again_for_new_unknown_iot_id_same_segment( home_trait: HomeTrait, - rooms_trait: RoomsTrait, + mock_rpc_channel: AsyncMock, mock_web_api: AsyncMock, ) -> None: """Test that get_rooms is called again for a new unknown iot_id on the same segment.""" + room_mapping_data_1 = [[16, "9999501"]] + room_mapping_data_2 = [[16, "9999502"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2] + map_info = MultiMapsListMapInfo( map_flag=42, name="Test Map", ) - rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - ] - mock_web_api.get_rooms.return_value = [] - with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): - await home_trait._refresh_map_info(map_info) - - # Same segment, but now with a different iot_id. - rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362999", name="Unknown"), - ] - await home_trait._refresh_map_info(map_info) + await home_trait._refresh_map_info(map_info) + await home_trait._refresh_map_info(map_info) assert mock_web_api.get_rooms.call_count == 2 async def test_get_rooms_failure_falls_back_to_room_segment_id( home_trait: HomeTrait, - rooms_trait: RoomsTrait, + mock_rpc_channel: AsyncMock, mock_web_api: AsyncMock, ) -> None: """Test that get_rooms failure gracefully falls back to 'Room {segment_id}'.""" + room_mapping_data = [[16, "9999401"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data] + map_info = MultiMapsListMapInfo( map_flag=7, name="Test Map", rooms=[ - MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None), + MultiMapsListRoom(id=16, iot_name_id="9999401", iot_name=None), ], ) - rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - ] - # Web API raises an exception mock_web_api.get_rooms.side_effect = Exception("API error") - with patch.object(rooms_trait, "refresh", new_callable=AsyncMock): - result = await home_trait._refresh_map_info(map_info) + result = await home_trait._refresh_map_info(map_info) assert result.rooms[0].name == "Room 16" mock_web_api.get_rooms.assert_called_once() diff --git a/tests/devices/traits/v1/test_rooms.py b/tests/devices/traits/v1/test_rooms.py index 492712c3..382c019f 100644 --- a/tests/devices/traits/v1/test_rooms.py +++ b/tests/devices/traits/v1/test_rooms.py @@ -87,40 +87,44 @@ def test_merge_home_data_rooms_appends_new_rooms(rooms_trait: RoomsTrait) -> Non assert home_data_rooms["9999999"] == "Office" -async def test_resolve_unknown_room_names_web_api_called_once( +async def test_refresh_unknown_room_names_web_api_called_once( rooms_trait: RoomsTrait, web_api_client: AsyncMock, + mock_rpc_channel: AsyncMock, ) -> None: """Test unknown room IDs trigger one web lookup per iot_id.""" web_api_client.get_rooms.return_value = [ - HomeDataRoom(id=2362048, name="Living Room"), + HomeDataRoom(id=9999911, name="Living Room"), ] - room_map = { - 16: NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - } - await rooms_trait.resolve_unknown_room_names(room_map) - assert room_map[16].name == "Living Room" - - second_room_map = { - 16: NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), - } - await rooms_trait.resolve_unknown_room_names(second_room_map) - assert second_room_map[16].name == "Room 16" + room_mapping_data = [[16, "9999911"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data] + + await rooms_trait.refresh() + assert rooms_trait.rooms + assert rooms_trait.rooms[0].name == "Living Room" + + await rooms_trait.refresh() + assert rooms_trait.rooms + assert rooms_trait.rooms[0].name == "Living Room" web_api_client.get_rooms.assert_called_once() -async def test_resolve_unknown_room_names_falls_back_to_segment_id( +async def test_refresh_unknown_room_names_unresolved_keeps_unknown( rooms_trait: RoomsTrait, web_api_client: AsyncMock, + mock_rpc_channel: AsyncMock, ) -> None: - """Test unresolved unknown names use Room {segment_id} fallback.""" + """Test unresolved unknown names stay unknown in RoomsTrait.""" web_api_client.get_rooms.return_value = [] - room_map = { - 33: NamedRoomMapping(segment_id=33, iot_id="9999911", name="Unknown"), - } + room_mapping_data = [[33, "9999922"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data] - await rooms_trait.resolve_unknown_room_names(room_map) + await rooms_trait.refresh() + assert rooms_trait.rooms + assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Unknown") - assert room_map[33].name == "Room 33" + await rooms_trait.refresh() + assert rooms_trait.rooms + assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Unknown") web_api_client.get_rooms.assert_called_once() From 75a6f49129008d01f1968857e4ced3bf2f97fa3a Mon Sep 17 00:00:00 2001 From: Luke Date: Sat, 7 Mar 2026 20:47:36 -0500 Subject: [PATCH 5/8] chore: address comments --- roborock/devices/traits/v1/home.py | 24 +++++----- roborock/devices/traits/v1/rooms.py | 65 +++++++++---------------- tests/devices/traits/v1/test_home.py | 8 ++-- tests/devices/traits/v1/test_rooms.py | 69 +++++++++++++++++---------- 4 files changed, 85 insertions(+), 81 deletions(-) diff --git a/roborock/devices/traits/v1/home.py b/roborock/devices/traits/v1/home.py index 9b874814..cbd11866 100644 --- a/roborock/devices/traits/v1/home.py +++ b/roborock/devices/traits/v1/home.py @@ -37,6 +37,10 @@ MAP_SLEEP = 3 +def _is_default_room_name(name: str, segment_id: int) -> bool: + return name in ("Unknown", f"Room {segment_id}") + + class HomeTrait(RoborockBase, common.V1TraitMixin): """Trait that represents a full view of the home layout.""" @@ -129,22 +133,20 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo: name=room.iot_name or "Unknown", ) - # Add rooms from rooms_trait. If room already exists and rooms_trait has "Unknown", don't override. + # Add rooms from rooms_trait. + # Prefer existing non-default map_info names over fallback names from RoomsTrait. if self._rooms_trait.rooms: for room in self._rooms_trait.rooms: if room.segment_id is not None and room.name: - if room.segment_id not in rooms or room.name != "Unknown": - # Add the room to rooms if the room segment is not already in it - # or if the room name isn't unknown. + existing_room = rooms.get(room.segment_id) + if existing_room is None: rooms[room.segment_id] = room + continue - 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}", - ) + if _is_default_room_name(existing_room.name, existing_room.segment_id) or not _is_default_room_name( + room.name, room.segment_id + ): + rooms[room.segment_id] = room return CombinedMapInfo( map_flag=map_info.map_flag, diff --git a/roborock/devices/traits/v1/rooms.py b/roborock/devices/traits/v1/rooms.py index 06e11867..698fa90f 100644 --- a/roborock/devices/traits/v1/rooms.py +++ b/roborock/devices/traits/v1/rooms.py @@ -3,15 +3,13 @@ import logging from dataclasses import dataclass -from roborock.data import HomeData, HomeDataRoom, NamedRoomMapping, RoborockBase +from roborock.data import HomeData, NamedRoomMapping, RoborockBase from roborock.devices.traits.v1 import common from roborock.roborock_typing import RoborockCommand from roborock.web_api import UserWebApiClient _LOGGER = logging.getLogger(__name__) -_DEFAULT_NAME = "Unknown" - @dataclass class Rooms(RoborockBase): @@ -46,10 +44,10 @@ async def refresh(self) -> None: if not isinstance(response, list): raise ValueError(f"Unexpected RoomsTrait response format: {response!r}") - segment_pairs = _extract_segment_pairs(response) - await self._populate_missing_home_data_rooms(segment_pairs) + segment_map = _extract_segment_map(response) + await self._populate_missing_home_data_rooms(segment_map) - new_data = self._parse_response(response) + new_data = self._parse_response(response, segment_map) self._update_trait_values(new_data) _LOGGER.debug("Refreshed %s: %s", self.__class__.__name__, new_data) @@ -58,42 +56,25 @@ def _iot_id_room_name_map(self) -> dict[str, str]: """Returns a dictionary of Room IOT IDs to room names.""" return {str(room.id): room.name for room in self._home_data.rooms or ()} - def _parse_response(self, response: common.V1ResponseData) -> Rooms: + def _parse_response(self, response: common.V1ResponseData, segment_map: dict[int, str] | None = None) -> Rooms: """Parse the response from the device into a list of NamedRoomMapping.""" if not isinstance(response, list): raise ValueError(f"Unexpected RoomsTrait response format: {response!r}") + if segment_map is None: + segment_map = _extract_segment_map(response) name_map = self._iot_id_room_name_map - segment_pairs = _extract_segment_pairs(response) return Rooms( rooms=[ - NamedRoomMapping(segment_id=segment_id, iot_id=iot_id, name=name_map.get(iot_id, _DEFAULT_NAME)) - for segment_id, iot_id in segment_pairs + NamedRoomMapping(segment_id=segment_id, iot_id=iot_id, name=name_map.get(iot_id, f"Room {segment_id}")) + for segment_id, iot_id in segment_map.items() ] ) - def merge_home_data_rooms(self, rooms: list[HomeDataRoom]) -> None: - """Merge newly discovered rooms into home data by room id.""" - updated_rooms = list(self._home_data.rooms or ()) - existing_by_id = {room.id: room for room in updated_rooms} - - for room in rooms: - existing_room = existing_by_id.get(room.id) - if existing_room is None: - updated_rooms.append(room) - existing_by_id[room.id] = room - elif room.name and existing_room.name in ("", _DEFAULT_NAME): - existing_room.name = room.name - - self._home_data.rooms = updated_rooms - - async def _populate_missing_home_data_rooms(self, segment_pairs: list[tuple[int, str]]) -> None: + async def _populate_missing_home_data_rooms(self, segment_map: dict[int, str]) -> None: """Load missing room names into home data for newly-seen unknown room ids.""" - name_map = self._iot_id_room_name_map - unknown_room_iot_ids = { - iot_id for _, iot_id in segment_pairs if name_map.get(iot_id, _DEFAULT_NAME) == _DEFAULT_NAME - } - new_unknown_room_iot_ids = unknown_room_iot_ids - self._seen_unknown_room_iot_ids - if not new_unknown_room_iot_ids: + missing_room_iot_ids = set(segment_map.values()) - set(self._iot_id_room_name_map.keys()) + new_missing_room_iot_ids = missing_room_iot_ids - self._seen_unknown_room_iot_ids + if not new_missing_room_iot_ids: return try: @@ -101,18 +82,18 @@ async def _populate_missing_home_data_rooms(self, segment_pairs: list[tuple[int, except Exception: _LOGGER.debug("Failed to fetch rooms from web API", exc_info=True) else: - if web_rooms: - self.merge_home_data_rooms(web_rooms) + if isinstance(web_rooms, list) and web_rooms: + self._home_data.rooms = web_rooms - self._seen_unknown_room_iot_ids.update(unknown_room_iot_ids) + self._seen_unknown_room_iot_ids.update(missing_room_iot_ids) -def _extract_segment_pairs(response: list) -> list[tuple[int, str]]: - """Extract segment_id and iot_id pairs from the response. +def _extract_segment_map(response: list) -> dict[int, str]: + """Extract a segment_id -> iot_id mapping from the response. The response format can be either a flat list of [segment_id, iot_id] or a list of lists, where each inner list is a pair of [segment_id, iot_id]. This - function normalizes the response into a list of (segment_id, iot_id) tuples + function normalizes the response into a dict of segment_id to iot_id. NOTE: We currently only partial samples of the room mapping formats, so improving test coverage with samples from a real device with this format @@ -120,13 +101,13 @@ def _extract_segment_pairs(response: list) -> list[tuple[int, str]]: """ if len(response) == 2 and not isinstance(response[0], list): segment_id, iot_id = response[0], response[1] - return [(segment_id, iot_id)] + return {segment_id: str(iot_id)} - segment_pairs: list[tuple[int, str]] = [] + segment_map: dict[int, str] = {} for part in response: if not isinstance(part, list) or len(part) < 2: _LOGGER.warning("Unexpected room mapping entry format: %r", part) continue segment_id, iot_id = part[0], part[1] - segment_pairs.append((segment_id, iot_id)) - return segment_pairs + segment_map[segment_id] = str(iot_id) + return segment_map diff --git a/tests/devices/traits/v1/test_home.py b/tests/devices/traits/v1/test_home.py index 476e4de4..a8f88956 100644 --- a/tests/devices/traits/v1/test_home.py +++ b/tests/devices/traits/v1/test_home.py @@ -654,16 +654,16 @@ async def test_refresh_map_info_room_override_and_addition_logic( ) # Mock rooms_trait to return multiple rooms covering all scenarios: - # - segment_id 16 with "Unknown": exists in map_info, should NOT override + # - segment_id 16 with fallback name: exists in map_info, should NOT override # - segment_id 19 with valid name: exists in map_info, should override - # - segment_id 17 with "Unknown": not in map_info, should be added + # - segment_id 17 with fallback name: not in map_info, should be added # - segment_id 18 with valid name: not in map_info, should be added rooms_trait.rooms = [ - NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"), # Exists in map_info, should not override + NamedRoomMapping(segment_id=16, iot_id="2362048", name="Room 16"), # Exists in map_info, should not override NamedRoomMapping( segment_id=19, iot_id="2362042", name="Updated Bedroom Name" ), # Exists in map_info, should override - NamedRoomMapping(segment_id=17, iot_id="2362044", name="Unknown"), # Not in map_info, should be added + NamedRoomMapping(segment_id=17, iot_id="2362044", name="Room 17"), # Not in map_info, should be added NamedRoomMapping(segment_id=18, iot_id="2362041", name="Example room 3"), # Not in map_info, should be added ] diff --git a/tests/devices/traits/v1/test_rooms.py b/tests/devices/traits/v1/test_rooms.py index 382c019f..b64e2c2d 100644 --- a/tests/devices/traits/v1/test_rooms.py +++ b/tests/devices/traits/v1/test_rooms.py @@ -73,18 +73,35 @@ async def test_refresh_rooms_trait( mock_rpc_channel.send_command.assert_any_call(RoborockCommand.GET_ROOM_MAPPING) -def test_merge_home_data_rooms_appends_new_rooms(rooms_trait: RoomsTrait) -> None: - """Test merge_home_data_rooms appends new rooms without replacing known names.""" - rooms_trait.merge_home_data_rooms( - [ +async def test_refresh_unknown_room_names_overwrites_home_data( + rooms_trait: RoomsTrait, + web_api_client: AsyncMock, + mock_rpc_channel: AsyncMock, +) -> None: + """Test web rooms are used to refresh home data for missing iot ids.""" + original_rooms = list(rooms_trait._home_data.rooms or ()) + try: + web_api_client.get_rooms.return_value = [ HomeDataRoom(id=2362048, name="Living Room"), + HomeDataRoom(id=2362044, name="Example room 2"), + HomeDataRoom(id=2362041, name="Example room 3"), HomeDataRoom(id=9999999, name="Office"), ] - ) - home_data_rooms = {str(room.id): room.name for room in rooms_trait._home_data.rooms} - assert home_data_rooms["2362048"] == "Example room 1" - assert home_data_rooms["9999999"] == "Office" + room_mapping_data = [[16, "2362048"], [17, "9999999"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data] + + await rooms_trait.refresh() + + assert rooms_trait.rooms + assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=16, iot_id="2362048", name="Living Room") + assert rooms_trait.rooms[1] == NamedRoomMapping(segment_id=17, iot_id="9999999", name="Office") + + home_data_rooms = {str(room.id): room.name for room in rooms_trait._home_data.rooms or ()} + assert home_data_rooms["2362048"] == "Living Room" + assert home_data_rooms["9999999"] == "Office" + finally: + rooms_trait._home_data.rooms = original_rooms async def test_refresh_unknown_room_names_web_api_called_once( @@ -93,38 +110,42 @@ async def test_refresh_unknown_room_names_web_api_called_once( mock_rpc_channel: AsyncMock, ) -> None: """Test unknown room IDs trigger one web lookup per iot_id.""" - web_api_client.get_rooms.return_value = [ - HomeDataRoom(id=9999911, name="Living Room"), - ] + original_rooms = list(rooms_trait._home_data.rooms or ()) + try: + web_api_client.get_rooms.return_value = [ + HomeDataRoom(id=9999911, name="Living Room"), + ] - room_mapping_data = [[16, "9999911"]] - mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data] + room_mapping_data = [[16, "9999911"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data] - await rooms_trait.refresh() - assert rooms_trait.rooms - assert rooms_trait.rooms[0].name == "Living Room" + await rooms_trait.refresh() + assert rooms_trait.rooms + assert rooms_trait.rooms[0].name == "Living Room" - await rooms_trait.refresh() - assert rooms_trait.rooms - assert rooms_trait.rooms[0].name == "Living Room" - web_api_client.get_rooms.assert_called_once() + await rooms_trait.refresh() + assert rooms_trait.rooms + assert rooms_trait.rooms[0].name == "Living Room" + web_api_client.get_rooms.assert_called_once() + finally: + rooms_trait._home_data.rooms = original_rooms -async def test_refresh_unknown_room_names_unresolved_keeps_unknown( +async def test_refresh_unknown_room_names_unresolved_uses_room_fallback( rooms_trait: RoomsTrait, web_api_client: AsyncMock, mock_rpc_channel: AsyncMock, ) -> None: - """Test unresolved unknown names stay unknown in RoomsTrait.""" + """Test unresolved unknown names use Room fallback in RoomsTrait.""" web_api_client.get_rooms.return_value = [] room_mapping_data = [[33, "9999922"]] mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data] await rooms_trait.refresh() assert rooms_trait.rooms - assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Unknown") + assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Room 33") await rooms_trait.refresh() assert rooms_trait.rooms - assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Unknown") + assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Room 33") web_api_client.get_rooms.assert_called_once() From 329f545c09b69980a21745bc8030d1ff0c7a4387 Mon Sep 17 00:00:00 2001 From: Luke Date: Sat, 7 Mar 2026 20:55:19 -0500 Subject: [PATCH 6/8] fix: tests --- tests/devices/traits/v1/fixtures.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/devices/traits/v1/fixtures.py b/tests/devices/traits/v1/fixtures.py index b50d62f2..3cee01dd 100644 --- a/tests/devices/traits/v1/fixtures.py +++ b/tests/devices/traits/v1/fixtures.py @@ -1,5 +1,6 @@ """Fixtures for V1 trait tests.""" +from copy import deepcopy from unittest.mock import AsyncMock import pytest @@ -89,7 +90,7 @@ def device_fixture( trait=v1.create( device_info.duid, product, - HOME_DATA, + deepcopy(HOME_DATA), mock_rpc_channel, mock_mqtt_rpc_channel, mock_map_rpc_channel, From 952d2e27440fda72356eb7d37dd3fe7b8c13f519 Mon Sep 17 00:00:00 2001 From: Luke Date: Sun, 8 Mar 2026 15:02:17 -0400 Subject: [PATCH 7/8] fix: comments --- roborock/devices/traits/v1/home.py | 14 +-- tests/devices/traits/v1/test_home.py | 160 ++------------------------ tests/devices/traits/v1/test_rooms.py | 55 +++++++++ 3 files changed, 66 insertions(+), 163 deletions(-) diff --git a/roborock/devices/traits/v1/home.py b/roborock/devices/traits/v1/home.py index cbd11866..b1ee666d 100644 --- a/roborock/devices/traits/v1/home.py +++ b/roborock/devices/traits/v1/home.py @@ -37,10 +37,6 @@ MAP_SLEEP = 3 -def _is_default_room_name(name: str, segment_id: int) -> bool: - return name in ("Unknown", f"Room {segment_id}") - - class HomeTrait(RoborockBase, common.V1TraitMixin): """Trait that represents a full view of the home layout.""" @@ -134,18 +130,12 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo: ) # Add rooms from rooms_trait. - # Prefer existing non-default map_info names over fallback names from RoomsTrait. + # Keep existing names from map_info unless they are unknown. if self._rooms_trait.rooms: for room in self._rooms_trait.rooms: if room.segment_id is not None and room.name: existing_room = rooms.get(room.segment_id) - if existing_room is None: - rooms[room.segment_id] = room - continue - - if _is_default_room_name(existing_room.name, existing_room.segment_id) or not _is_default_room_name( - room.name, room.segment_id - ): + if existing_room is None or existing_room.name == "Unknown": rooms[room.segment_id] = room return CombinedMapInfo( diff --git a/tests/devices/traits/v1/test_home.py b/tests/devices/traits/v1/test_home.py index a8f88956..7904dbcf 100644 --- a/tests/devices/traits/v1/test_home.py +++ b/tests/devices/traits/v1/test_home.py @@ -6,7 +6,7 @@ import pytest -from roborock.data.containers import CombinedMapInfo, HomeDataRoom, NamedRoomMapping +from roborock.data.containers import CombinedMapInfo, NamedRoomMapping from roborock.data.v1.v1_code_mappings import RoborockStateCode from roborock.data.v1.v1_containers import MultiMapsListMapInfo, MultiMapsListRoom from roborock.devices.cache import DeviceCache, DeviceCacheData, InMemoryCache @@ -624,17 +624,15 @@ async def test_single_map_no_switching( assert len(load_map_calls) == 0 -async def test_refresh_map_info_room_override_and_addition_logic( +async def test_refresh_map_info_prefers_map_info_names_and_adds_missing_rooms( home_trait: HomeTrait, rooms_trait: RoomsTrait, ) -> None: - """Test the room override and addition logic in _refresh_map_info. + """Test room merge behavior in _refresh_map_info. This test verifies: - 1. Room with "Unknown" does not override existing room from map_info - 2. Room with valid name overrides existing room from map_info - 3. Room with "Unknown" is added if not already in rooms - 4. Room with valid name is added if not already in rooms + 1. Existing map_info room names are preserved. + 2. Missing rooms from RoomsTrait are added. """ map_info = MultiMapsListMapInfo( map_flag=0, @@ -655,14 +653,14 @@ async def test_refresh_map_info_room_override_and_addition_logic( # Mock rooms_trait to return multiple rooms covering all scenarios: # - segment_id 16 with fallback name: exists in map_info, should NOT override - # - segment_id 19 with valid name: exists in map_info, should override + # - segment_id 19 with valid name: exists in map_info, should NOT override # - segment_id 17 with fallback name: not in map_info, should be added # - segment_id 18 with valid name: not in map_info, should be added rooms_trait.rooms = [ NamedRoomMapping(segment_id=16, iot_id="2362048", name="Room 16"), # Exists in map_info, should not override NamedRoomMapping( segment_id=19, iot_id="2362042", name="Updated Bedroom Name" - ), # Exists in map_info, should override + ), # Exists in map_info, should not override NamedRoomMapping(segment_id=17, iot_id="2362044", name="Room 17"), # Not in map_info, should be added NamedRoomMapping(segment_id=18, iot_id="2362041", name="Example room 3"), # Not in map_info, should be added ] @@ -693,147 +691,7 @@ async def test_refresh_map_info_room_override_and_addition_logic( assert sorted_rooms[2].name == "Example room 3" assert sorted_rooms[2].iot_id == "2362041" - # Room 19: from map_info, overridden by rooms_trait with valid name + # Room 19: from map_info, kept (not overridden by rooms_trait) assert sorted_rooms[3].segment_id == 19 - assert sorted_rooms[3].name == "Updated Bedroom Name" + assert sorted_rooms[3].name == "Bedroom from map_info" assert sorted_rooms[3].iot_id == "2362042" - - -async def test_get_rooms_resolves_unknown_rooms( - home_trait: HomeTrait, - mock_rpc_channel: AsyncMock, - mock_web_api: AsyncMock, -) -> None: - """Test that get_rooms from web API resolves unknown room names.""" - room_mapping_data = [[16, "9999801"], [17, "9999802"]] - mock_rpc_channel.send_command.side_effect = [room_mapping_data] - - map_info = MultiMapsListMapInfo( - map_flag=0, - name="Test Map", - rooms=[ - MultiMapsListRoom(id=16, iot_name_id="9999801", iot_name=None), - MultiMapsListRoom(id=17, iot_name_id="9999802", iot_name=None), - ], - ) - - # Web API returns fresh room names - mock_web_api.get_rooms.return_value = [ - HomeDataRoom(id=9999801, name="Living Room"), - HomeDataRoom(id=9999802, name="Kitchen"), - ] - - result = await home_trait._refresh_map_info(map_info) - - sorted_rooms = sorted(result.rooms, key=lambda r: r.segment_id) - assert sorted_rooms[0].name == "Living Room" - assert sorted_rooms[1].name == "Kitchen" - mock_web_api.get_rooms.assert_called_once() - - -async def test_get_rooms_called_once_for_same_unknown_room( - home_trait: HomeTrait, - mock_rpc_channel: AsyncMock, - mock_web_api: AsyncMock, -) -> None: - """Test that get_rooms is not re-called for an already-seen unknown room.""" - room_mapping_data = [[16, "9999701"]] - mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data] - - map_info = MultiMapsListMapInfo( - map_flag=42, - name="Test Map", - rooms=[ - MultiMapsListRoom(id=16, iot_name_id="9999701", iot_name=None), - ], - ) - - # Web API returns empty (no resolution) - mock_web_api.get_rooms.return_value = [] - - result1 = await home_trait._refresh_map_info(map_info) - result2 = await home_trait._refresh_map_info(map_info) - - # Both calls should fall back to "Room 16" - assert result1.rooms[0].name == "Room 16" - assert result2.rooms[0].name == "Room 16" - # get_rooms should only be called once for the same unknown room - mock_web_api.get_rooms.assert_called_once() - - -async def test_get_rooms_called_again_for_new_unknown_room( - home_trait: HomeTrait, - mock_rpc_channel: AsyncMock, - mock_web_api: AsyncMock, -) -> None: - """Test that get_rooms is called again when a new unknown room appears.""" - room_mapping_data_1 = [[16, "9999601"]] - room_mapping_data_2 = [[16, "9999601"], [17, "9999602"]] - mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2] - - map_info = MultiMapsListMapInfo( - map_flag=42, - name="Test Map", - rooms=[ - MultiMapsListRoom(id=16, iot_name_id="9999601", iot_name=None), - ], - ) - - # Web API returns empty (no resolution) - mock_web_api.get_rooms.return_value = [] - - result1 = await home_trait._refresh_map_info(map_info) - result2 = await home_trait._refresh_map_info(map_info) - - assert sorted(room.name for room in result1.rooms) == ["Room 16"] - assert sorted(room.name for room in result2.rooms) == ["Room 16", "Room 17"] - assert mock_web_api.get_rooms.call_count == 2 - - -async def test_get_rooms_called_again_for_new_unknown_iot_id_same_segment( - home_trait: HomeTrait, - mock_rpc_channel: AsyncMock, - mock_web_api: AsyncMock, -) -> None: - """Test that get_rooms is called again for a new unknown iot_id on the same segment.""" - room_mapping_data_1 = [[16, "9999501"]] - room_mapping_data_2 = [[16, "9999502"]] - mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2] - - map_info = MultiMapsListMapInfo( - map_flag=42, - name="Test Map", - ) - - mock_web_api.get_rooms.return_value = [] - - await home_trait._refresh_map_info(map_info) - await home_trait._refresh_map_info(map_info) - - assert mock_web_api.get_rooms.call_count == 2 - - -async def test_get_rooms_failure_falls_back_to_room_segment_id( - home_trait: HomeTrait, - mock_rpc_channel: AsyncMock, - mock_web_api: AsyncMock, -) -> None: - """Test that get_rooms failure gracefully falls back to 'Room {segment_id}'.""" - room_mapping_data = [[16, "9999401"]] - mock_rpc_channel.send_command.side_effect = [room_mapping_data] - - map_info = MultiMapsListMapInfo( - map_flag=7, - name="Test Map", - rooms=[ - MultiMapsListRoom(id=16, iot_name_id="9999401", iot_name=None), - ], - ) - - # Web API raises an exception - mock_web_api.get_rooms.side_effect = Exception("API error") - - result = await home_trait._refresh_map_info(map_info) - - assert result.rooms[0].name == "Room 16" - mock_web_api.get_rooms.assert_called_once() diff --git a/tests/devices/traits/v1/test_rooms.py b/tests/devices/traits/v1/test_rooms.py index b64e2c2d..ec930b6a 100644 --- a/tests/devices/traits/v1/test_rooms.py +++ b/tests/devices/traits/v1/test_rooms.py @@ -149,3 +149,58 @@ async def test_refresh_unknown_room_names_unresolved_uses_room_fallback( assert rooms_trait.rooms assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Room 33") web_api_client.get_rooms.assert_called_once() + + +async def test_refresh_unknown_room_names_called_again_for_new_unknown_room( + rooms_trait: RoomsTrait, + web_api_client: AsyncMock, + mock_rpc_channel: AsyncMock, +) -> None: + """Test get_rooms is called again when a new unknown room appears.""" + room_mapping_data_1 = [[16, "9999601"]] + room_mapping_data_2 = [[16, "9999601"], [17, "9999602"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2] + web_api_client.get_rooms.return_value = [] + + await rooms_trait.refresh() + assert rooms_trait.rooms + assert sorted(room.name for room in rooms_trait.rooms) == ["Room 16"] + + await rooms_trait.refresh() + assert rooms_trait.rooms + assert sorted(room.name for room in rooms_trait.rooms) == ["Room 16", "Room 17"] + assert web_api_client.get_rooms.call_count == 2 + + +async def test_refresh_unknown_room_names_called_again_for_new_unknown_iot_id_same_segment( + rooms_trait: RoomsTrait, + web_api_client: AsyncMock, + mock_rpc_channel: AsyncMock, +) -> None: + """Test get_rooms is called again for a new unknown iot_id on the same segment.""" + room_mapping_data_1 = [[16, "9999501"]] + room_mapping_data_2 = [[16, "9999502"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2] + web_api_client.get_rooms.return_value = [] + + await rooms_trait.refresh() + await rooms_trait.refresh() + + assert web_api_client.get_rooms.call_count == 2 + + +async def test_refresh_unknown_room_names_failure_falls_back_to_room_segment_id( + rooms_trait: RoomsTrait, + web_api_client: AsyncMock, + mock_rpc_channel: AsyncMock, +) -> None: + """Test get_rooms failure gracefully falls back to Room {segment_id}.""" + room_mapping_data = [[16, "9999401"]] + mock_rpc_channel.send_command.side_effect = [room_mapping_data] + web_api_client.get_rooms.side_effect = Exception("API error") + + await rooms_trait.refresh() + + assert rooms_trait.rooms + assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=16, iot_id="9999401", name="Room 16") + web_api_client.get_rooms.assert_called_once() From 84f85e404b008c0f6c46186dbfad002929621665 Mon Sep 17 00:00:00 2001 From: Luke Date: Sun, 8 Mar 2026 15:25:01 -0400 Subject: [PATCH 8/8] fix: make room name always room num and not unknown --- roborock/devices/traits/v1/home.py | 9 +++++---- tests/devices/traits/v1/test_home.py | 19 ++++++++++++++++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/roborock/devices/traits/v1/home.py b/roborock/devices/traits/v1/home.py index b1ee666d..0352b18d 100644 --- a/roborock/devices/traits/v1/home.py +++ b/roborock/devices/traits/v1/home.py @@ -120,22 +120,23 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo: rooms: dict[int, NamedRoomMapping] = {} if map_info.rooms: - # Not all vacuums resopnd with rooms inside map_info. + # Not all vacuums respond with rooms inside map_info. + # If we can determine if all vacuums will return everything with get_rooms, we could remove this step. for room in map_info.rooms: if room.id is not None and room.iot_name_id is not None: rooms[room.id] = NamedRoomMapping( segment_id=room.id, iot_id=room.iot_name_id, - name=room.iot_name or "Unknown", + name=room.iot_name or f"Room {room.id}", ) # Add rooms from rooms_trait. - # Keep existing names from map_info unless they are unknown. + # Keep existing names from map_info unless they are fallback names. if self._rooms_trait.rooms: for room in self._rooms_trait.rooms: if room.segment_id is not None and room.name: existing_room = rooms.get(room.segment_id) - if existing_room is None or existing_room.name == "Unknown": + if existing_room is None or existing_room.name == f"Room {room.segment_id}": rooms[room.segment_id] = room return CombinedMapInfo( diff --git a/tests/devices/traits/v1/test_home.py b/tests/devices/traits/v1/test_home.py index 7904dbcf..a6097561 100644 --- a/tests/devices/traits/v1/test_home.py +++ b/tests/devices/traits/v1/test_home.py @@ -633,6 +633,7 @@ async def test_refresh_map_info_prefers_map_info_names_and_adds_missing_rooms( This test verifies: 1. Existing map_info room names are preserved. 2. Missing rooms from RoomsTrait are added. + 3. map_info fallback names are replaced by RoomsTrait names. """ map_info = MultiMapsListMapInfo( map_flag=0, @@ -648,6 +649,11 @@ async def test_refresh_map_info_prefers_map_info_names_and_adds_missing_rooms( iot_name_id="2362042", iot_name="Bedroom from map_info", ), + MultiMapsListRoom( + id=20, + iot_name_id="9999001", + iot_name=None, + ), ], ) @@ -656,6 +662,7 @@ async def test_refresh_map_info_prefers_map_info_names_and_adds_missing_rooms( # - segment_id 19 with valid name: exists in map_info, should NOT override # - segment_id 17 with fallback name: not in map_info, should be added # - segment_id 18 with valid name: not in map_info, should be added + # - segment_id 20 with valid name: overrides map_info fallback "Room 20" rooms_trait.rooms = [ NamedRoomMapping(segment_id=16, iot_id="2362048", name="Room 16"), # Exists in map_info, should not override NamedRoomMapping( @@ -663,6 +670,7 @@ async def test_refresh_map_info_prefers_map_info_names_and_adds_missing_rooms( ), # Exists in map_info, should not override NamedRoomMapping(segment_id=17, iot_id="2362044", name="Room 17"), # Not in map_info, should be added NamedRoomMapping(segment_id=18, iot_id="2362041", name="Example room 3"), # Not in map_info, should be added + NamedRoomMapping(segment_id=20, iot_id="9999001", name="Office from rooms_trait"), ] # Mock rooms_trait.refresh to prevent actual device calls @@ -671,17 +679,17 @@ async def test_refresh_map_info_prefers_map_info_names_and_adds_missing_rooms( assert result.map_flag == 0 assert result.name == "Test Map" - assert len(result.rooms) == 4 + assert len(result.rooms) == 5 # Sort rooms by segment_id for consistent assertions sorted_rooms = sorted(result.rooms, key=lambda r: r.segment_id) - # Room 16: from map_info, kept (not overridden by Unknown) + # Room 16: from map_info, kept (not overridden by rooms_trait fallback) assert sorted_rooms[0].segment_id == 16 assert sorted_rooms[0].name == "Kitchen from map_info" assert sorted_rooms[0].iot_id == "2362048" - # Room 17: from rooms_trait with "Unknown", falls back to "Room 17" + # Room 17: from rooms_trait with fallback name, added because not in map_info assert sorted_rooms[1].segment_id == 17 assert sorted_rooms[1].name == "Room 17" assert sorted_rooms[1].iot_id == "2362044" @@ -695,3 +703,8 @@ async def test_refresh_map_info_prefers_map_info_names_and_adds_missing_rooms( assert sorted_rooms[3].segment_id == 19 assert sorted_rooms[3].name == "Bedroom from map_info" assert sorted_rooms[3].iot_id == "2362042" + + # Room 20: map_info fallback name replaced by rooms_trait name + assert sorted_rooms[4].segment_id == 20 + assert sorted_rooms[4].name == "Office from rooms_trait" + assert sorted_rooms[4].iot_id == "9999001"