[andyp1per]

Summary

If you land a copter on optical flow, the sensor goes out of focus over the last part of the descent, and what it returns there is not motion. EKF3 fuses it anyway and dead reckons a phantom horizontal velocity, which the position controller then chases on the way down. FLOW_HGT_MIN tells the filter the height below which this sensor's output is discarded instead. Discarding it on the ground too would let relative aiding time out after touchdown and drop an armed vehicle in LOITER into an EKF failsafe, so once the vehicle rests on the ground the flow is fused as zero, as it is before takeoff.

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

• Checked by a human programmer
• Automated test(s) verify changes (e.g. unit test, autotest)
• Tested manually, description below (e.g. SITL)
• Tested on hardware

SITL, Copter.OpticalFlowFocusHeight: hover in an asserted altitude band below the floor on flow-only nav, then inject a one-axis flow rate offset with SIM_FLOW_OFS_X to stand in for an unfocused reading. Peak EKF groundspeed is 0.03 m/s with the floor set and 0.90-1.22 m/s with it disabled, over three runs, so the test's bounds sit either side of a 40x separation rather than close to either result.

Copter.FlowFocusHoldAfterLanding fails if relative aiding stops through a flow-only landing below the floor, a dwell armed on the ground and a climb clear. A LOITER landing with an origin and the default DISARM_DELAY has to disarm normally, also when the range finder reads 0.19 m on the ground against RNGFND1_GNDCLR 0.10; discarding the flow there gave "EKF variance" and a failsafe LAND. In a hover below the floor the flow has to be discarded and aiding stop. A hover within 5 cm of an old ground reading fused zero flow 39-45 times while that reading was kept, and 0 with the clears described below.

The mechanism flew on a 4-inch flow quad as EK3_FLOW_MIN_H on the #33484 branch: on a descent through the floor (rangefinder 0.115 -> 0.054 m) the phantom VN/VE fell from +/-0.5 to +/-0.1 m/s and commanded roll/pitch from +/-14 to +/-2 deg. That rangefinder is valid to 1 cm, which is what made a 0.1 m floor reachable on it.

This PR moves the parameter into the flow library and routes its value to the filter through the DAL, discards the sample where the flown build zeroed it, holds the check below the rangefinder minimum and adds a floor at the ground clearance. The discard and that floor have since flown on an ARK Flow octaquad with FLOW_HGT_MIN raised to 2.0 m, and zero flow at rest with FLOW_HGT_MIN 0.15: flow innovations stopped updating in a hover below the floor, relative aiding was kept through 8 s armed on the ground, and there was no failsafe. The learned ground reading and its two clears have not flown.

Description

Detail for reviewers below; not needed to review the change in outline.

The parameter lives in the flow library because the height is a property of the sensor, not of the estimator, and it sits beside the mounting position and FLOW_HGT_OVR where someone configuring a flow sensor will look for it. It reaches the EKF as data travelling with the sample it applies to rather than as something the filter reads out of another library's parameters, which is what keeps Replay working.

It rides in its own ROFM record rather than in ROFH. log_ROFH ends in a uint8_t and the DAL structs are not packed, so a float appended after the quality brings three bytes of padding the format string cannot describe: the struct is 47 bytes against 44 from the format, and SITL rejects the log structures at boot. Put before the quality instead, the float takes the byte an older log holds the quality in, and since Replay copies only the logged length and zeroes the rest, every flow sample of an old log would replay with quality 0 and be thrown away. ROFM is written only when the value changes and once at the start of each log, so it costs one record rather than four bytes a sample, and a log without one replays with the feature disabled. 97b5b04 split RISJ out of RISI the same way.

Writing it only on change has one trap worth naming, since the next person to add a DAL record will hit it. WriteLogMessage drops everything written before logging starts, and a record that is only written when it changes never gets a second chance - the struct latches the value, the comparison then matches forever, and no record ever carries it. RISJ is safe because start_frame() runs on the frame that forces a write when logging starts; a record written from a sensor callback has no such frame, so the value is held back until it can actually be logged, and ROFM is written again when a new log starts. A SITL run with FLOW_HGT_MIN=3 logs ROFM 3.0 then 0.0 across 430 ROFH.

AP_NavEKF2 gains no behaviour but has to round-trip the field, because both estimators write the shared replay record and the two writes would otherwise differ on every sample and log it twice.

The decision stays in the EKF rather than moving to the flow library with the parameter. Two reasons. The library has no height above ground, so it would need its own rangefinder dependency and its own tilt correction, giving a second and worse answer to a question the EKF already answers at the fusion time horizon. And suppressing the sample in the library is not the same behaviour: reporting quality 0 would stop the filter fusing on the ground too, where it fuses zero to keep aiding, while zeroing the rate leaves the body rate term in flowRadXYcomp so the filter sees apparent motion equal to the gyro. "This sample is untrustworthy" is a sensor statement; "therefore assume zero motion" is an estimator one.

FLOW_HGT_MIN defaults to 0: sensors do not share a focus height, so set it from your own sensor. Independently of it, EKF3 holds flow off within 5 cm of the rangefinder ground clearance (the larger of RNGFNDx_GNDCLR and 0.05 m), where the range is clamped and the flow is not motion. FLOW_HGT_MIN only raises that floor. On the 4-inch airframe (RNGFND1_GNDCLR 0) that floor is the flown 0.10 m; a default airframe (0.10) gets 0.15 m.

The height compared with the floor is the last range sample carried forward by the vertical position change since. That covers both the lag behind the rangefinder's median filter and the stretch below its minimum range, where it stops reporting. A carried height is trusted for 5 s, because it cannot see the ground change under a vehicle that has moved. Beyond that, a hold continues while the rangefinder that gave the sample reports out of range low, which it does on the ground until disarm, and the carried height still has the vehicle within half a metre of the floor. Both are needed: out of range low is also what a dead sensor reports, and it reads as on the ground, so without the height term such a sensor would have zero flow fused through a climb. Height resets move the carried position with them, so a reset does not read as movement.

Below the floor the flow is fused as zero when the vehicle is at rest and discarded otherwise. At rest is the range finder's on_ground(), reading its ground clearance or too close to measure, so vehicle landed and flying flags, which read wrong in flight on some vehicles, are not used. A vehicle whose range finder reads more than its ground clearance on the ground, as a taller one with RNGFNDx_GNDCLR left at its default does, misses that test, so a reading within 5 cm of the one taken on the ground before takeoff counts too. That reading is learned while disarmed from each stored flow sample, kept only under the floor, and cleared on disarm and once no flow sample has been stored for a second: a sensor that stops storing samples on the ground, as one reporting quality 0 does, would otherwise carry an old reading to ground that reads closer, and a hover within 5 cm of it would fuse zero flow. Off the ground below the floor, in a low hover, held-off flow counts as not ready to use, so aiding that times out there stays off rather than restarting from the fresh samples and timing out again every 5 s.

