SPI and example update for 2.5 EA #70
Conversation
MPF ESB 2.5 release Update: - body_pose_estimation now uses original onnx files - updated flash tool (polarfire_esb) to use custom manifest yaml - updated MPF200-ETH-SENSOR Readme - manifest generation script can now be used to generate mchp yamls Fix: -proper display dimensions for dual camera script Signed-off-by: Victor Lian <victor.lian@microchip.com>
MPF ESB 2.5 release Update: - body_pose_estimation now uses original onnx files - updated flash tool (polarfire_esb) to use custom manifest yaml - updated MPF200-ETH-SENSOR Readme - manifest generation script can now be used to generate mchp yamls Fix: -proper display dimensions for dual camera script Signed-off-by: Victor Lian <victor.lian@microchip.com>
WalkthroughIntroduces manifest-driven FPGA SPI flashing and manifest generation, refactors Python tools to use a YAML manifest for downloads/verification, updates an example visualizer call, and performs widespread RTL restructuring: clock/reset routing, sensor RX/TX interface counts, and port/wire renames across Hololink and top-level FPGA modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
examples/linux_body_pose_estimation.py (1)
226-231: Update help text to reflect ONNX default.The help text states "TRT engine model" but the default is now an ONNX model. This inconsistency may confuse users.
Apply this diff to update the help text:
default_engine = os.path.join(os.path.dirname(__file__), "yolov8n-pose.onnx") parser.add_argument( "--engine", default=default_engine, - help="TRT engine model", + help="Model path (ONNX or TRT engine)", )examples/body_pose_estimation.py (1)
536-541: Update help text to reflect ONNX default.The help text states "TRT engine model" but the default is now an ONNX model. This inconsistency may confuse users.
Apply this diff to update the help text:
default_engine = os.path.join(os.path.dirname(__file__), "yolov8n-pose.onnx") parser.add_argument( "--engine", default=default_engine, - help="TRT engine model", + help="Model path (ONNX or TRT engine)", )examples/linux_body_pose_estimation_imx477.py (1)
211-216: Update help text to reflect ONNX default.The help text states "TRT engine model" but the default is now an ONNX model. This inconsistency may confuse users.
Apply this diff to update the help text:
default_engine = os.path.join(os.path.dirname(__file__), "yolov8n-pose.onnx") parser.add_argument( "--engine", default=default_engine, - help="TRT engine model", + help="Model path (ONNX or TRT engine)", )
🧹 Nitpick comments (4)
MPF200-ETH-SENSOR-BRIDGE_README.txt (1)
28-30: Grammar improvements look good.The corrections improve clarity and grammatical accuracy.
For slightly more concise phrasing, consider this optional change:
-6. Use the new "--force" switch if (new/latest) holoscan ethernet sensor bridge software is not able to detect ethernet packets. This situation arises if FPGA is running older bit file and NVIDIA +6. Use the new "--force" switch if (new/latest) holoscan ethernet sensor bridge software cannot detect ethernet packets. This situation arises if FPGA is running older bit file and NVIDIApython/tools/polarfire_esb.py (3)
52-68: Add context manager for file write operation.Line 61 writes to a file without using a context manager, which could leave the file handle open if an exception occurs.
Apply this diff:
if current_url: r = requests.get(current_url, timeout=300) # parse URL to get last set of / before zip zipname = current_url.split("/")[-1] - open(zipname, "wb").write(r.content) + with open(zipname, "wb") as f: + f.write(r.content) with zipfile.ZipFile(zipname) as zip_ref: zip_ref.extractall(".")Note on MD5 usage: While static analysis flags MD5 as insecure, it's acceptable here for integrity verification (not cryptographic security).
137-171: Consider using context manager for file operations.The file is opened at line 137 and closed at line 171. While this works, using a context manager would be more Pythonic and ensure proper cleanup even if an exception occurs.
- f = open(lfilename, "rb") - content = list(f.read()) + with open(spi_filename, "rb") as f: + content = list(f.read()) con_len = len(content) + START_ADDR tot_len = len(content) _spi_command(in_spi, [0x01, 0]) ... _wait_for_spi_ready(in_spi) - f.close()
275-276: Add comment explaining the delay and use logging.The 45-second sleep appears to be FPGA-specific behavior, but lacks explanation. Also, consider using logging instead of
elif args.program: _spi_program(hololink) + # Wait for FPGA programming to complete and stabilize time.sleep(45) - print("Please power cycle the board to finish programming process") + logging.info("Please power cycle the board to finish programming process")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
MPF200-ETH-SENSOR-BRIDGE_README.txt(1 hunks)examples/body_pose_estimation.py(1 hunks)examples/linux_body_pose_estimation.py(1 hunks)examples/linux_body_pose_estimation_imx477.py(1 hunks)examples/linux_single_network_stereo_imx477_player.py(0 hunks)python/tools/polarfire_esb.py(6 hunks)scripts/generate_manifest.py(5 hunks)scripts/mchp_manifest.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- examples/linux_single_network_stereo_imx477_player.py
🧰 Additional context used
🪛 LanguageTool
MPF200-ETH-SENSOR-BRIDGE_README.txt
[style] ~29-~29: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ...oloscan ethernet sensor bridge software is not able to detect ethernet packets. This situation...
(BE_ABLE_TO)
🪛 Ruff (0.14.5)
python/tools/polarfire_esb.py
66-66: Probable use of insecure hash functions in hashlib: md5
(S324)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
113-113: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
MPF200-ETH-SENSOR-BRIDGE_README.txt (1)
17-21: LGTM! Documentation correctly reflects YAML-driven workflow.The updated flash commands properly reference the new YAML-based configuration approach, and the explanatory text clearly describes the behavior.
python/tools/polarfire_esb.py (4)
26-26: LGTM! Required import for YAML-based configuration.
196-196: LGTM! New metadata field added.The addition of
"block_enable": 0appears to be a required configuration field for the enumeration metadata.
239-244: LGTM! CLI argument properly updated for YAML-based workflow.The new
--yaml-fileargument with a sensible default path aligns with the documented usage in the README.
272-272: LGTM! Function call correctly updated.The call to
_spi_flashproperly passes the YAML configuration file.scripts/generate_manifest.py (3)
93-96: LGTM! New mchp arguments properly defined.The
--mchp-fileand--mchp-urlarguments follow the same pattern as existing FPGA file arguments.Also applies to: 105-108
138-138: LGTM! Variable initialization is consistent.Also applies to: 141-141
195-196: LGTM! MCHP file processing is consistent with other file types.The
measure_fileandmeasure_urlcalls properly add mchp content to the manifest with the correct context.Also applies to: 207-208
scripts/mchp_manifest.yaml (1)
1-16: LGTM! Manifest is well-formed and verified.The YAML structure correctly defines all required fields (content, images, fpga_uuid, strategy) with proper formatting. The download URL is accessible (HTTP 200) and the MD5 hash is correctly formatted for integrity verification.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
python/tools/polarfire_esb.py (1)
52-71: Add error handling for network and file operations.The
download_extractfunction lacks error handling for several failure modes:
requests.get()can fail with network errors, timeouts, or HTTP errors- File writes and zip extraction can fail due to disk space or permissions
- If extraction fails, the downloaded zip file is not cleaned up
Apply this diff to add proper error handling:
def download_extract(current_file_cfg): current_url = current_file_cfg.get("url", None) filename = current_file_cfg.get("filename", None) expected_md5 = current_file_cfg.get("md5", None) if current_url: - r = requests.get(current_url, timeout=300) - # parse URL to get last set of / before zip - zipname = current_url.split("/")[-1] - with open(zipname, "wb") as f: - f.write(r.content) - with zipfile.ZipFile(zipname) as zip_ref: - zip_ref.extractall(".") + try: + r = requests.get(current_url, timeout=300) + r.raise_for_status() + # parse URL to get last set of / before zip + zipname = current_url.split("/")[-1] + with open(zipname, "wb") as f: + f.write(r.content) + with zipfile.ZipFile(zipname) as zip_ref: + zip_ref.extractall(".") + except requests.RequestException as e: + logging.error(f"Failed to download firmware from {current_url}: {e}") + raise + except (IOError, zipfile.BadZipFile) as e: + logging.error(f"Failed to extract firmware: {e}") + if 'zipname' in locals() and os.path.exists(zipname): + os.remove(zipname) + raise + with open(filename, "rb") as file_to_check: data = file_to_check.read() md5_returned = hashlib.md5(data).hexdigest() if md5_returned != expected_md5: raise ValueError( f"MD5 hash mismatch: expected {expected_md5}, got {md5_returned}" )Optional: Consider upgrading MD5 to SHA256.
While MD5 is acceptable for integrity verification (not cryptographic security), SHA256 is more robust and avoids static analysis warnings.
If upgrading to SHA256:
- md5_returned = hashlib.md5(data).hexdigest() - if md5_returned != expected_md5: + sha256_returned = hashlib.sha256(data).hexdigest() + expected_sha256 = current_file_cfg.get("sha256", None) + if sha256_returned != expected_sha256: raise ValueError( - f"MD5 hash mismatch: expected {expected_md5}, got {md5_returned}" + f"SHA256 hash mismatch: expected {expected_sha256}, got {sha256_returned}" )Note: This would require updating the YAML manifest to include SHA256 hashes instead of MD5.
🧹 Nitpick comments (1)
python/tools/polarfire_esb.py (1)
98-128: Good improvement on error handling from previous review.The YAML parsing and validation logic now properly handles errors and provides clear messages. The implementation addresses the concerns raised in past review comments.
Consider enhancing exception logging for better diagnostics.
Static analysis suggests two minor improvements:
- Use
logging.exception()in except blocks to automatically include stack traces- Use exception chaining with
raise ... from errto preserve the original exceptionApply this diff:
except FileNotFoundError: - logging.error(f"YAML configuration file not found: {yaml_path}") + logging.exception(f"YAML configuration file not found: {yaml_path}") raise except yaml.YAMLError as e: - logging.error(f"Failed to parse YAML file: {e}") + logging.exception(f"Failed to parse YAML file: {e}") raise try: images = default_config["hololink"]["images"] if not images: raise ValueError("No images defined in YAML manifest") fname = images[0]["content"] current_file_cfg = default_config["hololink"]["content"][fname] filename = current_file_cfg.get("filename", None) current_url = current_file_cfg.get("url", None) if current_url: zipname = current_url.split("/")[-1] archive_filename = zipname else: archive_filename = None spi_filename = filename except (KeyError, IndexError) as e: - logging.error(f"Invalid YAML structure: missing required key {e}") - raise ValueError(f"YAML manifest is missing required fields: {e}") + logging.exception(f"Invalid YAML structure: missing required key {e}") + raise ValueError(f"YAML manifest is missing required fields: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MPF200-ETH-SENSOR-BRIDGE_README.txt(1 hunks)examples/body_pose_estimation.py(1 hunks)examples/linux_body_pose_estimation.py(1 hunks)examples/linux_body_pose_estimation_imx477.py(1 hunks)python/tools/polarfire_esb.py(8 hunks)scripts/generate_manifest.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/linux_body_pose_estimation.py
- examples/linux_body_pose_estimation_imx477.py
- scripts/generate_manifest.py
🧰 Additional context used
🪛 Ruff (0.14.5)
python/tools/polarfire_esb.py
67-67: Probable use of insecure hash functions in hashlib: md5
(S324)
69-71: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
108-108: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
examples/body_pose_estimation.py (1)
536-541: ONNX default and help text look consistent with the inference pipelineSwitching the default
--enginepath toyolov8n-pose.onnxand clarifying the help to “Model path (ONNX or TRT engine)” aligns with usingargs.enginedirectly inmodel_path_map. No functional issues here as long asyolov8n-pose.onnxis shipped alongside the example.python/tools/polarfire_esb.py (3)
186-194: Cleanup logic improved with proper logging.The file cleanup section now uses descriptive variable names and includes warning logs when files don't exist, addressing concerns from previous reviews.
249-255: LGTM! CLI argument refactored correctly.The change from
--fpga-bit-versionto--yaml-filewith a sensible default aligns with the YAML-driven configuration approach.
286-288: Verify the 45-second delay is appropriate.The hardcoded 45-second sleep after FPGA programming appears arbitrary. Please confirm this duration is based on hardware requirements or empirical testing.
If this is based on hardware specifications, consider adding a comment explaining the requirement. If it's an estimate, consider making it configurable or documenting the rationale.
MPF200-ETH-SENSOR-BRIDGE_README.txt (2)
17-21: LGTM! Documentation correctly reflects YAML-driven workflow.The updated commands accurately describe the new
--yaml-fileargument and clarify when the--forceswitch is needed for older FPGA bit file versions.
28-30: Good clarity improvement.The enhanced explanation of the
--forceswitch makes it clearer when users should apply this option, improving the user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v (1)
20-30: Macro definitions duplicated from HOLOLINK_def.svh.These macros (
SENSOR_RX_IF_INST,HOST_IF_INST,DATAPATH_WIDTH, etc.) are already defined inHOLOLINK_def.svh. Consider including the header file instead of redefining them here to maintain a single source of truth and avoid potential inconsistencies.-`define SENSOR_RX_IF_INST 2 -`define HOST_IF_INST 2 -`define DATAPATH_WIDTH 64 -`define DATAKEEP_WIDTH `DATAPATH_WIDTH/8 -`define DATAUSER_WIDTH 1 -`define HOST_WIDTH 64 -`define HOSTKEEP_WIDTH 8 -`define HOSTUSER_WIDTH 1 -`define SPI_INST 1 -`define I2C_INST 3 -`define GPIO_INST 16 +`include "HOLOLINK_def.svh" +import HOLOLINK_pkg::*;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_def.svh(2 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v(6 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HoloLink_SD.v(3 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_BaseR.v(6 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_Hololink_top.v(17 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/top/PF_ESB_top.v(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HoloLink_SD.v
🔇 Additional comments (15)
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_BaseR.v (2)
105-105: Internal PRESETN routing looks correct.The COREABC module now generates PRESETN internally (line 347), which is routed to the CORE10GMAC instance (line 280). This centralizes reset control within the MAC_BaseR block rather than exposing it as a port, aligning with the broader clock/reset topology refactoring in this PR.
Also applies to: 280-280, 347-347
142-142: Standard SmartDesign clock net routing.The additional intermediate net
LANE0_RX_CLK_R_net_1follows SmartDesign's naming conventions for hierarchical net routing. Synthesis tools will optimize these redundant assignments.Also applies to: 204-205
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v (2)
200-200: Verify: SIF RX tready signal disconnected.The
i_sif_axis_treadyinput is commented out. Without this connection, the SIF RX path cannot receive ready signaling, which may cause data loss if downstream modules cannot consume data at the input rate. If this is intentional for bring-up, consider documenting it with a TODO comment.
224-230: SIF TX interface intentionally disabled.The commented-out SIF TX axis signals are consistent with the "Unimplemented" comment on lines 72-78. No concerns here.
fpga/microchip/mfp200-eth-sensor-bridge/rtl/top/PF_ESB_top.v (3)
232-237: CAM1_CLK and CAM2_CLK now share the same clock source.Both camera clocks are now driven from the same net (
CAM1_CLK_net_0). This is a significant change from having separate clock paths. Verify this is the intended behavior for your dual-camera configuration.
279-279: PLL_LOCK source consolidated to MAC_Hololink.The reset controller now uses
MAC_Hololink_0_PLL_LOCK_0for PLL lock indication, centralizing clock management. This is consistent with the removal of PF_CCC_C3.
354-357: SIF and new output port wiring looks correct.The SIF_TARGET signals are correctly mapped to their respective IMX477_IF_TOP instances. The new outputs (pps_stretch_o, OUT3_FABCLK_0, PLL_LOCK_0) are properly routed from MAC_Hololink_0.
Also applies to: 378-380
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_Hololink_top.v (4)
270-277: CAM1_EN, CAM1_TRNG_RSTN, CAM2_EN, CAM2_TRNG_RSTN all driven from same signal.All four camera control signals are now driven from the same source (
CAM2_TRNG_RSTN_net_0), which originates fromo_init_done. This ties both cameras' enable and training reset signals together. Verify this is the intended behavior for your dual-camera setup.
288-289: LED1 now shows PPS stretch indicator.LED1 is driven by the same signal as pps_stretch_o, providing visual indication of PPS activity. This is a reasonable debug/status indicator choice.
538-545: PF_CCC_C1 now uses recovered RX clock as reference.The PLL reference clock (
REF_CLK_0) is now driven byLANE0_RX_CLK_R, the recovered clock from MAC_BaseR_0's 10G transceiver. This creates a clock domain relationship with the line rate, which is appropriate for synchronous operation.Concern: If the Ethernet link is down (no valid RX clock), the PLL may lose lock and system clocks (OUT0-OUT3) will be unstable. Verify that the system handles the link-down scenario gracefully, particularly for camera and reset logic that depends on these clocks.
525-525: MAC_BaseR_1 LANE0_RX_CLK_R left unconnected.The second MAC instance's recovered RX clock is not used, while MAC_BaseR_0's clock drives the system PLL. This asymmetric design means the system clocking depends only on the first Ethernet link. Ensure this is acceptable for your use case.
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_def.svh (4)
28-28: Verify PTP_CLK_FREQ value.The PTP clock frequency is set to 104,161,000 Hz (approximately 104.161 MHz). This is an unusual frequency. Please verify this matches your actual PTP clock source configuration in the PLL settings.
33-42: UUID and EEPROM configuration well-structured.The UUID provides unique board identification for BOOTP, and the conditional EEPROM configuration allows flexibility between hardware EEPROM and soft configuration.
64-80: Sensor RX interface configuration is well-documented.The per-interface packetizer parameters are clearly defined with helpful comments explaining the array indexing. Packetizers are currently disabled (
SIF_RX_PACKETIZER_EN = {0, 0}).
128-136: Initialization sequence significantly reduced.
N_INIT_REGis reduced to 1, with only the I2C timeout configuration remaining. The comment indicates MAC/PCS register writes should be added here. Verify that any previously required MAC/PCS initialization is now handled elsewhere or is no longer needed.
| // Peripheral IF | ||
| //------------------------------------------------------------------------------ | ||
| // SPI Interface, QSPI compatible | ||
| // SPI Interface, QSPI compatable |
There was a problem hiding this comment.
Typo: "compatable" should be "compatible".
Minor typo in the SPI interface comment.
- // SPI Interface, QSPI compatable
+ // SPI Interface, QSPI compatible📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SPI Interface, QSPI compatable | |
| // SPI Interface, QSPI compatible |
🤖 Prompt for AI Agents
In
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v
around line 118, there's a comment typo: change "QSPI compatable" to "QSPI
compatible" so the SPI interface comment reads correctly; update the comment
text accordingly.
There was a problem hiding this comment.
Note that all code submissions must pass lint checking, which you can verify locally by running sh ./ci/lint.sh --format -- which points out this misspelling here.
There was a problem hiding this comment.
@p-ogrady, thank you for confirming and for pointing out the lint script! Running sh ./ci/lint.sh --format locally is indeed the best way to catch and auto-fix issues like this before submission.
✏️ Learnings added
Learnt from: p-ogrady
Repo: nvidia-holoscan/holoscan-sensor-bridge PR: 70
File: fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v:118-118
Timestamp: 2025-12-30T18:50:14.830Z
Learning: Applies to **/*.{v,sv,vh,svh} : All Verilog and SystemVerilog files must be formatted according to the rules in `ci/lint.sh` (can be auto-formatted using `ci/lint.sh --format`)
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: nvidia-holoscan/holoscan-sensor-bridge PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-26T00:20:33.480Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp} : All C++ source code must be formatted according to the rules in `ci/lint.sh` (can be auto-formatted using `ci/lint.sh --format`)
Learnt from: CR
Repo: nvidia-holoscan/holoscan-sensor-bridge PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-26T00:20:33.480Z
Learning: Applies to **/*.md : All Markdown files must be formatted according to the rules in `ci/lint.sh` (can be auto-formatted using `ci/lint.sh --format`)
Learnt from: CR
Repo: nvidia-holoscan/holoscan-sensor-bridge PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-26T00:20:33.480Z
Learning: Applies to **/*.py : All Python source code must be formatted according to the rules in `ci/lint.sh` (can be auto-formatted using `ci/lint.sh --format`)
Signed-off-by: Victor Lian <victor.lian@microchip.com>
Updates for stability and reliability.
Updates:
- RTL code for the mfp200-eth-sensor-bridge
Fixes:
- Changed body pose estimation demos to use binaries generated by TRT
rather than original onnx
Signed-off-by: Victor Lian <victor.lian@microchip.com>
Signed-off-by: LianVictor <victor.lian@microchip.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/tools/polarfire_esb.py (1)
128-150: Handle exceptions fromdownload_extractto provide clear error messages.The call to
download_extract(line 128) can raise multiple exceptions (requests.RequestException,OSError,ValueError,zipfile.BadZipFile) that aren't caught. If the download or verification fails, line 149 will attempt to open a non-existent file, resulting in a confusing error.Apply this diff to add error handling:
- download_extract(current_file_cfg) + try: + download_extract(current_file_cfg) + except requests.RequestException as e: + logging.error(f"Failed to download SPI image: {e}") + raise + except ValueError as e: + logging.error(f"MD5 verification failed: {e}") + raise + except Exception as e: + logging.error(f"Failed to prepare SPI image: {e}") + raise + hsb_ip_version = channel_metadata["hsb_ip_version"]
♻️ Duplicate comments (1)
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v (1)
118-118: Typo: "compatable" should be "compatible".Minor typo in the SPI interface comment.
- // SPI Interface, QSPI compatable + // SPI Interface, QSPI compatible
🧹 Nitpick comments (3)
scripts/generate_manifest.py (1)
93-96: LGTM! Consider consistent argument ordering.The new
--mchp-fileand--mchp-urlarguments are correctly implemented and follow the existing patterns. The help text is clear and consistent.For improved code organization, consider ordering the URL arguments to match the file arguments (cpnx, clnx, stratix, mchp), which would place
--mchp-urlafter--stratix-urlrather than before--clnx-url.Also applies to: 105-108
python/tools/polarfire_esb.py (2)
249-255: Consider minor help text improvement for consistency.The help text on line 253 says "Defaulted to" which is slightly informal. Consider "Defaults to" for consistency with standard argparse conventions.
Apply this diff:
"--yaml-file", type=str, nargs="?", - help="Yaml configuration file to load. Defaulted to: scripts/mchp_manifest.yaml", + help="YAML configuration file to load (default: scripts/mchp_manifest.yaml)", default="scripts/mchp_manifest.yaml",
125-127: Consider using exception chaining for better error context.When re-raising exceptions in line 127, consider using
raise ... from eto preserve the original exception chain, which aids debugging.Apply this diff:
except (KeyError, IndexError) as e: logging.error(f"Invalid YAML structure: missing required key {e}") - raise ValueError(f"YAML manifest is missing required fields: {e}") + raise ValueError(f"YAML manifest is missing required fields: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
MPF200-ETH-SENSOR-BRIDGE_README.txt(1 hunks)examples/linux_single_network_stereo_imx477_player.py(0 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_def.svh(2 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v(6 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HoloLink_SD.v(3 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_BaseR.v(6 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_Hololink_top.v(17 hunks)fpga/microchip/mfp200-eth-sensor-bridge/rtl/top/PF_ESB_top.v(7 hunks)python/tools/polarfire_esb.py(8 hunks)scripts/generate_manifest.py(5 hunks)
💤 Files with no reviewable changes (1)
- examples/linux_single_network_stereo_imx477_player.py
🚧 Files skipped from review as they are similar to previous changes (2)
- MPF200-ETH-SENSOR-BRIDGE_README.txt
- fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HoloLink_SD.v
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python source code must be formatted according to the rules in
ci/lint.sh(can be auto-formatted usingci/lint.sh --format)
Files:
scripts/generate_manifest.pypython/tools/polarfire_esb.py
🪛 Ruff (0.14.8)
python/tools/polarfire_esb.py
67-67: Probable use of insecure hash functions in hashlib: md5
(S324)
69-71: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
108-108: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (19)
scripts/generate_manifest.py (2)
138-138: LGTM! Validation correctly includes mchp options.The variable assignments and validation logic correctly incorporate
mchp_fileandmchp_url. The error message properly lists all options including the mchp variants, addressing the previous review feedback.Also applies to: 141-141, 148-149
195-196: LGTM! MCHP file processing implemented correctly.The
mchp_fileprocessing follows the established pattern and correctly passes the "mchp" context tomeasure_file.fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_BaseR.v (2)
105-105: PRESETN routing consolidation looks correct.The internal
COREABC_C0_0_PRESETNwire correctly connects thePRESETNoutput fromCOREABC_C0_0(Line 347) to thePRESETNinput ofCORE10GMAC_C0_0(Line 280). This change moves the reset control from an external port to the internal ABC controller, which is a valid architectural pattern for initialization sequencing.Also applies to: 280-280, 347-347
204-205: Clock routing through intermediate net is standard SmartDesign practice.The two-stage assignment for
LANE0_RX_CLK_R(_net_0→_net_1→ output) follows SmartDesign's convention for clean signal propagation. No functional concern.fpga/microchip/mfp200-eth-sensor-bridge/rtl/top/PF_ESB_top.v (2)
232-237: CAM1_CLK and CAM2_CLK now share the same clock source.Both camera clock outputs are now driven from
CAM1_CLK_net_0, which originates fromMAC_Hololink_0.OUT3_FABCLK_0. This consolidation removes the need for a separatePF_CCC_C3block. Verify this is the intended behavior for synchronized camera operation.
354-357: SIF and clock interface rewiring aligns with RX-centric topology.The SIF target valid signals are correctly mapped to their respective
IMX477_IF_TOPinstances. The newOUT3_FABCLK_0andPLL_LOCK_0outputs fromMAC_Hololink_0properly expose the camera clock and PLL lock status at the top level.Also applies to: 378-380
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_top_wrapper.v (2)
193-200: RX clock duplication and disabled tready signal.The
i_sif_rx_clkinput is duplicated ({i_sif_clk, i_sif_clk}) to feed both RX interfaces with the same clock, which is correct for synchronized sensor inputs.However,
i_sif_axis_treadyis commented out (Line 200). If the downstreamHOLOLINK_topmodule requires this signal for flow control, leaving it disconnected could cause data loss under backpressure conditions. Confirm this is intentional and that the SIF RX path operates without backpressure.
137-137: Sensor interface arrays now use RX-specific count.The
o_sw_sen_rstport width and internal bus arrays are now indexed bySENSOR_RX_IF_INSTinstead of a genericSENSOR_IF_INST. This aligns with the RX-centric interface topology changes inHOLOLINK_def.svh.Also applies to: 149-157
fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/HOLOLINK_def.svh (4)
23-28: PTP clock frequency definition added.
PTP_CLK_FREQis set to 104,161,000 Hz (~104 MHz). Verify this matches the actual clock frequency driving the PTP logic (appears to beOUT2_FABCLK_0from the CCC).
33-42: UUID and EEPROM configuration for board identification.The 128-bit UUID provides unique board identification for BOOTP. The conditional
ENUM_EEPROMblock allows switching between EEPROM-based and soft-coded MAC/serial number configurations, which is good for flexibility.
53-80: RX-specific sensor interface configuration is well-structured.The
SENSOR_RX_IF_INSTblock with per-interface packetizer parameters (SIF_RX_PACKETIZER_EN,SIF_RX_VP_COUNT, etc.) provides clear configuration. Note thatSIF_RX_PACKETIZER_ENis set to{0, 0}, meaning packetizers are disabled for both interfaces.
128-136: Simplified initialization with single I2C timeout register.
N_INIT_REGreduced to 1 with only the I2C timeout entry (0x0300_0210 = 0x004C_4B40). The timeout value of 0x4C4B40 (5,000,000 decimal) cycles at the APB clock frequency (~19.5 MHz) gives approximately 256ms timeout, which appears reasonable for I2C operations.fpga/microchip/mfp200-eth-sensor-bridge/rtl/MAC_Hololink/MAC_Hololink_top.v (4)
270-277: All camera enable/reset signals share the same source.
CAM1_EN,CAM1_TRNG_RSTN,CAM2_EN, andCAM2_TRNG_RSTNare all driven fromCAM2_TRNG_RSTN_net_0, which originates fromHoloLink_SD_0.o_init_done. This means both cameras are enabled/reset synchronously. Verify this is the intended design - if independent camera control is needed, this would need modification.
494-494: CCC reference clock sourced from MAC_BaseR_0 RX clock.
LANE0_RX_CLK_RfromMAC_BaseR_0is used as the reference clock forPF_CCC_C1_0.MAC_BaseR_1'sLANE0_RX_CLK_Routput is intentionally left unconnected, which is correct since only one RX clock is needed for the CCC reference.Also applies to: 525-525, 538-538
45-46: New clock and PLL lock outputs properly exposed.
OUT3_FABCLK_0andPLL_LOCK_0are correctly wired fromPF_CCC_C1_0through intermediate nets to the module outputs. These signals enable the top-level to use the generated camera clock and monitor PLL lock status.Also applies to: 101-102, 298-301, 540-544
288-289: LED1 and pps_stretch_o share the same signal source.Both
LED1(viapps_stretch_o_net_1) andpps_stretch_o(viapps_stretch_o_net_2) are driven frompps_stretch_o_net_0. This allows the LED to visually indicate PPS activity while also providing the signal for external use.Also applies to: 296-297
python/tools/polarfire_esb.py (3)
98-127: Past review comments successfully addressed.The YAML parsing and error handling implementation correctly addresses all suggestions from the previous review:
- Separate exception handling for
FileNotFoundErrorandyaml.YAMLError- Validation for required YAML keys with descriptive error messages
- Clear variable naming (
archive_filename,spi_filename)- Proper cleanup logic with warning messages
207-207: LGTM: Metadata field addition.The addition of
"block_enable": 0extends the metadata structure appropriately for the manual enumeration path.
286-288: LGTM: Appropriate wait time and clear user instructions.The 45-second delay allows FPGA programming to complete and stabilize, and the power-cycle instruction provides clear guidance to the user.
| def download_extract(current_file_cfg): | ||
| current_url = current_file_cfg.get("url", None) | ||
| filename = current_file_cfg.get("filename", None) | ||
| expected_md5 = current_file_cfg.get("md5", None) | ||
|
|
||
| if current_url: | ||
| r = requests.get(current_url, timeout=300) | ||
| # parse URL to get last set of / before zip | ||
| zipname = current_url.split("/")[-1] | ||
| with open(zipname, "wb") as f: | ||
| f.write(r.content) | ||
| with zipfile.ZipFile(zipname) as zip_ref: | ||
| zip_ref.extractall(".") | ||
| with open(filename_2507, "rb") as file_to_check: | ||
| data = file_to_check.read() | ||
| md5_returned = hashlib.md5(data).hexdigest() | ||
| if md5_returned != expected_md5_2507: | ||
| raise Exception("md5 Hash mismatch") | ||
| with open(filename, "rb") as file_to_check: | ||
| data = file_to_check.read() | ||
| md5_returned = hashlib.md5(data).hexdigest() | ||
| if md5_returned != expected_md5: | ||
| raise ValueError( | ||
| f"MD5 hash mismatch: expected {expected_md5}, got {md5_returned}" | ||
| ) |
There was a problem hiding this comment.
Add error handling and logging for download and extraction operations.
The download_extract function lacks error handling and observability:
- Network failures:
requests.get(line 58) doesn't handle HTTP errors, connection failures, or timeouts gracefully - File I/O failures: File operations (lines 61-62, 65-67) don't handle write/read errors
- ZIP extraction failures:
zip_ref.extractall(line 64) doesn't validate the archive or handle extraction errors - No logging: Users have no visibility into download/extraction progress
- No cleanup on error: If MD5 verification fails (line 69), invalid files remain on disk
Apply this diff to add error handling and logging:
def download_extract(current_file_cfg):
current_url = current_file_cfg.get("url", None)
filename = current_file_cfg.get("filename", None)
expected_md5 = current_file_cfg.get("md5", None)
if current_url:
- r = requests.get(current_url, timeout=300)
- # parse URL to get last set of / before zip
- zipname = current_url.split("/")[-1]
- with open(zipname, "wb") as f:
- f.write(r.content)
- with zipfile.ZipFile(zipname) as zip_ref:
- zip_ref.extractall(".")
- with open(filename, "rb") as file_to_check:
- data = file_to_check.read()
- md5_returned = hashlib.md5(data).hexdigest()
+ try:
+ logging.info(f"Downloading from {current_url}")
+ r = requests.get(current_url, timeout=300)
+ r.raise_for_status()
+ zipname = current_url.split("/")[-1]
+ logging.info(f"Saving to {zipname}")
+ with open(zipname, "wb") as f:
+ f.write(r.content)
+ logging.info(f"Extracting {zipname}")
+ with zipfile.ZipFile(zipname) as zip_ref:
+ zip_ref.extractall(".")
+ except requests.RequestException as e:
+ logging.error(f"Failed to download from {current_url}: {e}")
+ raise
+ except (OSError, zipfile.BadZipFile) as e:
+ logging.error(f"Failed to extract {zipname}: {e}")
+ raise
+
+ try:
+ logging.info(f"Verifying MD5 for {filename}")
+ with open(filename, "rb") as file_to_check:
+ data = file_to_check.read()
+ md5_returned = hashlib.md5(data).hexdigest()
+ except OSError as e:
+ logging.error(f"Failed to read {filename}: {e}")
+ raise
+
if md5_returned != expected_md5:
+ logging.error(f"MD5 mismatch for {filename}: expected {expected_md5}, got {md5_returned}")
+ # Clean up invalid files
+ if os.path.exists(filename):
+ os.remove(filename)
+ if current_url:
+ zipname = current_url.split("/")[-1]
+ if os.path.exists(zipname):
+ os.remove(zipname)
raise ValueError(
f"MD5 hash mismatch: expected {expected_md5}, got {md5_returned}"
)
+ logging.info(f"MD5 verification passed for {filename}")🧰 Tools
🪛 Ruff (0.14.8)
67-67: Probable use of insecure hash functions in hashlib: md5
(S324)
69-71: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In python/tools/polarfire_esb.py around lines 52 to 71, add robust error
handling, logging, and cleanup around the download/extract/verify flow: use the
logging module to emit progress and error messages; wrap requests.get in
try/except, call response.raise_for_status(), and handle timeout/connection
errors; write the downloaded bytes to a temporary file (or ensure file is closed
in finally) and wrap file I/O in try/except to catch write/read errors; validate
and extract the zip inside a try/except catching zipfile.BadZipFile and other
extraction errors; compute MD5 only after a successful read and on MD5 mismatch
remove the bad file(s) and raise a clear exception; ensure any partial or
temporary files are removed in error paths (use try/finally or context managers)
so no invalid artifacts remain.
There was a problem hiding this comment.
As long as the error conditions described here result in exceptions that halt program execution, the existing implementation is fine-- those original exceptions provide the necessary information for users to understand what went wrong, enabling them to resolve those themselves. Hiding the original exceptions creates a substantial risk for hiding actual causes, usually hindering quick problem resolution.
There was a problem hiding this comment.
@p-ogrady, understood. You make a valid point about preserving the original exceptions—native stack traces from requests, zipfile, and file I/O operations do provide clear diagnostic information, and wrapping them could obscure the root cause. I'll leave the implementation as-is. Thank you for the clarification.
| if mchp_url is not None: | ||
| measure_url(mchp_url, "mchp") |
There was a problem hiding this comment.
Remove trailing whitespace and consider consistent ordering.
Line 208 contains trailing whitespace after "mchp"). Per the coding guidelines, Python code must be formatted according to ci/lint.sh rules.
Additionally, the URL processing order (cpnx→clnx→mchp→stratix) differs from the file processing order (cpnx→clnx→stratix→mchp at lines 189-196). Consider processing mchp_url after stratix_url for consistency.
Apply this diff to fix the trailing whitespace:
if mchp_url is not None:
- measure_url(mchp_url, "mchp")
+ measure_url(mchp_url, "mchp")For consistency, consider moving the mchp_url processing block after stratix_url processing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if mchp_url is not None: | |
| measure_url(mchp_url, "mchp") | |
| if mchp_url is not None: | |
| measure_url(mchp_url, "mchp") |
🤖 Prompt for AI Agents
In scripts/generate_manifest.py around lines 207-208, remove the trailing
whitespace after "mchp") on line 208 and, for consistent URL processing order,
move the if-block that calls measure_url(mchp_url, "mchp") so it runs after the
stratix_url handling (i.e., reorder the mchp_url processing to follow the
stratix_url block), keeping the same conditional and call but without any
trailing spaces.
yosilv
left a comment
yosilv
left a comment
There was a problem hiding this comment.
@LianVictor please address comments from code rabbit as first stage of review.
Thanks!
| c. polarfire_esb flash --fpga-bit-version 2506 | ||
| c.1 The above command flashes(Transfers to on board SPI flash) 2506 version bit file. | ||
| d. polarfire_esb --force flash --fpga-bit-version 2506 | ||
| d.1 The above command flashes(Transfers to on board SPI flash) 2506 version bit file. |
There was a problem hiding this comment.
"polarfire_esb flash ..." is inconsistent with "polarfire_esb --program" -- consider changing it to either "polarfire_esb --flash ..." or "polarfire_esb program ...".
Also, consider that users will follow your directions one step at a time. Instructions 3c and 3d do the same thing, with reasoning on which to run only expressed after the commands (in 3.d.2). As written, users are likely to run both commands. Putting the decision first and using that to guide users to the specific command they need encourages users to do the right thing without requiring them to look ahead. Better yet, demonstrate use of "holoscan-enumerate" to inform users of what FPGA version they currently have, and use the results from there as the decision.
| framebuffer_srgb=True, | ||
| tensors=[left_spec, right_spec], | ||
| height=1920, | ||
| width=1080, |
There was a problem hiding this comment.
I just ran this demonstration, after downloading the firmware via polarfire_esb flash --yaml-file scripts/mchp_manifest.yaml, and power cycling the MPF200 unit, I observed this-- note the momentary distortion in the video.
IMG_3078.zip
| // Peripheral IF | ||
| //------------------------------------------------------------------------------ | ||
| // SPI Interface, QSPI compatible | ||
| // SPI Interface, QSPI compatable |
There was a problem hiding this comment.
Note that all code submissions must pass lint checking, which you can verify locally by running sh ./ci/lint.sh --format -- which points out this misspelling here.
| def download_extract(current_file_cfg): | ||
| current_url = current_file_cfg.get("url", None) | ||
| filename = current_file_cfg.get("filename", None) | ||
| expected_md5 = current_file_cfg.get("md5", None) | ||
|
|
||
| if current_url: | ||
| r = requests.get(current_url, timeout=300) | ||
| # parse URL to get last set of / before zip | ||
| zipname = current_url.split("/")[-1] | ||
| with open(zipname, "wb") as f: | ||
| f.write(r.content) | ||
| with zipfile.ZipFile(zipname) as zip_ref: | ||
| zip_ref.extractall(".") | ||
| with open(filename_2507, "rb") as file_to_check: | ||
| data = file_to_check.read() | ||
| md5_returned = hashlib.md5(data).hexdigest() | ||
| if md5_returned != expected_md5_2507: | ||
| raise Exception("md5 Hash mismatch") | ||
| with open(filename, "rb") as file_to_check: | ||
| data = file_to_check.read() | ||
| md5_returned = hashlib.md5(data).hexdigest() | ||
| if md5_returned != expected_md5: | ||
| raise ValueError( | ||
| f"MD5 hash mismatch: expected {expected_md5}, got {md5_returned}" | ||
| ) |
There was a problem hiding this comment.
As long as the error conditions described here result in exceptions that halt program execution, the existing implementation is fine-- those original exceptions provide the necessary information for users to understand what went wrong, enabling them to resolve those themselves. Hiding the original exceptions creates a substantial risk for hiding actual causes, usually hindering quick problem resolution.
3 participants
Summary by CodeRabbit
Documentation
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.