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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add NavSat messages #206

Merged
merged 2 commits into from
Jan 4, 2022
Merged

Add NavSat messages #206

merged 2 commits into from
Jan 4, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Dec 13, 2021 •
edited
Loading

🎉 New feature

Part of

Required by

Related to

Summary

On gazebosim/sdformat#453 we decided to use the more generic "NavSat" name for navigation satellite systems, instead of GPS, which is one specific type. This change should be reflected on the messages too.

This PR is only adding the new NavSat messages, but we should also update the sensors message to use it on Garden.

Here's the ROS 2 NavSatFix message as a reference.

I retargeted this PR from Dome to Fortress because the GPS sensor will need to use spherical coordinates (see gazebosim/gz-sim#981).

Test it

I'm updating the downstream PRs to use this message.

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 requested a review from caguero as a code owner December 13, 2021 23:06
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Dec 13, 2021
@codecov
Copy link

codecov bot commented Dec 13, 2021 •
edited
Loading

Codecov Report

Merging #206 (ca3cd0f) into ign-msgs8 (f0b2a69) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-msgs8     #206   +/-   ##
==========================================
  Coverage      84.56%   84.56%           
==========================================
  Files              7        7           
  Lines            855      855           
==========================================
  Hits             723      723           
  Misses           132      132

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b2a69...ca3cd0f. Read the comment docs.

@chapulina chapulina mentioned this pull request Dec 14, 2021
Merged
9 tasks
@chapulina
Add NavSat messages
987eb5f 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina changed the base branch from ign-msgs6 to ign-msgs8 December 14, 2021 00:19
@chapulina chapulina added 🏯 fortress Ignition Fortress and removed 🔮 dome Ignition Dome labels Dec 14, 2021
@chapulina chapulina mentioned this pull request Dec 14, 2021
Merged
9 tasks
caguero
caguero approved these changes Dec 14, 2021
View reviewed changes
Copy link
Collaborator

@caguero caguero left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me with just one minor comment!

caguero
caguero reviewed Dec 14, 2021
View reviewed changes
proto/ignition/msgs/navsat_sensor.proto Outdated
syntax = "proto3";
package ignition.msgs;
option java_package = "com.ignition.msgs";
option java_outer_classname = "Protos";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be NavSatSensorProtos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye! This was actually copy-pasted, there are other messages with the same problem:

$ grep -r "\"Protos\"" proto/ignition/msgs/
proto/ignition/msgs/sensor_noise.proto:option java_outer_classname = "Protos";
proto/ignition/msgs/polylinegeom.proto:option java_outer_classname = "Protos";
proto/ignition/msgs/gps_sensor.proto:option java_outer_classname = "Protos";

I'm not sure what this affects and whether fixing this could break ABI 🤔

@chapulina
Protos
ca3cd0f 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

Please don't merge this until the downstream PRs are done; I want to make sure the message has everything we need before releasing.

@chapulina chapulina mentioned this pull request Dec 23, 2021
Merged
9 tasks
@chapulina
Copy link
Contributor Author

A couple of downstream PRs have been approved, so I think this is good to go.

@chapulina chapulina enabled auto-merge (squash) January 4, 2022 21:43
@chapulina chapulina merged commit 8a98e6c into ign-msgs8 Jan 4, 2022
@chapulina chapulina deleted the chapulina/6/nav_sat branch January 4, 2022 21:45
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caguero caguero caguero approved these changes

Assignees
No one assigned
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chapulina @osrf-triage @caguero