Skip to content

Conversation

@sca075
Copy link
Owner

@sca075 sca075 commented Oct 7, 2025

Some function was moved to the MCV Render, type checking and other small refactors like utils for dedicated usage role added too.

Summary by CodeRabbit

  • New Features

    • Improved map rendering with smoother lines, circles, and alpha-aware blending; rotation-aware image sizing.
  • Performance

    • Faster drawing pipeline using optimized primitives and preblended rendering.
  • Bug Fixes

    • Prevented out-of-bounds drawing; more robust image-size and error handling.
  • Refactor

    • Unified rendering path, removed legacy drawing helpers, and simplified drawing APIs.
  • Documentation

    • Added docstrings to parsing utilities.
  • Chores

    • Reduced verbose logging; minor public-type and signature adjustments for better typing and error coverage.

sca075 and others added 9 commits October 2, 2025 18:18
…ean tested for rand.

Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
…unction with C based from mcvrender.

Signed-off-by: Sandro Cantarella <sandro@509dc541-f930-4e46-87da-4482e9d9a7bb.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@13de464c-e6f5-4665-b8d7-d6acf4207b24.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@13de464c-e6f5-4665-b8d7-d6acf4207b24.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
@sca075 sca075 self-assigned this Oct 7, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

Walkthrough

Refactors drawing to use mvcrender primitives for blending and rasterization, updates type aliases and signatures, adds rotation-aware image sizing, expands handler signatures/error handling, adds parser docstrings, adjusts tests for method binding, and removes deprecated drawing helpers.

Changes

