refactor(driver): centralize I2C locking and migrate polling to RTOS timers#7230
Open
raphaelcoeffic wants to merge 15 commits into
Open
refactor(driver): centralize I2C locking and migrate polling to RTOS timers#7230raphaelcoeffic wants to merge 15 commits into
raphaelcoeffic wants to merge 15 commits into
Conversation
Member
|
I think you need to include PA01 in your test plan as well. I have already implemented a few things in PA01 that maybe similar to what you did. The key driver always returns a cached value and key polling is triggered by 10ms timer. So see how the refactorization works for all devices. |
Add centralized I2C bus recovery via SCL bit-banging (per ST AN2824) to handle the well-known STM32F4 SDA-stuck-low condition. All I2C transfer functions now automatically attempt recovery + retry on failure. Migrate GX12 PCA9555 switch polling from synchronous mixer-task calls to the async/timer pattern (matching rm-h750), eliminating I2C bus access from the mixer task entirely. Add 3-level error recovery to the GX12 IO expander reads (PCA reset -> I2C bus recover -> give up). Fix GX12 EXTI config: PE14 (PCA9555 INT) needs EXTI15_10, not EXTI1. Fix latent deadlock in 2-arg stm32_i2c_is_dev_ready overload. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move per-bus mutex ownership from the STM32 I2C driver to the HAL layer (i2c_bus.cpp). Transfer functions no longer lock internally — callers are responsible for explicit locking matching their execution context: - i2c_lock/unlock: blocking, for regular task context - i2c_trylock/unlock: non-blocking, for timer task context - no locking needed before RTOS scheduler starts This avoids blocking the FreeRTOS timer service task (which must never block per FreeRTOS documentation) when I2C buses are shared between timer-polled devices (IMU, IO expanders) and task-polled devices (touch). Update GX12 bsp_io to use i2c_trylock from timer callback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update all I2C device drivers to use the new explicit locking API: Timer/ISR context (i2c_trylock, skip if busy): - GX12 bsp_io (PCA9555 switch polling) - rm-h750 bsp_io (PCA9555 switch polling) - PA01 key_driver (AW9523B matrix scanning) - CSD203 power sensor Regular task context (i2c_lock/unlock): - Touch drivers (GT911, CST8xx, CST340, CST836U, FT6236, CHSC5448) - IMU drivers (LSM6DS, ICM42607C, SC7U22) - TAS2505 audio codec volume control - EEPROM driver - Volume I2C driver - Seesaw GPIO expander Init-only drivers (no locking, pre-scheduler): - WM8904 audio codec - PCA95xx (locking at call site) - AW9523B (locking at call site) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move gyro/accelerometer I2C reads from the mixer task to a dedicated 10ms RTOS timer callback. Uses i2c_trylock so the timer service task never blocks — if the bus is held (e.g. by touch controller in menu task), the IMU sample is skipped and retried next cycle. This eliminates I2C bus contention between IMU and touch on targets where they share a bus (ST16, TX15, Horus X12S). Changes: - gyro.cpp: replace gyroWakeup() with timer callback using trylock - gyroStart() now takes bus parameter, starts the timer - IMU read functions (lsm6ds, icm42607C, sc7u22): remove internal i2c_lock since gyro timer callback handles locking - imuDetect() returns detected bus via output parameter - Remove gyroWakeup() from mixer task execMixerFrequentActions() - Update all board.cpp gyroInit() to pass bus to gyroStart() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The minimal_board_lib native target compiles i2c_bus.cpp without BOOT or FREE_RTOS defined. Change the preprocessor guard from !defined(BOOT) to defined(FREE_RTOS) so mutex code is only compiled when FreeRTOS is actually available. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move CSD203 power sensor from targets/common/arm/stm32/ to drivers/, replacing STM32-specific stm32_i2c_master_tx/rx with HAL i2c_read/write. Polling migrated from per10ms() to a dedicated RTOS timer (matching the IMU pattern), and the obsolete IICReadStatusFlag bus-preemption workaround removed from tp_gt911.cpp since the I2C mutex now handles bus sharing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gyro.cpp now calls i2c_trylock/i2c_unlock directly, which are only provided by the STM32 i2c_bus.cpp. Add no-op stubs for all i2c_driver.h functions in the simu target so native builds (simulator and unit tests) link correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
imuDetect() now returns the matched etx_imu_t* (a stable static copy) instead of a bare read fn plus a separate &bus out-param, and gyroStart() takes that single handle. This removes the redundant bus plumbing through the call sites and drops the etx_i2c_bus_t boilerplate at each board. Also drops the s_detected_valid flag, using s_detected.driver as the detection indicator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
raphaelcoeffic
force-pushed
the
i2c-conflicts
branch
from
ed53434 to
ea018cc
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds I2C locking, bus recovery, and related driver updates across the HAL, IMU, CSD203, GX12 BSP I/O, touch, EEPROM, volume, CLI, and peripheral code paths. It also changes gyro sampling to a timer callback and refactors PDM WAV recorder locking. ChangesI2C synchronization and driver updates
PDM WAV recorder locking refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The aw9523b expander accesses go through the HAL i2c_read/i2c_write transfer layer, which already performs SCL bit-bang bus recovery (ST AN2824) plus a retry on every failed transfer. The local _track_i2c_error/_recover_i2c path (peripheral deinit/init after 3 consecutive errors) is strictly weaker and only triggers after the central recovery has already failed, so remove it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
radio/src/targets/taranis/gx12/bsp_io.cpp (1)
154-166: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnable the GPIO interrupt after the poll timer is created.
gpio_init_int()can allow_io_int_handler()to queue_poll_switches()beforestart_poll_timer()runs; the IRQ path then callstimer_reset(&_poll_timer), which is justtimer_start(), on an uncreated timer. Move interrupt registration after initial read/timer creation.Proposed fix
- // setup expanders pin change interrupt - gpio_init_int(IO_INT_GPIO, GPIO_IN, GPIO_FALLING, _io_int_handler); - // setup expanders reset pin gpio_init(IO_RESET_GPIO, GPIO_OUT, GPIO_PIN_SPEED_LOW); gpio_set(IO_RESET_GPIO); @@ `#if` !defined(BOOT) start_poll_timer(); `#endif` + + // setup expanders pin change interrupt + gpio_init_int(IO_INT_GPIO, GPIO_IN, GPIO_FALLING, _io_int_handler);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/taranis/gx12/bsp_io.cpp` around lines 154 - 166, Move the GPIO interrupt setup in bsp_io.cpp so that gpio_init_int() for IO_INT_GPIO happens only after the initial expander reads and after start_poll_timer() has created _poll_timer; _io_int_handler() can otherwise trigger _poll_switches() and call timer_reset() before the timer exists. Keep the existing initialization flow in this area, but reorder the interrupt registration in the startup path for the Taranis GX12 I/O expander so the IRQ cannot fire until the polling timer is ready.radio/src/hal/eeprom/eeprom_driver.cpp (1)
141-158: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid holding the bus mutex across the unbounded EEPROM wait loops.
eepromPageWrite()andeepromWaitEepromStandbyState()already spin until success. With the new outeri2c_lock(), one unrecoverable EEPROM failure now blocks every other user ofEEPROM_I2C_BUSindefinitely instead of only stalling this caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/hal/eeprom/eeprom_driver.cpp` around lines 141 - 158, The EEPROM write path is holding EEPROM_I2C_BUS locked across the unbounded retry/wait sequence in the page write loop, which can block all other bus users if the device never recovers. Update the logic around eepromPageWrite() and eepromWaitEepromStandbyState() so the bus mutex is only held for the actual I2C transaction and released before any spin/wait retry behavior, keeping the lock scope minimal while preserving the existing write flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/boards/generic_stm32/i2c_bus.cpp`:
- Around line 118-120: The mutex access in the I2C bus helpers is indexing
_i2c_mutex and _i2c_mutex_initialized without validating the bus value at use
sites. Update the relevant logic in i2c_ensure_mutex, the scheduler_is_running()
lock path, and i2c_unlock() to guard the bus index before any array access,
matching the protection already needed in i2c_unlock. Keep the validation local
to these helper functions so unexpected bus values cannot walk past the mutex
arrays.
In `@radio/src/cli.cpp`:
- Around line 1853-1855: The i2c CLI help text and command registration are out
of sync with the actual handler behavior in cli.cpp: the implementation under
the i2c subcommand only supports scan, while the advertised entries still
include lockup and recover. Update the command table and any usage/help strings
in the i2c handler so they list only the subcommands that are actually
implemented, or add the missing subcommand handlers if they are intended to be
supported, keeping the help text, registration, and dispatch logic consistent.
- Around line 1859-1869: The i2c scan command in cli.cpp accepts user-provided
bus input and uses it directly in i2c_lock(bus) and i2c_dev_ready(bus, addr), so
add validation in the i2c scan handling path before any HAL call. In the command
block that parses bus with toInt and prints “Scanning I2C bus...”, reject
negative values and any bus outside the supported range, return the usage/error
path, and only proceed to locking and probing when bus is valid.
In `@radio/src/drivers/csd203.cpp`:
- Around line 160-172: The startup init sequence in csd203_start() calls
csd203_init() three times after i2c_init(), but those I2C transfers are not
protected by the HAL lock. Wrap the entire
csd203_main/csd203_internal/csd203_external initialization block with the same
bus lock used for timer polling, or move locking into csd203_init() so the
locking model is consistent for all I2C accesses.
- Around line 66-79: csd203_init() computes the calibration value using
current_lsb * rshunt, but that denominator can be zero for valid public inputs
and crash the init path. Add an early validation check in csd203_init before
calculating cal, and return a failure code if current_lsb or rshunt makes the
denominator zero; keep the check close to the existing config/calibration setup
so the failure is handled before calling csd203_write_reg().
- Around line 171-184: Prime the external battery-voltage cache during
csd203_start() so getBatteryVoltage() does not return a false 0V before timer
polling begins. After csd203_init() for csd203_external succeeds, take one
immediate external voltage reading and store it in csd203_ext_vbus, reusing the
same sensor access path used by csd203TimerCb. Keep the async timer start
behavior intact, but ensure the cached value is populated on initialization for
immediate board-init reads.
In `@radio/src/drivers/csd203.h`:
- Around line 35-36: The csd203_read_voltage and csd203_read_current APIs
currently return the sampled value directly, which hides I2C failures when they
fall back to zero. Update the csd203_t driver interface to return a status code
and move the measured voltage/current into an output parameter, then adjust the
implementations and all call sites accordingly so callers can distinguish read
errors from legitimate zero readings.
In `@radio/src/gyro.cpp`:
- Around line 33-34: The complementary filter in gyroFilter() is using a fixed
SAMPLE_TIME_S, which causes under-integration when i2c_trylock() skips reads or
timing jitters. Update the gyro sampling flow to track the timestamp of the last
successful sample, compute the real elapsed dt for each accepted update, and
pass that dt into gyroFilter() instead of relying on the constant SAMPLE_TIME_S.
Use the existing gyroFilter, i2c_trylock, and GYRO_POLL_PERIOD_MS symbols to
locate the sample loop and filter integration path.
- Around line 115-127: The gyro polling logic in the callback is permanently
disabling itself once the error counter reaches 100, so transient I2C failures
can latch the sensor offline forever. Update the error handling around the read
path in the gyro callback to use a capped retry/backoff strategy instead of an
unconditional early return on `errors >= 100`, and make sure the
`readFn`/`i2c_trylock` path still periodically probes after failures so recovery
is possible.
- Around line 111-123: Move the IMU acquisition out of gyroTimerCb() so the
FreeRTOS timer service task never performs blocking I2C work. Keep gyroTimerCb()
limited to lightweight signaling, then have the actual
readFn()/i2c_trylock()/i2c_unlock() sequence run in a dedicated task or worker
associated with the gyro path. Use the existing gyroTimerCb and readFn symbols
to locate the callback and preserve the current error
handling/usbPluggedInStorageMode gating in the new execution path.
In `@radio/src/hal/eeprom/eeprom_driver.cpp`:
- Around line 96-100: The retry loop in eeprom_driver::I2C_EE_ReadBlock handling
currently unlocks EEPROM_I2C_BUS before calling eepromInit(), which lets other
tasks use the shared I2C peripheral during reinitialization. Keep the bus lock
held across the reinit path in this loop, and only release it after the retry
sequence is complete so the caller-owned locking contract is preserved.
In `@radio/src/hal/i2c_driver.h`:
- Around line 31-35: The current `i2c_trylock()` contract is too broad because
the FreeRTOS implementation behind `mutex_trylock()` uses `xSemaphoreTake()`,
which is not safe from a real ISR. Update the API documentation/comments in
`i2c_driver.h` to restrict `i2c_trylock()` to task or timer-callback context
only, and keep `i2c_lock()` as the blocking task-context path. If true ISR usage
is needed, add a separate `FromISR`-based path rather than reusing
`i2c_trylock()`.
In `@radio/src/targets/common/arm/stm32/stm32_i2c_driver.cpp`:
- Around line 628-643: The retry logic in stm32_i2c_master_tx (and the related
stm32_i2c_write path) is blindly replaying write-like transfers after a partial
HAL failure. Remove the automatic second HAL_I2C_Master_Transmit attempt after
i2c_bus_recover, and keep only the recovery/reset path so the caller can decide
whether to retry a potentially non-idempotent write. Make the change in the
stm32_i2c_master_tx flow and apply the same fix to the matching write helper
referenced by the review.
---
Outside diff comments:
In `@radio/src/hal/eeprom/eeprom_driver.cpp`:
- Around line 141-158: The EEPROM write path is holding EEPROM_I2C_BUS locked
across the unbounded retry/wait sequence in the page write loop, which can block
all other bus users if the device never recovers. Update the logic around
eepromPageWrite() and eepromWaitEepromStandbyState() so the bus mutex is only
held for the actual I2C transaction and released before any spin/wait retry
behavior, keeping the lock scope minimal while preserving the existing write
flow.
In `@radio/src/targets/taranis/gx12/bsp_io.cpp`:
- Around line 154-166: Move the GPIO interrupt setup in bsp_io.cpp so that
gpio_init_int() for IO_INT_GPIO happens only after the initial expander reads
and after start_poll_timer() has created _poll_timer; _io_int_handler() can
otherwise trigger _poll_switches() and call timer_reset() before the timer
exists. Keep the existing initialization flow in this area, but reorder the
interrupt registration in the startup path for the Taranis GX12 I/O expander so
the IRQ cannot fire until the polling timer is ready.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 687dbdf7-08d4-435a-a32c-da76442c13aa
📒 Files selected for processing (40)
radio/src/boards/generic_stm32/i2c_bus.cppradio/src/boards/jumper-h750/touch_driver.cppradio/src/boards/rm-h750/bsp_io.cppradio/src/boards/rm-h750/tp_gt911.cppradio/src/cli.cppradio/src/drivers/csd203.cppradio/src/drivers/csd203.hradio/src/drivers/tas2505.cppradio/src/edgetx.cppradio/src/gyro.cppradio/src/gyro.hradio/src/hal/eeprom/eeprom_driver.cppradio/src/hal/i2c_driver.hradio/src/hal/imu.cppradio/src/hal/imu.hradio/src/mixer.cppradio/src/os/task.hradio/src/switches.cppradio/src/targets/common/arm/stm32/CMakeLists.txtradio/src/targets/common/arm/stm32/csd203_sensor.cppradio/src/targets/common/arm/stm32/stm32_i2c_driver.cppradio/src/targets/common/arm/stm32/stm32_i2c_driver.hradio/src/targets/horus/board.cppradio/src/targets/horus/cst8xx_driver.cppradio/src/targets/horus/tp_gt911.cppradio/src/targets/pa01/key_driver.cppradio/src/targets/pl18/touch_driver.cppradio/src/targets/simu/CMakeLists.txtradio/src/targets/simu/i2c_driver.cppradio/src/targets/simu/switch_driver.cppradio/src/targets/st16/touch_driver.cppradio/src/targets/st16/tp_cst340.cppradio/src/targets/stm32h7s78-dk/seesaw.cppradio/src/targets/stm32h7s78-dk/tp_gt911.cppradio/src/targets/taranis/board.cppradio/src/targets/taranis/gx12/bsp_io.cppradio/src/targets/taranis/gx12/bsp_io.hradio/src/targets/taranis/hal.hradio/src/targets/taranis/volume_i2c.cppradio/src/tasks/mixer_task.cpp
💤 Files with no reviewable changes (7)
- radio/src/tasks/mixer_task.cpp
- radio/src/targets/simu/switch_driver.cpp
- radio/src/switches.cpp
- radio/src/targets/common/arm/stm32/csd203_sensor.cpp
- radio/src/mixer.cpp
- radio/src/edgetx.cpp
- radio/src/os/task.h
MutexLock was removed earlier in this branch to make locking explicit and expose the lock-vs-trylock distinction. The MEMS-microphone feature (#7328) merged from main reintroduced it via PdmWavRecorder, which would have made the audio task block on the recorder mutex. Use explicit mutex calls with the right policy per context: - audioTick() runs in the high-priority audio task (prio +3). It now uses mutex_trylock() and skips the chunk on contention, so it never stalls waiting on the lower-priority CLI/GUI task (prio +1) holding the lock through slow SD-card I/O in start()/stop(). - start()/stop() run in the owner thread where blocking is fine, so they use mutex_lock(). start()'s body moves into startLocked() to keep a single unlock point across its early returns. FatFs reentrancy (FF_FS_REENTRANT) only serializes individual f_* calls; it does not protect the s_active/file lifecycle, so this lock is still required. Also moves the shared mutex/s_active statics into the class as private static members for encapsulation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
radio/src/pdm_wav_recorder.cpp (2)
160-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate header patch failures.
stop()currently returns onlyf_close(). If either WAV header seek/write fails but close succeeds, callers seeFR_OKfor a corrupted header.Suggested change
+ FRESULT res = FR_OK; + writeLE32(buf, 36U + dataBytes); - f_lseek(&file, 4); - f_write(&file, buf, 4, &written); + res = f_lseek(&file, 4); + if (res == FR_OK) { + written = 0; + res = f_write(&file, buf, 4, &written); + if (res == FR_OK && written != 4) res = FR_DISK_ERR; + } writeLE32(buf, dataBytes); - f_lseek(&file, 40); - f_write(&file, buf, 4, &written); + if (res == FR_OK) { + res = f_lseek(&file, 40); + } + if (res == FR_OK) { + written = 0; + res = f_write(&file, buf, 4, &written); + if (res == FR_OK && written != 4) res = FR_DISK_ERR; + } - FRESULT res = f_close(&file); + FRESULT closeRes = f_close(&file); mutex_unlock(&s_mutex); - return res; + return res != FR_OK ? res : closeRes;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/pdm_wav_recorder.cpp` around lines 160 - 170, The stop() path in PdmWavRecorder currently ignores failures from the WAV header patching steps and only returns the result of f_close(), so propagate errors from the f_lseek and f_write calls as well. Update the logic around the header update in the recorder’s stop/close flow to capture each FatFs result, preserve the first failure if any, and return that instead of blindly returning f_close() so callers can detect a corrupted header.
32-38: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake mutex initialization thread-safe.
s_mutexInitedis read/written without synchronization, so concurrent first use can creates_mutextwice. Initialize it from a single-threaded startup path or use a real one-time/critical-section guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/pdm_wav_recorder.cpp` around lines 32 - 38, Make the mutex setup in PdmWavRecorder::ensureMutex thread-safe: the current s_mutexInited check/set can race and create s_mutex more than once on concurrent first use. Replace the ad hoc boolean guard with a real one-time initialization mechanism or move mutex creation into a guaranteed single-threaded startup path, and ensure all callers of ensureMutex() rely on that safe initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/pdm_wav_recorder.cpp`:
- Around line 133-138: The null check on s_active in pdm_wav_recorder’s tick
path is racing with start()/stop() because it reads the shared pointer before
s_mutex is held. In the same block that calls ensureMutex() and mutex_trylock(),
move the s_active check to after a successful lock acquisition, then call
s_active->tickLocked() only while the mutex is held.
---
Outside diff comments:
In `@radio/src/pdm_wav_recorder.cpp`:
- Around line 160-170: The stop() path in PdmWavRecorder currently ignores
failures from the WAV header patching steps and only returns the result of
f_close(), so propagate errors from the f_lseek and f_write calls as well.
Update the logic around the header update in the recorder’s stop/close flow to
capture each FatFs result, preserve the first failure if any, and return that
instead of blindly returning f_close() so callers can detect a corrupted header.
- Around line 32-38: Make the mutex setup in PdmWavRecorder::ensureMutex
thread-safe: the current s_mutexInited check/set can race and create s_mutex
more than once on concurrent first use. Replace the ad hoc boolean guard with a
real one-time initialization mechanism or move mutex creation into a guaranteed
single-threaded startup path, and ensure all callers of ensureMutex() rely on
that safe initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c31838e3-5eba-4680-baa8-27e3a6b72eab
📒 Files selected for processing (2)
radio/src/pdm_wav_recorder.cppradio/src/pdm_wav_recorder.h
From the CodeRabbit review of the I2C refactoring:
- i2c_lock/i2c_trylock: guard the bus index against MAX_I2C_BUSES before
dereferencing the mutex array, matching i2c_unlock. Reachable now that the
CLI scan command takes a user-supplied bus.
- cli i2c: validate the bus argument before locking/probing, and trim the
command help to the only implemented subcommand ("scan <bus>").
- csd203: prime the external Vbus cache in csd203_start() so getBatteryVoltage()
does not report a false 0V before the timer's first external-sensor poll.
- i2c_driver.h: clarify that i2c_trylock is task/timer-callback only, not
ISR-safe (uses xSemaphoreTake, no FromISR path).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the MAX_I2C_BUSES definition from the generic_stm32 i2c_bus.cpp into hal/i2c_driver.h so it is part of the HAL contract (valid bus ids are 0 .. MAX_I2C_BUSES - 1). This lets the CLI bus bounds-check use it instead of the stm32-specific I2C_Bus_2 enum, and drops the direct stm32_i2c_driver.h include from cli.cpp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the unlocked early-return null check on s_active. start()/stop() write s_active under s_mutex, so the read belongs under the lock too. The authoritative check now happens after mutex_trylock(); the removed fast path only saved a cheap trylock on idle ticks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
i2c_lock/i2c_trylock/i2c_unlockAPI inhal/i2c_driver.h; mutex removed from STM32 driver, callers lock explicitlyi2c_trylock(non-blocking), task-context usesi2c_lock, pre-scheduler skips lockingi2c_trylocki2c_read/i2c_write), moved fromtargets/common/arm/stm32/todrivers/, polling moved to own RTOS timerIICReadStatusFlagbus-preemption workaround from CSD203 and GT911 touch driver — the I2C mutex handles bus sharingMutexLockRAII wrapper fromos/task.hstm32_i2c_is_dev_readyoverloadimuDetect/gyroStartsimplified —imuDetect()returns the matchedetx_imu_t*instead of a bare read fn plus a separate&busout-param;gyroStart()takes that single handle, dropping the redundant bus plumbing at every board_track_i2c_error/_recover_i2cpath frompa01/bsp_io.cpp; the AW9523B expander goes through the HAL transfer layer, which already does AN2824 recovery + retry. Rebased on top of fix(pa01): optimize key matrix polling with split-cycle scanning #7405 (PA01 split-cycle key scanning); the timer-drivenpollKeys()now takesi2c_trylockand falls back to the cached columns when the bus is busyTest plan
V12,V14, V16 (CSD203 targets) — verify battery voltage reads correctly🤖 Generated with Claude Code
Summary by CodeRabbit
i2c scan <bus>CLI command to probe and list responding devices.