What zero at rest does not cover, also said in the RNGFNDx_MIN and FLOW_HGT_MIN descriptions: too close counts as the ground, so where RNGFNDx_MIN is above the ground clearance plus 0.05 a hover below the minimum also fuses zero; a vehicle that moves on the ground with its range finder reading the ground, a plane rolling out, fuses zero while it moves. Rover builds leave the floor out altogether, as the EKF's other FLOW_HGT_OVR use already is Rover-only: OpticalFlowRangefinderOnGround drives a Rover with FLOW_HGT_OVR 1.5, with it at 0, and a small one with both sensors 0.14 m up, at 0.40, 0.40 and 0.43 m/s against 0.39 (0.01 m/s with FLOW_HGT_OVR 0 while the floor was built into Rover). Replay is its own build, so like that other use it runs the floor on a Rover log; and a landing that reads more than 5 cm further than before takeoff still discards the flow.

Rover is built without the floor: it drives at its range finder ground clearance, where the floor would fuse zero flow, and a run-time vehicle-class test would put vehicle-specific code in the EKF. Replay is its own build, so it follows the logged vehicle class there and replays a Rover log without the floor, as Rover ran it; vehicle builds are unchanged. SITL Rover flow logs now replay to 0.000 m/s and 0.00 m of the flight, from 0.3 m/s and 4.5 m apart. A Rover log with FLOW_HGT_OVR set still differs slightly, from the existing Rover-only height override, which Replay does not build.

The cutoff uses rangefinder height at the vehicle origin rather than at the sensor, so a large FLOW_POS lever arm biases it slightly.

Split out of #33484 after review feedback there that the parameter belonged in the flow library. The first two commits here, adding SIM_FLOW_OFS, are the same two commits that open #33484 - whichever lands first, the other should drop them. The rest is independent and the two can be reviewed in either order.

[tridge]

Deprecated — see below for the updated review.

Previous review

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

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_05/devcall_pr_reviews.html#pr34292

Reviewed at head 292ec09fef. REQUEST CHANGES, on one deterministic CI failure that hasn't been reached yet. Everything else here is sound, and the reasoning in the description and the test comments is better than most.

The parameter metadata doesn't parse

The @Param: _HGT_MIN doc block was inserted between _OPTIONS's doc block and its AP_GROUPINFO, so the parser attributes the new metadata to the previous parameter. Running the project's own tool on this head:

Error in ../libraries/AP_OpticalFlow/AP_OpticalFlow.cpp
At param FLOW_OPTIONS
FLOW_OPTIONS already has field DisplayName
FLOW_OPTIONS already has field Description
FLOW_OPTIONS already has field User

Deterministic. Moving AP_GROUPINFO("_HGT_MIN", 8, ...) up under its own doc block — or the doc block down past _OPTIONS's AP_GROUPINFO — fixes it. CI is 2 passing / 83 queued, so param_parse hasn't run yet.

Checked and sound

• The parameter is right. FLOW_HGT_MIN is 12 characters against the 16 limit; index 8 has never been used in this table so nothing stale can load into it; Range/Units/Increment/User all present; default 0 = disabled, so nothing changes on upgrade.
• The consumer is correct including the tilt term. rangeDataDelayed.rng * prevTnb.c.z < minHeight is the slant range projected onto the vertical, gated on takeOffDetected, a fresh range within 500 ms, and minHeight > 0. It zeroes both flow vectors rather than dropping the sample — the difference between "no motion" and "no data", and the right one.
• EKF2 round-trips it with a comment saying why ("does not use minHeight, but shares the replay record with EKF3 so must round-trip it"). Worth crediting; that's usually found later by someone debugging replay.
• The autotest fails if the feature is removed. Bounds sit either side of a measured 30× separation (0.03 vs 1.02 m/s) rather than close to either result, and it asserts it actually reached the altitude band because an ALT_HOLD takeoff overshoots by an unchecked amount.

Two things to know before merge

1. The Replay record grew a field, and MSG_CREATE doesn't zero the struct. log_ROFH gained float minHeight and the format went "ffffIffffB" → "ffffIfffffB". In Tools/Replay/LR_MsgHandler.cpp:12 the macro declares log_##sname msg; uninitialised and copies offsetof(_end) bytes from the record — so replaying a log recorded before this change reads four bytes past that record into minHeight. Since the consumer is gated on minHeight > 0.0f, a garbage positive float silently zeroes optical flow during replay.

Growing a DAL record is normal practice and this isn't a reason to hold the PR. But this is the first ROFH field whose garbage value changes behaviour rather than being inert, and #32471 does the same thing to RISJ in this same batch — one macro, two PRs, and log_##sname msg{}; fixes both.

2. Two commits here are byte-identical to two in #33484. SITL: add SIM_FLOW_OFS optical flow rate offset for fault injection and AP_OpticalFlow: apply SIM_FLOW_OFS offset to the SITL flow rate appear in both PRs under different SHAs with identical change lines. Whichever merges second will conflict or need them dropped. Worth landing the shared SITL support once, on its own.

One thing I'm not passing on: a cross-check pass reported that SIM_FLOW_OFS_X/Y reuse retired SITL indices 37/38. Parsing every AP_Param::GroupInfo table in SITL.cpp at this head shows the new entries are in SIM::var_info, which has no duplicate indices, and they follow FLOW_ENABLE=33, FLOW_RND=34, FLOW_RATE=35, FLOW_DELAY=36 sequentially. The other 37s are in var_info2, var_mag and var_ins, which have their own index spaces. No collision.

Minor: the cutoff uses rangefinder height at the IMU origin rather than at the flow sensor, so a large FLOW_POS lever arm biases it slightly; and the description says "the EKF" where only EKF3 acts on it.

[tridge]

Superseded — see the review at head 84ec31a99d: #34292 (comment)

Previous review, kept for the record

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

Reviewed at head 7d8ec8f344.

Full report, with what was checked and the carried-over context: https://uav.tridgell.net/DevCallReviews/2026_09_05/devcall_pr_reviews.html#pr34292

REQUEST CHANGES

Both findings from my previous round are resolved — details at the end. The core idea and the EKF3 consumer are sound. One concrete replay-plumbing bug remains, and separately @peterbarker's CHANGES_REQUESTED of 2026-09-04 is against this exact head with none of its seven comments answered yet.

A second log in the same power cycle carries no ROFM, so it replays with the feature off

7d8ec8f344 fixed the first instance of "a change-only record that is dropped never gets a second chance". The same mechanism still bites when logging stops and restarts without a reboot.

AP_DAL::WriteLogMessage() returns early on !logging_started without touching the _end retry flag, so the drop is silent and never retried. And force_write is set and cleared inside a single start_frame() invocation (AP_DAL.cpp:45-47 and :119), so a push-path callback like writeOptFlowMeas() never observes it — which is the direct answer to Peter's "isn't force_write supposed to take care of this?" at AP_DAL.cpp:352. He is right that it should; it doesn't, because it only reaches records written from inside start_frame().

The sequence: flight 1 writes ROFM and latches _ROFM.minHeight at 3.0; logging stops; a new file opens; the memcmp now matches so no ROFM is written; replaying that log leaves _ROFM zeroed and feeds minHeight = 0 to both estimators. The floor is off in replay while it was on in flight — precisely the divergence the DAL commit exists to prevent.

Realistic triggers, all of which close _write_fd:

