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

Add support for bridging NavSat #224

Merged
merged 3 commits into from
Apr 22, 2022
Merged

Conversation

TyHowellWork
Copy link
Contributor

@TyHowellWork TyHowellWork commented Mar 11, 2022

🎉 New feature

Closes #218

Summary

Adds two-way bridge for NavSat messages between ROS and IGN along with a demo launch file.

Test it

Test IGN->ROS by running roslaunch ros_ign_gazebo_demos navsat.launch.
Test ROS->IGN by roslaunch ros_ign_gazebo_demos navsat.launch and ign topic -t /navsat -e

rostopic pub -r 1 /navsat sensor_msgs/NavSatFix "header:
  seq: 10
  stamp: {secs: 10, nsecs: 0}
  frame_id: 'back_to_ign'
status: {status: 0, service: 0}
latitude: 10.0
longitude: 10.0
altitude: 10.0
position_covariance: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]
position_covariance_type: 0"

Even using the contributing guides I was unable to get codecheck or the tests running :(

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

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. The implementation, test and example look great!

The only catch is that this branch is supported with both Citadel and Fortress, but the NavSat message and the NavSat sensor are only available from Fortress. So we have a couple of options:

  • Add ifdefs so the NavSat is only present on IGN_MSGS_VER >= 8
  • Or backport the NavSat message to ign-msgs5 - while this is trivial, backporting the NavSat sensor itself isn't. It shouldn't hurt having the message there though.

In any case, we should add a note to ros_ign_gazebo_demos/README.md that the GNSS example only works from Fortress.

ros_ign_bridge/test/test_utils.h Outdated Show resolved Hide resolved
ros_ign_bridge/test/test_utils.h Outdated Show resolved Hide resolved
@TyHowellWork
Copy link
Contributor Author

@chapulina @ahcorde Thanks for the reviews I should have a fix :) by the end of the day.

Signed-off-by: Tyler Howell <[email protected]>
Signed-off-by: Tyler Howell <[email protected]>
@TyHowellWork
Copy link
Contributor Author

ign-msgs5 PR has been submitted that backports NavSat message only.

@chapulina chapulina mentioned this pull request Mar 16, 2022
7 tasks
@chapulina
Copy link
Contributor

chapulina commented Mar 16, 2022

ign-msgs5 gazebosim/gz-msgs#231 has been submitted that backports NavSat message only.

Thanks! Here are the next steps:

  • Release ign-msgs5 with the NavSat message: 🎈 5.9.0 gz-msgs#232
  • Pull that into packages.ros.org with reprepro
  • Run CI on this PR

@chapulina chapulina added the ROS 1 ROS 1 label Apr 7, 2022
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.

Ready! Thanks for the wait!

@chapulina chapulina merged commit 4c1c819 into gazebosim:melodic Apr 22, 2022
mjcarroll added a commit that referenced this pull request Jun 27, 2022
Authored-by: Tyler Howell <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
mjcarroll added a commit that referenced this pull request Jun 27, 2022
Co-authored-by: Tyler Howell <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
mjcarroll added a commit that referenced this pull request Jun 28, 2022
Co-authored-by: Tyler Howell <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
mjcarroll added a commit that referenced this pull request Jun 28, 2022
Co-authored-by: Tyler Howell <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ROS 1 ROS 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NavSat Ignition Gazebo ROS Bridge Update
3 participants