Add AckermannDriveStamped control to steering library #1563

wittenator · 2025-03-01T11:00:39Z

This PR adds the option to use steering angle and linear velocity for controllers that inherit from the steering library.

To send us a pull request, please:

Fork the repository.
Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
Commit to your fork using clear commit messages.
Send a pull request, answering any default questions in the pull request interface.
Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

wittenator · 2025-03-01T11:12:19Z

The tests on HEAD are currently broken, but I wanted to get this PR out in order to track progress.

christophfroehlich

why WIP?
Please fix the failing jobs, start with the pre-commit and clang job.
The tests on the master branch are green.

steering_controllers_library/src/steering_odometry.cpp

steering_controllers_library/include/steering_controllers_library/steering_odometry.hpp

mergify · 2025-05-24T18:33:34Z

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

christophfroehlich

I fixed the CMakeLists according to #1697. @wittenator do you have time to discuss the open conversations in this PR?

steering_controllers_library/src/steering_controllers_library.cpp

wittenator · 2025-06-09T09:22:52Z

@Juliaj How relevant are additional test cases for the inheriting controllers? Since I only added passthrough for the class variable in the controllers, tests for each inherited controller would essentially only test the functionality of the steering controller that is already covered and add a huge amount of boilerplate for the other controllers.

christophfroehlich

I'll wait for fixing the open conversations before the final review

edit: and please take care of the pre-commit errors (have you installed it?)

steering_controllers_library/test/test_steering_controllers_library_ackermann_input.hpp

Juliaj · 2025-06-12T17:16:06Z

@Juliaj How relevant are additional test cases for the inheriting controllers? Since I only added passthrough for the class variable in the controllers, tests for each inherited controller would essentially only test the functionality of the steering controller that is already covered and add a huge amount of boilerplate for the other controllers.

@wittenator, sorry for missing your comment. I noticed that you have updated the tests. Will take another look shortly.

Juliaj · 2025-06-13T01:27:23Z

steering_controllers_library/doc/userdoc.rst

 ----------------------------------

-The controller uses velocity input, i.e., stamped `twist messages <twist_msg_>`_ where linear ``x`` and angular ``z`` components are used.
+The controller uses velocity input, i.e., stamped `twist messages <twist_msg_>`_ where linear ``x`` and angular ``z`` components are used if ``twist_input == true``. If ``twist_input == false``, the controller uses `control_msgs/msg/SteeringControllerStatus <steering_controller_status_msg_>`_ where linear ``speed`` and angular ``steering_angle`` components are used.


Could we add more context for the inputs, for example

The controller uses velocity input, i.e., stamped `twist messages <twist_msg_>`_ where: - **Linear velocity (`linear x`)**: Represents the forward/backward motion of the robot along its x-axis (in meters per second, m/s). - **Angular velocity (`angular z`)**: Represents the rotational motion of the robot around its z-axis (in radians per second, rad/s). These components are used when ``twist_input == true``. If ``twist_input == false``, the controller uses `control_msgs/msg/SteeringControllerStatus <steering_controller_status_msg_>`_ where: - **Speed (`speed`)**: Represents the linear speed of the robot (in meters per second, m/s). - **Steering angle (`steering_angle`)**: Represents the angle of the steering joints (in radians, rad). Values in other components are ignored.

Sure, I can add this. One thing that comes up then is: What is a good way to describe the steering angle? The exact positions of the joints and wheels depends on both the steering angle as an abstract value and the exact kinematic setup of the robot. For an Ackermann steering geometry, the positions of the joints are different from each other and from the steering angle, so I had trouble finding a fitting description of it without mentioning something like a steering wheel or some common element that links the steering joints.

Are terminologies introduced at https://control.ros.org/rolling/doc/ros2_controllers/doc/mobile_robot_kinematics.html#mobile-robot-kinematics helpful to give a description ?

one could define the steering angle with the ICR being perpendicular to the imaginary single steering wheel.

I've added a small overview for both cases that describe the inputs and their physical equivalent in the docs.