• downloading a log over MAVLink between flights — AP_Logger_File.cpp:614 calls stop_logging() first. Fly → land → download → arm → fly is an ordinary sequence.
• SD out-of-space or a write timeout (:957, :1002), and EraseAll() (:412).
• LOG_FILE_DSRMROT=1 — though in your favour, that path is explicitly suppressed when LOG_REPLAY is set, so it does not fire for the users most likely to care.

Two-line fix that also covers the case you already fixed, so the logging_started special case can go away:

    _ROFM.minHeight = minHeight;
    if (!logging_started) {
        _ROFM._end = 1;   // WriteLogMessage drops this; force it out when logging starts
    }
    WRITE_REPLAY_BLOCK_IFCHANGED(ROFM, _ROFM, old_ROFM);

_end is outside the compared prefix and WriteLogMessage clears it on a successful write. Alternatively fix the class of bug for everyone by having the !logging_started early return set _end = 1 — the same one-liner, and arguably what force_write's intent was.

An independent cold review, with no sight of the above, reported the same ROFM omission.

Other findings

• FLOW_HGT_MIN is not constrained in code. @Range: 0 3 is advisory and the consumer is a bare comparison. With FLOW_HGT_MIN=50 and a rangefinder reporting Good to 50 m, the flow is zeroed on every sample — and under EK3_SRC*_VELXY=5 that fuses a zero-ground-velocity observation continuously while moving. That is a fly-away-shaped failure from one mistyped parameter. constrain_float(_height_min, 0, 5) at the accessor would make it degrade rather than disable.
• A second floor the docs don't mention. readRangeFinder() clamps with MAX(storedRngMeas[...], rngOnGnd) where rngOnGnd = MAX(RNGFNDx_GNDCLEAR, 0.05). So a user with RNGFND1_GNDCLEAR=0.10 who sets FLOW_HGT_MIN=0.10 gets silence, not a floor. Worth a clause in the @Description alongside the existing RNGFNDx_MIN warning.
• ofDataDelayed.minHeight is read when no flow sample was recalled (AP_NavEKF3_OptFlowFusion.cpp:33, :58). The vectors zero themselves but the scalars don't, and recall() only memcpys on success — so with flowDataToFuse == false that is an indeterminate read. Impact is nil today, but it is UB and free to fix with of_elements ofDataDelayed {};, which also cleans up the pre-existing heightOverride case.
• The autotest proves the floor suppresses bad flow, never that it costs good flow. Both assertions read the EKF's groundspeed while hovering, so "estimate stays near zero" is what zeroing the flow guarantees either way. Below the floor the EKF is fused with a zero-velocity observation that actively pulls the velocity state to zero — and on a Copter descent takeOffDetected stays true until onGround, so a vehicle drifting at 0.5 m/s at 0.5 m AGL is told it is stationary. A third subtest with real lateral motion below the floor would document that honestly. Not a blocker, since the default is off.
• Two commits (25c7364cb5, d70cb7a058) are still duplicated with AP_NavEKF3: recover horizontal velocity from single-axis optical flow lockout #33484 — you already flag this; whichever lands second needs them dropped.
• Nits: the description says "the EKF treats the flow as zero motion" where only EKF3 acts on it (EKF2 gets the value only to keep the DAL record identical); arducopter.py:4478 says "30x separation" while the commit message says 40x (1.02/0.03 = 34, 1.2/0.03 = 40 — both defensible, pick one).

On Peter's review — where I think you are right

• AP_DAL.cpp:352 — you are right on the merits, for the reason above. But the workaround as written is incomplete (see the finding); making WriteLogMessage set _end answers him and fixes it.
• AP_NavEKF3_OptFlowFusion.cpp:55 "why not terrainState - position.z like the block above?" — your choice is the correct one and worth defending in a comment: terrainState is itself fused from optical flow (:243, :278), so gating flow on it is circular, and it isn't updated at all when activeHgtSource == RANGEFINDER (inhibitGndState = true), which is a common flow-nav configuration. The block above gets away with it because it runs pre-takeoff. That reasoning is not in the code and should be.
• "you're checking staleness where the above doesn't" — the check is necessary here: rangeDataDelayed is only refreshed when recall() succeeds, so without it the gate would keep firing on the last value after the rangefinder drops out, which is exactly what happens below RNGFNDx_MIN. The 500 ms window matches the precedent at AP_NavEKF3_PosVelFusion.cpp:1249.
• "tie into gndOffsetValid" — both disjuncts are wrong for this purpose: 5 s is far too loose for a 0.1–3 m decision on a descending vehicle, and activeHgtSource == RANGEFINDER is unconditionally true in the target configuration.
• NaN rather than 0 for the sentinel — reasonable, but the adjacent heightOverride argument on the same call already uses 0-means-unused, so NaN would make the two disagree.
• DCM33FlowMin ignored — harmless (both consumers of the zeroed vectors are gated on tiltOK), but adding tiltOK to the condition is free and makes that obvious.
• The two autotest points (line splitting, guided-mode rather than RC) are straightforward and the raciness one is well taken.

Previous round — triage

1. Parameter metadata didn't parse — RESOLVED by d4d6cd08dd; param_parse.py runs clean locally and the CI job is green.
2. log_ROFH grew a field and MSG_CREATE didn't zero the struct — RESOLVED by 1f6babe468/f059f13244; ROFH is restored byte-for-byte and the height moved to the new ROFM, so no pre-change log is misparsed.
3. Duplicate commits with AP_NavEKF3: recover horizontal velocity from single-axis optical flow lockout #33484 — still open, above.
4. Rangefinder height at the IMU origin — RESOLVED in documentation; the FLOW_POS lever-arm caveat is now in the description.
5. "the EKF" where only EKF3 acts — still open, nit above.

I also confirmed the things that would be easy to get wrong and aren't: FLOW_HGT_MIN is 12 chars; index 8 has never been used in any released version (checked back to Copter-3.6.0); no name collision; default 0 is a genuine no-op; the gate is correctly tilt-compensated and correctly not keyed on terrainState; and it suppresses the measurement rather than the sample or the sensor, so flowDataValid, doingFlowNav and the failsafe paths are untouched.

[tridge]

Superseded — see the review at head 76d3538247: #34292 (comment)

Previous review, kept for the record

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

Reviewed at head 84ec31a99d.

Full report, with the replay-compatibility check and the carried-over context: https://uav.tridgell.net/DevCallReviews/followups/2026_09_06_1710/devcall_pr_reviews.html#pr34292

COMMENT — was REQUEST CHANGES

My blocker is fixed, and it is fixed the better of the two ways I offered. I raised it as a writeOptFlowMeas() problem; you fixed it in AP_DAL::WriteLogMessage() itself, which repairs the same latent hole in every WRITE_REPLAY_BLOCK_IFCHANGED record — REPH, REVH, RWOH, RBOH, RSLL, RTER and RISJ all had it too. Every other finding of mine is addressed. Details below; nothing here is blocking.

Resolved

