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 NavSat sensor (GPS) #1248

Merged
merged 17 commits into from
Jan 12, 2022
Merged

Add NavSat sensor (GPS) #1248

merged 17 commits into from
Jan 12, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Dec 14, 2021

🎉 New feature

Part of

Supersedes

Requires

Summary

The fork for the original PR (#519) has been removed, so I can't push changes to that PR. I kept @Dotrar's commits in this branch.

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

Test it

  1. ign gazebo spherical_coordinates.sdf
  2. ign topic -e -t /navsat

navsat_move2

navsat_origin

TODO

  • This message is showing up on the console: [GUI] [Err] [Conversions.cc:1103] Attempting to convert a NavSat SDF sensor, but the sensor pointer is null.

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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Dre Westcook and others added 4 commits December 13, 2021 16:34
Signed-off-by: Dre Westcook <[email protected]>
Signed-off-by: Dre Westcook <[email protected]>
Signed-off-by: Dre Westcook <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added enhancement New feature or request sensors Sensors and sensor data labels Dec 14, 2021
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Dec 14, 2021
@chapulina chapulina mentioned this pull request Dec 14, 2021
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Dec 14, 2021
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina marked this pull request as ready for review December 22, 2021 22:16
Signed-off-by: Louise Poubel <[email protected]>
This was referenced Jan 4, 2022
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Jan 6, 2022
Signed-off-by: Louise Poubel <[email protected]>
.gitignore Outdated Show resolved Hide resolved
src/Conversions.cc Outdated Show resolved Hide resolved
src/systems/navsat/NavSat.cc Outdated Show resolved Hide resolved
src/systems/physics/Physics.cc Show resolved Hide resolved
src/systems/navsat/NavSat.cc Outdated Show resolved Hide resolved
src/systems/navsat/NavSat.cc Outdated Show resolved Hide resolved
src/systems/navsat/NavSat.cc Show resolved Hide resolved
test/integration/navsat_system.cc Outdated Show resolved Hide resolved
test/integration/navsat_system.cc Outdated Show resolved Hide resolved
@azeey azeey removed their request for review January 10, 2022 22:36
@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #1248 (c2c18c7) into ign-gazebo6 (fe335f9) will increase coverage by 0.00%.
The diff coverage is 63.20%.

Impacted file tree graph

@@              Coverage Diff              @@
##           ign-gazebo6    #1248    +/-   ##
=============================================
  Coverage        61.97%   61.97%            
=============================================
  Files              276      278     +2     
  Lines            23000    23106   +106     
=============================================
+ Hits             14254    14321    +67     
- Misses            8746     8785    +39     
Impacted Files Coverage Δ
src/systems/physics/Physics.cc 71.55% <ø> (ø)
src/Conversions.cc 82.55% <13.33%> (-1.15%) ⬇️
src/systems/navsat/NavSat.cc 69.04% <69.04%> (ø)
include/ignition/gazebo/components/NavSat.hh 100.00% <100.00%> (ø)
src/SdfEntityCreator.cc 85.33% <100.00%> (+0.21%) ⬆️

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 fe335f9...c2c18c7. Read the comment docs.

Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

Looks great! I left one comment if you wouldn't mind taking a look before merging.

src/systems/navsat/NavSat.cc Outdated Show resolved Hide resolved
src/systems/navsat/NavSat.cc Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina merged commit 989cd28 into ign-gazebo6 Jan 12, 2022
@chapulina chapulina deleted the chapulina/6/navsat branch January 12, 2022 18:36
@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-24-citadel-edifice-fortress/1241/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🏯 fortress Ignition Fortress sensors Sensors and sensor data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants