[peterbarker]

Summary

Bound the three ICP201XX initialisation waits by elapsed time, so a sensor that answers the ID probe and then stops responding, or never fills its FIFO, fails its probe instead of hanging the boot for ever.

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

• Checked by a human programmer
• Non-functional change
• No-binary change
• Infrastructure change (e.g. unit tests, helper scripts)
• Automated test(s) verify changes (e.g. unit test, autotest)
• Tested manually, description below (e.g. SITL)
• Tested on hardware
• Logs attached
• Logs available on request

Tested under Renode on YJUAV_A6SE_H743. The emulated ICP201XX in the harness reports a FIFO fill of 1, so it can never satisfy the driver's first wait, which makes it a faithful stand-in for a part that probes and then never fills.

Test	Parent commit	This commit
Renode, YJUAV_A6SE_H743 Copter, time to first MAVLink heartbeat	none within 180 s	32 s
Firmware builds compared		differ only by this commit
YJUAV_A6SE_H743 Plane and Copter build	pass	pass
MatekH743 Copter build	pass	pass
SITL Plane build	pass	pass
Tools/scripts/check_branch_conventions.py		all checks pass

This has not been exercised against a real failing part. The fault was found in emulation, and on hardware the sensor has always come up normally.

Description

Three waits reachable from AP_Baro_ICP201XX::init() had no exit:

• mode_select() polled the device-status register until its sync bit was set.
• read_otp_data() polled the OTP status register until it read zero.
• wait_read() polled FIFO_FILL until 14 packets had arrived, and again after a flush until one had.

Each loop inspected only the value it polled and never whether the read succeeded, so a part that answers the ID probe and then stops acknowledging, or keeps acknowledging but never fills its FIFO, holds init() there indefinitely.

Nothing recovers from that. On every board except those defining HAL_EARLY_WATCHDOG_INIT, currently only Here4FC, setup() runs before the watchdog is started at all, so there is nothing to fire. Where the watchdog is started early, the scheduler keeps patting it until the system is marked initialised. Either way the board sits there with no MAVLink, no console message and no reboot, which is a much worse failure than a missing barometer.

Each wait now gives up after a fixed elapsed time and returns false: 100 ms for mode sync, 50 ms for an OTP read, and 2 s for each FIFO wait. wait_read() returns bool, init() takes its existing failed path on a timeout, and boot_sequence() now checks the mode_select() results it previously ignored. The probe fails cleanly and the other barometers on the board carry on.

Why elapsed time and not a count of polls. Each read_reg() can take several times the sleep between polls. The I2C layer defaults to three attempts per transaction with a 4 ms minimum timeout, and init() only calls set_retries(0) after everything has succeeded, so during initialisation a part that clock-stretches or holds SDA makes every poll expensive. A limit on the number of polls would therefore understate how long a dead part can block the boot. Bounding by elapsed time makes the stated figures real ceilings, give or take the duration of the last bus read in each loop.

Worst case. A failed mode_select() returns at once, the three OTP reads run as a group, and the two FIFO waits dominate. A dead part can now hold the boot for at most about four seconds, if it satisfies the first FIFO wait just before its limit and then fails the second.

Where the FIFO limit comes from. The driver configures OP_MODE1, which runs at 120 Hz, so the 14 settling packets take about 117 ms and the first packet after a flush about 8 ms. Two seconds is a wide margin for both. OP_MODE3 runs at 2 Hz and would need about 7 s just to settle, so the constant would have to be raised if the configured mode were ever slowed. The comment on the constant says so, so the next person to change _op_mode finds it.

Points worth knowing:

• The emulated sensor still does not register with this PR alone. It changes the emulated board from hanging to failing the ICP201XX probe after the timeout. Getting the emulated part to fill its FIFO and actually register as a barometer is the harness model fix in Tools: renode: Allow servo outputs to be capture in PulseView #34357.
• A pre-existing bug nearby is deliberately left alone. In boot_sequence() the OTP boot-done flag is written only when reading its status register fails, so on the success path it is never set and the OTP boot sequence re-runs on every boot of an A1 part. That predates this change and is not a hang, so it belongs in its own PR.

This PR was AI-assisted.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-10)

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

Reviewed at head 9eb8b2bc9b. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_10_AIReview/devcall_pr_reviews.html#pr34356

No blockers. The change is correct and worth merging — contrary to your own "may not be worth merging" note, an unbounded poll on a probe path is a genuine brick-the-board hazard, and CI is 101/101 green. An independent cold Codex pass returned APPROVE. Two things are worth fixing first, both about the justification rather than the code.

1. ISSUE — the bounds are on iteration count, not elapsed time, so the "100ms / 10ms / 2s" figures are floors, not ceilings

libraries/AP_Baro/AP_Baro_ICP201XX.cpp:201, :231, :441-442

Every iteration calls read_reg(), which is two I2C transactions, and during init() the device still carries the default retry count — dev->set_retries(0) is only reached at :130, after everything has already succeeded. I re-derived the constants from source rather than assuming them:

• I2CDevice::_retries(2) — libraries/AP_HAL_ChibiOS/I2CDevice.cpp:265
• for (uint8_t i=0; i <= _retries; i++) → 3 attempts per transaction — I2CDevice.cpp:372
• per-attempt timeout 1+2*(((8*1000000UL/busclock)*(send_len+recv_len))/1000) then MAX(..., _timeout_ms) — I2CDevice.cpp:375-376. With the defaults bus_clock=400000, timeout_ms=4 (AP_HAL/I2CDevice.h:75,78) and send_len+recv_len=2, that is MAX(1, 4) = 4 ms.

So one read_reg() against a part that clock-stretches or wedges SDA costs up to 6 x 4 = 24 ms:

loop	iterations x delay	worst case incl. I2C	commit message says
read_otp_data() :231	1000 x 10 us	~24 s, x3 calls from boot_sequence() = ~72 s	10 ms
mode_select() :201	100 x 1 ms	~2.5 s, x4 call sites = ~10 s	100 ms
wait_read() x2	200 x 10 ms each	~6.8 s each = ~14 s	2 s each

About 95 s of blocked boot worst case against the ~2.2 s implied. It is still finite, so the PR's goal is met and this is not a regression. The cold pass reached the same conclusion independently, phrasing it as "the attempt caps are finite, but their delay totals exclude bus transactions, retries and scheduling overhead."

Honest caveat: a device that simply NAKs returns MSG_RESET promptly, so 24 ms is the timeout case, not the NAK case. I verified the constants and the formula from source; I did not measure on hardware.

Suggested fix: switch the three loops to an elapsed-time bound — the idiom this very library already uses two files away, AP_Baro.cpp:333-338's do { ... } while (AP_HAL::millis() - tstart < 500), which is also overflow-safe by construction. Cheaper alternative: move dev->set_retries(0) to the top of init(), which cuts the worst case 3x.

2. ISSUE — the ODR justification in the comment and commit message is wrong on both counts

libraries/AP_Baro/AP_Baro_ICP201XX.cpp:437-438 — "At the slowest ODR (25Hz) 14 packets take 560ms; allow 2s ..."

• 25 Hz is not the slowest ODR. AP_Baro_ICP201XX.h:54 documents OP_MODE3 as Bw:0.5 Hz ODR: 2Hz (25 Hz is OP_MODE0). At 2 Hz, 14 packets take 7 s — 3.5x over the new 2 s bound, so a perfectly healthy sensor in that mode would fail init.
• 25 Hz is not the rate the driver uses. _op_mode{OP_MODE::OP_MODE1} at :56 is Mode 1, ODR 120 Hz, and configure() shifts it into the MEAS_CONFIG bits. 14 packets at 120 Hz is 117 ms, so the real margin is ~17x, not the "more than three times" claimed.

The 2 s bound is fine for the mode actually configured. It is the written reasoning that is wrong, and it is exactly the thing that misleads whoever next changes _op_mode. Both passes flagged this independently.

Suggested fix: cite the configured mode ("_op_mode is OP_MODE1, ODR 120 Hz, so 14 packets take 117 ms; 2 s gives ~17x margin"), and either note that OP_MODE3 would need a larger cap or derive max_polls from _op_mode.

3. NOTE — the Renode ICP201XX model can never satisfy wait_read(), and the Renode fix your description mentions is not in this diff

Tools/renode/peripherals/sensors/AP_AdditionalBarometers.cs:852-855 returns FIFO_FILL as a hardcoded 1, and 1 & 0x1F = 1 while the first loop needs >= 14. The model also presets version 0xB2 (so boot_sequence() early-returns) and mode-sync set, so init reaches wait_read() and can never leave it. Post-PR the matekh743-icp201xx scenario goes from hanging to failing the probe after 2 s — the emulated baro still never registers. No CI risk either way, since test_renode.yml does not run that scenario. Your description says "A fix the the renode simulation of this sensor also allows renode to build", but the diff is only the two AP_Baro files — was the model fix meant to be here?

4. NOTE — pre-existing inverted condition four lines above your change

libraries/AP_Baro/AP_Baro_ICP201XX.cpp:385-391 writes the OTP boot-up-done flag only when the status read failed — and since ret is already 0 there, boot_sequence() returns false regardless. On the success path the flag is never set, so the OTP boot sequence re-runs on every boot of an A1 part. Not touched by this PR and not a hang; flagging it only because it is adjacent to a line you changed. (int ret = 1; with &= on bools would also read better as bool.)

5. NOTE — style, and one commit-message overstatement

mode_select() uses a bare 100 and read_otp_data() a bare 1000, while wait_read() correctly names fifo_packets_to_skip / max_polls. const uint8_t max_polls = 200 with a uint8_t counter is close to the type's limit — bumping it past 255 silently gives zero iterations. And "a failed I2C read leaves the polled value untouched" is not strictly guaranteed: I2CDevice::_transfer() calls bus.bouncebuffer_finish(...) on the failure path too (I2CDevice.cpp:422), and bouncebuffer_finish_read() copies the bounce buffer into the caller's buffer when one is in use. It does not change the analysis — the new code checks the return value — but the sentence overstates it.

What I checked and found correct

I enumerated every loop reachable from init(): dummy_reg() is do{}while(0); mode_select, read_otp_data and both wait_read loops are now capped; flush_fifo() and get_sensor_data() have none. A grep for while(1)/while(true)/for(;;) across libraries/AP_Baro/*.cpp returns nothing else, and the only other do..while in the library is already time-bounded — so the fix is complete and no sibling driver shares the pattern. The semaphore is given on every goto failed; register_sensor() is only reached after wait_read() succeeds, so nothing half-registers; probe() deletes the backend and AP_Baro::probe_dev() deletes the device. hal.scheduler->delay_microseconds() is chThdSleep, not a spin, so the loops yield, and the new code uses no millis()/micros() at all, so there is no wrap exposure.

One low-priority nit on the failure path: on a wait_read() timeout the part is left in whatever active mode configure() put it in, drawing current for the rest of the flight. A mode_select(0x00) there would return it to standby.

Verdict: COMMENT — no blockers

[tridge]

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

Re-reviewed at head 25b674928d (you were last told 9eb8b2bc9b); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_13_2247/devcall_pr_reviews.html#pr34356

No blockers. The verdict moves to APPROVE (it was COMMENT). Both issues from my previous comment are fixed, and so are the style points. What's left: the pre-existing bug you've deferred, a nit, and a small note on the PR description.

Resolved

• 1. The bounds counted iterations, not time. All four waits are now do { ... } while (!ok && AP_HAL::millis() - start_ms < TIMEOUT), at libraries/AP_Baro/AP_Baro_ICP201XX.cpp:219-225, :249-254, :463-467 and :478-482.
  • I built a host harness around this PR's unmodified .cpp. With a part that stops answering after the ID check and 12 ms per transfer, probe() fails after 211 ms.
  • The parent commit never returns on the same input. Neither does this head with only the mode_select() time bound removed.
  • The loops always poll at least once, and a success on the last poll counts.
  • The uint32_t subtraction is wrap-safe: timings are the same with millis() wrapping mid-run.
  • start_ms is reset for the second FIFO wait, and nothing spins without sleeping.
• 2. The ODR justification. :48-54 now cites OP_MODE1 at 120 Hz. The numbers check out: 14 packets ≈ 117 ms, the first packet ≈ 8 ms, and OP_MODE3 ≈ 7 s.
• 3. Renode. The model isn't changed here, but the description now says so and points to Tools: renode: Allow servo outputs to be capture in PulseView #34357. That PR's AP_AdditionalBarometers.cs change makes FIFO_FILL grow per poll and reset on a flush (I read it but didn't run it). Tools: renode: Allow servo outputs to be capture in PulseView #34357's title doesn't mention that fix, which may surprise its reviewers.
• 5. Style and the commit-message overstatement. The timeouts are named macros, the uint8_t counter is gone, and so is the "leaves the polled value untouched" sentence.

Still open (not blocking)

• 4. The inverted OTP boot-done condition is still at libraries/AP_Baro/AP_Baro_ICP201XX.cpp:402-408 (if (!ret)).
  • You've deferred it to its own PR, which is fine.
  • A cold Codex pass that didn't read this thread found it independently.
• Nit: a wait_read() timeout still reaches failed: (:143-145) without mode_select(0x00). A part that answers but never fills its FIFO therefore stays in continuous mode.

New (optional)

• NOTE — the description's "at most about four seconds" is a little low for the worst case.
  • I scripted a part that passes each wait just before its limit and then never refills. It takes 4.4 s on a healthy bus, but 5.7 s at 12 ms per transfer. Codex measured 5.77 s independently.
  • The extra comes from about 40 unbounded register accesses in soft_reset()/boot_sequence(), plus one poll of overshoot per loop.
  • Only the description makes this claim, not the code or the commit message. It is still about 16x better than the ~95 s before.
• NOTE unconfirmed — the 50 ms OTP and 100 ms mode-sync limits (:46-47) weren't checked against the datasheet or on hardware. They look generous for OP_MODE1.

Also checked:

• The commit message's watchdog claim holds: only Here4FC sets HAL_EARLY_WATCHDOG_INIT, and the timer thread pats until _initialized.
• A YJUAV_A6SE_H743 copter build, a board with an ICP201XX, compiles the driver with no warnings.

CI at this head: 101 passing, 0 failing.