ros2_controllers icon indicating copy to clipboard operation
ros2_controllers copied to clipboard

Refactor to distinguish between wheel and steering joints (steering controller)

Open roncapat opened this issue 2 years ago • 14 comments

Pre-requirement of #648

If you have a look for example at: https://github.com/ros-controls/ros2_controllers/blob/master/ackermann_steering_controller/test/ackermann_steering_controller_params.yaml you can see that front_wheels_names and rear_wheels_names in fact might refer (depending on situation) to either a traction actuator or a steering actuator.

Here: https://github.com/ros-controls/ros2_controllers/blob/76b1ce7da5e7a102ef3a1c8c4f1b0de90ed0d137/steering_controllers_library/src/steering_controllers_library.cpp#L280-L316 a distinction is then made depending on front_steering param (and linked to #648, I should introduce here somehow all_wheel_drive parameter).

It feels to me that otherwise introducing additional implicit conventions in combination of new flags / number of passed joints and their interpretation as wheels or steers will complicate even more the steering_controller package...

Semantically, we have no enforcement at configuration level of what is "wheel" and what is "steer". I propose to add front_steer_names and rear_steer_names, and refactor the code in support of both #648 and #526 (which I am also involved in).

roncapat avatar Jul 04 '23 12:07 roncapat

Hope it's not too late to open this conversation back up. I think these changes both make sense as @roncapat suggests:

  1. add front_steer_names and rear_steer_names as parameters.
  2. add support for 4-wheel-drive and 4-wheel-steer.

My suggestion is to merge all models down to one controller which would result in steering_controller's configuration file to determine which kinematic needs to be used:

Vehicle Kinematic front_steer_names front_wheel_names rear_steer_names rear_wheel_names
Bicycle front-steer rear-only drive [front_steer_joint] [] [] [rear_drive_joint]
Bicycle with front and rear drive and rear-steer [] [front_drive_joint] [rear_steer_joint] [rear_drive_joint]
Ackermann with front-steer and rear-only drive [front_right_steer_joint, front_left_steer_joint] [] [] [rear_right_drive_joint, rear_left_drive_joint]
Ackermann All-wheel-drive [front_right_steer_joint, front_left_steer_joint] [front_right_drive_joint, front_left_drive_joint] [] [rear_right_drive_joint, rear_left_drive_joint]
4-wheel-steer 4-wheel-drive [front_right_steer_joint, front_left_steer_joint] [front_right_drive_joint, front_left_drive_joint] [rear_right_steer_joint, rear_left_steer_joint] [rear_right_drive_joint, rear_left_drive_joint]

There would be 16 configurations in general (where some fall under diff_drive and perhaps those would want to be ignored or yet better integrated into this somehow :exploding_head:), and if all 16 of those could use the same controller, I think it would be very elegant (at least from the user's perspective).

I understand this will be somewhat of a massive undertaking, but I also think this will massively enhance the experience of any ros2_control user trying to set up a steering robot.

@bmagyar @christophfroehlich @destogl, Could I ask for your thoughts?

ARK3r avatar Oct 05 '23 22:10 ARK3r

That seems very clean! Love the table for a comprehensive recap.

roncapat avatar Oct 05 '23 22:10 roncapat

Thanks for bringing this up again. My thoughts on that topic:

  • I'm a fan of making things cleaner, e.g. thinking of position/velocity/effort controllers in this repo which is just a specialization of forward_command_controller and confused me more in the beginning than it helped ;)
  • ~~But from a code-perspective this maybe makes life not easier, there would be all forward/inverse kinematics in a single controller file? Now we have the interfaces in the library and all implementation stuff inside the controllers. Different controllers with a good documentation is maybe as useful for the users, and easier to maintain for us.~~ edit: my memory was wrong, the odometry is already centralized in the library for the different controller types.
  • Do we need new/different kinematics for the all-wheel drive versions above? Or is it just a matter of parameter handling?

not directly related:

  • Now we have two tricycle controllers, is the newer one feature equal? I'd appreciate to start simplifying things there first.
  • Should we refactor the diff-drive controller to use steering controllers library, too?

christophfroehlich avatar Oct 08 '23 15:10 christophfroehlich

@christophfroehlich:

  • at least for now I would keep diff-drive controller aside, because it is a case where you don't point all wheels towards a virtual ICR but you skid instead. So I would think it of a "different family" of controllers.
  • don't know that much the tricicle controller, sorry
  • I wouldn't do everything in one step... I mean: if we stick at my original proposal in the 1st post of this Issue, before getting to the holy grail of generalizations, even just getting to distinguish between wheels and steers in parameters of ackermann node can help:
    • the user, when undestanding semantically the parameters while configuring the node
    • the developer, in extending the ackermann controller with the missing modes (up to the all-wheel, all-steer)

roncapat avatar Oct 09 '23 19:10 roncapat

@roncapat fine imho, I just wanted to understand better what is the (long-term) proposal by @ARK3r as he mentioned the diff_drive controller, too. You have my go for the mentioned renaming as a first step :+1:

christophfroehlich avatar Oct 10 '23 06:10 christophfroehlich

@roncapat is raising a fair point, and I think they should stay seperated, at least for now.

My thinking was that if we are to use one controller for any kind of "steering" robot, then the code would have to figure out based on the values provided to the arguments, similar to the table, what setup to use. I thought if we are letting the code decide this, then we can eventually have a "mobile robot controller" where if no steering_wheels are specific, in the front nor the back, then the code would recognize this as a diff drive robot.

I think it would be nice to have a unification of all mobile robots into one controller but this would need to get done in a way that would make it easier for the user and the developer, otherwise it doesn't make sense to make these changes. I think it is for now more urgent to just fix the controllers.

ARK3r avatar Oct 10 '23 19:10 ARK3r

Some thoughts:

  • general ackermann robots (all the cases in your table) are based on the concept of "front" & "rear" wheels.

    • it might be nice to extend long-term to 6-wheeled robots, if we want to be even more generic. Again, here lots of complexity -> no go before ever making a full-fledged all drive, all steer 4w controller IMHO.

    • keep into account that for some 4wheel, 4 steer robots, ackermann control is only one of the available modes, being the others crabbing or turn-in-place... these may be handled theoretically as very special cases of ackermann (instead of a combination of rotation and translation, you have only rotation or only translation, considering the Instantaneous Center of Rotation ICR either infinitely away from the robot (crabbing) or under its body (turn-in-place) -> again, no go before having a full ackermann implemention for 4w 4s vehicles IMHO.

      • controller switching?
  • diff drive/skid steer work on the concept of "left" & "right" wheel

  • then we are missing all the swedish wheels omni robots, another family... ah!

Conclusion: we have to do this at little steps, not messing up taxonomy of different vehicles, but trying to find a coherent frame for each.

roncapat avatar Oct 10 '23 19:10 roncapat