Add register-backed I2C targets - #3155
Conversation
e7d35e6 to
6457522
Compare
6457522 to
74ab798
Compare
a959e0d to
51a7be5
Compare
## 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>
51a7be5 to
d32a577
Compare
d32a577 to
612d88d
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughAdds a public ChangesI2C register target
Estimated code review effort: 4 (Complex) | ~60 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/resources/i2c_esp32.cc (2)
193-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProtect
dropped_write_count_like the counters inI2cTargetResource.
receive_from_isrincrementsdropped_write_count_from ISR context.dropped_write_count()reads it from a Toit task without synchronization.I2cTargetResourceusesspinlock_for the equivalent counters. Add the same spinlock, or document why an unsynchronizedwordaccess is sufficient here.Also applies to: 241-241, 253-253
🤖 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 `@src/resources/i2c_esp32.cc` at line 193, Protect all accesses to dropped_write_count_ in receive_from_isr, dropped_write_count(), and the additional counter-update sites with the same spinlock_ synchronization used by I2cTargetResource; otherwise document a concrete guarantee that unsynchronized ISR/task word accesses are safe.
597-599: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the stretch-support guard to the start of
register_target_create.When
SOC_I2C_SLAVE_CAN_GET_STRETCH_CAUSEis0,FAIL(UNSUPPORTED)returns unconditionally. The remaining declarations and setup are unreachable. The macro is correct for address-match clock stretching, includingI2C_SLAVE_STRETCH_CAUSE_ADDRESS_MATCH.🤖 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 `@src/resources/i2c_esp32.cc` around lines 597 - 599, Move the `#if` !SOC_I2C_SLAVE_CAN_GET_STRETCH_CAUSE FAIL(UNSUPPORTED) guard to the beginning of register_target_create, before any declarations or setup. Preserve the existing guard condition and unsupported return behavior, including support for I2C_SLAVE_STRETCH_CAUSE_ADDRESS_MATCH.tests/hw/esp32/i2c-register-target-shared.toit (1)
273-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a test with overlapping register access.
Line 275 runs the controller operation after board 2 has stopped accessing
target. Lines 276 and 282 then requireFINISHbefore board 2 reads the register data. This serializes every tested controller transfer and Toit-side register operation.Add a hardware test that performs
target.readortarget.writewhile board 1 performs controller reads and writes. This validates the concurrent ISR and Toit access contract stated in the PR objective.🤖 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-register-target-shared.toit` around lines 273 - 283, Add a hardware test in the shared I2C register-target test suite that overlaps a Toit-side target.read or target.write with board 1 controller read/write activity, rather than waiting for the controller operation to finish first. Reuse the existing expect-after/expect-dropped-after helpers and synchronization protocol where appropriate, and verify the register access completes correctly under concurrent ISR and Toit access.
🤖 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 694-698: Update read-bytes to validate reg before
byte-order_.put-uint, rejecting values outside the register-size range with
OUT_OF_RANGE; preserve the existing encoding and device_.read-address flow for
valid values.
In `@src/resources/i2c_esp32.cc`:
- Around line 191-227: Update receive_from_isr so the selected register address
remains separate from the write cursor: preserve register_pointer_ at the
decoded address while writing data through a separate cursor, or restore it
after the write. Ensure transmit_pointer_ and subsequent transmit_done_from_isr
advancement start from the selected address, so a read advances from address 0
to 1 regardless of prior written bytes.
- Around line 535-562: Replace RTC_IRAM_ATTR with IRAM_ATTR on
register_target_receive_handler, register_target_transmit_handler, and
register_target_transmit_done_handler so all three callbacks reside in IRAM and
pass ESP-IDF’s IRAM-safe registration checks.
---
Nitpick comments:
In `@src/resources/i2c_esp32.cc`:
- Line 193: Protect all accesses to dropped_write_count_ in receive_from_isr,
dropped_write_count(), and the additional counter-update sites with the same
spinlock_ synchronization used by I2cTargetResource; otherwise document a
concrete guarantee that unsynchronized ISR/task word accesses are safe.
- Around line 597-599: Move the `#if` !SOC_I2C_SLAVE_CAN_GET_STRETCH_CAUSE
FAIL(UNSUPPORTED) guard to the beginning of register_target_create, before any
declarations or setup. Preserve the existing guard condition and unsupported
return behavior, including support for I2C_SLAVE_STRETCH_CAUSE_ADDRESS_MATCH.
In `@tests/hw/esp32/i2c-register-target-shared.toit`:
- Around line 273-283: Add a hardware test in the shared I2C register-target
test suite that overlaps a Toit-side target.read or target.write with board 1
controller read/write activity, rather than waiting for the controller operation
to finish first. Reuse the existing expect-after/expect-dropped-after helpers
and synchronization protocol where appropriate, and verify the register access
completes correctly under concurrent ISR and Toit access.
🪄 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: 6d313d41-649d-4a9f-992c-99e13e98ea5c
📒 Files selected for processing (9)
lib/i2c.toitsrc/compiler/propagation/type_primitive_i2c.ccsrc/primitive.hsrc/resources/i2c_esp32.ccsrc/tags.htests/hw/esp32/i2c-register-target-board1.toittests/hw/esp32/i2c-register-target-board2.toittests/hw/esp32/i2c-register-target-shared.toitthird_party/esp-idf
612d88d to
66bf00d
Compare
1228737 to
e415498
Compare
e415498 to
0973ff0
Compare
1d6f501 to
1b92c97
Compare
af147be to
745bd66
Compare
8e0d1c5 to
3f53dcc
Compare
367a21f to
5283283
Compare
5283283 to
834debb
Compare
## Summary - Migrate controller probe, read, write, and write-read operations to the asynchronous ESP-IDF driver while preserving the synchronous-looking Toit API. - Serialize complete controller transactions per bus while keeping native primitive calls nonblocking. - Copy moving-heap data into per-operation native buffers and release it after completion or synchronous abort. - Finalize dispatched operations in a non-cancelable cleanup path; abort retires the transaction and then performs best-effort bus recovery. - Expose per-device clock-stretch timeout and ACK-check policy, preserve 7-bit/10-bit addressing, and return explicit `I2C_NACK` / `I2C_TIMEOUT` errors. - Route synchronous `ESP_ERR_NO_MEM` through the existing retryable OOM path after cleaning all partial state; callbacks allocate nothing and cannot report OOM. - Keep Toit I2C callbacks in IRAM only when `CONFIG_I2C_ISR_IRAM_SAFE` is enabled. ## Dependencies - Builds on #3155. - The required ESP-IDF patch head is already on master. ## Testing - Host, ESP32, and ESP32-S3 builds. - `toit analyze -Werror` for changed libraries and two-device hardware tests. - Paired-board coverage includes present/missing probes and scan, NACK and post-NACK recovery, ACK-check disable, 7-bit/10-bit device identity, 1–1024-byte transfers, FIFO/command-boundary sizes, concurrent task serialization, cancellation during clock stretching, and bus reuse. - Complete I2C/SPI hardware selections pass on ESP32 and ESP32-S3 both after setup and immediately without fixture setup. - `git diff --check` in both repositories. <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>
Summary
i2c.RegisterTarget, an autonomous native register map for common I2C peripherals.CONFIG_SPI_MASTER_ISR_IN_IRAM=yon classic ESP32.Dependencies
Testing
Stack created with GitHub Stacks CLI • Give Feedback 💬