Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update navsat proto to include covariance #489

Open
wants to merge 1 commit into
base: gz-msgs11
Choose a base branch
from

Conversation

EmmanuelMess
Copy link

@EmmanuelMess EmmanuelMess commented Mar 6, 2025

🦟 Bug fix

Fixes #488

Summary

This PR adds parity with https://github.com/ros2/common_interfaces/blob/master/sensor_msgs/msg/NavSatFix.msg message.
For completeness it also adds the velocity DOP.

Disclamer: I have no idea how this actually integrates with the rest of gazebo, and I couldn't find much info, so I would thank you if you tell me where I would need to add code to make it work with the gz ros2 bridge.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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.

@EmmanuelMess EmmanuelMess requested a review from caguero as a code owner March 6, 2025 01:34
@github-actions github-actions bot added 🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty labels Mar 6, 2025
Signed-off-by: EmmanuelMess <emmanuelbendavid@gmail.com>
@EmmanuelMess EmmanuelMess force-pushed the emmanuelmess/fix/navsat_msg_dop branch from db646e1 to a1f59d9 Compare March 6, 2025 01:48
@iche033
Copy link
Contributor

iche033 commented Mar 7, 2025

I think updating msg fields breaks ABI compatibility of the generated msg library. Is that still the case @caguero?

If so, we would probably need to add a new navsat_with_covariance.proto message (similar to other *_with_covariance protos). In any case, we'll also then need to update gz-sensors' NavSatSensor to publish this msg with covariance to a new topic.

For ros_gz, a new convert function needs to be added in https://github.com/gazebosim/ros_gz/blob/ros2/ros_gz_bridge/include/ros_gz_bridge/convert/sensor_msgs.hpp for converting between the new gz navsat with covariance msg and ROS' NavSatFix msg.

@EmmanuelMess
Copy link
Author

If so, we would probably need to add a new navsat_with_covariance.proto message (similar to other *_with_covariance protos). In any case, we'll also then need to update gz-sensors' NavSatSensor to publish this msg with covariance to a new topic.

Should the old message be deprecated in some way as part of this?

@iche033
Copy link
Contributor

iche033 commented Mar 8, 2025

Should the old message be deprecated in some way as part of this?

I think we can keep the old message there. The new message will include the existing one, plus the other additional fields for covariances like the ones you added. Btw I see that the covariance fields have the Float_V type in other messages, e.g. pose_with_covariance.proto instead of Double_V. May be good to keep them consistent.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: Inbox
Development

Successfully merging this pull request may close these issues.

Add covariance (or dop) for navsat proto
2 participants