Add GPS / NavSat sensor to sdf9 #453

Dotrar · 2020-12-30T08:06:19Z

Here is a basic GPS implementation built on SDF10

this is related to the following issue: gazebosim/gz-sensors#23 as the GPS sensor will need an SDFormat to go with it.

I copied / got inspired by the code in the imu and altimeter sensors, but I only made a single noise component for both position and velocity.

Hope this suits. I couldn't find a way to squash all of my commits into the 1 result. But if this is accepted then it can just be squashed at merge.

chapulina

Thank you for the contribution! This is going in a good direction, but the API needs to be updated to match the XML spec.

Also, please also add a test for the GPS to test/integration/link_dom.cc, similar to the IMU tests there. This should cover the code path from XML to C++.

https://github.com/osrf/sdformat/blob/f6e029bc88d2ca6c2586fc88dddc0c21f0798f03/test/integration/link_dom.cc#L388-L445

include/sdf/Gps.hh

Dotrar · 2020-12-30T23:16:42Z

I seem to have broken the DCO by using the github auto-edit feature, but each of the commits should be signed off.
my git log:

commit 26296f04edde7443e24c171c8c02e0f04db70c72 (HEAD -> gps_sensor_sdf10, origin/gps_sensor_sdf10)
Author: Dre Westcook <[email protected]>
Date:   Thu Dec 31 10:14:12 2020 +1100

    fixed lints
    
    Signed-off-by: Dre Westcook <[email protected]>

commit b5501938eb38e6d392a5dccc17908074d2e55d61
Author: Dre Westcook <[email protected]>
Date:   Thu Dec 31 09:23:52 2020 +1100

    changed gps to follow spec
    
    Signed-off-by: Dre Westcook <[email protected]>

commit 6599658c77bb818715079d069c16a51346e432af
Author: Dre West <[email protected]>
Date:   Thu Dec 31 08:38:53 2020 +1100

    Update include/sdf/Gps.hh
    
    Co-authored-by: Louise Poubel <[email protected]>
    Signed-off-by: Dre Westcook <[email protected]>