• The ROFM-lost-on-second-log blocker — libraries/AP_DAL/AP_DAL.cpp:304. The _end reference now sits above the logging_started check and the not-logging return sets _end = 1. I checked the part that makes it work: logging_started is recomputed every frame from AP::logger().logging_started() && allow_start_ekf() (AP_DAL.cpp:44), so it really does go false when a file closes between flights, and the next sample latches the flag. _end is outside the compared prefix and is cleared on a successful write. The if (logging_started) hold-back in writeOptFlowMeas() is gone. This also answers @peterbarker's force_write question on the merits.
• FLOW_HGT_MIN unbounded — libraries/AP_OpticalFlow/AP_OpticalFlow.h:114 now constrain_float(_height_min, 0, 5), and AP_OpticalFlow.cpp:290 is the only caller, so nothing bypasses it.
• ofDataDelayed.minHeight read with no recalled sample — libraries/AP_NavEKF3/AP_NavEKF3_OptFlowFusion.cpp:60 now leads with flowDataToFuse &&, and tiltOK covers Peter's DCM33FlowMin point. The comment at :54-59 states the terrainState and staleness arguments he asked for.
• The second floor — the @Description now names RNGFNDx_GNDCLR. Your spelling is right and my previous review's was stale: master renamed GNDCLEAR to GNDCLR.
• The autotest now discriminates, and it has passed in CI. The third subtest (Tools/autotest/arducopter.py:4513, FLOW_HGT_MIN=1.0 at 2.0 m) kills a floor that fires at every height — which both earlier subtests would have passed. OpticalFlowFocusHeight is registered in tests1c (arducopter.py:16649) and autotest (sitltest-copter-tests1c) is green at this head, so this is confirmed by CI rather than by reading.

Your one failing check is master's, not yours

cubeorangeplus-quadplane / "Run Renode unit tests" fails three Tools/renode/tests/test_driver_inventory.py assertions on AP_RANGEFINDER_BENEWAKE_TFS20L_ENABLED. The identical failure is on master's own CI — job 101293956282 at 371990d846. #34297 renamed that symbol to …_I2C_ENABLED; Tools/renode/driver_catalog.py:927, added by #33975 95 minutes later, still names the old one. One line fixes it, and nothing here is for you to do.

Small, not blocking

• @Range and the clamp disagree — AP_OpticalFlow.cpp:112 advertises @Range: 0 3; AP_OpticalFlow.h:114 clamps to [0, 5]. I'd keep the safety net wider than the advice, but a GCS will offer 0–3 and silently honour 4. Either make them agree or say "values above 5 m are clamped" in the @Description.
• The WriteLogMessage() change is under an optical-flow headline. Commit 17d5a572ed ("AP_DAL: move the optical flow focus height to a new ROFM message") also changes Replay determinism for every DAL record. It's the right fix, but Peter should see it as its own change rather than find it inside a flow commit — worth splitting out, or at least saying so in the message.
• Two commits are still duplicated with AP_NavEKF3: recover horizontal velocity from single-axis optical flow lockout #33484 — 25c7364cb5/d70cb7a058 here vs 9d8e218d67/2e02161d1a there, identical headlines, and AP_NavEKF3: recover horizontal velocity from single-axis optical flow lockout #33484 is still open. Worth deciding which PR owns SIM_FLOW_OFS.
• Pre-existing and not yours, but free while you're in the file: AP_NavEKF3_OptFlowFusion.cpp:33 of_elements ofDataDelayed; is uninitialised, and EstimateTerrainOffset() reads .flowRadXY and .heightOverride on the rangeDataToFuse-only path. of_elements ofDataDelayed {}; closes it.

The actual gate

@peterbarker's CHANGES_REQUESTED of 2026-09-04 is still standing and the thread has had no reply at all. Four of his six inline points are now answered in the code — force_write, terrainState/staleness, DCM33FlowMin, and both autotest points — and none of them in words. His two NaN-sentinel comments (AP_DAL.h:237, AP_NavEKF2.h:186) are neither adopted nor answered; for what it's worth I think 0 is right, since it matches the adjacent heightOverride argument, but that case hasn't been made anywhere. He won't re-review a silent force-push, and the PR can't merge until he clears his own review — a short reply saying which points the code now answers is the highest-value next step.

Replay compatibility checked and clean in both directions: log_ROFH and its format string are byte-identical to pre-PR master, "ROFM" collides with no name in AP_Logger/LogStructure.h, an old log replays with the feature off via the zeroed struct (no uninitialised read — the memcpy of offsetof(_end) fills the whole compared prefix), and the mid-enum LOG_ROFM_MSG insertion is harmless because LogReader dispatches on the 4-char name from the log's own FMT.

[andyp1per]

Force-pushed 4c6ae1f98a -> 76d3538247. The tree is unchanged - git diff 4c6ae1f98a 76d3538247 is empty - only the commit boundaries moved.

The WriteLogMessage fix is now its own commit, fed173b5f2, sitting ahead of the ROFM message. It repairs the same dropped-write hole in every WRITE_REPLAY_BLOCK_IFCHANGED record - REPH, REVH, RWOH, RBOH, RSLL, RTER and RISJ - so it had no business hiding inside a commit with an optical flow headline. That is the commit behind the force_write answer above.

Also folded the @Range: 0 3 metadata fix into f66adf417c, where the clamp it disagreed with was introduced.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-11)

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

Reviewed at head 76d3538247.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_06_1955/devcall_pr_reviews.html#pr34292

APPROVE — was COMMENT

Everything I have raised across four rounds on this PR is now closed. You pushed the @Range fix within minutes of the last comment, and the whole file-content delta is that one line: @Range: 0 3 → 0 5 at libraries/AP_OpticalFlow/AP_OpticalFlow.cpp:111, matching the constrain_float(_height_min, 0, 5) clamp. Both passes confirmed by full tree comparison that exactly one blob differs.

Two other things landed with it:

• The WriteLogMessage() change is now its own commit — fed173b5f2, touching only libraries/AP_DAL/AP_DAL.cpp, with a message naming all eight affected records and explaining why force_write does not cover the case. That is better than what I asked for.
• You answered @peterbarker in the thread, including the zero-versus-NaN rationale. His CHANGES_REQUESTED still stands — replies aren't approval — but the PR is no longer waiting on silence, which is what I said the highest-value next step was.
• The two commits shared with #33484 remain, and the description now says whichever lands second should drop them. Non-blocking, and that's the coordination I was asking for.

A correction to my previous comment

I said EstimateTerrainOffset() reads both .flowRadXY and .heightOverride from a potentially uninitialised ofDataDelayed. The cold pass flagged that and it's right: .heightOverride is read at AP_NavEKF3_OptFlowFusion.cpp:353 inside FuseOptFlow(), which is guarded by flowDataToFuse && tiltOK, so a sample was always recalled and there is no uninitialised read there.

The underlying concern survives on the other member, and is sharper than I described. EstimateTerrainOffset() runs when (flowDataToFuse && FLOW_USE_TERRAIN) || rangeDataToFuse, so on the range-only path flowDataToFuse is false and ofDataDelayed is untouched stack — yet :108 reads MAX(ofDataDelayed.flowRadXY[0], flowRadXY[1]) > _maxFlowRate as a term of cantFuseFlowData. Garbage there makes that flag non-deterministic, which perturbs terrain estimation and breaks Replay determinism. This is pre-existing master code and not a finding against this PR — but of_elements ofDataDelayed {}; at :33 closes it in one line and you're already in the file.