Juliaj · 2025-06-13T02:45:56Z

@Juliaj How relevant are additional test cases for the inheriting controllers? Since I only added passthrough for the class variable in the controllers, tests for each inherited controller would essentially only test the functionality of the steering controller that is already covered and add a huge amount of boilerplate for the other controllers.

@wittenator, great work on the test coverage added in test_steering_controllers_library_steering_input.cpp!

I'd suggest adding one more test to cover the new use_twist_input parameter. This could be added to the existing on_configure test or in a dedicated test case to verify the parameter is properly handled during configuration.

Overall, these changes look solid!

mergify · 2025-07-24T19:44:32Z

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

christophfroehlich

I responded to your questions, could you please address them and fix the conflicts from the latest refactoring?

christophfroehlich · 2025-07-24T19:55:59Z

I'd suggest adding one more test to cover the new use_twist_input parameter. This could be added to the existing on_configure test or in a dedicated test case to verify the parameter is properly handled during configuration.

Isn't this implicitly done by the new tests using the yaml parmeterfile?

wittenator · 2025-08-06T20:48:39Z

I responded to your questions, could you please address them and fix the conflicts from the latest refactoring?

I'm on it and going to fix the conflicts over the coming days. Thanks for answering the remaining few questions!

christophfroehlich

Please also address the old open conversation about the argument and variable renaming.

christophfroehlich · 2025-08-13T08:51:12Z

steering_controllers_library/src/steering_odometry.cpp

    right_traction_wheel_vel, left_traction_wheel_vel, steer_pos_);
-  const double angular_velocity =
-    convert_steering_angle_to_angular_velocity(linear_velocity, steer_pos_);
+  const double angular_velocity = std::tan(steer_pos_) * linear_velocity / wheel_base_;


shouldnt it use convert_steering_angle_to_angular_velocity here?

Yes, most definitely, I used it now instead.

christophfroehlich · 2025-08-13T09:14:16Z

steering_controllers_library/src/steering_controllers_library.cpp

  msg.twist.angular.z = std::numeric_limits<double>::quiet_NaN();
 }

