Skip to content
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

Gazebo transport bridge (#185) #187

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

roncapat
Copy link

@roncapat roncapat commented Mar 22, 2023

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 (gazebosim/gazebo-classic#3309), and thus is needed until upstream merge.

@roncapat roncapat changed the title Gazebo transport bridge (#185, part 1) Gazebo transport bridge (#185) Mar 22, 2023
@roncapat roncapat marked this pull request as ready for review March 23, 2023 21:08
@roncapat
Copy link
Author

roncapat commented Mar 29, 2023

@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
Copy link
Author

roncapat commented Mar 29, 2023

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?

@ahcorde
Copy link
Collaborator

ahcorde commented May 16, 2023

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 ?

@roncapat
Copy link
Author

roncapat commented May 16, 2023

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 roncapat force-pushed the sea branch 2 times, most recently from a332f0c to d98dea7 Compare May 18, 2023 09:55
@roncapat
Copy link
Author

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 gazebosim/gazebo-classic#3309

@bmagyar
Copy link
Member

bmagyar commented May 23, 2023

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?

@roncapat
Copy link
Author

roncapat commented May 23, 2023

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
Copy link
Author

roncapat commented Jun 5, 2023

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

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.

What I often have to ask for 😇 : 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.

@roncapat
Copy link
Author

roncapat commented Jun 7, 2023

What I often have to ask for 😇 : 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?

@christophfroehlich
Copy link
Contributor

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.

@roncapat
Copy link
Author

roncapat commented Jun 7, 2023

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.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Please clean this up so we can do a proper review

gazebo_ros2_control/CMakeLists.txt Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants