Skip to content

Remove front_steering from steering library #1166

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

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented Jun 8, 2024

To Accommodate controllers that are not only steering at front or rear
this change remove the front_steering variable from
steering_controller_library, as a byproduct of that the notion of
front or rear wheel radius is also removed from dependant controllers
and the library has know "traction_joints_names" and
"steering_joints_names" instead of "front_wheels_names" and
"rear_wheels_names".

Depends on:

@qinqon qinqon force-pushed the remove_front_steering_param_from_steering_library branch 4 times, most recently from c83dce3 to 71800b2 Compare June 8, 2024 08:32
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

I support the idea, thanks!

Please deprecate the old parameters and add the new ones to support both temporarily. Then we can also backport such changes.

Additionally, please add migration notes here.

@christophfroehlich
Copy link
Contributor

christophfroehlich commented Jun 8, 2024

Have you read and considered #692? Ideally, we should solve it with this PR.
I'm not sure if your proposal works for 4WD + 4x steering?

@qinqon qinqon changed the title [WIP] Remove front_steering from steering library Remove front_steering from steering library Jun 8, 2024
@qinqon
Copy link
Contributor Author

qinqon commented Jun 8, 2024

Have you read and considered #692? Ideally, we should solve it with this PR.

The idea is the same, is all about generalize the steering library a little more.

I'm not sure if your proposal works for 4WD + 4x steering?

Next PR will allow that, I am thinking about extending the current ackermann instead of new controller, if you add a pair of traction and steering joints and define the instantaneus center of robot you can have this by modifyting a little the IK and odometry. Also you can keep the current ackermann by configuring the instantaneus centor or robot at the traction joints and using the same IK and odometry.

@qinqon qinqon force-pushed the remove_front_steering_param_from_steering_library branch 12 times, most recently from 3965416 to c09d17b Compare June 12, 2024 20:58
@qinqon qinqon force-pushed the remove_front_steering_param_from_steering_library branch from c09d17b to 41b3219 Compare June 19, 2024 17:22
Copy link
Contributor

mergify bot commented Jun 19, 2024

This pull request is in conflict. Could you fix it @qinqon?

@qinqon
Copy link
Contributor Author

qinqon commented Jun 23, 2024

Tests working

$ colcon test 
Starting >>> steering_controllers_library
Finished <<< steering_controllers_library [0.44s]          
Starting >>> ackermann_steering_controller
Starting >>> bicycle_steering_controller
Starting >>> tricycle_steering_controller                                                                    
Finished <<< bicycle_steering_controller [1.50s]                                                                                                   
Finished <<< ackermann_steering_controller [1.58s]                                                            
Finished <<< tricycle_steering_controller [1.52s]

Summary: 4 packages finished [3.01s]


std::vector<std::string> rear_wheels_preceeding_names_ = {
std::vector<std::string> wheels_preceeding_names_ = {
Copy link
Contributor

@Juliaj Juliaj Apr 23, 2025

Choose a reason for hiding this comment

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

Typo, wheels_preceeding -> wheels_preceding. There are more instances of this typo in the current file ...

Shall we just use joints_preceding_names_ for consistency ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, fixed that in a separate PR and will merge that in here later. But we have wheels_preceeding_names_ and steers_preceeding_names_, joints do not work then?

Copy link
Contributor

Choose a reason for hiding this comment

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

One option is to rename wheels_preceding_names_ to traction_joints_preceding_names_, and steers_preceding_names_ to steering_joints_preceding_names_. There is a variable named preceding_joint_names_ which doesn't seem to be used anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

done. please have a look

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

I just had a hard time updating ros-controls/gz_ros2_control#566, because I misspelled a parameter.
traction_wheels_track instead of traction_wheel_track (due to the similarity to traction_wheels_radius). I propose renaming it to track_width instead.

Copy link
Contributor

mergify bot commented Apr 23, 2025

This pull request is in conflict. Could you fix it @qinqon?

Copy link
Contributor

@Juliaj Juliaj left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for all the work and efforts to get this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants