GPS: Log and maintain the delta time between the last position update #6556

WickedShell · 2017-07-08T23:19:20Z

This logs the delta time between GPS position updates, and uses it as an arming check.

The arming check was selected to limit us to 5Hz updates as a minimum, @priseborough if this is felt to be to strict we can change it, but from memory this is what we were requiring.

The logging should be highlighted as a fixing a frequent problem I encounter with users logs when things start going wrong. When I'm trying to tell if the GPS driver made an error/the GPS is misconfigured at the time it's hard to tell if we lost GPS (and thus aren't getting any logged messages) or if we just had to drop some logging messages. With the delta time from the GPS lib, we can tell if the GPS actually experienced a miss, or if we just lost some messages.

This implements #5749, however @rmackay9 I don't know how to emit ERR messages, so I'm hoping you can either take it on later/separately or point me at how to do it.

WickedShell · 2017-07-08T23:25:42Z

A thought is that this is basically all the work required for is_healthy(), I could transform it to that, we can move the cutoff value to living in the GPS library, and then arming can just ask is_healthy() (which also exposes to the sensor status message better). The only downside is it would cost the Arming message the note that says exactly whats wrong (the update rate) and would just have to say "GPS unhealthy"

After a bit of reflection thats probably a better way to present this though, so if everyones happy with that I will rework it to be is_healthy instead? (Would also replace #6399)

OXINARF · 2017-07-09T18:18:37Z

I haven't looked at the PR changes, but I like the idea of is_healthy. I would love that some day the checks the EKF does on the GPS would be moved into the GPS library, which would be a complement to the health of the GPS.

The only downside is it would cost the Arming message the note that says exactly whats wrong (the update rate) and would just have to say "GPS unhealthy"

Well, we can make like we do with the EKF and ask the GPS library the message when it isn't healthy.

amilcarlucas · 2017-07-19T13:33:34Z

Can you please rebase ?

WickedShell · 2017-07-19T17:42:59Z

@amilcarlucas it's being converted to the is_healthy() approach first. I've actually done the conversion here, but I haven't gotten to meaningfully test it yet, which is why I haven't pushed it.

WickedShell · 2017-08-29T20:41:35Z

Rebased and force pushed. I had to recreate the is_healthy() stuff as I appear to have lost it in my git stash somewhere...

OXINARF

Looks generally OK, I've made some inline comments.

OXINARF · 2017-09-11T18:30:18Z

libraries/AP_GPS/AP_GPS.cpp

@@ -626,8 +626,10 @@ void AP_GPS::update_instance(uint8_t instance)
            state[instance].hdop = GPS_UNKNOWN_DOP;
            state[instance].vdop = GPS_UNKNOWN_DOP;
            timing[instance].last_message_time_ms = tnow;
+            timing[instance].delta_time_ms = UINT16_MAX; // we are going to lose lock on what this is until after we have parsed 2 messages


The comment sentence isn't making sense to me.

The first time we get a fix it's going to be an implausibly short. I will admit the comment is a bit confusing, but it is trying to hint that the first reading on this after getting a fix will be wrong.

I understand your point now, but, besides being hard to understand, I think this is the wrong place to put such a comment. What about putting it two lines down where the clamp is? That's where the small implausible value will actually happen, right?

By the way, I think we should initialize the delta_time_ms to the same value instead of only when the driver is deleted.

OXINARF · 2017-09-11T18:50:02Z

libraries/AP_GPS/AP_GPS.cpp

        }
    } else {
+        timing[instance].delta_time_ms = MIN(tnow - timing[instance].last_message_time_ms, UINT16_MAX); // clamp the initial reading


Should we clamp it to 4000? More than that and the driver should be deleted - it should help with logging, otherwise the first delta time will be UINT16_MAX and all others much smaller.

I'd rather have the very clear distinction between no data and data. Although you have a point that we could clamp to the timeout value and it would still be sane. Will update.

I think the way it is now is probably the best, but the change you made wasn't actually what I was proposing. I've seen now that I was wrong in my thinking though.

Let me explain: I thought that last_message_time_ms was zero before passing here a first time, so my thought was that, when there wasn't a driver, the delay value would be UINT16_MAX and, when there was, the first value would be 4000 (because tnow - last_message_time would be bigger in the first time).
I see now that last_message_time is actually set to now when we create the driver. So my question now is, why do we need the clamp? I don't see when tnow - last_message_time would ever be great than 4000.

If it takes more then 4 seconds to detect the GPS after it was lost (which is possible) it could come out to be a much larger value.

If it takes more than 4 seconds then the driver will be deleted. Which is why I can't see any real code path that makes the clamp needed. The only possible code path would be if after the driver creation we wouldn't call the GPS update method for more than 4 seconds - but I think that would be a problem, right?

OXINARF · 2017-09-11T19:03:01Z

libraries/AP_GPS/AP_GPS.h

@@ -34,6 +34,7 @@
 #define GPS_MAX_RATE_MS 200 // maximum value of rate_ms (i.e. slowest update rate) is 5hz or 200ms
 #define GPS_UNKNOWN_DOP UINT16_MAX // set unknown DOP's to maximum value, which is also correct for MAVLink
 #define GPS_WORST_LAG_SEC 0.22f // worst lag value any GPS driver is expected to return, expressed in seconds
+#define GPS_MAX_DELTA_MS 245 // 200 ms (5Hz) + 50 ms buffer


Comment doesn't match the value

Yeah, I didn't want an actual 250ms buffet as that would have accepted a 4Hz GPS. Will update the comment none the less.

OXINARF · 2017-09-11T19:08:17Z

libraries/AP_GPS/AP_GPS.h

@@ -388,6 +397,10 @@ class AP_GPS
    // indicate which bit in LOG_BITMASK indicates gps logging enabled
    void set_log_gps_bit(uint32_t bit) { _log_gps_bit = bit; }

+    // report if the gps is healthy (this is defined as having recieved an update at a rate greater then 4Hz)


recieved -> received
then -> than
🙂

WickedShell · 2017-09-11T20:03:59Z

@OXINARF Pushed updates.

OXINARF · 2017-09-11T22:07:09Z

libraries/AP_GPS/AP_GPS.h

@@ -34,6 +34,7 @@
 #define GPS_MAX_RATE_MS 200 // maximum value of rate_ms (i.e. slowest update rate) is 5hz or 200ms
 #define GPS_UNKNOWN_DOP UINT16_MAX // set unknown DOP's to maximum value, which is also correct for MAVLink
 #define GPS_WORST_LAG_SEC 0.22f // worst lag value any GPS driver is expected to return, expressed in seconds
+#define GPS_MAX_DELTA_MS 245 // 200 ms (5Hz) + 454545 ms buffer


Humm, this is interesting 😝

I cannot explain that... Must have hit the period key twice...

WickedShell · 2017-09-11T23:25:26Z

@OXINARF pushed, but I'm not sure I really addressed what you were looking for, touching this while listening to dev call + local conversation was probably a bad idea.

OXINARF · 2017-09-12T16:13:35Z

libraries/AP_GPS/AP_GPS.cpp

        }
    } else {
+        // the delta time will be lost until we have parsed two messages, clamp it to prevent excessive growth


I still don't like the message, let me try to give an alternative: delta will only be correct after parsing two messages or the first delta time won't match reality, only works after parsing two messages.

This is particularly useful for assessing if a GPS actually is dropping out in a users log, or if the log is just dropping messages

WickedShell · 2017-09-13T00:59:44Z

@OXINARF should be all addressed.

OXINARF · 2017-09-13T12:36:44Z

Merged, thanks!

WickedShell added Enhancement GPS labels Jul 8, 2017

WickedShell force-pushed the wickedshell/arming-gps-update-rate branch from d763677 to 67f6d31 Compare August 29, 2017 20:39

WickedShell force-pushed the wickedshell/arming-gps-update-rate branch from 67f6d31 to a83b51c Compare August 30, 2017 19:49

OXINARF reviewed Sep 11, 2017

View reviewed changes

WickedShell force-pushed the wickedshell/arming-gps-update-rate branch from a83b51c to 5b23aa6 Compare September 11, 2017 20:03

WickedShell force-pushed the wickedshell/arming-gps-update-rate branch from 5b23aa6 to b8d22cf Compare September 11, 2017 21:28

OXINARF reviewed Sep 11, 2017

View reviewed changes

WickedShell force-pushed the wickedshell/arming-gps-update-rate branch from b8d22cf to 0e4d6d5 Compare September 11, 2017 23:24

OXINARF reviewed Sep 12, 2017

View reviewed changes

WickedShell added 4 commits September 12, 2017 12:17

AP_GPS: Track the delta time between the last GPS positions

34c5f4c

Dataflash: Log GPS delta times

44d7d36

This is particularly useful for assessing if a GPS actually is dropping out in a users log, or if the log is just dropping messages

AP_Arming: Check GPS update rate as part of the GPS checks

45ba3ad

AP_Arming: Collapse GPS checks into the same branch

25b7322

WickedShell force-pushed the wickedshell/arming-gps-update-rate branch from 0e4d6d5 to 25b7322 Compare September 12, 2017 19:24

OXINARF merged commit bde1b6e into ArduPilot:master Sep 13, 2017

This was referenced Sep 15, 2017

is_healthy AP_GPS methods #6399

Closed

AP_GPS: Log delta time between updates #5749

Closed

Copter: pre-arm check of GPS update rate #1570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPS: Log and maintain the delta time between the last position update #6556

GPS: Log and maintain the delta time between the last position update #6556

WickedShell commented Jul 8, 2017

WickedShell commented Jul 8, 2017

OXINARF commented Jul 9, 2017 •

edited

Loading

amilcarlucas commented Jul 19, 2017

WickedShell commented Jul 19, 2017

WickedShell commented Aug 29, 2017

OXINARF left a comment

OXINARF Sep 11, 2017

WickedShell Sep 11, 2017

OXINARF Sep 11, 2017

OXINARF Sep 11, 2017

WickedShell Sep 11, 2017

OXINARF Sep 11, 2017

WickedShell Sep 11, 2017

OXINARF Sep 12, 2017

OXINARF Sep 11, 2017

WickedShell Sep 11, 2017

OXINARF Sep 11, 2017

WickedShell commented Sep 11, 2017

OXINARF Sep 11, 2017

WickedShell Sep 11, 2017

WickedShell commented Sep 11, 2017

OXINARF Sep 12, 2017

WickedShell commented Sep 13, 2017

OXINARF commented Sep 13, 2017

GPS: Log and maintain the delta time between the last position update #6556

GPS: Log and maintain the delta time between the last position update #6556

Conversation

WickedShell commented Jul 8, 2017

WickedShell commented Jul 8, 2017

OXINARF commented Jul 9, 2017 • edited Loading

amilcarlucas commented Jul 19, 2017

WickedShell commented Jul 19, 2017

WickedShell commented Aug 29, 2017

OXINARF 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

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

WickedShell commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WickedShell commented Sep 11, 2017

Choose a reason for hiding this comment

WickedShell commented Sep 13, 2017

OXINARF commented Sep 13, 2017

OXINARF commented Jul 9, 2017 •

edited

Loading