Your cubeorangeplus-quadplane failure is master's, and master is now fixed. #34303 merged at 18:41Z and is master HEAD, so Tools/renode/driver_catalog.py:927 now reads AP_RANGEFINDER_BENEWAKE_TFS20L_I2C_ENABLED. A rebase clears it; there was never anything for you to fix.

Everything else from the previous round is unchanged and still holds: the DAL retry behaviour, the recalled-sample gate, the tilt check, the parameter clamp, the RNGFNDx_GNDCLR description and the replay formats. autotest (sitltest-copter-tests1c), which runs OpticalFlowFocusHeight, is green.

[tridge]

Deprecated — superseded by the updated review further down this thread.

Previous review (superseded 2026-09-12)

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

Re-reviewed at head 337cf08df6 (you were last told about 76d3538247); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_11_0118/devcall_pr_reviews.html#pr34292

The PR moved again while this review was being written — it is now at 946708d630. I checked: that is a rebase with the PR's own content byte-identical (21 own files, +/- 219 lines either side, rebased onto a newer master), so every finding below applies unchanged at the new head.

The delta is a fast-forward, not a force-push: two commits, 2 files, +10/−3. My last automated round was an APPROVE with nothing outstanding, so there is no previous AI finding to triage — but @peterbarker's CHANGES_REQUESTED of 2026-09-09 is still open, and that outranks anything here. You have replied to his inline comments (six replies), so this is not a case of silence; what remains is that the review has not been withdrawn, the enum request is unactioned, the zero-motion point is addressed only through documentation, and his validity question is unanswered.

Verdict: REQUEST CHANGES, on the finding below.

BUG — the new terrain withhold is bypassed whenever range data arrives in the same cycle

AP_NavEKF3_OptFlowFusion.cpp:76 gates the call with !flowBelowFocusHeight, but that flag only suppresses the first disjunct. When rangeDataToFuse is true, EstimateTerrainOffset() is called anyway — with the zeroed sample. Inside, the flow branch is not gated on the flag either: cantFuseFlowData at :111-115 tests only MAX(flowRadXY) > _maxFlowRate, which a zeroed sample passes. So the fabricated zero reaches terrainState.

On frequency: the range-sampling gate at AP_NavEKF3_Measurements.cpp:32 requires elapsed time strictly greater than 50 ms and is reached during fusion updates, so with uniform 12 ms updates sampling occurs every 60 ms — roughly one cycle in five. Entering the flow branch does not guarantee fusion, since the innovation checks still apply, so treat that as an upper bound on how often the zero actually lands.

Fix: OR the flag into cantFuseFlowData rather than gating the call.

ISSUE (pre-existing, not yours) — range-only calls try to fuse fabricated zero flow anyway

Same defect shape, already in the merge base: when the flow recall fails the vectors are zero-initialised, yet EstimateTerrainOffset() (AP_NavEKF3_OptFlowFusion.cpp:111) has no flow-availability check at all. A standalone probe reproduces the same terrain increase with no recalled flow sample. A single availability check would close both this and the bug above.

Notes

• LOG_ROFM_MSG is still mid-enum (AP_DAL/LogStructure.h:52), shifting six existing DAL IDs by one. This is @peterbarker's outstanding request and a one-line move. To be fair about the impact: I initially called it a bug, and it is not — Tools/Replay/LogReader.cpp:62 selects handlers from the FMT names and dispatches using the input format's ID, and a dispatch probe correctly selected all six handlers, so old logs still replay.
• The @Description rewrite is a real improvement and worth acknowledging: AP_OpticalFlow.cpp:110 now documents the actual floor (the larger of RNGFNDx_GNDCLR and 0.05) and states plainly that the substitution asserts zero motion rather than discarding the sample, with the commit message quantifying it (SITL reports 1.07 m/s against 5.19 m/s of truth below the floor). That kind of honesty about a model's limitation is what makes the parameter safe to use.
• Parameter is clean: FLOW + _HGT_MIN = 12 characters against the 16-character limit, no intervening subgroup prefixes, index 8 unique in the group, metadata/clamp/metre units consistent on both sides, default 0 so existing vehicles are unaffected. Checked arithmetically, and the ArduSub registration carries the same name.
• No test coverage for the new commit, and the existing Copter focus-height test structurally cannot provide it — it disables GPS and Copter defaults to navigation use, so it never exercises terrain-flow fusion. You say as much yourself. A Plane flight test is not the only option though: a unit/component test or a deliberately configured Copter scenario could cover this path.
• Two things I am dropping so you do not chase them: the earlier suggestion about uninitialised flowRadXY is wrong — Vector2's default constructor zeroes both components (AP_Math/vector2.h:50), so those are deterministic zeros, not stack garbage. And a draft note claiming that fixing the leak would make the parameter a no-op on Plane is also wrong — withholding an unusable terrain-flow observation is an effect. What is true, and worth a line in the description, is that Plane's default avoids the navigation-state zero-velocity update.
• CI is not usable as evidence at this head — snapshotted once with the great majority still queued or running, 0 failing.

Reviewed by Claude + an independent Codex cross-check: 7 confirmed, 9 adjusted, 2 refuted, 1 added.

[andyp1per]

This round was taken at 337cf08df6 and posted at 15:43Z; c08eaf0e43 has a commit date of 18:37Z, so both code findings were already fixed by the time it could be read. Triage at a204212074:

The terrain withhold bug is fixed, and the fix is the one this round proposes - flowDataToFuse is passed into EstimateTerrainOffset() and cantFuseFlowData opens with !flowDataToFuse, rather than the call being gated. It arrived via tridge's inline "possibly just set flowDataToFuse = false?" on 09-09, which removes the fabricated zero entirely, so there is no longer anything to withhold. The round is right that gating the call was never enough: EstimateTerrainOffset() is entered on rangeDataToFuse alone, which is exactly the Plane approach case the flag was written for.

The pre-existing availability gap goes with it. A failed recall() leaves flowDataToFuse false, so a never-recalled sample can no longer reach the flow branch as a deterministic zero - the single check the round asks for.

LOG_ROFM_MSG moved to the end of the list at da78928a9b, answered inline with the FMT-table numbers.

Test coverage for the terrain path is still missing, and I am not claiming otherwise. Copter defaults EK3_FLOW_USE to 1, so no Copter test reaches the terrain-flow branch.

One thing the round does not raise, which belongs on the record because it is where the head now differs from what was flown. Discarding is not free where zeroing was not: a zeroed sample was fused, which kept prevFlowFuseTime_ms advancing, and a discarded one is not, so at 5s without a flow fusion flowFusionTimeout (AP_NavEKF3_Control.cpp:308) drops the filter to constant position. attAidLossCritical is bounded by tiltDriftTimeMax_ms at 15s, so 5s is the binding one. The autotest sees that fallback only because it holds the floor far above any real sensor to keep it active long enough to measure. The argument that a realistic value is passed through well inside 5s is reasoning rather than a measurement, and the dwell time below a realistic floor on an approach is the next thing worth measuring.

(Posted by Claude.)

[andyp1per]

