gazebo_ros2_control icon indicating copy to clipboard operation
gazebo_ros2_control copied to clipboard

Gazebo transport bridge (#185)

Open roncapat opened this issue 1 year ago • 11 comments

See #185 for rationale.

This allows state_interfaces and command_interfaces with value different from "effort", "velocity" or "position" to be interpreted as Gazebo Transport topics to which either publish or subscribe.

Gazebo::Transport::Node class has to be redefined out-of-tree because the PR that introduces a required method addition has been submitted this same day (https://github.com/gazebosim/gazebo-classic/pull/3309), and thus is needed until upstream merge.

roncapat avatar Mar 22 '23 22:03 roncapat

@ahcorde may I ask you to trigger the workflows? Not sure if the last one was triggered by you or not, but since it failed I made some fixes :)

EDIT: nevermind, the issue is with rolling sensor_msgs:

CMake Error at /opt/ros/rolling/share/sensor_msgs/cmake/ament_cmake_export_dependencies-extras.cmake:21 (find_package):
  By not providing "Findservice_msgs.cmake" in CMAKE_MODULE_PATH this project
  has asked CMake to find a package configuration file provided by
  "service_msgs", but CMake did not find one.

  Could not find a package configuration file provided by "service_msgs" with
  any of the following names:

    service_msgsConfig.cmake
    service_msgs-config.cmake

EDIT 2: the issue is with ros-rolling-rcl package being kept back during workflow initialization.
I had to modify CI adding what follows (in my branch):

apt-get upgrade -q -y --with-new-pkgs ros-rolling-rcl

Fork CI (now only uncrustify issues):

fork CI

roncapat avatar Mar 29 '23 09:03 roncapat

Ok, up to now the code builds but CI fails uncrustify/cpplint due to foreign code (Gazebo's modified node.hh & node.cc).

This means that:

  • either we re-format the files (diverging from Gazebo's version)
  • or we wait for merge of the gazebo PR and delete here the files

Any suggestions or ideas?

roncapat avatar Mar 29 '23 15:03 roncapat

Hi @roncapat,

Thank you for your contribution but I would like to understand this better, what's the motivation behind this ? You already can access to these topics throught ROS 2 in particular the ros2_control APIs. Why do you need to expose this in gz::transport ?

ahcorde avatar May 16 '23 11:05 ahcorde

Hi @roncapat,

Thank you for your contribution but I would like to understand this better, what's the motivation behind this ? You already can access to these topics throught ROS 2 in particular the ros2_control APIs. Why do you need to expose this in gz::transport ?

Hi, this is to use standalone, non-ROS gazebo plugins and to have access /expose over ROS2 (via ros_control) to all gazebo topics. So, both read/write capability.

It's like to add universal exchange of data between ROS2 and GZ transport, potentially allowing a user not to port a third-party Gazebo plugin to gazebo_ros and explicitly pub/sub from the inside of the plugin.

A note, if you tell me it's something you are going to consider, I have some more commits to push here then ;) Fixed some nasty bug (and started using in our lab environment with no further issues since then).

And BTW, do you know some Gazebo-classic dev to speed up the approval of the "lambda" PR?

roncapat avatar May 16 '23 11:05 roncapat

Linting of Gazebo-included files can't be fully achieved since cpplint and uncrustify collide each other on some lines. This may of course be avoided at all by https://github.com/gazebosim/gazebo-classic/pull/3309

roncapat avatar May 18 '23 10:05 roncapat

I've tried pushing a release just now but there's a missing requirement for Gazebo that the package.xml picks up by default. I don't think that has anything to do with your conflict, I suspect this PR is based on an ignition PR?

bmagyar avatar May 23 '23 19:05 bmagyar

May I ask where I can see a log of the problem? From mobile I'm just seeing test failures related to linting.

Edit/verbose:

no, this patch in principle is standalone, in the sense that it ships with its modded gazebo files (node.cc and node.hh).

Those files may be removed theoretically if gazebo-classic ever merges the other PR I linked above, but even then all older releases of gazebo-classic would cease to be compatible with this patch.

This means that my personal idea/recommendation is, to just ship those files and do not plan their removal anytime soon. Question is: this is surely an ugly solution, so how can we handle those files in this repo? What about linting issues, for example?

Regarding the merge conflict, tomorrow I will rebase👍

roncapat avatar May 23 '23 21:05 roncapat

@bmagyar may I ask you how would you like to proceed here / what I can do to revise this patch set?

roncapat avatar Jun 05 '23 10:06 roncapat

What I often have to ask for :innocent: : Could you please add this feature to the documentation and/or add a new demo to the demo? Maybe then the use case gets clearer for everyone.

Hi @christophfroehlich, I will be happy to also work on minimum example + docs, but first I need to resolve my concern about the included files from Gazebo. If you ensure me that this PR is of interest and those 2 files are not an issue / you tell me how you want me to handle them differently, I will be happy then allocate effort to complete the PR with docs & demo.

May I ask then also to you an opinion on this?

roncapat avatar Jun 07 '23 06:06 roncapat

May I ask then also to you an opinion on this?

It seems that the gazebo-PR has a chance to get merged -> let's wait for that to avoid duplicating node.hh. Once gazebo is released, do we have to support older versions of gazebo from this package then? don't know..

Honestly speaking, I have too little experience to evaluate the benefits from this PR and therefore asked for a demo example.

christophfroehlich avatar Jun 07 '23 06:06 christophfroehlich

It seems that the gazebo-PR has a chance to get merged -> let's wait for that to avoid duplicating node.hh. Once gazebo is released, do we have to support older versions of gazebo from this package then? don't know...

My same doubt, since I do not know whether gazebo-classic is handled by any ROS "sync" or is handled standalone.

roncapat avatar Jun 07 '23 07:06 roncapat