Add asynchronous I2C target support - #3154
Conversation
fb73041 to
49e55f8
Compare
## 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. <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>
49e55f8 to
14f6fea
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughAdds an I2C ChangesI2C target support
Estimated code review effort: 5 (Critical) | ~90 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: 3
🧹 Nitpick comments (1)
tests/hw/esp32/i2c-target-shared.toit (1)
139-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the reconfigure repeats the same configuration.
Line 141 reconfigures to
SMALL-BUFFER-CONFIGwhile the target already uses that configuration. The reason is the counter reset that the new target provides, which makesexpect-equals 1 target.dropped-receive-countat Line 224 deterministic. State that reason in the comment, otherwise the call looks redundant.📝 Proposed comment
// Distinguish a transaction larger than the driver's receive buffer from // an application that merely leaves too many complete transactions unread. + // Recreating the target also resets the dropped-receive counter, so the + // expected count below is exact. reconfigure port SMALL-BUFFER-CONFIG🤖 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 `@tests/hw/esp32/i2c-target-shared.toit` around lines 139 - 145, Update the comment above the reconfigure call in the transaction-overflow test to explain that repeating SMALL-BUFFER-CONFIG resets the new target’s counters, making expect-equals 1 target.dropped-receive-count deterministic.
🤖 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 `@lib/i2c.toit`:
- Around line 160-182: Update write so each `i2c-target-write_` result of `-1`
waits on `REQUEST-STATE_` instead of yielding and immediately retrying. Preserve
the existing offset updates and successful completion returns, while allowing
other native errors to propagate unchanged.
- Around line 126-141: Update ResourceState_.dispose or Target.close to
explicitly notify all tasks waiting on the resource state before deleting the
monitor notifier and unregistering the native I2C resource. Ensure blocked read,
write, and wait-for-read-request operations wake and then observe the existing
"CLOSED" behavior.
In `@src/resources/i2c_esp32.cc`:
- Around line 381-392: Ensure the I2C event queue follows the queue-set
lifecycle contract: in src/resources/i2c_esp32.cc lines 381-392, call
group->register_resource(resource) before i2c_slave_register_event_callbacks so
the queue is empty and joined before ISR events can arrive; in
src/resources/i2c_esp32.cc lines 82-87, ensure unregister_resource removes the
queue from the queue set before I2cTargetResource destroys it with
vQueueDeleteWithCaps; in src/event_sources/ev_queue_esp32.cc lines 30-35, check
xQueueAddToSet’s result and report failure when the PERIPHERAL_EVENT_QUEUE_COUNT
budget is exhausted.
---
Nitpick comments:
In `@tests/hw/esp32/i2c-target-shared.toit`:
- Around line 139-145: Update the comment above the reconfigure call in the
transaction-overflow test to explain that repeating SMALL-BUFFER-CONFIG resets
the new target’s counters, making expect-equals 1 target.dropped-receive-count
deterministic.
🪄 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: 4757414c-0c25-4a9e-a2d9-4f54a5c99c9f
📒 Files selected for processing (17)
lib/i2c.toitsrc/compiler/propagation/type_primitive_i2c.ccsrc/event_sources/ev_queue_esp32.ccsrc/primitive.hsrc/resources/i2c_esp32.ccsrc/tags.htests/hw/esp32/i2c-target-board1.toittests/hw/esp32/i2c-target-board2.toittests/hw/esp32/i2c-target-pullup-test.toittests/hw/esp32/i2c-target-shared.toitthird_party/esp-idftoolchains/esp32/sdkconfigtoolchains/esp32c3/sdkconfigtoolchains/esp32c6/sdkconfigtoolchains/esp32p4/sdkconfigtoolchains/esp32s2/sdkconfigtoolchains/esp32s3/sdkconfig
| An addressable I2C target. | ||
|
|
||
| A target receives complete controller write transactions with $read and queues | ||
| bytes for controller read transactions with $write. Native primitives never |
There was a problem hiding this comment.
No need to state this explicitly. That's just how all of Toit works.
| supported by every ESP32 variant and cannot be combined with a 10-bit | ||
| address. | ||
|
|
||
| $allow-power-down allows ESP-IDF to power down the peripheral during light |
There was a problem hiding this comment.
Toit doesn't yet really deal with light sleep. (I believe).
I would remove the comment from the toitdocs and remove the argument.
The primitive can still have the flag.
There was a problem hiding this comment.
There is a public light-sleep API in esp32.pm-configure, so Toit has some support. However, the shipped ESP32 sdkconfigs have CONFIG_PM_ENABLE disabled, and this target-retention path is not tested. I agree that exposing a new permanent Target option is premature: remove it from the public constructor/docs and keep the native parameter available with false passed for now.
| sleep on chips that support peripheral retention. | ||
| */ | ||
| constructor | ||
| --sda/any |
There was a problem hiding this comment.
Don't support gpio.Pin. It's deprecated for the other i2c code already.
| offset += written | ||
| if offset == bytes.size: return | ||
|
|
||
| // Retry before waiting so a controller read that raced with the first |
There was a problem hiding this comment.
I don't understand this comment/special case.
There was a problem hiding this comment.
The special value is a nonblocking retry indication: the primitive maps ESP_ERR_TIMEOUT from i2c_slave_write to -1 because it calls ESP-IDF with timeout zero. In this driver, that timeout only means operation_mux was momentarily busy; it does not mean the TX buffer is full. A full buffer returns ESP_OK with written == 0. The yield retries mutex contention without blocking a primitive. Under Toit’s serialized primitive execution this should be very rare, but the handling is defensive.
| state_.clear-state REQUEST-STATE_ | ||
| count := i2c-target-take-request-count_ resource_ | ||
| if count != 0: return count | ||
| count = i2c-target-take-request-count_ resource_ |
There was a problem hiding this comment.
I agree that the second write is unnecessary. REQUEST-STATE_ is level-triggered in ResourceState_: after clear-state, a request racing with the write either sets the bit before wait-for-state or wakes it afterwards. The extra native call does not close a remaining lost-wakeup race. The same reasoning applies to the duplicated receive and request-count checks above.
| // member queue lengths. | ||
| #define PERIPHERAL_EVENT_QUEUE_COUNT 16 | ||
| #define MAX_QUEUE_SET_SIZE \ | ||
| (GPIO_QUEUE_SIZE + 2 * UART_QUEUE_SIZE + STDIN_QUEUE_SIZE + PERIPHERAL_EVENT_QUEUE_COUNT + 1) |
There was a problem hiding this comment.
Would be interesting to see if we forgot this for other events.
There was a problem hiding this comment.
Yes. At this PR’s base, the old formula also omitted ESP-NOW’s length-3 event queue and the length-1 queues created per I2S and RMT resource. Unused UART capacity probably masked that in common configurations. The new 16-slot peripheral budget covers those existing users as well as I2C targets; registration already fails loudly if the shared budget is exhausted.
| : ResourceGroup(process) {} | ||
| }; | ||
|
|
||
| const word kTargetReceiveState = 1 << 0; |
There was a problem hiding this comment.
should we create a separate file for the target?
or is there a lot of code that is easier to share with the controller code?
There was a problem hiding this comment.
There is little target/controller state worth sharing. The practical reason to keep them together is MODULE_IMPLEMENTATION: it declares the module’s primitive functions static and builds one table in this translation unit. A clean split would need primitive-registration/header plumbing, or an included implementation fragment. For roughly 300 target-specific lines I do not think that refactor belongs in this PR.
| # CONFIG_I2C_ISR_IRAM_SAFE is not set | ||
| # CONFIG_I2C_ENABLE_DEBUG_LOG is not set | ||
| # CONFIG_I2C_ENABLE_SLAVE_DRIVER_VERSION_2 is not set | ||
| CONFIG_I2C_ENABLE_SLAVE_DRIVER_VERSION_2=y |
There was a problem hiding this comment.
look at what 'make menuconfig' does, since it affects the sdkconfig.default which need to be updated as well.
87a29e9 to
490bb9c
Compare
c468664 to
310637e
Compare
2a01843 to
ba32282
Compare
46a9646 to
9c6bef0
Compare
9c6bef0 to
4e24611
Compare
Adds a `--pull-up` option to `rmt.In`, matching the integer-pin ownership model already used by RMT outputs. The ESP32 primitive applies the pull after the RMT driver configures an owned pin, while the deprecated `gpio.Pin` overload preserves its Toit-side configuration path. The hardware RMT suite exercises the new option.\n\nThis is the new base of the I2C/SPI target stack; #3154 follows it.
## Summary - Add `i2c.RegisterTarget`, an autonomous native register map for common I2C peripherals. - Serve register reads directly from C++/ISR-safe storage so the target is immediately ready and does not need Toit-side clock stretching. - Support one- and two-byte register addresses, wrapping reads, Toit-side register updates, and dropped-write accounting. - Allocate the complete register backing store during construction; callbacks perform no allocation and never report asynchronous OOM. - Keep concurrent Toit updates and ISR reads race-free while retaining `CONFIG_SPI_MASTER_ISR_IN_IRAM=y` on classic ESP32. ## Dependencies - Builds on #3154. - The required ESP-IDF patch head is already on master. ## Testing - Two-device hardware tests on ESP32 and ESP32-S3. - Covers address widths, register address widths, wrapping, partial updates, concurrent access, oversized/dropped writes, and controller/target teardown and reuse. - The #3155 branch compiles and links on classic ESP32 with SPI master ISR IRAM enabled. <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>
1 participant
Summary
i2c.TargetAPI with 7-bit and 10-bit addressing, broadcast support, bounded send/receive buffers, overflow reporting, and explicit read-request handling.ResourceState_, outside the primitive call.ESP-IDF dependency
Builds on #3203. The required ESP-IDF patch head is already on master.
Testing
Stack created with GitHub Stacks CLI • Give Feedback 💬