mavlink / qgroundcontrol

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

Terrain upload problems #8946

Closed bkueng closed 1 year ago

bkueng commented 4 years ago

Implementing terrain upload for PX4, I came across 2 problems in QGC:

Slow upload

The upload is extremely slow, it took about 30s for 20kb of data in SITL (!). Just removing the line https://github.com/mavlink/qgroundcontrol/blob/master/src/Vehicle/TerrainProtocolHandler.cc#L94 brings this down to a few seconds. I'm aware we should not just do that, but also check for link congestion.

Lookup corner case

During upload I saw this:

TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates hash:coordinate "06905128390001885200013738001" QGeoCoordinate(47.38, 8.52)
TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates Internal Error: coordinate not in tile region

And another one:

TerrainProtocolHandlerLog: _sendTerrainData TerrainAtCoordinateQuery::getAltitudesForCoordinates failed
TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates hash:coordinate "06905128390001885000013738001" QGeoCoordinate(47.38, 8.5)
TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates Internal Error: coordinate not in tile region
TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates hash:coordinate "06905128390001885000013737001" QGeoCoordinate(47.379999998, 8.5006640651)
TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates returning elevation from tile cache 411

This looks like a corner case in the lookup: for example from AirmapElevationProvider::lat2tileY(): (47.38+90)/srtm1TileSize =(47.38+90)/0.01 = 13738, which is an int, so at the border of a tile. Options I see to resolve that:

@DonLakeFlyer can you look into that? Or do you want me to?

DonLakeFlyer commented 4 years ago

Maybe @dogmaphobic will know about the tile has problem. I'm not that familiar with the code. If he doesn't know anything off the top of his head I can look.

DonLakeFlyer commented 4 years ago

I'll look at the perf thing.

DonLakeFlyer commented 4 years ago

Gus doesn't know much about the tile hash stuff either. I'll look at that as well.

bkueng commented 4 years ago

Cool, thanks

DonLakeFlyer commented 4 years ago

@bkueng Is there firmware side code that handles link congestion somewhere? Looking for examples of how to deal with that.

bkueng commented 4 years ago

Yes for example here: https://github.com/PX4/Firmware/blob/master/src/modules/mavlink/mavlink_parameters.cpp#L293 With the get_free_tx_buf definition: https://github.com/PX4/Firmware/blob/master/src/modules/mavlink/mavlink_main.cpp#L737:L778 This is mainly for NuttX though.

moreba1 commented 2 years ago

loading terrain data take many time . Can we download terrain altitude from other sources like ArcGIS server ? It is more accurate altitude and maybe it was faster.

jordancoult commented 1 year ago

In my branched repo, I also ran into TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates Internal Error: coordinate not in tile region which I confirmed was incorrect for the coordinate in question. I fixed it by adding a small epsilon to the comparisons in TerrainTile::isIn, like so:

bool TerrainTile::isIn(const QGeoCoordinate& coordinate) const {
    if (!_isValid) {
        qCWarning(TerrainTileLog) << "isIn requested, but tile not valid";
        return false;
    }

    const double epsilon = 1e-10;
    bool ret = (coordinate.latitude() - _southWest.latitude() >= -epsilon) &&
               (coordinate.longitude() - _southWest.longitude() >= -epsilon) &&
               (_northEast.latitude() - coordinate.latitude() >= -epsilon) &&
               (_northEast.longitude() - coordinate.longitude() >= -epsilon);

    qCDebug(TerrainTileLog) << "Checking isIn: " << coordinate << " , in sw " << _southWest
                            << " , ne " << _northEast << ": " << ret;
    return ret;
}

However, this function no longer exists in TerrainTile so the issue is probably obsolete.