Skip to content

feat: expose Q10 map archives - #937

Open
hCoureau wants to merge 18 commits into
Python-roborock:mainfrom
hCoureau:feat/q10-map-archives
Open

hCoureau wants to merge 18 commits into
Python-roborock:mainfrom
hCoureau:feat/q10-map-archives

Conversation

@hCoureau

@hCoureau hCoureau commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Q10 archives arrive on independent push streams. This adds read-only saved-map previews and clean-history map/path details while keeping archive data separate from the live map.

Public API and data flow

  • maps owns saved-map metadata and the latest received preview. detail_map_id comes from the pushed packet itself.
  • clean_history inherits its public data and derived accessors from the CleanHistory dataclass, a RoborockBase model. It exposes the latest received archive map, historical path and rendered bytes.
  • Both refresh_detail() methods return after publishing. Device pushes populate the read models and notify add_update_listener() subscribers. An asyncio.Lock serializes publication only; missing responses do not block later requests.
  • Clean-history selections use the complete raw firmware record identifier. Saved-map selections validate the ID against the received map list.
  • Q10PropertiesApi routes typed Q10CleanRecordDetail and saved-map packets to their archive owners. Archive updates cannot replace live-map state.
  • The factory supplies shared rendering configuration to all three map views. Archive constructors require concrete configuration; default configuration stays at the factory boundary.
  • Diagnostic serialization includes history records/path data and saved-map metadata while excluding binary map grids and PNG bytes. Images remain available through detail_image_content.

Consumer expectations and protocol limits

Subscribe before requesting an archive, then read the trait when its listener fires. roborock/devices/README.md documents persistent-view usage and a one-shot listener/event example with a consumer-owned deadline. Trait methods do not create response-waiting tasks or futures.

Clean-record detail packets carry no record identifier. Delayed pushes and selections from another client cannot be attributed reliably to a local selection, so this API exposes latest received detail without a detail_record association. Saved-map packets identify the map, but contain no request ID; consumers should inspect detail_map_id before displaying a preview. A later received preview replaces the previous preview regardless of arrival order.

Live-map updates use the normal trait listener API without revision counters. The CLI subscribes before requesting a push, waits for an update satisfying its state predicate, and unsubscribes on success, timeout or cancellation. Its existing 30-second deadline and optional cached-map fallback remain at the CLI boundary.

Dependencies

Archive parser support from #936 is merged, alongside #933, #908 and #965. This branch includes upstream main through b3a98b9 (7.12.0).

Validation

Validation of f1f5e24:

  • Full suite on Python 3.11 and 3.14: 1,440 passed, 53 xfailed, 92 snapshots passed on each version.
  • All pre-commit hooks passed, including Ruff, mypy, codespell and structured-file checks.
  • Wheel and source distribution builds passed.
  • Fake-channel tests cover selection commands, publication serialization, cancellation/send-failure recovery, missing responses, unsolicited/repeated pushes, listener notifications, live-map isolation and diagnostic serialization.
  • Render configuration tests check actual PNG dimensions at scales 1, 2 and 4.

No physical-device run was performed for this revision. No private map captures or account data are included. Related: #767.

Comment thread roborock/devices/traits/b01/q10/clean_history.py Outdated
Comment thread roborock/devices/traits/b01/q10/clean_history.py Outdated
Comment thread roborock/devices/traits/b01/q10/map.py Outdated
Comment thread tests/devices/traits/b01/q10/test_map.py Outdated
Comment thread tests/devices/traits/b01/q10/test_map.py Outdated
@hCoureau

hCoureau commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed all five review comments in f1f5e24 and pushed the changes to this branch.

The archive traits now follow the existing push/listener contract: selections return after publication, asyncio.Lock serializes sends, and received archives update the read models and notify subscribers. Clean-history data is in a RoborockBase dataclass and included in diagnostic serialization. Removed the response-waiting machinery, revision counters and unsupported clean-record request attribution. Added consumer documentation, fake-channel command/lifecycle tests and PNG-dimension tests for shared render configuration.

Validation: 1,440 passed, 53 expected failures and 92 snapshots passed on both Python 3.11 and 3.14; all pre-commit hooks and both package builds passed. The PR description now reflects the final API and protocol limitations.

@hCoureau
hCoureau requested a review from allenporter October 1, 2026 21:10
Comment thread roborock/devices/README.md Outdated
Comment thread tests/devices/traits/b01/q10/test_clean_history.py Outdated
Comment thread tests/devices/traits/b01/q10/test_clean_history.py Outdated
@hCoureau
hCoureau requested a review from allenporter October 7, 2026 22:12

@allenporter allenporter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Conducting this code review following our /review skill using Antigravity (AGY) and Gemini 4 Argon.

Thanks for updating the traits to use the push/listener pattern and extracting the RoborockBase read models!

