Repository navigation
Conversation
busReadBufAsync() starts an I2C read and busAsyncState() tells when it has ended, so the barometer task no longer spins on the bus. F4 steps its polled state machine from the realtime callbacks, F7/H7 use the HAL interrupt driven read and its existing handlers, AT32F43x gets a small polled state machine. A blocking transfer on the same bus lets the read finish first. DPS310 and BMP280 use it; on SPI and software I2C the read completes at once.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoRead barometers without blocking on supported I2C buses
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
1.
|
|
Test firmware build ready — commit Download firmware for PR #12063 251 targets built. Find your board's
|
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
af3ee38 to
e2302d4
Compare
|
Tested today on a second board and barometer: a Holybro Kakute H7 Mini with its BMP280 on I2C1, this branch merged on the current maintenance-10.x against that base alone, 12
One thing for anyone reading Earlier on the Lucid's DPS310 the BARO task measured 3 us. F4 and AT32 still untested. |
…othing else carries them
|
Ran on an F7 now: a Diatone Mamba F722 2022B with a DPS310 on I2C1 (
|
|
It looks like this and #12108 are covering the same issue? It may be useful to cross-reference and see if each PR offers any different that may be useful. Having not read the code yet, my prediction would be that the two differ in some ways and a combination would be best. |
|
Yes, same problem. #12108 does it for every I2C sensor and for writes too, mine only for baro reads, so I'd take #12108 as the base and close #12063 once we've sorted out what's worth keeping. A few bits of mine might still help. On F4 I kept the existing polled driver and just step it from the realtime callbacks, which could be a fallback if the new interrupt driver gives trouble, since nobody has run that one on an F4 yet. On AT32 my read doesn't block during the address phase. And my timeout only counts the time the transfer is actually being stepped, so a long gap in the main loop doesn't kill a good read. Two things in #12108 worth a look, @error414:
If you want the F4/AT32/timeout parts I can send them as a PR against your branch, and I'll rerun my tests on #12108 on the H743 and the F722. |
|
Ahhh, I did not notice that similar PR is already created. You PR just change baro and my PR covers whole. Probably would be best to go somewhere beetween. In INAV is every I2C driver blocking and must be rewrite all. But do it in one big PR is not best way. I did not F4 tested yet because I have to buy better F4 board with exposed SWD pins :(. So my proposal is:
what we have to be sure about
First step and first PR could be core + baro + mag (qmc) + OLED, these devices I can test. Could you check https://github.com/error414/inav/tree/error414/feature/ic_async/core each driver could use small state machine if needed (only steps if needs read more registers), in driver just call busReadBufStart or busWriteStart, end of transfer is handled in parent of driver for example compas.c, baro.c etc..... for special drivers like OLED we would need create parent task to handle end of transfer. So far I don't know if I like this aproach. But it's simple neat solution which can be used for every driver. And so far I did not checked memory usage of my solution. What do you think? BTW: so far I did not pay attention to new changes in SPL06 driver |
|
@error414 I like the plan: a core first, then the drivers one at a time, so each PR stays small enough to review and test. I went through your core branch (a7d4a0e) with your two goals in mind. Two devices on one bus look fine from the code: one transfer per bus, a result kept per address, and blocking calls wait for the bus to go idle first. The progress-based timeout and keeping barometer_spl06.c are good changes compared to #12108. A failing device can still hurt the others in three places:
Smaller things: the AT32 address phase still busy-waits; the doc says a NACK doesn't reinit the peripheral, but AT32 and HAL (other than HAL_BUSY) do; and nothing checks the BMP280 trigger write, so a NACKed trigger followed by a good read gives a stale sample. From #12063, the AT32 read that also does the address phase without blocking would cover the AT32 points, and I can bring it over. I can also test the core on an H743 (DPS310, BMP280) and an F722 (DPS310) with the same tests I used for #12063, the NACK one included. I have no QMC, OLED or F4 here. |
|
I just wanted to show direction how to make it universal. In specific driver just call aync reading, state machine in parent driver like baro.c or compas.c I knew that in my commit will be bugs :). Are you going to change this pr, or should I create new one? |
|
@error414 Better you open it: it's your design, and you have the F4, QMC and OLED to test it on. If you want, I'll bring #12063's AT32 read that doesn't block onto it, and test the core here with the tests I used for #12063, the NACK one included. What I have on I2C:
#12063 can stay open until yours is in, then I'll close it. |
|
Flight data: the four flights of 2026-10-17 and 18 on a fixed wing (21 minutes in the air), with this PR at its current
|
The barometer task waits on the I2C bus for every read. On my H743 (DPS310 on I2C at 400 kHz) the BARO task runs 64 times a second, 113 us on average and 297 us at most, and nearly all of that is the CPU spinning while the status byte and the six measurement bytes go over the wire. Those 300 us land in the main loop wherever the scheduler happens to put the task.
This makes barometer reads start the transfer and come back for the result later, so the CPU doesn't wait on the bus.
The change
A small API next to the blocking one,
busReadBufAsync()andbusAsyncState()indrivers/bus.c. On an I2C bus that supports it the read only starts, andbusAsyncState()says whether it is still on its way, done, or failed. On SPI, on software I2C and on RP2350 the read runs to the end insidebusReadBufAsync(), asbusReadBuf()does, and the state is reported done at once, so drivers need no special case.How the transfer moves on, per family, reusing what each driver already has:
bus_i2c_stm32f40x.c): the existing polled state machine, stepped fromtaskRunRealtimeCallbacks()instead of in a loop. Every wait of the I2Cv1 peripheral holds SCL low until software acts, so a late step only makes the transfer longer, and the one- and two-byte endings still happen within a single step.bus_i2c_hal.c):HAL_I2C_Mem_Read_IT(), completed by the I2C interrupt handlers that are already there. The main loop only looks at the handle's state.bus_i2c_at32f43x.c): the steps ofi2c_memory_read()as a small polled state machine, each step taken when its flag is up instead of waited for.One transfer per bus at a time. A blocking transfer on the same bus (compass, pitot, anything else) first lets an asynchronous one finish, so the two never collide, and a failed transfer, a NACK included, goes through the same recovery as a blocking one (error counter,
i2cInit()). A transfer that stops moving forI2C_TIMEOUTis abandoned the same way; a main loop that was busy elsewhere for a while (a settings save, for example) doesn't count as the transfer being stuck.Barometer: a driver that sets
asyncReadreturns false fromget_upwhile its read is on its way, andbaroUpdate()comes back 500 us later. Those extra runs don't feed the position estimator, so it gets the same updates as today. Converted: DPS310 / SPL07-003 (status, then the measurement) and BMP280 / BME280 (the measurement read; the one-byte write that starts a conversion stays blocking, so the conversion time is still counted from when it really starts). The other barometers are unchanged.Testing
TBS Lucid H7 Wing (H743), DPS310 on I2C2 at 400 kHz, one minute each, same board and config:
Flash: +976 B on MATEKF405SE, +988 B on BLUEBERRYF435WING, +2,364 B on MATEKF722SE (98.65% to 99.13%), +3,504 B on MATEKH743. On F7 and H7 most of it is the HAL's interrupt driven read, which was not linked before.
Built for MATEKF405SE, MATEKF722SE, MATEKH743, TBS_LUCID_H7_WING, BLUEBERRYF435WING and SITL.
Not tested:
If anyone has an F4, F7 or AT32 board with a DPS310 or BMP280 on I2C, the
tasksoutput (the BARO line) andstatus(the I2C error count) before and after would tell whether it behaves the same there.After review
busAsyncState(), so the call only cost ITCM. After the merge of maintenance-10.x, LTO pulled it intoscheduler()and KAKUTEF7HDV went 16 B over its ITCM. asyncfatfs: keep afatfs_poll out of the ITCM scheduler #12118 takesafatfs_poll()out ofscheduler(); with it, KAKUTEF7HDV links this PR at 15 416 B. F4 and AT32 still step their reads from there.Built on F4, F7, H7, AT32 and SITL. After the merge of maintenance-10.x, ITCM is at 16264 B on KAKUTEF7 and 16384 of 16384 B on KAKUTEF7HDV (maintenance-10.x: 16232 B on both). None of the growth is in code this PR touches: LTO lays out
scheduler(),pidApplyMulticopterRateController()andgyroFilter()differently. Holybro Kakute H7 Mini with the BMP280, the same as before the change: BARO task 25 to 27 us on average, 72 us at most, altitude noise 9.5 cm, I2C error count unchanged. On the TBS Lucid H7 Wing after review: DPS310 at 31 samples/s, no I2C error after boot. A stuck read can't be caused on the bench.Related PRs