Ros2 - #42
Open
nathantsoi wants to merge 57 commits into
Open
Conversation
…r config on installation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the UT Automata codebase from ROS 1 to ROS 2, updating dependencies, message types, APIs, and build infrastructure to support the new framework.
Key Changes:
- Complete migration from ROS 1 to ROS 2 APIs (roscpp → rclcpp)
- Updated message includes and type namespaces (e.g.,
sensor_msgs::LaserScan→sensor_msgs::msg::LaserScan) - Converted launch files from XML to Python format
- Updated build system from rosbuild to ament_cmake
Reviewed Changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/websocket/websocket_main.cc | Migrated to rclcpp, updated time/clock APIs, added graceful shutdown |
| src/websocket/websocket.h | Updated message type namespaces, added shutdown methods |
| src/websocket/websocket.cc | Updated message types and time initialization |
| src/vesc_driver/vesc_driver_node.cpp | Migrated to rclcpp node creation and spinning |
| src/vesc_driver/vesc_driver.h | Updated to rclcpp publishers/subscribers and timer |
| src/vesc_driver/vesc_driver.cpp | Migrated time APIs, callbacks, and added joystick mode config |
| src/simulator/vector_map.cc | Added default initialization for Vector2f |
| src/simulator/step_simulator_main.cc | Converted to ROS 2 with rclcpp APIs and parameters |
| src/simulator/simulator_main.cc | Basic ROS 2 node migration |
| src/simulator/simulator.h | Updated to rclcpp publishers/subscribers and tf2 |
| src/simulator/simulator.cc | Migrated to tf2, rclcpp time, and ament package paths |
| src/joystick/joystick_driver.cc | Added Bluetooth controller detection and auto-connection |
| src/gui/gui_mainwindow.h | Added camera display, disk space, and tmux controls |
| src/gui/gui_mainwindow.cc | Implemented camera feed, tmux management, instance locking |
| src/gui/gui_main.cc | Migrated to rclcpp with proper shutdown and timeout detection |
| scripts/udev.sh | Added udev rule setup script for DualShock 4 |
| scripts/keyboard_teleop.py | Converted from rospy to rclpy |
| scripts/joystick_teleop.py | Converted from rospy to rclpy with corrected message type |
| package.xml | Added ROS 2 package manifest |
| msg/VescStateStamped.msg | Fixed header type reference |
| launch/*.py | Converted launch files to ROS 2 Python format |
| config/joystick.lua | Updated defaults and added joystick mode config |
| Makefile | Changed default build mode to Hardware |
| CMakeLists.txt | Complete migration to ament_cmake build system |
| .github/workflows/buildTest.yml | Updated CI to use ROS 2 Humble container |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const auto now = ros::Time::now(); | ||
| if ((now - laser_scan_.header.stamp).toSec() > FLAGS_max_age) { | ||
| laser_scan_.header.stamp = ros::Time(0); | ||
| const auto now = rclcpp::Clock().now(); |
There was a problem hiding this comment.
Creating a new rclcpp::Clock() instance on each call is inefficient. Consider creating a single clock instance and reusing it, or use a node's clock via node->get_clock()->now().
| map.toStdString().c_str(), x, y, math_util::RadToDeg(theta)); | ||
| } | ||
| initial_pose_msg_.header.stamp = ros::Time::now(); | ||
| initial_pose_msg_.header.stamp = rclcpp::Clock().now(); |
There was a problem hiding this comment.
Creating a new rclcpp::Clock() instance on each call is inefficient. Consider using a node's clock via node->get_clock()->now() since a node is available in this scope.
| CONFIG_STRING(serial_port_, "serial_port"); | ||
|
|
||
| DEFINE_string(config_dir, "config", | ||
| DEFINE_string(config_dir, "/home/orin/roboracer_ws/src/ut_automata/config", |
There was a problem hiding this comment.
Hardcoded absolute path /home/orin/roboracer_ws/src/ut_automata/config reduces portability. Consider using ament_index_cpp::get_package_share_directory() to locate the config directory relative to the installed package.
| void VescDriver::checkCommandTimeout() { | ||
| static const double kTimeout = 0.5; | ||
| const double t_now = ros::WallTime::now().toSec(); | ||
| const double t_now = rclcpp::Clock(RCL_ROS_TIME).now().seconds(); |
There was a problem hiding this comment.
Creating a new rclcpp::Clock instance on each call is inefficient. Consider storing the clock instance as a member variable and reusing it.
Comment on lines
+264
to
+268
| size_t min_axes = 5; // Default for "both" mode (needs axes 0 and 4) | ||
| if (joystick_mode_ == "left") { | ||
| min_axes = 2; // Needs axes 0 and 1 | ||
| } else if (joystick_mode_ == "right") { | ||
| min_axes = 5; // Needs axes 3 and 4 |
There was a problem hiding this comment.
Magic numbers 5, 2, and 5 for min_axes are unclear. Define named constants like kMinAxesBoth = 5, kMinAxesLeft = 2, kMinAxesRight = 5 to improve readability and maintainability.
| void SaveControllerMAC(const string& mac_address) { | ||
| if (mac_address.empty()) return; | ||
|
|
||
| string config_file = string(getenv("HOME")) + "/roboracer_ws/car/joystick_controller_mac.txt"; |
There was a problem hiding this comment.
Hardcoded path /roboracer_ws/car/joystick_controller_mac.txt reduces portability. Consider using a more flexible path or storing configuration in a standard location like ~/.config/ut_automata/.
| // Get tmux configuration names (excluding "sim") | ||
| tmux_config_names_.clear(); | ||
| tmux_config_buttons_.clear(); | ||
| QDir tmux_dir("/home/orin/roboracer_ws/tmux"); |
There was a problem hiding this comment.
Hardcoded absolute path /home/orin/roboracer_ws/tmux reduces portability. Consider making this path configurable or using environment variables.
Suggested change
| QDir tmux_dir("/home/orin/roboracer_ws/tmux"); | |
| QString tmux_dir_path = QString::fromUtf8(qgetenv("ROBORACER_TMUX_DIR")); | |
| if (tmux_dir_path.isEmpty()) { | |
| tmux_dir_path = "/home/orin/roboracer_ws/tmux"; | |
| } | |
| QDir tmux_dir(tmux_dir_path); |
Comment on lines
+648
to
+649
| QString original_path = QString("/home/orin/roboracer_ws/tmux/%1/.tmuxinator.yaml").arg(QString::fromStdString(config_name)); | ||
|
|
There was a problem hiding this comment.
Hardcoded absolute path /home/orin/roboracer_ws/tmux reduces portability. Consider making this path configurable or using environment variables.
Suggested change
| QString original_path = QString("/home/orin/roboracer_ws/tmux/%1/.tmuxinator.yaml").arg(QString::fromStdString(config_name)); | |
| // Use environment variable ROBORACER_WS for workspace path | |
| QByteArray ws_path = qgetenv("ROBORACER_WS"); | |
| if (ws_path.isEmpty()) { | |
| // Environment variable not set, fallback to default or return error | |
| ws_path = "/home/orin/roboracer_ws"; | |
| } | |
| QString original_path = QString("%1/tmux/%2/.tmuxinator.yaml") | |
| .arg(QString::fromUtf8(ws_path)) | |
| .arg(QString::fromStdString(config_name)); |
| print('No joystick found!') | ||
| print('Please connect a joystick and try again.') | ||
| print('If your joystick is connected, ensure that the udev rules are set up correctly.') | ||
| print('You can run the script "src/ut_automata/scripts/udev.sh" to set up the appropriate rule.') |
There was a problem hiding this comment.
Hardcoded path reference src/ut_automata/scripts/udev.sh may not be accurate depending on where the package is installed. Consider using a more generic instruction or dynamically resolving the script location.
Suggested change
| print('You can run the script "src/ut_automata/scripts/udev.sh" to set up the appropriate rule.') | |
| print('You can run the "udev.sh" script in your package\'s scripts directory to set up the appropriate rule.') |
| add_custom_command( | ||
| OUTPUT ${GENERATED_CAR_LUA} | ||
| COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/share/${PROJECT_NAME}/config | ||
| COMMAND /bin/sh -c "hn=$(hostname); num=$(echo \"${hn}\" | grep -oE '[0-9]+' | tail -n1); if [ -z \"${num}\" ]; then num=0; fi; printf 'car_name = \"car%s\";\\n' \"${num}\" > \"${GENERATED_CAR_LUA}\"" |
There was a problem hiding this comment.
Complex shell command embedded in CMakeLists.txt is difficult to read and maintain. Consider extracting this logic into a separate script file for better maintainability.
for some reason the fuse_imu param is not read correctly from the vesc.lua file, but setting it to true in the header is fine for now
…iddle of the joystick zone and make steering at high speed easier
…onomous behaviors simply by moving the joysticks
Wrap ut_automata as an airfield package. airfield.yaml declares deps (incl urg_node for the Hokuyo launch) and passes hardware devices (/dev/ttyACM0 VESC, /dev/i2c-7 MPU6050 IMU, joystick) with the dialout+i2c groups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The airfield container entry wrapper now colcon-builds packages on demand;
ut_automata must build with --cmake-args -DCMAKE_BUILD_MODE=Hardware or
CMakeLists never compiles vesc_driver ("No executable found" at launch).
The new colcon_args field makes auto-builds match what scripts/build passed
explicitly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tick: fix exit-instead-of-wait, config path, Bluetooth stall
vesc_driver fixes:
1. Startup segfault when stale bytes sit in the serial buffer (init-order
race). VescDriver's constructor called vesc_.connect() — which starts the
serial rx thread — BEFORE creating the publishers, tf broadcaster, and
EKF that vescPacketCallback dereferences. If telemetry left behind by a
previous (killed) driver run was already buffered in the tty, the rx
thread parsed a complete frame within milliseconds and invoked the
callback while state_pub_ et al. were still null shared_ptrs:
"Out-of-sync with VESC ... Discarding 20 bytes." -> Segmentation fault
A fresh power-on leaves the buffer empty, which is why the driver only
crashed on relaunch-after-crash, not on first bringup. connect() now runs
after ALL member initialization, immediately before the polling timer is
created.
2. Serial::open() now discards buffered stale input/output (tcflush
TCIOFLUSH) after configuring the port, so the first bytes the driver
parses are replies to its own requests rather than a dead process's
leftovers. Also stop calling ::close(fd) on a NEGATIVE fd in the
open-failure path: close(-1) clobbered errno with EBADF, making every
failed open report "Bad file descriptor" instead of the real reason
(e.g. ENOENT when the VESC is unplugged/unpowered).
3. timerCallback checked the init deadline BEFORE checking whether the
firmware-version reply had already arrived. The rx thread records the
version asynchronously, so a timer tick delayed past the deadline by
system load (plan launches rebuild images and run parallel colcon builds)
aborted a handshake that had already succeeded:
"FAIL: Timed out while trying to initialize VESC."
Success is now checked first, and the deadline is widened 2s -> 10s: it
only needs to catch a VESC that stays silent, so it can tolerate launch-
storm scheduling stalls without hiding real failures.
joystick fixes:
4. The no-device-found wait loop spun on rclcpp::ok(), but rclcpp::init()
was not called until AFTER the loop — ok() is false pre-init, so with no
controller present the loop fell through instantly and the driver exited
1 with an empty device path ("ERROR: Unable to open joystick device:").
rclcpp::init() now runs at the top of main(); the driver genuinely waits
(rescanning every 2s) for a controller to appear.
5. Config was loaded from a hardcoded ~/roboracer_ws/src/ut_automata/config
(with a /home/ntsoi fallback) — absent in containers, where sources live
at ~/workspace/src. Now resolved via
ament_index_cpp::get_package_share_directory("ut_automata") + "/config",
falling back to the legacy path for setups that never ran colcon install.
CMakeLists adds ament_index_cpp to the joystick target deps.
6. Skip the Bluetooth pairing sequence when a usable joystick device already
exists. bluetoothctl is unavailable inside containers, so the pairing
wait burned its full 60s timeout on every launch before the /dev/input
scan (which actually finds the controller) ever ran.
airfield.yaml:
7. Pass the whole /dev/input directory instead of only /dev/input/js0, and
add the input group (GID 996). The ILITEK touchscreen permanently owns
js0 on this Jetson, so a real game controller enumerates as js1+ and was
invisible to the container. Docker maps the nodes present at container
START (no hotplug): the controller must be powered on before the plan
comes up, otherwise restart the joystick pane.
Verified on-car: SIGKILL mid-telemetry then instant relaunch comes up clean
("Connected to VESC with firmware version 3.62", no out-of-sync banner);
80s no-controller run rejects the touchscreen ("Likely joystick: NO") and
keeps waiting instead of exiting; live teleop vesc pane reconnected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
.air was git-ignored, so the container mount config it carries — the /tmp/.X11-unix socket dir and gdm's Xauthority dir for the touchscreen's Xorg :0 — existed only on this machine and a fresh clone could not render the gui on the car's touchscreen. Track it like the rest of the airfield packaging (airfield.yaml). Panes drawing on the touchscreen run with DISPLAY=:0 XAUTHORITY=/run/user/2002/gdm/Xauthority (uid 2002 = orin). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ding vesc_driver.h declared `bool fuse_imu_ = true;` while vesc_driver.cpp binds CONFIG_BOOL(fuse_imu_, "fuse_imu"). Inside class methods the member shadows the config variable, so vesc.lua's `fuse_imu = false` landed in a variable nobody reads and IMU fusion was force-enabled on every car. On a car whose MPU6050 does not answer on the I2C bus (i2cdetect shows an empty bus; reads fail only after a ~0.4s kernel timeout each, and the sensor class merely prints "Error! Errno:" without failing), that meant every VESC telemetry packet triggered three timing-out I2C reads (~1.26s) INSIDE the serial packet handler. Telemetry collapsed from 20Hz to ~1.6Hz bursts: the GUI's Drive indicator flashed red/green and /imu published zero-filled garbage every ~1.26s (IMU indicator green with red blips). With the shadow removed the lua value applies: fuse_imu=false disables fusion and /car_status is back to a steady 20Hz (verified on-car, std dev 0.8ms). Cars with a working IMU keep fusion by setting fuse_imu=true. Companion commit in mpu6050driver adds a WHO_AM_I probe so an absent sensor throws at init (fusion then disables gracefully) instead of stalling reads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The roboracer_ws camera_calibration plan runs `ros2 run camera_calibration cameracalibrator` in this container, so the package must be installed in the image. Adds the dependency (resolved via the xplatform camera_calibration manifest). Not linked by ut_automata code, same rationale as urg_node. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add launch/lidar.launch.py, which picks the driver from what is plugged in -- a USB RPLIDAR C1 (rplidar_ros) when a /dev/ttyUSB* is present, else the Ethernet Hokuyo UST-10LX (urg_node). Both publish sensor_msgs/LaserScan on /scan with frame_id laser, so TF and everything downstream is unchanged. Replaces the Hokuyo-only launch/hokuyo_10lx.launch.py; start_car.launch.py now includes the new launch. airfield.yaml gains the rplidar_ros dependency and passes through /dev/ttyUSB0 (skipped when absent, so the Hokuyo setup is unaffected). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A crashed node (e.g. the joystick SEGFAULT) drops a multi-MB core file in the package workdir; keep it out of git status. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The .air file is airfield's per-machine local config: it lists container mounts, including host paths that embed the login user's numeric uid (/run/user/<uid>/gdm). This repo is shared course infrastructure -- cloned by students on lab machines and adapted for other robot platforms -- so host-specific values do not belong in it. The consuming project now keeps those mounts in its own project-level .air, which is gitignored there and also survives a re-clone of this repo by checkout.sh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
No description provided.