Skip to content

Commit e48d163

Browse files
committed
fix(turtlebot3-demo): address PR review comments
- Fix covariance calculation (sqrt of sum, not sum of squares) - Fix has_active_goal race condition with multiple goals - Replace time.time() with ROS time for simulation compatibility - Fix docstring to match actual severity levels (ABORTED=ERROR, CANCELED=WARN) - Update launch comment to accurately describe fault reporting - Standardize navigation payload to use 'goal' instead of 'request' - Add execution_id fallback in inject-nav-failure.sh - Log KeyboardInterrupt on shutdown Addresses review comments from Copilot and mfaferek93.
1 parent 5d49b27 commit e48d163

4 files changed

Lines changed: 41 additions & 27 deletions

File tree

demos/turtlebot3_integration/inject-nav-failure.sh

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ RESPONSE=$(curl -s -X POST "${API_BASE}/apps/bt-navigator/operations/navigate_to
4040

4141
echo "$RESPONSE" | jq '.' 2>/dev/null || echo "$RESPONSE"
4242

43-
# Extract execution ID
44-
EXEC_ID=$(echo "$RESPONSE" | jq -r '.id' 2>/dev/null)
43+
# Extract execution ID (support both .execution_id and .id)
44+
EXEC_ID=$(echo "$RESPONSE" | jq -r '.execution_id // .id // empty' 2>/dev/null)
4545

4646
if [ -n "$EXEC_ID" ] && [ "$EXEC_ID" != "null" ]; then
4747
echo ""

demos/turtlebot3_integration/launch/demo.launch.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ def generate_launch_description():
165165
},
166166
],
167167
),
168-
# Launch anomaly detector to monitor navigation and publish diagnostics
168+
# Launch anomaly detector to monitor navigation and report faults via fault manager
169169
Node(
170170
package="turtlebot3_medkit_demo",
171171
executable="anomaly_detector.py",

demos/turtlebot3_integration/scripts/anomaly_detector.py

Lines changed: 37 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,19 @@
1717
Anomaly Detector Node for TurtleBot3 Demo
1818
1919
Monitors navigation metrics and reports faults directly to FaultManager:
20-
- Navigation goal ABORTED/CANCELED -> ERROR fault
20+
- Navigation goal ABORTED -> ERROR fault
21+
- Navigation goal CANCELED -> WARN fault
2122
- High AMCL localization uncertainty -> WARN/ERROR fault
2223
- No robot progress when goal active -> WARN fault
2324
2425
Reports faults directly via /fault_manager/report_fault service.
2526
"""
2627

2728
import math
28-
import time
2929
from typing import Optional
3030

3131
import rclpy
32+
from rclpy.time import Time
3233
from rclpy.node import Node
3334
from rclpy.qos import QoSProfile, ReliabilityPolicy, DurabilityPolicy
3435

@@ -112,12 +113,12 @@ def __init__(self):
112113
self.last_goal_status: dict = {} # goal_id -> last_status
113114
self.has_active_goal = False
114115
self.last_position: Optional[tuple] = None
115-
self.last_progress_time = time.time()
116+
self.last_progress_time: Optional[Time] = None
116117
self.current_covariance = 0.0
117118

118-
# Throttling
119-
self.last_covariance_report_time = 0.0
120-
self.last_no_progress_report_time = 0.0
119+
# Throttling (using ROS time for simulation compatibility)
120+
self.last_covariance_report_time: Optional[Time] = None
121+
self.last_no_progress_report_time: Optional[Time] = None
121122
self.report_throttle_sec = 5.0
122123

123124
# Track active faults for PASSED events
@@ -131,14 +132,19 @@ def __init__(self):
131132

132133
def goal_status_callback(self, msg: GoalStatusArray):
133134
"""Monitor navigation goal status for failures."""
135+
# Compute has_active_goal from entire message to avoid iteration-order issues
136+
active_statuses = {GoalStatus.STATUS_ACCEPTED, GoalStatus.STATUS_EXECUTING}
137+
self.has_active_goal = any(
138+
s.status in active_statuses for s in msg.status_list
139+
)
140+
134141
for status in msg.status_list:
135142
goal_id = ''.join(f'{b:02x}' for b in status.goal_info.goal_id.uuid)
136143
last_status = self.last_goal_status.get(goal_id)
137144

138-
# Track active goals
139-
if status.status in [GoalStatus.STATUS_ACCEPTED, GoalStatus.STATUS_EXECUTING]:
140-
self.has_active_goal = True
141-
self.last_progress_time = time.time()
145+
# Track active goals - update progress time when goal is active
146+
if status.status in active_statuses:
147+
self.last_progress_time = self.get_clock().now()
142148

143149
# Detect status changes to terminal failure states
144150
if last_status != status.status:
@@ -151,7 +157,6 @@ def goal_status_callback(self, msg: GoalStatusArray):
151157
description=f'Navigation goal ABORTED - path planning or execution failed (goal: {goal_id[:8]})',
152158
event_type=EVENT_FAILED
153159
)
154-
self.has_active_goal = False
155160
self.get_logger().warn(f'Navigation goal {goal_id[:8]} ABORTED')
156161

157162
elif status.status == GoalStatus.STATUS_CANCELED:
@@ -161,7 +166,6 @@ def goal_status_callback(self, msg: GoalStatusArray):
161166
description=f'Navigation goal CANCELED (goal: {goal_id[:8]})',
162167
event_type=EVENT_FAILED
163168
)
164-
self.has_active_goal = False
165169
self.get_logger().info(f'Navigation goal {goal_id[:8]} CANCELED')
166170

167171
elif status.status == GoalStatus.STATUS_SUCCEEDED:
@@ -178,17 +182,19 @@ def goal_status_callback(self, msg: GoalStatusArray):
178182
description='Navigation goal succeeded',
179183
event_type=EVENT_PASSED
180184
)
181-
self.has_active_goal = False
182185

183186
def amcl_pose_callback(self, msg: PoseWithCovarianceStamped):
184187
"""Monitor AMCL localization covariance."""
185188
# Calculate covariance magnitude from position covariance (indices 0, 7 for x, y variance)
189+
# cov[0] and cov[7] are already variances; use their sum to compute a position stddev magnitude
186190
cov = msg.pose.covariance
187-
self.current_covariance = math.sqrt(cov[0]**2 + cov[7]**2)
191+
self.current_covariance = math.sqrt(cov[0] + cov[7])
188192

189-
now = time.time()
190-
if now - self.last_covariance_report_time < self.report_throttle_sec:
191-
return
193+
now = self.get_clock().now()
194+
if self.last_covariance_report_time is not None:
195+
elapsed = (now - self.last_covariance_report_time).nanoseconds / 1e9
196+
if elapsed < self.report_throttle_sec:
197+
return
192198

193199
if self.current_covariance > self.covariance_error_threshold:
194200
self.report_fault(
@@ -217,7 +223,7 @@ def amcl_pose_callback(self, msg: PoseWithCovarianceStamped):
217223
description='AMCL localization normal',
218224
event_type=EVENT_PASSED
219225
)
220-
self.last_covariance_report_time = now
226+
self.last_covariance_report_time = now
221227

222228
def odom_callback(self, msg: Odometry):
223229
"""Track robot position for progress monitoring."""
@@ -229,7 +235,7 @@ def odom_callback(self, msg: Odometry):
229235
distance = math.sqrt((x - last_x)**2 + (y - last_y)**2)
230236

231237
if distance > self.min_progress_distance:
232-
self.last_progress_time = time.time()
238+
self.last_progress_time = self.get_clock().now()
233239
# Clear no-progress fault if robot is moving
234240
if 'NAVIGATION_NO_PROGRESS' in self.active_faults:
235241
self.report_fault(
@@ -246,11 +252,19 @@ def check_timer_callback(self):
246252
if not self.has_active_goal:
247253
return
248254

249-
now = time.time()
250-
time_since_progress = now - self.last_progress_time
255+
if self.last_progress_time is None:
256+
return
257+
258+
now = self.get_clock().now()
259+
time_since_progress = (now - self.last_progress_time).nanoseconds / 1e9
251260

252261
if time_since_progress > self.no_progress_timeout_sec:
253-
if now - self.last_no_progress_report_time > self.report_throttle_sec:
262+
can_report = True
263+
if self.last_no_progress_report_time is not None:
264+
elapsed = (now - self.last_no_progress_report_time).nanoseconds / 1e9
265+
can_report = elapsed > self.report_throttle_sec
266+
267+
if can_report:
254268
self.report_fault(
255269
fault_code='NAVIGATION_NO_PROGRESS',
256270
severity=SEVERITY_WARN,
@@ -298,7 +312,7 @@ def main(args=None):
298312
try:
299313
rclpy.spin(node)
300314
except KeyboardInterrupt:
301-
pass
315+
node.get_logger().info('AnomalyDetectorNode interrupted by user (KeyboardInterrupt), shutting down.')
302316
finally:
303317
node.destroy_node()
304318
rclpy.shutdown()

demos/turtlebot3_integration/send-nav-goal.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ echo ""
6565
RESPONSE=$(curl -s -X POST "${API_BASE}/apps/bt-navigator/operations/navigate_to_pose/executions" \
6666
-H "Content-Type: application/json" \
6767
-d "{
68-
\"request\": {
68+
\"goal\": {
6969
\"pose\": {
7070
\"header\": {\"frame_id\": \"map\"},
7171
\"pose\": {

0 commit comments

Comments
 (0)