Thanks. The carry-over your 2026-10-05 review raised was real, and the reply that day missed it: the rest reading was only cleared at EKF init, so after the first flight a sensor that stored no flow sample on the ground kept the previous flight's reading. It is now cleared on disarm, and FlowFocusHoldAfterLanding has a two-flight leg without a reboot: with the reading kept, a 0.44-0.47 m hover under a 1 m floor fused zero flow 40-45 times; cleared, the flow is discarded and aiding stops.

• Replay at every commit: the handle_message(ROFH) change is now its own AP_DAL commit after the EKF3 one, and copter and Replay build at all 18 commits.
• cb90789's message now gives the current reason for ROFM: log_ROFH ends in a uint8_t, so an appended float brings padding and SITL rejects the structure at boot (44 != 47), while inserting it before the quality makes older logs replay with quality 0.
• The FLOW_HGT_MIN text now says it assumes the rangefinder reads no more than 0.05 further at landing than before that flight's takeoff.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-10-11)

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

Reviewed at head f9cae55929.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34292/10.html#pr34292

Re-reviewed at f9cae55. The head has moved since my last review at 5240bea: same base (1f1251a), now 18 commits, with four files changed (the disarm reset in AP_NavEKF3_Control.cpp, a comment in AP_NavEKF3_core.h, the FLOW_HGT_MIN description and the new leg in arducopter.py). Nothing newer than f9cae55 is covered, and CI was still pending when this was pinned. Nothing here is blocking.

Thanks for the 2026-10-11 reply. Each point checks out at this head:

• The rest reading is cleared on disarm (AP_NavEKF3_Control.cpp:20), and the two-flight leg (arducopter.py:18303) passed 5 SITL runs out of 5 with 0 flow fusions in the second hover. With the three reset lines removed it failed with 43, in line with your 40-45.
• Copter and Replay build at all 18 commits.
• You are right about ROFH. An appended float gives a 47 byte record against 44 from the format, and my last note that an appended field would simply replay as 0 missed the padding. 121a9b6's message is correct.
• The FLOW_HGT_MIN text now states the range-reading assumption.

One remaining point: a reading learned before the first arming can go stale (AP_NavEKF3_OptFlowFusion.cpp:77). The reading is only refreshed from a stored flow sample while disarmed, and the disarm reset does not fire before the first flight. So if the sensor stores a sample with the range under the floor, then stops storing them (quality 0, or a rate over 4.2 rad/s), and the range on the ground then drops before arming, the old reading is kept. Any hover below the floor that reads at or below the old value plus 0.05 then counts as resting and fuses zero flow (:93). A SITL probe at this head, built like your two-flight leg but without the first flight, shows it: 0.79 m learned after boot, flow samples stopped, ground then reading 0.00 m, and a 0.38 m hover under a 1.0 m floor fused zero flow 33 times with aiding kept (25 on a second run). With samples left running the same hover fused none and aiding stopped. Your 2026-10-05 note about falling back to on_ground() covers a sensor that never stored a sample. This is one that stored a sample earlier in the same disarmed period. It needs a non-default FLOW_HGT_MIN and only acts below the floor, so I'm not treating it as blocking. One way to close it is to clear the reading while disarmed once no flow sample has been stored for about a second. I have not tested that change.

Unchanged notes, no change asked:

• A landing that reads more than 0.05 above both RNGFNDx_GNDCLR and the reading before takeoff still discards flow below the floor (:90). The description now documents this (AP_OpticalFlow.cpp:110), and accepting it is a maintainer call.
• With two downward range finders, DAL on_ground() uses the orientation-wide clearance (AP_DAL_RangeFinder.cpp:116) and the learned reading has no sensor index. EKF3's existing rngOnGnd uses the same clearance. The parameter text does not mention this case.
• rofm_resend is still not serialized with the flow write path (AP_DAL.cpp:361), as with base's unlocked ROFH writes.

Not covered: hardware, your flight logs, CI, ChibiOS builds and the full Copter suite. The two limits and the resend ordering were checked by reading and by small probes built from the source, not flown.

[andyp1per]

Thanks, confirmed and fixed in 16506a6: while disarmed the rest reading is now also cleared once no flow sample has been stored for a second, alongside the disarm clear; it is a commit on top rather than squashed, since the commits below it have flown. FlowFocusHoldAfterLanding has your probe as a second variant of the stale-reading leg (9e086e9): 0.79 m learned after boot, samples stopped, ground then 0 m, and a 0.42-0.44 m hover under a 1 m floor fused zero flow 42 and 39 times without the new clear, 0 with it, with aiding stopped.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-10-13)

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

Reviewed at head 9e086e9d5d.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34292/11.html#pr34292

Re-reviewed at 9e086e9. The head has moved since my last review at f9cae55: the same 18 commits plus 16506a6 and 9e086e9 on top, same base (1f1251a), now 20 commits. Nothing newer than 9e086e9 is covered, and CI for this head was still queued when I checked. Nothing here is blocking.

Thanks for the 2026-10-12 reply. The fix checks out. With the new clear (AP_NavEKF3_OptFlowFusion.cpp:78) FlowFocusHoldAfterLanding passed 3 SITL runs of 3 with 0 flow fusions in both stale-reading hovers. With those three lines removed it failed 2 of 2 in the after-boot leg with 37 and 44 fusions, in line with your 42 and 39, while the first-flight leg still passed. That point is closed.

The test's low climbs can miss their altitude band on a loaded machine (arducopter.py:18337, same pattern at :18228). Each climb waits for one SIM_STATE.alt sample between 0.25 and 0.5 m, and each sample is a poll_message round trip. When the harness runs slow against the simulation, one late poll steps over the band, the vehicle keeps climbing and the wait ends in 'Failed to attain Altitude'. The new after-boot leg makes it three such climbs. With the box quiet the test passed 14 runs of 14. With busy-loop CPU load added it failed 6 runs of 78, all from line 18337; in one, the progress lines a simulated second apart read 584.29 and 584.69, either side of a 584.34 to 584.59 band. A variant that sets SIM_SPEEDUP to 5 for just the climb and restores it after the hover throttle passed 40 of 40 alternated with the unmodified test under the same load, and still failed on a build without the new clear. I have not seen this fail in CI, and the load was artificial, so treat it as a possible flake rather than a known one.

The PR description is out of date in three places. It says FlowFocusHoldAfterLanding shows relative aiding stopping once on the ground and staying off, but the test now fails if aiding stops through the landing (arducopter.py:18208). 'Aiding that times out while the vehicle sits on the floor stays off' describes the behaviour before zero flow was fused at rest. The ROFM paragraph still gives the read-past-the-record reason, where 121a9b6's message has the padding one. It also does not mention zero flow on the ground, the learned rest reading or its two clears. The commit messages are current.

Unchanged notes, no change asked:

