Skip to content

TERRAIN Protocol Integration#2137

Open
zekesarosi wants to merge 7 commits intomavlink:ros2from
zekesarosi:ros2
Open

TERRAIN Protocol Integration#2137
zekesarosi wants to merge 7 commits intomavlink:ros2from
zekesarosi:ros2

Conversation

@zekesarosi
Copy link
Contributor

The Terrain Server plugin implements the companion-computer side of the MAVLink Terrain Protocol, allowing a ROS 2 system to supply real-world elevation data to a flight controller over MAVLink. When an autopilot like ArduPilot needs to know the ground height beneath it — for terrain-following flight, safe altitude planning, or landing — it sends a TERRAIN_REQUEST message asking for elevation data over a grid of coordinates. This plugin catches those requests and responds with the corresponding altitude values, sourced from NASA's Shuttle Radar Topography Mission (SRTM) dataset.

If a tile isn't available locally, the plugin can automatically download it. When auto-download is enabled, background worker threads fetch the missing .hgt.zip from ArduPilot's terrain server (or a custom host), extract it, and slot it into the cache — all without blocking the main request-serving loop. A continent lookup table avoids wasting time on ocean tiles that don't exist.

@vooon
Copy link
Member

vooon commented Mar 9, 2026

The problem i see it conflicts with existing plugin, also there requirement to pull curl into main process.
Maybe better to use another approach, where plugin translates messages, but tile server is a separate node?
That way it can be written e.g. in python, so much simpler network code.

@zekesarosi
Copy link
Contributor Author

yes thanks for the feedback, will change up the framework

@zekesarosi zekesarosi marked this pull request as ready for review March 9, 2026 22:14
@vooon
Copy link
Member

vooon commented Mar 10, 2026

I'd done more precise review together with Copilot and also suggest how to improve architecture:

Bugs / blockers:

  • Thread safety on pending_requests_: send_next_tile() reads the
    vector under lock, releases, does work, then re-locks using the saved
    index. Between the two locks handle_terrain_request can push_back
    (reallocating the vector), making the index stale. This is a TOCTOU race.
  • Missing curl timeouts: If a download hangs, the destructor's join()
    blocks indefinitely. Need CURLOPT_TIMEOUT / CURLOPT_CONNECTTIMEOUT.
  • package.xml: <depend>libcurl4-openssl-dev</depend> installs dev
    headers at runtime. Should be
    <build_depend>libcurl4-openssl-dev</build_depend> +
    <exec_depend>libcurl4</exec_depend>.

Integration items:

  • Per-tile RCLCPP_INFO on every load/completion will be noisy in
    production — consider RCLCPP_DEBUG for those, keep INFO for summary
    events.
  • Missing pluginlist entries in apm_pluginlists.yaml /
    px4_pluginlists.yaml (blocklist by default so it doesn't activate
    unexpectedly).
  • Missing parameter documentation in config yamls.

Architecture suggestion:

The plugin is doing a lot — SRTM parsing, LRU caching, bilinear
interpolation, multi-threaded curl downloads, geodesic math, MAVLink
handling, and ROS publishing — all in one ~1000-line class. I'd suggest
splitting it into three layers:

  1. SRTM tile library (mavros_extras/src/lib/srtm_tile.cpp + header)
    .hgt loading, endian swap, interpolation, LRU cache, continent
    lookup. Pure C++, no ROS/MAVLink/curl dependency. This is the part that
    really needs unit tests and is easy to test in isolation with a small
    synthetic .hgt fixture.

  2. Terrain server node (separate executable or composable node, could
    even be Python) — owns the download workers, cache directory, curl
    logic. Uses the SRTM library for lookups. Exposes a ROS service for
    elevation queries. All threading/download complexity is contained here,
    and the curl dependency stays on this target only rather than pulling
    into the entire mavros_extras_plugins library.

  3. Plugin wiring — adjust the existing terrain plugin to also handle
    TERRAIN_REQUEST / TERRAIN_CHECK by calling the server node's ROS
    service and sending TERRAIN_DATA back. Stays thin and consistent with
    other mavros plugins. No file I/O, no threads, just message plumbing.

This keeps the SRTM code testable, makes the server independently reusable
(e.g. for terrain-aware mission planning), avoids adding curl as a
dependency for all mavros_extras users, and resolves the ~/report topic
overlap with the existing terrain plugin naturally.

@zekesarosi
Copy link
Contributor Author

did you review after I refactored to your original suggestions?
I will update again but this current implementation should be much cleaner with the separate python node that handles all the messy network calls and then the unified terrain plugin that bridges with the mavlink connection

@vooon
Copy link
Member

vooon commented Mar 11, 2026

@zekesarosi not yet. Not expected that fast...
Right now just some organisational:

  • Need to add python setup.py/cfg and setup like in main mavros (for py-lib)
  • Place generator into root /tools/
  • Make an entry-point so it could be ros2 run ... terrain_tile_server
  • Please fix code style (see the CI failures, basically you can use Ruff with config from mavros)
  • Please make some unit tests inside extras/tests/.

request_pub_ = node->create_publisher<mavros_msgs::msg::TerrainRequest>(
"~/request", 10);

data_sub_ = node->create_subscription<mavros_msgs::msg::TerrainData>(
Copy link
Member

Choose a reason for hiding this comment

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

New docs extractor gets the descrption from the comment above node->create_*(),
So please move it here.


_MAP: bytes = zlib.decompress(base64.b64decode(
{data_literal}
))
Copy link
Member

Choose a reason for hiding this comment

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

Where are that compress/b64 came from? It doesn't looks like we really needs that, but makes check harder.

int32 lon # Longitude degE7
uint16 grid_spacing # Grid spacing in meters
uint8 gridbit # Index of the 4x4 block inside the grid
int16[16] data # Terrain elevation values in meters
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit unsure about almost raw mavlink message repr, e.g. int32 lat/lon when in most of other places we use GeoPoint/float64.

@zekesarosi
Copy link
Contributor Author

thank you again for the feedback. Let me know again about this revision.

'ce905e5ba3834408946be595611a48e879595323abe8ea65f0148d01efe56a22e5dc19970f8381'
'8135078a9ec89cde271d6b2ad107e7b86d9aa549d2ff002e1e877d'
)
)
Copy link
Member

Choose a reason for hiding this comment

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

About base64 - i mean, do we really need to store that as a compressed blob?


from mavros_extras.terrain_server_node import main

main()
Copy link
Member

Choose a reason for hiding this comment

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

Just thought - maybe good to have some script to preload tiles?
I.e. give some rectangle (two coordinates pair) and script will download absent data.
Or maybe like a cache management commands, like preload / update (check and load changed data) / validate (i.e. ready for offline work).

Also regarding loading of the data, good to support http(s) proxy.
Is there a checksum files available on the source? If so - possible to get those files via https and tiles via http, so proxy like a Squid can cache the data in your local site.
If not, well, there nginx, but setup a little bit harder.

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.

2 participants