Added package for steer_drive_controller. by MoriKen254 · Pull Request #289 · ros-controls/ros_controllers

MoriKen254 · 2017-07-18T13:14:17Z

I developed steer_drive_controller for 4 wheel autonomous robot, which is made based on the concept of diff_drive_controller. Thus I would like this package to be one of official ros_controllers to contribute research on autonomous driving.

The following video shows demonstration of our robot with the controller on Gazebo. We actually succeeded in driving the real robot and won a competition in Japan. More information on the robot is described on http://wiki.ros.org/Robots/CIR-KIT-Unit03.

Detailed documentation is currently posted on http://wiki.ros.org/steer_drive_controller. Test codes passed on industrial_ci system on https://github.com/CIR-KIT/steer_drive_ros.

We also provide whole sample system where steer_drive_controller works properly. The important repository is http://wiki.ros.org/cirkit_unit03_apps. If you want to check functionality of the controller on Gazebo, please try the following commands on your PC.

(in your workspace)
$ git clone -b kinetic-devel https://github.com/CIR-KIT-Unit03/cirkit_unit03_apps
$ cd cirkit_unit03_apps
$ sh install.sh

The script resolves all dependencies properly. Then please launch the following autorun file after catkin_make.

$ roslaunch cirkit_unit03_autorun autorun_gazebo.launch

Then the world shown on the Youtube video is supposed to appear and ready for navigation, which means 2d nav goal from RViz is available.

This is my first time for pull request so I might have made mistakes on license and author stuff. For instance, I just added my name as an author to each code I modified, which was originally from diff_drive_controller. I was not sure if this way was correct. This is my first trial to request so please let me know if I did a wrong thing.

I hope I could contribute to enhancement of ros_controller by this.

Best regards.

graiola · 2017-07-18T16:13:42Z

Hello,

Thanks for the contribution!
I don't know if you noticed, but there is already an open pull request to add a four wheels steering controller (see #280).
I suggest to join the efforts with @vincentrou to see what are the similarities and the differences between the two controllers so that we don't have duplicated pull requests.

MoriKen254 · 2017-07-19T00:14:03Z

Thank you for your reply.

Oh, excuse me. I didn't notice the similar one was already going on.

Ok, I'm glad to join the effort of #280 with @vincentrou to solve current problems they are working on. Moreover, I'm gonna check and compare both packages and try to merge them into just one pull request.

bmagyar · 2017-07-19T10:15:46Z

Amazing, thanks guys! Having the two controllers merged would be great!

MoriKen254 · 2017-07-19T15:37:48Z

@graiola @bmagyar

Thank for your response!

Main difference between controller of #280 & one of #289

