Add ackermann_steering_controller to kinetic-devel

[MoriKen254]

Taken over from #289 to safely and cleanly rebase the branch.

[MoriKen254]

Do I need to add <exec_depend>ackermann_steering_controller</exec_depend> into kinetic-devel/ros_controllers/package.xml?

[bmagyar]

Yes please. I'm pushing a new release now but after you made the change I think we are ready for a merge.

[MoriKen254]

Thank you very much for your reply. I just added <exec_depend>ackermann_steering_controller</exec_depend> into package.xml of ros_controllers meta package.

If this PR is merged, I would like to add the controllers into other distributions later.

[MoriKen254]

In terms of CI build test, all of them look passed in my forked res_controlelrs repository.

[bmagyar]

Thanks for this. Even though there are a few things I could be nit-picky about, it is time to merge this controller, we can do refactoring further down the line.

[bmagyar]

@ipa-mdl please approve if you like it and go ahead with the merge 👍

[mathias-luedtke]

I don't see why this code needs a dependency on Gazebo.
Please check the other dependencies as well.

[mathias-luedtke]

In general this PR contains a lot of duplcated code (from diff_drive_controller), I would prefer to refactor things properly.

[MoriKen254]

Thank you so much for kind review.

Yes, the comment #356 (comment) makes sense.

Probably https://github.com/ros-controls/ros_controllers/tree/kinetic-devel/four_wheel_steering_controller has the same matter.

I would like to refactor them at the next step.

[mathias-luedtke]

Thanks for the update!
There are still some issues (see comments).

I have now compiled the list of dependencies:

<depend> (find_package + CATKIN_DEPENDS):
controller_interface
diff_drive_controller
hardware_interface
nav_msgs
realtime_tools
roscpp
tf

boost is missing as well, but I can ignore the fact for now ;)

<build_depend> + <exec_depend> (find_package):
pluginlib
urdf_parser (urdf?)

<test_depend> (find_package):
controller_manager
(geometry_msgs)
(std_msgs)
rosunit
std_srvs

<test_depend> (no compile options):
gazebo_ros
xacro

[mathias-luedtke]

I believe that catkin should not get resolved twice with a different set of components, but I have to double check.

[MoriKen254]

Fixed at a8087f7

[mathias-luedtke]

Not fixed, but I guess it is okay for now. Other packages do the same and it makes sense to fix them in a follow-up PR.

[MoriKen254]

We'll fix the point later as a follow-up PR including other controllers as well!

[mathias-luedtke]

I would like to refactor them at the next step.

Okay :)

[MoriKen254]

I've learnt a lot from you! Thank you so much.

boost is missing as well, but I can ignore the fact for now ;)

I added Boost related lines into CMakeLists.

Speaking of refactoring, I'm considering a model to add a super (could be an abstract) class such as wheel_mobile_robot_controller and the others, diff_drive, ackermann_steering and four_wheel_steering inherit that.

Of course it would require a huge modification but simplifies the whole codes at the same time. hope I can hear your opinion :)

[bmagyar]

I'd call it mobile_base_controller and add it as a separate package. It could be done on the melodic branch.

[mathias-luedtke]

Almost there 👍

[MoriKen254]

I'm sorry for bothering you because of my mistakes and very appreciate you guys on giving patiently me advises.

I would like to work on some points remaining in other PRs!

[mathias-luedtke]

All points addressed, thank you very much!