search

gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
621 stars 251 forks source link

Ackermann steering plugin's steering only mode shows opposite behaviour. Additionally added a gz PID for steering #2315

Closed sauk2 closed 3 months ago

sauk2 commented 4 months ago

🦟 Bug fix

Fixes #2314

Summary

Issue pertains to Ackermann Steering plugin's steering_only mode specifically these lines.

Screenshot from 2024-02-15 14-27-11

Steps to reproduce

  1. Add the following tag in the example SDF file of Ackermann Steering plugin. 
    <steering_only>True</steering_only>
  2. Build and source the workspace
  3. Launch using the following command 
    gz sim ackermann_steering.sdf
  4. Publish the following 
    gz topic -t "/model/vehicle_blue/steer_angle" -m gz.msgs.Double -p "data: 0.4"

Additionally comment out chassis visual for better visibility

Solution

Changing signs in these equations should suffice.

Check output after implementation Screenshot from 2024-02-15 14-51-32

Checklist

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Summary

Test it

For steering only mode

  1. Add the following tag in the example SDF file of Ackermann Steering plugin. 
    <steering_only>True</steering_only>
  2. Build and source the workspace
  3. Launch using the following command 
    gz sim ackermann_steering.sdf
  4. Publish the following 
    gz topic -t "/model/vehicle_blue/steer_angle" -m gz.msgs.Double -p "data: 0.4"

    For velocity mode

  5. Build and source the workspace
  6. Launch using the following command 
    gz sim ackermann_steering.sdf
  7. Publish the following 
    gz topic -t "/model/vehicle_blue/cmd_vel" -m gz.msgs.Twist -p "linear: {x: 1.0}, angular: {z: 0.5}"

    Checklist

    • [x] Signed all commits for DCO
    • [ ] Added tests
    • [ ] Added example and/or tutorial
    • [x] 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

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

retinfai commented 3 months ago

A little tangent but somewhat related. Is it possible with the current plugin to be able to drive the vehicle using steering angle and forward velocity?

If not, is it something that can be implemented within the scope of this plugin?

bperseghetti commented 3 months ago

A little tangent but somewhat related. Is it possible with the current plugin to be able to drive the vehicle using steering angle and forward velocity?

If not, is it something that can be implemented within the scope of this plugin?

Hmmmm, I think that's more the role of JointController, Here's how I use them in a model together: https://github.com/CogniPilot/b3rb_simulator/blob/31554eb3126d6ee306a295b2f9f5e8524907b940/b3rb_gz_resource/models/b3rb/model.sdf#L540-L562

Note that JointController uses the velocity rad/s field when using actuator_msg and steering_only in Ackermann with actuator_msg uses the position (rad).

sauk2 commented 3 months ago

@bperseghetti, on a completely unrelated note. Would it be a good idea to calculate wheel separation in case it is not provided by the user?

We can use the world pose of the left and right steering joints and calculate that value. This calculation will not be performed if there is a value given in the SDF.

Would like to hear your thoughts on this

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 77.27273% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 65.98%. Comparing base (633ce72) to head (929a15b). Report is 5 commits behind head on gz-sim8.

Files Patch % Lines
...rc/systems/ackermann_steering/AckermannSteering.cc 77.27% 10 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## gz-sim8 #2315 +/- ## ======================================== Coverage 65.97% 65.98% ======================================== Files 327 327 Lines 31328 31379 +51 ======================================== + Hits 20668 20704 +36 - Misses 10660 10675 +15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bperseghetti commented 3 months ago

@bperseghetti, on a completely unrelated note. Would it be a good idea to calculate wheel separation in case it is not provided by the user?

We can use the world pose of the left and right steering joints and calculate that value. This calculation will not be performed if there is a value given in the SDF.

Would like to hear your thoughts on this

Hmmmm, I think that might be risky depending on the intended use case for it. IE using 4 wheel steering (like you see on a good amount of newer electric trucks). I think maybe you could calculate it though and then see if it does not match the provided value within a "reasonable error percentage" (IE 15%) that it publishes a gz-warn to let you know what value it thinks it should be.

But depending on things like suspension etc the steering joint might actually be a relative reference to another joint and then that would be somewhat messy to follow that potential dependency tree.

IE having joints for a suspension such as macpherson, wishbone, etc would make the steering joint a relative joint to those other suspension joints and that just starts to possibly become more risk than reward.

Therefore, my personal take on it might lean toward it "probably not being a good choice"?

bperseghetti commented 3 months ago

@sauk2 if you merge this it should fix the code coverage issue and cleanly separate the PID steering_only SDF elements: https://github.com/sauk2/gz-sim/pull/1

sauk2 commented 3 months ago

Hmmmm, I think that might be risky depending on the intended use case for it. IE using 4 wheel steering (like you see on a good amount of newer electric trucks). I think maybe you could calculate it though and then see if it does not match the provided value within a "reasonable error percentage" (IE 15%) that it publishes a gz-warn to let you know what value it thinks it should be.

But depending on things like suspension etc the steering joint might actually be a relative reference to another joint and then that would be somewhat messy to follow that potential dependency tree.

IE having joints for a suspension such as macpherson, wishbone, etc would make the steering joint a relative joint to those other suspension joints and that just starts to possibly become more risk than reward.

Therefore, my personal take on it might lean toward it "probably not being a good choice"?

Yes, that makes sense. Thanks!

bperseghetti commented 3 months ago

A little tangent but somewhat related. Is it possible with the current plugin to be able to drive the vehicle using steering angle and forward velocity?

If not, is it something that can be implemented within the scope of this plugin?

Yes, for that you just set wheel velocity with rad/s using the JointController plugin and also the steering angle all using the actuator msg, example is here:

https://github.com/CogniPilot/b3rb_simulator/blob/7830685f9b9c3aca2105ee9e149969d2db696350/b3rb_gz_resource/models/b3rb/model.sdf#L1090-L1112

sauk2 commented 3 months ago

@bperseghetti Created a seperate PR #2342 for the steering error fix! Request you to take a look.