Surface tracking rangefinder bug

[Georacer]

Bug Report

Bug report

The surface tracking code is using the rangefinder to get a sense of the ground distance.
There are cases where a glitch is misdetected, leading to undesirable altitude change.

Issue details

AP_SurfaceDistance.cpp relies on RANGEFINDER_GLITCH_ALT_M to decide if two consecutive measurements are too far apart, hence classified as a glitch.

Later, in surface_tracking.cpp (link), the altitude target is reset.

But if the distance step is legitimate (e.g. fly over a vertical wall), there will be no change in altitude initially (undesired). Then, if you happen to go back over the step (without an glitch detected), you might actually descend too low or ascend too high.

It would be nice to have a parameter disabling the glitch detection at least. I don't think it's an easy problem to improve reliably.

Version
master

Platform
[ ] All
[ ] AntennaTracker
[X] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

Airframe type
N/A.

Hardware type
N/A

Logs
Not publicly available, but upon request.

[rmackay9]

I think a SURFTRAK_OPTION to turn on/off the glitch protection or adding a SURFTRAK_GLITCH_M param would be OK

Thanks for bringing this up

[IamPete1]

We could add the glitch distance as a param such that a value of 0 would disable.

Going forward I would like to get the surface tracking lib to use all available sensors. This would allow two sensors to be compared for example.

For glitch detection we could use a Hampel filter. I was also thinking of using something like that to try and improve RPM measurements.

[Georacer]

I spent the last hour trying to transplant the SURFTRAK_TC parameter inside AP_SurfaceDistance and create another parameter for the glitch threshold. I'm tripping up on how to make them common for both rangefinder_state and rangefinder_up_state.

I'll keep trying tomorrow.

[timtuxworth]

This is definitely a big deal, I've worked with partners struggling with this. A filter probably makes a lot of sense.

[IamPete1]

I spent the last hour trying to transplant the SURFTRAK_TC parameter inside AP_SurfaceDistance and create another parameter for the glitch threshold. I'm tripping up on how to make them common for both rangefinder_state and rangefinder_up_state.

I'll keep trying tomorrow.

You should be able to pass it as a const refence in the constructor, there is a example for the log rate limiter here:

ardupilot/libraries/AP_Logger/AP_Logger_Backend.h

Line 28 in 958493b

const AP_Float &rate_limit_hz;

[Georacer]

I thought it would be nice to finally encapsulate SURFTRAK_TC and the new parameter in AP_SurfaceDistasnce. Right now the related functionality is a bit scattered.

But I'll take your suggestion if it proves too hard, thanks!

[Georacer]

I spent the last hour trying to transplant the SURFTRAK_TC parameter inside AP_SurfaceDistance and create another parameter for the glitch threshold. I'm tripping up on how to make them common for both rangefinder_state and rangefinder_up_state.
I'll keep trying tomorrow.

You should be able to pass it as a const refence in the constructor, there is a example for the log rate limiter here:

ardupilot/libraries/AP_Logger/AP_Logger_Backend.h

Line 28 in 958493b
const AP_Float &rate_limit_hz;

@IamPete1 does this pattern allow live update of the parameter thanks to passing a reference? Or am I reading this wrong?