Stepping back, I want to make sure I understand the intended consumer use case here—especially around rendering standalone PNG images (detail_image_content) on CleanHistoryTrait and MapsTrait:

  1. Clean record & saved map rendering vs. live map isolation:

    • Protecting properties.map so that 03 01 (clean record archive) and 04 01 (saved map preview) packets on the push stream do not overwrite the active live map (01 01) makes complete sense.
    • However, it's surprising to see CleanHistoryTrait and MapsTrait eagerly rendering and storing their own PNG images (detail_image_content). Neither V1 (v1/clean_summary.py, v1/maps.py) nor Q7 (q7/clean_summary.py) renders map images on clean_summary or maps.
    • Because 03 01 packets carry no record ID on the wire, clean_history.detail / detail_image_content cannot even be reliably attributed to a specific Q10CleanRecord (e.g., if another client/app triggers a history push, we also synchronously run PIL rendering on the event loop for an unattributable packet).
    • How do you envision a downstream consumer (like Home Assistant) using clean_history.detail_image_content and maps.detail_image_content? If there isn't a concrete consumer need for rendering archive images right now, should we simplify this PR to just isolating properties.map (and/or exposing the parsed packet data without eager image rendering)?
  2. Removing _detail_lock and lock-serialization tests:

    • In my earlier comment, asyncio.Lock was only mentioned as an alternative to the hand-rolled locking when refresh_detail() was blocking on a response future. Now that refresh_detail() is a fire-and-forget self._command.send(...) just like refresh(), we don't need _detail_lock at all—nor the send_gate / lock-serialization / cancellation tests.

)

async def refresh_detail(self, record: Q10CleanRecord) -> None:
"""Publish a selection using the complete firmware record identifier.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically saying: Please refresh the latest data for this clean record? but also, it displays it on the map?

The comment here is discussing fairly low level implementation detail, but not really the higher level thing we're trying to accomplish here by using this API.

This speeds up startup time and reduces load on the Roborock cloud APIs.


## Q10 archive updates

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roborock/devices/README.md is a brief high-level overview of the device architecture (DeviceManager, RoborockDevice, Traits, Channels, Cache). Since Q10 archive traits now work the same way as all other Q10 push-driven traits, let's remove this Q10-specific section from the top-level README and keep any protocol-specific notes (like 03 01 lacking a record correlation ID) concise in the CleanHistoryTrait / MapsTrait docstrings.

"""Historical path points from the latest received archive."""
return list(self.detail_trace.points) if self.detail_trace else []

def as_dict(self, exclude: set[str] | None = None) -> dict[str, Any]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than hand-rolling custom as_dict() methods that manually construct dicts with hardcoded camelCase keys ("multiMap", "detailMapId", "detailPath"), can we rely on RoborockBase.as_dict() (or super().as_dict(exclude=...)) so serialization stays consistent across all RoborockBase models? (Same in roborock/devices/traits/b01/q10/maps.py.)

if not record.raw or not record.map_len:
raise RoborockException("The Q10 clean record has no saved map detail")
record_id = record.raw
async with self._detail_lock:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that refresh_detail() simply publishes via self._command.send(...) and returns without awaiting a response, self._detail_lock is unnecessary (just like refresh() doesn't lock around self._command.send). Let's remove self._detail_lock from both CleanHistoryTrait and MapsTrait, along with the send_gate / send_started test helpers in conftest.py and the lock-recovery/serialization tests.



@pytest.mark.parametrize("failure", ["cancel", "send_error"])
async def test_saved_map_publication_recovers(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the earlier feedback on test_clean_history.py, test_saved_map_publication_recovers has a conditional branch (if failure == "cancel":) inside the test, and test_saved_map_selections_serialize_publication is only testing asyncio.Lock. Once _detail_lock is removed from MapsTrait, both of these tests can be removed.

@allenporter

Copy link
Copy Markdown
Contributor

Sorry for the long delay on reviewing this stack—I set aside time today to go through #937, #938, #939, #934, and #935 so you have feedback across the whole set rather than getting drip-fed comments one PR at a time.

First, the Q10 reverse-engineering and wire-format work across these PRs is great—having verified byte layouts for the map tail sections, zones/virtual walls, and room settings is super valuable.

To be transparent about why these have been slow for me to get through: the core protocol and trait changes are surrounded by a lot of extra defensive machinery (locks, confirmation futures, revision/freshness tracking, runtime type checks on typed args, and hundreds of lines of tests exercising those edge cases) plus the cumulative branch stacking. That makes the diffs much larger and harder to reason about than the underlying protocol changes need to be.

For Q10, we really want the traits to stay thin and boring:

  • Commands simply encode and publish to the channel (self._command.send(...)).
  • Incoming pushes decode into RoborockBase / RoborockEnum models, update trait state, and notify listeners.
  • No extra state machines, locks, or speculative APIs unless a downstream consumer (like Home Assistant) directly needs them.

Once we strip out that extra layer, these changes should be much smaller, independent of each other where possible (for example, #938 can target main directly without waiting on #937), and much faster for me to review and merge.

Thanks again!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants