Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tricycle controller #345

Merged
merged 18 commits into from
Aug 3, 2022
Merged

Tricycle controller #345

merged 18 commits into from
Aug 3, 2022

Conversation

tonynajjar
Copy link
Contributor

@tonynajjar tonynajjar commented May 2, 2022

Hey! We at Pixel Robotics recently started our migration to ros2_control. We are in the process of creating a tricycle controller and decided to contribute back.

@AndyZe
Copy link
Contributor

AndyZe commented May 3, 2022

Random question, out of curiosity: does this work for (steered wheel in the front, 2 in the back) as well as (2 wheels in front, steered wheel in the back)?

@tonynajjar
Copy link
Contributor Author

tonynajjar commented May 3, 2022

Isn't what you described the same configuration, just turned around?

@tonynajjar
Copy link
Contributor Author

Friendly ping, when can I expect to get some feedback on this PR? Thanks

Copy link
Member

@arne48 arne48 left a comment

Choose a reason for hiding this comment

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

Adding new controllers for new drive-train configurations should be always welcome.
For this repository there was a previous effort started to provide an Ackermann-Steering controller #288 #264.

And I am wondering because the "inverse" tricycle configuration with two driven wheels in the front would be a good candidate to use an ackermann-steering-controller.
So could the "tricycle" configuration (one driven wheel in the front) maybe just be considered as a specific wheel configuration of the same system at least from the kinematic point of view?

If my assumption is correct maybe a good next step could be to resurrect the efforts for the ackermann-steering-controller and giving it a wider choice of parameters so that your configuration would be covered as well.

My preference to make the tricycle controller a configuration for the general ackermann-steering-controller is based on the ackermann's versatility and that it is still missing for ROS2.

@tonynajjar
Copy link
Contributor Author

tonynajjar commented May 13, 2022

Hi @arne48. I do agree that the tricycle controller might be a special case of a more general ackermann controller that can be achieved with additional configuration.
However, I think we could add a tricycle controller as a first step and see from there how to generalize it to ackermann. I am given the time to work on the tricycle configuration because this is what my company uses; I'm not sure I'll have time to work on an ackermann controller right now and it would be a shame to discontinue this initial effort, which works well enough as-is and can be easily extended to ackermann.

@tonynajjar tonynajjar requested a review from arne48 May 13, 2022 07:34
Copy link
Member

@arne48 arne48 left a comment

Choose a reason for hiding this comment

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

I agree with you that it would be a shame to let a good working controller go to waste.
Also it seems that the development/migration of the ackermann-steering-controller is currently stalled somehow.
And in this case I could see the merit of an easy to use specialized controller.
Especially for tricycles, which I think are likely to be a common drive train design for mobile robots besides differential and holonomic setups.

But this is finally a decision for the maintainers of the ros2_controllers package like @bmagyar and if they want to add more specialized controllers and the thereby growing codebase for cases which could otherwise also be covered with more general controllers through configuration.

As you said during the absence of the ackermann-steering-controller this could be added as a temporary solution but if the ackermann-steering-controller should come, those temporary controllers and their replacement would cause problems in future releases where this controller then got replaced.
So once a controller got added - at least in my opinion - it should be a permanent part of the package.

So now before we would go into a more detailed code review I would like to wait for the opinion of the maintainers in regards to this topic.

Lets hope for a positive response :)

@tonynajjar
Copy link
Contributor Author

if the ackermann-steering-controller should come, those temporary controllers and there replacement would cause problems in future releases where this controller then got replaced.

I totally understand this point now. It's my first time contributing to a project where breaking changes affect many people so I didn't give it much thought 😅.

@tonynajjar
Copy link
Contributor Author

While investigating on how to turn this into a more general-purpose ackermann controller I stumbled on http://wiki.ros.org/steer_bot_hardware_gazebo.

Their approach for navigating an ackermann vehicle was to generate tricycle commands with a tricycle controller (similar to the one in this PR) and transform these to ackermann commands in the hardware interface.

Do you consider the transformation tricycle->ackermann to be the responsibility of the controller or the hardware interface?

@bmagyar I'd appreciate guidance on how to continue with this PR.

@tonynajjar

This comment was marked as resolved.

@arne48
Copy link
Member

arne48 commented Jul 3, 2022

@bmagyar I think we should add this controller. As it seems that in the meatime the efforts for general Ackermann controller are currently are currently stagnant, so this controller could cover most use-cases and therefore has a stand-alone value.
Plus it is rather simple and shouldn't be too hard to maintain especially because I would guess @tonynajjar at Pixel Robotics will continue to use their controller and might have a look at potential issues that arise.
So can we proceed to integrate this controller into the package?

tricycle_controller/CMakeLists.txt Outdated Show resolved Hide resolved
tricycle_controller/CMakeLists.txt Outdated Show resolved Hide resolved
tricycle_controller/CMakeLists.txt Show resolved Hide resolved
tricycle_controller/src/tricycle_controller.cpp Outdated Show resolved Hide resolved
tricycle_controller/src/tricycle_controller.cpp Outdated Show resolved Hide resolved
@bmagyar
Copy link
Member

bmagyar commented Jul 16, 2022

Also please rebase this PR against the latest master branch

Tony Najjar and others added 4 commits July 17, 2022 16:47
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
@tonynajjar
Copy link
Contributor Author

tonynajjar commented Jul 18, 2022

Thanks for the review @bmagyar. I synced to master and also added our latest changes which were on a different branch.

This PR still needs some work, not everything is well tested, especially the features we don't use. Now that I have your confirmation that this is the way forward, I can work on these. Will request another review once it's ready.

@tonynajjar
Copy link
Contributor Author

You might have noticed that the rate limiters actually limit the steering angle and traction speed as opposed to the Twist (e.g in the diff drive controller). I think that Twist limiting should be done in the local planner/controller (e.g TEB does it).
I implemented it like that mainly so that our gazebo simulation could be more realistic, e.g so that the steering angle change is slower. Rate limiting on our real motors is not needed as they have that feature built-in.
Happy to hear opinions.

@tonynajjar
Copy link
Contributor Author

Ready for review again, I made a demo also so you can try it out: https://github.com/ros-controls/gazebo_ros2_control/pull/145/files

@tonynajjar tonynajjar requested review from bmagyar and arne48 July 23, 2022 11:43
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
@tonynajjar
Copy link
Contributor Author

tonynajjar commented Jul 26, 2022

@bmagyar is there anything in the CI I need to fix? I think the failures are not related to my changes

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Aug 3, 2022

Hi, can I know what is still needed to move this forward? It's in the back of my mind for a long time now.

@bmagyar
Copy link
Member

bmagyar commented Aug 3, 2022

🚢

Thanks for sticking through the process!

@bmagyar bmagyar merged commit 279ba38 into ros-controls:master Aug 3, 2022
@tonynajjar
Copy link
Contributor Author

@bmagyar thank you for the review and thank you @pixel-robotics (@jplapp) for financing the time put into this. Looking forward for community feedback and improving it

mamueluth pushed a commit to StoglRobotics-forks/ros2_controllers that referenced this pull request Aug 26, 2022
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.

5 participants