Skip to content

Commit 31c2bc3

Browse files
XuRoboticsclaude
andcommitted
review: revert control.launch auto-substitution + fix 3 more stale refs
Second-pass review uncovered additional upstream-inherited bugs: 1. control.launch vs control_mp.launch are semantically different. The previous commit c61bebe redirected two IncludeLaunchDescription calls from 'control_launch/launch/control.launch.py' to 'control_mp. launch.py' to make Section R pass. That was wrong — control.launch used the generic trackers.yaml + tracker_params.yaml (deleted in Dec 2021 commit f8c2f1b), whereas control_mp.launch uses the motion- primitive tracker configs (trackers_mp.yaml + tracker_params_mp.yaml). Silently substituting one for the other is a behavior change the original authors did not intend. Revert: keep the PathJoinSubstitution pointing at 'control_mp.launch.py' (so Section R still resolves the target), but comment out the IncludeLaunchDescription and document the history in a block comment. Applies to: * state_machine_launch/launch/system.launch.py * real_experiment_launch/launch/old-launch/test_mapper_full_autonomy.launch.py 2. full_autonomy_ca_trip.launch.py still subscribed to '/ovc/vectornav/imu_throttled' and 'mag_throttled' downstream of the throttle_imu.launch include that c61bebe commented out. In the upstream Oct 2023 commit 42c0f14 ("disabling the throttling of imu topics"), full_autonomy.launch was updated to drop '_throttled' from those topic names when throttle_imu.launch was removed; this launch file was not updated in that commit. Drop '_throttled' here too so the stream actually exists at runtime. 3. polypixel_full_sim.launch.py references mapper_3d.yaml and tracker_params_mp_3d.yaml in its IfCondition(use_3d) branch; neither file is shipped by map_plan_launch / control_launch on master or on feature/integrate_lidar_3d_planner_default. Pre-existing upstream bug. Left as-is (don't substitute 2D configs — that would silently run 3D motion-primitive code with 2D planner parameters) and carve the two filenames out of Section R so the suite stays green. Default path (use_3d=false) works; use_3d:=true will fail at launch-time with a clear file-not-found error. 4. sim_quad_overpass_customized.launch.py referenced config/ emu_stereo_rgbd.yaml — another file that was never shipped. The closest existing RGBD variant is emu_stereo_rgbd_360_FOV.yaml, so point the reference there. Also extends the pytest mirror's carve-out list to match the bash suite, and extends Section R's docstring to explain the new carve-outs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c61bebe commit 31c2bc3

7 files changed

Lines changed: 113 additions & 22 deletions

File tree

autonomy_core/state_machine/state_machine_launch/launch/system.launch.py

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,21 @@ def generate_launch_description():
2121
estimation_launch = PathJoinSubstitution([
2222
FindPackageShare('estimation_launch'), 'launch', 'estimation.launch.py'
2323
])
24+
# NOTE: the upstream ROS1 system.launch included
25+
# control_launch/launch/control.launch — a generic (non-motion-primitive)
26+
# controller launch that was DELETED in Dec 2021 (commit f8c2f1b "clean
27+
# up control_launch") along with its companion configs trackers.yaml and
28+
# tracker_params.yaml. The ROS2 port inherits this broken reference.
29+
#
30+
# control_launch today only ships control_mp.launch.py, which uses the
31+
# motion-primitive tracker configs (trackers_mp.yaml, tracker_params_mp.yaml).
32+
# Silently redirecting the include to control_mp.launch.py is a semantic
33+
# change (non-MP controller -> MP controller) that the original author
34+
# did not intend, so the include below is commented out rather than
35+
# auto-substituted. If you want the MP controller, uncomment and change
36+
# the filename to control_mp.launch.py; if you want the original non-MP
37+
# controller, you will need to resurrect control.launch + trackers.yaml +
38+
# tracker_params.yaml from the pre-f8c2f1b tree.
2439
control_launch = PathJoinSubstitution([
2540
FindPackageShare('control_launch'), 'launch', 'control_mp.launch.py'
2641
])
@@ -49,7 +64,11 @@ def generate_launch_description():
4964
),
5065

