WIP feat/stretch-isaac #100
Conversation
- docker: using default setting ref to templete_ws, no other package install - stretch3_brinbgup: import usd from isaacsim/assets folder, and open isaacsim - isaacsim: launch file and isaacsim node ref from go2_ws, usd for stretch3 downloaded from stretch3_isaacsim
open usda file in original model folder instead of install
- add .bashrc overwrite for building and quick source libraries - move ros2 packages install into dockerfile - disable install_stretch.sh install in dockerfile, saved the file for future develop - fix bug in compose.yaml, correct env variable name 'ROS2_WS'
…h is the omni_wheel
WalkthroughIntroduces a complete ROS 2 Stretch3 workspace with Docker containerization, Isaac Sim integration, and robot modeling. Includes environment setup scripts, multi-architecture Docker configuration, launch infrastructure, and comprehensive URDF/Xacro robot descriptions for multiple gripper and camera configurations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_head_nav_cam.xacro (1)
54-55: Remove redundant axis attribute for fixed joints.Same as the issue in stretch_base_imu.xacro - the axis attribute is not used for fixed joints.
🔎 Suggested fix
<child link="link_head_nav_cam" /> - <axis - xyz="0 0 0" /> </joint>
🟡 Minor comments (17)
stretch3_ws/src/isaacsim/package.xml-6-8 (1)
6-8: Minor grammatical issue in description.The description contains a grammatical error: "can used" should be "can be used".
🔎 Proposed fix
<description> - The isaacsim package that contains the script which can used to launch Isaac Sim as a ROS2 node from a launch file. + The isaacsim package that contains the script which can be used to launch Isaac Sim as a ROS2 node from a launch file. </description>stretch3_ws/src/isaacsim/model/assets/xacro/d405/_materials_d405.urdf.xacro-7-8 (1)
7-8: Typo in comment.Minor typo: "multple" should be "multiple".
🔎 Proposed fix
Collection of materials to be used in other macros. -This avoids the redefinition of materials in case multple cameras are imported. +This avoids the redefinition of materials in case multiple cameras are imported.stretch3_ws/src/isaacsim/package.xml-19-19 (1)
19-19: Update documentation URL to Isaac Sim 5.1.0.The documentation URL references Isaac Sim version 5.0.0, but the latest stable release is version 5.1.0 (released October 2025). Update the URL to
https://docs.isaacsim.omniverse.nvidia.com/5.1.0/index.html.stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_materials.urdf.xacro-3-9 (1)
3-9: Fix typo in comment.Line 8 contains a typo: "multple" should be "multiple".
🔎 Proposed fix
-This avoids the redefinition of materials in case multple cameras are imported. +This avoids the redefinition of materials in case multiple cameras are imported.stretch3_ws/src/isaacsim/model/assets/xacro/stretch_head_nav_cam.xacro-1-2 (1)
1-2: Fix incorrect robot name attribute.The robot name is declared as "stretch_respeaker" but this file defines the head navigation camera component. This appears to be a copy-paste error.
🔎 Suggested fix
<?xml version="1.0"?> -<robot xmlns:xacro="http://www.ros.org/wiki/xacro" name="stretch_respeaker"> +<robot xmlns:xacro="http://www.ros.org/wiki/xacro" name="stretch_head_nav_cam">stretch3_ws/src/isaacsim/model/assets/xacro/d405/_usb_plug_d405.urdf.xacro-6-7 (1)
6-7: Incorrect camera model in documentation comment.The comment references "Intel RealSense 415 camera" but this file defines the USB plug for the D405 camera model.
🔎 Proposed fix
-This is the URDF model for the Intel RealSense 415 camera, in it's -aluminum peripherial evaluation case. +This is the URDF model for the Intel RealSense D405 camera, in its +aluminum peripheral evaluation case.stretch3_ws/README.md-41-41 (1)
41-41: Fix typo: "isaccsim" → "isaacsim".🔎 Proposed fix
- - description of the stretch3 isaacsim environment, launch it with isaccsim and you can have a movable stretch3 in a clean playground + - description of the stretch3 isaacsim environment, launch it with isaacsim and you can have a movable stretch3 in a clean playgroundstretch3_ws/README.md-26-26 (1)
26-26: Fix typo: "modifiy" → "modification".🔎 Proposed fix
-5. Go to `File` -> `Save`, if you see `stretch_movable.usda` is marked `modified` by Git, you have saved your modifiy. +5. Go to `File` -> `Save`. If you see `stretch_movable.usda` marked as `modified` by Git, you have saved your modification.stretch3_ws/README.md-31-31 (1)
31-31: Fix command typo: "ros2 topc pub" → "ros2 topic pub".This typo will cause the command to fail when users copy and execute it.
🔎 Proposed fix
-ros2 topc pub /cmd_vel geometry_msgs/Twist "{'linear': {'x': 0.2, 'y': 0.0, 'z': 0.0}, 'angular': {'x': 0.0, 'y': 0.0, 'z': 0.0}}" +ros2 topic pub /cmd_vel geometry_msgs/Twist "{'linear': {'x': 0.2, 'y': 0.0, 'z': 0.0}, 'angular': {'x': 0.0, 'y': 0.0, 'z': 0.0}}"stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_usb_plug.urdf.xacro-2-8 (1)
2-8: Inconsistent comment: mentions RealSense 415 but file is for D435i.The license header references the "Intel RealSense 415 camera", but this file is in the
d435idirectory and is used by the D435i camera assembly. This appears to be a copy-paste artifact.🔎 Proposed fix
<!-- License: Apache 2.0. See LICENSE file in root directory. Copyright(c) 2017 Intel Corporation. All Rights Reserved -This is the URDF model for the Intel RealSense 415 camera, in it's -aluminum peripherial evaluation case. +This is the URDF model for the Intel RealSense D435i camera USB plug component. -->stretch3_ws/README.md-18-18 (1)
18-18: Improve grammar and clarity of the important note.The note has several grammar issues: "exist file" should be "existing files", and the phrasing is awkward. Consider rephrasing for clarity.
🔎 Proposed fix
-> the dependencies of the usda and usdz is very trick, some in install and some in src, **it is very important to not delete any exist file and keep the project structure** +> The dependencies of the USDA and USDZ files are tricky—some are in `install/` and some in `src/`. **It is crucial not to delete any existing files and to preserve the project structure.**stretch3_ws/src/isaacsim/model/assets/xacro/stretch_d435i.xacro-5-5 (1)
5-5: Passadd_plugparameter to thesensor_d435imacro or remove the unused argument.The
add_plugargument (line 5) is defined but never passed to thesensor_d435imacro. The underlyingsensor_d435macro in_d435.urdf.xacrosupports this parameter, but it's not wired throughsensor_d435i. Either addadd_plugto thesensor_d435imacro signature and pass it through tosensor_d435, or remove the argument from this file if it's not needed.stretch3_ws/src/isaacsim/launch/run_isaacsim.launch.py-64-68 (1)
64-68: Unused variablebash_commandsuggests incomplete implementation.The
bash_commandlist is constructed by iterating overparameters_to_checkbut is never used in the launch setup. This could indicate:
- Dead code that should be removed
- An incomplete implementation where
bash_commandwas intended for use withExecuteProcessbut the approach was changed to use aNodeinsteadPlease verify whether this variable serves a purpose or should be removed.
🔎 Proposed fix if unused
- bash_command = [] - - # Add parameters to the command list if they are set - for param in parameters_to_check: - bash_command = add_parameter_argument(param, bash_command) - # Run isaac sim as a ROS2 node with default parameters. Parameters can be overridden here or via launch arguments from other launch files. isaacsim_node = Node(stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435.urdf.xacro-7-8 (1)
7-8: Inconsistent camera model reference in comment.The comment mentions "Intel RealSense 430 camera" but this file defines the D435 model (as evidenced by variable names like
d435_cam_*and mesh filenamed435.dae).Suggested fix
-This is the URDF model for the Intel RealSense 430 camera, in its +This is the URDF model for the Intel RealSense D435 camera, in its aluminum peripherial evaluation case.stretch3_ws/src/isaacsim/scripts/open_isaacsim_stage.py-17-18 (1)
17-18: Incomplete help text for--start-on-playargument.The help text is truncated:
'If present, to true.'should be something like'If present, start the timeline playback when the stage loads.'Suggested fix
parser.add_argument('--start-on-play', action='store_true', - help='If present, to true.') + help='If present, start the timeline playback when the stage loads.')stretch3_ws/docker/Dockerfile-73-81 (1)
73-81: Invalid CUDA paths whenCUDA_TOOLKIT_VERSIONis empty.When
CUDA_TOOLKIT_VERSION=""(default), the PATH and LD_LIBRARY_PATH will include/usr/local/cuda-/binand/usr/local/cuda-/lib64, which are invalid paths. Consider conditionalizing or using a symlink.Suggested fix
# Ref: https://docs.nvidia.com/cuda/cuda-installation-guide-linux/#post-installation-actions -ENV PATH="${PATH}:/usr/local/cuda-${CUDA_TOOLKIT_VERSION}/bin" -ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/usr/local/cuda-${CUDA_TOOLKIT_VERSION}/lib64" +ENV PATH="${PATH}${CUDA_TOOLKIT_VERSION:+:/usr/local/cuda-${CUDA_TOOLKIT_VERSION}/bin}" +ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}${CUDA_TOOLKIT_VERSION:+:/usr/local/cuda-${CUDA_TOOLKIT_VERSION}/lib64}"This uses bash parameter expansion to only append the path when
CUDA_TOOLKIT_VERSIONis non-empty.stretch3_ws/src/isaacsim/scripts/run_isaacsim.py-128-129 (1)
128-129: Use non-zero exit code for error conditions.
sys.exit(0)indicates success. For unsupported versions or platform errors, usesys.exit(1)to signal failure.🔎 Proposed fix
else: print(f"Unsupported Isaac Sim version: {args.version}") - sys.exit(0) + sys.exit(1)if sys.platform == "win32": print("use_internal_libs parameter is not supported in Windows") - sys.exit(0) + sys.exit(1)Also applies to: 135-136
🧹 Nitpick comments (35)
stretch3_ws/src/isaacsim/CMakeLists.txt (1)
4-5: Unusedrclpydependency.The
find_package(rclpy REQUIRED)is declared but never used in this CMakeLists.txt. Since this is a pure CMake/ament package that only installs Python scripts and assets, this dependency can be removed unless it's intended for future use.🔎 Proposed fix
cmake_minimum_required(VERSION 3.5) project(isaacsim) find_package(ament_cmake REQUIRED) -find_package(rclpy REQUIRED) install(DIRECTORYstretch3_ws/src/isaacsim/model/assets/xacro/stretch_respeaker.xacro (2)
55-56: Zero axis vector for fixed joint.The axis
xyz="0 0 0"is a null vector. While the axis is ignored for fixed joints, specifying a zero vector is technically invalid per URDF conventions. Consider removing the axis element entirely or using a standard axis like0 0 1.🔎 Proposed fix
<child link="respeaker_base" /> - <axis - xyz="0 0 0" /> </joint>
58-62: Trailing whitespace and empty lines.Minor cleanup: there are trailing whitespace characters and unnecessary blank lines at the end of the file.
stretch3_ws/src/isaacsim/stretch.xml (1)
56-64: Misleading indentation in default class hierarchy.The indentation within the
<default class="wrist">block (lines 41-64) is inconsistent. The closing</default>tags forfinger_open(line 59) andfinger_slide(line 63) are at the same level as their opening tags, but the closing tag forwristat line 64 appears at a different indentation level, making the hierarchy hard to follow. Consider aligning the indentation consistently.stretch3_ws/src/isaacsim/package.xml (1)
12-17: Consider using a standard license identifier.The
<license>tag typically contains a short license identifier (e.g.,Apache-2.0,BSD-3-Clause, orProprietary) rather than the full license text. The full text is usually placed in a separate LICENSE file. This approach improves readability and tool compatibility.🔎 Proposed fix
- <license>Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. - NVIDIA CORPORATION and its licensors retain all intellectual property - and proprietary rights in and to this software, related documentation - and any modifications thereto. Any use, reproduction, disclosure or - distribution of this software and related documentation without an express - license agreement from NVIDIA CORPORATION is strictly prohibited.</license> + <license>NVIDIA Proprietary</license>stretch3_ws/src/isaacsim/model/assets/xacro/stretch_d405_sg3.xacro (1)
14-18: Consider removing trailing whitespace.Lines 14-18 contain unnecessary trailing whitespace and blank lines after the closing
</robot>tag.🔎 Proposed cleanup
</xacro:sensor_d405> - </robot> - - -stretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_tablet_12in.xacro (1)
14-18: Consider removing trailing whitespace.Lines 15-18 contain unnecessary trailing whitespace and blank lines after the closing
</robot>tag.🔎 Proposed cleanup
<xacro:include filename="stretch_head_nav_cam.xacro" /> </robot> - - -stretch3_ws/src/isaacsim/model/assets/xacro/stretch_base_imu.xacro (1)
54-55: Remove redundant axis attribute for fixed joints.The axis attribute is ignored for fixed joints and can be removed for cleaner URDF code. This pattern appears in other xacro files as well.
🔎 Suggested fix
<child link="base_imu" /> - <axis - xyz="0 0 0" /> </joint>stretch3_ws/docker/scripts/install_stretch.sh (1)
24-24: Consider pinning the hello-robot-stretch-body version.Installing without a version pin may lead to non-reproducible builds if the package is updated upstream. Consider specifying a version for consistency.
🔎 Example fix
-pip3 install hello-robot-stretch-body +pip3 install hello-robot-stretch-body==0.7.2 # Replace with desired versionstretch3_ws/src/isaacsim/model/assets/xacro/d405/_usb_plug_d405.urdf.xacro (2)
12-12: Consider using the built-in${pi}constant for consistency.The D435i IMU modules file (stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435i_imu_modules.urdf.xacro) uses
value="${pi}"instead of hardcoding the value, which is more idiomatic for Xacro files.🔎 Proposed refactor
- <xacro:property name="M_PI" value="3.1415926535897931" /> + <xacro:property name="M_PI" value="${pi}" />
25-26: Remove commented-out code.The commented-out box geometries on lines 25 and 33 should be removed unless they serve a specific purpose (e.g., debugging or future reference). If they are intentionally kept, add a comment explaining why.
Also applies to: 33-34
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_sg3_pro.xacro (1)
16-17: Remove trailing whitespace.Lines 16-17 contain trailing whitespace after the closing
</robot>tag, which should be removed for cleaner version control diffs.stretch3_ws/docker/.bashrc (1)
20-27: Add error handling for directory navigation.Line 24 navigates to
$ROS2_WSwithout checking if the directory exists or if the variable is set. If the directory doesn't exist, subsequent commands may fail or execute in an unexpected location.🔎 Proposed improvement
echo "rosdep update has not been executed yet. Running rosdep update..." rosdep update --rosdistro $ROS_DISTRO - cd $ROS2_WS + cd $ROS2_WS || { echo "Failed to navigate to $ROS2_WS"; return 1; } # Ref: https://docs.ros.org/en/humble/Tutorials/Intermediate/Rosdep.html rosdep install --from-paths src --ignore-src -y -rstretch3_ws/src/isaacsim/model/assets/xacro/stretch_tool_tablet_12in.xacro (2)
27-31: Consider providing a meaningful material name.The material element has an empty name attribute (line 28). While this may work for inline materials, providing a descriptive name improves readability and potential reusability.
🔎 Proposed improvement
<material - name=""> + name="tablet_surface"> <color rgba="0.79216 0.81961 0.93333 1" /> </material>
53-55: Remove unnecessary axis element.The
axiselement on line 54 has no effect on fixed joints and can be removed for cleaner code.🔎 Proposed simplification
<child link="link_DW3_tablet_12in" /> - <axis - xyz="0 0 0" /> </joint>stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_usb_plug.urdf.xacro (1)
25-25: Consider removing commented-out box geometry.The commented
<box>elements in the visual and collision sections appear to be unused. If these are kept for reference (showing the mesh dimensions), consider adding an explanatory comment; otherwise, remove them to reduce clutter.Also applies to: 33-33
stretch3_ws/README.md (1)
9-9: Consider adding language identifiers to code blocks.Multiple code blocks are missing language identifiers (flagged by markdownlint MD040). Adding identifiers like
bash` orpython` improves syntax highlighting and accessibility.Example for line 9:
🔎 Suggested improvement
-``` +```bash docker compose build docker compose up -d docker exec -it ros2-stretch3-ws bash -``` +````Also applies to: 15-15, 21-21, 30-30, 35-35, 49-49, 53-53
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_laser_range_finder.xacro (1)
55-56: Consider removing the redundant axis element.For
fixedjoint types, the<axis>element is ignored by URDF parsers. Specifyingxyz="0 0 0"is redundant and can be removed for cleaner URDF.🔎 Proposed cleanup
<child link="laser" /> - <axis - xyz="0 0 0" /> </joint>stretch3_ws/src/isaacsim/model/assets/xacro/stretch_d435i.xacro (1)
19-23: Remove trailing blank lines.Multiple blank lines after the closing
</robot>tag are unnecessary and should be removed for cleaner formatting.🔎 Proposed cleanup
</xacro:sensor_d435i> - </robot> - - -stretch3_ws/src/isaacsim/model/assets/xacro/stretch_wrist_dw3.xacro (1)
44-56: Remove redundant axis definitions from fixed joints.The
axiselement is ignored for fixed joints since they have no degrees of freedom. While not incorrect, removing these lines improves clarity and reduces clutter.🔎 Proposed fix
<joint name="joint_wrist_yaw_bottom" type="fixed"> <origin xyz="0 0 0" rpy="-3.14159265358979 0 -1.5707963267949" /> <parent link="link_wrist_yaw" /> <child link="link_wrist_yaw_bottom" /> - <axis - xyz="0 0 0" /> </joint>Apply the same change to
joint_wrist_quick_connectat lines 170-182.Also applies to: 170-182
stretch3_ws/src/isaacsim/launch/run_isaacsim.launch.py (1)
51-62: Consider using iterable unpacking for better performance.The static analysis tool suggests using iterable unpacking instead of list concatenation, which can be more efficient and Pythonic.
🔎 Proposed fix
def add_parameter_argument(param_name, command_list): param_value = LaunchConfiguration(param_name).perform(context) # Here you'll get the runtime config value if param_value != '': if param_value == 'true': - return command_list + ['--' + param_name.replace('_', '-')] + return [*command_list, '--' + param_name.replace('_', '-')] elif param_value == 'false': return command_list else: - return command_list + ['--' + param_name.replace('_', '-'), param_value,] + return [*command_list, '--' + param_name.replace('_', '-'), param_value] else: return command_liststretch3_ws/src/isaacsim/scripts/open_isaacsim_stage.py (4)
20-24: Catching broadExceptionmasks argument parsing errors.Using a blanket
Exceptioncatch here will swallow all argument parsing errors (includingSystemExitfrom--help), potentially hiding issues.argparse.parse_args()raisesSystemExitfor help/error, which should propagate normally.Suggested fix
- try: - options = parser.parse_args() - except Exception as e: - carb.log_error(str(e)) - return + options = parser.parse_args()
35-40: Silent exception swallowing hides potential import/initialization failures.The
try-except-passpattern silently ignores any failure when checking for live sessions. Consider logging a debug message to aid troubleshooting.Suggested fix
try: import omni.kit.usd.layers as layers live_session_name = layers.get_live_session_name_from_shared_link(path) is_stage_with_session = live_session_name is not None - except Exception: - pass + except Exception as e: + carb.log_info(f"Live session check unavailable: {e}")
51-57: Redundantpassstatement afterThe
passon line 57 is unreachable/unnecessary.Suggested fix
if timeline_interface is not None: await omni.kit.app.get_app().next_update_async() await omni.kit.app.get_app().next_update_async() timeline_interface.play() print("Stage loaded and simulation is playing.") - pass
67-72: Broad exception catch when importing nucleus connector.Catching
Exceptionhere is acceptable for optional functionality, but logging the actual error would help diagnose connection issues.Suggested fix
try: from omni.kit.widget.nucleus_connector import get_nucleus_connector nucleus_connector = get_nucleus_connector() - except Exception: - carb.log_warn("Open stage: Could not import Nucleus connector.") + except Exception as e: + carb.log_warn(f"Open stage: Could not import Nucleus connector: {e}") returnstretch3_ws/src/isaacsim/model/assets/xacro/stretch_tool_sg3.xacro (1)
3-3: Unused propertyscale_finger_length.The property
scale_finger_lengthis declared but never referenced in the file. Remove it or apply it where intended.stretch3_ws/src/stretch3_bringup/launch/stretch3_bringup.launch.py (1)
29-31: Fragile path resolution using relative traversal.The path calculation assumes a specific directory structure (
../../../../../) which is brittle and will break if the file is moved or the workspace layout changes. The commented-outFindPackageShareapproach (line 28) is more robust for installed packages.Consider using
FindPackageShareafter ensuring the model files are installed viaCMakeLists.txt, or useget_package_share_directory("isaacsim")for source builds.stretch3_ws/src/isaacsim/scripts/run_isaacsim.py (3)
44-50: Consider adding error handling for process termination.The shutdown function doesn't handle the case where a process may have already terminated, which could raise
ProcessLookupErrororOSError.🔎 Proposed fix with error handling
def isaac_sim_shutdown(): for proc_id in subprocesses: - if sys.platform == "win32": - os.kill(proc_id, signal.SIGTERM) - else: - os.killpg(os.getpgid(proc_id), signal.SIGKILL) + try: + if sys.platform == "win32": + os.kill(proc_id, signal.SIGTERM) + else: + os.killpg(os.getpgid(proc_id), signal.SIGKILL) + except (ProcessLookupError, OSError): + pass # Process already terminated print("All subprocesses terminated.")
55-59: Version parsing may crash on malformed version strings.If a version string contains non-numeric parts (e.g., "4.2.0-beta"),
int()will raiseValueError. Consider adding validation or using a robust version parsing library.🔎 Proposed fix with error handling
def version_ge(v1, v2): - return tuple(map(int, (v1.split(".")))) >= tuple(map(int, (v2.split(".")))) + try: + return tuple(map(int, (v1.split(".")))) >= tuple(map(int, (v2.split(".")))) + except ValueError: + return False def version_gt(v1, v2): - return tuple(map(int, (v1.split(".")))) > tuple(map(int, (v2.split(".")))) + try: + return tuple(map(int, (v1.split(".")))) > tuple(map(int, (v2.split(".")))) + except ValueError: + return False
64-66: Use idiomaticnot inoperator.Per static analysis hint (E713), use
not infor membership tests.🔎 Proposed fix
for path in env_var_value.split(os.pathsep): - if not version_to_remove in path and not path.startswith(specified_path_to_remove): + if version_to_remove not in path and not path.startswith(specified_path_to_remove): new_env_var_value.append(path)stretch3_ws/src/isaacsim/model/assets/xacro/stretch_aruco.xacro (4)
115-130: Zero mass may cause physics simulation instability.
link_aruco_shoulder,link_aruco_top_wrist, andlink_aruco_inner_wristhavemass value="0". While this may be intentional for lightweight marker holders, some physics engines (including Isaac Sim/PhysX) may behave unexpectedly with zero-mass bodies. Consider using a very small mass value (e.g., 0.001 kg) if these links participate in dynamics.
55-57: Axis element is redundant for fixed joints.For fixed joints, the
<axis xyz="0 0 0" />element has no effect since fixed joints don't rotate or translate. This is harmless but can be removed for cleaner URDF.Also applies to: 111-113
280-283: Remove trailing empty lines.There are extra blank lines after the closing
</robot>tag.
24-27: Consider usingpackage://URIs for better portability and robustness.The mesh paths use relative notation (
./meshes/link_aruco_*.STL). While these resolve correctly in the Isaac Sim framework context (which sets the working directory to the package's model/assets folder), usingpackage://isaacsim/model/assets/meshes/link_aruco_*.STLwould be more explicit and portable across different execution contexts. This applies to both lines 24-27 and 39-41.stretch3_ws/src/isaacsim/model/stretch_movable.usda (1)
44-59: Duplicate DistantLight definitions.There are two DistantLight definitions with identical settings:
/World/DistantLight(lines 44-59)/Environment/defaultLight(lines 614-629)Both have
intensity = 3000and identical orientations. This may cause over-lighting. Consider removing one if redundant.Also applies to: 614-629
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
stretch3_ws/src/isaacsim/model/assets/meshes/d435.daeis excluded by!**/*.daestretch3_ws/src/isaacsim/model/assets/xacro/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (132)
stretch3_ws/.gitignorestretch3_ws/README.mdstretch3_ws/docker/.bashrcstretch3_ws/docker/.dockerignorestretch3_ws/docker/Dockerfilestretch3_ws/docker/compose.yamlstretch3_ws/docker/cyclonedds.xmlstretch3_ws/docker/scripts/install_stretch.shstretch3_ws/src/isaacsim/CMakeLists.txtstretch3_ws/src/isaacsim/launch/run_isaacsim.launch.pystretch3_ws/src/isaacsim/model/assets/meshes/base_imu.STLstretch3_ws/src/isaacsim/model/assets/meshes/base_imu_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/base_link.STLstretch3_ws/src/isaacsim/model/assets/meshes/base_link_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/d405.stlstretch3_ws/src/isaacsim/model/assets/meshes/laser.STLstretch3_ws/src/isaacsim/model/assets/meshes/laser_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/laser_collision_old.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_tablet_12in.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_tablet_12in_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_wrist_pitch.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_wrist_pitch_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_wrist_quick_connect.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_wrist_roll.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_wrist_roll_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_wrist_yaw_bottom.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_DW3_wrist_yaw_bottom_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SE3_head_nav_cam.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SE3_head_nav_cam_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SE3_head_nav_cam_collision_old.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_aruco_d405.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_aruco_fingertip_left.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_aruco_fingertip_right.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_body.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_body_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_body_new.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_body_old.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_finger_left.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_finger_left_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_finger_right.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_finger_right_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_fingertip_left.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_fingertip_left_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_fingertip_right.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_SG3_gripper_fingertip_right_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l0.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l0_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l1.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l1_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l2.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l2_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l3.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l3_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l4.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_arm_l4_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_inner_wrist.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_inner_wrist_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_left_base.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_left_base_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_right_base.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_right_base_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_shoulder.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_shoulder_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_top_wrist.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_aruco_top_wrist_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_camera.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_camera_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_ground_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_head.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_head_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_head_pan.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_head_pan_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_head_tilt.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_head_tilt_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_left_wheel.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_left_wheel_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_lift.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_lift_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_mast.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_mast_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_puller.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_respeaker.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_right_wheel.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_right_wheel_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_wrist_yaw.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_wrist_yaw_bottom.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_wrist_yaw_bottom_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/link_wrist_yaw_collision.STLstretch3_ws/src/isaacsim/model/assets/meshes/omni_wheel_m.STLstretch3_ws/src/isaacsim/model/assets/meshes/plug.stlstretch3_ws/src/isaacsim/model/assets/meshes/plug_collision.stlstretch3_ws/src/isaacsim/model/assets/meshes/respeaker_base.STLstretch3_ws/src/isaacsim/model/assets/meshes/respeaker_base_collision.STLstretch3_ws/src/isaacsim/model/assets/xacro/d405/_d405.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/d405/_materials_d405.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/d405/_usb_plug_d405.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435i.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435i_imu_modules.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/d435i/_materials.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/d435i/_usb_plug.urdf.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_aruco.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_base_imu.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_d405_sg3.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_d435i.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_nil.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_sg3.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_sg3_pro.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_tablet_12in.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_head_nav_cam.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_laser_range_finder.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_main.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_respeaker.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_tool_sg3.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_tool_tablet_12in.xacrostretch3_ws/src/isaacsim/model/assets/xacro/stretch_wrist_dw3.xacrostretch3_ws/src/isaacsim/model/stretch.project.usdzstretch3_ws/src/isaacsim/model/stretch_description_SE3_eoa_wrist_dw3_tool_sg3.urdfstretch3_ws/src/isaacsim/model/stretch_description_SE3_eoa_wrist_dw3_tool_sg3/configuration/stretch_description_SE3_eoa_wrist_dw3_tool_sg3_base.usdstretch3_ws/src/isaacsim/model/stretch_description_SE3_eoa_wrist_dw3_tool_sg3/configuration/stretch_description_SE3_eoa_wrist_dw3_tool_sg3_physics.usdstretch3_ws/src/isaacsim/model/stretch_description_SE3_eoa_wrist_dw3_tool_sg3/configuration/stretch_description_SE3_eoa_wrist_dw3_tool_sg3_robot.usdstretch3_ws/src/isaacsim/model/stretch_description_SE3_eoa_wrist_dw3_tool_sg3/configuration/stretch_description_SE3_eoa_wrist_dw3_tool_sg3_sensor.usdstretch3_ws/src/isaacsim/model/stretch_description_SE3_eoa_wrist_dw3_tool_sg3/stretch_description_SE3_eoa_wrist_dw3_tool_sg3.usdstretch3_ws/src/isaacsim/model/stretch_movable.usdastretch3_ws/src/isaacsim/package.xmlstretch3_ws/src/isaacsim/scripts/open_isaacsim_stage.pystretch3_ws/src/isaacsim/scripts/run_isaacsim.pystretch3_ws/src/isaacsim/stretch.xmlstretch3_ws/src/stretch3_bringup/CMakeLists.txtstretch3_ws/src/stretch3_bringup/launch/stretch3_bringup.launch.pystretch3_ws/src/stretch3_bringup/package.xmlstretch3_ws/src/stretch3_bringup/scripts/.gitkeep
🧰 Additional context used
🧬 Code graph analysis (3)
stretch3_ws/src/stretch3_bringup/launch/stretch3_bringup.launch.py (1)
stretch3_ws/src/isaacsim/launch/run_isaacsim.launch.py (1)
generate_launch_description(92-96)
stretch3_ws/src/isaacsim/scripts/run_isaacsim.py (1)
stretch3_ws/src/isaacsim/scripts/open_isaacsim_stage.py (1)
main(11-26)
stretch3_ws/src/isaacsim/launch/run_isaacsim.launch.py (1)
stretch3_ws/src/stretch3_bringup/launch/stretch3_bringup.launch.py (1)
generate_launch_description(19-57)
🪛 Hadolint (2.14.0)
stretch3_ws/docker/Dockerfile
[error] 145-145: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 165-165: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🪛 LanguageTool
stretch3_ws/README.md
[style] ~18-~18: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ome in install and some in src, **it is very important to not delete any exist file and keep t...
(EN_WEAK_ADJECTIVE)
[grammar] ~41-~41: Ensure spelling is correct
Context: ...h3 isaacsim environment, launch it with isaccsim and you can have a movable stretch3 in ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
stretch3_ws/README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
stretch3_ws/src/isaacsim/scripts/open_isaacsim_stage.py
22-22: Do not catch blind exception: Exception
(BLE001)
39-40: try-except-pass detected, consider logging the exception
(S110)
39-39: Do not catch blind exception: Exception
(BLE001)
70-70: Do not catch blind exception: Exception
(BLE001)
stretch3_ws/src/isaacsim/scripts/run_isaacsim.py
39-39: Unused function argument: sig
(ARG001)
39-39: Unused function argument: frame
(ARG001)
65-65: Test for membership should be not in
Convert to not in
(E713)
148-148: subprocess call with shell=True identified, security issue
(S602)
151-151: subprocess call with shell=True identified, security issue
(S602)
159-159: subprocess call with shell=True identified, security issue
(S602)
176-176: subprocess call with shell=True identified, security issue
(S602)
stretch3_ws/src/isaacsim/launch/run_isaacsim.launch.py
56-56: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
60-60: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🔇 Additional comments (53)
stretch3_ws/src/isaacsim/CMakeLists.txt (1)
7-18: Scripts directory installed twice with different mechanisms.The
scriptsdirectory is installed viainstall(DIRECTORY ...)toshare/${PROJECT_NAME}, and two specific Python files from the same directory are also installed viainstall(PROGRAMS ...)tolib/${PROJECT_NAME}. This is intentional and correct: thePROGRAMSinstall ensures the executables are placed in the package'slibdirectory with proper execute permissions forros2 run, while theDIRECTORYinstall makes them available as resources.stretch3_ws/src/isaacsim/stretch.xml (2)
1-10: MuJoCo simulation configuration looks reasonable.The simulation options configure implicit fast integration with elliptic cones and multi-CCD enabled. The commented-out alternative with
noslip_iterations="3"is retained for reference, which is helpful for tuning. The constraint buffer sizes (njmax,nconmax= 5000) should be sufficient for this robot model.
537-540: Verify keyframe control values match actuator order.The keyframes define 10 control values. Ensure these correspond to the actuator order:
left_wheel_vel,right_wheel_vel,lift,arm,wrist_yaw,wrist_pitch,wrist_roll,gripper,head_pan,head_tilt. For example, the "home" keyframe sets lift=0.6 and arm=0.1, and "stow" sets lift=0.23 and wrist_yaw=3.14.stretch3_ws/.gitignore (1)
1-11: LGTM!Standard
.gitignorepatterns for a ROS 2 workspace. The inclusion of/devel(ROS 1 artifact) alongside/install(ROS 2 artifact) provides backward compatibility if anyone accidentally uses catkin tools.stretch3_ws/docker/cyclonedds.xml (1)
5-24: LGTM!The CycloneDDS configuration is well-suited for containerized ROS 2 environments. The
autodeterminenetwork interface selection, default multicast settings, and 65500B max message size (near UDP limit) are appropriate defaults. TheEnableTopicDiscoveryEndpointsis useful for debugging and topic introspection.stretch3_ws/src/isaacsim/model/assets/xacro/d405/_materials_d405.urdf.xacro (1)
11-17: LGTM!The material definitions are clean and appropriately encapsulated for reuse across multiple D405 camera instances. Using a separate fragment file prevents material redefinition errors.
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_respeaker.xacro (1)
25-26: No changes needed. The relative mesh path./meshes/respeaker_base.STLis the established convention used consistently across all xacro files in this project. The meshes directory is properly co-located alongside the xacro files in the directory structure, making relative paths appropriate for this codebase.Likely an incorrect or invalid review comment.
stretch3_ws/docker/.dockerignore (1)
1-4: LGTM!The ignore-all-then-whitelist pattern is correct and follows Docker best practices.
stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_materials.urdf.xacro (1)
11-18: LGTM!The material definitions follow standard URDF syntax and use appropriate color values for aluminum and plastic materials.
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_d405_sg3.xacro (2)
8-12: LGTM!The sensor macro instantiation is properly configured with appropriate parent link and origin values.
4-5: Verify the unusedadd_plugargument.The
add_plugargument is declared on line 5 but appears to be unused in this file. Confirm whether this should be passed to thesensor_d405macro instantiation or if it's dead code that should be removed.stretch3_ws/src/stretch3_bringup/CMakeLists.txt (3)
1-11: LGTM!The project setup and compiler configuration follow ROS 2 best practices.
19-22: LGTM!The installation rules follow standard ROS 2 conventions for launch file packages.
13-17: Remove unnecessary dependenciesrclcppandrclpy.The package declares
rclcppandrclpyas build dependencies but neither is used in the codebase. The launch file only uses the launch framework andament_index_python, and the scripts directory is empty. Remove these unused dependencies from bothCMakeLists.txtandpackage.xml.Consider also removing
ament_cmake_pythonif no Python modules are being built or installed.stretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_tablet_12in.xacro (1)
1-13: LGTM!The modular composition approach follows good practices for robot description maintainability. The included fragments are logically organized.
stretch3_ws/docker/scripts/install_stretch.sh (1)
1-42: Script structure looks reasonable for reference/disabled status.The installation flow is well-organized with good error checking (especially the portaudio.h verification). Since the PR description notes this script is "disabled but retained," keeping it as-is for reference purposes makes sense.
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_sg3.xacro (1)
4-13: All included xacro files verified to exist.The aggregator structure is consistent with the nil variant and appropriately includes sg3-specific components. All referenced files—including the tool-specific stretch_d405_sg3.xacro and stretch_tool_sg3.xacro—are present in the xacro directory.
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_description_SE3_eoa_wrist_dw3_tool_nil.xacro (1)
4-11: All referenced xacro files exist and are properly structured.Verification confirms all 8 included xacro files exist at the expected paths in the same directory. The aggregator structure is clean and appropriate for the "nil" (no tool) variant with no missing dependencies.
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_base_imu.xacro (1)
10-11: Verify the mass value against Stretch specifications.The mass is specified as 0.00092 kg (0.92 grams). While this is light, typical IMU breakout modules weigh 1-5 grams (not 5-20 grams as previously stated), making 0.92 grams plausible for a minimal sensor mount. However, verify this value against the actual Stretch robot's base IMU specifications (FXOS8700 + FXAS21002) to confirm it is correct.
stretch3_ws/docker/.bashrc (3)
1-14: LGTM!The PATH setup and ROS 2 environment sourcing follow standard best practices.
15-19: Consider edge cases in the apt lists check.The check
[ -z "$( ls -A '/var/lib/apt/lists' )" ]will return true if the directory is empty, but may have edge cases if the directory structure changes. This is generally acceptable for a development container setup.
41-49: LGTM!The architecture-specific Gazebo sourcing and final workspace sourcing are implemented correctly.
stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435i_imu_modules.urdf.xacro (1)
1-46: LGTM!The D435i IMU modules macro is well-structured and follows Xacro best practices, including the use of the built-in
${pi}constant for M_PI.stretch3_ws/src/isaacsim/model/assets/xacro/stretch_laser_range_finder.xacro (1)
4-43: LGTM!The laser link definition is well-structured with appropriate inertial properties, visual mesh, and collision geometry. The empty material name is standard practice for inline material definitions in URDF.
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_d435i.xacro (1)
8-17: LGTM!The sensor instantiation is well-documented with coordinate system comments and correctly configured with the parent link and origin transformation.
stretch3_ws/src/stretch3_bringup/package.xml (1)
1-22: LGTM!The package manifest is well-formed and follows ROS 2 conventions. The version
0.0.0is appropriate for a work-in-progress package, and the dependencies (ament_cmake, ament_cmake_python, rclcpp, rclpy) are suitable for a bringup package.stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435i.urdf.xacro (1)
1-21: LGTM!The Xacro macro is well-structured and correctly composes the D435 camera base with IMU modules. The parameter passing and origin insertion are properly implemented.
stretch3_ws/src/isaacsim/model/assets/xacro/d405/_d405.urdf.xacro (1)
1-165: LGTM!The D405 camera Xacro macro is well-structured with:
- Proper parametrization for flexible integration
- Conditional blocks for nominal extrinsics and optional USB plug
- Clear documentation referencing the datasheet
- Complete camera frame hierarchy (depth, infrared, color)
The implementation follows ROS URDF/Xacro best practices.
stretch3_ws/src/isaacsim/model/stretch_description_SE3_eoa_wrist_dw3_tool_sg3.urdf (2)
87-124: Zero-friction caster configuration is appropriate.The caster link correctly specifies zero friction coefficients (
mu=0,mu2=0) with full slip parameters (slip1=1.0,slip2=1.0), which is appropriate for an omni-directional caster wheel. This aligns with the PR description noting the zero-friction material setting for the omni wheel.
2-5: No action required. Generated URDF files are intentionally version-controlled as part of the project's asset distribution. The xacro sources exist alongside pre-generated URDFs, and the build process (CMakeLists.txt) installs them as-is without regeneration, confirming this is the project's design choice rather than a maintenance issue.stretch3_ws/src/isaacsim/launch/run_isaacsim.launch.py (1)
44-47: The exclusion of 'headless' and 'custom_args' from parameters_to_check is intentional and correct.Both parameters are properly passed to the ROS node (lines 84-85) and are handled separately in
run_isaacsim.py:headlessis used to select the executable script (line 165), andcustom_argsis appended verbatim to the command (line 169). Unlike the parameters inparameters_to_check, which are converted to standard--parameter=valuebash arguments, these require special handling and should not be in that list.Likely an incorrect or invalid review comment.
stretch3_ws/src/isaacsim/model/assets/xacro/d435i/_d435.urdf.xacro (1)
16-158: LGTM!The
sensor_d435macro is well-structured with proper parameterization for mesh usage, nominal extrinsics, and USB plug options. The property calculations for camera geometry offsets are documented with datasheet references.stretch3_ws/src/isaacsim/model/assets/xacro/stretch_tool_sg3.xacro (3)
97-114: Revolute joint effectively locked with zero range.
joint_gripper_finger_righthaslower="0" upper="0"with zero effort and velocity, making it functionally a fixed joint. If the gripper fingers are intended to be movable, update the limits; otherwise, consider usingtype="fixed"for clarity.
261-278: Same concern for left finger joint.
joint_gripper_finger_leftalso haslower="0" upper="0". Ensure this is intentional for simulation purposes.
438-451: LGTM!The
link_grasp_centerand its fixed joint provide a useful reference frame for grasp planning, correctly positioned relative to the gripper body.stretch3_ws/src/stretch3_bringup/launch/stretch3_bringup.launch.py (1)
36-51: LGTM!The
IncludeLaunchDescriptionis correctly configured with the condition to only launch Isaac Sim whenplatform == 'isaacsim'. The launch arguments are appropriately passed.stretch3_ws/docker/Dockerfile (3)
115-117: Hardcoded Omniverse credentials in image.
OMNI_USER=adminandOMNI_PASS=adminare baked into the image. While this may be acceptable for local development, consider documenting this or using environment variable overrides at runtime for different deployment scenarios.
145-148: UsingsudoafterUSERswitch is flagged by Hadolint but acceptable here.Hadolint flags
sudousage (DL3004) as it can lead to unpredictable behavior. However, for development containers where the non-root user needs to install packages, this is a common and accepted pattern. TheNOPASSWDconfiguration on line 92 supports this use case.Also applies to: 165-180
182-188: Commented-out Stretch installation block.The
install_stretch.shscript is commented out. If this is intentional for WIP, consider adding a note explaining why, or remove if not needed.stretch3_ws/src/isaacsim/model/assets/xacro/stretch_main.xacro (2)
90-103: LGTM!Wheel joints are properly configured as continuous with appropriate dynamics parameters for damping, friction, and spring properties.
Also applies to: 146-159
308-324: LGTM!The
joint_liftprismatic joint is correctly configured with appropriate limits. The inline comments explaining the range adjustments are helpful.stretch3_ws/docker/compose.yaml (4)
49-50:privileged: truegrants full host access.The comment says to disable if not needed, but leaving it enabled poses security risks. Consider using more granular capabilities or device mounts only for what's actually required (e.g.,
/dev/ttyUSB*for serial devices).
175-184: External volumes must be pre-created.The volumes
gazebo-cache,isaac-sim-cache, andisaac-ros-assetsare marked asexternal: true, meaning they must exist before runningdocker compose up. Ensure the README documents the required volume creation commands:docker volume create ros2-gazebo-cache docker volume create ros2-isaac-sim-cache docker volume create ros2-isaac-ros-assets
3-16: LGTM!The
volume-instantiationservice correctly prepares the cache directories with proper ownership before the main service starts. Usingdepends_onensures correct ordering.
117-171: LGTM!The volume mounts with subpaths for Isaac Sim caches are comprehensive and follow the official container deployment documentation structure.
stretch3_ws/src/isaacsim/scripts/run_isaacsim.py (3)
39-42: LGTM on signal handler.The
sigandframeparameters are required by thesignal.signalcallback signature, so the static analysis warning about unused arguments can be safely ignored.
157-160: Shell injection risk with user-provided paths.While the static analyzer flags
shell=True(S602), the actual risk here is thatargs.standalone,args.custom_args, andargs.guiare constructed from ROS parameters which could contain shell metacharacters. If these values come from untrusted sources, this could lead to command injection.For a development/simulation environment this may be acceptable, but consider using
shlex.split()and avoidingshell=Trueif security is a concern.Also applies to: 176-177
179-186: LGTM on main function structure.The ROS2 lifecycle is properly initialized, spun, and cleaned up. The shutdown handler ensures subprocesses are terminated.
stretch3_ws/src/isaacsim/model/assets/xacro/stretch_aruco.xacro (1)
1-43: LGTM on link and joint structure.The
link_aruco_right_baselink and its corresponding fixed joint follow standard URDF conventions with proper inertial, visual, and collision elements.stretch3_ws/src/isaacsim/model/stretch_movable.usda (4)
1-40: LGTM on USD scene metadata.The scene is properly configured with metersPerUnit = 1, Z-up axis, and appropriate time code settings for simulation.
586-604: Verify if the Sphere object is intentional.A physics-enabled
Spherewith ZeroFriction material is defined at a specific position near the robot. The PR description mentions "removing a test cube" - verify if this sphere is also a test artifact that should be removed, or if it's intentional for the simulation.
344-576: Two differential controller graphs may conflict.There are two OmniGraph definitions for robot control:
RosActionGraph(lines 344-576) - Subscribes to ROS2 Twist messagesGraphs/differential_controller(lines 697-984) - Keyboard WASD controlBoth target the same articulation (
/World/stretch/base_link) and control the same wheel joints. If both are active simultaneously, they may issue conflicting velocity commands. Ensure only one control mode is active at a time, or implement proper command arbitration.Also applies to: 697-984
476-481: LGTM on ROS differential controller configuration.The ROS2-connected differential controller has correct wheel parameters (wheelDistance = 0.341m, wheelRadius = 0.05m) matching typical Stretch robot specifications.
| # Optionally build the workspace if it has not been built yet | ||
| if [ ! -f $ROS2_WS/install/setup.bash ]; then | ||
| echo "Workspace has not been built yet. Building workspace..." | ||
| cd $ROS2_WS | ||
| # TODO: If command `arch` outputs `aarch64`, consider adding `--packages-ignore <package>` to ignore x86 packages | ||
| # Ref: https://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Creating-Your-First-ROS2-Package.html | ||
| if [ $(arch) == "aarch64" ]; then | ||
| colcon build --symlink-install | ||
| else | ||
| colcon build --symlink-install | ||
| fi | ||
| echo "Workspace built." | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify redundant conditional logic.
The if/else block on lines 34-38 has identical commands in both branches, making the conditional unnecessary. Either implement the architecture-specific package ignoring mentioned in the TODO comment, or simplify to a single build command.
🔎 Proposed simplification
echo "Workspace has not been built yet. Building workspace..."
- cd $ROS2_WS
+ cd $ROS2_WS || { echo "Failed to navigate to $ROS2_WS"; return 1; }
# TODO: If command `arch` outputs `aarch64`, consider adding `--packages-ignore <package>` to ignore x86 packages
# Ref: https://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Creating-Your-First-ROS2-Package.html
- if [ $(arch) == "aarch64" ]; then
- colcon build --symlink-install
- else
- colcon build --symlink-install
- fi
+ colcon build --symlink-install
echo "Workspace built."If architecture-specific handling is needed, implement it properly:
echo "Workspace has not been built yet. Building workspace..."
- cd $ROS2_WS
+ cd $ROS2_WS || { echo "Failed to navigate to $ROS2_WS"; return 1; }
# TODO: If command `arch` outputs `aarch64`, consider adding `--packages-ignore <package>` to ignore x86 packages
# Ref: https://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Creating-Your-First-ROS2-Package.html
if [ $(arch) == "aarch64" ]; then
- colcon build --symlink-install
+ colcon build --symlink-install --packages-ignore <x86_only_package>
else
colcon build --symlink-install
fi
echo "Workspace built."📝 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.
| # Optionally build the workspace if it has not been built yet | |
| if [ ! -f $ROS2_WS/install/setup.bash ]; then | |
| echo "Workspace has not been built yet. Building workspace..." | |
| cd $ROS2_WS | |
| # TODO: If command `arch` outputs `aarch64`, consider adding `--packages-ignore <package>` to ignore x86 packages | |
| # Ref: https://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Creating-Your-First-ROS2-Package.html | |
| if [ $(arch) == "aarch64" ]; then | |
| colcon build --symlink-install | |
| else | |
| colcon build --symlink-install | |
| fi | |
| echo "Workspace built." | |
| fi | |
| # Optionally build the workspace if it has not been built yet | |
| if [ ! -f $ROS2_WS/install/setup.bash ]; then | |
| echo "Workspace has not been built yet. Building workspace..." | |
| cd $ROS2_WS || { echo "Failed to navigate to $ROS2_WS"; return 1; } | |
| # TODO: If command `arch` outputs `aarch64`, consider adding `--packages-ignore <package>` to ignore x86 packages | |
| # Ref: https://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Creating-Your-First-ROS2-Package.html | |
| colcon build --symlink-install | |
| echo "Workspace built." | |
| fi |
🤖 Prompt for AI Agents
In stretch3_ws/docker/.bashrc around lines 28 to 40, the if/else on lines 34-38
runs the exact same colcon build in both branches, so remove the redundant
conditional and replace it with a single colcon build --symlink-install executed
unconditionally (or, if you intend to honor the TODO, implement the arch check
to add --packages-ignore <package> for aarch64 before invoking colcon); ensure
you keep the surrounding workspace existence check and messages intact.
| <link name="caster_link"> | ||
| <collision> | ||
| <geometry> | ||
| <sphere radius="0.032"/> | ||
| </geometry> | ||
| <origin rpy="0 0 0" xyz="0 0 0"/> | ||
| <surface> | ||
| <friction> | ||
| <ode> | ||
| <mu>0</mu> | ||
| <mu2>0</mu2> | ||
| <slip1>1.0</slip1> | ||
| <slip2>1.0</slip2> | ||
| </ode> | ||
| </friction> | ||
| </surface> | ||
| </collision> |
There was a problem hiding this comment.
Invalid URDF syntax: <surface> element inside <collision>.
The <surface><friction><ode> block is Gazebo/SDF syntax, not valid URDF. URDF <collision> elements only support <origin> and <geometry> children. This block will be ignored by standard URDF parsers and may cause validation errors.
For Gazebo friction properties, use a <gazebo> extension tag:
Suggested fix
<link name="caster_link">
<collision>
<geometry>
<sphere radius="0.032"/>
</geometry>
<origin rpy="0 0 0" xyz="0 0 0"/>
- <surface>
- <friction>
- <ode>
- <mu>0</mu>
- <mu2>0</mu2>
- <slip1>1.0</slip1>
- <slip2>1.0</slip2>
- </ode>
- </friction>
- </surface>
</collision>
<!-- ... visual and inertial ... -->
</link>
+
+ <gazebo reference="caster_link">
+ <mu1>0</mu1>
+ <mu2>0</mu2>
+ <slip1>1.0</slip1>
+ <slip2>1.0</slip2>
+ </gazebo>📝 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.
| <link name="caster_link"> | |
| <collision> | |
| <geometry> | |
| <sphere radius="0.032"/> | |
| </geometry> | |
| <origin rpy="0 0 0" xyz="0 0 0"/> | |
| <surface> | |
| <friction> | |
| <ode> | |
| <mu>0</mu> | |
| <mu2>0</mu2> | |
| <slip1>1.0</slip1> | |
| <slip2>1.0</slip2> | |
| </ode> | |
| </friction> | |
| </surface> | |
| </collision> | |
| <link name="caster_link"> | |
| <collision> | |
| <geometry> | |
| <sphere radius="0.032"/> | |
| </geometry> | |
| <origin rpy="0 0 0" xyz="0 0 0"/> | |
| </collision> |
🤖 Prompt for AI Agents
In stretch3_ws/src/isaacsim/model/assets/xacro/stretch_main.xacro around lines
161-177, the <surface>...<friction><ode> block is invalid inside a URDF
<collision> element; remove that entire <surface> block from the <collision> and
instead add a Gazebo extension element after the link (e.g., a <gazebo> tag
referencing "caster_link") containing the equivalent surface->friction->ode
configuration so the friction parameters are applied by Gazebo without breaking
URDF syntax.
| } | ||
|
|
||
| def "flat_plane" ( | ||
| prepend payload = @../../../install/isaacsim/share/isaacsim/model/stretch.project.usdz[SubUSDs/flat_plane.usd]@ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the referenced USD files exist and explore the directory structure
echo "=== Searching for stretch.project.usdz ==="
fd -t f "stretch.project.usdz" stretch3_ws/
echo ""
echo "=== Searching for flat_plane.usd ==="
fd -t f "flat_plane.usd" stretch3_ws/
echo ""
echo "=== Directory structure around the USDA file ==="
ls -la stretch3_ws/src/isaacsim/model/ 2>/dev/null || echo "Directory not found"
echo ""
echo "=== Checking if install directory exists at the referenced path ==="
ls -la stretch3_ws/install/ 2>/dev/null || echo "Install directory not found at stretch3_ws/install/"Repository: j3soon/ros2-essentials
Length of output: 905
🏁 Script executed:
#!/bin/bash
# Read the stretch_movable.usda file around lines 62 and 72
echo "=== Content around line 62 ==="
sed -n '55,70p' stretch3_ws/src/isaacsim/model/stretch_movable.usda
echo ""
echo "=== Content around line 72 ==="
sed -n '65,80p' stretch3_ws/src/isaacsim/model/stretch_movable.usda
echo ""
echo "=== Full line count ==="
wc -l stretch3_ws/src/isaacsim/model/stretch_movable.usda
echo ""
echo "=== Check if flat_plane.usd exists in other common locations ==="
find stretch3_ws -name "*flat*" -type f 2>/dev/null
echo ""
echo "=== Check what's in the assets directory ==="
ls -la stretch3_ws/src/isaacsim/model/assets/Repository: j3soon/ros2-essentials
Length of output: 1998
🏁 Script executed:
#!/bin/bash
# Check for build/install configuration
echo "=== Looking for CMakeLists.txt or package.xml ==="
find stretch3_ws -maxdepth 3 -name "CMakeLists.txt" -o -name "package.xml" | head -20
echo ""
echo "=== Check if there are other .usda files with similar path patterns ==="
fd -t f "\.usda$" stretch3_ws/ | head -10
echo ""
echo "=== Search for other references to 'install/isaacsim' in the codebase ==="
rg "install/isaacsim" stretch3_ws/ --type usda --type xml -A 2 -B 2
echo ""
echo "=== Check the relative path from stretch_movable.usda ==="
echo "Current file: stretch3_ws/src/isaacsim/model/stretch_movable.usda"
echo "Relative ../../../install/isaacsim/... would resolve to: stretch3_ws/install/isaacsim/..."Repository: j3soon/ros2-essentials
Length of output: 746
🏁 Script executed:
#!/bin/bash
# Read the CMakeLists.txt to understand installation strategy
echo "=== stretch3_ws/src/isaacsim/CMakeLists.txt ==="
cat stretch3_ws/src/isaacsim/CMakeLists.txt
echo ""
echo "=== stretch3_ws/src/isaacsim/package.xml ==="
cat stretch3_ws/src/isaacsim/package.xmlRepository: j3soon/ros2-essentials
Length of output: 1810
Remove hardcoded install paths and use relative references instead.
The paths reference ../../../install/isaacsim/share/isaacsim/model/... which don't match the actual installation structure created by the ROS2 build system. When installed via the CMakeLists.txt configuration, files are placed in install/share/isaacsim/model/, making these relative paths incorrect. Use relative paths like ./stretch.project.usdz[SubUSDs/flat_plane.usd] and ./stretch.project.usdz[SubUSDs/stretch_description_SE3_eoa_wrist_dw3_tool_sg3.usd] instead.
🤖 Prompt for AI Agents
In stretch3_ws/src/isaacsim/model/stretch_movable.usda around line 62, the file
currently prepends a hardcoded install path
(../../../install/isaacsim/share/isaacsim/model/...) which is incorrect for
packaged installs; replace the absolute install-style references with relative
references to the local USD package files (for example use
./stretch.project.usdz[SubUSDs/flat_plane.usd] and similarly
./stretch.project.usdz[SubUSDs/stretch_description_SE3_eoa_wrist_dw3_tool_sg3.usd]
where applicable), ensuring all prepend payload entries use ./relative paths so
the USD resolves correctly when installed via the ROS2/CMake install layout.
| custom double inputs:wheelDistance = 341 | ||
| custom double inputs:wheelRadius = 0.05000000074505806 |
There was a problem hiding this comment.
Inconsistent wheelDistance value - likely a units error.
wheelDistance = 341 here appears to be in millimeters, but wheelRadius = 0.05 is in meters. Compare with the RosActionGraph at line 480 which uses wheelDistance = 0.341 (meters). This inconsistency will cause the keyboard-controlled differential controller to compute incorrect wheel velocities.
🔎 Proposed fix
- custom double inputs:wheelDistance = 341
+ custom double inputs:wheelDistance = 0.341📝 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.
| custom double inputs:wheelDistance = 341 | |
| custom double inputs:wheelRadius = 0.05000000074505806 | |
| custom double inputs:wheelDistance = 0.341 | |
| custom double inputs:wheelRadius = 0.05000000074505806 |
🤖 Prompt for AI Agents
In stretch3_ws/src/isaacsim/model/stretch_movable.usda around lines 740-741, the
custom double inputs:wheelDistance is set to 341 (mm) while wheelRadius is 0.05
(m), causing a units mismatch; change inputs:wheelDistance to 0.341 (meters) to
match the RosActionGraph value and wheelRadius units, then scan the file (and
related configs) for any other wheelDistance occurrences to ensure all use
meters and add/update an inline comment indicating the unit (meters) if absent.
| if version_ge(args.version, "4.2.0") and not version_gt(args.version, "2021.2.0"): | ||
| if sys.platform == "win32": | ||
| filepath_root = os.path.join("C:", "isaacsim") | ||
| else: | ||
| filepath_root = os.path.join(home_path, "isaacsim") | ||
| elif args.version == "4.2.0": | ||
| if sys.platform == "win32": | ||
| filepath_root = os.path.join(home_path, "AppData", "Local", "ov", "pkg", f"isaac-sim-{args.version}") | ||
| else: | ||
| filepath_root = os.path.join(home_path, ".local", "share", "ov", "pkg", f"isaac-sim-{args.version}") | ||
| elif version_ge(args.version, "2021.2.1") and not version_ge(args.version, "2023.1.2"): | ||
| if sys.platform == "win32": | ||
| filepath_root = os.path.join(home_path, "AppData", "Local", "ov", "pkg", f"isaac_sim-{args.version}") | ||
| else: | ||
| filepath_root = os.path.join(home_path, ".local", "share", "ov", "pkg", f"isaac_sim-{args.version}") | ||
| else: | ||
| print(f"Unsupported Isaac Sim version: {args.version}") | ||
| sys.exit(0) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file in question
find . -type f -name "run_isaacsim.py" | head -20Repository: j3soon/ros2-essentials
Length of output: 162
🏁 Script executed:
# Look for version_ge and version_gt function definitions
rg -n "def version_g[et]" --type pyRepository: j3soon/ros2-essentials
Length of output: 359
🏁 Script executed:
# Also check imports in the target file
head -30 stretch3_ws/src/isaacsim/scripts/run_isaacsim.pyRepository: j3soon/ros2-essentials
Length of output: 910
🏁 Script executed:
# Extract the version comparison functions
sed -n '55,75p' stretch3_ws/src/isaacsim/scripts/run_isaacsim.pyRepository: j3soon/ros2-essentials
Length of output: 939
🏁 Script executed:
# Extract the problematic code block
sed -n '112,129p' stretch3_ws/src/isaacsim/scripts/run_isaacsim.pyRepository: j3soon/ros2-essentials
Length of output: 1243
Line 117 is unreachable due to conflicting conditions.
The condition on line 112 version_ge(args.version, "4.2.0") and not version_gt(args.version, "2021.2.0") matches any version >= 4.2.0 AND <= 2021.2.0 (via tuple comparison). Since 4.2.0 satisfies both conditions, the check on line 117 elif args.version == "4.2.0" can never execute—it's already caught by line 112.
This reveals a logic error: the first range [4.2.0, 2021.2.0] is intended to be handled on line 112, making the second condition for 4.2.0 redundant. The second conditional branch (line 117) appears to be dead code and should be removed or the line 112 condition should be corrected.
Additionally, versions like 2023.1.2 will fail all conditions and be marked unsupported, even though it appears to be a valid year-based version in this codebase. Verify the intended version ranges and correct the overlapping conditions.
🤖 Prompt for AI Agents
In stretch3_ws/src/isaacsim/scripts/run_isaacsim.py around lines 112-129, the
condition at line 112 (version_ge(args.version, "4.2.0") and not
version_gt(args.version, "2021.2.0")) overlaps & captures "4.2.0", making the
subsequent elif args.version == "4.2.0" unreachable; remove or fix the overlap
by either changing the first condition to start after "4.2.0" (use
version_gt(..., "4.2.0")) so the explicit "4.2.0" branch can run, or delete the
redundant elif args.version == "4.2.0" branch and consolidate its filepath logic
into the first branch, and while editing ensure valid ranges include newer
year-based versions (e.g., add/adjust a branch to handle versions >= "2023.1.2")
so supported versions like 2023.1.2 are not marked unsupported.
| elif args.ros_installation_path: | ||
| # If a custom ros installation path is provided | ||
| if sys.platform == "win32": | ||
| proc = subprocess.Popen(f"call {args.ros_installation_path}", shell=True, start_new_session=True) | ||
| subprocesses.append(proc.pid) | ||
| else: | ||
| proc = subprocess.Popen(f"source {args.ros_installation_path}", shell=True, start_new_session=True) | ||
| subprocesses.append(proc.pid) |
There was a problem hiding this comment.
Sourcing ROS installation via subprocess won't affect current process environment.
Running source or call in a subprocess only modifies that subprocess's environment, not the parent Python process. The environment changes won't be available for the subsequent Isaac Sim subprocess.
🔎 Recommended approach
If you need to source a ROS setup script and capture its environment:
import shlex
def source_ros_env(script_path):
"""Source a shell script and capture environment changes."""
if sys.platform == "win32":
cmd = f'cmd /c "call {script_path} && set"'
else:
cmd = f'bash -c "source {script_path} && env"'
result = subprocess.run(cmd, shell=True, capture_output=True, text=True)
for line in result.stdout.splitlines():
if '=' in line:
key, _, value = line.partition('=')
os.environ[key] = valueAlternatively, if the ROS environment is already sourced before running this script, you may not need this block at all.
🧰 Tools
🪛 Ruff (0.14.10)
148-148: subprocess call with shell=True identified, security issue
(S602)
151-151: subprocess call with shell=True identified, security issue
(S602)
| use_sim_time = SetParameter( | ||
| name="use_sim_time", | ||
| value=PythonExpression(["'false' if '", LaunchConfiguration("platform"), "' == 'real_robot' else 'true'"]), | ||
| ) |
There was a problem hiding this comment.
use_sim_time receives string instead of boolean.
The PythonExpression evaluates to the strings 'false' or 'true', but ROS 2 use_sim_time parameter expects a boolean. This may cause unexpected behavior.
Suggested fix
use_sim_time = SetParameter(
name="use_sim_time",
- value=PythonExpression(["'false' if '", LaunchConfiguration("platform"), "' == 'real_robot' else 'true'"]),
+ value=PythonExpression(["False if '", LaunchConfiguration("platform"), "' == 'real_robot' else True"]),
)📝 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.
| use_sim_time = SetParameter( | |
| name="use_sim_time", | |
| value=PythonExpression(["'false' if '", LaunchConfiguration("platform"), "' == 'real_robot' else 'true'"]), | |
| ) | |
| use_sim_time = SetParameter( | |
| name="use_sim_time", | |
| value=PythonExpression(["False if '", LaunchConfiguration("platform"), "' == 'real_robot' else True"]), | |
| ) |
🤖 Prompt for AI Agents
In stretch3_ws/src/stretch3_bringup/launch/stretch3_bringup.launch.py around
lines 21 to 24, the PythonExpression used for use_sim_time returns the strings
'false'/'true' instead of boolean values; change the expression to return actual
booleans (True/False) so the SetParameter passes a boolean to ROS 2 (e.g.,
replace the quoted 'false'/'true' branches with unquoted False/True in the
PythonExpression or otherwise evaluate to a Python boolean).
j3soon
left a comment
j3soon
left a comment
There was a problem hiding this comment.
Thanks for the PR! I encountered some issues while following the instructions, but can reproduce the bouncing issue from a fresh environment.
Please review PR #101, which is a minimal fix of the bouncing issue.
After that PR is merged, consider rebasing your changes (ROS2 control integrations) onto the latest commit, and then re-request review.
In addition, please follow the contribution guide while rebasing to make reviewing much easier. Currently, the commit history of this PR doesn't follow the guide, you can refer to the guide and PR #101 to observe the differences.
Thanks again.
|
Thank you for the feedback and for providing the fix. I really appreciate your help in identifying the root cause of the bouncing issue. I’ve also noted the contribution guide and will ensure my commit history and messages follow the standards moving forward. Regarding the issues you encountered while following my instructions, could you please share more details about them? I've tried my original environment setup on @yuhsiang1117 's linux laptop, and it can be correctly open after Thanks again for your guidance! |
2 participants
This branch implements the development environment for the Stretch 3 robot in NVIDIA Isaac Sim.
For a quick start guide and detailed instructions, please refer to stretch3_ws/README.md.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.