Skip to content

autotest: add a test for RADIO_RC_CHANNELS support - #34337

Open
peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr-claude2/radio-rc-channels-test
Open

autotest: add a test for RADIO_RC_CHANNELS support#34337
peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr-claude2/radio-rc-channels-test

Conversation

@peterbarker

Copy link
Copy Markdown
Contributor

Summary

Adds a test for RADIO_RC_CHANNELS mavlink message

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

Description

We added this message but didn't add tests at the same time. So test it.

@tridge

tridge commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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

Reviewed at head 27dcbf666f. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_09_AIReview/devcall_pr_reviews.html#pr34337

COMMENT — no blockers; one real coverage gap with a cheap fix

The only question worth asking about a new test is whether it actually tests anything, so it was answered by mutation, built the way the rover job builds (./waf configure --board sitl --enable-math-check-indexes, i.e. -O3CI_BUILD_DEBUG is unset for sitltest-rover, so a --debug build would have proved nothing about the shipped one). 11 of 12 mutations caught, including the subtle ones: PWM offset 1500→1501, divisor /32→/31, changing truncation-toward-zero to floor (a one-count difference on one channel — the test really does pin the C division direction), a channel index off-by-one, clearing the failsafe bool, swapping FAILSAFE for OUTDATED, ignoring packet->count, clamping to 4 and to 8, and never marking the input valid.

Also: 100/100 clean passes across 4 isolated checkouts × 25 runs, each in its own network namespace and TMPDIR; and 3× runs of a 7-test chain ending RadioRCChannels → MANUAL_CONTROL → Sprayer, 7/7 each time, with a clean parameter-leak check and the vehicle returning to RCInput: decoding UDP. It does not poison its neighbours and it is not flaky.

ISSUE — rover.py:7607: the test cannot detect a backend that latches the first frame

Mutating the backend to retain each channel's first PWM, while still updating count, flags and input validity, leaves the entire test green. The reason is structural and visible in the test itself: every subtest feeds the same underlying vector — values, then values[0:8], then values + values, then values again for both failsafe subtests — so the expected PWMs never change after the first frame, and "correctly converting each frame" is indistinguishable from "converted the first frame and froze".

This is not a duplicate of the conversion mutations above: those all alter the arithmetic, which changes every frame and is caught; this one leaves the arithmetic correct and stops applying it.

Suggested fix: send a second value vector with the same count and assert the PWM follows. Changing channel 1 from 1500 to 1750 was tested — it catches the mutation and passes against unmodified production code.

NOTE — rover.py:7614-7617: the "oversize count is clamped" subtest claims more than it can demonstrate

Changing MIN(packet->count, MAX_RCIN_CHANNELS) (18) to MIN(packet->count, 16) leaves the test green. That is a limit of the observation channel, not a fixable assertion: chancount comes from RC_Channels::get_valid_channel_count(), which caps at NUM_RC_CHANNELS = 16, and GCS_Common.cpp:2175 fills the message via get_radio_in(), which reads only MIN(n, NUM_RC_CHANNELS); probing chan17_raw/chan18_raw during the subtest returns 0, not the fed values. So the 16-vs-18 distinction simply is not observable here. The comment says "ArduPilot takes at most MAX_RCIN_CHANNELS and reports at most NUM_RC_CHANNELS", but what the subtest demonstrates is only "not more than 16" — worth softening so a later reader does not over-trust it.

Minor

  • vehicle_test_suite.py:9049count and time_last_update_ms are dead parameters; the sole caller passes only values and flags. count in particular would be the natural way to test a count that disagrees with the payload length, which nothing currently does.
  • vehicle_test_suite.py:9055list(values) + [0] * (32 - len(values)) silently builds an over-length list if given more than 32 values, surfacing as an obscure pymavlink packing error. Not reachable from any current caller.
  • rover.py:7640 — the wait_statustext("Radio Failsafe") runs after the pump has stopped, so no frames are flowing and the vehicle would emit that text regardless of the flag. Not load-bearing (the preceding SYS_STATUS pump is what proves causation, and both failsafe mutations die there) — worth knowing only so nobody later "simplifies" the test down to the statustext wait.
  • Placement: radio_rc_channels_pump and wait_radio_rc_channels_pwm are in rover.py while send_radio_rc_channels and radio_rc_channels_value_to_pwm are in vehicle_test_suite.py, so adding this for Copter/Plane would mean moving the first two.

Checked and clean

Frame pacing is wall-clock driven but has ~30× headroom (~50 frames/s wall ≈ 10/s sim at speedup=5 against RC_FS_TIMEOUT of 3 sim seconds); forcing speedup=50 still passes, so the speedup=5 is belt-and-braces rather than a knife edge. The released-pymavlink trap does not apply: RADIO_RC_CHANNELS is in development.xml, which ardupilotmega does not include, but the suite connects with dialect="all" and released pymavlink 2.4.49 exposes both radio_rc_channels_send and the FAILSAFE flag there, both referenced inside functions rather than at module scope. The PWM helper matches the production conversion including the negative cases, and all sixteen commented expectations verify by hand. flake8 clean; +155/−0 in one subsystem in one commit. (Incidentally the diff has no deletions, so it is +155/−0 rather than the +157/−2 the file list suggests.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants