Skip to content

Conversation

nitin2606
Copy link
Contributor

Description
This PR introduces the swerve_drive_controller, a new controller for swerve drive robots with four independently steerable wheels, enabling omnidirectional motion in ROS 2. It complements controllers like diff_drive_controller by supporting advanced mobile robot platforms.

Features

Supports geometry_msgs/msg/Twist or TwistStamped velocity inputs (x, y linear; z angular).
Publishes raw nav_msgs/msg/Odometry for user-defined post-processing.
Publishes /tf transforms (optional, if enable_odom_tf=true).
Configurable via YAML for wheel geometry and kinematic constraints.
Changes
Added swerve_drive_controller package (src/, include/, test/).
Exported as a pluginlib plugin (swerve_drive_controller_plugin.xml).
Updated ros2_controllers CMakeLists.txt and package.xml.
Included gtest tests (test/test_swerve_drive_controller.cpp) and config (test/config/test_swerve_drive_controller.yaml).

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution. Could you please fix the failing tests? A very quick first review below.

Do you mind add a brief section in the kinematics section of the docs, even in a new PR to get it merged faster? Thanks!

@nitin2606
Copy link
Contributor Author

I'll make changes and push.

@christophfroehlich
Copy link
Contributor

do you mind cherry-picking the changes to the docs in a separate PR? Splitting this up would help to reviewing things.

@nitin2606
Copy link
Contributor Author

do you mind cherry-picking the changes to the docs in a separate PR? Splitting this up would help to reviewing things.

I have raised a separate PR for changes in docs. PR #1712

Copy link
Member

@Maverobot Maverobot left a comment

Choose a reason for hiding this comment

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

Hi there, thanks for your contribution! I've started reviewing the changes and have left a few comments. I will try to find time to work through it and will follow up once I'm done.

Copy link
Contributor

mergify bot commented Jun 24, 2025

This pull request is in conflict. Could you fix it @nitin2606?

Copy link

codecov bot commented Jul 22, 2025

Codecov Report

❌ Patch coverage is 83.21918% with 98 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.15%. Comparing base (09f2dd6) to head (c670b2e).

Files with missing lines Patch % Lines
...e_drive_controller/src/swerve_drive_controller.cpp 74.44% 47 Missing and 22 partials ⚠️
...e_controller/test/test_swerve_drive_controller.hpp 84.68% 9 Missing and 8 partials ⚠️
...werve_drive_controller/swerve_drive_controller.hpp 76.00% 4 Missing and 2 partials ⚠️
...e_controller/test/test_swerve_drive_controller.cpp 95.41% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1694      +/-   ##
==========================================
- Coverage   85.22%   85.15%   -0.08%     
==========================================
  Files         142      148       +6     
  Lines       13735    14318     +583     
  Branches     1191     1261      +70     
==========================================
+ Hits        11706    12192     +486     
- Misses       1632     1698      +66     
- Partials      397      428      +31     
Flag Coverage Δ
unittests 85.15% <83.21%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...e_drive_controller/src/swerve_drive_kinematics.cpp 100.00% <100.00%> (ø)
...troller/test/test_load_swerve_drive_controller.cpp 100.00% <100.00%> (ø)
...werve_drive_controller/swerve_drive_controller.hpp 76.00% <76.00%> (ø)
...e_controller/test/test_swerve_drive_controller.cpp 95.41% <95.41%> (ø)
...e_controller/test/test_swerve_drive_controller.hpp 84.68% <84.68%> (ø)
...e_drive_controller/src/swerve_drive_controller.cpp 74.44% <74.44%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@Amronos Amronos left a comment

Choose a reason for hiding this comment

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

