fix(i2c): make async master transactions reliable - #123
Conversation
83186a9 to
66288ee
Compare
c545f1e to
0471101
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughAsynchronous I2C transactions now retain device handles and asynchronous error events. Startup resets state and handles prior errors. ISR and completion paths separately process NACK, timeout, and normal completion events. ChangesAsync I2C transaction handling
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@components/esp_driver_i2c/i2c_master.c`:
- Around line 757-760: Update the NULL-device handling in s_i2c_send_commands to
identify probe transactions separately from late timeout interrupts, signaling
cmd_semphr via xSemaphoreGiveFromISR for probes while retaining the existing
timeout lifetime protection for late interrupts.
- Line 895: Update the queued-transaction dequeue path to assign
i2c_master->ack_check_disable from the dequeued transaction’s t.device before
calling s_i2c_send_command_async. Keep the existing immediate-start assignment
and queued device storage unchanged.
- Around line 739-742: Retain the terminal timeout state at
components/esp_driver_i2c/i2c_master.c:739-742 until controller recovery
finishes. In the completion/queued-transaction path at
components/esp_driver_i2c/i2c_master.c:785-802, perform bounded FSM recovery,
restore I2C_LL_MASTER_EVENT_INTR, then clear the terminal state and start the
next queued transaction; preserve normal behavior when no timeout occurred.
- Around line 622-623: In the busy-bus rejection branch of
s_i2c_asynchronous_transaction, restore the transaction to its idle state before
returning ESP_ERR_INVALID_STATE: reset sent_all and trans_finish appropriately,
and clear the rejected descriptor or set its command count to zero so later
callers do not queue work against a nonexistent active transaction.
🪄 Autofix
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: 2643d40b-5fb2-4baa-96d9-e98a3555a1f3
📒 Files selected for processing (2)
components/esp_driver_i2c/i2c_master.ccomponents/esp_driver_i2c/i2c_private.h
| } else if (i2c_ll_is_bus_busy(hal->dev)) { | ||
| return ESP_ERR_INVALID_STATE; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore asynchronous state when startup rejects a busy bus.
Before this return, s_i2c_asynchronous_transaction sets sent_all and trans_finish to false. The error path does not restore them. A later caller queues work because sent_all remains false, but no active transaction can start that queue.
Restore the idle transaction state before propagating ESP_ERR_INVALID_STATE. Clear the rejected descriptor or set its command count to zero.
🤖 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 `@components/esp_driver_i2c/i2c_master.c` around lines 622 - 623, In the
busy-bus rejection branch of s_i2c_asynchronous_transaction, restore the
transaction to its idle state before returning ESP_ERR_INVALID_STATE: reset
sent_all and trans_finish appropriately, and clear the rejected descriptor or
set its command count to zero so later callers do not queue work against a
nonexistent active transaction.
| // A hardware timeout is terminal. Leaving the mask enabled causes a | ||
| // level-triggered interrupt storm until a later transaction resets | ||
| // the peripheral. | ||
| i2c_ll_disable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Recover the controller before starting queued work after a timeout.
A timeout disables I2C_LL_MASTER_EVENT_INTR. The completion path then clears the terminal state and starts the next queued transaction without re-enabling the interrupt mask or resetting the FSM. The queued transaction cannot receive completion interrupts.
components/esp_driver_i2c/i2c_master.c#L739-L742: retain the terminal timeout state until recovery completes.components/esp_driver_i2c/i2c_master.c#L785-L802: perform bounded FSM recovery and restore the master event interrupt mask before starting the next queued transaction.
📍 Affects 1 file
components/esp_driver_i2c/i2c_master.c#L739-L742(this comment)components/esp_driver_i2c/i2c_master.c#L785-L802
🤖 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 `@components/esp_driver_i2c/i2c_master.c` around lines 739 - 742, Retain the
terminal timeout state at components/esp_driver_i2c/i2c_master.c:739-742 until
controller recovery finishes. In the completion/queued-transaction path at
components/esp_driver_i2c/i2c_master.c:785-802, perform bounded FSM recovery,
restore I2C_LL_MASTER_EVENT_INTR, then clear the terminal state and start the
next queued transaction; preserve normal behavior when no timeout occurred.
| i2c_master_dev_handle_t i2c_dev = i2c_master->i2c_trans.device; | ||
| if (i2c_dev == NULL) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Route probe interrupts to the synchronous completion path.
i2c_master_probe sets i2c_trans.device to NULL on Line 1315, then calls s_i2c_send_commands. If async_trans is enabled, this branch returns without xSemaphoreGiveFromISR. The probe therefore cannot complete through its synchronous wait path.
Distinguish probe transactions from late timeout interrupts. Signal cmd_semphr for a probe transaction while preserving the timeout lifetime protection.
🤖 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 `@components/esp_driver_i2c/i2c_master.c` around lines 757 - 760, Update the
NULL-device handling in s_i2c_send_commands to identify probe transactions
separately from late timeout interrupts, signaling cmd_semphr via
xSemaphoreGiveFromISR for probes while retaining the existing timeout lifetime
protection for late interrupts.
| i2c_master->sent_all = false; | ||
| i2c_master->trans_finish = false; | ||
| i2c_master->queue_trans = false; | ||
| i2c_master->ack_check_disable = i2c_dev->ack_check_disable; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply ACK policy when a queued transaction becomes active.
Line 895 updates ack_check_disable only for an immediately started transaction. The queued path stores the device on Line 927, but the dequeue path assigns i2c_trans and starts commands without restoring ack_check_disable from t.device.
A queued transaction can therefore inherit the previous device’s ACK policy. Set i2c_master->ack_check_disable from the dequeued transaction device before s_i2c_send_command_async.
Also applies to: 927-927
🤖 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 `@components/esp_driver_i2c/i2c_master.c` at line 895, Update the
queued-transaction dequeue path to assign i2c_master->ack_check_disable from the
dequeued transaction’s t.device before calling s_i2c_send_command_async. Keep
the existing immediate-start assignment and queued device storage unchanged.
0471101 to
87256d4
Compare
1 participant
Summary
ESP_ERR_INVALID_STATEfor an unexpectedly busy bus instead of entering synchronous bus-clear logic.Dependencies
Testing
Exercised by Toit's two-board asynchronous controller suite on ESP32 and ESP32-S3: probes, scan, NACK and timeout recovery, ACK policy, 7-bit/10-bit identity, FIFO/command boundaries, concurrent callers, RMT timing probes, and clock stretching.
The same callback and recovery gaps are still present in the inspected ESP-IDF v6.1-dev checkout, so this patch remains a focused delta from the current upstream API.
Stack created with GitHub Stacks CLI • Give Feedback 💬