Skip to content

feat: render Q10 map obstacle markers - #938

Open
hCoureau wants to merge 25 commits into
Python-roborock:mainfrom
hCoureau:feat/q10-obstacles
Open

hCoureau wants to merge 25 commits into
Python-roborock:mainfrom
hCoureau:feat/q10-obstacles

Conversation

@hCoureau

@hCoureau hCoureau commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Q10 map packets contain obstacle positions without validated type or photo metadata. This parses their bounded point tables and exposes calibrated, position-only markers on live, saved, and clean-history maps.

Implementation

  • Parse obstacle and skip-clean point tables using their declared counts and section boundaries. Truncated tables do not shift parsing into later sections.
  • Expose typed Q10Obstacle values through the existing map traits, returning copies of their lists.
  • Preserve archive obstacle data alongside the composed clean-record map/path response.
  • Render obstacle positions using their map-header coordinate calibration and honor configured drawable selection.
  • Keep obstacle state on its owning map packet; no additional request lifecycle is introduced.

Dependencies

Depends on #937. Parser support from #936 and the model-conformance fix in #965 are merged. Upstream main through b3a98b9 (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 f157a8c:

  • uv run pytest -q on Python 3.13: 1,452 passed, 53 xfailed, 92 snapshots passed.
  • All pre-commit hooks passed, including Ruff, mypy, codespell, and structured-file checks.
  • uv build passed for both wheel and sdist.
  • The affected cumulative protocol suite passed on Python 3.11 and 3.14: 96 passed, 10 snapshots passed on each version. The full cumulative suite passed on both versions before this test-only follow-up and merge.

No physical-device run was performed for this revision.

Tests cover bounded/truncated tables, archive data, marker placement, and drawable configuration. Earlier physical Q10 S5 validation exercised position-only obstacles; those hardware checks have not been rerun for this revision.

Obstacle types, confidence, and photo identifiers are not inferred. Rendering requires usable map-header calibration. Archive requests inherit the bounded lifecycle from #937, including its clean-history ambiguity barrier and same-map freshness limitation.

No private map captures or account data are included. Related: #767.

@lboue

lboue commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

@hCoureau Could you fix the conflict?

@hCoureau

hCoureau commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the merge conflicts with current main in ded170d by merging main and retaining its accepted 30-second Q10 CLI map-push timeout. The PR's feature changes are preserved. GitHub now reports this PR as conflict-free, and it remains ready for review.

Validation: 1,026 tests passed, 92 snapshots passed, all pre-commit hooks passed, and sdist/wheel builds passed. GitHub CI was triggered for the new head. Required maintainer approval still applies.

The existing stacked dependencies on #936 and #937 still apply; this update does not bypass their review or merge order. Conflict resolution and validation performed with OpenAI Codex assistance.

@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.

The map parser and renderer changes for decoding the post-carpet obstacle/skip-clean point tables and drawing Drawable.OBSTACLES look clean and well-bounded.

A few architectural and scoping items to simplify this PR:

  1. Decouple from #937 and keep raw obstacle coordinates internal to the map packet/renderer:

    • Neither V1 (v1/map_content.py) nor Q7 (b01/q7/map_content.py) exposes a separate obstacles or detail_obstacles property on MapContentTrait, CleanSummaryTrait, or MapsTrait—obstacles are parsed on the map packet and rendered into the map image (image_content / MapData.obstacles).
    • Moreover, Q10Obstacle is in an internal raw map-packet coordinate frame (50 raw units/pixel relative to the internal map header origin), which isn't usable by a trait consumer without Q10HeaderCalibration.
    • If we keep obstacles and skip_cleaning_points on Q10MapPacket for the parser and renderer and drop map.obstacles, clean_history.detail_obstacles, and maps.detail_obstacles, this PR no longer depends on #937 (since #936 is already merged into main) and can be rebased directly onto main.
  2. Avoid Q10Obstacle(Q10Point) inheritance and stringified forward references in b01_q10_map_parser.py:

    • See inline comments on Q10Obstacle inheriting Q10Point.to_roborock() (which applies the 2.5 mm trace scale factor rather than the 50 raw units/pixel obstacle scale) and the stringified forward references on Q10MapPacket.
  3. Test public behavior rather than private renderer helpers / patched factory call_args:

    • See inline comments on tests/map/test_b01_q10_render.py and tests/devices/test_device_manager.py.

return self._trace_packet.points if self._trace_packet else []

@property
def obstacles(self) -> list[Q10Obstacle]:

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.

Let's remove obstacles from MapContentTrait (and "obstacles" from as_dict()), as well as detail_obstacles from CleanHistoryTrait and MapsTrait. Neither V1 nor Q7 exposes raw obstacle lists on traits, and Q10Obstacle is in raw map-header-relative coordinates (50 units/pixel) rather than common coordinates. Keeping obstacles on Q10MapPacket for render_q10_map also allows this PR to be rebased directly onto main without depending on #937.

"""Carpet mask decoded from the packet tail: a full ``width*height`` grid in
the same (top-down) pixel space as :attr:`grid`, where a non-zero cell is
carpet (the value is the carpet kind). ``None`` if the packet carried none."""
obstacles: list["Q10Obstacle"] = field(default_factory=list)

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.

Please define Q10Obstacle (and Q10Point) above Q10MapPacket so we can use concrete types here instead of stringified forward references ("Q10Obstacle", "Q10Point").



@dataclass
class Q10Obstacle(Q10Point):

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.

Q10Point represents a 2.5 mm trace-coordinate point and defines to_roborock() using Q10RoborockPoint.from_trace(self.x, self.y). Subclassing Q10Point here means Q10Obstacle (which uses 50 raw units/pixel) inherits to_roborock() with the wrong coordinate scale factor.

Let's make Q10Obstacle(RoborockBase) a standalone dataclass with x: int and y: int (without inheriting Q10Point.to_roborock()), and have _parse_counted_points(tail: bytes, offset: int) return tuple[list[tuple[int, int]], int | None] (or construct the points at the call site) without needing _PointType = TypeVar(...).


device_manager = await create_device_manager(USER_PARAMS)
map_parser_config = MapParserConfig(drawables=[Drawable.OBSTACLES], map_scale=2)
with patch("roborock.devices.device_manager.b01.q10.create", wraps=create_q10) as q10_create:

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.

Avoid patching b01.q10.create with wraps just to assert on call_args.kwargs["map_parser_config"]. If we want to verify that map_parser_config is forwarded to the Q10 trait, we can either check the rendered map behavior or keep config-wiring tests in tests/devices/traits/b01/q10/test_map.py.

)
map_data = MapData()

calibration = _obstacle_calibration(packet)

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 importing and testing private renderer helpers (_obstacle_calibration and _place_obstacles) directly here and in test_place_obstacles_requires_header_calibration, please test through the public parser/renderer entrypoints (or B01Q10MapParser.parse / render_q10_map).

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.

3 participants