Pre-comp dev merge#64
Open
osu-uwrt-bot2 wants to merge 49 commits into
Open
Conversation
…nto BinaryClassifier
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughTalos vehicle tuning, actuator geometry, simulator properties, ZED camera configuration, hardware launch composition, pinger brokering, image capture behavior, depth timestamping, ROS service definitions, and acoustics sampling were updated. ChangesTalos hardware integration
ROS service schemas
Acoustics sampling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant SamplingServices
participant AcousticsNode
participant AmplitudeStream
participant ResultTopic
SamplingServices->>AcousticsNode: start or stop sampling
AmplitudeStream->>AcousticsNode: deliver amplitude samples
AcousticsNode->>AcousticsNode: compare buffer percentiles
AcousticsNode->>ResultTopic: publish comparison result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
riptide_hardware/riptide_hardware2/picture_taker.py (2)
154-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out dead code in
capture_image_callback.Lines 154–156 contain commented-out logic that appears to be leftover from development. Remove it to keep the codebase clean.
♻️ Proposed cleanup
- - # if not self.save_stereo or self.save_split: - # return - # Generate filename with timestamp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@riptide_hardware/riptide_hardware2/picture_taker.py` around lines 154 - 173, Remove the commented-out conditional return block at the beginning of capture_image_callback, including the lines referencing self.save_stereo and self.save_split, while leaving the active image-saving logic unchanged.
91-121: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSync
enable_subscription_callbackwith the parameter system to prevent state desync.The existing
enable_subscription_callback(lines 223–249) setsself.subscription_enableddirectly without callingself.set_parameters, so the ROS2 parameter value stays stale. With the newon_param_changecallback relying on attribute/parameter consistency, this creates a desync: external tools querying thesubscription_enabledparameter will see the old value after the service toggles it. Consider usingself.set_parametersin the service callback so both the attribute and the ROS2 parameter stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@riptide_hardware/riptide_hardware2/picture_taker.py` around lines 91 - 121, Update enable_subscription_callback to change subscription_enabled through self.set_parameters rather than assigning the attribute directly, ensuring the ROS2 parameter and instance state remain synchronized and on_param_change handles subscriber updates consistently.riptide_hardware/launch/diagnostics.launch.py (1)
52-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the now-unused
sensor_monitor_nodedefinition.The variable is still defined (lines 52–61) but commented out in the
LaunchDescription(line 80), making it dead code. Consider removing the definition or adding a comment explaining why it's kept.Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@riptide_hardware/launch/diagnostics.launch.py` around lines 52 - 61, Remove the unused sensor_monitor_node Node definition and any related dead references from the launch description, since it is not included in LaunchDescription.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@riptide_hardware/launch/zed.launch.py`:
- Around line 35-64: The launch description unconditionally starts both ZED
components and disables container recovery for non-default robots. Update the
launch setup around `DeclareLaunchArgument`, `GroupAction`, and
`ComposableNodeContainer` to gate robot-specific nodes based on the selected
`robot` value, preserving support for `tempest` while avoiding incompatible
components for other robots; enable respawning or otherwise configure crash
recovery for the container.
In `@riptide_hardware/riptide_hardware2/picture_taker.py`:
- Around line 77-89: In on_param_change, wrap handle_param_update(changed) in
try/except, log the exception, and return SetParametersResult(successful=False,
reason=...) on failure; preserve the existing successful return otherwise.
Ensure updated attributes are rolled back to their previous values before
returning failure so node state remains consistent with ROS2 parameters.
---
Nitpick comments:
In `@riptide_hardware/launch/diagnostics.launch.py`:
- Around line 52-61: Remove the unused sensor_monitor_node Node definition and
any related dead references from the launch description, since it is not
included in LaunchDescription.
In `@riptide_hardware/riptide_hardware2/picture_taker.py`:
- Around line 154-173: Remove the commented-out conditional return block at the
beginning of capture_image_callback, including the lines referencing
self.save_stereo and self.save_split, while leaving the active image-saving
logic unchanged.
- Around line 91-121: Update enable_subscription_callback to change
subscription_enabled through self.set_parameters rather than assigning the
attribute directly, ensuring the ROS2 parameter and instance state remain
synchronized and on_param_change handles subscriber updates consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a57c0607-8a62-4433-8149-ce4af600a746
📒 Files selected for processing (15)
riptide_descriptions/config/talos.yamlriptide_descriptions/urdf/robot_actuators.xacroriptide_hardware/cfg/dfc_config.yamlriptide_hardware/cfg/ffc_config.yamlriptide_hardware/cfg/talos_ekf.yamlriptide_hardware/launch/apriltag.launch.pyriptide_hardware/launch/diagnostics.launch.pyriptide_hardware/launch/hardware.launch.pyriptide_hardware/launch/hardware_fake_dvl.launch.pyriptide_hardware/launch/navigation.launch.pyriptide_hardware/launch/zed.launch.pyriptide_hardware/riptide_hardware2/depth_converter.pyriptide_hardware/riptide_hardware2/picture_taker.pyriptide_msgs/srv/SetString.srvriptide_msgs/srv/StartBinaryClassifier.srv
Comment on lines
35
to
+64
| return LaunchDescription([ | ||
| DeclareLaunchArgument( | ||
| name="robot", | ||
| default_value="tempest", | ||
| description="name of the robot" | ||
| ), | ||
| # Group actions under the robot namespace | ||
| DeclareLaunchArgument("robot", default_value="tempest"), | ||
|
|
||
| GroupAction([ | ||
| PushRosNamespace(LC("robot")), | ||
|
|
||
| ComposableNodeContainer( | ||
| name="ffc_container", | ||
| namespace="", | ||
| package="rclcpp_components", | ||
| executable="component_container", | ||
| output="screen", | ||
| respawn=True, | ||
| composable_node_descriptions=[ | ||
| # First ZED Node (FFC) | ||
| ComposableNode( | ||
| package="zed_components", | ||
| plugin="stereolabs::ZedCamera", | ||
| namespace="ffc", | ||
| name="zed_node", | ||
| parameters=[ | ||
| zed_config_path, | ||
| zedx_camera_path, | ||
| ffc_config_path, | ||
| ] | ||
| ), | ||
| ], | ||
| condition=IfCondition( | ||
| PythonExpression(["'", LC("robot"), "' == 'talos'"])) | ||
| ), | ||
|
|
||
| ComposableNodeContainer( | ||
| name="dfc_container", | ||
| name="zed_container", | ||
| namespace="", | ||
| package="rclcpp_components", | ||
| executable="component_container", | ||
| output="screen", | ||
| respawn=True, | ||
| composable_node_descriptions=[ | ||
| # Second ZED Node (DFC) | ||
| ComposableNode( | ||
| package="zed_components", | ||
| plugin="stereolabs::ZedCamera", | ||
| namespace="dfc", | ||
| name="zed_node", | ||
| parameters=[ | ||
| zed_config_path, | ||
| zedxm_camera_path, | ||
| dfc_config_path, | ||
| ] | ||
| ), | ||
| ], | ||
| condition=IfCondition( | ||
| PythonExpression(["'", LC("robot"), "' == 'talos'"])) | ||
| respawn=False, | ||
| composable_node_descriptions=[ffc_node, dfc_node], | ||
| ), | ||
|
|
||
| # ComposableNodeContainer( | ||
| # name="ffc_container", | ||
| # namespace="/", | ||
| # package="rclcpp_components", | ||
| # executable="component_container", | ||
| # output="screen", | ||
| # respawn=True, | ||
| # composable_node_descriptions=[ | ||
| # # The tank camera | ||
| # ComposableNode( | ||
| # package="zed_components", | ||
| # plugin="stereolabs::ZedCamera", | ||
| # namespace="ffc", | ||
| # name="zed_node", | ||
| # parameters=[ | ||
| # # zedxm_camera_path, | ||
| # zed_config_path, | ||
| # tank_config_path, | ||
| # # zed_compression_path, | ||
| # {"general.camera_name": "liltank/ffc"}, | ||
| # {"mapping.clicked_point_topic": "/clicked_point"}, | ||
| # ] | ||
| # ), | ||
| # ], | ||
| # condition=IfCondition( | ||
| # PythonExpression(["'", LC("robot"), "' == 'liltank'"])) | ||
| # ), | ||
|
|
||
| # Disabled for now as both are on by default | ||
| # # Launch the ZedManager node | ||
| # Node( | ||
| # package='riptide_hardware2', | ||
| # executable='zed_manager.py', | ||
| # name='ZedManager', | ||
| # output='screen' | ||
| # ) | ||
|
|
||
| # Node( | ||
| # package='riptide_hardware2', | ||
| # executable='picture_taker.py', | ||
| # name='picture_taker', | ||
| # output='screen', | ||
| # parameters=[ | ||
| # {"robot_namespace": LC("robot")}, | ||
| # {"camera_name": "ffc"}, | ||
| # {"save_stereo": True}, | ||
| # {"save_split": True} | ||
| # ] | ||
| # ) | ||
|
|
||
| ], scoped=True), | ||
|
|
||
|
|
||
| Node( | ||
| package='riptide_hardware2', | ||
| executable='picture_taker.py', | ||
| name='picture_taker', | ||
| output='screen', | ||
| parameters=[ | ||
| {"robot_namespace": LC("robot")}, | ||
| {"camera_name": "ffc"}, | ||
| {"subscription_enabled": True}, | ||
| {"save_stereo": False}, | ||
| {"save_split": True} | ||
| ] | ||
| ) | ||
| ], scoped=True), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if zed.launch.py is included by any other launch file with a robot argument
rg -n "zed.launch" --type=py -g '!**/zed.launch.py' .Repository: osu-uwrt/riptide_core
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== zed.launch.py ==\n'
sed -n '1,220p' riptide_hardware/launch/zed.launch.py
printf '\n== launch-file references to zed.launch ==\n'
rg -n "zed\.launch" riptide_hardware -g '*.py' || true
printf '\n== robot LaunchArgument / LC(\"robot\") usage in launch files ==\n'
rg -n 'DeclareLaunchArgument\("robot"|LC\("robot"\)' riptide_hardware -g '*.py' || true
printf '\n== ZED/XM-specific launch files ==\n'
fd -a 'zed*.launch.py' riptide_hardware || trueRepository: osu-uwrt/riptide_core
Length of output: 4268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== robot names / conditionals in riptide_hardware launch files ==\n'
rg -n "'tempest'|'talos'|'puddles'|IfCondition|UnlessCondition|PythonExpression\\(" riptide_hardware/launch -g '*.py' || true
printf '\n== hardware.launch.py ==\n'
sed -n '1,180p' riptide_hardware/launch/hardware.launch.py
printf '\n== package.xml / README mentions of zed.launch or camera launch ==\n'
rg -n "zed\.launch|zed_wrapper|stereolabs::ZedCamera|picture_taker|robot" riptide_hardware -g 'README*' -g 'package.xml' -g '*.md' || trueRepository: osu-uwrt/riptide_core
Length of output: 5492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== all refs to ZedCamera / zed_container / picture_taker ==\n'
rg -n "ZedCamera|zed_container|picture_taker|ffc_config|dfc_config" . -g '*.py' -g '*.launch.py' -g '*.yaml' || true
printf '\n== tempest/talos/puddles mentions in riptide_hardware and neighboring packages ==\n'
rg -n "'tempest'|'talos'|'puddles'" . -g '*.py' -g '*.launch.py' -g '*.yaml' -g '*.md' -g 'package.xml' || trueRepository: osu-uwrt/riptide_core
Length of output: 1811
Keep the robot gate if this launch needs to support more than tempest. zed.launch.py now always starts both ffc_node and dfc_node under the default robot="tempest", so other robot configs will still try to bring up both ZED-XM components. respawn=False also means the container stays down if it crashes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@riptide_hardware/launch/zed.launch.py` around lines 35 - 64, The launch
description unconditionally starts both ZED components and disables container
recovery for non-default robots. Update the launch setup around
`DeclareLaunchArgument`, `GroupAction`, and `ComposableNodeContainer` to gate
robot-specific nodes based on the selected `robot` value, preserving support for
`tempest` while avoiding incompatible components for other robots; enable
respawning or otherwise configure crash recovery for the container.
Comment on lines
+77
to
+89
| def on_param_change(self, params): | ||
| changed = [] | ||
| for p in params: | ||
| if hasattr(self, p.name): | ||
| current = getattr(self, p.name) | ||
| if current != p.value: | ||
| setattr(self, p.name, p.value) | ||
| self.get_logger().info(f"Parameter '{p.name}' changed: {current} -> {p.value}") | ||
| changed.append(p) | ||
|
|
||
|
|
||
| self.handle_param_update(changed) | ||
| return SetParametersResult(successful=True) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add error handling around handle_param_update in on_param_change.
If handle_param_update raises an exception (e.g., create_save_dir fails on a permission error, or create_subscription fails), the callback crashes before returning SetParametersResult. ROS2 will treat this as a failed parameter set, but the setattr calls on lines 83 have already modified instance attributes — leaving the node in an inconsistent state where attributes are updated but the ROS2 parameter values are not.
🔒 Proposed fix: wrap handle_param_update in try-except
def on_param_change(self, params):
changed = []
for p in params:
if hasattr(self, p.name):
current = getattr(self, p.name)
if current != p.value:
setattr(self, p.name, p.value)
self.get_logger().info(f"Parameter '{p.name}' changed: {current} -> {p.value}")
changed.append(p)
-
- self.handle_param_update(changed)
- return SetParametersResult(successful=True)
+
+ try:
+ self.handle_param_update(changed)
+ except Exception as e:
+ self.get_logger().error(f"Error handling parameter update: {str(e)}")
+ return SetParametersResult(successful=False)
+
+ return SetParametersResult(successful=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.
Suggested change
| def on_param_change(self, params): | |
| changed = [] | |
| for p in params: | |
| if hasattr(self, p.name): | |
| current = getattr(self, p.name) | |
| if current != p.value: | |
| setattr(self, p.name, p.value) | |
| self.get_logger().info(f"Parameter '{p.name}' changed: {current} -> {p.value}") | |
| changed.append(p) | |
| self.handle_param_update(changed) | |
| return SetParametersResult(successful=True) | |
| def on_param_change(self, params): | |
| changed = [] | |
| for p in params: | |
| if hasattr(self, p.name): | |
| current = getattr(self, p.name) | |
| if current != p.value: | |
| setattr(self, p.name, p.value) | |
| self.get_logger().info(f"Parameter '{p.name}' changed: {current} -> {p.value}") | |
| changed.append(p) | |
| try: | |
| self.handle_param_update(changed) | |
| except Exception as e: | |
| self.get_logger().error(f"Error handling parameter update: {str(e)}") | |
| return SetParametersResult(successful=False) | |
| return SetParametersResult(successful=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@riptide_hardware/riptide_hardware2/picture_taker.py` around lines 77 - 89, In
on_param_change, wrap handle_param_update(changed) in try/except, log the
exception, and return SetParametersResult(successful=False, reason=...) on
failure; preserve the existing successful return otherwise. Ensure updated
attributes are rolled back to their previous values before returning failure so
node state remains consistent with ROS2 parameters.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit