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

diff_drive_controller doesn't use joint.velocity #159

Open
paulbovbel opened this issue Jan 23, 2015 · 14 comments
Open

diff_drive_controller doesn't use joint.velocity #159

paulbovbel opened this issue Jan 23, 2015 · 14 comments

Comments

@paulbovbel
Copy link

In https://github.com/ros-controls/ros_controllers/blob/indigo-devel/diff_drive_controller/src/odometry.cpp#L78, diff_drive_controller currently differentiates joint.position to obtain velocity, differentiates again to obtain acceleration, and then obtains the final odometry twist component by integrating acceleration.

This results in filtered twist component on the outgoing odometry with a very slow response. Since it's possible to update joint.velocity directly from hardware, would it be possible to generate the twist message directly from this information, improving the response? Could either parametrize or detect from incoming data whether the velocity information is filled out.

@bmagyar
Copy link
Member

bmagyar commented Jan 23, 2015

Parametrizing on the way of retrieving velocity feedback is sensible. I would prefer it explicit rather than implicitly setting it based on detection.

Currently I'm with other projects and also don't really have hardware that gives direct velocity feedback but I'm sure that many people would welcome and appreciate this feature in a pull request.

I imagine one param to set the method of acquiring velocity feedback.
Based on this the update function could also take the velocity from the joints and forward it to a slightly refactored Odometry object that has an update function with separate arguments for position and velocity. This would basically disable the first integration you mentioned above. Would this be sufficient for your use case? I'm asking this because joint handles currently don't support acceleration.

@paulbovbel
Copy link
Author

Thanks for the quick feedback, sounds good, I'll do a pull when ready

@bmagyar
Copy link
Member

bmagyar commented Jan 23, 2015

Cool! Please take some time to add some tests as well.
We put great emphasis on having this controller well tested since it drives all our wheel based robots. (Coverage is always >90%)

@efernandez
Copy link
Contributor

@paulbovbel As Bence is saying, if you have velocity feedback directly from your hw, it makes sense to avoid differentiating to compute it. Simply at a param for those who don't have velocity feedback.

@paulbovbel
Copy link
Author

It's a little trickier than that, since currently this chain happens:

joint.position --diff--> velocity_est --int--> odometry.pose

velocity_est --diff--> accel --int--> odometry.twist

We can't just stick joint.velocity into velocity_est, since while the hw measurement will provide a more accurate velocity profile, it's not an authority on the actual robot travel.

Anyways, I'll think about how to do the change in a way that makes sense and doesn't break any existing configurations.


On the side, I think it would actually makes sense for the diff_drive_controller to use the data as-is from the JointInterface, and the responsibility of the RobotHW implementation to 'fake' velocity data by differentiating, if it cannot be measured directly. That way on the diff_drive_controller side, the joints.position can go directly into odometry.pose, while joints.velocity can go into odometry.twist.

@bmagyar
Copy link
Member

bmagyar commented Jan 23, 2015

We can't just stick joint.velocity into velocity_est, since while the hw measurement will provide a more accurate velocity profile, it's not an authority on the actual robot travel.
I'm not sure I got your point here. Why not? The more accurate the measurement, the better for us in the end. The velocity estimates inside the Odometry class are per wheel, so it wouldn't change anything there. The fact that this odometry computed might not relate to the actual robot travel that well (due to driving in mud, slippery floor, etc) is a different story and is not related to where the velocity estimation is coming.

There's a little bit more to it than putting the joints.velocity value in odometry.twist but yes, it could be used. (You have to drop it in the bycicle-model and then put the result in the twist :))


I can see the point with the idea of speed estimation for velocity joints. This could be implemented in the velocity handles for each joint. The only downside of it is that it may break ABI compatibility with some existing code, so it could only appear in Jade or ROS K. I think you should discover the opportunities of this on an issue opened against the ros_control repo where those handles are implemented.

@paulbovbel
Copy link
Author

Okay, I'll implement my thoughts and do a pull, we can chat more then.


What I meant was actually that it's the client RobotHW implementations that might have the responsibility to fill out joint.position and joint.velocity, and perform differentiation if the hardware doesn't report velocity. That way the diff_drive_controller would assume that those fields in the joint interface are properly filled out by the client implementation, and wouldn't need to be parametrized.

So this wouldn't go into ros_control (rather in your mobile base driver), but would still require refactoring for any diff_drive_controller users.

@bmagyar
Copy link
Member

bmagyar commented Jan 23, 2015

👍

@tappan-at-git
Copy link
Contributor

Hi, I was wondering what the state of this issue was. A robot I am working with would benefit drastically from using the provided joint velocity rather than making Diff Drive Controller recompute it.

@bmagyar
Copy link
Member

bmagyar commented Dec 18, 2015

The robots that we had access to didn't have this capability at the time. I
think there isn't anything blocking the implementation of such a feature.
The only thing we'd need to make sure is that the current behaviour is kept
default.
On 17 Dec 2015 21:14, "Eric Tappan" [email protected] wrote:

Hi, I was wondering what the state of this issue was. A robot I am working
with would benefit drastically from using the provided joint velocity
rather than making Diff Drive Controller recompute it.


Reply to this email directly or view it on GitHub
#159 (comment)
.

@tappan-at-git
Copy link
Contributor

By crazy random happenstance, I happen to have a pull request with that behavior.

@efernandez
Copy link
Contributor

Sorry for this late reply, but I've been on a long flight.

It seems that clearpathrobotics#10 also addresses this.

I'd try to push the clearpathrobotics fork changes upstream before EOY.

@wxmerkt
Copy link
Contributor

wxmerkt commented Jun 6, 2017

@efernandez We'd like to have the solution for basing odometry directly off available joint velocities upstreamed and integrated.

cc @bmagyar

@fayyazpocker
Copy link

Seems like this issue was addressed in 22e9c2c#. But I couldn't find any params like position_feedback to set the diff_drive_controller to rely on the velocity from RobotHW rather than getting it by differentiating position.

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

6 participants