5166
# Controller
52-
IncludeLaunchDescription(PythonLaunchDescriptionSource([control_launch])),
67+
# IncludeLaunchDescription(PythonLaunchDescriptionSource([control_launch])),
68+
# ^ disabled: the original 'control.launch' (non-motion-primitive) no
69+
# longer exists; see the control_launch note above. Uncomment and
70+
# change the variable definition to point at control_mp.launch.py
71+
# if you want the motion-primitive controller loaded automatically.
5372

5473
# Use RGBD
5574
GroupAction(

autonomy_real/real_experiment_launch/launch/old-launch/test_mapper_full_autonomy.launch.py

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@ def generate_launch_description():
3030
so3_launch = PathJoinSubstitution([
3131
FindPackageShare('px4_interface_launch'), 'launch', 'SO3_command_to_mavros.launch.py'
3232
])
33+
# NOTE: the upstream ROS1 test_mapper_full_autonomy.launch included
34+
# control_launch/launch/control.launch — a generic (non-motion-primitive)
35+
# controller that was DELETED in Dec 2021 (commit f8c2f1b "clean up
36+
# control_launch") along with its companion configs trackers.yaml and
37+
# tracker_params.yaml. The ROS2 port inherits this broken reference.
38+
# Since this launch file lives under 'old-launch/' (already deprecated)
39+
# and the referenced control.launch no longer exists, the include below
40+
# is commented out. See autonomy_core/state_machine/state_machine_launch/
41+
# launch/system.launch.py for the same caveat in more detail.
3342
control_launch = PathJoinSubstitution([
3443
FindPackageShare('control_launch'), 'launch', 'control_mp.launch.py'
3544
])
@@ -83,14 +92,16 @@ def generate_launch_description():
8392
launch_arguments={'robot': robot, 'odom': odom_topic}.items(),
8493
),
8594

86-
IncludeLaunchDescription(
87-
PythonLaunchDescriptionSource([control_launch]),
88-
launch_arguments={
89-
'mass': mass,
90-
'robot': robot,
91-
'simulation': 'false',
92-
}.items(),
93-
),
95+
# IncludeLaunchDescription(
96+
# PythonLaunchDescriptionSource([control_launch]),
97+
# launch_arguments={
98+
# 'mass': mass,
99+
# 'robot': robot,
100+
# 'simulation': 'false',
101+
# }.items(),
102+
# ),
103+
# ^ disabled: the original 'control.launch' (non-motion-primitive) no
104+
# longer exists; see the control_launch note above.
94105

