improve GPS2_RAW mavlink message #6534

amilcarlucas · 2017-07-03T18:01:01Z

implement missing "RTK number of satellites" and "RTK correction age" information in the GPS2_RAW MAVLink message for Nova and sbf GPS types

I splitted the #6424 into three ardupilot PRs to make it easier to review. This one should go in first to avoid conflicts.
I also expect this one to be less controversial.

This is the first one to go in. This is just a small part of it (been working and testing this since May 9th) , the rest of the related work is at #6424 #6541 ArduPilot/mavlink#48 ArduPilot/mavlink#49 ArduPilot/pymavlink#88 https://github.com/emlid/reach-docs/pull/26 emlid/RTKLIB#21 emlid/RTKLIB#28

WickedShell

I strongly appreciate the effort to improve the documentation on what we expect units to be, however I'm not sure if RMS is actually correct. I can't find strong indicators either way for u-blox and a couple of other ones.

With that being said maybe that's fine and we request that RMS is what we are looking at for accuracies and move towards that.

WickedShell · 2017-07-03T19:25:40Z

libraries/AP_GPS/AP_GPS.h

@@ -135,13 +135,17 @@ class AP_GPS
        uint8_t num_sats;                   ///< Number of visible satellites
        Vector3f velocity;                  ///< 3D velocity in m/s, in NED format
        float speed_accuracy;               ///< 3D velocity accuracy estimate in m/s
-        float horizontal_accuracy;          ///< horizontal accuracy estimate in m
-        float vertical_accuracy;            ///< vertical accuracy estimate in m
+        float horizontal_accuracy;          ///< horizontal accuracy estimate in m RMS


I'm not sure this is actually true for all the instances. U-blox doesn't note it formally either way in their protocol data sheet (that I can find), but they cite the accuracies as CEP when stating the performance of the unit, which I suspect means they might stick with that reporting for real time reasons as well since it would correspond with the data sheet better.

WickedShell · 2017-07-03T19:29:24Z

libraries/AP_GPS/AP_GPS_SBP.cpp

@@ -289,6 +289,7 @@ AP_GPS_SBP::_attempt_state_update()

        last_full_update_tow = last_vel_ned.tow;
        last_full_update_cpu_ms = now;
+        state.rtk_age_cs       = (last_full_update_cpu_ms - last_injected_data_ms) / 10;


I don't think this is really a fair metric. It might be sorta correct, but provides no guarantees that the needed data was injected, or that the data you have scheduled for injection has been injected yet (or processed).

Better than nothing I think.

WickedShell · 2017-07-03T19:30:50Z

libraries/AP_GPS/AP_GPS_NOVA.h

-        float latsdev;
-        float lngsdev;
-        float hgtsdev;
+        uint32_t solstat;      //!< Solution status


I have to ask whats the meaning you use in comments? I see things that are // ///< and //!< and it just looks really inconsistent.

///< and //!< are both accepted doxygen markups.

WickedShell · 2017-07-03T19:32:18Z

libraries/AP_GPS/AP_GPS.h

@@ -398,7 +418,7 @@ class AP_GPS

 private:
    // returns the desired gps update rate in milliseconds
-    // this does not provide any gurantee that the GPS is updating at the requested
+    // this does not provide any guarantee that the GPS is updating at the requested


My ongoing bad spelling continues!

WickedShell · 2017-07-03T19:35:55Z

libraries/AP_GPS/AP_GPS_NOVA.cpp

@@ -202,10 +202,12 @@ AP_GPS_NOVA::process_message(void)

        state.num_sats = bestposu.svsused;

-        state.horizontal_accuracy = (float) ((bestposu.latsdev + bestposu.lngsdev)/2);
-        state.vertical_accuracy = (float) bestposu.hgtsdev;
+        state.horizontal_accuracy = (float) ((bestposu.latsdev + bestposu.lngsdev)/2); // RMS


I'm a bit confused, you have already labeled the horizontal_accuracy storage location RMS in it's comments, you updated all the comments for the messages to say the accuracy in millimeters, but omitted the RMS tag there, but then applied it here. This should really just be documented in the header, rather then in line in the code randomly like this, and particularly in the structs that are used to decode the messages.

WickedShell · 2017-07-03T19:46:14Z

libraries/AP_GPS/AP_GPS_ERB.cpp

@@ -159,8 +159,8 @@ AP_GPS_ERB::_parse_gps(void)
        state.location.alt    = (int32_t)(_buffer.pos.altitude_msl * 100);
        state.status          = next_fix;
        _new_position = true;
-        state.horizontal_accuracy = _buffer.pos.horizontal_accuracy * 1.0e-3f;
-        state.vertical_accuracy = _buffer.pos.vertical_accuracy * 1.0e-3f;
+        state.horizontal_accuracy = _buffer.pos.horizontal_accuracy * 1.0e-3f; // RMS


I thought I had seen an e-mail about this, but now I can't find the comment anywhere, but I don't see anything in the ERB sheet that indicates characterizing this as RMS is correct. (And I admit I hesitated before peaking at RTKlib :) a quick google search filed to yield an anwser)

I looked at the RTKLIB code and even patched it. It's RMS

Take a look at emlid/RTKLIB#28

amilcarlucas · 2017-07-03T20:14:45Z

I think I have addressed your comments.

amilcarlucas · 2017-07-04T07:59:38Z

I think I have addressed all your comments, can this one go in ?

amilcarlucas · 2017-07-05T16:26:55Z

I cleaned this one up, even more.

amilcarlucas · 2017-07-11T19:30:15Z

@rrr6399 This is the first one that needs to go in. And it is IMHO the one that is less controversial.

