Add Q7 map_content support and room segment cleaning helper#774
Add Q7 map_content support and room segment cleaning helper#774arduano wants to merge 6 commits intoPython-roborock:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a typed helper on the Q7 (B01) trait API to start room/segment cleaning by routing through the existing service.set_room_clean command, while leaving existing start/pause/stop helpers unchanged.
Changes:
- Introduce
Q7PropertiesApi.clean_segments(room_ids)to start ROOM-type cleaning viaSET_ROOM_CLEAN. - Use existing
CleanTaskTypeMapping/SCDeviceCleanParammappings to build the command payload.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def clean_segments(self, room_ids: list[int]) -> None: | ||
| """Start segment/room cleaning for the given room ids.""" | ||
| await self.send( | ||
| command=RoborockB01Q7Methods.SET_ROOM_CLEAN, | ||
| params={ | ||
| "clean_type": CleanTaskTypeMapping.ROOM.code, | ||
| "ctrl_value": SCDeviceCleanParam.START.code, | ||
| "room_ids": room_ids, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This new public helper isn’t covered by the existing Q7PropertiesApi unit tests (there are tests for start/pause/stop). Please add a test that calls clean_segments([...]) and asserts the published RPC method/params, similar to test_q7_api_start_clean, to prevent regressions in the command payload.
There was a problem hiding this comment.
Openclaw: Added coverage in tests/devices/traits/b01/q7/test_init.py (test_q7_api_clean_segments) asserting RPC method and payload for clean_segments([...]).
Greptile SummaryThis PR adds comprehensive Q7 map support with B01 protocol decoding and room-based segment cleaning:
The implementation is well-structured with proper error handling, though there's a minor style inconsistency in Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Q7PropertiesApi
participant Q7MapContentTrait
participant send_map_command
participant MqttChannel
participant Device
Client->>Q7PropertiesApi: map_content.refresh()
Q7PropertiesApi->>Q7MapContentTrait: refresh()
Note over Q7MapContentTrait: Step 1: Get map list
Q7MapContentTrait->>send_map_command: send_decoded_command(GET_MAP_LIST)
send_map_command->>MqttChannel: publish(request)
MqttChannel->>Device: MQTT publish
Device-->>MqttChannel: JSON response
MqttChannel-->>send_map_command: decoded response
send_map_command-->>Q7MapContentTrait: {map_list: [{id, cur}]}
Note over Q7MapContentTrait: Step 2: Extract current map_id
Q7MapContentTrait->>Q7MapContentTrait: _extract_current_map_id()
Note over Q7MapContentTrait: Step 3: Fetch map payload
Q7MapContentTrait->>send_map_command: send_map_command(UPLOAD_BY_MAPID)
Note over send_map_command: Lock acquired (serialized)
send_map_command->>MqttChannel: publish(request)
MqttChannel->>Device: MQTT publish
Device-->>MqttChannel: MAP_RESPONSE payload
MqttChannel-->>send_map_command: raw encrypted bytes
Note over send_map_command: Lock released
send_map_command-->>Q7MapContentTrait: raw_payload
Note over Q7MapContentTrait: Step 4: Decode pipeline
Q7MapContentTrait->>b01_map_parser: decode_b01_map_payload()
Note over b01_map_parser: - Base64 decode<br/>- AES decrypt (local_key + map_key)<br/>- Decompress (zlib)
b01_map_parser-->>Q7MapContentTrait: inflated SCMap protobuf
Note over Q7MapContentTrait: Step 5: Parse and render
Q7MapContentTrait->>b01_map_parser: parse_scmap_payload()
b01_map_parser-->>Q7MapContentTrait: B01MapData (dimensions, grid, rooms)
Q7MapContentTrait->>b01_map_parser: render_map_png()
b01_map_parser-->>Q7MapContentTrait: PNG bytes
Q7MapContentTrait-->>Client: B01MapContent (image_content, rooms)
Last reviewed commit: aea98ed |
|
Hey, just a draft PR I'm scrapping together with my openclaw instance This is for a Q7T+ that a family member recently purchased, and I was sad to find out that their purchase isn't compatible with my HASS so I decided to go fix it myself |
|
You'll need something similar for getting the room list as well. Haven't dove into it myself yet. Point your openclaw instance to look at honey Roborock code and it should be able to find the logic there. I think he has it implemented there |
arduano
changed the title
|
@Lash-L what do you mean by "honey Roborock code", do you mean |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| from Crypto.Cipher import AES | ||
| from Crypto.Util.Padding import pad, unpad | ||
| from PIL import Image |
There was a problem hiding this comment.
This module imports PIL.Image, but Pillow is not a direct dependency of this project (it’s currently only present via transitive dependencies in uv.lock). Relying on a transitive dependency can break installs if upstream dependency graphs change.
Add Pillow as an explicit runtime dependency (or avoid using Pillow here) so the import is guaranteed to be available for end users.
There was a problem hiding this comment.
Openclaw: Addressed. Pillow is now an explicit runtime dependency in pyproject.toml and uv.lock.
| def __init__(self, channel: MqttChannel, *, local_key: str, serial: str, model: str) -> None: | ||
| """Initialize the B01Props API.""" | ||
| self._channel = channel | ||
| self.clean_summary = CleanSummaryTrait(channel) | ||
| self.map_content = Q7MapContentTrait(channel, local_key=local_key, serial=serial, model=model) |
There was a problem hiding this comment.
Q7PropertiesApi.__init__ (and create) now require local_key, serial, and model, which is a breaking change for any callers instantiating Q7PropertiesApi(channel) directly (the class is exported in __all__).
If backwards compatibility matters, consider keeping the old signature by making these parameters optional and either (a) only enabling map_content when they’re provided, or (b) accepting a single device-info object and/or providing an alternate constructor for map support.
There was a problem hiding this comment.
Openclaw: Addressed for backward compatibility. Q7PropertiesApi.init/create now accept optional local_key/serial/model; map support is enabled only when provided. Added a constructor-compatibility test for direct callers.
roborock/map/__init__.py
Outdated
| @@ -1,7 +1,13 @@ | |||
| """Module for Roborock map related data classes.""" | |||
There was a problem hiding this comment.
The module docstring says this is a module for map-related data classes, but this file now also exports parsing/rendering functions (decode_b01_map_payload, parse_scmap_payload, render_map_png). Consider updating the docstring to reflect the broader surface area.
| """Module for Roborock map related data classes.""" | |
| """Utilities and data classes for working with Roborock maps, including parsing and rendering functions.""" |
There was a problem hiding this comment.
Openclaw: Updated the module docstring to reflect broader map utilities (data classes plus parsing/rendering helpers).
| if response_message.protocol == RoborockMessageProtocol.MAP_RESPONSE and response_message.payload: | ||
| future.set_result(response_message.payload) | ||
| return |
There was a problem hiding this comment.
send_map_command resolves the waiting future on the first MAP_RESPONSE seen, without correlating it to request_message.msg_id (or otherwise confirming it’s the response for this request). If multiple map uploads overlap (or any other MAP_RESPONSE arrives on the same subscription), this can complete with the wrong payload.
Consider correlating by first waiting for the matching RPC ack (same msgId / code==0) and only then accepting the next MAP_RESPONSE, or serializing map requests with a per-channel lock so only one send_map_command is in-flight at a time.
There was a problem hiding this comment.
Openclaw: Addressed by serializing B01 map commands per channel (lock) and keeping msgId-aware response handling to prevent cross-wired payloads under overlap.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| future.set_exception(RoborockException(f"B01 command failed with code {code} ({request_message})")) | ||
| return | ||
| data = inner.get("data") | ||
| if isinstance(data, dict) and isinstance(data.get("payload"), str): | ||
| try: | ||
| future.set_result(bytes.fromhex(data["payload"])) |
There was a problem hiding this comment.
Missing if not future.done() checks before setting exception/result. While unlikely, if multiple dps values match the same msgId, this could raise InvalidStateError. Compare with send_decoded_command which checks before setting (lines 73-74, 79-80, 84-85).
| future.set_exception(RoborockException(f"B01 command failed with code {code} ({request_message})")) | |
| return | |
| data = inner.get("data") | |
| if isinstance(data, dict) and isinstance(data.get("payload"), str): | |
| try: | |
| future.set_result(bytes.fromhex(data["payload"])) | |
| code = inner.get("code", 0) | |
| if code != 0: | |
| if not future.done(): | |
| future.set_exception(RoborockException(f"B01 command failed with code {code} ({request_message})")) | |
| return | |
| data = inner.get("data") | |
| if isinstance(data, dict) and isinstance(data.get("payload"), str): | |
| try: | |
| if not future.done(): | |
| future.set_result(bytes.fromhex(data["payload"])) | |
| return | |
| except ValueError: | |
| pass |
|
Openclaw: Thanks for the review. I addressed the |
|
@Lash-L properly ready for review now I looked through the code, don't see any red flags personally. I verified that it does work on my vacuum, pulling the map, coords, rooms, etc, and being able to queue commands such as vacuuming a specific room |
|
I just connected it to my home assistant (manually build the hass container with this new PR as a dependency) and I can now view the map and send the vacuum to clean specific segments. The only thing I'm unsure about is map feature rendering. Am I supposed to make this PR also render things like the vacuum location into the map png, or is that handled in hass? Right now it just sends the plain map |
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Good progress here. Some early thoughts
| cross-wire responses between callers. | ||
| """ | ||
|
|
||
| async with _get_map_command_lock(mqtt_channel): |
There was a problem hiding this comment.
Please move the asyncio.Lock to the trait, which is one per device. We don't need to do this indirectly via a map to the channel as we can just do it there before calling into this map function.
| future.set_result(response_message.payload) | ||
| return | ||
|
|
||
| try: |
There was a problem hiding this comment.
Does this happen in practice that we get a normal protocol response back or is this speculative? I'm wondering if this could have a high level comment describing the scenarios in which we observe this happening. Second, i think this is complex enough that likely we'd want to reuse the common parts across both protocols, in a way that is easy to read.
| return None | ||
|
|
||
|
|
||
| def decode_b01_map_payload(raw_payload: bytes, *, local_key: str, serial: str, model: str) -> bytes: |
There was a problem hiding this comment.
Problem: This code is combining map decoding with protocol decoding and these responsibilities should be split. e.g. review how the v1 code separates the protocol encoding/decoding from map encoding. e.g.
or
create_v1_channel.
The idea is we shouldn't need to be passing around local keys everywhere. Make a separate rpc channel object responsible for decoding the bytes. Then the map trait can get decoded bytes and pass them to a map content parser.
You can also review how existing encryption code is structured in protocol.py and protocols/
There was a problem hiding this comment.
Lets put this in tests/map/testdata/
|
|
||
| from roborock.devices.rpc.b01_q7_channel import send_decoded_command, send_map_command | ||
| from roborock.devices.traits import Trait | ||
| from roborock.devices.traits.v1.map_content import MapContent |
There was a problem hiding this comment.
Let's not directly depend on the traits of another device type. For now, create a new object with the fields we need here rather than reusing this and we can decide to reuse in the future. The protocols are different enough that I don't think we'll have calling code or anything that will treat these map objects the same.
| async def refresh(self) -> B01MapContent: | ||
| map_list_response = await send_decoded_command( | ||
| self._channel, | ||
| Q7RequestMessage(dps=10000, command=RoborockB01Q7Methods.GET_MAP_LIST, params={}), |
There was a problem hiding this comment.
Can we extract a named constant for 10000?
| self._model = model | ||
|
|
||
| async def refresh(self) -> B01MapContent: | ||
| map_list_response = await send_decoded_command( |
There was a problem hiding this comment.
In v1 we have a separate trait for holding on to the map list and current map MapsTrait. I assume we'll want the other content here also?
We decided to separate them in v1 just because there was a lot of existing code to unwind and introduced the Home object to help with caching map content across different maps, but not sure we need that here yet.
Can you review the existing MapsTrait / HomeTrait / MapContentTrait for v1 and consider what would be a reasonable first step to take with that in mind? I do worry that getting the current_map logic wrong is harder to fix later, but we can also do something simple for now. Basically i think it'd be good to at least keep the map list somewhere.
Today this happens in roborock/map/map_parser.py via another 3rd party package. I wonder if the format is the same or different? |
Yes I did mean Homey. |
Pretty much completely different. We could theoretically just make a new instance of MapDataParser to make all of the objects use the same functions, objects, etc. as input, but i'm not sure if that makes sense or not. The biggest benefit of doing so is that it would work with https://github.com/PiotrMachowski/lovelace-xiaomi-vacuum-map-card which is always one of the biggest requests with vacuum maps. I'm also not sure if it should live here or in the 3rd party library. My gut is thinking it should live here so that we have more control over it. We could also probably transfer it into the python-roborock org to give us more control, but i'm not sure if the abstraction is worth it or not. We are also the only users of the 3rd party library (for roborock). As well Q10 will be completely different than this as well, so we have another obstacle there. |
|
Happy to mess around with the code further if needed |
|
What are your thoughts regarding the map parser? I'm kind of leaning towards it living in this library, but it does feel a little weird having one device type having a map in a different library than the rest. But I think it makes sense? I do think it makes sense to still utilize the same base object if possible |
Yeah, I agree with your proposals of just doing it here and/or copying the other library in here. My impression is you are the main person making changes to the library anyway. It's kind of weird how its split across two other packages also, and is a bit overly abstracted. |
Technically it's split over like 5 different libraries with a different one per each brand, but yes i agree. Okay @arduano I think action items from here:
Feel free to add any allen |
Which ones? Or do you mean the ones in the hass-core repo. Should I be migrating anything my AI did in that PR over to this PR? I'll address later today |
The ones he made here during this review. I haven't looked at the core PR, but the library should contain all complex logic. It should be very easy and clear to utilize from core if we design it well |
|
@Lash-L before I do a deeper review myself, is this the kind of PR splitting you'd like? |
Summary
Q7PropertiesApi.clean_segments(room_ids)helper for write-dependent room cleaningmap_contenttrait for map image bytesmap_idviaservice.get_map_listand fetch payload viaservice.upload_by_mapid(map_idparam)Why
This provides a clean, typed foundation for both:
without relying on HA-side decode logic.
Testing
Unit tests (repo-local
.venv, nix-shell Python 3.12)pytest -q tests/map/test_b01_map_parser.py tests/devices/traits/b01/q7/test_init.py tests/devices/traits/b01/q7/test_clean_summary.py27 passedRead-only live checks against Q7 (no cleaning/write actions)
status,wind,cleaning_time)map_content.refresh()\x89PNG...)Lint
ruff checkon modified parser/trait/tests passed