Rc calib by amilcarlucas · Pull Request #2308 · ArduPilot/MethodicConfigurator

amilcarlucas · 2026-10-31T01:16:11Z

Description

RC calibration fix

Checklist

Run pre-commit checks locally
Verified by a human programmer
All commits are signed off (use git commit --signoff)
Code follows our coding standards
Documentation updated if needed
No breaking changes or properly documented

Testing

Describe how you tested these changes:

Unit tests pass
Integration tests pass
Manual testing performed
Tested on flight controller hardware

AP-Review · 2026-10-31T02:40:46Z

Deprecated — see below for the updated review.

Previous review (2026-10-31)

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

Reviewed at head 7698ba3905.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/methodicconfigurator/2308/1.html#prMethodicConfigurator-2308

Two problems: the ESC MAVLink1/BDShot and RC sample-loss fixes look correct, but the PR breaks the pylint gates, and the new single-read RC poll can fall behind the stream.

Blocking

Lint CI is red. Both pylint jobs fail (and Docker's lint step, which passed on base) on data_model_esc_telemetry.py:87 too-many-branches, data_model_rc_calibration.py:62 too-many-instance-attributes, test_data_model_rc_receiver.py:105 redefined-builtin type, test_frontend_tkinter_rc_calibration.py:703 too-many-statements, and Docker also flags duplicate-code for data_model_rc_calibration.py:410-414 vs data_model_rc_receiver.py:101-109. Add # pylint: disable=... next to the existing noqa comments (as in test_frontend_tkinter_parameter_editor_table.py:2174), wrap or share the duplicated stream-request block, or split the long functions.

Minor

data_model_rc_calibration.py:415: only one RC_CHANNELS/HEARTBEAT packet is consumed per 100 ms poll. If the 5 Hz interval request is rejected or overridden and RC arrives at 10 Hz or more, the display lags further and further behind and Finish can stage min/max without queued extremes. Drain a bounded batch per poll like RCReceiverDataModel.get_receiver_state, fold every sample into min/max and show the newest.

The windows-latest pytest failures match base and are not caused by this PR.

github-actions · 2026-10-31T03:16:44Z

☂️ Code Coverage

current status: ✅

Overall Coverage

Statements	Covered	Coverage	Threshold	Status
29908	28340	95%	89%	🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 36f3141 by action🐍

github-actions · 2026-10-31T03:17:16Z

Test Results

3 files ± 0 3 suites ±0 42m 46s ⏱️ - 1m 16s
9 135 tests +26 9 127 ✅ +26 5 💤 ±0 3 ❌ ±0
27 273 runs +78 27 235 ✅ +78 35 💤 ±0 3 ❌ ±0

For more details on these failures, see this check.

Results for commit f9fa81a. ± Comparison against base commit ea6bd3f.

This pull request removes 5 and adds 31 tests. Note that renamed tests count towards both.

tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelFlightMode ‑ test_flight_mode_reports_not_connected_without_link
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelFlightMode ‑ test_flight_mode_returns_custom_mode_from_heartbeat
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelFlightMode ‑ test_flight_mode_returns_no_data_without_heartbeat
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelFlightMode ‑ test_flight_mode_swallows_recv_match_exception
tests.test_esc_telemetry ‑ test_busy_links_have_bounded_reads_and_preserve_other_observers

tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelSources ‑ test_calibration_stages_only_selected_controller_extremes[source0]
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelSources ‑ test_calibration_stages_only_selected_controller_extremes[source1]
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelSources ‑ test_foreign_heartbeat_and_channels_cannot_establish_calibration[source0]
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelSources ‑ test_foreign_heartbeat_and_channels_cannot_establish_calibration[source1]
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelTelemetry ‑ test_batch_preserves_extremes_and_displays_latest_sample
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelTelemetry ‑ test_monitor_rerequests_stream_after_reconnection
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelTelemetry ‑ test_monitor_uses_existing_stream_when_interval_request_is_rejected
tests.plugins.test_data_model_rc_calibration.TestRCCalibrationDataModelTelemetry ‑ test_poll_bounds_unrelated_packets
tests.plugins.test_data_model_rc_receiver ‑ test_controller_step_restarts_stream_and_preserves_interleaved_rc_samples
tests.plugins.test_frontend_tkinter_rc_calibration ‑ test_controller_step_refreshes_bars_for_consecutive_samples
…

♻️ This comment has been updated with latest results.

Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>

Fixed and verified on the connected Cube: step 08 now shows ESC 9–12. 0* means zero RPM or unavailable individual telemetry; electrical values remain unavailable. Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>

AP-Review · 2026-10-31T14:47:50Z

Deprecated — see below for the updated review.

Previous review (2026-10-31)

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

Reviewed at head 0e633dd661.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/methodicconfigurator/2308/2.html#prMethodicConfigurator-2308

No problems found at the reviewed head 0e633dd6. The PR has since moved to 79060d68 ("refactor(telemetry): share polling and MAVLink observer lifecycle"), which this review does not cover.

Both problems from the last review are fixed. The pylint and Docker lint jobs pass again: the flagged sites now have suppressions, and the duplicated RC stream-request code moved into the shared backend_rc_telemetry.py. RC calibration now drains up to 50 packets per poll and folds every sample into min/max, so the display no longer falls behind a fast RC stream.

The earlier note that the windows-latest pytest failures match base still holds: the same three layout asserts fail on base. The macOS job fails in Homebrew setup before any tests run.

Not fully reviewed: commit 79060d6, pushed after the reviewed head.

Head moved during review: observed 79060d6886f240bef5efba48d380c0e597ff779b. This review covers only 0e633dd661956b27e38323c10a579adfff63aadd; followup eligible.

AP-Review · 2026-10-31T15:11:53Z

Deprecated — see below for the updated review.

Previous review (2026-10-31)

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

Reviewed at head 79060d6886.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/methodicconfigurator/2308/3.html#prMethodicConfigurator-2308

Two lint gates are red at 79060d68, both introduced by that commit.

Since the last review, the new commit 79060d68 shares the telemetry polling and observer code and filters RC calibration by source. The RC backlog fix still holds, but the commit turns the pylint and Docker lint jobs red again.

Blocking

tests/telemetry_test_helpers.py:30: ty 0.0.86 reports too-many-positional-arguments on the starred MAVLink_rc_channels_message(...) call. Both pylint jobs stop at the ty step. Add # ty: ignore[too-many-positional-arguments], or pass an explicit 18-element tuple.
backend_esc_telemetry.py:86 and data_model_rc_calibration.py:422: Docker lint fails on four pylint duplicate-code (R0801) reports. The commit dropped the # pylint: disable=duplicate-code that covered the ESC command_long layout, and the new RC source-filter loop repeats data_model_rc_receiver.py:95-100. Restore the ESC suppression and add one before the RC loop, or share the loop. With both, pylint 4.1.3 passes.

Note

windows-latest and macOS CI failures are not caused by this PR (unchanged from the last review). The same three Windows layout asserts also fail on master, and macOS stops in Homebrew python-tk setup.

The earlier notes that 79060d68 was unreviewed are closed by this review.

Consolidate telemetry transport and RC view polling to keep connection handling, bounded reads, rendering, and warnings consistent. - Add MavlinkTelemetryBackend for shared observer registration, connection changes, cleanup, and the 200-packet/10-ms polling limit. - Reuse the backend in ESC and data telemetry while preserving their message decoding and request behavior. - Share polling, rendering, warning counters, and timer scheduling between the embedded RC calibration view and floating monitor. - Apply the existing selected-controller source filter to calibration so foreign RC samples and heartbeats cannot affect its results. - Remove the unused heartbeat reader that could discard RC samples. - Reuse source-aware MAVLink messages and controller fixtures in tests. - Cover foreign-source rejection, polling limits, reconnects, and preservation of other observers. - Include the shared backend in mutation-testing targets. Validation: - 482 relevant tests pass. - Two known GUI layout failures remain excluded. - Pre-commit, Pylint, mypy, and Pyright pass. Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>

Copilot

🟡 Changes recommended

Two telemetry tests leave real heartbeat threads running after teardown because backend.close() only detaches the observer; both need to disable the heartbeat side effect before polling.

1 open issue

Disable heartbeat threads in telemetry polling tests · New

What changed in this PR

This PR fixes RC calibration telemetry handling and improves ESC monitoring in ArduPilot Methodic Configurator.

Changes:

Share RC stream lifecycle handling, filter telemetry by controller source, and preserve calibration extremes across queued samples.
Support ESC telemetry on connections initially using MAVLink 1 and mark ambiguous zero-RPM readings for configured bidirectional DShot outputs.
Consolidate bounded telemetry polling and expand regression coverage.

File	Description
tests/test_frontend_tkinter_navigation_lock.py	Adapt cleanup mocking to the new polling mixin.
tests/test_esc_telemetry.py	Cover protocol compatibility, zero RPM, and polling limits.
tests/telemetry_test_helpers.py	Add a shared real RC message builder.
tests/plugins/test_frontend_tkinter_rc_calibration.py	Test stream resumption and consecutive display updates.
tests/plugins/test_data_model_rc_receiver.py	Test receiver-to-calibration stream handoff.
tests/plugins/test_data_model_rc_calibration.py	Cover source filtering, queued extremes, and reconnection.
tests/plugins/conftest.py	Share a selected-controller fixture.
pyproject.toml	Include the shared telemetry backend in mutation testing.
ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py	Share polling logic and release streams on deactivation.
ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py	Use the shared RC telemetry transport.
ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py	Process bounded batches while preserving calibration extremes.
ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py	Add shared stream management and source filtering.
ardupilot_methodic_configurator/frontend_tkinter_esc_telemetry.py	Explain ambiguous zero-RPM readings in the tooltip.
ardupilot_methodic_configurator/data_model_esc_telemetry.py	Display configured bidirectional DShot zero readings as `0*`.
ardupilot_methodic_configurator/backend_mavlink_telemetry.py	Centralize observer lifecycle and bounded reads.
ardupilot_methodic_configurator/backend_esc_telemetry.py	Request and decode MAVLink 2 ESC telemetry.
ardupilot_methodic_configurator/backend_data_telemetry.py	Reuse the shared observer and polling implementation.

_{Review effort: Balanced}

AP-Review · 2026-11-01T19:38:28Z

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

Reviewed at head f9fa81a31d.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/methodicconfigurator/2308/4.html#prMethodicConfigurator-2308

Only a minor test-cleanup issue remains at f9fa81a; no runtime problem was found.

Both blockers from the last review are fixed: ty and pylint (including duplicate-code) pass again, and the pylint and Docker lint jobs are green.

Minor (1)

tests/test_esc_telemetry.py:698 and :729: the new DataTelemetryBackend cases start a real gcs-heartbeat thread that outlives the test, because the link fixture never disconnects. Set link.controller.start_gcs_heartbeat.side_effect = None before polling, as in the existing empty-poll test, or disconnect in fixture teardown. This is the same as Copilot's comment.

Note

CI: the windows-latest and macOS failures are not caused by this PR. The same three Windows layout asserts also fail on base, and macOS stops in Homebrew python-tk setup on both.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Amilcar Lucas <amilcar.lucas@iav.de>

amilcarlucas added the AIReview Request an automated AI review; picked up by the reviewprs sweep label Oct 31, 2026

amilcarlucas added 2 commits October 31, 2026 14:53

fix(rc_calibration plugin): Fix loading and unloading of the plugin

f37bce0

Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>

fix(ESC telemetry plugin): fix startup

0e633dd

Fixed and verified on the connected Cube: step 08 now shows ESC 9–12. 0* means zero RPM or unavailable individual telemetry; electrical values remain unavailable. Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>

amilcarlucas force-pushed the rc_calib branch from 7698ba3 to 0e633dd Compare October 31, 2026 13:53

amilcarlucas force-pushed the rc_calib branch from 79060d6 to f9fa81a Compare November 1, 2026 18:55

Copilot AI balanced review requested due to automatic review settings November 1, 2026 18:55

Copilot started reviewing on behalf of amilcarlucas November 1, 2026 18:55

View session

Copilot AI reviewed Nov 1, 2026

View reviewed changes

Comment thread tests/test_esc_telemetry.py Resolved

test(heartbeat): fix GCS heartbeat side effect in telemetry test

36f3141

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Amilcar Lucas <amilcar.lucas@iav.de>

amilcarlucas merged commit 9f80d2d into master Nov 1, 2026
24 of 27 checks passed

amilcarlucas deleted the rc_calib branch November 1, 2026 20:20

Navigation Menu

Repository navigation

Rc calib - #2308

Rc calib#2308
amilcarlucas merged 4 commits into
masterfrom
rc_calib

amilcarlucas commented Oct 31, 2026 •

edited

Loading

Uh oh!

AP-Review commented Oct 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 31, 2026 •

edited

Loading

Uh oh!

AP-Review commented Oct 31, 2026 •

edited

Loading

Uh oh!

AP-Review commented Oct 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

AP-Review commented Nov 1, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

amilcarlucas commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing

Uh oh!

AP-Review commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☂️ Code Coverage

Overall Coverage

New Files

Modified Files

Uh oh!

github-actions Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

AP-Review commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AP-Review commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

AP-Review commented Nov 1, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amilcarlucas commented Oct 31, 2026 •

edited

Loading

AP-Review commented Oct 31, 2026 •

edited

Loading

github-actions Bot commented Oct 31, 2026 •

edited

Loading

github-actions Bot commented Oct 31, 2026 •

edited

Loading

AP-Review commented Oct 31, 2026 •

edited

Loading

AP-Review commented Oct 31, 2026 •

edited

Loading