…ion age" information in the GPS2_RAW MAVLink message

rrr6399 · 2017-07-17T20:36:01Z

@WickedShell , I just wanted to say that @amilcarlucas improvements for RTK support are very important. There is currently no way of retrieving the calculated offsets from the rover to base without the GPS_RTK message (especially for the moving base problem). This message is important not only for better diagnostics but also for more accurate "follow me" mode navigation. Please merge these changes soon since the master branch is going through many large changes right now.

WickedShell · 2017-07-18T01:22:46Z

libraries/AP_GPS/AP_GPS_SBP.cpp

@@ -289,6 +289,7 @@ AP_GPS_SBP::_attempt_state_update()

        last_full_update_tow = last_vel_ned.tow;
        last_full_update_cpu_ms = now;
+        state.rtk_age_ms        = last_full_update_cpu_ms - last_injected_data_ms;


Pending dropping this I'm fine with this.

amilcarlucas · 2017-07-18T01:28:37Z

libraries/AP_GPS/AP_GPS_SBP.cpp

@@ -289,6 +289,7 @@ AP_GPS_SBP::_attempt_state_update()

        last_full_update_tow = last_vel_ned.tow;
        last_full_update_cpu_ms = now;
+        state.rtk_age_ms        = last_full_update_cpu_ms - last_injected_data_ms;


@WickedShell I did not get what you meant with https://github.com/amilcarlucas/ardupilot/blob/7af8de19a0d93030e3926695346f85d40bf275e6/libraries/AP_GPS/AP_GPS_SBP.cpp#L405

I'm using 'last_injected_data_ms' here already

…es, and the age of the RTK correction messages

amilcarlucas added the GPS label Jul 3, 2017

amilcarlucas requested review from meee1, WickedShell and OXINARF July 3, 2017 18:01

amilcarlucas mentioned this pull request Jul 3, 2017

Publish SBP number of integer ambiguity hypotheses in GPS_RTK and GPS2_RTK mavlink messages #6424

Closed

WickedShell reviewed Jul 3, 2017

View reviewed changes

amilcarlucas force-pushed the pr-improve-GPS2_RAW-mavlink-message branch from b4e5556 to 7c7cfed Compare July 3, 2017 20:13

amilcarlucas force-pushed the pr-improve-GPS2_RAW-mavlink-message branch from 7c7cfed to 93538d8 Compare July 3, 2017 20:27

amilcarlucas force-pushed the pr-improve-GPS2_RAW-mavlink-message branch from 0e6fcd4 to fe82c2e Compare July 5, 2017 16:20

amilcarlucas mentioned this pull request Jul 5, 2017

Add GPS_RTK and GPS2_RTK MAVLink messages support in Emlid reach #6541

Closed

amilcarlucas added 2 commits July 11, 2017 21:42

AP_GPS: Improve comments and documentation (NFC)

3ef073c

AP_GPS: implement missing "RTK number of satellites" and "RTK correct…

c27e2b9

…ion age" information in the GPS2_RAW MAVLink message

amilcarlucas force-pushed the pr-improve-GPS2_RAW-mavlink-message branch from fe82c2e to 7af8de1 Compare July 11, 2017 19:42

WickedShell reviewed Jul 18, 2017

View reviewed changes

amilcarlucas commented Jul 18, 2017

View reviewed changes

AP_GPS: Publish the number of satellites in the RTK correction messag…

c69dc1a

…es, and the age of the RTK correction messages

amilcarlucas force-pushed the pr-improve-GPS2_RAW-mavlink-message branch from 7af8de1 to c69dc1a Compare July 18, 2017 01:36

rmackay9 merged commit b791fef into ArduPilot:master Jul 19, 2017

amilcarlucas deleted the pr-improve-GPS2_RAW-mavlink-message branch July 19, 2017 11:48

OXINARF mentioned this pull request Oct 24, 2017

Emlid Reach FW >= 2.7.4 outputs GPS2 _RTK mavlink message #6858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve GPS2_RAW mavlink message #6534

improve GPS2_RAW mavlink message #6534

amilcarlucas commented Jul 3, 2017 •

edited

Loading

WickedShell left a comment

WickedShell Jul 3, 2017

WickedShell Jul 3, 2017

amilcarlucas Jul 3, 2017

WickedShell Jul 3, 2017

amilcarlucas Jul 3, 2017 •

edited

Loading

WickedShell Jul 3, 2017

WickedShell Jul 3, 2017

amilcarlucas Jul 3, 2017

WickedShell Jul 3, 2017

amilcarlucas Jul 3, 2017

amilcarlucas Jul 3, 2017

amilcarlucas commented Jul 3, 2017

amilcarlucas commented Jul 4, 2017

amilcarlucas commented Jul 5, 2017

amilcarlucas commented Jul 11, 2017

rrr6399 commented Jul 17, 2017 •

edited

Loading

WickedShell Jul 18, 2017

amilcarlucas Jul 18, 2017

amilcarlucas Jul 18, 2017

improve GPS2_RAW mavlink message #6534

improve GPS2_RAW mavlink message #6534

Conversation

amilcarlucas commented Jul 3, 2017 • edited Loading

WickedShell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amilcarlucas Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amilcarlucas commented Jul 3, 2017

amilcarlucas commented Jul 4, 2017

amilcarlucas commented Jul 5, 2017

amilcarlucas commented Jul 11, 2017

rrr6399 commented Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amilcarlucas commented Jul 3, 2017 •

edited

Loading

amilcarlucas Jul 3, 2017 •

edited

Loading

rrr6399 commented Jul 17, 2017 •

edited

Loading