Start Beast motion enabled and interlock on detection - #155
Merged
Conversation
📝 WalkthroughWalkthroughBEAST ROS startup now enables motion by default. Ethernet and charging interlocks can disable motion through the safety monitor. Launch configurations, deployment settings, documentation, logging, and safety tests now reflect this behavior. ChangesBEAST motion defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
Coldaine
marked this pull request as ready for review
August 3, 2026 06:30
coderabbitai
Bot
added
the
size:L
There was a problem hiding this comment.
Pull request overview
Updates BEAST-01 ROS 2 bringup defaults to start with motion enabled (allow_motion:=true) while keeping the Ethernet/charging interlock monitor active so it can request disarm when an interlock is present, and aligns tests/docs/service config to the new default.
Changes:
- Flip
allow_motiondefault totrueacross bringup and related SLAM/Nav launch files. - Update
ugv_bringupparameter default and systemd unit to launch motion-enabled. - Refresh safety monitor docs/tests to match the new “motion-enabled by default” behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| robot/beast/ros2_ws/src/ugv_main/ugv_slam/launch/slam_toolbox.launch.py | Default forwarded allow_motion to true for SLAM launch. |
| robot/beast/ros2_ws/src/ugv_main/ugv_nav/launch/nav.launch.py | Default forwarded allow_motion to true for Nav launch. |
| robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_monitor.py | Update monitor doc/log wording to match new motion-enabled startup behavior. |
| robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_logic.py | Adjust interlock state/comment wording consistent with new defaults. |
| robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_twist_mux_spine.py | Update guard-rail tests for allow_motion default-on. |
| robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.py | Update safety-monitor tests for bringup default-on. |
| robot/beast/ros2_ws/src/ugv_main/ugv_bringup/ugv_bringup/ugv_bringup.py | Change allow_motion parameter default to true in bringup node. |
| robot/beast/ros2_ws/src/ugv_main/ugv_bringup/launch/bringup_lidar.launch.py | Change launch default to true; update launch argument descriptions. |
| robot/beast/ros2_ws/deploy/systemd/ugv.env.example | Update on-robot env example comments for motion-enabled startup. |
| robot/beast/ros2_ws/deploy/systemd/beast-ros-base.service | Launch service with allow_motion:=true. |
| docs/beast-ops.md | Update ops runbook to describe motion-enabled startup and interlock behavior. |
Suppressed comments (2)
robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_monitor.py:109
- This startup log line claims the monitor “disables only active locks”, but the logic also disarms on unknown/unreadable Ethernet carrier (fail-closed). Consider rewording to avoid implying it will never disarm on uncertainty/misconfiguration.
self.get_logger().info(
f'ugv_safety_monitor: iface={self._iface} path={self._carrier_path} '
f'(client of {self._set_service_name}; disables only active locks)'
)
robot/beast/ros2_ws/src/ugv_main/ugv_bringup/launch/bringup_lidar.launch.py:88
- This description says the safety monitor “disables motion only when an interlock is observed”, but the monitor also disarms on unknown/unreadable Ethernet carrier (treating uncertainty as unsafe). The description should mention that fail-closed behavior.
use_safety_monitor_arg = DeclareLaunchArgument(
'use_safety_monitor', default_value='true',
description=(
'Start ugv_safety_monitor (ethernet/charging interlocks as a '
'client of /ugv/set_allow_motion). It disables motion only when '
'an interlock is observed.'
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+12
to
+14
| Normal bringup starts motion-enabled. When Ethernet or charging is observed, | ||
| this monitor asks ugv_bringup to disable motion. It does not auto-arm when an | ||
| interlock later clears; ugv_bringup remains the sole motion authority. |
Comment on lines
74
to
79
| allow_motion_arg = DeclareLaunchArgument( | ||
| 'allow_motion', default_value='false', | ||
| 'allow_motion', default_value='true', | ||
| description=( | ||
| 'Permit non-zero cmd_vel commands; keep false until physical ' | ||
| 'safety validation passes. Runtime flips go through ' | ||
| '/ugv/set_allow_motion (ugv_bringup) — never default this true.' | ||
| 'Permit non-zero cmd_vel commands by default. Active Ethernet or ' | ||
| 'charging interlocks may disable it through /ugv/set_allow_motion.' | ||
| ) |
Comment on lines
+185
to
+187
| `beast-ros-base.service` is **active** from the RobotOverview workspace. It starts with | ||
| `use_lidar:=true`, `allow_motion:=true`; the active `ugv_safety_monitor` disables motion | ||
| only after detecting Ethernet or charging. Base driver, LD19 LiDAR, pan-tilt |
Comment on lines
9
to
+11
| # LD19 USB serial — stable by-id symlink (Wave 0 verified: serial …5970075705 → ttyACM1). | ||
| # beast-ros-base launches with use_lidar:=true; keep allow_motion:=false until Set 1 re-gate. | ||
| # beast-ros-base launches motion-enabled. ugv_safety_monitor disables motion when it | ||
| # observes an active Ethernet or charging interlock. |
Comment on lines
14
to
16
| Environment=PYTHONUNBUFFERED=1 | ||
| ExecStart=/bin/bash -lc 'source /opt/ros/humble/setup.bash && source /home/beast/beast/RobotOverview/robot/beast/ros2_ws/install/setup.bash && exec ros2 launch ugv_bringup bringup_lidar.launch.py use_lidar:=true use_rviz:=false allow_motion:=false' | ||
| ExecStart=/bin/bash -lc 'source /opt/ros/humble/setup.bash && source /home/beast/beast/RobotOverview/robot/beast/ros2_ws/install/setup.bash && exec ros2 launch ugv_bringup bringup_lidar.launch.py use_lidar:=true use_rviz:=false allow_motion:=true' | ||
| KillSignal=SIGINT |
Comment on lines
+963
to
965
| assert "self.declare_parameter('allow_motion', True)" in source, ( | ||
| 'allow_motion must default on; the interlock monitor disables active locks' | ||
| ) |
Comment on lines
27
to
31
| allow_motion_arg = DeclareLaunchArgument( | ||
| 'allow_motion', default_value='false', | ||
| 'allow_motion', default_value='true', | ||
| description=( | ||
| 'Forwarded to bringup_lidar; keep false until Set 1 motion re-gate' | ||
| 'Forwarded to bringup_lidar; active interlocks may disable motion' | ||
| ), |
Comment on lines
164
to
171
| DeclareLaunchArgument( | ||
| 'allow_motion', | ||
| default_value='false', | ||
| default_value='true', | ||
| description=( | ||
| 'Forwarded to bringup_lidar; keep false until Set 1 motion re-gate. ' | ||
| 'Forwarded to bringup_lidar; active interlocks may disable motion. ' | ||
| 'Never stack this launch on a live beast-ros-base (second twist_mux) — ' | ||
| 'stop the service first.' | ||
| ), |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
robot/beast/ros2_ws/src/ugv_main/ugv_bringup/ugv_bringup/ugv_bringup.py (1)
113-136: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAlways stop the base during initialization.
With
allow_motion=True, Lines 135-136 no longer callsend_stop_command(). The supplieddocs/beast-ops.mdstates that the ESP32 latches the last non-zero command and has no lower-level failsafe. A service restart can therefore retain stale motion before the first command or interlock request. The watchdog does not cover this state because it is not armed until a non-zero command is accepted.Send an explicit stop unconditionally after opening the controller.
Suggested startup safing change
- if not self.allow_motion: - self.send_stop_command() + # Clear any velocity latched by the ESP32 before accepting commands. + self.send_stop_command()🤖 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 `@robot/beast/ros2_ws/src/ugv_main/ugv_bringup/ugv_bringup/ugv_bringup.py` around lines 113 - 136, Update the initialization flow in the constructor around base_controller creation so send_stop_command() is called unconditionally immediately after opening the controller and before any conditional handling, ensuring startup always clears a latched motion command regardless of allow_motion.robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_logic.py (1)
85-93: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake charging-signal loss fail closed.
_seen_charging_topicstarts false, so the charging lock is not added untilSafetyMonitor._on_chargingreceives a message. A missing, late, or non-republishing/ugv/charging_activepublisher therefore leavesallow_motion=truewhile charging may be active. Add a stale or missing-signal lock, or cover an always-present initial state and heartbeat in tests.🤖 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 `@robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_logic.py` around lines 85 - 93, Update SafetyDecision.evaluate so a missing or stale charging signal fails closed instead of allowing motion: add LOCK_CHARGING when _seen_charging_topic is false or the signal heartbeat has expired, while retaining the active-charging lock for valid true signals. Use the existing charging-state and timeout symbols in SafetyMonitor, and cover initial absence and signal-loss behavior in tests.
🤖 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 `@robot/beast/ros2_ws/deploy/systemd/beast-ros-base.service`:
- Line 15: Update the systemd unit’s Description field to identify startup as
motion-enabled and interlock-monitored, replacing the outdated “zero-motion
staging” wording; leave the ExecStart command unchanged.
- Line 15: Make the safety-monitor contract explicit in all motion-enabled entry
points: update beast-ros-base.service#15 to pass use_safety_monitor:=true and
interlock_override:=false to ros2 launch, and update the bringup_lidar.launch.py
argument forwarding in nav.launch.py#164-172 and slam_toolbox.launch.py#27-32 to
pass use_safety_monitor: 'true' and interlock_override: 'false'.
In `@robot/beast/ros2_ws/src/ugv_main/ugv_bringup/launch/bringup_lidar.launch.py`:
- Around line 74-87: Update the bringup launch configuration around
allow_motion_arg and use_safety_monitor_arg so motion cannot remain enabled when
use_safety_monitor is false: either reject that argument combination during
launch or derive an effective allow_motion value of false. Preserve
interlock_override as the only explicit maintenance bypass.
In `@robot/beast/ros2_ws/src/ugv_main/ugv_bringup/ugv_bringup/ugv_bringup.py`:
- Line 113: Update the declare_parameter call in the UGV bringup initialization
to pass the boolean default using the supported value=True keyword, and adjust
any source-text assertions that depend on the previous positional spelling.
In `@robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.py`:
- Around line 146-157: Strengthen the launch-default assertions so both tests
require safety monitoring to be enabled by default. In
robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.py lines
146-157, add the exact "'use_safety_monitor', default_value='true'" assertion
against text; apply the same assertion to source in
robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_twist_mux_spine.py lines
986-989.
In `@robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_monitor.py`:
- Around line 12-14: Update SafetyMonitor._enforce() so an active interlock
never returns harmlessly when _set_client.service_is_ready() is false; preserve
disarmed/fail-closed behavior until the disarm service becomes available,
including misconfigured service names, or trigger the monitor’s existing
local-stop mechanism and fail-closed state.
- Around line 12-14: Update the safety monitor description to state that it asks
ugv_bringup to disable motion whenever any safety lock is present, including
unknown or unreadable Ethernet carrier state, rather than only when Ethernet or
charging is observed. Keep the existing behavior that the monitor does not
auto-arm after locks clear and ugv_bringup remains the motion authority.
---
Outside diff comments:
In `@robot/beast/ros2_ws/src/ugv_main/ugv_bringup/ugv_bringup/ugv_bringup.py`:
- Around line 113-136: Update the initialization flow in the constructor around
base_controller creation so send_stop_command() is called unconditionally
immediately after opening the controller and before any conditional handling,
ensuring startup always clears a latched motion command regardless of
allow_motion.
In `@robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_logic.py`:
- Around line 85-93: Update SafetyDecision.evaluate so a missing or stale
charging signal fails closed instead of allowing motion: add LOCK_CHARGING when
_seen_charging_topic is false or the signal heartbeat has expired, while
retaining the active-charging lock for valid true signals. Use the existing
charging-state and timeout symbols in SafetyMonitor, and cover initial absence
and signal-loss behavior in tests.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4728e077-d167-42c4-a5cb-bf4a772a6444
📒 Files selected for processing (11)
docs/beast-ops.mdrobot/beast/ros2_ws/deploy/systemd/beast-ros-base.servicerobot/beast/ros2_ws/deploy/systemd/ugv.env.examplerobot/beast/ros2_ws/src/ugv_main/ugv_bringup/launch/bringup_lidar.launch.pyrobot/beast/ros2_ws/src/ugv_main/ugv_bringup/ugv_bringup/ugv_bringup.pyrobot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.pyrobot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_twist_mux_spine.pyrobot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_logic.pyrobot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_monitor.pyrobot/beast/ros2_ws/src/ugv_main/ugv_nav/launch/nav.launch.pyrobot/beast/ros2_ws/src/ugv_main/ugv_slam/launch/slam_toolbox.launch.py
| EnvironmentFile=/etc/beast/ugv.env | ||
| Environment=PYTHONUNBUFFERED=1 | ||
| ExecStart=/bin/bash -lc 'source /opt/ros/humble/setup.bash && source /home/beast/beast/RobotOverview/robot/beast/ros2_ws/install/setup.bash && exec ros2 launch ugv_bringup bringup_lidar.launch.py use_lidar:=true use_rviz:=false allow_motion:=false' | ||
| ExecStart=/bin/bash -lc 'source /opt/ros/humble/setup.bash && source /home/beast/beast/RobotOverview/robot/beast/ros2_ws/install/setup.bash && exec ros2 launch ugv_bringup bringup_lidar.launch.py use_lidar:=true use_rviz:=false allow_motion:=true' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the unit description for motion-enabled startup.
The Description at Line 2 still says zero-motion staging, but this command starts with allow_motion:=true. systemctl status will display an incorrect safety state. Update the description to state that startup is motion-enabled and interlock-monitored.
🤖 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 `@robot/beast/ros2_ws/deploy/systemd/beast-ros-base.service` at line 15, Update
the systemd unit’s Description field to identify startup as motion-enabled and
interlock-monitored, replacing the outdated “zero-motion staging” wording; leave
the ExecStart command unchanged.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Make the safety-monitor contract explicit in every motion-enabled entry point.
All three entry points enable or forward allow_motion without explicitly binding the safety-monitor enable and override values. The current nested defaults are correct, but this safety contract can drift independently.
robot/beast/ros2_ws/deploy/systemd/beast-ros-base.service#L15-L15: adduse_safety_monitor:=true interlock_override:=falsetoExecStart.robot/beast/ros2_ws/src/ugv_main/ugv_nav/launch/nav.launch.py#L164-L172: forwarduse_safety_monitor: 'true'andinterlock_override: 'false'tobringup_lidar.launch.py.robot/beast/ros2_ws/src/ugv_main/ugv_slam/launch/slam_toolbox.launch.py#L27-L32: forward the same explicit safety settings tobringup_lidar.launch.py.
📍 Affects 3 files
robot/beast/ros2_ws/deploy/systemd/beast-ros-base.service#L15-L15(this comment)robot/beast/ros2_ws/src/ugv_main/ugv_nav/launch/nav.launch.py#L164-L172robot/beast/ros2_ws/src/ugv_main/ugv_slam/launch/slam_toolbox.launch.py#L27-L32
🤖 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 `@robot/beast/ros2_ws/deploy/systemd/beast-ros-base.service` at line 15, Make
the safety-monitor contract explicit in all motion-enabled entry points: update
beast-ros-base.service#15 to pass use_safety_monitor:=true and
interlock_override:=false to ros2 launch, and update the bringup_lidar.launch.py
argument forwarding in nav.launch.py#164-172 and slam_toolbox.launch.py#27-32 to
pass use_safety_monitor: 'true' and interlock_override: 'false'.
Comment on lines
74
to
+87
| allow_motion_arg = DeclareLaunchArgument( | ||
| 'allow_motion', default_value='false', | ||
| 'allow_motion', default_value='true', | ||
| description=( | ||
| 'Permit non-zero cmd_vel commands; keep false until physical ' | ||
| 'safety validation passes. Runtime flips go through ' | ||
| '/ugv/set_allow_motion (ugv_bringup) — never default this true.' | ||
| 'Permit non-zero cmd_vel commands by default. Active Ethernet or ' | ||
| 'charging interlocks may disable it through /ugv/set_allow_motion.' | ||
| ) | ||
| ) | ||
|
|
||
| use_safety_monitor_arg = DeclareLaunchArgument( | ||
| 'use_safety_monitor', default_value='true', | ||
| description=( | ||
| 'Start ugv_safety_monitor (ethernet/charging interlocks as a ' | ||
| 'client of /ugv/set_allow_motion). Does not auto-arm.' | ||
| 'client of /ugv/set_allow_motion). It disables motion only when ' | ||
| 'an interlock is observed.' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Reject motion-enabled bringup when the safety monitor is disabled.
When allow_motion is true, use_safety_monitor:=false starts ugv_bringup without a node that observes Ethernet or charging interlocks. Non-zero /cmd_vel commands can then reach the base without interlock enforcement. Reject this combination or force effective allow_motion to false. Keep interlock_override as the only explicit maintenance bypass.
🤖 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 `@robot/beast/ros2_ws/src/ugv_main/ugv_bringup/launch/bringup_lidar.launch.py`
around lines 74 - 87, Update the bringup launch configuration around
allow_motion_arg and use_safety_monitor_arg so motion cannot remain enabled when
use_safety_monitor is false: either reject that argument combination during
launch or derive an effective allow_motion value of false. Preserve
interlock_override as the only explicit maintenance bypass.
| self.declare_parameter('wifi_interface', '') | ||
| self.declare_parameter('ethernet_interface', '') | ||
| self.declare_parameter('allow_motion', False) | ||
| self.declare_parameter('allow_motion', True) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use a keyword for the boolean default.
Ruff FBT003 flags the positional True at Line 113. Use the keyword form supported by the target ROS 2 Humble rclpy.Node.declare_parameter API, for example value=True, and update source-text assertions that depend on the exact call spelling.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 113-113: Boolean positional value in function call
(FBT003)
🤖 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 `@robot/beast/ros2_ws/src/ugv_main/ugv_bringup/ugv_bringup/ugv_bringup.py` at
line 113, Update the declare_parameter call in the UGV bringup initialization to
pass the boolean default using the supported value=True keyword, and adjust any
source-text assertions that depend on the previous positional spelling.
Source: Linters/SAST tools
Comment on lines
+146
to
157
| def test_bringup_launch_defaults_motion_enabled_and_wires_monitor(): | ||
| launch = ( | ||
| Path(__file__).resolve().parents[2] | ||
| / 'ugv_bringup' | ||
| / 'launch' | ||
| / 'bringup_lidar.launch.py' | ||
| ) | ||
| text = launch.read_text(encoding='utf-8') | ||
| assert "'allow_motion', default_value='false'" in text | ||
| assert "'allow_motion', default_value='true'" in text | ||
| assert 'ugv_safety_monitor' in text | ||
| assert 'use_safety_monitor' in text | ||
| assert "'interlock_override', default_value='false'" in text |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the launch tests fail when the safety monitor is disabled by default.
Both tests check only for names or symbols. They do not require use_safety_monitor to default to true, so a motion-enabled launch could lose interlock monitoring while both tests pass.
robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.py#L146-L157: assert"'use_safety_monitor', default_value='true'" in text.robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_twist_mux_spine.py#L986-L989: assert the same exact launch default insource.
📍 Affects 2 files
robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.py#L146-L157(this comment)robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_twist_mux_spine.py#L986-L989
🤖 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 `@robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.py`
around lines 146 - 157, Strengthen the launch-default assertions so both tests
require safety monitoring to be enabled by default. In
robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_safety_monitor.py lines
146-157, add the exact "'use_safety_monitor', default_value='true'" assertion
against text; apply the same assertion to source in
robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/test/test_twist_mux_spine.py lines
986-989.
Comment on lines
+12
to
+14
| Normal bringup starts motion-enabled. When Ethernet or charging is observed, | ||
| this monitor asks ugv_bringup to disable motion. It does not auto-arm when an | ||
| interlock later clears; ugv_bringup remains the sole motion authority. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make an unavailable disarm service fail closed.
SafetyMonitor._enforce() returns when self._set_client.service_is_ready() is false at Lines 140-141. With allow_motion now true, an active interlock is not enforced during service discovery or when the service name is misconfigured. Keep ugv_bringup disarmed until the service is ready, or make service unavailability trigger a local stop and fail-closed state.
🤖 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 `@robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_monitor.py`
around lines 12 - 14, Update SafetyMonitor._enforce() so an active interlock
never returns harmlessly when _set_client.service_is_ready() is false; preserve
disarmed/fail-closed behavior until the disarm service becomes available,
including misconfigured service names, or trigger the monitor’s existing
local-stop mechanism and fail-closed state.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe unknown Ethernet state as a lock.
SafetyState.evaluate() in robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_logic.py treats ethernet_carrier=None as LOCK_ETHERNET; Lines 87-90 also document missing or unreadable carrier state as fail closed. The phrases “when Ethernet is observed” and “disables only active locks” omit this disarm path. State that the monitor disarms when a lock is present, including unknown carrier state.
Also applies to: 106-108
🤖 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 `@robot/beast/ros2_ws/src/ugv_main/ugv_cockpit/ugv_cockpit/safety_monitor.py`
around lines 12 - 14, Update the safety monitor description to state that it
asks ugv_bringup to disable motion whenever any safety lock is present,
including unknown or unreadable Ethernet carrier state, rather than only when
Ethernet or charging is observed. Keep the existing behavior that the monitor
does not auto-arm after locks clear and ugv_bringup remains the motion
authority.
This was referenced Aug 3, 2026
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
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.
What changed
Normal BEAST bringup now starts with
allow_motion:=true. The Ethernet/charging safety monitor remains enabled and requestsallow_motion=falseonly after it observes an active physical interlock.Why
Default-disarmed startup was project policy, not a ROS 2 requirement, and conflicts with the requested normal operating behavior. The command-timeout watchdog is unchanged.
Validation
66 passed: focused bringup safety, interlock, and command-spine testsgit diff --checkSummary
Normal BEAST bringup now starts with
allow_motion:=true./ugv/set_allow_motion.Validation
git diff --checkpassed.