• A landing that reads more than 0.05 above both RNGFNDx_GNDCLR and the reading before takeoff still discards flow below the floor (:96), as the FLOW_HGT_MIN text says (AP_OpticalFlow.cpp:110). The new clear adds one way to have no reading before takeoff: no flow sample stored in the last second before arming.
• With two downward range finders the learned reading has no sensor index and DAL on_ground() uses the orientation-wide clearance (AP_DAL_RangeFinder.cpp:116). Recording the sensor index with the reading would close half of it without a new Replay field.
• rofm_resend is still not serialized with the flow write path (AP_DAL.cpp:361), as with base's unlocked ROFH writes.
• Keeping 16506a6 and 9e086e9 on top rather than squashed is fine by the commit rules; whether to squash before merge is a maintainer call.

Not covered: hardware, your flight logs, CI at this head, the full Copter suite, a two range finder flight and a real log restart and Replay cycle. The three notes were checked by reading and by a small probe built from the source, not flown.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-10-13)

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

Reviewed at head 8cd64105c4.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34292/12.html#pr34292

Re-reviewed at 8cd6410. The head has moved since my last review at 9e086e9: still 20 commits on the same base, and the only change is the slowed climb in arducopter.py. Nothing newer is covered, and CI for this head was still queued when I checked.

Thanks for taking the slowed climb. Under added CPU load, alternated run for run on one binary, the previous test file failed 12 of 63 and this one 1 of 62, and it still fails 2 of 2 with the ground-reading clear reverted. That point is closed. The one remaining failure was the same step over the band at speedup 5 with the load average near 40; I am not asking for anything more there.

One new point, and it is blocking: an armed Rover with a downward range finder fuses zero flow while it drives (AP_NavEKF3_OptFlowFusion.cpp:114). I missed this in the earlier rounds, where I only traced the Copter callers; the code is unchanged since then. Rover sets VehicleClass::GROUND (Rover/system.cpp:316), so an armed Rover has onGround false (AP_NavEKF3_VehicleStatus.cpp:379) and takeOffDetected latches on its first turn over 0.1 rad/s (:476). The ground check then runs with FLOW_HGT_MIN at 0, because the floor is at least rngOnGnd + 0.05 (:87), and on_ground() is true whenever the range finder reads within 0.05 of RNGFNDx_GNDCLR (:94). On a Rover that is all the time, so every flow sample is zeroed and fused.

In SITL, a flow-only Rover (EK3_SRC1_VELXY 5, FLOW_HGT_OVR set, FLOW_HGT_MIN 0) with a downward range finder reading Good at its ground clearance, driven in a circle at 0.40 m/s, read 0.03 m/s from EKF3 at this head (2 runs of 2) and 0.40 m/s on the merge base (2 of 2). It stays in relative aiding and reports the vehicle as still. At this head it read 0.40 with no range finder, 0.40 with the range finder reading well above its ground clearance, and 0.40 with the block skipped for VehicleClass::GROUND. For the probe I had to patch the SITL flow model, which gives no translational flow at zero height, and I used a 1.5 m sensor height to stay under the flow rate limit, so this is one artificial geometry and not hardware. The FLOW_HGT_MIN text does say zero flow does not suit a vehicle that drives on the ground, but the ground clearance check applies at the default, so an existing Rover setup changes without setting anything. Skipping the check for ground vehicles (or when FLOW_HGT_OVR is in use) and a Rover test would cover it.

The PR description is still out of date in the same three places as last time: FlowFocusHoldAfterLanding 'stops once and stays off' (the test now fails if aiding stops, arducopter.py:18219), aiding that 'stays off' on the floor, and the read-past-the-record reason for ROFM where 121a9b6's message has the padding one. It also does not mention zero flow on the ground, the learned ground reading or its two clears.

The notes from last time are unchanged and need no action: the landing reading more than 0.05 above both references (:96), two downward range finders sharing one learned reading and clearance (AP_DAL_RangeFinder.cpp:116), and rofm_resend not serialised with the flow write path (AP_DAL.cpp:361).

Not covered: hardware, your flight logs, CI at this head, the full Copter and Rover suites, a two range finder flight and a real log restart and Replay cycle. The Rover result is from one SITL setup with a review-only sensor model change.

[andyp1per]

Thanks, reproduced: with the autotest and 8 busy loops pinned to four cores the unmodified test failed 4 of 24, at all three climbs, each time one sample just off the ground and the next at 0.51-0.56 m. The climbs now share a helper that drops SIM_SPEEDUP to 5 until the hover throttle, as you suggested: 20 of 20 under the same load, 6 of 6 without, and still a fail with 44-48 fusions without the one-second clear or without both clears.

[andyp1per]

Reproduced (a flow-only Rover with FLOW_HGT_OVR 1.5 and a downward range finder at its ground clearance read 0.01 m/s while driving at 0.39 m/s) and fixed in 244d608: a flow sample with a declared sensor height now skips the focus floor and clears its flags, 0.40 m/s in OpticalFlowRangefinderOnGround (c0498ea; a0ab050 lets the SITL flow model see a sensor mounted above a vehicle at zero height). The VehicleClass condition was not used because the EKF keeps no vehicle-specific code; FLOW_HGT_OVR is what a ground vehicle already declares, and a Rover with a downward range finder but no FLOW_HGT_OVR stays the stated limitation in the description.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-10-14)

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

Reviewed at head c0498ea0d5.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34292/13.html#pr34292

Re-reviewed at c0498ea. The head has moved since my last review at 8cd6410: three new commits (244d608, a0ab050, c0498ea) on the same base, 23 in all, changing AP_NavEKF3_OptFlowFusion.cpp, AP_OpticalFlow_SITL.cpp and rover.py. Nothing newer is covered, and CI for this head was still queued when I checked (76 of 77 check runs at 2026-10-14T00:44Z).

Thanks for the fix and the test. The setup I measured last time is fixed: with FLOW_HGT_OVR 1.5 the new OpticalFlowRangefinderOnGround reads 0.40 m/s against 0.39 here, and 0.01 with 244d608 reverted, which are your numbers. The description is now up to date in every place I listed, so that point is closed, and so is the slowed climb.

Still blocking in my view: a flow Rover with a downward range finder and FLOW_HGT_OVR left at 0 fuses zero flow while it drives (AP_NavEKF3_OptFlowFusion.cpp:82). First my part in this: last time I wrote "ground vehicles (or when FLOW_HGT_OVR is in use)" as if either would do. They are not the same, and you took the one that covers the setup I had measured. Without a declared height the block still runs: the floor is at least rngOnGnd + 0.05 (:91), on_ground() is true at the ground clearance (:98), and the flow is zeroed and fused (:121). Your test with only FLOW_HGT_OVR changed to 0 reads 0.01 m/s at this head (3 runs of 3) and 0.40 on the merge base (3 of 3); the merge base does not need FLOW_HGT_OVR there because the range finder gives the EKF its height. A small-rover geometry, both sensors 0.14 m up with RNGFND1_GNDCLR at its default 0.10, reads 0.00 at this head and 0.43 on the merge base (2 of 2 each). I also tried a speed-controlled mode this time: in ACRO, at a stick position that holds 1.00 m/s on the merge base and at this head with FLOW_HGT_OVR set, this head with FLOW_HGT_OVR 0 drove at 1.83 m/s (2 of 2).