I have done a rough review right now and haven't gone through the main source code properly.
Aside from my other review comments, I noticed the following:

  1. You have the GPL parameters yaml, but aren't using GPL properly anywhere in your source code.
  2. Why not make this controller a chained controller?
  3. Are you using the new APIs here? (Look at #1566 and #1830)

Copy link
Contributor

@Amronos Amronos left a comment

Choose a reason for hiding this comment

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

Please add my suggestions to batch on the files changed tab and commit from there. This will make it easier to review.

@nitin2606 nitin2606 requested a review from Amronos July 28, 2025 09:38
@nitin2606
Copy link
Contributor Author

@Amronos @christophfroehlich Could you please review when you get a chance?

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

To emphasize the last commit above: Please either use the github UI to accept proposed changes, or leave the conversations open so that the reviewing person can immediately check if they have been addressed or a new comment was added. Only the reviewer should manually close the conversations. This improves effectivity of the next review round.

Please fix the failing linter jobs: Install pre-commit via pre-commit install for the next commits; the current issues can be fixed with pre-commit run --all from the repo.

@nitin2606
Copy link
Contributor Author

To emphasize the last commit above: Please either use the github UI to accept proposed changes, or leave the conversations open so that the reviewing person can immediately check if they have been addressed or a new comment was added. Only the reviewer should manually close the conversations. This improves effectivity of the next review round.

Please fix the failing linter jobs: Install pre-commit via pre-commit install for the next commits; the current issues can be fixed with pre-commit run --all from the repo.

I have attempted to resolve the linter issues, but I am still encountering a persistent error reported by the pre-commit check at line 152 of the file: https://github.com/nitin2606/ros2_controllers/blob/feature/swerve-drive-controller/swerve_drive_controller/test/test_swerve_drive_controller.cpp
Could you kindly review it and help me identify the underlying issue?

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

A first review round, haven't gone over the kinematics implementation yet.

#ifndef TEST_SWERVE_DRIVE_CONTROLLER_HPP_
#define TEST_SWERVE_DRIVE_CONTROLLER_HPP_

#include <gtest/gtest.h>
Copy link
Contributor

@christophfroehlich christophfroehlich Aug 18, 2025

Choose a reason for hiding this comment

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

we use gmock instead

Suggested change
#include <gtest/gtest.h>
#include <gmock/gmock.h>

if(BUILD_TESTING)
find_package(controller_manager REQUIRED)
find_package(ament_cmake_gmock REQUIRED)
find_package(ament_cmake_gtest REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
find_package(ament_cmake_gtest REQUIRED)

find_package(ament_cmake_gtest REQUIRED)
find_package(ros2_control_test_assets REQUIRED)

ament_add_gtest(test_swerve_drive_controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ament_add_gtest(test_swerve_drive_controller
ament_add_gmock(test_swerve_drive_controller

Comment on lines 166 to 169
if (!init_successful_)
{
GTEST_SKIP() << "Skipping due to on_init failure";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We use gmock in our projects, please use a pattern like this

Suggested change
if (!init_successful_)
{
GTEST_SKIP() << "Skipping due to on_init failure";
}
ASSERT_TRUE(init_successful_) << "Skipping due to on_init failure";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it I'll make necessary changes


int main(int argc, char ** argv)
{
::testing::InitGoogleTest(&argc, argv);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
::testing::InitGoogleTest(&argc, argv);
::testing::InitGoogleMock(&argc, argv);

odometry_message.pose.pose.orientation.y = orientation.y();
odometry_message.pose.pose.orientation.z = orientation.z();
odometry_message.pose.pose.orientation.w = orientation.w();
realtime_odometry_publisher_->unlockAndPublish();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the publisher API like in #1830

auto node = get_node();
if (!node)
{
RCLCPP_ERROR(rclcpp::get_logger("SwerveController"), "Node is null in on_init");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you always creating a new logger? Please use

Suggested change
RCLCPP_ERROR(rclcpp::get_logger("SwerveController"), "Node is null in on_init");
RCLCPP_ERROR(node->get_logger(), "Node is null in on_init");

instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applies to all occurrences

Comment on lines 608 to 611
static_assert(
!std::is_const_v<std::remove_reference_t<decltype(*command_handle)>>,
"Command handle is const!");
return std::make_unique<T>(std::ref(*command_handle), std::ref(*state_handle), name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please elaborate why you used this pattern? When should this ever be constant?
In other controllers, we use std::reference_wrapper instead of a unique ptr

std::reference_wrapper<hardware_interface::LoanedCommandInterface> velocity;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this change was suggested by one of the reviewers.

Comment on lines 114 to 119
publishe_rate: {
type: double,
default_value: 50.0, # Hz
description: "Publishing rate (Hz) of the odometry and TF messages.",
read_only: true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the publish rate. We decided to remove this from all controllers, they should publish with the update rate of the controller.
#473

return odometry_;
}

double SwerveDriveKinematics::normalize_angle(double angle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use angles::normalize_angle from angles/angles.h instead of adding a new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make all the mentioned changes

Copy link

@wsxq2 wsxq2 left a comment

Choose a reason for hiding this comment

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

I find a problem.

Comment on lines +404 to +406
double x_offset = wheel_params_.x_offset;
double y_offset = wheel_params_.y_offset;
double radius = wheel_params_.radius;
Copy link

Choose a reason for hiding this comment

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

Hi, these three variables are not being used, but they are useful. The x_offset and y_offset can be used to calculate wheel positions. I see that in your code the wheel positions are hardcoded, which doesn't seem reasonable.

Also, the radius can be used to calculate the wheel's angular velocity. I notice you're directly sending linear velocity to the hardware component, which also doesn't seem reasonable, since wheel speeds are typically expressed as angular velocities.

Copy link
Contributor

mergify bot commented Aug 25, 2025

This pull request is in conflict. Could you fix it @nitin2606?

@nitin2606
Copy link
Contributor Author

nitin2606 commented Aug 29, 2025

@christophfroehlich, could you kindly review the changes in test_swerve_drive_controller.cpp line no 29. While the code appears to be correct, it is still failing the pre-commit checks. I would also greatly appreciate it if you could review the other changes you suggested.

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.

Add a controller for afour wheel independent steering / drive mobile base, aka a swerve drive
6 participants