Repository navigation
Conversation
|
Tested against a real device: Roborock Q10 S5+ (ss07 firmware family), on commit Watched Observations:
This was a single-room spot clean, so I didn't get to observe a room-to-room transition in this pass. |
# Conflicts: # roborock/devices/traits/b01/q10/maps.py # tests/devices/traits/b01/q10/test_clean_history.py
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Note: Conducting this code review following our /review skill using Antigravity (AGY) and Gemini 4 Argon.
Preserving custom room labels verbatim in Q10Room.name, redacting rawName in diagnostics, and calibrating trace coordinates from the header metadata look good.
We can simplify the current_room trait state and tests considerably:
-
Remove
_CURRENT_ROOM_STATESandself._current_room_trace_freshstate tracking inMapContentTrait:- Right now
MapContentTraitgatescurrent_roomon_CURRENT_ROOM_STATESand tracksself._current_room_trace_freshacross status and trace updates. - This makes
current_roominconsistent withrobot_position: when the robot isIDLEorRETURNING_HOME(or when map+trace packets arrive before the first status DPS),trait.robot_positionreturns the live position fromself._trace_packetwhiletrait.current_roomis forced toNone. - In V1 (
RoborockMapDataParser._get_current_vacuum_room), the current room (map_data.vacuum_room) is simply the room at the robot's position whenever the map and robot position are present—without gating on cleaning status. - Since
MapContentTraitalready clearsself._trace_packet = Nonewhenself._map_dps.robot_at_dockis true,current_roomcan be a stateless 3-line property derived directly from(self._map_packet, self._trace_packet). That also removes the need for the 7 mock-heavy status/arrival-order permutation tests intests/devices/traits/b01/q10/test_map.py.
- Right now
-
Remove defensive
math.isfinitechecks onintcoordinates andreplace(room)cloning:- See inline comments in
roborock/map/b01_q10_render.py.
- See inline comments in
-
Test through public entrypoints rather than private helpers or patched functions:
- See inline comments in
tests/devices/traits/b01/q10/test_map.pyandtests/map/test_b01_q10_render.py.
- See inline comments in
| outside cleaning states or whenever the position cannot be mapped | ||
| unambiguously. | ||
| """ | ||
| if ( |
There was a problem hiding this comment.
Let's remove _CURRENT_ROOM_STATES and self._current_room_trace_fresh so current_room stays consistent with robot_position (and V1's map_data.vacuum_room):
if self._map_packet is None or self._trace_packet is None:
return None
return resolve_q10_current_room(self._map_packet, self._trace_packet)(self._trace_packet is already cleared when self._map_dps.robot_at_dock is true.)
| room. | ||
| """ | ||
| pixel_x, pixel_y = calibration.world_to_pixel(position.x, position.y) | ||
| if not math.isfinite(pixel_x) or not math.isfinite(pixel_y): |
There was a problem hiding this comment.
Q10Point.x and Q10Point.y are int values unpacked from int16 wire bytes, and calibration.resolution > 0 is already validated, so pixel_x and pixel_y are always finite. Let's remove the math.isfinite checks here and in _trace_projects_onto_floor (as well as test_room_at_position_rejects_nonfinite_coordinates which uses cast(int, float("nan"))).
| return None | ||
| return solve_calibration_with_origin(packet.layers, points, origin, resolutions=_Q10_RESOLUTIONS) | ||
| room = _room_at_position(packet, position, calibration) | ||
| return replace(room) if room is not None else None |
There was a problem hiding this comment.
No need to clone room via replace(room) on every property access (consistent with trait.rooms and trait.path). We can return _room_at_position(packet, position, calibration) directly and remove the mutation assertions in the tests.
| trait.update_from_map_packet(packet) | ||
| trait.update_from_trace_packet(trace) | ||
|
|
||
| with patch( |
There was a problem hiding this comment.
Once _CURRENT_ROOM_STATES and self._current_room_trace_fresh are removed, we can replace these 7 status/arrival-order tests that patch resolve_q10_current_room with a direct test using a map packet and a trace packet whose point resolves to one of the fixture rooms.
| q10_api._handle_message(parse_map_packet(b"\x04\x01" + payload[2:])) | ||
| assert q10_api.map.current_room == current.rooms[0] | ||
|
|
||
| assert not hasattr(q10_api.clean_history, "current_room") |
There was a problem hiding this comment.
We don't need to assert not hasattr(..., "current_room") on unrelated traits.
| packet = _room_packet([12, 12, 12, 12, 8, 12, 12, 12, 12], width=3) | ||
| calibration = GridCalibration(resolution=10, origin_x=0, origin_y=0, y_sign=-1) | ||
|
|
||
| assert _room_at_position(packet, Q10Point(10, 10), calibration) == packet.rooms[0] |
There was a problem hiding this comment.
Please test room resolution through the public resolve_q10_current_room(packet, trace) function rather than importing and calling the private _room_at_position helper directly.
This branch has not been deployed
3 participants
Q10 firmware does not provide a validated active-room field. This exposes
map.current_roomby conservatively matching the latest live robot position to the segmented map grid.Behavior
Q10Roommodel orNone, with a defensive copy of a resolved room.None.Session validity stays in the trait. Spatial resolution is a pure function of typed map/trace models.
Dependencies
Depends on #938 and #937. #936, #908, and #965 are merged. Upstream
mainthroughb3a98b9(7.12.0) is integrated.Allen’s follow-up feedback on merged #936 is addressed by separate clean-record and saved-map decoder tests, with no conditional assertions.
Validation and limits
Validation of
33b5af5:uv run pytest -qon Python 3.13: 1,497 passed, 53 xfailed, 92 snapshots passed.uv buildpassed for both wheel and sdist.No physical-device run was performed for this revision.
Existing tests cover room resolution, ambiguity, calibration, session freshness, custom labels, and diagnostic redaction. Earlier physical Q10 S5/S5+ testing observed a custom room label during a single-room clean and clearing during return/docking. A room-to-room transition was not covered by that reported run.
This is inferred room occupancy during cleaning, not an authoritative firmware report of the selected cleaning segment. Missing or unusable map/trace data returns
None. Archive requests inherit the bounded lifecycle from #937, including its clean-history ambiguity barrier and same-map freshness limitation.No private map captures, room labels, or account data are included. Related: #767.