shared updated #21
shared updated #21
Conversation
…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>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@03829bc9-740a-4ae1-9848-e7d8baa7c8ba.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@cabf9afe-642b-4ef6-b6d4-34d9c877830b.fritz.box>
WalkthroughRefactors drawing to use mvcrender.polygon_u8 and removes legacy drawing helpers; adds streaming/battery state and pil_size_rotation; introduces reusable image work buffer and explicit memory cleanup across handlers; minor type widening, docstrings, logging removals, and a version/dependency bump. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ReImageHandler
participant Buffers as Buffers(img_base/img_work)
participant Drawable as Drawable(polygon_u8)
participant Utils as pil_size_rotation
Client->>ReImageHandler: get_image_from_rrm(json, destinations?)
ReImageHandler->>ReImageHandler: _setup_robot_and_image(json)
Note right of ReImageHandler #DDEBF7: delete old img_base_layer\ncopy from temp np array\ndelete temp
ReImageHandler->>Buffers: Allocate or reuse img_work_layer
ReImageHandler->>Drawable: Draw zones/flags using polygon_u8
Drawable-->>ReImageHandler: Updated img_work_layer
ReImageHandler->>Utils: pil_size_rotation(image_rotate, PIL)
Utils-->>ReImageHandler: (width, height)
ReImageHandler-->>Client: return PIL image
sequenceDiagram
autonumber
participant Caller
participant CameraShared
participant Types as NOT_STREAMING_STATES
participant Utils as pil_size_rotation
Caller->>CameraShared: to_dict()
CameraShared->>CameraShared: vacuum_bat_charged(...) updates _battery_state
CameraShared->>Types: check vacuum_state ∈ NOT_STREAMING_STATES
CameraShared->>CameraShared: is_streaming() decision
alt streaming true
Note right of CameraShared #E6F4EA: docked & charged\nbinary image present\nnot in NOT_STREAMING_STATES
else streaming false
Note right of CameraShared #FDECE8: any required condition missing
end
CameraShared->>Utils: pil_size_rotation(image_rotate, image)
Utils-->>CameraShared: (w,h)
CameraShared-->>Caller: dict with size and streaming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
sca075
temporarily deployed
to
valetudo_map_parser_pypi
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
SCR/valetudo_map_parser/rand256_handler.py (2)
441-448: Bounding-box check reversed (always false for typical boxes).Use left ≤ x ≤ right, not right ≤ x ≤ left. This breaks room detection fallback.
- if ( - self.robot_in_room["right"] - <= int(robot_x) - <= self.robot_in_room["left"] - ) and ( - self.robot_in_room["up"] - <= int(robot_y) - <= self.robot_in_room["down"] - ): + if ( + self.robot_in_room["left"] + <= int(robot_x) + <= self.robot_in_room["right"] + ) and ( + self.robot_in_room["up"] + <= int(robot_y) + <= self.robot_in_room["down"] + ):
290-296: No-op: temporary rooms_pos is set then immediately restored without using it.This block should run detection while temp rooms are active to affect zoom/room state.
- # Store original rooms_pos and temporarily use the new one - original_rooms_pos = self.rooms_pos - self.rooms_pos = temp_rooms_pos - - # Restore original rooms_pos - self.rooms_pos = original_rooms_pos + # Store original rooms_pos and temporarily use the new one + original_rooms_pos = self.rooms_pos + self.rooms_pos = temp_rooms_pos + # Run detection with temp rooms to update zooming/room state + if robot_position: + _ = await self.async_get_robot_in_room( + int(robot_position[0] * 10), + int(robot_position[1] * 10), + robot_position_angle, + ) + # Restore original rooms_pos + self.rooms_pos = original_rooms_pos
🧹 Nitpick comments (3)
SCR/valetudo_map_parser/config/drawable.py (1)
418-433: Prefer mask alpha channel over RGB for robustness.Use the filled polygon’s alpha to derive the boolean mask. Avoids assumptions about RGB fill.
- # Extract boolean mask from first channel - zone_mask = (mask_rgba[:, :, 0] > 0) + # Extract boolean mask from alpha channel + zone_mask = mask_rgba[:, :, 3] > 0SCR/valetudo_map_parser/rand256_handler.py (2)
126-135: Docstring nit: destinations is used now.Update the comment to reflect its role in labels/room extraction.
- @param destinations: MQTT destinations for labels (unused). + @param destinations: MQTT destinations for labels/room extraction.
71-75: Signature change LGTM.Optional: return type of extract_room_properties should be Optional[RoomsProperties] since you return None on errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
SCR/valetudo_map_parser/config/drawable.py(6 hunks)SCR/valetudo_map_parser/config/rand256_parser.py(6 hunks)SCR/valetudo_map_parser/config/shared.py(4 hunks)SCR/valetudo_map_parser/config/types.py(1 hunks)SCR/valetudo_map_parser/config/utils.py(4 hunks)SCR/valetudo_map_parser/hypfer_draw.py(0 hunks)SCR/valetudo_map_parser/hypfer_handler.py(2 hunks)SCR/valetudo_map_parser/rand256_handler.py(8 hunks)SCR/valetudo_map_parser/rooms_handler.py(1 hunks)pyproject.toml(2 hunks)tests/refactored.py(5 hunks)
💤 Files with no reviewable changes (1)
- SCR/valetudo_map_parser/hypfer_draw.py
🧰 Additional context used
🧬 Code graph analysis (6)
SCR/valetudo_map_parser/config/utils.py (3)
SCR/valetudo_map_parser/config/shared.py (1)
to_dict(228-238)SCR/valetudo_map_parser/config/types.py (2)
to_dict(62-69)to_dict(660-662)SCR/valetudo_map_parser/map_data.py (1)
to_dict(768-770)
SCR/valetudo_map_parser/rand256_handler.py (4)
SCR/valetudo_map_parser/config/types.py (1)
Destinations(37-41)SCR/valetudo_map_parser/config/async_utils.py (2)
AsyncPIL(38-66)async_fromarray(42-44)SCR/valetudo_map_parser/hypfer_draw.py (1)
async_get_robot_in_room(379-542)SCR/valetudo_map_parser/config/utils.py (1)
async_copy_array(475-477)
tests/refactored.py (2)
SCR/valetudo_map_parser/hypfer_draw.py (1)
ImageDraw(19-573)SCR/valetudo_map_parser/config/drawable.py (1)
async_draw_obstacles(572-628)
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/hypfer_handler.py (1)
SCR/valetudo_map_parser/config/utils.py (1)
async_copy_array(475-477)
SCR/valetudo_map_parser/config/shared.py (1)
SCR/valetudo_map_parser/config/utils.py (1)
pil_size_rotation(696-704)
🔇 Additional comments (13)
SCR/valetudo_map_parser/rooms_handler.py (1)
164-173: LGTM! Explicit memory cleanup improves resource management.The explicit cleanup of intermediate arrays (
local_mask,struct_elem,eroded,mask) after their use is a good practice in memory-constrained environments or when processing large datasets.While Python's garbage collector will eventually reclaim these, explicit deletion ensures immediate memory release, which aligns with the broader memory-management improvements across this PR.
SCR/valetudo_map_parser/hypfer_handler.py (2)
257-262: LGTM! Proper memory cleanup before buffer reallocation.Explicitly deleting the old base layer before allocating a new one ensures that memory is freed immediately rather than waiting for garbage collection. This is particularly important for large image buffers and aligns with the memory-management improvements across this PR.
276-278: LGTM! Consistent memory management for working buffer.The explicit cleanup of the old working buffer before creating a new one follows the same memory-management pattern as the base layer and ensures efficient resource utilization.
SCR/valetudo_map_parser/config/rand256_parser.py (1)
1-2: LGTM! Docstrings improve code documentation.The addition of docstrings clarifies the parsing methods and their Xiaomi/Roborock implementation origins. This improves maintainability without any functional changes.
Also applies to: 82-82, 92-92, 120-120, 155-155, 218-218
tests/refactored.py (1)
11-11: LGTM! Formatting improvements without functional changes.The changes include import reordering and multi-line parameter list formatting, which improve code readability. No functional modifications were made.
Also applies to: 17-17, 323-641
SCR/valetudo_map_parser/config/utils.py (3)
114-119: LGTM! Proper image resource cleanup before replacement.Explicitly closing the old
last_imagebefore replacing it ensures PIL image resources are freed immediately. The try-except wrapper safely handles cases where the image is already closed.
184-184: LGTM! Better error return with actual shared data.Returning
self.shared.to_dict()instead of an empty dictionary provides more useful information even when image generation fails, improving error handling and diagnostics.
696-704: LGTM! Useful helper for rotation-aware size calculation.The
pil_size_rotationhelper centralizes the logic for computing image dimensions based on rotation angle. Returns(0, 0)for missing input, providing a safe fallback.SCR/valetudo_map_parser/config/shared.py (2)
125-133: LGTM! Battery state tracking logic is correct.The battery state machine properly tracks charging transitions:
- Sets to "not_charging" when not docked
- Transitions to "charged" when docked, was charging, and reaches 100%
- Otherwise sets based on battery level
The final return ensures
vacuum_bat_charged()only returnsTruewhen both docked and fully charged.
233-233: LGTM! Consistent size calculation using the new helper.Using
pil_size_rotation(self.image_rotate, self.new_image)ensures consistent rotation-aware size calculations across the codebase.SCR/valetudo_map_parser/config/types.py (1)
41-41: No direct usages found—please verify integer handling
I didn’t find any.updated,["updated"], or.get("updated")references in the codebase. Confirm that any code (serialization, deserialization, calculations, formatting, etc.) consuming this field handles bothintandfloattimestamp values correctly.SCR/valetudo_map_parser/config/drawable.py (1)
197-202: Switch to polygon_u8 looks good.Triangle draw is now efficient and consistent with the new draw pipeline.
SCR/valetudo_map_parser/rand256_handler.py (1)
163-177: Work buffer reuse looks good.Shape/dtype guard, delete old buffer, np.copyto usage are appropriate for reducing per-frame allocations.
Please confirm img_base_layer is always initialized (frame 0 path) before this executes to avoid None.shape errors.
Signed-off-by: Sandro Cantarella <sandro@cabf9afe-642b-4ef6-b6d4-34d9c877830b.fritz.box>
sca075
temporarily deployed
to
valetudo_map_parser_pypi
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
SCR/valetudo_map_parser/config/shared.py (1)
220-227: Logic error previously identified: streaming condition is inverted.The condition
or not self.binary_imageon line 225 causesis_streaming()to returnTruewhen there is no binary image, which is incorrect. Streaming should require the presence of an image.This issue was already identified in a previous review. Please apply the suggested fix from the earlier review to correct the logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
SCR/valetudo_map_parser/config/shared.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
SCR/valetudo_map_parser/config/shared.py (1)
SCR/valetudo_map_parser/config/utils.py (1)
pil_size_rotation(696-704)
🔇 Additional comments (2)
SCR/valetudo_map_parser/config/shared.py (2)
13-13: LGTM: Imports are used appropriately.Both
pil_size_rotationandNOT_STREAMING_STATESare correctly imported and utilized in the modified code segments below.Also applies to: 43-43
228-238: Image size calculation improved, but streaming field inherits logic error.Positive change: The use of
pil_size_rotation()on line 233 correctly accounts for image rotation (swapping width/height for 90°/270° rotations), which is an improvement over directly usingnew_image.size.Concern: The new "streaming" field on line 234 relies on
is_streaming(), which has a critical logic error (flagged separately). Onceis_streaming()is fixed, this field will work correctly.
|
@coderabbitai review all please |
|
@sca075, I'll perform a comprehensive review of all the changes in this PR. ✅ Actions performedReview triggered.
|
remove the shared usage from the end point to reduce the overhead
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation