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

sauk2 · 2024-02-15T07:09:59Z

🦟 Bug fix

Summary

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

Expected behavior: The inner wheel should turn more than the outer wheel during turning
Actual behavior: The opposite takes place. (Here I have commented out the chassis visual in the SDF for better visibility)

Steps to reproduce

Add the following tag in the example SDF file of Ackermann Steering plugin.

  <steering_only>True</steering_only>

Build and source the workspace
Launch using the following command

  gz sim ackermann_steering.sdf

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

Checklist

Signed all commits for DCO
Added tests
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.

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

🎉 New feature

Summary

Added a programmable PID (gz::math::PID) to control the steering joints for both modes - velocity and steering only.
Default gains and other parameters are defined in Configure which can be overwritten from the SDF using appropriate tags.
A single PID is used to control both, left and right steering joints.

Test it

For steering only mode

Add the following tag in the example SDF file of Ackermann Steering plugin.

  <steering_only>True</steering_only>

Build and source the workspace
Launch using the following command

  gz sim ackermann_steering.sdf

Publish the following

  gz topic -t "/model/vehicle_blue/steer_angle" -m gz.msgs.Double -p "data: 0.4"

For velocity mode

Build and source the workspace
Launch using the following command

  gz sim ackermann_steering.sdf

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

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

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.

Signed-off-by: Saurabh Kamat <saurabh.kamat@accelerationrobotics.com>

retinfai · 2024-03-12T00:10:20Z

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

Good catch, it looks like there is a trailing whitespace that needs fixed to pass CI.

src/systems/ackermann_steering/AckermannSteering.cc

bperseghetti · 2024-03-12T01:42:59Z

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).

Signed-off-by: Saurabh Kamat <saurabh.kamat@accelerationrobotics.com>

sauk2 · 2024-03-12T06:29:16Z

@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 · 2024-03-12T16:56:42Z

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 ⚠️

Additional details and impacted files

@@           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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bperseghetti · 2024-03-13T18:14:04Z

@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"?

Adds test for steering_only PID and conditional for the SDF PID elements. Signed-off-by: Benjamin Perseghetti <bperseghetti@rudislabs.com>

bperseghetti · 2024-03-13T19:06:11Z

@sauk2 if you merge this it should fix the code coverage issue and cleanly separate the PID steering_only SDF elements:
sauk2#1

Ackermann: steering_only add tests and conditional.

sauk2 · 2024-03-14T04:57:47Z

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!

Signed-off-by: Benjamin Perseghetti <bperseghetti@rudislabs.com>

azeey · 2024-03-15T21:32:18Z

src/systems/ackermann_steering/AckermannSteering.cc

+  // Simple PID control with settable gains.
+  this->leftSteeringJointSpeed = this->steerPosPid.Update(
+    leftDelta, _info.dt);
+  this->steerPosPid.Reset();


Is there a reason for resetting steerPosPid here? If I'm not mistaken, if we reset it, the D and I terms of the PID will not do anything.

I was resetting the steerPosPid because I was using the same PID for both steering joints and it was causing random motions. I didn't realise it would render the I and D terms useless.

After giving it more thought, I think this will be solved if I create 2 PID controllers in Configure - one for each steering joint.

Would like to hear your thoughts on this.

@sauk2 So, I have two thoughts, first is typically Ackermann steering is done through a physical linkage with a single steering actuator. In this case the "PID" would only be the central steering angle and that would cause some problems for the method attempting to be implemented here, I wouldn't really like to have one of my wheels with a different PID tuning or response based on angle than the other. So if it were to be done I think you would have to take the average of both wheel angles back to the central steering angle and run the PID on the central steering angle till they aligned properly (this could also have other downsides like lower convergence or a skew between the two wheels that solves the recalculated central steering angle but not having the wheels in the desired spot as they offset eachother).

The second thought I have is that while a PID might make sense for a "cmd_vel" based "miniature controller", when using the steering_only I think the only thing that should be used is P gain as it can always be wrapped with some sort of input shaping, and a lot of "raw" actuators are just proportional anyhow.

I think we should get the fix in for the steering angle, but I don't think we should necessarily couple that with the PID changes. I think these should be separate PRs.

Makes sense. So all the changes will have to be rolled back. The easiest way I can see is to reset this branch to commit id 633ce72 and then resolve the bug.

Should I go ahead with this or is there any alternate method that you have thought of?

I guess I would just pull latest commit on branch and then make the fix ontop of it: 6cfcd07

And then push your current branch to another branch like ackermann-pid so you could reference it later, and push the changes ontop the other temp working branch based on 6cfcd07 to this current branch so you don't lose history in this PR.

bperseghetti · 2024-03-21T17:01:18Z

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 · 2024-03-23T07:44:03Z

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

sauk2 added 3 commits February 15, 2024 14:07

Added programmable PID for steer joint

de5dcdb

Signed-off-by: Saurabh Kamat <saurabh.kamat@accelerationrobotics.com>

Updated usage documentation

839a10a

Signed-off-by: Saurabh Kamat <saurabh.kamat@accelerationrobotics.com>

Added PID control to steer joint in velocity mode

68a81fa

Signed-off-by: Saurabh Kamat <saurabh.kamat@accelerationrobotics.com>

sauk2 requested a review from mjcarroll as a code owner February 15, 2024 07:10

github-actions bot added the 🎵 harmonic Gazebo Harmonic label Feb 15, 2024

sauk2 changed the title ~~Ackermann steering plugin's steering only mode shows opposite behaviour~~ Ackermann steering plugin's steering only mode shows opposite behaviour. Additionally added a gz PID for steering Feb 15, 2024

azeey requested a review from bperseghetti February 28, 2024 16:42

bperseghetti reviewed Mar 12, 2024

View reviewed changes

src/systems/ackermann_steering/AckermannSteering.cc Outdated Show resolved Hide resolved

Removed trailing whitespace

007ef7a

Signed-off-by: Saurabh Kamat <saurabh.kamat@accelerationrobotics.com>

sauk2 requested a review from bperseghetti March 12, 2024 06:31

Ackermann: steering_only add tests and conditional.

28c1ffd

Adds test for steering_only PID and conditional for the SDF PID elements. Signed-off-by: Benjamin Perseghetti <bperseghetti@rudislabs.com>

Merge pull request #1 from rudislabs/feat/ackermann-pid

57f694a

Ackermann: steering_only add tests and conditional.

Newline fix for Ackermann sdf.

929a15b

Signed-off-by: Benjamin Perseghetti <bperseghetti@rudislabs.com>

bperseghetti approved these changes Mar 15, 2024

View reviewed changes

azeey reviewed Mar 15, 2024

View reviewed changes

retinfai mentioned this pull request Mar 20, 2024

Add support for AckermannDrive message gazebosim/gz-msgs#435

Closed

24 tasks

bperseghetti mentioned this pull request Mar 26, 2024

Fixed turning error in ackermann steering #2342

Merged

4 tasks

bperseghetti closed this Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sauk2 commented Feb 15, 2024

retinfai commented Mar 12, 2024

bperseghetti left a comment

bperseghetti commented Mar 12, 2024

sauk2 commented Mar 12, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

bperseghetti commented Mar 13, 2024 •

edited

Loading

bperseghetti commented Mar 13, 2024

sauk2 commented Mar 14, 2024

azeey Mar 15, 2024

sauk2 Mar 17, 2024

bperseghetti Mar 18, 2024 •

edited

Loading

bperseghetti Mar 18, 2024

sauk2 Mar 18, 2024

bperseghetti Mar 18, 2024

bperseghetti Mar 18, 2024 •

edited

Loading

bperseghetti commented Mar 21, 2024

sauk2 commented Mar 23, 2024

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

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

Conversation

sauk2 commented Feb 15, 2024

🦟 Bug fix

Summary

Steps to reproduce

Solution

Checklist

🎉 New feature

Summary

Test it

For steering only mode

For velocity mode

Checklist

retinfai commented Mar 12, 2024

bperseghetti left a comment

Choose a reason for hiding this comment

bperseghetti commented Mar 12, 2024

sauk2 commented Mar 12, 2024

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

bperseghetti commented Mar 13, 2024 • edited Loading

bperseghetti commented Mar 13, 2024

sauk2 commented Mar 14, 2024

azeey Mar 15, 2024

Choose a reason for hiding this comment

sauk2 Mar 17, 2024

Choose a reason for hiding this comment

bperseghetti Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

bperseghetti Mar 18, 2024

Choose a reason for hiding this comment

sauk2 Mar 18, 2024

Choose a reason for hiding this comment

bperseghetti Mar 18, 2024

Choose a reason for hiding this comment

bperseghetti Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

bperseghetti commented Mar 21, 2024

sauk2 commented Mar 23, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

bperseghetti commented Mar 13, 2024 •

edited

Loading

bperseghetti Mar 18, 2024 •

edited

Loading

bperseghetti Mar 18, 2024 •

edited

Loading