Skip to content

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

Merged
floitsch merged 3 commits into
floitsch/i2c-target-fixesfrom
floitsch/i2c-register-provider
Aug 10, 2026
Merged

feat(i2c): add synchronous slave transmit provider#122
floitsch merged 3 commits into
floitsch/i2c-target-fixesfrom
floitsch/i2c-register-provider

Conversation

@floitsch

@floitsch floitsch commented Aug 6, 2026

Copy link
Copy Markdown
Member

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

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.

Stack created with GitHub Stacks CLIGive Feedback 💬

@floitsch floitsch changed the title floitsch/i2c register provider feat(i2c): add synchronous slave transmit provider Aug 6, 2026
@floitsch
floitsch force-pushed the floitsch/i2c-register-provider branch from e63b8f7 to 7b6ac1f Compare August 10, 2026 12:42
@floitsch
floitsch force-pushed the floitsch/i2c-register-provider branch from 7b6ac1f to e589619 Compare August 10, 2026 14:28
@floitsch

Copy link
Copy Markdown
Member Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
✅ 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

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR adds synchronous transmit and transmit-completion callbacks to the version-two I2C slave driver. It updates ISR FIFO handling and callback validation. It also corrects 10-bit I2C master read-after-restart address sequencing.

Changes

I2C slave transmit callbacks

Layer / File(s) Summary
Transmit callback contract
components/esp_driver_i2c/include/driver/i2c_types.h, components/esp_driver_i2c/include/driver/i2c_slave.h
The API defines callbacks for supplying synchronous transmit data and reporting transmitted length.
Callback registration and state
components/esp_driver_i2c/i2c_private.h, components/esp_driver_i2c/i2c_slave_v2.c
The driver stores transmit callbacks, validates their support and memory location, and rejects i2c_slave_write when a transmit callback is registered.
Transmit FIFO servicing and completion
components/esp_driver_i2c/i2c_slave_v2.c
The ISR requests bounded callback data, writes it to the TX FIFO, tracks the byte count, handles completion, and prevents duplicate TX processing.

10-bit I2C master read addressing

Layer / File(s) Summary
10-bit read address sequencing
components/esp_driver_i2c/i2c_master.c
The driver emits the extra address preamble only for the first transaction and writes one byte for the read address.

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

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: adding synchronous slave transmit support for I2C.
Description check ✅ Passed The description accurately covers the synchronous transmit provider, transmitted-byte reporting, the 10-bit read fix, dependencies, and testing.
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 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 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-register-provider

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 coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_private.h`:
- Line 224: Replace tx_data_count in components/esp_driver_i2c/i2c_private.h:224
with state that tracks callback-origin bytes still pending in the FIFO. Update
FIFO loading in components/esp_driver_i2c/i2c_slave_v2.c:73-92 to account
separately for callback and ring-buffer data, and update transmitted calculation
at lines 126-133 to count only callback bytes consumed. At lines 575-576,
serialize callback-mode transitions with pending TX state or preserve source
accounting for already queued data.
🪄 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: e3e64984-9bc2-46fb-b503-b4be14c11b85

📥 Commits

Reviewing files that changed from the base of the PR and between 2fda6ba and e589619.

📒 Files selected for processing (5)
  • components/esp_driver_i2c/i2c_master.c
  • components/esp_driver_i2c/i2c_private.h
  • components/esp_driver_i2c/i2c_slave_v2.c
  • components/esp_driver_i2c/include/driver/i2c_slave.h
  • components/esp_driver_i2c/include/driver/i2c_types.h

Comment thread components/esp_driver_i2c/i2c_private.h
@floitsch
floitsch force-pushed the floitsch/i2c-register-provider branch from 738be03 to a3ff58b Compare August 10, 2026 17:58
@floitsch
floitsch marked this pull request as ready for review August 10, 2026 19:40
@floitsch
floitsch merged commit eafad99 into patch-head-5.4.2 Aug 10, 2026
2 checks passed
@floitsch
floitsch deleted the floitsch/i2c-register-provider branch August 10, 2026 21:44
floitsch added a commit that referenced this pull request Aug 10, 2026
## Summary

- Make asynchronous controller callbacks identify the actual transaction
device rather than searching by raw address.
- Apply per-device ACK policy and retain NACK/timeout status until the
transaction is terminal, producing exactly one accurate completion
callback.
- Delay normal completion until every command batch has been sent, clear
stale interrupt state before a new transfer, and mask terminal timeout
interrupts.
- Recover after NACK/timeout with a bounded FSM reset and return
`ESP_ERR_INVALID_STATE` for an unexpectedly busy bus instead of entering
synchronous bus-clear logic.

## Dependencies

- Builds on #122.
- Required by toitlang/toit#3156.

## 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.

<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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant