feat(i2c): abort asynchronous master transactions - #127
Merged
Conversation
Member
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughAsynchronous I2C transactions now use synchronized active-state tracking. The ISR ignores stale interrupts, completion clears state before callbacks, and a new API aborts active transactions and resets the bus when no queued work exists. ChangesAsynchronous I2C transaction abort
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
components/esp_driver_i2c/i2c_master.c (1)
779-888: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not hold
transaction_lockduring an unbounded bus-busy wait.Line 779 holds
transaction_lockwhile the ISR callss_i2c_send_command_async. That function spins at line 623 untili2c_ll_is_bus_busy()returns false. If a target holds a line low,i2c_master_bus_abort_transactionblocks on the same lock and cannot reset or clear the bus.Replace the busy wait with an ISR state transition that exits promptly and resumes only when hardware signals progress. Keep the lock hold time bounded.
🤖 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 779 - 888, Update the ISR flow around s_i2c_send_command_async so it never performs its unbounded bus-busy wait while transaction_lock is held. Replace the synchronous wait with state tracking that exits the critical section promptly and resumes command submission only after a hardware progress interrupt, while preserving transaction completion and error handling in the surrounding ISR logic.
🤖 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 1299-1342: Serialize i2c_master_bus_abort_transaction() with
asynchronous submissions by keeping a bus-wide exclusion mechanism active
through s_i2c_hw_fsm_reset(). Ensure public async submission paths cannot start
or program hardware between releasing transaction_lock and completing reset,
then allow submissions to proceed after the bus status is restored to
I2C_STATUS_IDLE.
---
Outside diff comments:
In `@components/esp_driver_i2c/i2c_master.c`:
- Around line 779-888: Update the ISR flow around s_i2c_send_command_async so it
never performs its unbounded bus-busy wait while transaction_lock is held.
Replace the synchronous wait with state tracking that exits the critical section
promptly and resumes command submission only after a hardware progress
interrupt, while preserving transaction completion and error handling in the
surrounding ISR logic.
🪄 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: f57797ea-7e4e-4b0b-9da2-0976792e94a4
📒 Files selected for processing (3)
components/esp_driver_i2c/i2c_master.ccomponents/esp_driver_i2c/i2c_private.hcomponents/esp_driver_i2c/include/driver/i2c_master.h
Comment on lines
+1299
to
+1342
| esp_err_t i2c_master_bus_abort_transaction(i2c_master_bus_handle_t bus_handle) | ||
| { | ||
| ESP_RETURN_ON_FALSE(bus_handle != NULL, ESP_ERR_INVALID_ARG, TAG, "I2C bus is not initialized"); | ||
| ESP_RETURN_ON_FALSE(bus_handle->async_trans, ESP_ERR_INVALID_STATE, TAG, "I2C bus is not asynchronous"); | ||
|
|
||
| bool aborted = false; | ||
| bool queued = false; | ||
| portENTER_CRITICAL(&bus_handle->transaction_lock); | ||
| queued = bus_handle->queue_trans || bus_handle->num_trans_inqueue != 0; | ||
| if (!queued && bus_handle->transaction_active) { | ||
| i2c_hal_context_t *hal = &bus_handle->base->hal; | ||
| portENTER_CRITICAL(&bus_handle->base->spinlock); | ||
| i2c_ll_disable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); | ||
| i2c_ll_clear_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); | ||
| portEXIT_CRITICAL(&bus_handle->base->spinlock); | ||
|
|
||
| bus_handle->transaction_active = false; | ||
| bus_handle->i2c_trans = (i2c_transaction_t) {}; | ||
| bus_handle->cmd_idx = 0; | ||
| atomic_store(&bus_handle->trans_idx, 0); | ||
| bus_handle->rx_cnt = 0; | ||
| bus_handle->read_len_static = 0; | ||
| bus_handle->read_buf_pos = 0; | ||
| bus_handle->contains_read = false; | ||
| bus_handle->trans_done = false; | ||
| bus_handle->sent_all = true; | ||
| bus_handle->trans_finish = true; | ||
| bus_handle->in_progress = false; | ||
| bus_handle->new_queue = true; | ||
| bus_handle->event = I2C_EVENT_ALIVE; | ||
| bus_handle->async_error_event = I2C_EVENT_ALIVE; | ||
| aborted = true; | ||
| } | ||
| portEXIT_CRITICAL(&bus_handle->transaction_lock); | ||
|
|
||
| ESP_RETURN_ON_FALSE(!queued, ESP_ERR_INVALID_STATE, TAG, "cannot abort a bus with queued transactions"); | ||
| ESP_RETURN_ON_FALSE(aborted, ESP_ERR_INVALID_STATE, TAG, "no active transaction"); | ||
|
|
||
| // The interrupt handler can no longer access the retired transaction. | ||
| // Clear the physical bus outside the critical section because a target | ||
| // can hold a line low until the bounded bus-clear timeout expires. | ||
| esp_err_t ret = s_i2c_hw_fsm_reset(bus_handle, true); | ||
| atomic_store(&bus_handle->status, I2C_STATUS_IDLE); | ||
| return ret; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the affected symbols and inspect lock ownership at call sites.
ast-grep outline components/esp_driver_i2c/i2c_master.c --items all --type function
rg -n -C 8 --type c \
'i2c_master_bus_abort_transaction\s*\(|s_i2c_asynchronous_transaction\s*\(|s_i2c_transaction_start\s*\(|transaction_lock|bus_lock_mux|sent_all' \
components/esp_driver_i2cRepository: toitware/esp-idf
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transaction start ---'
sed -n '665,730p' components/esp_driver_i2c/i2c_master.c
printf '%s\n' '--- async submission ---'
sed -n '952,1050p' components/esp_driver_i2c/i2c_master.c
printf '%s\n' '--- async public wrappers ---'
sed -n '1350,1535p' components/esp_driver_i2c/i2c_master.c
printf '%s\n' '--- abort and wait APIs ---'
sed -n '1282,1345p' components/esp_driver_i2c/i2c_master.c
sed -n '1550,1625p' components/esp_driver_i2c/i2c_master.c
printf '%s\n' '--- all async helper callers ---'
rg -n -C 4 --type c \
's_i2c_asynchronous_transaction\s*\(|i2c_master_(multi_buffer_transmit|transmit|transmit_receive|receive|probe|execute_defined_operations)\s*\(' \
components/esp_driver_i2c/i2c_master.cRepository: toitware/esp-idf
Length of output: 28548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("components/esp_driver_i2c/i2c_master.c")
s = p.read_text()
async_start = s[s.index("static esp_err_t s_i2c_asynchronous_transaction"):
s.index("static esp_err_t s_i2c_synchronous_transaction")]
abort = s[s.index("esp_err_t i2c_master_bus_abort_transaction"):
s.index("esp_err_t i2c_master_get_bus_handle")]
first_branch = async_start[async_start.index("{"):]
first_branch = first_branch[:first_branch.index("} else {")]
checks = {
"immediate async branch checks retired state": "sent_all == true && i2c_master->num_trans_inqueue == 0" in first_branch,
"immediate async branch starts transaction": "s_i2c_transaction_start(i2c_dev, timeout_ms)" in first_branch,
"immediate async branch does not take bus semaphore": "xSemaphoreTake(i2c_master->bus_lock_mux" not in first_branch,
"abort releases transaction lock": "portEXIT_CRITICAL(&bus_handle->transaction_lock);" in abort,
"abort resets hardware after releasing transaction lock":
abort.index("portEXIT_CRITICAL(&bus_handle->transaction_lock);") <
abort.index("s_i2c_hw_fsm_reset(bus_handle, true)"),
"async public wrappers use async helper": all(
name in s and "s_i2c_asynchronous_transaction" in s[s.index(name):s.index(name) + 2500]
for name in (
"esp_err_t i2c_master_multi_buffer_transmit",
"esp_err_t i2c_master_transmit_receive",
"esp_err_t i2c_master_receive",
"esp_err_t i2c_master_execute_defined_operations",
)
),
}
for label, result in checks.items():
print(f"{label}: {'PASS' if result else 'FAIL'}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: toitware/esp-idf
Length of output: 453
Serialize abort recovery with asynchronous submission
i2c_master_bus_abort_transaction() releases transaction_lock before s_i2c_hw_fsm_reset(). A concurrent public asynchronous submission can start before the reset and have its hardware state cleared. Hold a bus-wide lock through reset, or block submissions with an abort-in-progress state until reset completes.
🤖 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 1299 - 1342, Serialize
i2c_master_bus_abort_transaction() with asynchronous submissions by keeping a
bus-wide exclusion mechanism active through s_i2c_hw_fsm_reset(). Ensure public
async submission paths cannot start or program hardware between releasing
transaction_lock and completing reset, then allow submissions to proceed after
the bus status is restored to I2C_STATUS_IDLE.
floitsch
force-pushed
the
floitsch/i2c-master-abort
branch
from
August 12, 2026 21:47
123d2cd to
d7fcaaa
Compare
floitsch
force-pushed
the
floitsch/i2c-master-abort
branch
from
August 12, 2026 22:16
d7fcaaa to
8339c49
Compare
floitsch
changed the base branch from
floitsch/i2c-async-controller
to
patch-head-5.4.2
August 12, 2026 22:16
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
1 participant
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_master_bus_abort_transactionfor active asynchronous master transactions.Testing
git diff --check.