commit 0e54367ab6f874373bac239d3553068f2cac7781
Author: Alejandro Hernández Cordero <[email protected]>
Date:   Tue Dec 29 00:22:16 2020 +0100

    Added render_order to material (#446)
    
    Signed-off-by: ahcorde <[email protected]>

commit 8623a03459644429ebcb45c5df8357073dbd5e08
Author: Dre Westcook <[email protected]>
Date:   Tue Dec 29 10:54:46 2020 +1100

    Add GPS into SDFormat.
    
    Signed-off-by: Dre Westcook <[email protected]>

commit 7dbf12136f01c70470a41255618dd08937e7bfad (tag: sdformat10_10.1.0)
Author: Nate Koenig <[email protected]>
Date:   Tue Dec 15 13:59:31 2020 -0800

    Prepare for 10.1.0 (#442)
    
    Signed-off-by: Nate Koenig <[email protected]>
    
    Co-authored-by: Nate Koenig <[email protected]>

codecov-io · 2020-12-30T23:32:40Z

Codecov Report

Merging #453 (29e9cae) into sdf10 (d33b4ae) will increase coverage by 0.02%.
The diff coverage is 90.72%.

@@            Coverage Diff             @@
##            sdf10     #453      +/-   ##
==========================================
+ Coverage   87.78%   87.81%   +0.02%     
==========================================
  Files          62       63       +1     
  Lines        9577     9672      +95     
==========================================
+ Hits         8407     8493      +86     
- Misses       1170     1179       +9

Impacted Files	Coverage Δ
src/Sensor.cc	`80.68% <60.00%> (-1.39%)`	⬇️
src/SatNav.cc	`96.34% <96.34%> (ø)`

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 d33b4ae...2beecde. Read the comment docs.

mjcarroll · 2021-01-06T14:24:17Z

How would you feel about changing this to be something like NavSat, as it is more generic. GPS is the American variant of a Satellite Navigation system, and chips that use multiple variants (Glonass, Galileo, Compass) are becoming widely available

chapulina · 2021-01-06T19:09:28Z

How would you feel about changing this to be something like NavSat

We could add NavSat and keep supporting GPS for a while as legacy, like we're doing for the ray sensors:

https://github.com/osrf/sdformat/blob/cd2ad01d709e754bfdd63b76eb5ffb714cf679a3/sdf/1.7/sensor.sdf#L33

chapulina

I just have minor comments, this is looking good to me.

What do you think of the NavSat idea, @Dotrar ? You'd need to change the new class to sdf::NavSat, as well as the enum... As a reference, see how lidar / ray sensors are implemented.

src/Gps.cc

Dotrar · 2021-01-13T08:11:58Z

I just have minor comments, this is looking good to me.

What do you think of the NavSat idea, @Dotrar ? You'd need to change the new class to sdf::NavSat, as well as the enum... As a reference, see how lidar / ray sensors are implemented.

Yeah no worries; I'll see if i can track down all references and change it to NavSat

should it be as easy as a s/Gps/NavSat/g ? also I'll put in those other changes soon.

chapulina · 2021-01-14T03:43:46Z

should it be as easy as a s/Gps/NavSat/g ?

Thanks! You may also need some s/GPS/NAVSAT/g and s/gps/navSat/g. And git mv Gps NavSat.

chapulina · 2021-02-06T04:17:09Z

Hi @Dotrar , are you planning to follow up on the NavSat idea? Thanks!

Dotrar · 2021-02-07T06:01:59Z

Hi @Dotrar , are you planning to follow up on the NavSat idea? Thanks!

yes, Sorry I'm being a little slow here, all sorts of stuff going on; I'll make sure I get this done soon.

Dotrar · 2021-02-09T09:31:07Z

Hey @chapulina ;

I've changed GPS to SATNAV, but I'm not sure if I went too far with changing stuff like the enum in Sensor.hh

https://github.com/osrf/sdformat/blob/master/include/sdf/Sensor.hh#L72

do you want to see if I've got this right? I did try to copy the ray/lidar example but I don't know if that includes in the enum, as there isn't a RAY in the enum.

also I think I've buggered up the commit log a little when I committed without signing then went back trying to sign previous commits; do you mind if I nuke this PR and write it all into a new one? or is this recoverable/not a huge drama?

chapulina

Thank you for iterating! This is looking good!

I've changed GPS to SATNAV

Would it be possible to use NAVSAT everywhere instead of SATNAV?

I did try to copy the ray/lidar example but I don't know if that includes in the enum, as there isn't a RAY in the enum.

I think that one predated the enum, so we never added it. For GPS, let's keep it there for backwards compatibility. We can remove it in a future version.

do you mind if I nuke this PR and write it all into a new one? or is this recoverable/not a huge drama?

I'm fine with creating a new PR. This should be recoverable though. Try rebasing your branch onto sdf10 and make sure the signature is in every commit's comment. Alternatively, you can squash all commits into one and only sign that one.

I think we should add a navsat.sdf file too, which is a copy of gps. Like we have both ray.sdf and lidar.sdf.

include/sdf/CMakeLists.txt

include/sdf/Sensor.hh

src/CMakeLists.txt

src/SatNav.cc

src/Sensor.cc

adlarkin · 2021-06-14T23:11:20Z

@chapulina and @scpeters: I have addressed the latest review comments, and believe that this is ready for final review!

Would it be possible to use NAVSAT everywhere instead of SATNAV?

Done in b128c9c. Some files had to be re-named, so apologies if it's a bit of a pain to review these changes 😬

I think we should add a navsat.sdf file too, which is a copy of gps.sdf. Like we have both ray.sdf and lidar.sdf.

I added this in b128c9c. I also updated sensors.sdf to note that gps is now deprecated in favor of navsat. If there are any other documentation files that need to be updated, let me know!

Regarding DCO: once final reviews are done and this PR has been approved, I can try to fix the Git history so that DCO will pass.

azeey · 2021-06-15T01:39:50Z

Can I ask why Navsat is preferred over Satnav? The latter seems more generic to me.

adlarkin · 2021-06-15T13:01:11Z

Can I ask why Navsat is preferred over Satnav? The latter seems more generic to me.

I think the original motivation came from #453 (comment). Maybe @mjcarroll or @chapulina can give their thoughts?

src/Sensor_TEST.cc

chapulina · 2021-06-25T00:10:37Z

Can I ask why Navsat is preferred over Satnav? The latter seems more generic to me.

Good question, Satellite Navigation refers to the navigation system, while to me "Navigation Satellite" seems to refer to the specific device, i.e. sensor, being used.

But what motivated my original request was the equivalent ROS message sensor_msgs/NavSatFix. ROS messages are widely used and usually receive a lot of scrutiny (like the recent API review) so I think it's usually a good idea to follow their patterns unless we have a good reason not to.

That said, I don't feel too strongly about this after giving it more thought. If there's enough interest in keeping SatNav I'm ok with that.

azeey · 2021-06-25T16:54:13Z

Ah, I didn't know about sensor_msgs/NavSatFix. It makes sense to use NavSat then since there's a precedent.

mjcarroll · 2021-06-25T16:57:32Z

That said, I don't feel too strongly about this after giving it more thought. If there's enough interest in keeping SatNav I'm ok with that.

If it helps at all, NAVSAT was the precursor to GPS https://en.wikipedia.org/wiki/Transit_(satellite)

azeey

The code looks good. I think it can go into sdf9 though.

src/Sensor_TEST.cc

Signed-off-by: Dre Westcook <[email protected]>

Co-authored-by: Louise Poubel <[email protected]> Signed-off-by: Dre Westcook <[email protected]>

Signed-off-by: Dre Westcook <[email protected]>

Signed-off-by: Ashton Larkin <[email protected]>

chapulina

Rebased to sdf9, LGTM!

github-actions bot added 🏢 edifice Ignition Edifice 🔮 dome Ignition Dome labels Dec 30, 2020

This was referenced Dec 30, 2020

Add Gps Sensor to IGN-Sensors library gazebosim/gz-sensors#72

Closed

GPS sensor in Gazebo gazebosim/gz-sim#519

Closed

chapulina suggested changes Dec 30, 2020

View reviewed changes

include/sdf/Gps.hh Outdated Show resolved Hide resolved

include/sdf/Gps.hh Outdated Show resolved Hide resolved

chapulina reviewed Jan 13, 2021

View reviewed changes

src/Gps.cc Outdated Show resolved Hide resolved

src/Gps.cc Outdated Show resolved Hide resolved

src/Gps.cc Outdated Show resolved Hide resolved

Dotrar requested a review from chapulina February 9, 2021 09:48

chapulina removed the 🏢 edifice Ignition Edifice label Mar 12, 2021

chapulina requested a review from adlarkin March 15, 2021 18:56

chapulina reviewed Apr 6, 2021

View reviewed changes

include/sdf/CMakeLists.txt Outdated Show resolved Hide resolved

include/sdf/Sensor.hh Outdated Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

src/SatNav.cc Outdated Show resolved Hide resolved

src/Sensor.cc Outdated Show resolved Hide resolved

scpeters assigned adlarkin May 24, 2021

adlarkin requested review from scpeters and chapulina June 14, 2021 23:12

adlarkin reviewed Jun 15, 2021

View reviewed changes

src/Sensor_TEST.cc Show resolved Hide resolved

azeey approved these changes Aug 4, 2021

View reviewed changes

src/Sensor_TEST.cc Show resolved Hide resolved

Dre Westcook and others added 8 commits August 13, 2021 16:33

Add GPS into SDFormat.

2f5e932

Signed-off-by: Dre Westcook <[email protected]>

Update include/sdf/Gps.hh

5a1fe49

Co-authored-by: Louise Poubel <[email protected]> Signed-off-by: Dre Westcook <[email protected]>

changed gps to follow spec

2726e8c

Signed-off-by: Dre Westcook <[email protected]>

fixed lints

b616135

Signed-off-by: Dre Westcook <[email protected]>

change gps to satnav

972ac6b

changed for line limit

6f6648c

rename satnav to navsat, and other review commments

12526b7

Signed-off-by: Ashton Larkin <[email protected]>

add missing navsat.sdf include in sensor.sdf

b98dbac

Signed-off-by: Ashton Larkin <[email protected]>

chapulina changed the base branch from sdf10 to sdf9 August 13, 2021 23:35

chapulina changed the title ~~Add GPS sensor to SDF10~~ Add GPS sensor to sdf9 Aug 13, 2021

chapulina added 🏰 citadel Ignition Citadel and removed 🔮 dome Ignition Dome labels Aug 13, 2021

chapulina approved these changes Aug 13, 2021

View reviewed changes

chapulina enabled auto-merge (squash) August 13, 2021 23:38

chapulina merged commit b889d93 into gazebosim:sdf9 Aug 14, 2021

This was referenced Dec 13, 2021

Add NavSat messages gazebosim/gz-msgs#206

Merged

Add NavSat (GPS) sensor gazebosim/gz-sensors#177

Merged

Add NavSat sensor (GPS) gazebosim/gz-sim#1248

Merged

chapulina changed the title ~~Add GPS sensor to sdf9~~ Add GPS / NavSat sensor to sdf9 Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GPS / NavSat sensor to sdf9 #453

Add GPS / NavSat sensor to sdf9 #453

Dotrar commented Dec 30, 2020

chapulina left a comment

Dotrar commented Dec 30, 2020

codecov-io commented Dec 30, 2020 •

edited

Loading

mjcarroll commented Jan 6, 2021

chapulina commented Jan 6, 2021

chapulina left a comment

Dotrar commented Jan 13, 2021

chapulina commented Jan 14, 2021

chapulina commented Feb 6, 2021

Dotrar commented Feb 7, 2021

Dotrar commented Feb 9, 2021

chapulina left a comment •

edited

Loading

adlarkin commented Jun 14, 2021

azeey commented Jun 15, 2021

adlarkin commented Jun 15, 2021

chapulina commented Jun 25, 2021

azeey commented Jun 25, 2021

mjcarroll commented Jun 25, 2021

azeey left a comment

chapulina left a comment

Add GPS / NavSat sensor to sdf9 #453

Add GPS / NavSat sensor to sdf9 #453

Conversation

Dotrar commented Dec 30, 2020

chapulina left a comment

Choose a reason for hiding this comment

Dotrar commented Dec 30, 2020

codecov-io commented Dec 30, 2020 • edited Loading

Codecov Report

mjcarroll commented Jan 6, 2021

chapulina commented Jan 6, 2021

chapulina left a comment

Choose a reason for hiding this comment

Dotrar commented Jan 13, 2021

chapulina commented Jan 14, 2021

chapulina commented Feb 6, 2021

Dotrar commented Feb 7, 2021

Dotrar commented Feb 9, 2021

chapulina left a comment • edited Loading

Choose a reason for hiding this comment

adlarkin commented Jun 14, 2021

azeey commented Jun 15, 2021

adlarkin commented Jun 15, 2021

chapulina commented Jun 25, 2021

azeey commented Jun 25, 2021

mjcarroll commented Jun 25, 2021

azeey left a comment

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 30, 2020 •

edited

Loading

chapulina left a comment •

edited

Loading