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

Making room for mecanum_drive_controller #147

Open
arennuit opened this issue Oct 27, 2014 · 13 comments
Open

Making room for mecanum_drive_controller #147

arennuit opened this issue Oct 27, 2014 · 13 comments

Comments

@arennuit
Copy link

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
Copy link
Member

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

@arennuit
Copy link
Author

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

  • diff_drive_controller: 50%
  • odometry: 30%
  • speed_limiter: 100%

@bmagyar
Copy link
Member

bmagyar commented Oct 29, 2014

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
Copy link
Member

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

@efernandez
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Author

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 arennuit mentioned this issue Oct 29, 2014
@arennuit
Copy link
Author

I have created a related PR: #149

@bmagyar
Copy link
Member

bmagyar commented Oct 30, 2014

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
Copy link
Contributor

@arennuit @bmagyar Regarding the frames:

  • Velocity commands : BODY
  • Odometry : navigation frame, which it's specified in the frame_id and it's odom by default

@arennuit
Copy link
Author

@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
Copy link
Author

@efernandez thanks for your input ;)

@arennuit
Copy link
Author

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.

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

No branches or pull requests

4 participants