Repository navigation
Conversation
|
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, 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. |
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.
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:
-
Clean record & saved map rendering vs. live map isolation:
- Protecting
properties.mapso that03 01(clean record archive) and04 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
CleanHistoryTraitandMapsTraiteagerly 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 onclean_summaryormaps. - Because
03 01packets carry no record ID on the wire,clean_history.detail/detail_image_contentcannot even be reliably attributed to a specificQ10CleanRecord(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_contentandmaps.detail_image_content? If there isn't a concrete consumer need for rendering archive images right now, should we simplify this PR to just isolatingproperties.map(and/or exposing the parsed packet data without eager image rendering)?
- Protecting
-
Removing
_detail_lockand lock-serialization tests:- In my earlier comment,
asyncio.Lockwas only mentioned as an alternative to the hand-rolled locking whenrefresh_detail()was blocking on a response future. Now thatrefresh_detail()is a fire-and-forgetself._command.send(...)just likerefresh(), we don't need_detail_lockat all—nor thesend_gate/ lock-serialization / cancellation tests.
- In my earlier comment,
| ) | ||
|
|
||
| async def refresh_detail(self, record: Q10CleanRecord) -> None: | ||
| """Publish a selection using the complete firmware record identifier. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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]: |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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.
|
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:
Once we strip out that extra layer, these changes should be much smaller, independent of each other where possible (for example, #938 can target Thanks again! |
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
mapsowns saved-map metadata and the latest received preview.detail_map_idcomes from the pushed packet itself.clean_historyinherits its public data and derived accessors from theCleanHistorydataclass, aRoborockBasemodel. It exposes the latest received archive map, historical path and rendered bytes.refresh_detail()methods return after publishing. Device pushes populate the read models and notifyadd_update_listener()subscribers. Anasyncio.Lockserializes publication only; missing responses do not block later requests.Q10PropertiesApiroutes typedQ10CleanRecordDetailand saved-map packets to their archive owners. Archive updates cannot replace live-map state.detail_image_content.Consumer expectations and protocol limits
Subscribe before requesting an archive, then read the trait when its listener fires.
roborock/devices/README.mddocuments 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_recordassociation. Saved-map packets identify the map, but contain no request ID; consumers should inspectdetail_map_idbefore 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
mainthroughb3a98b9(7.12.0).Validation
Validation of
f1f5e24:No physical-device run was performed for this revision. No private map captures or account data are included. Related: #767.