On the EKF keeping no vehicle-specific code: EKF3 already tests the vehicle class in assume_zero_sideslip() (AP_NavEKF3_Control.cpp:714), and the use of heightOverride is itself inside #if APM_BUILD_TYPE(APM_BUILD_Rover) (AP_NavEKF3_OptFlowFusion.cpp:416). The class reaches the EKF through the DAL's RFRN record, so Replay follows it. In a review-only build, testing dal.get_vehicle_class() == AP_DAL::VehicleClass::GROUND on line 82 in place of the override read 0.40, 0.40 and 0.43 m/s in the three Rover setups (your test, FLOW_HGT_OVR 0, small rover). If you would rather keep this as a limitation, that is a maintainer's call, but then it needs to reach users and not only the PR description: FLOW_HGT_OVR's own text (AP_OpticalFlow.cpp:94) does not say a Rover with a downward range finder now has to set it. Either way a leg of the Rover test with FLOW_HGT_OVR at 0 would pin it (rover.py:7400). All of this is SITL with simulated sensor heights, not hardware.

Smaller, not blocking: FLOW_HGT_OVR set on a Copter now switches the focus floor off. The parameter is hidden off Rover but can still be set by name, and line 82 does not look at the vehicle. Before this head the override did nothing off Rover. With FLOW_HGT_OVR 1.5 added to OpticalFlowFocusHeight, the floor-active subtest reached 1.97 m/s where the test allows 0.5 (3 of 3), against a pass unmodified (2 of 2). The vehicle class test above passes that variant too (2 of 2), so the one condition would cover both.

Smaller, not blocking: the FLOW_HGT_MIN text says the ground clearance check applies "whatever this is set to" (AP_OpticalFlow.cpp:110). With a declared height neither that check nor FLOW_HGT_MIN applies: FLOW_HGT_OVR 1.5 with FLOW_HGT_MIN 2.0 reads 0.40 m/s on Rover. One clause would cover it, whichever condition line 82 ends up with.

Notes, no action needed. The new Rover test does not check that the range finder reads its ground clearance (rover.py:7394), so with the fix in it passes whatever the range finder reads. The three notes from last time are unchanged: the landing reading more than 0.05 above both references (:100), two downward range finders sharing one learned reading and clearance (AP_DAL_RangeFinder.cpp:116), and rofm_resend not serialised with the flow write path (AP_DAL.cpp:361).

CI at the previous head finished with one failure, sitltest-copter-tests1e on Copter.MaxAltFence, a test with no flow or range finder. That group passed whole here at this head, so I read it as unrelated, though I could not read the job log.

Not covered: hardware, your flight logs, CI at this head, the full Copter suite, ChibiOS and other vehicle builds, a two range finder flight and a real log restart and Replay cycle. The full Rover suite ran once at this head: 135 passed and 3 failed, none of which I attribute to this PR, though one (NetworkingWebServerPPP) has no merge-base control.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-10-16)

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

Reviewed at head b582cc7fe7.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34292/14.html#pr34292

Re-reviewed at b582cc7. The head has moved since my last review at c0498ea by three commits (b43ace7, a0e43dc, b582cc7) on the same base, 26 in all. Nothing newer is covered, and CI at this head was still pending.

Thanks, the blocker is fixed. With the floor left out of Rover builds, OpticalFlowRangefinderOnGround passes 3 of 3 here, reading 0.40, 0.40 and 0.43 m/s against 0.39 for declared height, FLOW_HGT_OVR 0 and the small rover. With b43ace7's EKF hunk reverted it fails at 0.01. The new range-at-clearance check (rover.py:7441) closes my note that the test passed whatever the range finder read. FLOW_HGT_OVR on a Copter no longer turns the floor off (OpticalFlowFocusHeight with it set to 1.5 passes), and the FLOW_HGT_MIN text is accurate for the vehicle again. Those points are closed.

One point, not blocking: Replay of a Rover flow log diverges (AP_NavEKF3_OptFlowFusion.cpp:56). Your commit message says Replay is its own build and runs the floor. Here is what that does. I replayed a SITL Rover log (flow, range finder at its 1.5 m ground clearance, FLOW_HGT_OVR 0) with the head's Replay. check_replay gave 104133 mismatches, and the replayed EKF read 0.02 m/s against 0.39 live. With the block also left out of Replay it matched exactly. The vehicle is unaffected, and the existing Rover-only heightOverride already has the same Replay gap, so I am not treating this as blocking. But it now hits the default flow-plus-range-finder Rover setup. A runtime dal.get_vehicle_class() != AP_DAL::VehicleClass::GROUND test, inside your existing guard if you want to keep the flash saving, would follow the logged class (as assume_zero_sideslip() does). I have not built that at this head. If you would rather leave it, a note in the commit or parameter text is a maintainer's call.

The three earlier notes are unchanged and need no action: the landing reading more than 0.05 above both references (:97), two range finders sharing one learned reading and clearance (AP_DAL_RangeFinder.cpp:116), and rofm_resend not being serialised (AP_DAL.cpp:361).

Not covered: hardware, your flight logs, CI at this head, the full Copter and Rover suites, ChibiOS and Plane/Sub builds, a two range finder flight and a real log restart. Everything above is SITL with simulated sensor heights.

[andyp1per]

Replay now follows the logged vehicle class, compiled into Replay alone (3917ef3), so a Rover log replays without the floor as Rover ran it; vehicle builds are unchanged and keep the compile-time exclusion rather than a run-time class test. On three SITL Rover flow logs the replayed speed was 0.01-0.04 m/s against 0.31-0.34 flown before, and matches to 0.000 m/s and 0.00 m after; the log with FLOW_HGT_OVR set still differs by 0.02 m/s, from the existing Rover-only override, which Replay does not build. check_replay flagged every message on these logs either way, as the replay's first frame lands one frame after the flight's, so the comparison matches XKF1 by TimeUS.

[AP-Review]

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

Reviewed at head 3917ef38f2.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34292/15.html#pr34292

Re-reviewed at 3917ef3, one commit after b582cc7 on the same base (27 in all). Nothing newer is covered, and CI at this head was pending when this review started.

Thanks, the Replay point is fixed. With the class test in Replay, I rebuilt Replay at this head and replayed the same SITL Rover flow log that gave 104133 check_replay mismatches at b582cc7. It now passes with 0 errors over 34649 messages. A Copter log flown under FLOW_HGT_MIN still replays with the floor: it gives 0 field errors, where a Replay with the floor forced off gives 17834 mismatches and a 1.42 m/s phantom. The Rover and Copter flow tests pass at this head, and outside Replay the extracted function's code is unchanged.

On your check_replay remark: on my log, recorded from boot with LOG_REPLAY 1, check_replay passes outright at this head, and every live XKF1 timestamp appears in the replay. So it can confirm this fix on a log like that. I can't tell why your logs start a frame late, because I don't have them. No change requested.

The three earlier notes are unchanged and need no action: the landing reading more than 0.05 above both references (:103), two range finders sharing one learned reading and clearance (AP_DAL_RangeFinder.cpp:116), and rofm_resend not being serialised (AP_DAL.cpp:361).

Not covered: hardware, your logs, CI at this head, a FLOW_HGT_OVR Rover log at this head, the full suites, ChibiOS and Plane/Sub builds, a two range finder flight and a real log restart. Everything above is SITL with simulated sensor heights.