Skip to content

SITL support for UDP connected network peripherals - #34159

Open
timtuxworth wants to merge 5 commits into
ArduPilot:masterfrom
timtuxworth:pr-sitl-net-udp
Open

SITL support for UDP connected network peripherals#34159
timtuxworth wants to merge 5 commits into
ArduPilot:masterfrom
timtuxworth:pr-sitl-net-udp

Conversation

@timtuxworth

@timtuxworth timtuxworth commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds UDP support for simulated network-attached devices (--net-device), alongside the existing TCP-only path.

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

Built SITL (./waf copter) with these two commits in isolation and confirmed the existing MountTopotekNetwork autotest (TCP path, unaffected by this change) still passes.

Description

SerialDevice::listen_on_tcp_port() was the only way to attach a simulated network-attached device (--net-device NAME:PORT, used for devices reached over a NET_Pn port rather than a serial port). Some real hardware being simulated is UDP-only, so this adds:

  • SerialDevice::listen_on_udp_port() and a network_update_udp() counterpart to network_update(). Unlike TCP there's no connection to accept — the autopilot's address is learned from whichever datagram it last sent, and replies go back to that address.
  • create_net_serial_sim() now accepts an optional protocol after the port, e.g. topotek:15005,udp (TCP remains the default with no suffix). The spec format changes from NAME:PORT to NAME:PORT[,PROTOCOL] — the protocol is comma-grouped with the port rather than colon-chained, since it (and any future per-port option, e.g. a simulated firmware version) is logically attached to the port, not another top-level field like NAME.
  • an autotest for Topotek gimbals over UDP

Split out of #34155 (a new SkyDroid gimbal driver, whose real hardware is UDP-only) at Peter Barker's suggestion that this capability doesn't depend on anything SkyDroid-specific and is useful on its own. That PR will rebase on top of this once merged.


This PR was developed with AI assistance (Claude). All changes have been reviewed and tested by me as part of #34155 .

@tridge

tridge commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/AIReview/devcall_pr_reviews.html#pr34159

