fix(i2c): harden slave v2 transactions by floitsch · Pull Request #121 · toitware/esp-idf

floitsch · 2026-08-06T17:14:54Z

Summary

Harden the I2C target-v2 state machine across repeated starts, reads, writes, STOP handling, and transaction completion.
Correct callback/event ordering and byte accounting so an event-driven runtime can safely retain and release transaction buffers.
Add the low-level ESP32 support needed for reliable target transaction boundaries.

Consumer

Required by toitlang/toit#3154.

Testing

Exercised by Toit's two-board target, register-target, and async-controller hardware suites on ESP32 and ESP32-S3.

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

floitsch · 2026-08-10T12:29:54Z

@CodeRabbit review

coderabbitai · 2026-08-10T12:30:01Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2026-08-10T12:33:39Z

Walkthrough

Changes

The I2C slave v2 driver now tracks receive-buffer overflow and pending read requests. It reports overflow in receive events, improves ISR FIFO handling, updates allocation errors, enables an auto-start workaround, and coordinates pending-request writes with stretch release.

I2C slave v2 behavior

Layer / File(s)	Summary
Receive and request state contracts `components/esp_driver_i2c/i2c_private.h`, `components/esp_driver_i2c/include/driver/i2c_types.h`, `components/hal/esp32/include/hal/i2c_ll.h`	The slave state tracks receive overflow and pending requests. Receive events expose overflow status, and the HAL adds an ACK-waiting helper.
ISR FIFO and transaction flow `components/esp_driver_i2c/i2c_slave_v2.c`	ISR FIFO handlers now protect semaphore access, record dropped receive bytes, report receive completion, and load queued transmit data during request handling.
Initialization and pending-request writes `components/esp_driver_i2c/i2c_slave_v2.c`	Initialization uses cleanup-based allocation errors and enables the ESP32-S3 and ESP32-C3 auto-start workaround. `i2c_slave_write` now handles semaphore timeouts, pending-request FIFO state, conditional TX interrupts, and stretch release.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description accurately summarizes the I2C target-v2 hardening, event ordering, buffer ownership, low-level support, and testing.
Title check	✅ Passed	The title clearly and concisely describes the main change: hardening I2C slave version 2 transactions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch floitsch/i2c-target-fixes

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 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_slave_v2.c`:
- Around line 71-72: Remove the ISR dependency on operation_mux in the FIFO
service paths, including the checks around the address-match and FIFO handlers,
so ISR servicing continues when a task owns the mutex. Refactor i2c_slave_write
and related operation_mux usage so the mutex is not held across blocking
xRingbufferSend operations; preserve FIFO draining and stretch clearing through
an immediately available ISR path or guaranteed deferred retry.

🪄 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: 145ba99e-82f7-4b46-97cc-dd493aaf96ca

📥 Commits

Reviewing files that changed from the base of the PR and between ded0ae5 and 844f0bd.

📒 Files selected for processing (4)

components/esp_driver_i2c/i2c_private.h
components/esp_driver_i2c/i2c_slave_v2.c
components/esp_driver_i2c/include/driver/i2c_types.h
components/hal/esp32/include/hal/i2c_ll.h

coderabbitai · 2026-08-10T12:33:42Z

+    if (xSemaphoreTakeFromISR(i2c_slave->operation_mux, &xTaskWoken) != pdTRUE) {
+        return xTaskWoken;


🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift

Do not block ISR FIFO service with operation_mux.

At Lines 71-72 and Lines 102-104, the ISR stops servicing FIFO data when a task owns operation_mux. i2c_slave_write acquires the same mutex at Lines 394-396 and can block in xRingbufferSend while the TX ring buffer is full.

If a master then starts a read, the address-match handler cannot load queued TX data or clear stretch. The master cannot consume the TX ring buffer. A write with timeout_ms == -1 then waits forever.

Do not hold this mutex across blocking ring-buffer operations. Keep FIFO service available to the ISR, or add a guaranteed deferred retry that drains the FIFO and clears stretch after contention.

Also applies to: 102-104, 157-164, 228-228, 394-396

🤖 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_slave_v2.c` around lines 71 - 72, Remove the ISR dependency on operation_mux in the FIFO service paths, including the checks around the address-match and FIFO handlers, so ISR servicing continues when a task owns the mutex. Refactor i2c_slave_write and related operation_mux usage so the mutex is not held across blocking xRingbufferSend operations; preserve FIFO draining and stretch clearing through an immediately available ISR path or guaranteed deferred retry.

floitsch

lgtm

## Summary - Correct the 10-bit controller read address sequence. - Add an ISR-context transmit provider to the I2C target-v2 callbacks, allowing a fixed register map to provide bytes without waiting for an application task. - Report the transmitted byte count after the transaction so the provider can advance register state safely. ## Dependencies - Builds on #121. - Required by toitlang/toit#3155. ## Testing Exercised by Toit's autonomous register-target hardware suite on ESP32 and ESP32-S3, including 7-bit/10-bit addressing, register wrapping, updates, and oversized writes. <sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub>

fix(i2c): harden slave v2 transactions

844f0bd

This was referenced Aug 6, 2026

feat(i2c): add synchronous slave transmit provider #122

Merged

Add asynchronous I2C target support toitlang/toit#3154

Merged

coderabbitai Bot reviewed Aug 10, 2026

View reviewed changes

i2c: clarify auto-start workaround provenance

07ee4f7

floitsch commented Aug 10, 2026

View reviewed changes

fix(i2c): keep slave FIFO service independent

2fda6ba

floitsch marked this pull request as ready for review August 10, 2026 14:58

floitsch merged commit de07eb5 into patch-head-5.4.2 Aug 10, 2026
2 checks passed

floitsch deleted the floitsch/i2c-target-fixes branch August 10, 2026 21:44

Navigation Menu

Repository navigation

fix(i2c): harden slave v2 transactions - #121

fix(i2c): harden slave v2 transactions#121
floitsch merged 3 commits into
patch-head-5.4.2from
floitsch/i2c-target-fixes

floitsch commented Aug 6, 2026 •

edited

Loading

Uh oh!

floitsch commented Aug 10, 2026

Uh oh!

coderabbitai Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 10, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Aug 10, 2026

Uh oh!

floitsch left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		if (xSemaphoreTakeFromISR(i2c_slave->operation_mux, &xTaskWoken) != pdTRUE) {
		return xTaskWoken;

Conversation

floitsch commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Consumer

Testing

Uh oh!

floitsch commented Aug 10, 2026

Uh oh!

coderabbitai Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 10, 2026

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

floitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

floitsch commented Aug 6, 2026 •

edited

Loading

coderabbitai Bot commented Aug 10, 2026 •

edited

Loading