mavlink / qgroundcontrol

Cross-platform ground control station for drones (Android, iOS, Mac OS, Linux, Windows)
http://qgroundcontrol.io
3.19k stars 3.53k forks source link

Grid Survey `distanceToSurface` used as MAV_FRAME_GLOBAL_RELATIVE_ALT #8273

Open remster opened 4 years ago

remster commented 4 years ago

Expected Behavior

With the takeoff on a good hill and a grid survey below that hill; like so: hill

Current Behavior

QGC recalculates the survey transects based CameraCalc::distanceToSurface, which sounds correct because the closer the vehicle is to the surface the smaller bit of ground its field-of-view sees and the denser should the transects be. However, QGC defaults CameraCalc::distanceToSurface to the mission item altitude which (with default alt mode) is not distance to surface where the survey is but distance to surface at takeoff (which may be hundreds of metres below or above the survey). The consequence of this is that, as one moves the survey between hills and valleys, the transects do not change despite the distance to surface does change.

Importantly the .plan must separate MAV_CMD_NAV_WAYPOINT altitude from distanceToSurface as (for MAV_FRAME_GLOBAL_RELATIVE_ALT) they are two different values:

"TransectStyleComplexItem": {
    "CameraCalc": {
        "AdjustedFootprintFrontal": 25,
        "AdjustedFootprintSide": 25,
        "CameraName": "Manual (no camera specs)",
        "DistanceToSurface": 50, <<-- distance to surface at survey centroid
        "DistanceToSurfaceRelative": true,
        "version": 1
    },
    "CameraShots": 98,
    "CameraTriggerInTurnAround": true,
    "FollowTerrain": false,
    "HoverAndCapture": false,
    "Items": [
        {
            "autoContinue": true,
            "command": 16,
            "doJumpId": 3,
            "frame": 3,
            "params": [
                0,
                0,
                0,
                null,
                35.78288117563359,
                -121.33179433564969,
                50 <<-- distance to surface at mission takeoff
            ],
            "type": "SimpleItem"
        },

System Information

When posting bug reports, include the following information

DonLakeFlyer commented 4 years ago

A "Relative altitude" is defined as relative to the altitude of the home position. That concept cannot be changed. It is how all mission items work.

If you don't want to use a relative altitude you currently have two options:

  • it should be possible to set the rel altitude of that survey (as it is possible for a waypoint) to a negative value - and fly below the takeoff point (that's high on a hill);

You should be able to override the altitude value to a negative value using "Force Save". That said, I'm not sure if anyone has ever tried that though. I give it a try to see what happens.

What you describe would be a new feature with respect to specifying a centroid/terrain based altitude for the survey. Somewhat useful but I'm not sure if adding yet another option to Survey is a good idea. You can use workarounds above to get what you need even though they are not as simple. But given the possible limited usage of this new feature I think for now that's ok.

remster commented 4 years ago

Thanks for picking this up.

A "Relative altitude" is defined as relative to the altitude of the home position. That concept cannot be changed. It is how all mission items work.

If you don't want to use a relative altitude you currently have two options:

I don't necessarily want to - I am drawing your attention to the fact that QGC uses distanceToSurface as both:

Those are very different numbers and I argue they need separating in the code. I suggest that TransectStyleComplexItem grows a property called altitude that is interpreted similarly to SimpleMissionItem::altitude

I mention that it should be possible to set grid altitude to negative value not because i want to but to draw your attention to a symptom of bundling two distinct meanings of distanceToSurface. A survey needs to be above the surface but can be below the home position. In fact you can set a waypoint rel altitude to a negative value w/o any complains from QGC.

What you describe would be a new feature with respect to specifying a centroid/terrain based altitude for the survey.

I argue this feature already exist, but doesn't work correctly. QGC calculates the transects based on distanceToSurface - it uses this value as if it described the distance of the survey centroid to the ground directly below it.

imageDenisty = (distanceToSurface / sensorWidth * 100.0) / (imageWidth * focalLength);

As if it were AGL: IMG_20200207_165010

But this value isn't AGL, it is relative altitude above home position - and it practically can be negative (because home position can be on a hill) and when it is - things (used to) crash. And it isn't negative, the resulting transects will still be:

DonLakeFlyer commented 4 years ago

Ah, thanks. I get what you are saying now. You are correct the current survey implementation (and corridor scan as well) expects a fairly flat area that you are flying to/over/from. Which as you say means negative will not work to support a flight like you image. The only way to do that is to do a terrain following survey.

Not quite sure how to handle a non-flat in a simple way though without adding additional complexity.

@antiheavy FYI Have you ever seen anything handle this sort of thing in a rational way?

Antiheavy commented 4 years ago

@Antiheavy FYI Have you ever seen anything handle this sort of thing in a rational way?

this is a complex topic and I've seen it argued and debated all sort of ways on different projects. It sort of depends on if your system fundamentally thinks in terms of MSL (or amsl) vs fundamentally thinks in terms of altitude relative to home. There are many pros and cons to each approach. QGC sort of mixes both approaches and adds extra complexity. One way I've seen this be a little simpler is to only allow one type of altitude for an entire mission. The whole thing should be Rel, AGL, or MSL otherwise it's extra confusing.

QGC should recalculate the transects based on distance to surface at the survey's centroid;

@remster is the issue here that QGC doesn't allow you to select an altitude datum for Surveys like it does for regular waypoints? I'm not totally following all your points.

DonLakeFlyer commented 4 years ago

The issue is that if you are specifying a survey using GSD where the transects and camera triggering are calced based on GSD. This calcuation is done based on the distance to the surface it is surveying. The survey surface is assumed to be a flat plane at the altitude of the home position. Due to that if the survey area height is above or below the home altitude (as shown in the graphic) it won't work calc the values correctly.

remster commented 4 years ago

@Antiheavy

@remster is the issue here that QGC doesn't allow you to select an altitude datum for Surveys like it does for regular waypoints? I'm not totally following all your points.

No. that's not the issue. In fact, we're locked for some reason to relative alt no matter what we put here - but that's unrelated.

@DonLakeFlyer gave it a pretty good summary over my over-wordiness.

I think, similarly toSimpleMissionItem, TransectStyleComplexItem should accept altitude and altiudeMode as parameters (although it does escape me why would one want to mix altitude modes in a single mission) and it should probably calculate distanceToSurface (mission items seem to have unrestricted access to the terrain model).