I deeply looked inside of four_wheel_steering_controller by @vincentrou (#280), and found a big difference from steer_drive_controller on this request (#289). Please have a look at the following figure.

In conclusion, target application robot might be different each other.

In detail

four_wheel_steering_controller by @vincentrou uses 4 position interfaces and 4 velocity interfaces because it assumes each wheel has both steer and wheel joint actuators. It seems to be applicable for rovers.

On the other hand, steer_drive_controller which I developed uses only one position interface and one velocity interface because it assumes to handle a general car-like mobile robot mechanism which consists of an ackermann link for steering and a differential gear for rear wheel driving. (sorry it hasn't been compatible with ackermann_msg yet, meaning it only recieves geometry_msgs::Twist for now, but I can add it soon if needed.)

Actually, we had to use this configuration to apply it to our real car-like robot. When we used Gazebo, we resolved a closed link problem by creating a specific RobotHWSim class. We decided to take this configuration in order to make it be able to be used more generally for various car-like robots. Please refer http://wiki.ros.org/steer_bot_hardware_gazebo for detailed documentation about this concept.
(The robot in the Youtube video on first comment of this request uses this function actually.)

Thus, I came to believe a target robot of #280 & that of #289 might be different so I beg your suggestion.

What would we do?

Now, I think there are two ways to solve this issue.

merge Add four_wheel_steering_controller #280 & Added package for steer_drive_controller. #289 into single controller and switch functionality by a parameter
separate Add four_wheel_steering_controller #280 & Added package for steer_drive_controller. #289 controllers

The first one can reduce the number of controller packages while codes in the package might be complicated.

The second one will increase the number of controller packages while codes in each package would be simpler. I feel this way makes cohesion higher because the target robot differs.

Which would you think is better? Or is there another better solution?

vincentrou · 2017-07-21T15:34:57Z

Thank you for this detailed comparison.
Yes in fact, our packages are quite different.
I do not think that we can merge it since one is controlling 8 joints and the other 2 joints.

There is two ways for me :

separate Add four_wheel_steering_controller #280 & Added package for steer_drive_controller. #289 controllers (as you said)
use the four_wheel_steering_controller instead of the steer_drive_controller :
- the real robot base controller can use the 2 rear speed and 2 front steering to command the robot
- the real robot base controller can fake the 8 joints base on his rear speeds and front steering
1. the pros :
- the command can always have the rear steering to 0 and you will have the same turning behavior
- no need to have an other controller for gazebo
1. the cons :
- extra computation for the real robot since 8 joints will be computed instead of 2
- extra computation for the simulated robot since 8 joints will be computed instead of 4

MoriKen254 · 2017-07-22T14:59:43Z

@vincentrou
Thanks for your suggestion!

Your second idea looks great. It'll cover more various configurations.

I wanna check one thing on that idea. As you said the real robot base controller can't take a rear speed and a steer position command. Maybe the controller must convert 2 rear speed and 2 front steering commands into one rear speed and one front steering respectively. The flow converting two commands into one seems somewhat strange for me.

Then I have another idea. How about creating another class such like JointIFManager so that we can merge both of our requirement? Yes, I've come to think about possibility of merging inspired from your second idea.

The following figure shows my current idea about modified class diagram.

As you talked in Add four_wheel_steering_controller #280, I firstly assumed there is WheelDriveController abstract super class having common SpeedLimiter with implementation.
WheelDriveController has two abstract classes Odometry and JointIFManager.
JointIFManager handles joint command conversion related processes.
DiffDriveController and FourWheelSteeringController inherits WheelDriveController and uses sub classes of Odometry and JointIFManager according to each functionality.
FourWheelSteeringController uses two types JointIFManager.
- FourWheelSteeringJointIFManager has a role same as you mentioned.
- CarLikeSteeringJointIFManager has a role to handle general car-like robot as I've been mentioning.
- The two functions will be switched by a parameter.

Yes, this is NOT just merging #280 & #289 into one code but enables both rover-like function and car-like function to coexist with keeping separated each other.

More than welcome comments.

vincentrou · 2017-08-03T16:04:34Z

I am not sure it is a good idea to make multiple controller coexisting in the same code.

My idea was to use the four_wheel_steering_controller as it is now for your car-like vehicle.
You can use the four_wheel_steering_controller without modification but in your real robot node you need to convert the 4 velocity and position to your 1 velocity and front position.
In gazebo there is nothing special to do.
But you will need an urdf with 4 position and 4 velocity joint.
Here is an attempt to draw it :

On the real robot

In simulation :

MoriKen254 · 2017-09-04T11:00:54Z

@vincentrou
Sorry for late reply. As you mentioned, our controllers have a different concept each.
I agree with your comment on #280.

Maybe we can share some super class like this. (will be different in exact)

bmagyar · 2017-09-04T12:51:10Z

Thank you guys, you are awesome to have posted all the analysis.

Please bear with us while we review your pull requests.

bmagyar · 2017-09-06T10:29:48Z

@@ -0,0 +1,79 @@
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


This file should be removed

bmagyar · 2017-09-06T10:31:09Z

+project(steer_drive_controller)
+
+find_package(catkin REQUIRED COMPONENTS
+  controller_interface


please sort these dependencies

(in vim you can do it by selecting these lines in visual mode and :sort )

bmagyar · 2017-09-06T10:31:23Z

+  INCLUDE_DIRS include
+  LIBRARIES ${PROJECT_NAME}
+  CATKIN_DEPENDS
+  roscpp


please sort these dependencies
(in vim you can do it by selecting these lines in visual mode and :sort )

bmagyar · 2017-09-06T10:32:30Z

+
+Controller for a steer drive mobile base.
+
+## 仕様


Please use English, I'd love to learn Japanese but I feel this is not the right place or time for it :)

Oops, I'm so sorry. I forgot to remove Japanese stuff from here. I added a link to the English ROS Wiki page currently :)

bmagyar · 2017-09-06T10:33:34Z

+   * \brief The Odometry class handles odometry readings
+   * (2D pose and velocity with related timestamp)
+   */
+  class Odometry


is this Odometry class different from the original one from the diff_drive_controller, if yes, how?

Yes, it is. In odometry.cpp, update() method follows car-like steer drive kinematics.

bmagyar · 2017-09-06T10:33:53Z

+namespace steer_drive_controller
+{
+
+  class SpeedLimiter


is this class different from the original one from the diff_drive_controller, if yes, how?

No, it isn't. SpeedLimitter class is same as the diff_drive_controller's one.

Can you include it from there?

Fixed in MoriKen254@1c26e56 and MoriKen254@4f00fab.

bmagyar · 2017-09-06T10:35:17Z

+  {
+    // constants
+  private:
+    enum INDX_WHEEL {


I couldn't find a reference to these indices anywhere. It's also bad practice to rely on positions in arrays.

Ok, I'll remove INDX_WHEEL indices. Could you please which way is better to specify positions without arrays? I would like to follow a better way if possible.

The general practice where you'd use arrays is when you have something global or semi-global storing all the data and then parts of the code "magically know" which index they need. A practice that limits the scope of functions is passing handles to data a given function needs to have access to. I'm not sure this directly applies here, as in this case the issue I raised was that there's simply no reference to those enum indices :)

Fixed in MoriKen254@4abe230.

bmagyar · 2017-09-06T10:35:54Z

+  <license>BSD</license>
+
+  <url type="bugtracker">https://github.com/CIR-KIT/steer_drive_ros/issues</url>
+  <url type="repository">https://github.com/CIR-KIT/steer_drive_ros.git</url>


these links should be updated

bmagyar · 2017-09-26T12:38:43Z

A general but important note that I forgot to add:
we need to think about naming. We are going to have four_wheel_steering_controller which is pretty explicit in what is does. Do you think we could rename yours to something along the lines of ackermann_steering_controller?

bmagyar · 2018-01-11T11:33:13Z

ping @MoriKen254

MoriKen254 · 2018-01-25T11:53:22Z

@bmagyar

Thank you very much for pinging me.
Excuse me. Please give me a time. I'll do my best from now!

The test still fails for some reason, but at least it compiles now. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

…we want to wait stateCB after waitForState find new states

…r topics than when we confirmed with waitForState function

…ing to 'enabled/disabled'

…drive-controller-kinetic

MoriKen254 · 2018-08-16T23:36:11Z

@bmagyar

Sorry for making you wait for a long term (which is because of my very private reason though...). I've been thinking about this request during the term and now I'm ready to follow your review.

Please give me another chance and let me resume this request to be completed.

MoriKen254 · 2018-08-17T05:18:03Z

Fixed @bmagyar's #289 (comment) in MoriKen254@d89255a.

bmagyar · 2018-08-20T17:27:00Z

@MoriKen254 Glad you are still around!

Sorry to be a pain but could you please rebase this Pull Request on the latest melodic-devel?
This should resolve the current conflicts. I find it odd that we have conflicts on changelogs and package.xml-s...

MoriKen254 · 2018-08-21T14:37:17Z

Thank you for your response! Glad too still your welcoming this request.

So sorry on the conflicts. it's my fault. I just actually wanted to merge into the newest ver. of kinetic-devel first...

Please let me create another pull request to sum up all reviews here for now into a single commit safely and cleanly. I have got confused about the branch of mine... Sorry about that.

#356

Please let me know if this causes problems.

mathias-luedtke · 2018-11-11T10:38:41Z

merged #356

Added package for steer_drive_controller.

79a39fc

This was referenced Jul 18, 2017

Kinetic上のGazeboで前輪が回りにくい CIR-KIT-Unit03/cirkit_unit03_simulator#20

Closed

steer_drive_controller のPull requestを本家に出すか CIR-KIT/steer_drive_ros#4

Closed

MoriKen254 mentioned this pull request Jul 19, 2017

Add four_wheel_steering_controller #280

Merged

bmagyar requested a review from efernandez September 4, 2017 12:50

bmagyar reviewed Sep 6, 2017

View reviewed changes

MoriKen254 added 5 commits September 9, 2017 20:32

Remove CHANGELOG.rst

ebe2d32

Sort package names

30f872f

Update README in English with a link to ROS Wiki page.

fd21fb9

Fix package related links in package.xml

2c715dd

Remove unnecessary files.

4ae42f1

bmagyar added the enhancement label Sep 26, 2017

bmagyar added the new controller label Oct 13, 2017

bmagyar and others added 5 commits April 26, 2018 08:22

Update travis for melodic

667596f

Make the compiler happy in the test.

a9e6507

The test still fails for some reason, but at least it compiles now. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Update changelogs

938caea

0.14.0

89aaf6c

Use a copy of the pointer in update()

7865360

k-okada and others added 12 commits May 15, 2018 11:12

isPublishngCmdVelOut to check getNumPublisheres until timeout

60d12d9

joint_trajectory_controller_test.cpp adds waitForNextStates, because …

430d0b8

…we want to wait stateCB after waitForState find new states

use short_timeout in jointVelocityFeedForward

79adb69

emptyTopicCancelsActionTraj: make sure that stateCB received the newe…

79a4bc5

…r topics than when we confirmed with waitForState function

reloadController, forget to explictly 'return true'

b3f8ddb

ignoreOldTopicTraj add waitForNextState

0f0fbbb

add gtest_filter argument

3182c1e

Added 'multiplier' in DynamicParams ostream and changed boolean print…

d43e5a2

…ing to 'enabled/disabled'

Update changelogs

b112b1c

0.14.1

ffe44f3

Update README.md

4d203d6

Update maintainers

172b84f

MoriKen254 mentioned this pull request Aug 16, 2018

Is there a working example in Gazebo? CIR-KIT/steer_drive_ros#22

Closed

Merge https://github.com/ros-controls/ros_controllers into add-steer-…

e860f80

…drive-controller-kinetic

MoriKen254 added 4 commits August 17, 2018 12:51

[Modify] include speedlimitter from diff_drive_controller's one

1c26e56

[Delete] speed_limitter.cpp from new controller

4f00fab

[Delete] unecessary indices

4abe230

Rename to ackermann_steering_controller

d89255a

MoriKen254 mentioned this pull request Aug 21, 2018

Add ackermann_steering_controller to kinetic-devel #356

Merged

mathias-luedtke closed this Nov 11, 2018

mathias-luedtke mentioned this pull request Nov 11, 2018

Unify drive controllers with common base package #377

Open

Conversation

MoriKen254 commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graiola commented Jul 18, 2017

Uh oh!

MoriKen254 commented Jul 19, 2017

Uh oh!

bmagyar commented Jul 19, 2017

Uh oh!

MoriKen254 commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Main difference between controller of #280 & one of #289

In detail

What would we do?

Uh oh!

vincentrou commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoriKen254 commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentrou commented Aug 3, 2017

Uh oh!

MoriKen254 commented Sep 4, 2017

Uh oh!

bmagyar commented Sep 4, 2017

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!

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!

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!

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!

bmagyar commented Sep 26, 2017

Uh oh!

bmagyar commented Jan 11, 2018

Uh oh!

MoriKen254 commented Jan 25, 2018

Uh oh!

MoriKen254 commented Aug 16, 2018

Uh oh!

MoriKen254 commented Aug 17, 2018

Uh oh!

bmagyar commented Aug 20, 2018

Uh oh!

MoriKen254 commented Jul 18, 2017 •

edited

Loading

MoriKen254 commented Jul 19, 2017 •

edited

Loading

vincentrou commented Jul 21, 2017 •

edited

Loading

MoriKen254 commented Jul 22, 2017 •

edited

Loading