Skip to content

Rc calib - #2308

Open
amilcarlucas wants to merge 3 commits into
masterfrom
rc_calib
Open

Rc calib#2308
amilcarlucas wants to merge 3 commits into
masterfrom
rc_calib

Conversation

@amilcarlucas

@amilcarlucas amilcarlucas commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator

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

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

AP-Review commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown

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

  1. 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

github-actions Bot commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

☂️ Code Coverage

current status: ✅

Overall Coverage

Statements Covered Coverage Threshold Status
29910 28342 95% 89% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 79060d6 by action🐍

@github-actions

github-actions Bot commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Test Results

     3 files  ± 0       3 suites  ±0   43m 9s ⏱️ -53s
 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 79060d6. ± 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.

@amilcarlucas
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>
@amilcarlucas
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
refactor(telemetry): share polling and MAVLink observer lifecycle
79060d6 
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>
@AP-Review

AP-Review commented Oct 31, 2026 •
edited
Loading

Copy link
Copy Markdown

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

AP-Review commented Oct 31, 2026

Copy link
Copy Markdown

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

  1. 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.
  2. 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.

This branch has not been deployed

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

Reviewers

No reviews

Assignees

No one assigned

Labels

AIReview Request an automated AI review; picked up by the reviewprs sweep

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amilcarlucas @AP-Review