q7/b01: command-layer segment clean + map payload retrieval helpers (split 1/3)#778
q7/b01: command-layer segment clean + map payload retrieval helpers (split 1/3)#778arduano wants to merge 6 commits intoPython-roborock:mainfrom
Conversation
|
Openclaw: Update / correction This earlier note is now superseded. After real-device sniffing on Q7T+ (B01), I did not observe RPC-wrapped The PR has been updated accordingly:
|
1 similar comment
|
Openclaw: Update / correction This earlier note is now superseded. After real-device sniffing on Q7T+ (B01), I did not observe RPC-wrapped The PR has been updated accordingly:
|
There was a problem hiding this comment.
Pull request overview
This PR adds the initial command-layer support for Q7-class B01 devices to trigger room/segment cleaning and to retrieve raw map payload bytes for later decoding/parsing work.
Changes:
- Added a
send_map_command()helper that publishes a Q7/B01 request and waits for a raw map payload response. - Extended
Q7PropertiesApiwith segment (room-id) cleaning, map list retrieval, current map-id resolution, and raw map payload retrieval (with per-device serialization via anasyncio.Lock). - Added tests covering segment cleaning and current-map payload retrieval via map-list + upload-by-map-id.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
roborock/devices/rpc/b01_q7_channel.py |
Adds send_map_command() to retrieve raw map bytes from either MAP_RESPONSE or RPC-wrapped hex payload. |
roborock/devices/traits/b01/q7/__init__.py |
Adds Q7 map/segment-clean helpers and serializes map upload requests via an instance lock. |
tests/devices/traits/b01/q7/test_init.py |
Adds tests for Q7 segment cleaning and current-map payload retrieval + fallback behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def test_q7_api_get_current_map_payload( | ||
| q7_api: Q7PropertiesApi, | ||
| fake_channel: FakeChannel, | ||
| message_builder: B01MessageBuilder, | ||
| ): | ||
| """Fetch current map by map-list lookup, then upload_by_mapid.""" | ||
| fake_channel.response_queue.append(message_builder.build({"map_list": [{"id": 1772093512, "cur": True}]})) | ||
| fake_channel.response_queue.append( | ||
| RoborockMessage( | ||
| protocol=RoborockMessageProtocol.MAP_RESPONSE, | ||
| payload=b"raw-map-payload", | ||
| version=b"B01", | ||
| seq=message_builder.seq + 1, | ||
| ) | ||
| ) | ||
|
|
||
| raw_payload = await q7_api.get_current_map_payload() | ||
| assert raw_payload == b"raw-map-payload" | ||
|
|
||
| assert len(fake_channel.published_messages) == 2 | ||
|
|
||
| first = fake_channel.published_messages[0] | ||
| first_payload = json.loads(unpad(first.payload, AES.block_size)) | ||
| assert first_payload["dps"]["10000"]["method"] == "service.get_map_list" | ||
| assert first_payload["dps"]["10000"]["params"] == {} | ||
|
|
||
| second = fake_channel.published_messages[1] | ||
| second_payload = json.loads(unpad(second.payload, AES.block_size)) | ||
| assert second_payload["dps"]["10000"]["method"] == "service.upload_by_mapid" | ||
| assert second_payload["dps"]["10000"]["params"] == {"map_id": 1772093512} | ||
|
|
There was a problem hiding this comment.
The PR description says map uploads may return either a MAP_RESPONSE or a decoded RPC response with a hex string under data.payload, but the added tests only cover the MAP_RESPONSE path. Add a test case that queues an RPC_RESPONSE containing data: {payload: "...hex..."} and asserts get_current_map_payload() returns the decoded bytes (and errors clearly on non-hex payloads).
| if future.done(): | ||
| return | ||
|
|
||
| if response_message.protocol == RoborockMessageProtocol.MAP_RESPONSE and response_message.payload: |
There was a problem hiding this comment.
send_map_command resolves the future on the first MAP_RESPONSE with a non-empty payload, without any correlation to request_message (e.g., msgId/seq). Since MqttChannel.subscribe() delivers all incoming device messages, an unrelated MAP_RESPONSE could satisfy this await and return the wrong map bytes. Consider correlating MAP_RESPONSEs to the initiating request (for example by matching on seq if the device echoes it, or by only accepting MAP_RESPONSE after a matching RPC response for this msgId indicates the upload is for this request).
| if response_message.protocol == RoborockMessageProtocol.MAP_RESPONSE and response_message.payload: | |
| if response_message.protocol == RoborockMessageProtocol.MAP_RESPONSE and response_message.payload: | |
| # Correlate MAP_RESPONSE to this request when a sequence identifier is available | |
| resp_seq = getattr(response_message, "seq", None) | |
| req_seq = getattr(request_message, "seq", None) | |
| if resp_seq is not None and req_seq is not None and resp_seq != req_seq: | |
| # MAP_RESPONSE belongs to a different request; ignore it | |
| return |
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
If data.payload is present but not valid hex, the ValueError is swallowed and the call will likely time out, obscuring the real parsing failure. It would be better to fail fast by setting an exception on the future (including the bad payload) so callers get an actionable error.
| except ValueError: | |
| pass | |
| except ValueError as ex: | |
| _LOGGER.debug( | |
| "Invalid hex payload in B01 map response (msgId=%s): %s (%s)", | |
| inner.get("msgId"), | |
| data.get("payload"), | |
| request_message, | |
| ) | |
| if not future.done(): | |
| future.set_exception( | |
| RoborockException( | |
| f"Invalid hex payload in B01 map response: {data.get('payload')} ({request_message})" | |
| ) | |
| ) | |
| return |
|
Openclaw: Update This comment is superseded by later commits. Current state on this PR:
|
allenporter
left a comment
There was a problem hiding this comment.
Thank you for the contribution, and splitting into smaller PR.
|
|
||
| In practice, B01 map responses may arrive either as: | ||
| - a dedicated ``MAP_RESPONSE`` message with raw payload bytes, or | ||
| - a regular decoded RPC response that wraps a hex payload in ``data.payload``. |
There was a problem hiding this comment.
How positive are you this this the case? Are both raw payloads the same in those cases? or is this halucinated?
Is there a specific dps int value this is sent with? or it can be any? (i notice the dps key is ignored entirely)
There was a problem hiding this comment.
AI did calls to the real API just then to sanity check and decided that yeah it's hallucinated. MAP_RESPONSE is the only case
| if future.done(): | ||
| return | ||
|
|
||
| # Avoid accepting queued/stale MAP_RESPONSE messages before we actually |
There was a problem hiding this comment.
It this actually needed? I can't tell if this is needed because the code below doesn't work at determining which method was sent to it, or because its over optimized for performance. (if the latter, i don't think its worth it)
There was a problem hiding this comment.
This was openclaw picking up on a copilot review comment, should I remove it? I can't really tell either
There was a problem hiding this comment.
openclaw, please remove it :)
| params={}, | ||
| ) | ||
|
|
||
| async def get_map_list(self) -> dict[str, Any] | None: |
There was a problem hiding this comment.
Can we please use a dataclass / roborock base dataclass to hold this response?
| params={}, | ||
| ) | ||
|
|
||
| async def get_map_list(self) -> dict[str, Any] | None: |
There was a problem hiding this comment.
Map related commands seem like they should be on a separate trait, similar to how the v1 API looks. You can see the clean summary as a nexample.
That will also let us keep state on the trait about the latest map list, so you don't need to ref-etch the map list to get the current map id. it can be cached on the trait.
There was a problem hiding this comment.
Yeah sorry I'm not familiar enough with python to have caught this one, I come from typescript
Will adjust
There was a problem hiding this comment.
AI updated it for me now but again I'm unsure about best practices with traits
There was a problem hiding this comment.
yeah we kind of just made up a pattern.
OpenClaw, think about it form the callers perspective:
(1) You want an easy way to refresh all the content
https://github.com/home-assistant/core/blob/a3d8d766782e075ee8d23e4328666ca1e15807bd/homeassistant/components/roborock/coordinator.py#L338
(2) Anytime you want to get the current value you can just get it directly from the trait without any other i/o e.g. https://github.com/home-assistant/core/blob/a3d8d766782e075ee8d23e4328666ca1e15807bd/homeassistant/components/roborock/sensor.py#L96
You can see these for some examples of traits in the code base:
https://github.com/Python-roborock/python-roborock/blob/main/roborock/devices/traits/v1/home.py
https://github.com/Python-roborock/python-roborock/blob/main/roborock/devices/traits/v1/map_content.py
|
I think most of this is now addressed, minus best practices because I'm not 100% confident. Let me know if there's anything else |
| On a real Q7 B01 device, map uploads arrive as a dedicated | ||
| ``MAP_RESPONSE`` message with raw payload bytes. | ||
|
|
||
| We still decode RPC responses so we can fail fast on non-zero ``code`` |
There was a problem hiding this comment.
What causes this to happen? It seems speculative, and is a lot of code.
| self._map_command_lock = asyncio.Lock() | ||
| self._map_list_cache: Q7MapList | None = None | ||
|
|
||
| async def refresh_map_list(self) -> Q7MapList: |
There was a problem hiding this comment.
to elaborate from my examples: The idea for these traits is they have a single refresh method that refreshes the content. You can see the v1 device has separate traits for getting the map list vs caching the map content.
| if future.done(): | ||
| return | ||
|
|
||
| # Avoid accepting queued/stale MAP_RESPONSE messages before we actually |
There was a problem hiding this comment.
openclaw, please remove it :)
| params={}, | ||
| ) | ||
|
|
||
| async def get_map_list(self) -> dict[str, Any] | None: |
There was a problem hiding this comment.
yeah we kind of just made up a pattern.
OpenClaw, think about it form the callers perspective:
(1) You want an easy way to refresh all the content
https://github.com/home-assistant/core/blob/a3d8d766782e075ee8d23e4328666ca1e15807bd/homeassistant/components/roborock/coordinator.py#L338
(2) Anytime you want to get the current value you can just get it directly from the trait without any other i/o e.g. https://github.com/home-assistant/core/blob/a3d8d766782e075ee8d23e4328666ca1e15807bd/homeassistant/components/roborock/sensor.py#L96
You can see these for some examples of traits in the code base:
https://github.com/Python-roborock/python-roborock/blob/main/roborock/devices/traits/v1/home.py
https://github.com/Python-roborock/python-roborock/blob/main/roborock/devices/traits/v1/map_content.py
| self._map_command_lock = asyncio.Lock() | ||
| self._map_list_cache: Q7MapList | None = None | ||
|
|
||
| async def refresh_map_list(self) -> Q7MapList: |
| raise RoborockException(f"Unable to determine map_id from map list response: {map_list!r}") | ||
| return map_id | ||
|
|
||
| async def get_map_payload(self, *, map_id: int) -> bytes: |
There was a problem hiding this comment.
I don't think this needs to be exposed publicly so prefix with a _
|
Openclaw: Pushed another follow-up to address the latest review feedback. What changed in this update:
Commit:
|
|
I'm getting a bit lost on what's changing because I don't have the energy to absorb a python codebase today :( But yeah let me (aka me forwarding to openclaw) know if there's anything else, happy to continue iterating this |
|
If you're curious, this is running gpt-3.5-codex directly through openclaw, so if there's egregious slop lmn and I'll see what I can do about it |
|
Yes I get to review a number of PRa lately that are cleary ai generated and it usually takes a little more back and forth to explain the code base norms. Maybe not a problem though because it's getting new features in I don't have to do myself or couldn't so myself so I try to be patient. |
Openclaw: This PR is 1/3 of a planned split of the original Q7/B01 map+segment work, to keep review size and scope tight.
What this PR does (high level)
This first slice adds the command-layer plumbing needed to:
No SCMap parsing or map rendering is included here — those come in later splits.
Changes in detail
1) Add
send_map_command()helper for B01/Q7 map uploadsFile:
roborock/devices/rpc/b01_q7_channel.pyOpenclaw: Introduces
send_map_command(mqtt_channel, request_message) -> byteswhich publishes a B01/Q7 request and waits for the corresponding map payload.Current behavior in this PR:
MAP_RESPONSEmessages with rawpayloadbytes.coderesponses correlated to the initiatingmsgIdand requesteddps.Note on evidence:
roborock.vacuum.sc05, B01) observed map payload delivery viaMAP_RESPONSEraw bytes.data.payloadmap bytes.2) Extend Q7 traits with segment cleaning + map retrieval
Files:
roborock/devices/traits/b01/q7/__init__.pyroborock/devices/traits/b01/q7/map.pyOpenclaw: Adds Q7 segment-clean helper and introduces a dedicated map trait:
clean_segments(segment_ids: list[int])sendsservice.set_room_cleanwithROOMclean type (Q7 uses room IDs).MapTraitholds map-related commands/state.Q7MapList/Q7MapListEntrydataclasses (RoborockBase) model map-list response.get_map_list()cache +refresh_map_list()refresh).get_current_map_id(),get_map_payload(map_id=...),get_current_map_payload()live on the map trait.Concurrency/safety:
asyncio.Lockto avoid response cross-wiring when multiple map requests are in-flight.3) Tests
File:
tests/devices/traits/b01/q7/test_init.pyOpenclaw: Adds coverage for:
clean_segments()request shapeGET_MAP_LIST+UPLOAD_BY_MAPIDFollow-ups (separate PRs)
Openclaw: Subsequent split PRs will build on this by:
map_contenttrait that exposesroomsand a PNG render.