Cohort / File(s) Summary
Rendering pipeline refactor
SCR/valetudo_map_parser/config/drawable.py
Replaced manual blending and Bresenham-based rasterization with mvcrender APIs (get_blended_color, sample_and_blend_color, line_u8, circle_u8). Simplified alpha handling, preblend segments, removed draw_filled_circle and batch_draw_elements, and changed _line/other signatures to use NumpyArray.
Handler updates and initialization
SCR/valetudo_map_parser/rand256_handler.py
Updated public method signatures to accept `Destinations
Utilities and image sizing
SCR/valetudo_map_parser/config/utils.py
Added pil_size_rotation(image_rotate, pil_img) and centralized size computation via it in resize flow. Removed deprecated blending helpers and reduced debug logging.
Shared config updates
SCR/valetudo_map_parser/config/shared.py
Imported Image and pil_size_rotation, added ATTR_IMAGE_LAST_UPDATED to exports, and changed to_dict image size to use rotation-aware computation. Minor reordering of exports/types.
Type aliases and annotations
SCR/valetudo_map_parser/config/types.py
Updated Destinations.updated to `Optional[float
Parser docstrings
SCR/valetudo_map_parser/config/rand256_parser.py
Added docstrings to several private parsing methods (_parse_carpet_map, _parse_area, _parse_zones, _parse_walls, _parse_path_block, _parse_goto_target, parse_blocks) and minor formatting tweaks; no functional changes.
Tests adaptation
tests/refactored.py
Converted Drawable.draw_filled_circle and Drawable.async_draw_obstacles from @staticmethod to instance methods; added asyncio import and minor formatting adjustments.
Logging cleanup
SCR/valetudo_map_parser/hypfer_draw.py
Removed an unused logging branch in async_draw_zones.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant ReImageHandler
  participant Shared
  participant Utils as utils.pil_size_rotation
  participant Drawable
  participant MVCR as mvcrender

  Client->>ReImageHandler: get_image_from_rrm(m_json, destinations?)
  ReImageHandler->>ReImageHandler: extract_room_properties(...)
  ReImageHandler->>Shared: initialize_drawing_config(self)
  Note right of Shared: returns (drawing_config, draw)
  ReImageHandler->>Utils: pil_size_rotation(image_rotate, pil_img)
  Utils-->>ReImageHandler: (width, height)
  ReImageHandler->>Drawable: draw map elements
  Drawable->>MVCR: get_blended_color / sample_and_blend_color
  Drawable->>MVCR: line_u8 / circle_u8
  MVCR-->>Drawable: stamped primitives
  ReImageHandler-->>Client: PilPNG | None
Loading
sequenceDiagram
  autonumber
  participant Caller
  participant Drawable
  participant MVCR as mvcrender

  Caller->>Drawable: _line(layer, x1,y1,x2,y2,color,width)
  alt color alpha < 255
    Drawable->>MVCR: get_blended_color(layer, color)
  else
    Note right of Drawable: Use original color
  end
  Drawable->>MVCR: line_u8(layer, x1,y1,x2,y2,width,color')
  MVCR-->>Caller: updated layer
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Possibly related PRs

Poem

I nibble through pixels with whiskered delight,
Lines now glide smooth, circles neat and bright.
Colors preblended, no Bresenham fight—
Rotate, resize, we frame it just right.
A hop, a crop, and off I go, rendering maps aglow. 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The title “Removing Unused Code” is a generic, non-descriptive summary that refers to some deletions but fails to convey the primary focus of the changeset—migrating to mvcrender, updating type signatures, and refactoring the rendering pipeline—making its relevance to the main changeset inconclusive. Please update the title to clearly reflect the main changes, for example “Migrate rendering pipeline to mvcrender and remove legacy drawing utilities,” so readers immediately understand the core intent of the pull request.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
SCR/valetudo_map_parser/rooms_handler.py (1)

400-457: Destinations lookup never matches because of type mismatch.

room_id_to_data uses the raw room["id"] (string UUIDs in Valetudo destinations), while segment_id comes from unsorted_id as int. if segment_id in room_id_to_data: is therefore always false, so room names never update from destinations. Convert keys to int (if numeric) or coerce segment_id to str before looking up.

Example fix:

-        room_id_to_data = {room["id"]: room for room in room_data}
+        room_id_to_data = {}
+        for room in room_data:
+            rid = room.get("id")
+            if rid is None:
+                continue
+            try:
+                rid_key = int(rid)
+            except (TypeError, ValueError):
+                rid_key = str(rid)
+            room_id_to_data[rid_key] = room
...
-                if segment_id in room_id_to_data:
-                    room_info = room_id_to_data[segment_id]
+                lookup_key = segment_id if segment_id in room_id_to_data else str(segment_id)
+                if lookup_key in room_id_to_data:
+                    room_info = room_id_to_data[lookup_key]
SCR/valetudo_map_parser/config/rand256_parser.py (1)

216-278: Block pointer skips headers; parser loses sync.

block_start_position now advances by block_data_length + self._get_int8(header, 2). The header length (returned earlier as block_header_length) is no longer added, so the loop points into the middle of the current header/data blob and subsequent blocks parse garbage. Restore advancement by block_header_length + block_data_length (or _get_int8(header, 2) if that’s the header length, but add the full header bytes you just consumed).

Fix:

-                block_start_position = (
-                    block_start_position + block_data_length + self._get_int8(header, 2)
-                )
+                block_start_position = (
+                    block_start_position + block_header_length + block_data_length
+                )
SCR/valetudo_map_parser/rand256_handler.py (2)

82-122: Handle optional destinations safely

destinations is declared optional, but the new code still executes dict(dest_json) even when it is None, which raises TypeError. Any caller using the default (e.g., internal retries without MQTT payloads) will now crash before room extraction runs.

Guard the optional parameter before coercing it to a dict.

-            dest_json = destinations
-            zones_data = dict(dest_json).get("zones", [])
-            points_data = dict(dest_json).get("spots", [])
+            dest_json = dict(destinations or {})
+            zones_data = dest_json.get("zones", [])
+            points_data = dest_json.get("spots", [])

333-353: Await async zoom helper

AutoCrop.auto_trim_and_zoom_image is defined as an async coroutine. The new call site dropped the await, so the coroutine object is returned and later treated as a NumPy array, breaking rendering (e.g., AsyncPIL.async_fromarray will receive a coroutine). Restore the await so the image is actually processed.

-        img_np_array = self.auto_trim_and_zoom_image(
+        img_np_array = await self.auto_trim_and_zoom_image(
             img_np_array,
             detect_colour=colors["background"],
             margin_size=int(self.shared.margins),
             rotate=int(self.shared.image_rotate),
             zoom=self.zooming,
             rand256=True,
         )
🧹 Nitpick comments (1)
tests/refactored.py (1)

575-648: Type annotation contradicts actual usage.

draw_filled_circle expects an iterable of circle centers (for cx, cy in centers:) and the docstring calls it an (N, 2) NumPy array, yet the type hint is Tuple[int, int]. Static type checking will flag this, and it obscures intent. Please annotate as np.ndarray (or Sequence[Tuple[int, int]]) consistently for both the signature and docstring.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b759c45 and 470f31c.

📒 Files selected for processing (17)
  • SCR/valetudo_map_parser/__init__.py (1 hunks)
  • SCR/valetudo_map_parser/config/color_utils.py (2 hunks)
  • SCR/valetudo_map_parser/config/drawable.py (9 hunks)
  • SCR/valetudo_map_parser/config/drawable_elements.py (0 hunks)
  • SCR/valetudo_map_parser/config/enhanced_drawable.py (0 hunks)
  • SCR/valetudo_map_parser/config/rand256_parser.py (7 hunks)
  • SCR/valetudo_map_parser/config/shared.py (3 hunks)
  • SCR/valetudo_map_parser/config/status_text/status_text.py (1 hunks)
  • SCR/valetudo_map_parser/config/types.py (3 hunks)
  • SCR/valetudo_map_parser/config/utils.py (6 hunks)
  • SCR/valetudo_map_parser/hypfer_handler.py (3 hunks)
  • SCR/valetudo_map_parser/map_data.py (3 hunks)
  • SCR/valetudo_map_parser/rand256_handler.py (12 hunks)
  • SCR/valetudo_map_parser/reimg_draw.py (1 hunks)
  • SCR/valetudo_map_parser/rooms_handler.py (1 hunks)
  • pyproject.toml (2 hunks)
  • tests/refactored.py (5 hunks)
💤 Files with no reviewable changes (2)
  • SCR/valetudo_map_parser/config/drawable_elements.py
  • SCR/valetudo_map_parser/config/enhanced_drawable.py
🧰 Additional context used
🧬 Code graph analysis (8)
SCR/valetudo_map_parser/hypfer_handler.py (4)
SCR/valetudo_map_parser/config/auto_crop.py (1)
  • AutoCrop (27-452)
SCR/valetudo_map_parser/config/async_utils.py (1)
  • AsyncPIL (38-66)
SCR/valetudo_map_parser/config/utils.py (1)
  • initialize_drawing_config (704-727)
SCR/valetudo_map_parser/config/types.py (1)
  • RoomStore (96-128)
SCR/valetudo_map_parser/rand256_handler.py (4)
SCR/valetudo_map_parser/config/auto_crop.py (1)
  • AutoCrop (27-452)
SCR/valetudo_map_parser/config/async_utils.py (1)
  • AsyncPIL (38-66)
SCR/valetudo_map_parser/config/types.py (2)
  • Destinations (37-41)
  • RoomStore (96-128)
SCR/valetudo_map_parser/config/utils.py (3)
  • initialize_drawing_config (704-727)
  • check_zoom_and_aspect_ratio (260-268)
  • prepare_resize_params (229-246)
SCR/valetudo_map_parser/__init__.py (3)
SCR/valetudo_map_parser/config/drawable.py (1)
  • status_text (626-670)
SCR/valetudo_map_parser/config/status_text/status_text.py (1)
  • StatusText (16-96)
SCR/valetudo_map_parser/config/types.py (5)
  • CameraModes (616-625)
  • RoomStore (96-128)
  • SnapshotStore (181-218)
  • TrimCropData (54-93)
  • UserLanguageStore (132-177)
SCR/valetudo_map_parser/config/shared.py (1)
SCR/valetudo_map_parser/config/utils.py (1)
  • pil_size_rotation (693-701)
SCR/valetudo_map_parser/rooms_handler.py (1)
SCR/valetudo_map_parser/map_data.py (1)
  • RandImageData (411-710)
SCR/valetudo_map_parser/config/utils.py (6)
SCR/valetudo_map_parser/map_data.py (2)
  • HyperMapData (714-818)
  • Size (110-112)
SCR/valetudo_map_parser/config/async_utils.py (1)
  • AsyncNumPy (17-35)
SCR/valetudo_map_parser/config/drawable.py (2)
  • Drawable (27-670)
  • status_text (626-670)
SCR/valetudo_map_parser/config/status_text/status_text.py (1)
  • StatusText (16-96)
SCR/valetudo_map_parser/config/types.py (1)
  • Destinations (37-41)
SCR/valetudo_map_parser/rand256_handler.py (1)
  • get_rooms_attributes (370-378)
tests/refactored.py (1)
SCR/valetudo_map_parser/config/drawable.py (1)
  • async_draw_obstacles (567-623)
SCR/valetudo_map_parser/config/drawable.py (3)
SCR/valetudo_map_parser/config/color_utils.py (1)
  • get_blended_color (8-104)
SCR/valetudo_map_parser/config/colors.py (1)
  • sample_and_blend_color (457-519)
tests/refactored.py (2)
  • Drawable (20-689)
  • _line (177-228)

sca075 added 2 commits October 7, 2025 17:36
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@03829bc9-740a-4ae1-9848-e7d8baa7c8ba.fritz.box>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
SCR/valetudo_map_parser/config/utils.py (3)

189-192: Fix return arity in exception path (breaks the declared return type).

async_get_image declares Tuple[PilPNG | bytes, dict] but the except block returns a single value. Return a 2‑tuple consistently.

Apply this diff:

-            return (
-                self.shared.last_image if hasattr(self.shared, "last_image") else None
-            )
+            return (
+                self.shared.last_image if hasattr(self.shared, "last_image") else None,
+                {},
+            )

162-166: Store timestamp as float, not datetime (type drift vs Shared.image_last_updated).

CameraShared.image_last_updated is a float; here a datetime is assigned. Use time() to keep types consistent.

-                self.shared.image_last_updated = datetime.datetime.fromtimestamp(time())
+                self.shared.image_last_updated = time()

682-686: Order-of-operations bug: set crop_size before computing offsets.

async_map_coordinates_offset reads self.crop_img_size to compute offsets. You update crop_size after calling it, so offsets are computed with stale dimensions.

Apply this reorder:

-        if (params.crop_size is not None) and (params.offset_func is not None):
-            offset = OffsetParams(wsf, hsf, new_width, new_height, params.is_rand)
-            params.crop_size[0], params.crop_size[1] = await params.offset_func(offset)
+        if (params.crop_size is not None) and (params.offset_func is not None):
+            # Update crop size first so offset computation uses the new dimensions
+            params.crop_size[0], params.crop_size[1] = new_width, new_height
+            offset = OffsetParams(wsf, hsf, new_width, new_height, params.is_rand)
+            await params.offset_func(offset)
🧹 Nitpick comments (4)
SCR/valetudo_map_parser/config/utils.py (2)

691-700: Type hints and explicit None check for pil_size_rotation.

Minor polish: add annotations and explicit None handling for clarity.

-def pil_size_rotation(image_rotate, pil_img):
-    """Return the size of the image."""
-    if not pil_img:
-        return 0, 0
+from typing import Optional, Tuple
+
+def pil_size_rotation(image_rotate: int, pil_img: Optional[Image.Image]) -> Tuple[int, int]:
+    """Return (width, height), accounting for rotation."""
+    if pil_img is None:
+        return (0, 0)
     if image_rotate in [0, 180]:
         width, height = pil_img.size
     else:
         height, width = pil_img.size
-    return width, height
+    return (width, height)

159-163: Avoid unnecessary PNG encoding when bytes_format=False.

Encoding self.shared.last_image every frame is wasteful. Only compute binary_image when requested.

-                if bytes_format:
-                    self.shared.binary_image = pil_to_png_bytes(new_image)
-                else:
-                    self.shared.binary_image = pil_to_png_bytes(self.shared.last_image)
+                if bytes_format:
+                    self.shared.binary_image = pil_to_png_bytes(new_image)
SCR/valetudo_map_parser/config/drawable.py (2)

287-311: Coordinate convention inconsistency (center as (y, x) here, elsewhere often (x, y)).

To reduce mistakes, standardize Point ordering across methods or rename parameters to (cy, cx). At minimum, document the convention in the public docstring.

-    def _filled_circle(
+    def _filled_circle(
         image: NumpyArray,
-        center: Point,
+        center: Point,  # expected as (y, x)
         radius: int,
         color: Color,

56-80: Blending per tile uses a single center sample; acceptable but may band on large pixel_size.

If artifacts appear with big pixel_size, consider sampling a small grid (e.g., 2×2) or skipping sampling when region uniformity is guaranteed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4298db and b3dd06a.

📒 Files selected for processing (4)
  • SCR/valetudo_map_parser/config/drawable.py (9 hunks)
  • SCR/valetudo_map_parser/config/shared.py (3 hunks)
  • SCR/valetudo_map_parser/config/utils.py (6 hunks)
  • SCR/valetudo_map_parser/hypfer_draw.py (0 hunks)
💤 Files with no reviewable changes (1)
  • SCR/valetudo_map_parser/hypfer_draw.py
🧰 Additional context used
🧬 Code graph analysis (3)
SCR/valetudo_map_parser/config/drawable.py (1)
SCR/valetudo_map_parser/config/colors.py (1)
  • sample_and_blend_color (457-519)
SCR/valetudo_map_parser/config/shared.py (1)
SCR/valetudo_map_parser/config/utils.py (1)
  • pil_size_rotation (691-699)
SCR/valetudo_map_parser/config/utils.py (7)
SCR/valetudo_map_parser/map_data.py (1)
  • HyperMapData (714-818)
SCR/valetudo_map_parser/config/async_utils.py (1)
  • AsyncNumPy (17-35)
SCR/valetudo_map_parser/config/drawable.py (2)
  • Drawable (27-651)
  • status_text (607-651)
SCR/valetudo_map_parser/config/drawable_elements.py (1)
  • DrawingConfig (55-290)
SCR/valetudo_map_parser/config/status_text/status_text.py (1)
  • StatusText (16-96)
SCR/valetudo_map_parser/config/types.py (1)
  • Destinations (37-41)
SCR/valetudo_map_parser/rand256_handler.py (1)
  • get_rooms_attributes (370-378)
🔇 Additional comments (4)
SCR/valetudo_map_parser/config/shared.py (1)

215-221: Rotation-aware image size in payload looks good.

Using pil_size_rotation(self.image_rotate, self.new_image) removes the previous fixed fallback and aligns size with rotation.

SCR/valetudo_map_parser/config/drawable.py (2)

222-237: Line drawing refactor: preblend + line_u8 is a solid improvement.

Segment-level preblend with get_blended_color then stamping via line_u8 is cleaner and faster.


17-19: Confirm mvcrender wheel coverage. mvcrender is already pinned at >=0.0.5 in pyproject.toml; verify that binary wheels exist for all your deployment platforms to avoid fallback to source builds of the C extension.

SCR/valetudo_map_parser/config/utils.py (1)

196-201: Remove tuple-unpacking suggestion for get_rooms_attributes
get_rooms_attributes returns only room_properties (self.room_propriety), not a 3-tuple; no unpacking needed.

Likely an incorrect or invalid review comment.

@sca075 sca075 marked this pull request as draft October 7, 2025 20:16
@sca075 sca075 marked this pull request as ready for review October 7, 2025 20:18
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
SCR/valetudo_map_parser/config/rand256_parser.py (1)

153-189: Remove duplicate _parse_walls method definition.

The _parse_walls method is defined twice (lines 153-170 and lines 173-189). In Python, the second definition silently overrides the first, which means:

  1. The docstring added at line 154 is lost
  2. The formatting changes in lines 162-169 never execute
  3. This creates confusion for maintainers

Remove one of the duplicate definitions. If the formatting changes in lines 162-169 were intentional, keep the first definition (lines 153-170). Otherwise, keep the second (lines 173-189) and add the docstring to it.

Apply this diff to remove the duplicate and preserve both the docstring and the cleaner formatting:

-    @staticmethod
-    def _parse_walls(data: bytes, header: bytes) -> list:
-        wall_pairs = RRMapParser._get_int16(header, 0x08)
-        walls = []
-        for wall_start in range(0, wall_pairs * 8, 8):
-            x0 = RRMapParser._get_int16(data, wall_start + 0)
-            y0 = RRMapParser._get_int16(data, wall_start + 2)
-            x1 = RRMapParser._get_int16(data, wall_start + 4)
-            y1 = RRMapParser._get_int16(data, wall_start + 6)
-            walls.append(
-                [
-                    x0,
-                    RRMapParser.Tools.DIMENSION_MM - y0,
-                    x1,
-                    RRMapParser.Tools.DIMENSION_MM - y1,
-                ]
-            )
-        return walls
🧹 Nitpick comments (1)
SCR/valetudo_map_parser/config/utils.py (1)

692-700: Good extraction of rotation logic into a reusable helper.

The function correctly handles image rotation by swapping dimensions for 90/270 degree rotations and safely handles None images.

Consider adding type hints and improving the docstring for this public API function:

-def pil_size_rotation(image_rotate, pil_img):
-    """Return the size of the image."""
+def pil_size_rotation(image_rotate: int, pil_img: Optional[PilPNG]) -> Tuple[int, int]:
+    """
+    Return the size of the image accounting for rotation.
+    
+    For 90/270 degree rotations, width and height are swapped.
+    
+    Args:
+        image_rotate: Rotation angle in degrees (0, 90, 180, or 270)
+        pil_img: PIL Image or None
+        
+    Returns:
+        Tuple of (width, height), or (0, 0) if image is None
+    """
     if not pil_img:
         return 0, 0
     if image_rotate in [0, 180]:
         width, height = pil_img.size
     else:
         height, width = pil_img.size
     return width, height
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b3dd06a and c9147c7.

📒 Files selected for processing (2)
  • SCR/valetudo_map_parser/config/rand256_parser.py (5 hunks)
  • SCR/valetudo_map_parser/config/utils.py (2 hunks)
🔇 Additional comments (2)
SCR/valetudo_map_parser/config/rand256_parser.py (1)

81-81: LGTM! Docstrings improve code clarity.

The added docstrings consistently document the parsing methods with clear descriptions of their purpose and approach (Xiaomi method).

Also applies to: 91-91, 119-119, 236-236

SCR/valetudo_map_parser/config/utils.py (1)

234-235: Nice refactoring to centralize size rotation logic.

Extracting the rotation-aware size calculation into pil_size_rotation improves maintainability and reduces duplication. The implementation correctly swaps dimensions for 90/270 degree rotations.

sca075 added 2 commits October 7, 2025 22:57
Signed-off-by: Sandro Cantarella <sandro@e12ecfb7-0503-4719-87b7-0b014abdffa8.fritz.box>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
SCR/valetudo_map_parser/config/rand256_parser.py (1)

175-175: Use a docstring instead of a regular comment.

Line 175 uses a regular comment rather than a docstring, which is inconsistent with the other parsing methods that have proper docstrings.

Apply this diff:

-    def _parse_path_block(buf: bytes, offset: int, length: int) -> Dict[str, Any]:
-        """Parse path block using EXACT same method as working parser."""
+    def _parse_path_block(buf: bytes, offset: int, length: int) -> Dict[str, Any]:
+        """Parse path block using EXACT same method as working parser.
+        
+        Args:
+            buf: Raw map buffer bytes.
+            offset: Starting offset in buffer.
+            length: Length of path data.
+        
+        Returns:
+            Dictionary with 'current_angle' and 'points' keys.
+        """
🧹 Nitpick comments (6)
SCR/valetudo_map_parser/config/rand256_parser.py (6)

82-82: Consider enriching the docstring with parameter and return type details.

While the docstring mentions the parsing approach, it would be more helpful to document the parameter type and return value.

Example enhancement:

-    """Parse carpet map using Xiaomi method."""
+    """Parse carpet map using Xiaomi method.
+    
+    Args:
+        data: Raw carpet map bytes.
+    
+    Returns:
+        Set of carpet pixel indices.
+    """

92-92: Consider enriching the docstring with parameter and return type details.

The docstring would benefit from documenting parameters and return values for better maintainability.

Example enhancement:

-    """Parse area using Xiaomi method."""
+    """Parse area using Xiaomi method.
+    
+    Args:
+        header: Block header bytes.
+        data: Area data bytes.
+    
+    Returns:
+        List of area coordinate arrays [x0, y0, x1, y1, x2, y2, x3, y3].
+    """

120-120: Consider enriching the docstring with parameter and return type details.

Adding parameter and return documentation would improve code clarity.

Example enhancement:

-    """Parse zones using Xiaomi method."""
+    """Parse zones using Xiaomi method.
+    
+    Args:
+        data: Zone data bytes.
+        header: Block header bytes.
+    
+    Returns:
+        List of zone coordinate arrays [x0, y0, x1, y1].
+    """

155-155: Consider enriching the docstring with parameter and return type details.

The docstring would be more helpful with parameter and return documentation.

Example enhancement:

-    """Parse walls using Xiaomi method."""
+    """Parse walls using Xiaomi method.
+    
+    Args:
+        data: Wall data bytes.
+        header: Block header bytes.
+    
+    Returns:
+        List of wall coordinate arrays [x0, y0, x1, y1].
+    """

190-190: Consider enriching the docstring with parameter and return type details.

Adding parameter and return documentation would improve the docstring's utility.

Example enhancement:

-    """Parse goto target using Xiaomi method."""
+    """Parse goto target using Xiaomi method.
+    
+    Args:
+        data: Goto target data bytes.
+    
+    Returns:
+        List [x, y] coordinates, or [0, 0] on error.
+    """

218-218: Consider enriching the docstring with parameter and return type details.

The docstring would benefit from documenting parameters and return values.

Example enhancement:

-    """Parse all blocks using Xiaomi method."""
+    """Parse all blocks using Xiaomi method.
+    
+    Args:
+        raw: Raw map buffer bytes.
+        pixels: Whether to extract individual pixel data (default True).
+    
+    Returns:
+        Dictionary mapping block type IDs to parsed block data.
+    """
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9147c7 and 0a29d0c.

📒 Files selected for processing (2)
  • SCR/valetudo_map_parser/config/rand256_parser.py (6 hunks)
  • SCR/valetudo_map_parser/rand256_handler.py (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
SCR/valetudo_map_parser/rand256_handler.py (1)
SCR/valetudo_map_parser/config/types.py (1)
  • Destinations (37-41)
🔇 Additional comments (7)
SCR/valetudo_map_parser/config/rand256_parser.py (1)

1-2: LGTM! Clear module-level documentation.

The module docstring provides a concise overview of the parser's purpose and implementation basis.

SCR/valetudo_map_parser/rand256_handler.py (6)

22-24: LGTM! Proper imports for enhanced functionality.

The addition of LOGGER and Destinations imports supports the new destinations-aware workflow and improved logging.


70-74: LGTM! Signature extension preserves backward compatibility.

The addition of the destinations parameter with a None default ensures backward compatibility while enabling destinations-aware processing. The return type annotation RoomsProperties is clear and helpful.


88-91: LGTM! Safe fallback for missing destinations.

The code safely handles the case when destinations is None by defaulting to an empty dict and extracting zones/spots with safe .get() calls.


128-128: LGTM! Signature extension with clear documentation.

The addition of the destinations parameter with proper type hints and a comment noting it's for MQTT destinations provides good context. The return type annotation PilPNG | None is appropriately specific.


221-225: LGTM! Improved readability with multi-line formatting.

The multi-line condition formatting enhances readability without changing the logic.


272-272: LGTM! Comprehensive error handling.

The broadened exception handling to include ValueError, KeyError, and TypeError appropriately covers potential failure modes when processing destinations data.

@sca075 sca075 merged commit 6d4cea3 into main Oct 8, 2025
3 checks passed
This was referenced Oct 8, 2025
This was referenced Oct 23, 2025
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