ros-controls / ros_controllers

Generic robotic controllers to accompany ros_control
http://wiki.ros.org/ros_control
BSD 3-Clause "New" or "Revised" License
571 stars 525 forks source link

Making room for mecanum_drive_controller #147

Open arennuit opened 10 years ago

arennuit commented 10 years ago

Hi guys,

I am currently developping a mecanum_drive_controller which (as its name states) will take a twist as input and generate desired wheel velocities. From what I have seen this class will be rather close to diff_drive_controller. So I was thinking of factoring the common code out into a drive_controller class (from which diff_drive_controller and mecanum_drive_controller would derive). It does not look like a big issue, but I was wondering how you would welcome this proposition... If you consider its worth the effort, then I'll make a proposition in a fork of mine, otherwise no worries I'll do a completely separate package in a repo of mine.

Just let me know what you think.

Thanks,

Antoine.

adolfo-rt commented 10 years ago

How much of diff_drive_controller do you think could be factored-out and reused?.

arennuit commented 10 years ago

On a very rough estimate I would say 50%... and spread like this:

bmagyar commented 10 years ago

I would advise against further generalization of the existing diff_drive_controller code since it would introduce more complexity to something that is otherwise extremely simple.

Luckily though in C++ it is possible to inherit code from a parent class. I'd suggest an approach where you introduce a dependency to diff_drive_controller, inherit code from there and override the necessary parts in your controller.

Feel free to add some virtuals to whereever it's necessary in the diff_drive_controller code.

adolfo-rt commented 10 years ago

...or see if some controller components can be factored out in separate classes / functions, so compose instead of extend.

efernandez commented 10 years ago

@arennuit What do you mean a mecanum driver controller? Do you mean an omnidirectional base using mecanum wheels, so it can move sideways?

The diff_drive_controller simply computes the odometry and transforms robot-space velocities into wheel-space velocities for differential and skid steer platforms. IMO an omnidirectional (or even an Ackermann) platform could have a similar structure to the diff_drive_controller, but at the end the equations differ, so there's no so much you can use from the diff_drive_controller (apart from auto-imposing you a set of methods, classes and attributes). I think it's better that you implement it separately and then we can see whether it makes sense or not to factor something or create a class hierarchy.

arennuit commented 10 years ago

@bmagyar, @adolfo-rt : no worries I do not plan to extend the diff_drive_controller but only to factor the common code out (as Adolfo is mentioning)

@efernandez: yes I mean a controller which computes mecanum wheels velocities given a desired mobile base twist and also computes the odometry


I am currently putting together some code completely separate from diff_drive_controller (but unshamingly based on it ;)). Then we can review it and decide where to go.

arennuit commented 10 years ago

I have a basic implementation working but currently it is assuming the input twist (topic cmd_vel) is a body velocity and the computed odometry velocity is also "body". Can you confirm? I have not been able to find the answer by simply reading documents about the navigation stack... This relates to this question : http://answers.ros.org/question/196233/geometry_msgstwist-references-navigation-stack-ros_control/

arennuit commented 10 years ago

I have created a related PR: #149

bmagyar commented 10 years ago

In the end you have to make an assumption on the frame of the input twist, and yes, you can interpret this as "body". The computed odometry I believe is always "body" by convention.

Be prepared though, you will have a hard time to convince me about the necessity of factoring out parts of the diff_drive_controller.

efernandez commented 10 years ago

@arennuit @bmagyar Regarding the frames:

arennuit commented 10 years ago

@bmagyar I still need to review your point on factoring code out, but I must admit the 2 controllers share less code than I initially thought

arennuit commented 10 years ago

@efernandez thanks for your input ;)

arennuit commented 10 years ago

I believe this code is starting to be in a usable state. Still needs further testing, unit tests (none written so far) and code review.