Re-reviewed at head 0ef30f5d6f: all three substantive findings from my previous review are resolved, and the UDP path is now exercised end-to-end in CI (sitltest-copter-tests2b runs MountTopotekNetworkUDP and passed). Verdict: COMMENT — mergeable after cosmetics.

  • UDP >300-byte truncation — resolved (SIM_SerialDevice.cpp:287 caps at MIN(sizeof(buffer), 300) with a comment explaining the coupling; MIN(300U, space) re-verified on master's AP_Networking_port.cpp:332).
  • "is dropped" comment — resolved (now accurately says pre-peer data is left unread, nothing lost).
  • Stale "via TCP" sim_update() comment — resolved.
  • Stale PR body — still open: it still says "No new autotest is added here" and "these two commits"; the PR has 5 commits including the new autotest.

Remaining, none blocking:

  1. SIM_SerialDevice.cpp:181 — SO_REUSEADDR on the UDP listen socket lets a second SITL instance bind the same port "successfully" with datagrams silently going to one socket, instead of a clean EADDRINUSE panic. Note dropping the explicit reuseaddress() call isn't enough — SocketAPM::bind() itself calls it unconditionally (Socket.cpp:250) — so this needs a bind-API variant or a deliberate decision to accept it.
  2. SITL_State_common.h:100 — the declaration comment still documents TCP-only NAME:TCPPORT; the spec is now NAME:PORT[,PROTOCOL] (and, trivially, a third comma token like name:port,udp,garbage is silently accepted).
  3. Commit tidy-up before merge: 49a9971 and 0ef30f5 are amendments to earlier commits in this PR; squashing them into their base SITL / AP_HAL_SITL commits would keep the clean 3-commit structure (preference, not a gate — convention CI passes as-is).
  4. Coordination: Skydroid driver for C11 and C13 gimbals #34155 still carries an older copy of this transport without the 300-byte cap — whichever lands first, the other needs a rebase.

The stm32h7 build failure is an unrelated Micro-XRCE-DDS-Gen install flake (fails in Gradle before any compilation) — worth a re-run for a fully green board. Codex cross-checked: 6 confirmed, 1 adjusted (the bind() point in item 1), 2 added (item 2).

timtuxworth and others added 5 commits August 28, 2026 12:06
Adds SerialDevice::listen_on_udp_port(), a UDP counterpart to the
existing listen_on_tcp_port(), plus a network_update_udp() variant of
network_update() to move bytes over it. Unlike TCP there is no
connection to accept; the autopilot's address is learned from
whichever datagram it last sent, and replies go back to that address.

Split out of the SkyDroid gimbal driver PR (ArduPilot#34155)
at Peter Barker's review suggestion - the real C11 hardware is
UDP-only, but this capability doesn't depend on anything
SkyDroid-specific and is useful for any simulated network-attached
device that only speaks UDP.
create_net_serial_sim() previously only attached a simulated
network-attached device (--net-device NAME:PORT) over TCP. Extends it
to support UDP too, using SerialDevice::listen_on_udp_port(), and
changes the spec format to NAME:PORT[,PROTOCOL] - comma-grouping the
port and any options together rather than colon-chaining an arbitrary
sequence of unrelated fields, since further options (e.g. a simulated
firmware version) are logically grouped with the port rather than
another top-level, NAME-like field.

Split out of the SkyDroid gimbal driver PR (ArduPilot#34155)
at Peter Barker's review suggestion - the real C11 hardware is
UDP-only, but this capability doesn't depend on anything
SkyDroid-specific.
…vices

create_net_serial_sim()'s new UDP support and NAME:PORT,PROTOCOL spec format
had no autotest coverage. Adds MountTopotekNetworkUDP, deliberately the same
body as the existing MountTopotekNetwork() other than the transport - this is
a regression guard for the generic UDP-attached-device capability itself, not
a Topotek-specific test (Topotek is used only because it's an existing,
already-generic simulated gimbal with no dependency on this PR).
AP_Networking_port::run() only ever reads up to 300 bytes per recv()
call. UDP is datagram-based, so a larger datagram sent in one go isn't
queued for a later read -- the kernel discards whatever doesn't fit.
Cap what SerialDevice::network_update_udp() sends per datagram to what
the far end can actually receive.

Also correct network_update_udp()'s docstring: pre-peer device output
isn't dropped, it's simply left unread until a peer becomes known.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This loop now dispatches to network_update_udp() for UDP-attached
devices too, not just TCP.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
timtuxworth added a commit to timtuxworth/ardupilot that referenced this pull request Aug 29, 2026
This PR's own copy of the UDP network-attached-device transport
predates the split of that work into ArduPilot#34159, which already carries
this fix - reapplying it here directly so this PR doesn't reintroduce
the truncation bug if it merges before/independently of ArduPilot#34159.

AP_Networking_port::run() reads at most 300 bytes per recv() call.
UDP is datagram-based, not a byte stream like TCP, so a datagram
larger than that isn't queued for a later read - the kernel silently
discards whatever didn't fit. Cap what network_update_udp() sends to
what the far end can actually receive in one call, or a device that
bursts more than 300 bytes at once (unlikely for SkyDroid/Topotek's
own small packets, but not something this transport should silently
get wrong for other devices) would have its frame truncated.

Flagged by AI-assisted review on PR ArduPilot#34155 (tridge/Claude), verified
against the code and against ArduPilot#34159's existing fix before applying.
timtuxworth added a commit to timtuxworth/ardupilot that referenced this pull request Aug 29, 2026
This PR's own copy of the UDP network-attached-device transport
predates the split of that work into ArduPilot#34159, which already carries
this fix - reapplying it here directly so this PR doesn't reintroduce
the truncation bug if it merges before/independently of ArduPilot#34159.

AP_Networking_port::run() reads at most 300 bytes per recv() call.
UDP is datagram-based, not a byte stream like TCP, so a datagram
larger than that isn't queued for a later read - the kernel silently
discards whatever didn't fit. Cap what network_update_udp() sends to
what the far end can actually receive in one call, or a device that
bursts more than 300 bytes at once (unlikely for SkyDroid/Topotek's
own small packets, but not something this transport should silently
get wrong for other devices) would have its frame truncated.

Flagged by AI-assisted review on PR ArduPilot#34155 (tridge/Claude), verified
against the code and against ArduPilot#34159's existing fix before applying.
timtuxworth added a commit to timtuxworth/ardupilot that referenced this pull request Aug 31, 2026
This PR's own copy of the UDP network-attached-device transport
predates the split of that work into ArduPilot#34159, which already carries
this fix - reapplying it here directly so this PR doesn't reintroduce
the truncation bug if it merges before/independently of ArduPilot#34159.

AP_Networking_port::run() reads at most 300 bytes per recv() call.
UDP is datagram-based, not a byte stream like TCP, so a datagram
larger than that isn't queued for a later read - the kernel silently
discards whatever didn't fit. Cap what network_update_udp() sends to
what the far end can actually receive in one call, or a device that
bursts more than 300 bytes at once (unlikely for SkyDroid/Topotek's
own small packets, but not something this transport should silently
get wrong for other devices) would have its frame truncated.

Flagged by AI-assisted review on PR ArduPilot#34155 (tridge/Claude), verified
against the code and against ArduPilot#34159's existing fix before applying.
timtuxworth added a commit to timtuxworth/ardupilot that referenced this pull request Aug 31, 2026
This PR's own copy of the UDP network-attached-device transport
predates the split of that work into ArduPilot#34159, which already carries
this fix - reapplying it here directly so this PR doesn't reintroduce
the truncation bug if it merges before/independently of ArduPilot#34159.

AP_Networking_port::run() reads at most 300 bytes per recv() call.
UDP is datagram-based, not a byte stream like TCP, so a datagram
larger than that isn't queued for a later read - the kernel silently
discards whatever didn't fit. Cap what network_update_udp() sends to
what the far end can actually receive in one call, or a device that
bursts more than 300 bytes at once (unlikely for SkyDroid/Topotek's
own small packets, but not something this transport should silently
get wrong for other devices) would have its frame truncated.

Flagged by AI-assisted review on PR ArduPilot#34155 (tridge/Claude), verified
against the code and against ArduPilot#34159's existing fix before applying.
@timtuxworth
timtuxworth requested a review from rmackay9 September 3, 2026 15:05
@timtuxworth

Copy link
Copy Markdown
Contributor Author

FYI @tridge

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