Skip to content

FilterReview: only use armed raw IMU data if disarmed logging is rate limited - #367

Open
j-w9 wants to merge 2 commits into
ArduPilot:mainfrom
j-w9:filter-review-rate-limit
Open

j-w9 wants to merge 2 commits into
ArduPilot:mainfrom
j-w9:filter-review-rate-limit

Conversation

@j-w9

@j-w9 j-w9 commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #259, following your diagnosis there: with LOG_DARM_RATEMAX set, raw GYR samples logged while disarmed are rate limited, and the loader's batch sample rate no longer matches the data.

Reproduced in SITL (LOG_DISARMED 1, LOG_DARM_RATEMAX 10, INS_RAW_LOG_OPT 1, raw IMU logging on, takeoff, hover, land). GYR runs at ~25 Hz disarmed and 1000 Hz armed. The step from 25 Hz to 1000 Hz isn't a gap, so the whole log becomes one batch with a mean rate of 793.5 Hz, and every frequency is scaled by 0.79 (spectrum ends at 397 Hz). If a gap does split off a disarmed first batch, run_batch_fft takes its rate from data_set[0], which gives the very low rate and the oversized spectrogram from the issue.

When LOG_DARM_RATEMAX > 0 and the log has ARM messages, raw samples logged while disarmed are dropped before batching. The gap left behind splits the armed sections as before. Logs without LOG_DARM_RATEMAX, or with no armed data, take the existing path unchanged.

I don't have the log from the issue, so it would be good to check it against that one.

Before and after

@tridge tridge added the AIReview label Nov 9, 2026
@AP-Review

AP-Review commented Nov 9, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-11-09)

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

Reviewed at head cc4f13a118.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/367/1.html#prWebTools-367

The rate-limit fix works for logs with disarmed logging on, but in some multi-arm logs it now drops a whole flight.

Blocking

  1. FilterReview/FilterReview.js:2234 armed_mask() starts disarmed, but when LOG_DISARMED is 0 the first arming has no ARM message (ARM is written before the logger is marked armed). In a log with a later re-arm, every armed sample before the first logged ARM is thrown away, although none of it was rate limited. Fix: seed the state from the first logged message, let armed = (ARM.ArmState.length > 0) && (ARM.ArmState[0] == 0), or also require LOG_DISARMED != 0 at line 2270. The first option was tested on SITL logs: it recovers both flights and gives the same result for LOG_DISARMED=1.

With disarmed logging off the first arm is not logged, so starting
disarmed dropped the whole first flight of a multi-flight log.
@AP-Review

Copy link
Copy Markdown

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

Reviewed at head 76381adefc.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/367/2.html#prWebTools-367

No problems found at the new head 76381ad. The blocker from the last review is fixed: armed_mask() now starts from the opposite of the first logged arm state (FilterReview/FilterReview.js:2235), so logs with disarmed logging off keep their first flight again. Replaying the SITL logs keeps both flights, and with LOG_DISARMED=1 the batches are still clean ~1000 Hz.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error when trying to open certain logs

3 participants