+void reset_controller_reference_msg(


do we need this specialization?

You're right, this is an unnecessary specialization. I removed it

steering_controllers_library/src/steering_controllers_library.cpp

christophfroehlich · 2025-08-13T09:22:52Z

steering_controllers_library/src/steering_controllers_library.cpp

+    // store (for open loop odometry) and set commands
+    last_linear_velocity_ = timeout ? 0.0 : reference_interfaces_[0];
+    last_angular_velocity_ = timeout ? 0.0 : reference_interfaces_[1];


why do we need this here, and haven't needed it before?

Sorry, this slipped in from some playing around in the code. I removed it now

christophfroehlich · 2025-08-13T09:32:41Z

steering_controllers_library/test/test_steering_controllers_library_steering_input.hpp

+  std::string steering_interface_name_ = "position";
+  // defined in setup
+  std::string traction_interface_name_ = "";
+  std::string preceeding_prefix_ = "pid_controller";


Suggested change

std::string preceeding_prefix_ = "pid_controller";

std::string preceding_prefix_ = "pid_controller";

we already discussed this, haven't this changed?
However, this is not used at all. Please don't copy dead code.

Co-authored-by: Christoph Fröhlich <[email protected]>

wittenator added 8 commits March 1, 2025 08:50

Add additional tests

768ef15

Merge branch 'ros-controls:master' into master

e22df84

Remove twist_input from closed loop odometry calculation

36bc9d5

Merge branch 'ros-controls:master' into master

94eebd0

Add in missing test case

c0f103e

Merge branch 'master' of github.com:wittenator/ros2_controllers

0564ada

Fixed comment

eaebf09

Update on ackermann message

3e84edd

github-actions bot requested review from anfemosa, christophfroehlich, destogl, erickisos, mhubii, moriarty and rosterloh March 1, 2025 11:00

wittenator changed the title ~~Add AckermannDriveStamped control to steering library~~ WIP: Add AckermannDriveStamped control to steering library Mar 1, 2025

wittenator mentioned this pull request Mar 1, 2025

WIP: Add AckermannDriveStamped control to steering library #1171

Closed

6 tasks

wittenator and others added 3 commits March 3, 2025 14:03

Merge branch 'master' into master

4f14831

Merge branch 'master' into master

e20e63c

Merge branch 'master' into master

031ddc6

christophfroehlich requested changes Mar 19, 2025

View reviewed changes

steering_controllers_library/src/steering_odometry.cpp Show resolved Hide resolved

steering_controllers_library/include/steering_controllers_library/steering_odometry.hpp Outdated Show resolved Hide resolved

wittenator added 8 commits April 6, 2025 18:16

Merge branch 'master' into master

7564e34

Fix reference name

0c3ded5

Merge branch 'master' of github.com:wittenator/ros2_controllers

a15dc7f

Merge branch 'ros-controls:master' into master

66effcf

Fix typo

d204e7e

Merge branch 'master' of github.com:wittenator/ros2_controllers

948b67c

Fix tests

c612f3c

Fix exported interfaces tests

915e3bd

Merge remote-tracking branch 'origin/master' into wittenator/master

f159c60

christophfroehlich requested changes May 28, 2025

View reviewed changes

steering_controllers_library/src/steering_controllers_library.cpp Outdated Show resolved Hide resolved

Merge branch 'ros-controls:master' into master

1fd197c

wittenator added 2 commits June 9, 2025 09:40

Rename various tests and variables

9c8bb76

Merge commit

f150a9b

christophfroehlich requested changes Jun 10, 2025

View reviewed changes

steering_controllers_library/test/test_steering_controllers_library_ackermann_input.hpp Outdated Show resolved Hide resolved

steering_controllers_library/test/test_steering_controllers_library_ackermann_input.hpp Outdated Show resolved Hide resolved

wittenator mentioned this pull request Jun 11, 2025

Add message to control a robot via linear velocity and steering position ros-controls/control_msgs#217

Merged

wittenator added 5 commits June 12, 2025 08:53

Merge branch 'master' of github.com:wittenator/ros2_controllers

d01dd0f

Fix formatting

1b7cf57

Merge branch 'master' into master

990d2fe

Added consistent naming in tests and fixed docs

ef29f5f

Merge branch 'master' of github.com:wittenator/ros2_controllers

9d8bd04

Juliaj reviewed Jun 13, 2025

View reviewed changes

christophfroehlich requested changes Jul 24, 2025

View reviewed changes

wittenator added 3 commits August 11, 2025 11:09

Merge remote-tracking branch 'upstream/master'

aebb8be

fix tests

39dafe9

Format with precommit

ed7a782

christophfroehlich requested changes Aug 13, 2025

View reviewed changes

wittenator and others added 5 commits September 28, 2025 11:00

Merge branch 'ros-controls:master' into master

3f64a4d

Merge branch 'ros-controls:master' into master

dd64226

Revert naming in doc string

e0a58db

Co-authored-by: Christoph Fröhlich <[email protected]>

Integrated more feedback

48a66cc

Merge branch 'master' of github.com:wittenator/ros2_controllers

4adb592

	std::string preceeding_prefix_ = "pid_controller";
	std::string preceding_prefix_ = "pid_controller";

Add AckermannDriveStamped control to steering library #1563

Are you sure you want to change the base?

Add AckermannDriveStamped control to steering library #1563

Uh oh!

Conversation

wittenator commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wittenator commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mergify bot commented May 24, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wittenator commented Jun 9, 2025

Uh oh!

christophfroehlich left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Juliaj commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Juliaj commented Jun 13, 2025

Uh oh!

mergify bot commented Jul 24, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

christophfroehlich commented Jul 24, 2025

Uh oh!

wittenator commented Aug 6, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

wittenator commented Mar 1, 2025 •

edited

Loading

wittenator commented Mar 1, 2025 •

edited

Loading

christophfroehlich left a comment •

edited

Loading