esp32: migrate i2c drivers to the new I2C master driver (i2c_master.h)#2349
Draft
harmon25 wants to merge 3 commits into
Draft
esp32: migrate i2c drivers to the new I2C master driver (i2c_master.h)#2349harmon25 wants to merge 3 commits into
harmon25 wants to merge 3 commits into
Conversation
Migrate both the "i2c" port driver and the i2c NIF-resource driver
from the legacy driver/i2c.h API to the new driver/i2c_master.h API.
The legacy driver is end-of-life as of IDF v6.0 and scheduled for
removal in v7.0.
- Add a shared i2c_bus_manager helper used by both implementations for
bus open/close and per-address device add/transact/remove, since the
new driver requires a persistent per-address device handle while
AtomVM's i2c API takes the target address on every call (e.g. the
i2c_scanner example probes arbitrary addresses on one open bus).
- Replace the legacy command-link queuing (begin_transmission/
write_byte(s)/end_transmission) with a small growable byte buffer
that is flushed as a single i2c_master_transmit() call.
- Redefine i2c_driver_acquire/i2c_driver_release to hand out the new
i2c_master_bus_handle_t instead of i2c_port_t; this API has no
in-tree callers and was already documented as subject to change.
- Normalize the new driver's NACK error codes (ESP_ERR_INVALID_STATE
on IDF <= 5.5, ESP_ERR_INVALID_RESPONSE on IDF >= 6.0) back to
ESP_FAIL, preserving the {error, esp_fail} contract existing
applications and the i2c test suite rely on.
- Link esp_driver_i2c explicitly for IDF >= 6.0, matching the existing
esp_driver_dac/tinyusb pattern in this component.
Verified by building libavm_builtins.a against real ESP-IDF v5.2.7 and
v5.5.4 for both esp32 and esp32c3 targets.
Signed-off-by: harmon25 <dougwright1@gmail.com>
- i2c_bus_manager_transmit: the new driver's i2c_master_transmit()
rejects zero-length write buffers with ESP_ERR_INVALID_ARG, unlike
the legacy driver, which happily emitted a bare START/address/STOP
sequence. This broke the address-only ACK/NACK probe idiom (e.g.
i2c:write_bytes(I2C, Addr, <<>>), or an empty
begin_transmission/end_transmission pair), commonly used for device
presence or busy-polling checks. A write_size == 0 now falls back to
i2c_master_probe(), normalizing ESP_ERR_NOT_FOUND to ESP_FAIL to
match the existing {error, esp_fail} contract.
- i2c_resource: close_i2c_resource now always releases the pending
I2CTxBuffer, even if closing the underlying bus handle fails,
avoiding a leak of the buffer across the resource's lifetime.
- i2cdriver_write_bytes / nif_i2c_write_bytes: only allocate a scratch
buffer when a register prefix is present; otherwise the caller's
data pointer is passed directly to the transmit call, avoiding an
unnecessary alloc + copy on the common register-less write path.
Verified by building against ESP-IDF v5.5.4 for both esp32 and
esp32s3 targets (no warnings).
Signed-off-by: harmon25 <dougwright1@gmail.com>
Author
|
Pushed a follow-up commit (d424a5a) addressing a couple of issues found in self-review:
Re-verified by building against ESP-IDF v5.5.4 for both |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL's atomvm/term-use-after-gc check flagged make_einprogress_error: it took the transmitting pid as a 'term owner_pid' parameter, which callers populated from i2c_data->transmitting_pid *before* the function's internal memory_ensure_free() call, then used *after* it. That capture-before-GC/use-after-GC shape is exactly the hazard the check looks for, since a GC can move/invalidate a previously read boxed term. transmitting_pid is in practice always immediate (a local pid or term_invalid_term()), so this was not an exploitable bug, but the pattern is fragile and worth fixing structurally rather than suppressing. make_einprogress_error now takes the owning 'struct I2CData *' instead, and reads i2c_data->transmitting_pid directly after the GC check, removing the flagged pattern entirely. All six call sites are updated accordingly. Addresses the github-advanced-security bot review comment on PR atomvm#2349. Signed-off-by: harmon25 <dougwright1@gmail.com>
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
2 participants
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
Migrates AtomVM's ESP32 I2C stack off the legacy
driver/i2c.hAPI (end-of-life as of IDF v6.0, scheduled for removal in v7.0) onto the newdriver/i2c_master.hAPI. Both existing implementations are migrated:"i2c"port driver (i2c_driver.c, defaulti2c:open/1path)i2c_resource.c, opt-in via{use_nif, true})No changes to the Erlang-level
i2cAPI (libs/avm_esp32/src/i2c.erl,libs/eavmlib/src/i2c_hal.erl) or to STM32/RP2 drivers.Design
i2c_bus_managerhelper (newi2c_bus_manager.c/include/i2c_bus_manager.h): owns bus open/close and per-address device handling. Because AtomVM'si2cAPI takes the target address on every call rather than at open time (e.g.i2c_scanner.erlprobes arbitrary addresses on one open bus), while the new driver requires a persistent per-address device handle for any transaction, this helper adds a transient device handle immediately before each transaction and removes it immediately after.i2c_master_cmd_begin()via a command link built up acrossbegin_transmission/write_byte(s)/end_transmissioncalls. The new driver has no command-link equivalent, so a small growableI2CTxBufferaccumulates the bytes instead, flushed as a singlei2c_master_transmit()call fromend_transmission.i2c_driver_acquire/i2c_driver_release(native C extension point for other drivers to share the port driver's bus) now hand outi2c_master_bus_handle_tinstead ofi2c_port_t. This header was already explicitly markedATOMVM_ESP32_I2C_OLD_API/"will be changed in future," and has no in-tree callers.ESP_ERR_INVALID_STATE(IDF <= 5.5) orESP_ERR_INVALID_RESPONSE(IDF >= 6.0), whereas the legacy driver always returnedESP_FAIL. Both codes are normalized back toESP_FAILini2c_bus_manager.cto preserve the{error, esp_fail}contract that existing applications andtest_i2c.erlrely on, across all supported IDF versions.esp_driver_i2cis linked explicitly for IDF >= 6.0 (where the legacydrivercomponent drops its public dependency on it), mirroring the existingesp_driver_dac/tinyusb pattern already used inavm_builtins/CMakeLists.txt.Testing
No ESP-IDF toolchain was available locally, so I pulled the same
espressif/idfDocker images used by CI and builtlibavm_builtins.adirectly:I also manually traced
test_i2c.erl(BMP180 register read/write, split transactions,einprogressand NACK error paths) andi2c_scanner.erl(arbitrary-address probing) against the new implementation to confirm behavioral compatibility. I was not able to run the Wokwi-simulated hardware tests locally (requires aWOKWI_CLI_TOKEN), so CI coverage fortest_i2cviaesp32-simtest.yaml/esp32-build.yamlis relied on to validate actual bus transactions.Opening as draft pending CI results, since I could not execute the full on-target test suite locally.