Realsense D555 linux receiver demos - #80
Conversation
WalkthroughAdds RealSense D555 support: three Holoscan example apps, a CUDA image-decoder operator with Python bindings, a D555 sensor driver and mode definitions, and updates DataChannel and receiver operators to accept an optional UDP port for socket configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 10
🤖 Fix all issues with AI agents
In @docs/user_guide/examples.md:
- Around line 118-123: The fenced code block that shows the hololink-enumerate
output is missing a language specifier; update the triple-backtick opening fence
for the block containing the hololink-enumerate output (the example showing
mac_id=98:4F:EE:1A:F4:A9 ... and ^C) to include a language such as text or
console (e.g., change ``` to ```text) so the markdown complies with lint rules.
In @examples/linux_d555_dual_stream.py:
- Line 19: The file imports the unused module name "ctypes" causing a Flake8
warning; remove the unused import line (the standalone "import ctypes") from
examples/linux_d555_dual_stream.py so the module is no longer imported but
otherwise leave the rest of the file unchanged.
- Line 1: Fix the typos in the file license header: change "FileCopystream2Text"
to "FileCopyrightText" and replace "stream2s reserved" with "rights reserved" so
the header reads correctly (keep the SPDX prefix and existing copyright holder
text intact).
- Line 268: Fix the incorrect indentation for the lone comment "#" in
examples/linux_d555_dual_stream.py (around the block containing that comment) so
it uses an indentation level that is a multiple of 4 and matches the surrounding
block scope (either align to the enclosing block's indentation or move to column
0 if top-level) to satisfy Flake8.
In @python/hololink/operators/image_decoder/image_decoder.cpp:
- Around line 57-70: The constructor PyImageDecoder accepts align_depth_to_rgb
but ImageDecoder::setup() never declares or store it; update
ImageDecoder::setup() to call spec.param("align_depth_to_rgb", ...) to declare
the parameter, add a member variable (e.g., bool align_depth_to_rgb_) to
ImageDecoder to store the value, read the declared parameter into that member
during setup, and then modify the operator's compute logic (methods that perform
decoding/resizing/alignment) to respect align_depth_to_rgb_ when aligning depth
to RGB; alternatively remove align_depth_to_rgb from PyImageDecoder if you
prefer not to support it.
In @python/hololink/sensors/d555/d555.py:
- Line 137: The buffer write_bytes in set_register is a fixed 4 bytes and can
overflow if reg_size and val_size are both BITS_32; change allocation to compute
total_bytes from reg_size and val_size (convert bits to bytes) and allocate
bytearray(total_bytes) instead of bytearray(4), add a defensive check to
validate total_bytes is within expected limits (and raise/handle if too large),
and update any code that writes into write_bytes to use the computed size;
reference symbols: set_register, write_bytes, reg_size, val_size, BITS_16,
BITS_32.
In @python/hololink/sensors/d555/model.py:
- Around line 1-10: Add the missing Apache-2.0 SPDX license header at the top of
the file so it matches other files in the repo; update
python/hololink/sensors/d555/model.py by inserting the standard single-line SPDX
identifier and full Apache 2.0 header comment before any imports (above the
"from enum import Enum" line) so the file containing the Endianness and
DataWidth Enum definitions includes the required license declaration.
In @src/hololink/core/data_channel.cpp:
- Around line 363-366: The comment above the sockaddr_in setup is stale: it says
we use bind(local_ip,0) to let the kernel pick a port, but the code sets
address.sin_port = htons(udp_port) so we may bind to a specific port (if
udp_port != 0) or let the kernel assign one (if udp_port == 0). Update the
comment near the sockaddr_in/address and address.sin_port = htons(udp_port)
lines to state that we bind to udp_port (which can be zero to let the kernel
choose a port) rather than always using 0.
In @src/hololink/operators/image_decoder/image_decoder.cpp:
- Around line 291-294: The YUYV branch is missing a check of
stream_handler_result after calling
cuda_stream_handler_.to_message(out_message); update the YUYV path to mirror the
Z16 handling by verifying stream_handler_result (and out_message.has_error() if
applicable) before constructing holoscan::gxf::Entity and calling output.emit;
on failure, log or report the error and return without emitting. Ensure you
reference cuda_stream_handler_.to_message, stream_handler_result, out_message,
holoscan::gxf::Entity, and output.emit when adding the guard and error handling.
🧹 Nitpick comments (16)
examples/linux_d555_dual_stream.py (1)
119-119: Consider extracting magic number to a named constant.The UDP port base
54739is hardcoded. Consider defining it as a named constant for clarity and maintainability.Proposed refactor
+# Base UDP port for RealSense streams +REALSENSE_UDP_PORT_BASE = 54739 + class HoloscanApplication(holoscan.core.Application): ... receiver_operator_stream1 = hololink_module.operators.LinuxReceiverOperator( ... - udp_port=54739 + hololink_module.sensors.d555.d555_mode.RealSense_StreamId.DEPTH.value, + udp_port=REALSENSE_UDP_PORT_BASE + hololink_module.sensors.d555.d555_mode.RealSense_StreamId.DEPTH.value,python/hololink/sensors/d555/__init__.py (1)
1-1: Consider updating the copyright year for consistency.The copyright year is "2023" while other files in this PR use "2023-2025". For consistency with the rest of the codebase, consider updating to match the pattern used in other files.
📅 Suggested fix
-# SPDX-FileCopyrightText: Copyright (c) 2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2023-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.examples/linux_d555_player.py (4)
19-19: Remove unused import.The
ctypesimport is unused since theBlockMemoryPoolcode that used it (lines 77-86) is commented out. Per static analysis hint (F401).Proposed fix
-import ctypes
77-87: Clean up commented-out code.The commented-out
BlockMemoryPoolcode should be removed if no longer needed, or restored if it provides better performance. Leaving dead code in production makes maintenance harder.
182-188: Consider proper error handling instead ofassertfor CUDA operations.Using
assertfor CUDA error checking can be problematic since assertions can be disabled with Python's-Oflag, silently masking failures. Consider raising explicit exceptions.Proposed fix
- (cu_result,) = cuda.cuInit(0) - assert cu_result == cuda.CUresult.CUDA_SUCCESS + (cu_result,) = cuda.cuInit(0) + if cu_result != cuda.CUresult.CUDA_SUCCESS: + raise RuntimeError(f"cuInit failed: {cu_result}") cu_device_ordinal = 0 - cu_result, cu_device = cuda.cuDeviceGet(cu_device_ordinal) - assert cu_result == cuda.CUresult.CUDA_SUCCESS - cu_result, cu_context = cuda.cuDevicePrimaryCtxRetain(cu_device) - assert cu_result == cuda.CUresult.CUDA_SUCCESS + cu_result, cu_device = cuda.cuDeviceGet(cu_device_ordinal) + if cu_result != cuda.CUresult.CUDA_SUCCESS: + raise RuntimeError(f"cuDeviceGet failed: {cu_result}") + cu_result, cu_context = cuda.cuDevicePrimaryCtxRetain(cu_device) + if cu_result != cuda.CUresult.CUDA_SUCCESS: + raise RuntimeError(f"cuDevicePrimaryCtxRetain failed: {cu_result}")
230-230: Remove or make configurable the debug environment variable.Hardcoding
GXF_MEMORY_DEBUG=1in production code can impact performance. Consider removing this or making it conditional on a debug flag.Proposed fix
- os.environ["GXF_MEMORY_DEBUG"] = "1" + # Uncomment for debugging memory issues: + # os.environ["GXF_MEMORY_DEBUG"] = "1"examples/linux_d555_peoplenet.py (3)
106-106: Potential type inconsistency withstream_id.At line 266,
RealSense_StreamId.RGB(an IntEnum) is passed directly to the constructor, whereaslinux_d555_player.pypassesstream_id.value(an int). While IntEnum supports arithmetic operations, using.valueconsistently across files would improve clarity.
233-239: Consider proper error handling instead ofassertfor CUDA operations.Same concern as in
linux_d555_player.py- usingassertfor CUDA error checking is fragile since assertions can be disabled with Python's-Oflag.
275-275: Remove or make configurable the debug environment variable.Same as
linux_d555_player.py- hardcodingGXF_MEMORY_DEBUG=1should be avoided in production code.src/hololink/operators/image_decoder/image_decoder.cpp (1)
72-76: Inefficient sequential prefix sum on GPU.The
prefix_sum_histogramkernel runs a sequential O(n) loop on a single GPU thread (launched with grid{1,1,1}). For 65536 elements, this negates GPU parallelism benefits. Consider using a parallel scan algorithm (e.g., Blelloch scan) or CUB'sDeviceScan::InclusiveSumfor significantly better performance.python/hololink/sensors/d555/d555.py (3)
173-173: Remove extraneous f-string prefix.The f-string has no placeholders. Per static analysis hint (F541).
Proposed fix
- logging.debug(f"[Realsense] Configuring converter") + logging.debug("[Realsense] Configuring converter")
195-196: Consider documenting or using theenableparameter.The
enableparameter is unused. Since this is a stub, consider adding a TODO or documenting the intended behavior. Per static analysis hint (ARG002).Proposed fix
def test_pattern(self, enable=False): + # TODO: Implement test pattern control when supported by RealSense D555 logging.info("Test pattern control is not implemented for RealSense.")
82-83: Fix continuation line indentation.The continuation lines are under-indented per E128. As per coding guidelines, run
ci/lint.sh --formatto auto-format.Proposed fix
- self.set_register(address=MUX_I2C_ADDR, register=data_low, value=data_high,\ - reg_size=DataWidth.BITS_16, val_size=DataWidth.BITS_16, endian=Endianness.LITTLE) + self.set_register( + address=MUX_I2C_ADDR, + register=data_low, + value=data_high, + reg_size=DataWidth.BITS_16, + val_size=DataWidth.BITS_16, + endian=Endianness.LITTLE, + )src/hololink/operators/image_decoder/image_decoder.hpp (1)
47-51: Consider renaming customfloat3struct to avoid potential confusion.CUDA defines
float3invector_types.h. While your struct is in a different namespace (hololink::operators::ImageDecoder::float3), having the same name could cause confusion when reading code that mixes CUDA and host-side logic. Consider a name likeColorRGBorfloat3_host.python/hololink/sensors/d555/d555_mode.py (2)
73-76: Inconsistent namedtuple naming.The variable is named
stream_infobut the namedtuple's internal name is"stream_profile". This can cause confusion in debugging and repr output. Consider aligning the names.Proposed fix
-stream_info = namedtuple( - "stream_profile", +stream_info = namedtuple( + "stream_info", ["width", "height", "framerate", "pixel_format"] )
89-96: Preferappend()overinsert()when building a list sequentially.Using
insert(i, ...)wheniequals the current list length is equivalent toappend()but less idiomatic and slightly less efficient.Proposed fix
for i, (w, h, fps) in enumerate(depth_profiles): - depth_stream_profiles.insert( - i, + depth_stream_profiles.append( stream_info( w, h, fps, hololink_module.operators.ImageDecoderOp.PixelFormat.Z16 ) )Apply the same pattern to
rgb_stream_profiles(lines 111-117).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/user_guide/examples.mdexamples/linux_d555_dual_stream.pyexamples/linux_d555_peoplenet.pyexamples/linux_d555_player.pypython/hololink/hololink.cpppython/hololink/operators/CMakeLists.txtpython/hololink/operators/__init__.pypython/hololink/operators/base_receiver_op.pypython/hololink/operators/image_decoder/CMakeLists.txtpython/hololink/operators/image_decoder/image_decoder.cpppython/hololink/operators/linux_receiver_operator.pypython/hololink/sensors/__init__.pypython/hololink/sensors/d555/__init__.pypython/hololink/sensors/d555/d555.pypython/hololink/sensors/d555/d555_mode.pypython/hololink/sensors/d555/model.pypython/setup.pysrc/hololink/core/data_channel.cppsrc/hololink/core/data_channel.hppsrc/hololink/operators/CMakeLists.txtsrc/hololink/operators/image_decoder/CMakeLists.txtsrc/hololink/operators/image_decoder/image_decoder.cppsrc/hololink/operators/image_decoder/image_decoder.hpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
python/hololink/operators/__init__.pyexamples/linux_d555_player.pypython/setup.pyexamples/linux_d555_dual_stream.pypython/hololink/sensors/d555/model.pypython/hololink/sensors/d555/__init__.pyexamples/linux_d555_peoplenet.pypython/hololink/operators/linux_receiver_operator.pypython/hololink/sensors/d555/d555.pypython/hololink/operators/base_receiver_op.pypython/hololink/sensors/__init__.pypython/hololink/sensors/d555/d555_mode.py
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All C++ source code must be formatted according to the rules in
ci/lint.sh(can be auto-formatted usingci/lint.sh --format)
Files:
src/hololink/core/data_channel.hpppython/hololink/operators/image_decoder/image_decoder.cpppython/hololink/hololink.cppsrc/hololink/core/data_channel.cppsrc/hololink/operators/image_decoder/image_decoder.cppsrc/hololink/operators/image_decoder/image_decoder.hpp
**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Markdown files must be formatted according to the rules in
ci/lint.sh(can be auto-formatted usingci/lint.sh --format)
Files:
docs/user_guide/examples.md
🧬 Code graph analysis (9)
python/hololink/operators/image_decoder/image_decoder.cpp (1)
src/hololink/operators/image_decoder/image_decoder.cpp (6)
setup(136-143)setup(136-136)configure(301-331)configure(301-305)get_csi_length(333-338)get_csi_length(333-333)
python/hololink/hololink.cpp (2)
src/hololink/core/data_channel.cpp (2)
configure_socket(302-377)configure_socket(302-302)src/hololink/core/enumerator.cpp (2)
configure_socket(690-723)configure_socket(690-690)
examples/linux_d555_dual_stream.py (5)
src/hololink/core/data_channel.hpp (3)
frame_size(179-179)DataChannel(69-71)DataChannel(169-169)python/hololink/operators/linux_receiver_operator.py (1)
LinuxReceiverOperator(33-149)python/hololink/sensors/d555/d555_mode.py (3)
RealSense_StreamId(9-11)RealSense_Depth_Mode(38-55)RealSense_RGB_Mode(19-36)src/hololink/operators/image_decoder/image_decoder.hpp (1)
width(59-62)python/hololink/__init__.py (1)
logging_level(288-289)
examples/linux_d555_peoplenet.py (2)
python/hololink/sensors/d555/d555_mode.py (2)
RealSense_RGB_Mode(19-36)RealSense_StreamId(9-11)python/hololink/__init__.py (1)
logging_level(288-289)
python/hololink/operators/linux_receiver_operator.py (1)
src/hololink/core/data_channel.cpp (2)
configure_socket(302-377)configure_socket(302-302)
python/hololink/sensors/d555/d555.py (2)
python/hololink/sensors/d555/model.py (2)
Endianness(3-5)DataWidth(7-10)python/hololink/sensors/d555/d555_mode.py (2)
RealSense_StreamCommand(13-16)RealSense_StreamId(9-11)
src/hololink/core/data_channel.cpp (1)
src/hololink/core/data_channel.hpp (1)
socket_fd(137-137)
python/hololink/operators/base_receiver_op.py (6)
src/hololink/operators/image_decoder/image_decoder.hpp (1)
spec(55-55)src/hololink/operators/packed_format_converter/packed_format_converter.hpp (1)
spec(45-45)src/hololink/operators/sipl_capture/sipl_capture.hpp (1)
spec(49-49)examples/sipl_player.cpp (2)
spec(37-40)spec(37-37)src/hololink/operators/roce_receiver/roce_receiver_op.hpp (1)
spec(41-41)src/hololink/operators/base_receiver_op.hpp (1)
spec(65-65)
src/hololink/operators/image_decoder/image_decoder.cpp (1)
src/hololink/operators/base_receiver_op.hpp (1)
input(87-88)
🪛 Clang (14.0.6)
src/hololink/operators/image_decoder/image_decoder.hpp
[error] 21-21: 'memory' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.19.0)
python/hololink/operators/image_decoder/image_decoder.cpp
[error] 73-73: syntax error
(syntaxError)
🪛 Flake8 (7.3.0)
examples/linux_d555_player.py
[error] 19-19: 'ctypes' imported but unused
(F401)
examples/linux_d555_dual_stream.py
[error] 19-19: 'ctypes' imported but unused
(F401)
[error] 268-268: indentation is not a multiple of 4 (comment)
(E114)
[error] 268-268: unexpected indentation (comment)
(E116)
python/hololink/sensors/d555/d555.py
[error] 83-83: continuation line under-indented for visual indent
(E128)
[error] 88-88: continuation line under-indented for visual indent
(E128)
[error] 98-98: continuation line under-indented for visual indent
(E128)
[error] 173-173: f-string is missing placeholders
(F541)
🪛 markdownlint-cli2 (0.18.1)
docs/user_guide/examples.md
101-101: Dollar signs used before commands without showing output
(MD014, commands-show-output)
102-102: Dollar signs used before commands without showing output
(MD014, commands-show-output)
103-103: Dollar signs used before commands without showing output
(MD014, commands-show-output)
113-113: Dollar signs used before commands without showing output
(MD014, commands-show-output)
118-118: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
142-142: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🪛 Ruff (0.14.10)
python/hololink/sensors/d555/d555.py
173-173: f-string without any placeholders
Remove extraneous f prefix
(F541)
195-195: Unused method argument: enable
(ARG002)
🔇 Additional comments (17)
python/setup.py (1)
60-60: LGTM!The new
hololink/sensors/d555package entry is correctly added to support the RealSense D555 sensor integration.examples/linux_d555_dual_stream.py (1)
110-133: Verify frame size compatibility between depth and RGB streams.Both receiver operators use
frame_sizecomputed fromimage_decoder_stream1(depth stream), but RGB and depth streams may have different resolutions or pixel formats. If the RGB stream requires a different buffer size, this could cause data truncation or buffer overflows.Consider computing the frame size independently for each stream:
frame_size = image_decoder_stream1.get_csi_length() + frame_size_stream2 = image_decoder_stream2.get_csi_length() frame_context = self._cuda_context receiver_operator_stream1 = hololink_module.operators.LinuxReceiverOperator( ... frame_size=frame_size, ... ) receiver_operator_stream2 = hololink_module.operators.LinuxReceiverOperator( ... - frame_size=frame_size, + frame_size=frame_size_stream2, ... )src/hololink/operators/CMakeLists.txt (1)
73-73: LGTM!The
image_decodersubdirectory is correctly added to the build, following the existing pattern for operator modules.python/hololink/operators/CMakeLists.txt (1)
41-41: LGTM!The Python bindings for
image_decoderare correctly included in the build configuration.src/hololink/core/data_channel.hpp (1)
137-137: LGTM!The
configure_socketAPI extension with an optionaludp_portparameter is backward compatible and uses the appropriateuint16_ttype for port numbers.python/hololink/operators/base_receiver_op.py (1)
61-61: LGTM!The
udp_portparameter is correctly declared with a default value of 0, matching the C++ API. This enables derived classes likeLinuxReceiverOperatorto configure the UDP port when setting up the socket.python/hololink/operators/__init__.py (1)
30-30: LGTM!The addition of
ImageDecoderOpfollows the established lazy loading pattern and is correctly positioned in the module dictionary.python/hololink/sensors/__init__.py (1)
29-29: LGTM!The addition of the
d555sensor module follows the established pattern for lazy loading and is correctly integrated into the module structure.python/hololink/sensors/d555/__init__.py (1)
16-21: LGTM!The package initialization correctly imports and exposes the
d555andd555_modesubmodules following the standard Python package pattern.python/hololink/operators/linux_receiver_operator.py (1)
72-72:udp_portis properly defined in the base class through the parameter framework.The
udp_portparameter is registered inBaseReceiverOp.setup()usingspec.param("udp_port", 0), which makes it available asself.udp_porton all subclass instances. The usage at line 72 is correct and will work as intended with the updatedconfigure_socketAPI.src/hololink/core/data_channel.cpp (1)
302-302: LGTM! The function signature change to accept audp_portparameter is well-designed and maintains backward compatibility through the default value of 0 in the header declaration. Both call sites are properly compatible: the Python caller passes an explicit value, while the C++ caller relies on the default.python/hololink/operators/image_decoder/CMakeLists.txt (1)
1-8: LGTM!The pybind11 module definition follows the project's established patterns and correctly configures the image_decoder Python binding.
src/hololink/operators/image_decoder/CMakeLists.txt (1)
16-23: Verify the commented realsense2 dependency.The
realsense2library is commented out at line 19. Given that this PR adds RealSense D555 support, please confirm whether this dependency is:
- Not needed for the current implementation
- Planned for future work
- Should be uncommented
If the D555 image decoder does not actually depend on the RealSense SDK (e.g., it only processes raw streams), consider removing the comment entirely to avoid confusion.
python/hololink/hololink.cpp (1)
469-469: LGTM!The updated binding correctly exposes the two-parameter
configure_socketsignature with named arguments and a sensible default forudp_port. This aligns with the C++ API changes for configurable UDP port support.examples/linux_d555_peoplenet.py (1)
93-93: Verify emptyout_tensor_nameis intentional.The
out_tensor_name=""is empty here, whereaslinux_d555_player.pyuses"output". This may cause issues with tensor lookup or downstream operators if they expect a named tensor.src/hololink/operators/image_decoder/image_decoder.hpp (1)
37-94: LGTM on class structure.The
ImageDecoderclass has a clean public API with proper lifecycle methods (start,stop,setup,compute), configuration method, and well-encapsulated private members. ThePixelFormatenum provides clear type-safe format selection.python/hololink/sensors/d555/d555_mode.py (1)
9-56: LGTM on enum definitions.The
RealSense_StreamId,RealSense_StreamCommand,RealSense_RGB_Mode, andRealSense_Depth_Modeenums are well-structured with clear naming conventions that encode resolution and framerate information.
| ``` | ||
| mac_id=98:4F:EE:1A:F4:A9 hsb_ip_version=0x2501 fpga_crc=0xffff ip_address=192.168.11.55 fpga_uuid=889b7ce3-65a5-4247-8b05-4ff1904c3359 serial_number=70255f4343534c interface=eno1 board=hololink-lite | ||
| mac_id=98:4F:EE:1A:F4:A9 hsb_ip_version=0x2501 fpga_crc=0xffff ip_address=192.168.11.55 fpga_uuid=889b7ce3-65a5-4247-8b05-4ff1904c3359 serial_number=70255f4343534c interface=eno1 board=hololink-lite | ||
| mac_id=98:4F:EE:1A:F4:A9 hsb_ip_version=0x2501 fpga_crc=0xffff ip_address=192.168.11.55 fpga_uuid=889b7ce3-65a5-4247-8b05-4ff1904c3359 serial_number=70255f4343534c interface=eno1 board=hololink-lite | ||
| ^C | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to the code block.
The fenced code block showing the hololink-enumerate output is missing a language specifier. Add text or console to comply with markdown linting rules.
📝 Proposed fix
-```
+```text
mac_id=98:4F:EE:1A:F4:A9 hsb_ip_version=0x2501 fpga_crc=0xffff ip_address=192.168.11.55 fpga_uuid=889b7ce3-65a5-4247-8b05-4ff1904c3359 serial_number=70255f4343534c interface=eno1 board=hololink-lite
mac_id=98:4F:EE:1A:F4:A9 hsb_ip_version=0x2501 fpga_crc=0xffff ip_address=192.168.11.55 fpga_uuid=889b7ce3-65a5-4247-8b05-4ff1904c3359 serial_number=70255f4343534c interface=eno1 board=hololink-lite
mac_id=98:4F:EE:1A:F4:A9 hsb_ip_version=0x2501 fpga_crc=0xffff ip_address=192.168.11.55 fpga_uuid=889b7ce3-65a5-4247-8b05-4ff1904c3359 serial_number=70255f4343534c interface=eno1 board=hololink-lite
^C
</details>
As per coding guidelines, all markdown files must be formatted according to `ci/lint.sh` rules.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
118-118: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/user_guide/examples.md around lines 118 - 123, The fenced code block
that shows the hololink-enumerate output is missing a language specifier; update
the triple-backtick opening fence for the block containing the
hololink-enumerate output (the example showing mac_id=98:4F:EE:1A:F4:A9 ... and
^C) to include a language such as text or console (e.g., change totext)
so the markdown complies with lint rules.
</details>
<!-- fingerprinting:phantom:poseidon:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
| @@ -0,0 +1,322 @@ | |||
| # SPDX-FileCopystream2Text: Copystream2 (c) 2023-2024 NVIDIA CORPORATION & AFFILIATES. All stream2s reserved. | |||
There was a problem hiding this comment.
Fix typos in license header.
The license header contains copy-paste errors: "FileCopystream2Text" should be "FileCopyrightText", and "stream2s reserved" should be "rights reserved".
Proposed fix
-# SPDX-FileCopystream2Text: Copystream2 (c) 2023-2024 NVIDIA CORPORATION & AFFILIATES. All stream2s reserved.
+# SPDX-FileCopyrightText: Copyright (c) 2023-2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.📝 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.
| # SPDX-FileCopystream2Text: Copystream2 (c) 2023-2024 NVIDIA CORPORATION & AFFILIATES. All stream2s reserved. | |
| # SPDX-FileCopyrightText: Copyright (c) 2023-2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. |
🤖 Prompt for AI Agents
In @examples/linux_d555_dual_stream.py at line 1, Fix the typos in the file
license header: change "FileCopystream2Text" to "FileCopyrightText" and replace
"stream2s reserved" with "rights reserved" so the header reads correctly (keep
the SPDX prefix and existing copyright holder text intact).
| # See README.md for detailed information. | ||
|
|
||
| import argparse | ||
| import ctypes |
There was a problem hiding this comment.
Remove unused import.
The ctypes module is imported but never used in this file (flagged by Flake8).
Proposed fix
import argparse
-import ctypes
import logging📝 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.
| import ctypes | |
| import argparse | |
| import logging |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 19-19: 'ctypes' imported but unused
(F401)
🤖 Prompt for AI Agents
In @examples/linux_d555_dual_stream.py at line 19, The file imports the unused
module name "ctypes" causing a Flake8 warning; remove the unused import line
(the standalone "import ctypes") from examples/linux_d555_dual_stream.py so the
module is no longer imported but otherwise leave the rest of the file unchanged.
| hololink_module.DataChannel.use_sensor(channel_metadata_stream1, 0) | ||
| channel_metadata_stream2 = hololink_module.Metadata(channel_metadata) | ||
| hololink_module.DataChannel.use_sensor(channel_metadata_stream2, 1) | ||
| # |
There was a problem hiding this comment.
Fix indentation.
The comment on line 268 has incorrect indentation (not a multiple of 4, and unexpected indentation). This was flagged by Flake8.
Proposed fix
hololink_module.DataChannel.use_sensor(channel_metadata_stream2, 1)
- #
+ #
hololink_channel_stream_depth = hololink_module.DataChannel(channel_metadata_stream1)📝 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.
| # | |
| hololink_module.DataChannel.use_sensor(channel_metadata_stream2, 1) | |
| # | |
| hololink_channel_stream_depth = hololink_module.DataChannel(channel_metadata_stream1) |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 268-268: indentation is not a multiple of 4 (comment)
(E114)
[error] 268-268: unexpected indentation (comment)
(E116)
🤖 Prompt for AI Agents
In @examples/linux_d555_dual_stream.py at line 268, Fix the incorrect
indentation for the lone comment "#" in examples/linux_d555_dual_stream.py
(around the block containing that comment) so it uses an indentation level that
is a multiple of 4 and matches the surrounding block scope (either align to the
enclosing block's indentation or move to column 0 if top-level) to satisfy
Flake8.
| f"WRITE >> address=0x{int(address):X} set_register(register=0x{int(register):04X}, value=0x{int(value):04X})" | ||
| ) | ||
|
|
||
| write_bytes = bytearray(4) |
There was a problem hiding this comment.
Potential buffer overflow in set_register.
The write_bytes buffer is fixed at 4 bytes, but if reg_size and val_size are both BITS_32, the total would be 8 bytes, causing a buffer overflow. While current usage is safe (BITS_16 + BITS_16), the function signature allows dangerous combinations.
Proposed fix
- write_bytes = bytearray(4)
+ buffer_size = reg_size.value + val_size.value
+ write_bytes = bytearray(buffer_size)📝 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.
| write_bytes = bytearray(4) | |
| buffer_size = reg_size.value + val_size.value | |
| write_bytes = bytearray(buffer_size) |
🤖 Prompt for AI Agents
In @python/hololink/sensors/d555/d555.py at line 137, The buffer write_bytes in
set_register is a fixed 4 bytes and can overflow if reg_size and val_size are
both BITS_32; change allocation to compute total_bytes from reg_size and
val_size (convert bits to bytes) and allocate bytearray(total_bytes) instead of
bytearray(4), add a defensive check to validate total_bytes is within expected
limits (and raise/handle if too large), and update any code that writes into
write_bytes to use the computed size; reference symbols: set_register,
write_bytes, reg_size, val_size, BITS_16, BITS_32.
| from enum import Enum | ||
|
|
||
| class Endianness(Enum): | ||
| LITTLE = "little" | ||
| BIG = "big" | ||
|
|
||
| class DataWidth(Enum): | ||
| BITS_8 = 1 | ||
| BITS_16 = 2 | ||
| BITS_32 = 4 |
There was a problem hiding this comment.
Missing SPDX license header.
This file is missing the standard Apache 2.0 license header that is present in all other files in this PR. Please add the license header for consistency.
Proposed fix
+# SPDX-FileCopyrightText: Copyright (c) 2024-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
from enum import Enum🤖 Prompt for AI Agents
In @python/hololink/sensors/d555/model.py around lines 1 - 10, Add the missing
Apache-2.0 SPDX license header at the top of the file so it matches other files
in the repo; update python/hololink/sensors/d555/model.py by inserting the
standard single-line SPDX identifier and full Apache 2.0 header comment before
any imports (above the "from enum import Enum" line) so the file containing the
Endianness and DataWidth Enum definitions includes the required license
declaration.
| // Not multicast; use bind(local_ip,0) so that the kernel assigns us a UDP port. | ||
| sockaddr_in address {}; | ||
| address.sin_family = AF_INET; | ||
| address.sin_port = htons(0); | ||
| address.sin_port = htons(udp_port); |
There was a problem hiding this comment.
Update the comment to reflect the new behavior.
The comment on line 363 states that the code uses bind(local_ip,0) to let the kernel assign a UDP port, but the implementation now binds to udp_port, which may be non-zero. This makes the comment misleading when a specific port is requested.
📝 Suggested fix for the comment
- } else {
- // Not multicast; use bind(local_ip,0) so that the kernel assigns us a UDP port.
+ } else {
+ // Not multicast; bind to the specified UDP port (or 0 to let the kernel assign one).
sockaddr_in address {};
address.sin_family = AF_INET;
address.sin_port = htons(udp_port);🤖 Prompt for AI Agents
In @src/hololink/core/data_channel.cpp around lines 363 - 366, The comment above
the sockaddr_in setup is stale: it says we use bind(local_ip,0) to let the
kernel pick a port, but the code sets address.sin_port = htons(udp_port) so we
may bind to a specific port (if udp_port != 0) or let the kernel assign one (if
udp_port == 0). Update the comment near the sockaddr_in/address and
address.sin_port = htons(udp_port) lines to state that we bind to udp_port
(which can be zero to let the kernel choose a port) rather than always using 0.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
python/hololink/operators/image_decoder/image_decoder.cpp (1)
93-96: Improve enum docstrings for clarity.The current docstrings ("Z16-bit" and "YUYV-bit") are unclear. Consider providing more descriptive documentation for these pixel formats.
📝 Suggested improvement
py::enum_<ImageDecoder::PixelFormat>(op, "PixelFormat") - .value("Z16", ImageDecoder::PixelFormat::Z16, R"pbdoc(Z16-bit)pbdoc") - .value("YUYV", ImageDecoder::PixelFormat::YUYV, R"pbdoc(YUYV-bit)pbdoc") + .value("Z16", ImageDecoder::PixelFormat::Z16, R"pbdoc(16-bit depth format)pbdoc") + .value("YUYV", ImageDecoder::PixelFormat::YUYV, R"pbdoc(YUYV 4:2:2 color format)pbdoc") .export_values();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
python/hololink/operators/image_decoder/image_decoder.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All C++ source code must be formatted according to the rules in
ci/lint.sh(can be auto-formatted usingci/lint.sh --format)
Files:
python/hololink/operators/image_decoder/image_decoder.cpp
🧬 Code graph analysis (1)
python/hololink/operators/image_decoder/image_decoder.cpp (1)
src/hololink/operators/image_decoder/image_decoder.cpp (6)
setup(136-143)setup(136-136)configure(301-331)configure(301-305)get_csi_length(333-338)get_csi_length(333-333)
🪛 Cppcheck (2.19.0)
python/hololink/operators/image_decoder/image_decoder.cpp
[error] 72-72: syntax error
(syntaxError)
🔇 Additional comments (3)
python/hololink/operators/image_decoder/image_decoder.cpp (3)
1-38: LGTM!License header, includes, and using declarations are appropriate for pybind11 bindings. The include set covers all required dependencies.
51-70: LGTM!The trampoline class correctly follows the Holoscan operator pybind11 pattern. The initialization sequence mirrors
Fragment::make_operator<OperatorT>as documented.
72-91: LGTM!The module definition and class bindings are correct. The static analysis syntax error on line 72 is a false positive—
PYBIND11_MODULEis a macro that Cppcheck cannot parse, but it expands to valid C++ code.The
configurebinding correctly exposes all parameters with appropriate defaults matching the C++ API.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @python/hololink/sensors/d555/d555.py:
- Around line 173-174: The log call inside configure_converter uses an
unnecessary f-string with no placeholders; change the logging.debug call in the
configure_converter method to use a plain string (e.g.,
logging.debug("[Realsense] Configuring converter")) instead of the current
f-string to remove the extraneous 'f' prefix.
- Around line 76-99: The multi-line calls to set_register in start_camera_stream
and stop_camera_stream use backslash continuations and are mis-indented (Flake8
E128); replace the backslashes with implicit line continuation by wrapping the
arguments in parentheses and align the continued lines under the opening
parenthesis for set_register in both start_camera_stream and stop_camera_stream,
keeping the same argument order (address=MUX_I2C_ADDR, register=..., value=...,
reg_size=DataWidth.BITS_16, val_size=DataWidth.BITS_16,
endian=Endianness.LITTLE) so the lines pass ci/lint.sh.
- Around line 53-56: The else branch handling an invalid stream_id leaves the
object in an inconsistent state by only setting self._mode = -1; update that
error path in the method that selects the stream profile so that you either
raise a clear exception (e.g., ValueError) when an invalid stream_id is
provided, or reset all related fields to safe defaults (set self._stream_profile
= None, self._height = 0, self._width = 0, self._pixel_format = None) in
addition to setting self._mode = -1 and logging the error; ensure you apply this
change where the invalid stream_id is detected so callers cannot observe stale
profile/size/format values.
- Around line 167-170: The code shadows the function parameter named value and
may truncate data because write_uint32 writes only 4 bytes while
serializer.length()/buffer_size can exceed 4; rename the local variable (e.g.,
reg_value) to avoid shadowing, compute buffer_size from serializer.length() (or
the DataWidth sum) and add a guard that raises/logs if buffer_size > 4, or
switch to a multi-byte write API (e.g., a hololink.write_bytes or equivalent)
when buffer_size > 4 so the full write_bytes payload is written instead of using
write_uint32; ensure you still use reg_data_buffer_ as the destination and
preserve byteorder='little' when composing the multi-byte write.
In @src/hololink/operators/image_decoder/image_decoder.cpp:
- Line 203: The code currently calls entity.get<nvidia::gxf::Tensor>().value()
without checking the optional; change this to first retrieve the optional (auto
opt_tensor = entity.get<nvidia::gxf::Tensor>();) and check
opt_tensor.has_value() (or if (!opt_tensor) ) before calling .value(); if
missing, log a clear error referencing the operator/component (e.g.,
image_decoder) and return an appropriate failure status (or throw a descriptive
exception) instead of allowing std::bad_optional_access to be thrown, then use
opt_tensor.value() to assign input_tensor.
- Around line 297-300: The code calls
cuda_stream_handler_.to_message(out_message) and assigns stream_handler_result
but does not check for failure before constructing an Entity and calling
output.emit; update the YUYV branch to mirror the Z16 branch by checking
stream_handler_result (e.g., success()/ok()/has_value() depending on result
type) and, on failure, log the error and return early instead of emitting; use
the same variables (stream_handler_result, out_message,
cuda_stream_handler_.to_message, holoscan::gxf::Entity, output.emit) so the flow
and error handling match the Z16 case.
🧹 Nitpick comments (5)
src/hololink/operators/image_decoder/image_decoder.cpp (3)
65-70: Redundant condition check.On line 69,
d < 65536is always true for auint16_t(max value is 65535). Thed > 0check is valid to exclude invalid depth values.Suggested simplification
__global__ void compute_histogram(const uint16_t* depth, int* hist, int size) { int idx = blockIdx.x * blockDim.x + threadIdx.x; if (idx >= size) return; uint16_t d = depth[idx]; - if (d > 0 && d < 65536) atomicAdd(&hist[d], 1); + if (d > 0) atomicAdd(&hist[d], 1); }
72-76: Sequential prefix sum on GPU is inefficient.This kernel runs a sequential loop over 65536 elements on a single GPU thread (launched with grid
{1,1,1}at line 256). This negates GPU parallelization benefits and will likely be a performance bottleneck for real-time depth processing.Consider using a parallel scan algorithm (e.g., Blelloch scan, or
thrust::inclusive_scan) for significantly better throughput.
319-332: Duplicated case handling in configure().The
Z16andYUYVcases have identical logic. Consider combining them to reduce duplication.Suggested refactor
switch (pixel_format_) { case PixelFormat::Z16: - bytes_per_line_ = width * 2; - line_start_size_ += margin_left * 2; - line_end_size_ += margin_right * 2; - break; case PixelFormat::YUYV: bytes_per_line_ = width * 2; line_start_size_ += margin_left * 2; line_end_size_ += margin_right * 2; break; default: throw std::runtime_error("Unsupported pixel format"); }python/hololink/sensors/d555/d555.py (2)
105-112: Remove commented-out code and clarify comment.The comment "Setting these register is time-consuming" appears disconnected from the surrounding code, and there's commented-out code at line 112. Either remove the dead code or add a TODO explaining why it's kept.
Suggested cleanup
def start(self): """Start Streaming""" logging.info("RealSenseCam: start()") - # - # Setting these register is time-consuming. - logging.info( - "RealSenseCam: Stopping any previous streaming before starting new one." - ) - - # If the camera stream is already running, stop it first - # self.stop_camera_stream() - logging.info("RealSenseCam: Attempting to start streaming...") self.start_camera_stream() self._running = True
196-197: Unused parameterenable.If this is a placeholder for future implementation, prefix with underscore to suppress the warning.
Suggested fix
- def test_pattern(self, enable=False): + def test_pattern(self, _enable=False): logging.info("Test pattern control is not implemented for RealSense.")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/hololink/sensors/d555/d555.pysrc/hololink/operators/image_decoder/image_decoder.cpp
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
python/hololink/sensors/d555/d555.py
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All C++ source code must be formatted according to the rules in
ci/lint.sh(can be auto-formatted usingci/lint.sh --format)
Files:
src/hololink/operators/image_decoder/image_decoder.cpp
🧬 Code graph analysis (1)
src/hololink/operators/image_decoder/image_decoder.cpp (1)
src/hololink/operators/base_receiver_op.hpp (1)
input(87-88)
🪛 Flake8 (7.3.0)
python/hololink/sensors/d555/d555.py
[error] 83-83: continuation line under-indented for visual indent
(E128)
[error] 88-88: continuation line under-indented for visual indent
(E128)
[error] 98-98: continuation line under-indented for visual indent
(E128)
[error] 174-174: f-string is missing placeholders
(F541)
🪛 Ruff (0.14.10)
python/hololink/sensors/d555/d555.py
174-174: f-string without any placeholders
Remove extraneous f prefix
(F541)
196-196: Unused method argument: enable
(ARG002)
🔇 Additional comments (8)
src/hololink/operators/image_decoder/image_decoder.cpp (3)
145-175: LGTM with minor consistency note.The start method correctly initializes CUDA context, compiles kernels, and allocates device memory. Error handling is present for all allocations.
Minor: Consider using a consistent error-checking pattern throughout (either
CudaCheckwrapper or manual checks) for maintainability.
177-194: LGTM!Resource cleanup is handled correctly with proper null checks before freeing device memory.
339-344: LGTM!Proper validation before returning the computed CSI length.
python/hololink/sensors/d555/d555.py (5)
1-19: LGTM!Imports and module constants are well-organized.
22-39: LGTM!Constructor initialization is clean with sensible default values.
64-74: LGTM!The
power_onandconfiguremethods are straightforward with appropriate delegation.
119-122: LGTM!Clean implementation of the stop method.
186-194: LGTM!Simple delegation and accessor methods are appropriate.
| else: | ||
| logging.error("Incorrect mode for Realsense D555 camera.") | ||
| self._mode = -1 | ||
| return |
There was a problem hiding this comment.
Inconsistent state on error path.
When an invalid stream_id is encountered, _mode is set to -1 but _stream_profile, _height, _width, and _pixel_format retain their previous values. Consider also resetting these fields or raising an exception to prevent the object from being in an inconsistent state.
Suggested fix
else:
logging.error("Incorrect mode for Realsense D555 camera.")
- self._mode = -1
- return
+ raise ValueError(f"Invalid stream_id: {self._stream_id}")Alternatively, if you prefer to keep the error-logging approach, reset the related fields to safe defaults.
📝 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.
| else: | |
| logging.error("Incorrect mode for Realsense D555 camera.") | |
| self._mode = -1 | |
| return | |
| else: | |
| logging.error("Incorrect mode for Realsense D555 camera.") | |
| raise ValueError(f"Invalid stream_id: {self._stream_id}") |
🤖 Prompt for AI Agents
In @python/hololink/sensors/d555/d555.py around lines 53 - 56, The else branch
handling an invalid stream_id leaves the object in an inconsistent state by only
setting self._mode = -1; update that error path in the method that selects the
stream profile so that you either raise a clear exception (e.g., ValueError)
when an invalid stream_id is provided, or reset all related fields to safe
defaults (set self._stream_profile = None, self._height = 0, self._width = 0,
self._pixel_format = None) in addition to setting self._mode = -1 and logging
the error; ensure you apply this change where the invalid stream_id is detected
so callers cannot observe stale profile/size/format values.
|
|
||
| reg_data_buffer_ = self._i2c_controller_address + 16 # Offset for register data buffer | ||
| value = int.from_bytes(write_bytes[: serializer.length()], byteorder='little') | ||
| self._hololink.write_uint32(reg_data_buffer_, value, timeout=None) |
There was a problem hiding this comment.
Variable shadowing and potential truncation issue.
Line 169 shadows the value parameter with a local variable, which is confusing. Additionally, write_uint32 writes only 4 bytes, but buffer_size could exceed 4 if larger DataWidth values are used (e.g., BITS_32 + BITS_32 = 8 bytes).
Suggested fix
-
- reg_data_buffer_ = self._i2c_controller_address + 16 # Offset for register data buffer
- value = int.from_bytes(write_bytes[: serializer.length()], byteorder='little')
- self._hololink.write_uint32(reg_data_buffer_, value, timeout=None)
+ reg_data_buffer = self._i2c_controller_address + 16 # Offset for register data buffer
+ packed_value = int.from_bytes(write_bytes[: serializer.length()], byteorder='little')
+ self._hololink.write_uint32(reg_data_buffer, packed_value, timeout=None)Consider adding a guard to ensure buffer_size <= 4 or using a different write method for larger payloads.
📝 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.
| reg_data_buffer_ = self._i2c_controller_address + 16 # Offset for register data buffer | |
| value = int.from_bytes(write_bytes[: serializer.length()], byteorder='little') | |
| self._hololink.write_uint32(reg_data_buffer_, value, timeout=None) | |
| reg_data_buffer = self._i2c_controller_address + 16 # Offset for register data buffer | |
| packed_value = int.from_bytes(write_bytes[: serializer.length()], byteorder='little') | |
| self._hololink.write_uint32(reg_data_buffer, packed_value, timeout=None) |
🤖 Prompt for AI Agents
In @python/hololink/sensors/d555/d555.py around lines 167 - 170, The code
shadows the function parameter named value and may truncate data because
write_uint32 writes only 4 bytes while serializer.length()/buffer_size can
exceed 4; rename the local variable (e.g., reg_value) to avoid shadowing,
compute buffer_size from serializer.length() (or the DataWidth sum) and add a
guard that raises/logs if buffer_size > 4, or switch to a multi-byte write API
(e.g., a hololink.write_bytes or equivalent) when buffer_size > 4 so the full
write_bytes payload is written instead of using write_uint32; ensure you still
use reg_data_buffer_ as the destination and preserve byteorder='little' when
composing the multi-byte write.
| auto& entity = static_cast<nvidia::gxf::Entity&>(maybe_entity.value()); | ||
| gxf_result_t stream_handler_result = cuda_stream_handler_.from_message(context.context(), entity); | ||
| if (stream_handler_result != GXF_SUCCESS) throw std::runtime_error("Failed to get stream"); | ||
| auto input_tensor = entity.get<nvidia::gxf::Tensor>().value(); |
There was a problem hiding this comment.
Unchecked optional access may throw unexpectedly.
entity.get<nvidia::gxf::Tensor>().value() calls .value() without first checking if the optional has a value. If the tensor is missing from the entity, this will throw std::bad_optional_access with a less informative error message.
Suggested fix with explicit check
- auto input_tensor = entity.get<nvidia::gxf::Tensor>().value();
+ auto maybe_tensor = entity.get<nvidia::gxf::Tensor>();
+ if (!maybe_tensor) {
+ throw std::runtime_error("Input entity missing tensor");
+ }
+ auto input_tensor = maybe_tensor.value();📝 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.
| auto input_tensor = entity.get<nvidia::gxf::Tensor>().value(); | |
| auto maybe_tensor = entity.get<nvidia::gxf::Tensor>(); | |
| if (!maybe_tensor) { | |
| throw std::runtime_error("Input entity missing tensor"); | |
| } | |
| auto input_tensor = maybe_tensor.value(); |
🤖 Prompt for AI Agents
In @src/hololink/operators/image_decoder/image_decoder.cpp at line 203, The code
currently calls entity.get<nvidia::gxf::Tensor>().value() without checking the
optional; change this to first retrieve the optional (auto opt_tensor =
entity.get<nvidia::gxf::Tensor>();) and check opt_tensor.has_value() (or if
(!opt_tensor) ) before calling .value(); if missing, log a clear error
referencing the operator/component (e.g., image_decoder) and return an
appropriate failure status (or throw a descriptive exception) instead of
allowing std::bad_optional_access to be thrown, then use opt_tensor.value() to
assign input_tensor.
| stream_handler_result = cuda_stream_handler_.to_message(out_message); | ||
| auto out_entity = holoscan::gxf::Entity(std::move(out_message.value())); | ||
| output.emit(out_entity); | ||
| return; |
There was a problem hiding this comment.
Missing error check for stream handler result.
Unlike the Z16 case (lines 276-279), the YUYV case assigns stream_handler_result but does not check if it succeeded before emitting the output.
Suggested fix
stream_handler_result = cuda_stream_handler_.to_message(out_message);
+ if (stream_handler_result != GXF_SUCCESS) {
+ throw std::runtime_error("Failed to emit RGB image");
+ }
auto out_entity = holoscan::gxf::Entity(std::move(out_message.value()));
output.emit(out_entity);📝 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.
| stream_handler_result = cuda_stream_handler_.to_message(out_message); | |
| auto out_entity = holoscan::gxf::Entity(std::move(out_message.value())); | |
| output.emit(out_entity); | |
| return; | |
| stream_handler_result = cuda_stream_handler_.to_message(out_message); | |
| if (stream_handler_result != GXF_SUCCESS) { | |
| throw std::runtime_error("Failed to emit RGB image"); | |
| } | |
| auto out_entity = holoscan::gxf::Entity(std::move(out_message.value())); | |
| output.emit(out_entity); | |
| return; |
🤖 Prompt for AI Agents
In @src/hololink/operators/image_decoder/image_decoder.cpp around lines 297 -
300, The code calls cuda_stream_handler_.to_message(out_message) and assigns
stream_handler_result but does not check for failure before constructing an
Entity and calling output.emit; update the YUYV branch to mirror the Z16 branch
by checking stream_handler_result (e.g., success()/ok()/has_value() depending on
result type) and, on failure, log the error and return early instead of
emitting; use the same variables (stream_handler_result, out_message,
cuda_stream_handler_.to_message, holoscan::gxf::Entity, output.emit) so the flow
and error handling match the Z16 case.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@python/hololink/sensors/d555/d555.py`:
- Around line 198-199: The test_pattern method declares a parameter enable that
is unused and triggers ARG002; to silence the lint error, rename the parameter
to _enable (or otherwise prefix it with an underscore) in the test_pattern
signature or explicitly mark it used (e.g., assign to _ = enable) so the linter
recognizes it as intentionally unused; update the function definition for
test_pattern accordingly.
- Around line 104-121: The start() method's log claims it stops previous
streaming but the actual stop call is commented out; update start() to match the
log by re-enabling the stop logic: check self._running (or query running state)
and call self.stop_camera_stream() before calling self.start_camera_stream(),
then set self._running = True; alternatively, if you prefer not to stop
automatically, change the logging lines to not state that it stops previous
streaming and remove or adjust the misleading message. Ensure references to
start_camera_stream(), stop_camera_stream(), _running, and start() are updated
accordingly.
♻️ Duplicate comments (2)
python/hololink/sensors/d555/d555.py (2)
49-56: Avoid stale state on invalid stream_id.
Line 53–56 only sets_mode = -1and returns, leaving prior profile/size/format intact. Prefer raising or resetting related fields to avoid inconsistent state.🛠️ Suggested fix (raise on invalid stream_id)
else: logging.error("Incorrect mode for Realsense D555 camera.") - self._mode = -1 - return + raise ValueError(f"Invalid stream_id: {self._stream_id}")
127-172: Guard against truncation when payload exceeds 4 bytes.
Line 170–172 always useswrite_uint32; ifserializer.length()> 4, the payload will truncate. Add a size guard or use a multi-byte write API.🛠️ Suggested fix (guard for 4-byte writes)
- packed_value = int.from_bytes(write_bytes[: serializer.length()], byteorder='little') - self._hololink.write_uint32(reg_data_buffer, packed_value, timeout=None) + payload_len = serializer.length() + if payload_len > 4: + raise ValueError(f"Register write payload too large for write_uint32: {payload_len} bytes") + packed_value = int.from_bytes(write_bytes[:payload_len], byteorder='little') + self._hololink.write_uint32(reg_data_buffer, packed_value, timeout=None)If a multi-byte write API exists, please verify and switch to it:
#!/bin/bash # Search for alternative write APIs that support >4 bytes rg -n "write_.*(bytes|buffer|uint64|data)" -S
| def start(self): | ||
| """Start Streaming""" | ||
| logging.info("RealSenseCam: start()") | ||
|
|
||
| # | ||
| # Setting these register is time-consuming. | ||
| logging.info( | ||
| "RealSenseCam: Stopping any previous streaming before starting new one." | ||
| ) | ||
|
|
||
| # If the camera stream is already running, stop it first | ||
| # self.stop_camera_stream() | ||
|
|
||
| logging.info("RealSenseCam: Attempting to start streaming...") | ||
|
|
||
| self.start_camera_stream() | ||
| self._running = True | ||
|
|
There was a problem hiding this comment.
Align start logic with the log message.
Line 110–116 says it stops prior streaming, but the stop call is commented out. Either re-enable the stop when _running or update the log to match behavior.
✅ Suggested fix (stop when already running)
logging.info(
"RealSenseCam: Stopping any previous streaming before starting new one."
)
# If the camera stream is already running, stop it first
- # self.stop_camera_stream()
+ if self._running:
+ self.stop_camera_stream()🤖 Prompt for AI Agents
In `@python/hololink/sensors/d555/d555.py` around lines 104 - 121, The start()
method's log claims it stops previous streaming but the actual stop call is
commented out; update start() to match the log by re-enabling the stop logic:
check self._running (or query running state) and call self.stop_camera_stream()
before calling self.start_camera_stream(), then set self._running = True;
alternatively, if you prefer not to stop automatically, change the logging lines
to not state that it stops previous streaming and remove or adjust the
misleading message. Ensure references to start_camera_stream(),
stop_camera_stream(), _running, and start() are updated accordingly.
| def test_pattern(self, enable=False): | ||
| logging.info("Test pattern control is not implemented for RealSense.") |
There was a problem hiding this comment.
Silence unused-argument lint.
enable is unused (Ruff ARG002). Consider marking it as intentionally unused to keep lint clean.
🧹 Suggested fix
def test_pattern(self, enable=False):
+ _ = enable # TODO: implement test pattern control
logging.info("Test pattern control is not implemented for RealSense.")🧰 Tools
🪛 Ruff (0.14.13)
198-198: Unused method argument: enable
(ARG002)
🤖 Prompt for AI Agents
In `@python/hololink/sensors/d555/d555.py` around lines 198 - 199, The
test_pattern method declares a parameter enable that is unused and triggers
ARG002; to silence the lint error, rename the parameter to _enable (or otherwise
prefix it with an underscore) in the test_pattern signature or explicitly mark
it used (e.g., assign to _ = enable) so the linter recognizes it as
intentionally unused; update the function definition for test_pattern
accordingly.
|
Our D555 firmware hardcodes the UDP destination port per stream, so the host must bind to that exact port to receive data about the stream configuration. This optional udp_port parameter (default 0) lets us specify the firmware's expected port when needed, while preserving the existing kernel-auto-assign behavior for all other sensors. |
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.