Skip to content

Pre-comp dev merge#64

Open
osu-uwrt-bot2 wants to merge 54 commits into
masterfrom
dev
Open

Pre-comp dev merge#64
osu-uwrt-bot2 wants to merge 54 commits into
masterfrom
dev

Conversation

@osu-uwrt-bot2

@osu-uwrt-bot2 osu-uwrt-bot2 commented Jul 3, 2026

Copy link
Copy Markdown

Summary by CodeRabbit

  • New Features
    • Added services for setting a string value and starting a binary classification task.
    • Added acoustics sampling with start/stop controls and buffer-based “winner” comparison.
    • Added runtime controls for image subscription/saving and optional claw support.
    • Added a pinger frequency broker that publishes updates on a fixed interval.
  • Bug Fixes
    • Improved depth message timestamps to match the source measurement timing.
    • Enabled predict-to-current-time in localization EKF.
  • Improvements
    • Consolidated ZED camera bring-up and updated camera/framerate/calibration settings.
    • Refreshed vehicle dynamics tuning, sensor/attachment poses, and related launch/monitoring behavior.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Talos 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.

Changes

Talos hardware integration

Layer / File(s) Summary
Vehicle tuning and actuator configuration
riptide_descriptions/config/{talos,simulator}.yaml, riptide_descriptions/urdf/robot_actuators.xacro
Vehicle properties, controller tuning, sensor extrinsics, thruster configuration, and optional claw actuator generation are updated.
ZED camera configuration and launch
riptide_hardware/cfg/*camera_config.yaml, riptide_hardware/launch/{zed,navigation,apriltag}.launch.py
FFC and DFC use updated ZED-XM parameters, and both camera nodes are launched in one composable container.
Hardware launch composition and monitoring
riptide_hardware/cfg/talos_ekf.yaml, riptide_hardware/launch/{hardware,hardware_fake_dvl,diagnostics}.launch.py
EKF prediction, logging, copro-agent, acoustics, and pinger-broker launch wiring are updated; pressure and sensor monitor nodes are removed from active launch lists.
Runtime image and depth handling
riptide_hardware/riptide_hardware2/{picture_taker.py,depth_converter.py}
Picture capture supports dynamic parameters and conditional subscriptions, while depth pose messages use incoming measurement timestamps.
Pinger frequency broker
riptide_hardware/src/pinger_broker.cpp, riptide_hardware/CMakeLists.txt
A timer-driven ROS 2 node forwards broker frequency updates to the pinger frequency topic and is installed with the hardware package.

ROS service schemas

Layer / File(s) Summary
Service request and response contracts
riptide_msgs/srv/{SetString,StartBinaryClassifier}.srv
SetString and StartBinaryClassifier request and response fields are defined.

Acoustics sampling

Layer / File(s) Summary
Sampling and comparison node
riptide_acoustics/src/Acoustics.cpp
Amplitude windows can be started and stopped through services, compared by configured mode, and published as a boolean result.
Package and launch wiring
riptide_acoustics/{CMakeLists.txt,package.xml,launch/acoustics.launch.py}
Build dependencies, launch installation, and the mode launch parameter are updated.
Legacy acoustics cleanup
riptide_acoustics/src/Acoustics_Initial_Development/Acoustics/two_pulse_depth_test.m
An obsolete acoustics development test script is removed.

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 sample buffers
  AcousticsNode->>ResultTopic: publish comparison result
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is generic and does not describe the actual changes in the merge. Rename it to a concise summary of the main change, such as the hardware and acoustics integration updates included here.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
riptide_hardware/riptide_hardware2/picture_taker.py (2)

154-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove 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 win

Sync enable_subscription_callback with the parameter system to prevent state desync.

The existing enable_subscription_callback (lines 223–249) sets self.subscription_enabled directly without calling self.set_parameters, so the ROS2 parameter value stays stale. With the new on_param_change callback relying on attribute/parameter consistency, this creates a desync: external tools querying the subscription_enabled parameter will see the old value after the service toggles it. Consider using self.set_parameters in 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 value

Remove the now-unused sensor_monitor_node definition.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ba58eb4 and b24fd9e.

📒 Files selected for processing (15)
  • riptide_descriptions/config/talos.yaml
  • riptide_descriptions/urdf/robot_actuators.xacro
  • riptide_hardware/cfg/dfc_config.yaml
  • riptide_hardware/cfg/ffc_config.yaml
  • riptide_hardware/cfg/talos_ekf.yaml
  • riptide_hardware/launch/apriltag.launch.py
  • riptide_hardware/launch/diagnostics.launch.py
  • riptide_hardware/launch/hardware.launch.py
  • riptide_hardware/launch/hardware_fake_dvl.launch.py
  • riptide_hardware/launch/navigation.launch.py
  • riptide_hardware/launch/zed.launch.py
  • riptide_hardware/riptide_hardware2/depth_converter.py
  • riptide_hardware/riptide_hardware2/picture_taker.py
  • riptide_msgs/srv/SetString.srv
  • riptide_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),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 || true

Repository: 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' || true

Repository: 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' || true

Repository: 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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_acoustics/src/Acoustics.cpp`:
- Around line 114-117: Update the mode-selection logic in Acoustics.cpp so the
p95 branch retains the response returned by summary("p95"). Move the
"UNRECOGNIZED MODE" assignment into an else branch associated with the
recognized-mode checks, ensuring it runs only for unsupported modes.
- Around line 176-180: Update the maximum-value comparison logic in the relevant
acoustics function so the non-fallback branch, where the difference is greater
than or equal to MODE_MAX_FALLBACK_DIFF, calculates and assigns buffer0Closer
directly from the maximum values and resets did_fallback_on_max to false.
Preserve the existing compare_avg_buffers behavior and true flag assignment for
the fallback branch.
🪄 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: 0bcda51b-2a57-4997-b4ef-2257d526f020

📥 Commits

Reviewing files that changed from the base of the PR and between d32ee3d and ad2e881.

📒 Files selected for processing (3)
  • riptide_acoustics/launch/acoustics.launch.py
  • riptide_acoustics/src/Acoustics.cpp
  • riptide_descriptions/config/talos.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • riptide_descriptions/config/talos.yaml

Comment on lines +114 to +117
} else if (mode == "p95") {
resp = summary("p95");
resp = "UNRECOGNIZED MODE, CHECK LAUNCH FILE OR SOMETHING ";
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Fix unrecognized mode string overwrite for p95.

The p95 branch unconditionally overwrites the valid response summary with the "unrecognized mode" error string. Introduce an else block to properly route truly unrecognized modes.

🐛 Proposed fix
       } else if (mode == "p95") {
         resp = summary("p95");
-        resp = "UNRECOGNIZED MODE, CHECK LAUNCH FILE OR SOMETHING ";
+      } else {
+        resp = "UNRECOGNIZED MODE, CHECK LAUNCH FILE OR SOMETHING ";
       }
📝 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
} else if (mode == "p95") {
resp = summary("p95");
resp = "UNRECOGNIZED MODE, CHECK LAUNCH FILE OR SOMETHING ";
}
} else if (mode == "p95") {
resp = summary("p95");
} else {
resp = "UNRECOGNIZED MODE, CHECK LAUNCH FILE OR SOMETHING ";
}
🤖 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_acoustics/src/Acoustics.cpp` around lines 114 - 117, Update the
mode-selection logic in Acoustics.cpp so the p95 branch retains the response
returned by summary("p95"). Move the "UNRECOGNIZED MODE" assignment into an else
branch associated with the recognized-mode checks, ensuring it runs only for
unsupported modes.

Comment on lines +176 to +180
if (std::abs(result_0 - result_1) < MODE_MAX_FALLBACK_DIFF) {
did_fallback_on_max = true;
compare_avg_buffers(buffer0, buffer1);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Set buffer0Closer and reset fallback flag when not falling back.

If the difference between the maximum values is greater than or equal to MODE_MAX_FALLBACK_DIFF, the function exits without calculating buffer0Closer. This leaves the comparison result unchanged from the previous run.

Additionally, did_fallback_on_max is never reset to false, causing subsequent successful max comparisons to falsely log that they fell back to the average mode.

🐛 Proposed fix to add the missing branch
       if (std::abs(result_0 - result_1) < MODE_MAX_FALLBACK_DIFF) {
         did_fallback_on_max = true;
         compare_avg_buffers(buffer0, buffer1);
+      } else {
+        did_fallback_on_max = false;
+        buffer0Closer = result_0 >= result_1;
       }
     }
📝 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
if (std::abs(result_0 - result_1) < MODE_MAX_FALLBACK_DIFF) {
did_fallback_on_max = true;
compare_avg_buffers(buffer0, buffer1);
}
}
if (std::abs(result_0 - result_1) < MODE_MAX_FALLBACK_DIFF) {
did_fallback_on_max = true;
compare_avg_buffers(buffer0, buffer1);
} else {
did_fallback_on_max = false;
buffer0Closer = result_0 >= result_1;
}
}
🤖 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_acoustics/src/Acoustics.cpp` around lines 176 - 180, Update the
maximum-value comparison logic in the relevant acoustics function so the
non-fallback branch, where the difference is greater than or equal to
MODE_MAX_FALLBACK_DIFF, calculates and assigns buffer0Closer directly from the
maximum values and resets did_fallback_on_max to false. Preserve the existing
compare_avg_buffers behavior and true flag assignment for the fallback branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants