Skip to content

Separate acceleration and deceleration limits. #17

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

Merged
merged 3 commits into from
Jan 29, 2016

Conversation

p6chen
Copy link

@p6chen p6chen commented Jan 28, 2016

The current diff_drive_controller suffers a big issue of min_acceleration being both the reverse acceleration and forward deceleration value, and max_acceleration being both the forward acceleration and reverse deceleration value.

This coupling makes it impossible to tune the robots to accelerate slowly, because doing so would cause it to also decelerate slowly in the opposite direction, causing collisions etc.

I've made the changes to backwards compatible as much as I can, which is why the variable names (at least IMO) are a bit confusing.

@efernandez @servos @mikepurvis @paulbovbel

… tied to accelerating in the opposite direction.
@efernandez
Copy link
Member

Initially I thought that with min_acceleration there was enough flexibility, but from your comment it looks like the *_deceleration params are also needed. Give me some time to look into it... for now I'm going to comment a few minor things.

@@ -58,7 +58,9 @@ namespace diff_drive_controller
double min_acceleration,
double max_acceleration,
double min_jerk,
double max_jerk
double max_jerk,
double min_deceleration,
Copy link
Member

Choose a reason for hiding this comment

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

move them up, just below the _acceleration ones

Copy link
Author

Choose a reason for hiding this comment

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

I had them at the end intentionally to keep backwards compatibility of any code that may have invoked the constructor with the old arguments. Having them at the end allows the new params to be default initialized.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then declared them after max_jerk to address my other/similar comment below

dv = min_deceleration * dt;
if (v0 + dv < 0)
{
dv = min_acceleration * (dt - v0 / min_deceleration);
Copy link

Choose a reason for hiding this comment

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

I think you need to check if v0 + dv is < v here somewhere.

Copy link
Author

Choose a reason for hiding this comment

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

Good point..

Copy link

Choose a reason for hiding this comment

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

Or rather clamp dv between this math and v-v0

Copy link
Author

Choose a reason for hiding this comment

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

I readjusted it so now I calculate dv_min and dv_max and clamp in between that like how the general case does it.

Has an added benefit of if the decelerations were poorly defined (i.e. a positive min deceleration), we will simply stay at the last velocity, instead of decelerating in the wrong direction and increasing the velocity error.

@servos
Copy link

servos commented Jan 29, 2016

LGTM

@efernandez
Copy link
Member

I don't have time to look into this now... I'm afraid there's some issue with the new acceleration limiting logic, but I thinks it's fine to merge it for now.

I've verified all the tests still pass.

👍

p6chen added a commit that referenced this pull request Jan 29, 2016
Separate acceleration and deceleration limits.
@p6chen p6chen merged commit 52ddce0 into indigo-devel Jan 29, 2016
@efernandez efernandez deleted the deceleration_limits branch February 1, 2016 15:29
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.

3 participants