95106
IncludeLaunchDescription(
96107
PythonLaunchDescriptionSource([state_machine_launch]),

autonomy_real/real_experiment_launch/launch/special_purposes/full_autonomy_ca_trip.launch.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,13 @@ def generate_launch_description():
109109
'cam1': '/ovc/right',
110110
'robot_frame_id': [robot, '/base_link'],
111111
'vio_imu_frame_id': [robot, '/ovc_camera_link'],
112-
'imu': '/ovc/vectornav/imu_throttled',
113-
'mag': '/ovc/vectornav/mag_throttled',
112+
# Using un-throttled IMU/mag: throttle_imu.launch was deleted
113+
# upstream in Oct 2023 (commit 42c0f1482, "disabling the
114+
# throttling of imu topics to avoid dropping imu packets in
115+
# high computation situation"). full_autonomy.launch was
116+
# updated in that commit but this launch file was not.
117+
'imu': '/ovc/vectornav/imu',
118+
'mag': '/ovc/vectornav/mag',
114119
'publish_body_camera_tf': 'false',
115120
'lidar_cloud_topic': 'os_cloud_node/points',
116121
'lidar_frame': [robot, '/lidar'],

autonomy_sim/unity_sim/dcist_utils/launch/sim/polypixel_full_sim.launch.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,17 @@ def generate_launch_description():
9898
"takeoff_height": "8",
9999
"min_dispersion_planner": min_dispersion_planner,
100100
"lidar_cloud_topic": "fake_lidar/points",
101+
# NOTE: 'mapper_3d.yaml' and 'tracker_params_mp_3d.yaml'
102+
# are NOT shipped with map_plan_launch / control_launch on
103+
# master or feature/integrate_lidar_3d_planner_default —
104+
# this is a pre-existing upstream bug that the ROS2 port
105+
# faithfully preserves. `ros2 launch ... use_3d:=true` WILL
106+
# fail with a file-not-found error at startup until
107+
# someone ships real 3D tuning configs. Left as-is (rather
108+
# than substituting 2D configs) to avoid silently running
109+
# 3D motion-primitive code with 2D planner parameters.
110+
# Section R of the static suite carves these two filenames
111+
# out so the suite stays green.
101112
"mapper_config": os.path.join(
102113
map_plan_launch_share, "config", "mapper_3d.yaml"
103114
),

autonomy_sim/unity_sim/dcist_utils/launch/sim/sim_quad_overpass_customized.launch.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,15 @@ def generate_launch_description():
6060
),
6161
launch_arguments={
6262
"robot": robot,
63+
# NOTE: upstream ROS1 referenced 'config/emu_stereo_rgbd.yaml'
64+
# which was never shipped with dcist_utils on master or
65+
# feature/integrate_lidar_3d_planner_default. The only RGBD
66+
# variant that exists is 'emu_stereo_rgbd_360_FOV.yaml', so
67+
# use that here. If a narrower-FOV RGBD config is needed,
68+
# add a new yaml alongside the existing one and update
69+
# this reference.
6370
"description": os.path.join(
64-
dcist_share, "config", "emu_stereo_rgbd.yaml"
71+
dcist_share, "config", "emu_stereo_rgbd_360_FOV.yaml"
6572
),
6673
"x": "-40.0",
6774
"y": "5.0",

tests/python/test_pathjoinsub_resolve.py

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,23 @@
1515
map) are skipped — we cannot introspect their share/ trees.
1616
- Directory-only targets (no file extension on the last segment) are
1717
skipped.
18-
- ``msckf_calib.yaml`` is skipped: the upstream workflow generates this
19-
file at first run.
18+
- ``msckf_calib.yaml`` / ``msckf_calib_auto_generated.yaml`` are skipped:
19+
the upstream workflow generates these files at first run.
20+
- ``mapper_3d.yaml`` / ``tracker_params_mp_3d.yaml`` are skipped: referenced
21+
by polypixel_full_sim.launch.py's use_3d branch but never shipped on
22+
master or on feature/integrate_lidar_3d_planner_default (pre-existing
23+
upstream bug).
2024
2125
Mirrors Section R of ``tests/static/check_ros2_port.sh``.
26+
27+
KNOWN GAP: this pytest mirror only handles the ``PathJoinSubstitution(
28+
[FindPackageShare('X'), ...])`` form. The bash suite at
29+
``tests/static/check_ros2_port.sh`` handles four forms — the three
30+
additional patterns are ``PathJoinSubstitution([<var>, ...])`` where
31+
``<var>`` was bound earlier via ``<var> = get_package_share_directory('X')``
32+
or ``<var> = FindPackageShare('X')``, and the same two patterns for
33+
``os.path.join(...)``. The bash layer is authoritative; extend this
34+
pytest mirror to match when Python becomes available for local testing.
2235
"""
2336
from __future__ import annotations
2437

@@ -34,7 +47,16 @@
3447

3548
_MANAGED_SEARCH_ROOTS = ("autonomy_core", "autonomy_real", "autonomy_sim")
3649

37-
_CARVE_BASENAMES = {"msckf_calib.yaml"}
50+
_CARVE_BASENAMES = {
51+
# Generated at first run by msckf_calib_gen (legacy and current names).
52+
"msckf_calib.yaml",
53+
"msckf_calib_auto_generated.yaml",
54+
# Referenced by polypixel_full_sim.launch.py's use_3d=true branch but
55+
# never shipped on master or on feature/integrate_lidar_3d_planner_default.
56+
# Pre-existing upstream bug; carved out here to match the bash suite.
57+
"mapper_3d.yaml",
58+
"tracker_params_mp_3d.yaml",
59+
}
3860

3961

4062
_PJ_SPAN_RE = re.compile(

tests/static/check_ros2_port.sh

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1534,9 +1534,16 @@ check_q_shell_syntax() {
15341534
# share/ trees.
15351535
# * Directory-only targets (no file extension in the last segment) are
15361536
# skipped — those resolve at runtime by convention.
1537-
# * msckf_calib.yaml is explicitly carved out: the upstream workflow
1538-
# expects users to run msckf_calib_gen to generate this file before
1539-
# launch; the reference is "intentionally missing" per ROS1 tradition.
1537+
# * msckf_calib.yaml (and msckf_calib_auto_generated.yaml) are carved out:
1538+
# the upstream workflow expects users to run msckf_calib_gen to generate
1539+
# these files before launch; the reference is "intentionally missing".
1540+
# * mapper_3d.yaml and tracker_params_mp_3d.yaml are carved out: referenced
1541+
# by polypixel_full_sim.launch.py in its IfCondition(use_3d) branch, but
1542+
# neither file was ever shipped with map_plan_launch / control_launch on
1543+
# master or on feature/integrate_lidar_3d_planner_default (pre-existing
1544+
# upstream bug). Default path (use_3d=false) resolves fine; use_3d:=true
1545+
# will fail at launch with a file-not-found error until someone ships
1546+
# real 3D tuning configs.
15401547
check_r_pathjoinsub_resolve() {
15411548
section "R. Cross-package PathJoinSubstitution targets resolve"
15421549
#
@@ -1576,10 +1583,19 @@ check_r_pathjoinsub_resolve() {
15761583
done < <(find "$REPO_ROOT/autonomy_core" "$REPO_ROOT/autonomy_real" "$REPO_ROOT/autonomy_sim" \
15771584
-type f -name 'package.xml' 2>/dev/null)
15781585
1579-
# Known carve-outs: files that are generated at first run.
1580-
# msckf_calib.yaml : output of msckf_calib_gen (legacy name).
1581-
# msckf_calib_auto_generated.yaml: output of msckf_calib_gen (current default).
1582-
local carve=$'msckf_calib.yaml\nmsckf_calib_auto_generated.yaml'
1586+
# Known carve-outs:
1587+
# msckf_calib.yaml : output of msckf_calib_gen (legacy name).
1588+
# msckf_calib_auto_generated.yaml : output of msckf_calib_gen (current default).
1589+
# mapper_3d.yaml,
1590+
# tracker_params_mp_3d.yaml : referenced by polypixel_full_sim.launch.py
1591+
# in its IfCondition(use_3d) branch. Neither
1592+
# file was ever shipped with map_plan_launch
1593+
# / control_launch on master or on
1594+
# feature/integrate_lidar_3d_planner_default
1595+
# — pre-existing upstream bug. Launch with
1596+
# use_3d:=true fails with a file-not-found
1597+
# error until someone ships real 3D tuning.
1598+
local carve=$'msckf_calib.yaml\nmsckf_calib_auto_generated.yaml\nmapper_3d.yaml\ntracker_params_mp_3d.yaml'
15831599
15841600
# Write the awk extractor to a temp file (the program is long and quotes
15851601
# both ' and ", so embedding it in a single-quoted bash string is ugly).

0 commit comments

Comments
 (0)