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

Fixed GPS timestamping and EKF time horizon issues #5060

Closed
wants to merge 9 commits into from

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Oct 25, 2016

This fixes a problem with timing jitter on UARTs leading to higher EKF position innovations. It does 3 things:

  • adds simulation of UART timing jitter in SITL with SIM_GPS_JITTER
  • makes the fix time from AP_GPS automatically correct for UART timing jitter
  • fixes the EKF to propogate GPS data to the correct time horizon to remove IMU/GPS timing jitter

This work was done after 70ms timing jitter was seen in flight logs for a Disco, but it applies (to a smaller degree) to all systems.

@tridge tridge added this to the Plane v3.8.0 milestone Oct 25, 2016
Copy link
Contributor

@WickedShell WickedShell left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the SITL part yet, or actually flashed it yet, still has a header problem.

@@ -23,6 +23,7 @@
#include <AP_HAL/Util.h>
#include <DataFlash/DataFlash.h>
#include <GCS_MAVLink/GCS.h>
#include <stdio.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we used to avoid adding stdio.h as it broke on AVR, but now it is fine on all platforms, and it makes adding debug code easier

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly pointed it out, as I don't actually see any new code that uses anything from it, not that worried either way though.

*/
void AP_GPS::estimate_fix_time(uint8_t instance, uint32_t now)
{
const uint64_t ms_per_week = 604800000ULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to just see this as the math to get the ms_per_week just for auditing reasons, unless someone objects to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you mean by that, sorry

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I'd rather see 7 * 24 * 60 * 60 * 1000 for magic constants like this as it makes it a bit more apparent where the number came from in general (this one is fairly self explanatory and is obviously correct)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is now AP_MSEC_PER_WEEK in definitions.h

void NavEKF2_core::propogateGpsData(gps_elements &gpsData, uint32_t new_time_ms)
{
float dtime = (new_time_ms - gpsData.time_ms) * 1.0e-3f;
gpsData.pos.x += dtime * gpsData.vel.x;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the EKF already accounting for this for you when it rewinds the data? As I understand it this should only be needed if the GPS data is more delayed then the IMU history. (Highly possible I'm missing something about the EKF here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the EKF wasn't coping with it fully. I spent a lot of time discussing this with Paul.

Copy link
Contributor

Choose a reason for hiding this comment

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

The EKF aligns the data to within the granularity of the prediction/update step which is every 10msec for copter and every 20msec for plane. This still leaves some residual jitter which this change compensates for.

@magicrub
Copy link
Contributor

Ahh, didn't notice that was a SITL file.

On Oct 25, 2016 3:56 PM, "Andrew Tridgell" notifications@github.com wrote:

@tridge commented on this pull request.

In libraries/AP_HAL_SITL/UARTDriver.h
#5060:

@@ -75,6 +75,12 @@ class HALSITL::UARTDriver : public AP_HAL::UARTDriver {
const char *_uart_path;
uint32_t _uart_baudrate;

  • struct {
  •    bool enabled;
    
  •    uint64_t delay_till_us;
    
  •    uint16_t promised;
    
  • } _jitter;

in SITL all vars are aligned and packing is really not relevant


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#5060, or mute the thread
https://github.com/notifications/unsubscribe-auth/AEj7GzdPBfeFn8jFTBdpvXXKiQ1t9Qa6ks5q3okwgaJpZM4KfkAJ
.

Copy link
Contributor

@priseborough priseborough left a comment

Choose a reason for hiding this comment

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

I have reviewed the AP_NavEKF2 changes and they look OK. If the GPS library changes have been reviewed and it has been flown on the Disco, then it should be good to merge.

@tridge
Copy link
Contributor Author

tridge commented Oct 26, 2016

thanks Paul, I haven't flown it yet, although Craig plans to fly it tomorrow. He's using a Trimble GPS and his logs were showing GPS timing jitter of around 50ms in the last flights.

@lvale
Copy link
Member

lvale commented Oct 29, 2016

@rmackay9 Do we want this on Copter 3.4.1 ??

@OXINARF
Copy link
Member

OXINARF commented Nov 5, 2016

It generally looks OK but I have four questions:

  • wouldn't the offset be better if it was an average? I mean, you can stay with a 4 second offset forever even if it only happened once
  • the use of last_message_time stays in the EKF code to check if GPS data is current or timed out, but isn't the use of last_fix_time more appropriate for this?
  • in readGpsData() the prevention for GPS time not being older than IMU was changed to only be done if origin is valid, is this correct? If it is then probably more code could be changed to be under this if
  • also if the previous point is correct, why not doing the propagate (isn't that spelled wrong in code?) just when doing the recall? When would the propagate in the GPS read be useful?

@amilcarlucas
Copy link
Contributor

Any progress on this ?

@magicrub
Copy link
Contributor

This needs a rebase

this ensures the time_week_ms stays in sync with the rest of the data
this allows us to simulate the jitter that happens on real UARTs,
which affects the GPS data

Settable in milliseconds using SIM_GPS_JITTER
this copes with large UART timing jitter without discarding samples by
forward propogating GPS position and height to the IMU delayed time
horizon.
this removes any remaining timing difference, which will be on the
order of 10 to 20ms (depending on loop rate). At typical fixed wing
flying speeds this removes about 20cm or so of position jitter, which
should be worthwhile for RTK based systems
not needed with new GPS jitter correction
@WickedShell WickedShell modified the milestones: Plane v3.9.0, Plane v3.8.0 Jun 19, 2017
@WickedShell
Copy link
Contributor

@tridge is this still on the radar/something you are looking to bring into master?

@karthikdesai
Copy link
Contributor

@tridge friendly ping

@amilcarlucas
Copy link
Contributor

This is an interesting PR, especially for people that are using MAVROS.
Can I help?

@tridge
Copy link
Contributor Author

tridge commented Jul 1, 2019

I have done some work on this last week, it is a bit complex, so not ready yet

@amilcarlucas
Copy link
Contributor

Needs a rebase. Whould be great having this.

@davidbuzz
Copy link
Collaborator

last update more than 2 years ago, last comment from @tridge was 'not ready yet', so i propose closing it.

@amilcarlucas
Copy link
Contributor

Would be great to have for precise navigation :)

@davidbuzz davidbuzz closed this Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants