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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose Contacts through ROS bridge #175

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Expose Contacts through ROS bridge #175

merged 1 commit into from
Aug 18, 2021

Conversation

vatanaksoytezer
Copy link
Contributor

@vatanaksoytezer vatanaksoytezer commented Aug 17, 2021 •
edited
Loading

Signed-off-by: Vatan Aksoy Tezer vatanaksoytezer@gmail.com

馃帀 New feature

Closes #174

Summary

The ultimate goal was to expose contact sensor messages through bridge. This adds a new message type to ros_ign_interfaces: JointWrench and adds six different types to ros_ign_bridge, namely:

  • UInt32
  • geometry_msgs/msgWrench
  • ros_ign_interfaces/msg/JointWrench
  • ros_ign_interfaces/msg/Entity
  • ros_ign_interfaces/msg/Contact
  • ros_ign_interfaces/msg/Contacts

I added UInt32 thinking I would need it but it turns out I don't but nevertheless it's just a new type for bridge 馃帀

Test it

ign gazebo -r contact_sensor.sdf
ros2 run ros_ign_bridge parameter_bridge '/contact_example@ros_ign_interfaces/msg/Contacts@ignition.msgs.Contacts'
ros2 topic echo /contact_example

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻馃敻

chapulina
chapulina suggested changes Aug 17, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! CI is unhappy:

 In file included from /github/home/ws/src/ros_ign/ros_ign_image/src/image_bridge.cpp:19:
/github/home/ws/install/ros_ign_bridge/include/ros_ign_bridge/convert.hpp:30:10: fatal error: ros_ign_interfaces/msg/entity.hpp: No such file or directory
   30 | #include <ros_ign_interfaces/msg/entity.hpp>
      |

ros_ign_bridge/src/convert.cpp Outdated Show resolved Hide resolved
ros_ign_interfaces/msg/Contact.msg
@@ -3,4 +3,4 @@ ros_ign_interfaces/Entity collision2 # Contact collision2
geometry_msgs/Vector3[] positions # List of contact position
geometry_msgs/Vector3[] normals # List of contact normals
float64[] depths # List of penetration depths
geometry_msgs/Wrench[] wrenches # List of forces/torques
ros_ign_interfaces/JointWrench[] wrenches # List of joint wrenches including forces/torques
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll just note that changing the definition of a message breaks downstream users, so we need to be mindful about it. Since this message isn't used by anything in this repository yet, and this is targeting the ros2 branch, I think it's a good move though 馃憤

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up!

ros_ign_interfaces/msg/JointWrench.msg
@@ -0,0 +1,8 @@
std_msgs/Header header # Time stamp
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this new message is matching ignition::msgs::JointWrench, but I'll just leave a note that we should be modernizing this in the future to use clearer simulation concepts (i.e. link or collision instead of body and ignition::msgs::Entity instead of id + name).

I'd suggest preemptively doing that here now even though it won't match the Ignition message, but I don't have a strong preference.

Copy link
Contributor Author

@vatanaksoytezer vatanaksoytezer Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I am OK changing this and also the proto possibly to do this more high level. Do you mind if I do that in another PR and just open an issue to track? I am also down to change any other types as well, I can take care of them in that future PR.

@vatanaksoytezer
Copy link
Contributor Author

vatanaksoytezer commented Aug 17, 2021 •
edited
Loading

Thanks for the PR! CI is unhappy:

@chapulina I was looking at this, do you have any idea why ros_ign_image is unhappy with ros_ign_interfaces?

Should be fixed.

@vatanaksoytezer
Expose Contacts through ROS bridge
72b8bc0 
Signed-off-by: Vatan Aksoy Tezer <vatanaksoytezer@gmail.com>
chapulina
chapulina approved these changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks again, this looks great 馃憤

@chapulina chapulina merged commit 25e5ba0 into gazebosim:ros2 Aug 18, 2021
@chapulina chapulina mentioned this pull request Oct 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vatanaksoytezer @chapulina