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

Ackermann Steering Plugin: separate linear and angular #2018

Closed

Conversation

Bac0nEater
Copy link

@Bac0nEater Bac0nEater commented Jun 16, 2023

🎉 New feature

Summary

This separate fields between linear and angular in Ackermann Steering Plugin, such as linear, acceleration and jerk.

Linear velocity, acceleration and jerk fields stay the same. However, we moved the angular velocity, acceleration and jerk fields, so we can specify the velocity, acceleration and jerk differently for linear and angular.

Explanation:
Screenshot from 2023-06-15 14-57-15

We also added P control to normal mode (not SteeringOnly mode).

The plugin and integration tests are written based on the original velocity and acceleration of Ackermann Steering.

Test it

The plugin can be tested using examples/worlds/ackermann_steering_with_angular.sdf

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jun 16, 2023
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

This is a change in the behaviour of the plugin, I think we should retarget this to main. What do you think @mjcarroll ?

test/integration/ackermann_steering_system.cc Outdated Show resolved Hide resolved
@Bac0nEater Bac0nEater force-pushed the separate-angular-and-displacement branch from f7d3254 to f8443f8 Compare June 19, 2023 00:29
ThreonX and others added 7 commits June 19, 2023 12:31
…in normal mode

Added separate parameters for angular speed, min_angular_velocity, max_angular_velocity, min_angular_acceleration, etc.

Added the gainPAng to normal mode (not steering-only mode)

Signed-off-by: ThreonX <76905674+ThreonX@users.noreply.github.com>
Signed-off-by: Bac0nEater <thenickys123@gmail.com>
update pre-commit-hooks

Signed-off-by: mosfet80 <realeandrea@yahoo.it>
Signed-off-by: Bac0nEater <thenickys123@gmail.com>
As of gazebosim/sdformat#1117, SDFormat supports joints under world (//world/joint in xpath). This PR adds support for it in gz-sim.

Note that some systems that operate on joints, such as the JointPositionController, require that the system is instantiated inside a model, and therefore, will not work with world joints. It's not clear whether this is a real problem since world joints are most likely to be fixed joints used to attach two models together.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Bac0nEater <thenickys123@gmail.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Bac0nEater <thenickys123@gmail.com>
Signed-off-by: Bac0nEater <thenickys123@gmail.com>
Signed-off-by: Bac0nEater <thenickys123@gmail.com>
Signed-off-by: Bac0nEater <thenickys123@gmail.com>
@Bac0nEater Bac0nEater force-pushed the separate-angular-and-displacement branch from f8443f8 to 90a21e6 Compare June 19, 2023 00:32
@Bac0nEater Bac0nEater requested a review from azeey as a code owner June 19, 2023 00:32
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@azeey azeey requested a review from ahcorde August 21, 2023 18:41
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

As I already mentioned in my previous comment, this might break other users.

a current user of this plugin needs to include these new tags otherwise the min angular vel, max angular vel, min angular acceleration and max angular acceleration will not be defined.

there are two ways to include this:

  • target this to main
  • Add a legacy tag, to be able to choose the current behaviour or use this new one. The default behaviour should be the current behaviour

.pre-commit-config.yaml Outdated Show resolved Hide resolved
src/LevelManager.cc Outdated Show resolved Hide resolved
@Bac0nEater Bac0nEater changed the base branch from gz-sim7 to main August 21, 2023 23:58
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
@Bac0nEater
Copy link
Author

As I already mentioned in my previous comment, this might break other users.

a current user of this plugin needs to include these new tags otherwise the min angular vel, max angular vel, min angular acceleration and max angular acceleration will not be defined.

there are two ways to include this:

  • target this to main
  • Add a legacy tag, to be able to choose the current behaviour or use this new one. The default behaviour should be the current behaviour

I'm sorry. I'm not sure what is the legacy tag. Is it the tags and releases in Github?

@ahcorde
Copy link
Contributor

ahcorde commented Aug 22, 2023

As I already mentioned in my previous comment, this might break other users.
a current user of this plugin needs to include these new tags otherwise the min angular vel, max angular vel, min angular acceleration and max angular acceleration will not be defined.
there are two ways to include this:

  • target this to main
  • Add a legacy tag, to be able to choose the current behaviour or use this new one. The default behaviour should be the current behaviour

I'm sorry. I'm not sure what is the legacy tag. Is it the tags and releases in Github?

what I was trying to say, it;s that you can add a new tag inside the plugin, for example <angular_limits>:

        <angular_limits>true</angular_limits>
        <min_angular_velocity>-10</min_angular_velocity>
        <max_angular_velocity>10</max_angular_velocity>
        <min_angular_acceleration>-20</min_angular_acceleration>
        <max_angular_acceleration>20</max_angular_acceleration>
      </plugin>

if this is true then use the specific angular limits otherwise use the other limits to avoid break other users.

Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
Signed-off-by: Nick <69305722+Bac0nEater@users.noreply.github.com>
@azeey azeey removed their request for review August 22, 2023 17:46
@azeey
Copy link
Contributor

azeey commented Aug 29, 2023

Removing beta since there hasn't been any activity in a while. We can merge after Harmonic is released.

@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 29, 2023
@bperseghetti
Copy link
Member

Thanks for the contribution. Looking over this I'm not sure if this makes sense to me for an Ackermann based vehicle.

The problem resides in the fact that angular velocity is only a function of the heading/steering angle and the forward drive velocity of the wheels. This would rely upon someone doing the math to know which one is "limiting the system" and does not seem to add any functionality I could think of offhand for a real Ackermann based vehicle, instead this could be potentially much more risky to users.

Also not sure the value in setting min/max angular rates would do for Ackermann as well since it's not a diff drive.

What problem were you hoping to solve or added functionality that is missing were you hoping for this to have?

@retinfai
Copy link
Contributor

Can we rescope this PR to this for UpdateVelocity:
image

This already exists for UpdateAngle. aka. steering only.

In simulation, our robot is turning quite slowly, so we copied over the gain to help.

@bperseghetti
Copy link
Member

bperseghetti commented Mar 27, 2024

Can we rescope this PR to this for UpdateVelocity: image

This already exists for UpdateAngle. aka. steering only.

In simulation, our robot is turning quite slowly, so we copied over the gain to help.

Sure, that seems reasonable, I would just maybe edit title of PR to match and cleanup the feature summary, or you can just open a new PR if you want and close this one referencing that new one. Your choice, but fastest route might be just the new PR route anyhow to avoid needing to handle the stale/blocking reviews on this anyhow.

@retinfai
Copy link
Contributor

Sweet as, I can't close this from my side, as these guys have left the lab. But safe to say no more work on this thread will be done; so if you can close on your side, perfect.

I'll open a PR

@ahcorde ahcorde closed this Mar 27, 2024
@bperseghetti
Copy link
Member

@retinfai, please make sure to rebase your branch on the latest merge for gz-sim8 (which includes the changes to the ackermann plugin) before creating that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants