Mchp rev2 update - #122
Closed
LianVictor wants to merge 0 commit into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesAdded PolarFire R2 enumeration and firmware flashing support. Added multi-camera IMX477 players and Depth Anything V2 applications. Updated IMX477 example commands and PolarFire R2 hardware documentation. PolarFire R2 and IMX477 support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
examples/single_network_multi_imx477_player.py (1)
304-310: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not overwrite the core R2 strategy.
The core R2 registration adds
board_descriptionandgpio_pin_count. Both players replace that strategy with one that has empty additional metadata. The core registration already uses the required four sensors, two dataplanes, and one SIF per sensor.
examples/single_network_multi_imx477_player.py#L304-L310: remove the localset_uuid_strategycall.examples/linux_single_network_multi_imx477_player.py#L287-L293: remove the localset_uuid_strategycall.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/single_network_multi_imx477_player.py` around lines 304 - 310, Remove the local Enumerator.set_uuid_strategy call and its BasicEnumerationStrategy setup in examples/single_network_multi_imx477_player.py:304-310 and examples/linux_single_network_multi_imx477_player.py:287-293. Preserve the core R2 registration, including its board_description and gpio_pin_count metadata, and rely on its existing four-sensor, two-dataplane, one-SIF-per-sensor configuration.examples/depth_anything_v2/linux_depth_anything_v2_imx477.py (2)
76-81: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider keeping the transpose on the device.
Each frame moves the tensor to host memory and back. On the 4K camera path this adds latency to every frame.
cp.ascontiguousarray(cp.moveaxis(tensor, 2, 0)[None])produces a contiguous device tensor and avoids both copies. Test it before you adopt it, because the comment reports a stride problem with the inference backend.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/depth_anything_v2/linux_depth_anything_v2_imx477.py` around lines 76 - 81, Update the transpose block around the preprocessed tensor to keep the operation on the device, using CuPy moveaxis and contiguous conversion before inference. Replace the host round-trip involving tensor.get() and NumPy moveaxis while preserving the added batch dimension, and validate that the inference backend no longer encounters the reported stride issue.
335-350: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRelease the CUDA primary context in a
finallyblock.If
application.run()raises, the code skipscuDevicePrimaryCtxRelease. Process exit reclaims the context, so the impact is limited to this example script. Nesting the release in its owntry/finallymakes the cleanup explicit.♻️ Proposed refactor
hololink = hololink_channel.hololink() hololink.start() try: hololink.reset() camera.configure() # IMX477 Analog gain settings function. Analog gain value range is 0-1023 in decimal (10 bits). Users are free to experiment with the register values. camera.set_analog_gain(0x2FF) camera.set_exposure_reg(args.exposure) application.run() finally: hololink.stop() - - (cu_result,) = cuda.cuDevicePrimaryCtxRelease(cu_device) - assert cu_result == cuda.CUresult.CUDA_SUCCESS + (cu_result,) = cuda.cuDevicePrimaryCtxRelease(cu_device) + assert cu_result == cuda.CUresult.CUDA_SUCCESS🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/depth_anything_v2/linux_depth_anything_v2_imx477.py` around lines 335 - 350, Move the cuda.cuDevicePrimaryCtxRelease call into a finally block that wraps the existing hololink startup, camera configuration, and application.run flow, ensuring primary-context cleanup occurs even when application.run or earlier setup raises; preserve the existing CUDA success assertion.examples/depth_anything_v2/depth.py (1)
151-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the split position from the image shape.
self.image_dimhardcodes 518 and duplicatespreprocessor.resize_widthindepth.yaml. If a user changes the resize width, the interactive split position no longer matches the frame. Use the actual image width instead.♻️ Proposed refactor
- pos = int(self.image_dim * self.ratio) + pos = int(image.shape[1] * self.ratio)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/depth_anything_v2/depth.py` at line 151, Update the split-position calculation around self.image_dim in the interactive depth display to derive the width from the current image/frame shape instead of the hardcoded dimension, while preserving the existing ratio behavior so changes to the configured resize width remain aligned with the frame.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user_guide/sensor_bridge_hardware_setup.mdx`:
- Line 124: Update the Microchip MPF200-ETH-SENSOR-BRIDGE-R2 link in the
hardware setup documentation to target the dedicated R2 product page instead of
the generic Ethernet Sensor Bridge page, keeping the existing link label
unchanged.
In `@examples/depth_anything_v2/depth.py`:
- Around line 38-46: Initialize self.x and self.framebuffer_size with safe
defaults in __init__, alongside the existing display state. Update
toggle_display_mode and cursor_pos_callback to use these initialized values, and
guard any framebuffer-width division so a zero width cannot raise
ZeroDivisionError while preserving normal coordinate behavior.
- Around line 308-311: Update the default config path in the args.config ==
"none" branch to reference the shipped depth.yaml filename instead of
depth_anything_v2.yaml, while preserving the explicit args.config path
unchanged.
- Around line 136-160: Update the depth_colormap handling in both the
“side-by-side” and interactive branches of the display method to convert the
NumPy result from cv2.applyColorMap into a CuPy array before passing it to
cp.hstack. Preserve the existing slicing and concatenation behavior, and apply
the appropriate image format or explicit channel conversion if the displayed
output requires RGB ordering.
In `@examples/single_network_multi_imx477_player.py`:
- Around line 281-286: Restrict the --num-cameras argument to values 1 through 4
by adding choices=range(1, 5) in the argument definitions for
examples/single_network_multi_imx477_player.py:281-286 and
examples/linux_single_network_multi_imx477_player.py:263-268. No other changes
are required.
In `@src/hololink/core/enumerator.cpp`:
- Around line 166-167: Update the branch assigning MICROCHIP_POLARFIRE_R2_UUID
so it uses a distinct R2 discriminator rather than repeating
MICROCHIP_POLARFIRE_BOARD_ID; use the R2-specific board ID or derive the
revision from BOOTP-v1 metadata, ensuring standard boards retain
MICROCHIP_POLARFIRE_UUID and the two-sensor strategy.
---
Nitpick comments:
In `@examples/depth_anything_v2/depth.py`:
- Line 151: Update the split-position calculation around self.image_dim in the
interactive depth display to derive the width from the current image/frame shape
instead of the hardcoded dimension, while preserving the existing ratio behavior
so changes to the configured resize width remain aligned with the frame.
In `@examples/depth_anything_v2/linux_depth_anything_v2_imx477.py`:
- Around line 76-81: Update the transpose block around the preprocessed tensor
to keep the operation on the device, using CuPy moveaxis and contiguous
conversion before inference. Replace the host round-trip involving tensor.get()
and NumPy moveaxis while preserving the added batch dimension, and validate that
the inference backend no longer encounters the reported stride issue.
- Around line 335-350: Move the cuda.cuDevicePrimaryCtxRelease call into a
finally block that wraps the existing hololink startup, camera configuration,
and application.run flow, ensuring primary-context cleanup occurs even when
application.run or earlier setup raises; preserve the existing CUDA success
assertion.
In `@examples/single_network_multi_imx477_player.py`:
- Around line 304-310: Remove the local Enumerator.set_uuid_strategy call and
its BasicEnumerationStrategy setup in
examples/single_network_multi_imx477_player.py:304-310 and
examples/linux_single_network_multi_imx477_player.py:287-293. Preserve the core
R2 registration, including its board_description and gpio_pin_count metadata,
and rely on its existing four-sensor, two-dataplane, one-SIF-per-sensor
configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4780379-4442-4fd8-9f1b-c24ea894deb6
⛔ Files ignored due to path filters (2)
docs/user_guide/imx477_connect.pngis excluded by!**/*.pngdocs/user_guide/microchip_board.pngis excluded by!**/*.png
📒 Files selected for processing (13)
docs/user_guide/examples.mdxdocs/user_guide/sensor_bridge_firmware_setup.mdxdocs/user_guide/sensor_bridge_hardware_setup.mdxexamples/depth_anything_v2/depth.pyexamples/depth_anything_v2/depth.yamlexamples/depth_anything_v2/linux_depth_anything_v2_imx477.pyexamples/linux_single_network_multi_imx477_player.pyexamples/single_network_multi_imx477_player.pypython/hololink/sensors/imx477.pysrc/hololink/core/enumerator.cppsrc/hololink/core/enumerator.hpptools/hsb_flasher/firmware_flash_strategies/hsb_polarfire.pytools/hsb_flasher/firmware_information/hsb_polarfire_r2.yaml
yosilv
approved these changes
Sep 8, 2026
LianVictor
force-pushed
the
MCHP-Rev2-update
branch
from
September 8, 2026 23:51
1c62ed3 to
234c46b
Compare
LianVictor
force-pushed
the
MCHP-Rev2-update
branch
from
September 8, 2026 23:57
234c46b to
0c70b4d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated:
Additions:
Summary by CodeRabbit
New Features
Documentation
Improvements