Migrate I2C controller operations to async - #3156
Conversation
4c31bd3 to
f3cf340
Compare
f3cf340 to
ce68a9f
Compare
ce68a9f to
5303ce1
Compare
66a3170 to
dc12d52
Compare
48de4dd to
b979f55
Compare
## 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>
b979f55 to
8ebebe5
Compare
8ebebe5 to
6b39845
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughThe PR changes ESP32 I2C operations to asynchronous native transactions with event-based completion. Toit APIs now serialize operations, support timeouts and ACK configuration, map native results, and coordinate shutdown. New board tests cover the controller behavior. ChangesAsynchronous I2C controller
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tests/hw/esp32/i2c-async-controller-shared.toit (1)
126-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe quartile split does not separate the two frequencies.
low-periodsholds SCL low periods from the 50 kHz transfer and the 400 kHz transfer in one list. The code sorts the list and then reads the lower quartile asfast-lowand the upper quartile asslow-low. This works only if the two transfers contribute similar sample counts and if no idle or stretched period lands in either quartile. Theslow.write-read #[0] 8transfer and thefast.write-read #[0] 8transfer produce the same number of clock pulses, so the assumption holds today, but any change of transfer size makes the thresholds silently wrong.Capture the two transfers in separate reads, or compute the medians of the two halves that are split by the long gap between transfers.
🤖 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-async-controller-shared.toit` around lines 126 - 138, Update the frequency probe logic around low-period collection so the 50 kHz and 400 kHz transfer samples are measured separately rather than relying on quartiles from the combined low-periods list. Preserve the existing slow-low and fast-low assertions by assigning each value from its corresponding transfer, and remove the assumption that both transfers contribute equal sample counts.lib/i2c.toit (1)
474-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck the Toitdoc reference and the duplicated default value.
Two small points:
- Line 475 references
$(device i2c-address --frequency --address-size). The primary overload now also declares--timeout-usand--disable-ack-check. Confirm that the Toitdoc resolver still matches this overload, otherwise the reference breaks and the analyzer can report an unresolved reference.- The default
--timeout-us/int=100_000is repeated in both overloads (Line 458 and Line 480). If one default changes, the two overloads diverge silently. Consider a named constant, for exampleDEFAULT-TIMEOUT-US.#!/bin/bash # Look for existing toitdoc references to overloaded methods with partial named-argument lists. rg -n '\$\(device ' lib/ rg -n 'DEFAULT-.*-US|DEFAULT_.*_US' lib/i2c.toit🤖 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 `@lib/i2c.toit` around lines 474 - 487, Update the Toitdoc reference for i2c-address so it resolves unambiguously to the overload that includes --frequency, --address-size, --timeout-us, and --disable-ack-check. Introduce a shared DEFAULT-TIMEOUT-US constant and use it for the --timeout-us defaults in both i2c-address overloads, preserving the current default behavior.
🤖 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 387-398: Update perform-controller-operation_ so the completion
wait is bounded rather than unconditionally blocking inside critical-do; derive
the deadline from the configured device timeout plus a watchdog margin, preserve
native-buffer lifetime until completion, and report I2C_TIMEOUT when
CONTROLLER-DONE-STATE_ is not signaled. Ensure timeout handling releases mutex_
so subsequent operations and close can proceed.
- Around line 433-441: In lib/i2c.toit lines 433-441, update Bus.close so its
mutex-protected teardown runs inside critical-do, and snapshot devices_.values
before clearing the map so Device.close-native_ cannot mutate devices_ during
iteration. In lib/i2c.toit lines 746-758, update the bus.mutex_ section around
close-native_ to use critical-do, ensuring all four state updates complete
atomically.
In `@src/resources/i2c_esp32.cc`:
- Around line 879-890: Update bus_probe and its Toit-layer call sites so the
temporary i2c_device_config_t uses the bus’s configured frequency instead of
hardcoded 100000. Propagate the existing bus frequency through the relevant API,
preserving the configured speed for both scan and test probing paths.
- Around line 1073-1076: Add the same closed-device guard used by device_write,
device_read, and device_write_read to device_write_finish, device_read_finish,
and device_write_read_finish. Check resource->handle() == null and return
ALREADY_CLOSED before calling resource->bus() or finish_controller_operation,
preventing null-bus dereferences.
- Around line 391-409: Update the callback-unregistration calls in the resource
cleanup paths, including the device-destruction flow and
I2cBusResource::finish_operation, to capture and validate the result of
i2c_master_register_event_callbacks before calling i2c_master_bus_rm_device or
freeing buffers. Handle ESP_ERR_INVALID_STATE according to the project’s
established error-handling pattern, and only continue destruction or buffer
cleanup after the callback state is safely handled.
In `@tests/hw/esp32/i2c-async-controller-shared.toit`:
- Line 111: Remove the redundant architecture guards in test-board1, eliminate
the return that exits before cleanup, and ensure the normal CLOSE handshake, bus
reuse check, and port.close execute before any destructive timeout path. Update
test-board2 so its CLOSE branch remains reachable and closes targets and the
UART normally; remove the duplicated bus reuse logic while preserving the
intended cleanup behavior.
- Around line 97-109: Remove the --unwind option from the catch expression
inside the task block so exceptions are stored in errors[task-index], allowing
done[task-index].set true to execute and the later errors.do propagation to
report failures without hanging.
---
Nitpick comments:
In `@lib/i2c.toit`:
- Around line 474-487: Update the Toitdoc reference for i2c-address so it
resolves unambiguously to the overload that includes --frequency,
--address-size, --timeout-us, and --disable-ack-check. Introduce a shared
DEFAULT-TIMEOUT-US constant and use it for the --timeout-us defaults in both
i2c-address overloads, preserving the current default behavior.
In `@tests/hw/esp32/i2c-async-controller-shared.toit`:
- Around line 126-138: Update the frequency probe logic around low-period
collection so the 50 kHz and 400 kHz transfer samples are measured separately
rather than relying on quartiles from the combined low-periods list. Preserve
the existing slow-low and fast-low assertions by assigning each value from its
corresponding transfer, and remove the assumption that both transfers contribute
equal sample counts.
🪄 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: 915e0bb1-7d91-47ad-97fd-3e519b6bc5ac
📒 Files selected for processing (8)
lib/i2c.toitsrc/compiler/propagation/type_primitive_i2c.ccsrc/primitive.hsrc/resources/i2c_esp32.cctests/hw/esp32/i2c-async-controller-board1.toittests/hw/esp32/i2c-async-controller-board2.toittests/hw/esp32/i2c-async-controller-shared.toitthird_party/esp-idf
| perform-controller-operation_ [start] [finish]: | ||
| return mutex_.do: | ||
| if not resource_: throw "CLOSED" | ||
| result := null | ||
| // Once dispatched, the native buffers must remain alive until finish. | ||
| critical-do --no-respect-deadline: | ||
| state_.clear-state CONTROLLER-DONE-STATE_ | ||
| start.call | ||
| state_.wait-for-state CONTROLLER-DONE-STATE_ | ||
| result = finish.call | ||
| return result | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The completion wait is unbounded and non-cancelable while holding the bus mutex.
critical-do --no-respect-deadline makes state_.wait-for-state CONTROLLER-DONE-STATE_ immune to task cancellation and to any enclosing with-timeout. If the driver never delivers a completion event for a dispatched transaction, the calling task blocks permanently. The task also holds mutex_, so every other task that uses this bus blocks permanently, and close can never run.
The design relies on the hardware clock-stretch timeout (scl_wait_us) always producing I2C_EVENT_TIMEOUT. Any driver path that drops a queued transaction without a callback (for example a rejected queue insert) turns into a permanent bus deadlock.
Consider adding a bounded wait with a watchdog margin derived from the device timeout, and reporting I2C_TIMEOUT if the completion event does not arrive.
🤖 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 `@lib/i2c.toit` around lines 387 - 398, Update perform-controller-operation_ so
the completion wait is bounded rather than unconditionally blocking inside
critical-do; derive the deadline from the configured device timeout plus a
watchdog margin, preserve native-buffer lifetime until completion, and report
I2C_TIMEOUT when CONTROLLER-DONE-STATE_ is not signaled. Ensure timeout handling
releases mutex_ so subsequent operations and close can proceed.
| close -> none: | ||
| if not resource_: return | ||
| devices_.values.do: it.close | ||
| devices_.clear | ||
| i2c-bus-close_ resource_ | ||
| resource_ = null | ||
| mutex_.do: | ||
| if not resource_: return | ||
| devices_.values.do: it.close-native_ | ||
| devices_.clear | ||
| state_.dispose | ||
| i2c-bus-close_ resource_ | ||
| resource_ = null | ||
| remove-finalizer this |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The two controller close methods are not cancellation-safe. Both perform multi-step teardown of native resources without critical-do. Target.close at Line 187 and RegisterTarget.close at Line 298 establish the expected pattern in this file. A cancellation in the middle of either sequence leaks the native handle, the reserved GPIO pins, or the map entry.
lib/i2c.toit#L433-L441: wrap theBus.closebody insidemutex_.doincritical-do, and snapshotdevices_.valuesbeforedevices_.clearsoDevice.close-native_does not mutate the map during iteration.lib/i2c.toit#L746-L758: wrap thebus.mutex_.do: close-native_call incritical-doso the four state updates inclose-native_complete atomically.
📍 Affects 1 file
lib/i2c.toit#L433-L441(this comment)lib/i2c.toit#L746-L758
🤖 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 `@lib/i2c.toit` around lines 433 - 441, In lib/i2c.toit lines 433-441, update
Bus.close so its mutex-protected teardown runs inside critical-do, and snapshot
devices_.values before clearing the map so Device.close-native_ cannot mutate
devices_ during iteration. In lib/i2c.toit lines 746-758, update the bus.mutex_
section around close-native_ to use critical-do, ensuring all four state updates
complete atomically.
| i2c_master_event_callbacks_t callbacks = {}; | ||
| i2c_master_register_event_callbacks(device->handle(), &callbacks, null); | ||
| i2c_master_bus_rm_device(device->handle()); | ||
| device->bus_ = null; | ||
| device->handle_ = null; | ||
| DeviceList::unlink(device); | ||
| } | ||
|
|
||
| void I2cBusResource::finish_operation() { | ||
| ASSERT(operation_in_flight_); | ||
| if (probe_handle_ != null) { | ||
| i2c_master_event_callbacks_t callbacks = {}; | ||
| i2c_master_register_event_callbacks(probe_handle_, &callbacks, null); | ||
| i2c_master_bus_rm_device(probe_handle_); | ||
| } | ||
| free(tx_buffer_); | ||
| free(rx_buffer_); | ||
| cancel_prepared_operation(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
ESP-IDF i2c_master_register_event_callbacks null on_trans_done unregister callback
💡 Result:
To unregister a previously registered callback in the ESP-IDF I2C driver, you must call the i2c_master_register_event_callbacks function and set the specific callback member within the i2c_master_event_callbacks_t structure to NULL [1][2][3]. For example, to unregister the on_trans_done callback, you would prepare a structure with that field set to NULL and pass it to the registration function: i2c_master_event_callbacks_t cbs = {.on_trans_done = NULL, }; ESP_ERROR_CHECK(i2c_master_register_event_callbacks(dev_handle, &cbs, NULL)); The official ESP-IDF documentation explicitly confirms this method for deregistering callbacks [2][4][5]. Setting the callback to NULL effectively disables the asynchronous behavior associated with that callback [6][2][7]. When no callback is registered, I2C transactions will typically default to synchronous (blocking) mode [6][7].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/i2c.html
- 2: https://github.com/espressif/esp-idf/blob/v5.3.3/components/esp_driver_i2c/include/driver/i2c_master.h
- 3: https://sourcevu.sysprogs.com/espressif/esp-idf/symbols/i2c_master_register_event_callbacks
- 4: https://documentation.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/i2c.html
- 5: https://docs.espressif.com/projects/esp-idf/en/v6.0/esp32/api-reference/peripherals/i2c.html
- 6: Switch between Synchronous and Asynchronous I2C modes in master mode don't work! (IDFGH-15065) espressif/esp-idf#15749
- 7: https://github.com/espressif/esp-idf/blob/v5.2/components/driver/i2c/include/driver/i2c_master.h
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository references ---'
rg -n -C 4 'i2c_master_register_event_callbacks|i2c_master_bus_rm_device|finish_operation|remove_device' src lib .github 2>/dev/null | head -240
printf '%s\n' '--- ESP-IDF version/configuration references ---'
rg -n -i 'esp-idf|idf_version|ESP_IDF|i2c_master_event_callbacks_t' . --glob '!node_modules/**' --glob '!build/**' --glob '!dist/**' 2>/dev/null | head -240Repository: toitlang/toit
Length of output: 1912
🌐 Web query:
site:docs.espressif.com/projects/esp-idf i2c_master_register_event_callbacks esp_err_t unregister callback on_trans_done
💡 Result:
To unregister a previously registered event callback (such as on_trans_done) in the ESP-IDF I2C driver, you use the same function used for registration: i2c_master_register_event_callbacks [1][2]. Instead of passing a valid function pointer, you must set the specific callback member within the i2c_master_event_callbacks_t structure to NULL and pass that structure to the function [1][2][3]. For example, to unregister the on_trans_done callback: i2c_master_event_callbacks_t cbs = {.on_trans_done = NULL, }; esp_err_t err = i2c_master_register_event_callbacks(i2c_dev_handle, &cbs, NULL); Note that this function is designed to register or update callbacks; setting a member to NULL effectively removes the callback for that specific event [1][2]. Ensure that the i2c_master_event_callbacks_t structure is properly initialized before passing it to the registration function [1].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/v6.0/esp32/api-reference/peripherals/i2c.html
- 2: https://docs.espressif.com/projects/esp-idf/en/v5.5.3/esp32/api-reference/peripherals/i2c.html
- 3: https://docs.espressif.com/projects/esp-idf/en/v6.0.2/esp32c2/api-reference/peripherals/i2c.html
🌐 Web query:
site:github.com/espressif/esp-idf "i2c_master_register_event_callbacks" "ESP_ERR_INVALID_STATE"
💡 Result:
In the ESP-IDF I2C driver, an ESP_ERR_INVALID_STATE error returned by i2c_master_register_event_callbacks generally indicates that the I2C master device handle provided is not in a valid state to register callbacks [1]. Based on the ESP-IDF driver implementation, this error typically occurs for the following reasons: 1. Improper Initialization: The I2C port or the specific device handle (i2c_master_dev_handle_t) may not have been correctly initialized or added to the bus before attempting to register callbacks [1]. 2. Driver Constraints: The API requires the device handle to be properly managed [1]. If the underlying bus or device state is invalid, the operation cannot proceed [1]. Although i2c_master_register_event_callbacks is specifically designed to enable asynchronous I2C transactions [2][3], it requires a valid and active device handle created via i2c_master_bus_add_device. Ensure that: - You have successfully created the bus handle and added the device to the bus. - You are passing the correct i2c_master_dev_handle_t to the function. - The I2C bus itself has not encountered a critical error or entered a faulted state that prevents further configuration [4][1]. For debugging, verify that your code successfully initializes the bus and the device before attempting to register callbacks, and check if any prior I2C calls returned an error. If you are attempting to use asynchronous features, also ensure you are not violating the constraint that only one device per bus can perform asynchronous operations at a time [2][3].
Citations:
- 1: https://github.com/espressif/esp-idf/blob/v5.4/components/esp_driver_i2c/include/driver/i2c_master.h
- 2: https://github.com/espressif/esp-idf/blob/v5.2.5/components/driver/i2c/include/driver/i2c_master.h
- 3: https://github.com/espressif/esp-idf/blob/master/docs/en/api-reference/peripherals/i2c.rst
- 4: https://github.com/espressif/esp-idf/blob/master/components/esp_driver_i2c/i2c_master.c
🏁 Script executed:
#!/bin/bash
set -eu
for url in \
'https://raw.githubusercontent.com/espressif/esp-idf/v5.4/components/esp_driver_i2c/i2c_master.c' \
'https://raw.githubusercontent.com/espressif/esp-idf/v5.4/components/esp_driver_i2c/include/driver/i2c_master.h' \
'https://raw.githubusercontent.com/espressif/esp-idf/v5.2.5/components/driver/i2c/i2c.c' \
'https://raw.githubusercontent.com/espressif/esp-idf/v5.2.5/components/driver/i2c/include/driver/i2c_master.h'
do
echo "--- $url ---"
curl -LfsS "$url" | rg -n -C 12 'i2c_master_register_event_callbacks|register.*callback|unregister|ESP_ERR_INVALID_STATE' | head -220 || true
doneRepository: toitlang/toit
Length of output: 1912
🌐 Web query:
ESP-IDF v5.4 i2c_master_register_event_callbacks return ESP_ERR_INVALID_STATE callback already registered asynchronous transaction source
💡 Result:
The ESP_ERR_INVALID_STATE error when calling i2c_master_register_event_callbacks in ESP-IDF v5.4 typically indicates a violation of the I2C driver's asynchronous operation constraints [1][2]. Specifically, the driver documentation mandates that on any single I2C bus, only one device is permitted to perform asynchronous operations at a time [1][3][4]. If you receive this error, ensure the following conditions are met: 1. Asynchronous Constraint: Verify that no other device on the same I2C bus has already had callbacks registered for asynchronous transactions [1][4]. The driver tracks this state globally for the bus [5]. 2. Proper Deregistration: If you need to switch modes or re-register, you must first properly deregister the existing callbacks for the device. This is done by calling i2c_master_register_event_callbacks with the callback members in the i2c_master_event_callbacks_t structure set to NULL [1][6]. 3. Context Requirements: Ensure that your application logic is not attempting to mix synchronous and asynchronous modes on the same bus incorrectly. Some users have reported challenges when attempting to switch between these modes dynamically, often requiring careful management of the transaction queue and bus state [6]. If you are certain that no other device is using asynchronous mode and you have correctly attempted to clear the callbacks, verify that the I2C port and device handles are initialized correctly, as ESP_ERR_INVALID_STATE can also be returned if the underlying I2C hardware state is invalid (e.g., port not initialized) [2].
Citations:
- 1: https://github.com/espressif/esp-idf/blob/v5.4/components/esp_driver_i2c/include/driver/i2c_master.h
- 2: https://docs.espressif.com/projects/esp-idf/en/v5.4.2/esp32/api-reference/peripherals/i2c.html
- 3: https://github.com/espressif/esp-idf/blob/v5.3.3/components/esp_driver_i2c/include/driver/i2c_master.h
- 4: https://docs.espressif.com/projects/esp-idf/en/v6.0.1/esp32/api-reference/peripherals/i2c.html
- 5: https://github.com/espressif/esp-idf/blob/master/components/esp_driver_i2c/i2c_master.c
- 6: Switch between Synchronous and Asynchronous I2C modes in master mode don't work! (IDFGH-15065) espressif/esp-idf#15749
Handle callback-unregistration errors before destroying the resource.
A zero-initialized i2c_master_event_callbacks_t clears on_trans_done, but i2c_master_register_event_callbacks can return ESP_ERR_INVALID_STATE. Check the result before removing the device or freeing buffers.
🤖 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 391 - 409, Update the
callback-unregistration calls in the resource cleanup paths, including the
device-destruction flow and I2cBusResource::finish_operation, to capture and
validate the result of i2c_master_register_event_callbacks before calling
i2c_master_bus_rm_device or freeing buffers. Handle ESP_ERR_INVALID_STATE
according to the project’s established error-handling pattern, and only continue
destruction or buffer cleanup after the callback state is safely handled.
| i2c_device_config_t config = { | ||
| .dev_addr_length = I2C_ADDR_BIT_LEN_7, | ||
| // The address is emitted explicitly below so its WRITE command can check | ||
| // the ACK. A START/address/STOP transaction generated by the regular | ||
| // device path does not attach ACK checking to the address-only transfer. | ||
| .device_address = I2C_DEVICE_ADDRESS_NOT_USED, | ||
| .scl_speed_hz = 100000, | ||
| .scl_wait_us = static_cast<uint32_t>(timeout_us), | ||
| .flags = { | ||
| .disable_ack_check = false, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The probe frequency is hardcoded to 100 kHz.
bus_probe always creates the temporary device with .scl_speed_hz = 100000, and it ignores the frequency configured for the bus in lib/i2c.toit. A bus that operates at 400 kHz is therefore scanned at a different clock rate than the one used by real transfers. Targets that only respond correctly at the configured speed can produce inconsistent scan results.
Consider passing the bus frequency from the Toit layer to bus_probe, or document the fixed probe speed in the scan and test documentation.
🤖 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 879 - 890, Update bus_probe and its
Toit-layer call sites so the temporary i2c_device_config_t uses the bus’s
configured frequency instead of hardcoded 100000. Propagate the existing bus
frequency through the relevant API, preserving the configured speed for both
scan and test probing paths.
| PRIMITIVE(device_write_finish) { | ||
| ARGS(I2cDeviceResource, resource); | ||
| return finish_controller_operation(resource->bus(), null, 0, process); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The finish primitives do not guard against a closed device.
device_write (Line 1055), device_read (Line 1080), and device_write_read (Line 1104) all check resource->handle() == null and return ALREADY_CLOSED. The three matching finish primitives do not perform any check. They call resource->bus() directly and pass the result to finish_controller_operation, which dereferences it with bus->operation_in_flight().
I2cBusResource::remove_device sets device->bus_ = null, so a device resource can outlive its bus and expose a null pointer here. The Toit layer currently prevents this order, but the native layer must not depend on that.
The same gap exists in device_read_finish (Lines 1095-1100) and device_write_read_finish (Lines 1125-1130).
🛡️ Proposed guard
PRIMITIVE(device_write_finish) {
ARGS(I2cDeviceResource, resource);
+ if (resource->handle() == null || resource->bus() == null) FAIL(ALREADY_CLOSED);
return finish_controller_operation(resource->bus(), null, 0, process);
}Apply the same guard to device_read_finish and device_write_read_finish.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PRIMITIVE(device_write_finish) { | |
| ARGS(I2cDeviceResource, resource); | |
| return finish_controller_operation(resource->bus(), null, 0, process); | |
| } | |
| PRIMITIVE(device_write_finish) { | |
| ARGS(I2cDeviceResource, resource); | |
| if (resource->handle() == null || resource->bus() == null) FAIL(ALREADY_CLOSED); | |
| return finish_controller_operation(resource->bus(), null, 0, process); | |
| } |
🤖 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 1073 - 1076, Add the same
closed-device guard used by device_write, device_read, and device_write_read to
device_write_finish, device_read_finish, and device_write_read_finish. Check
resource->handle() == null and return ALREADY_CLOSED before calling
resource->bus() or finish_controller_operation, preventing null-bus
dereferences.
| done := List 4: monitor.Latch | ||
| errors := List 4: null | ||
| 4.repeat: | task-index/int | | ||
| task:: | ||
| errors[task-index] = catch --unwind: | ||
| 25.repeat: | iteration/int | | ||
| index := task-index * 53 + iteration | ||
| expected := initial[index] | ||
| expect-equals #[expected] (device.write-read #[index] 1) | ||
| done[task-index].set true | ||
| done.do: it.get | ||
| errors.do: | error/any? | | ||
| if error: throw error |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
catch --unwind prevents the error collection and can hang the test.
--unwind rethrows the exception inside the task. The assignment to errors[task-index] and the following done[task-index].set true never run. done.do: it.get then waits forever, and the failure appears as a test hang instead of the intended error report.
Remove --unwind so the error is captured and the latch is always set.
🐛 Proposed fix
4.repeat: | task-index/int |
task::
- errors[task-index] = catch --unwind:
+ errors[task-index] = catch:
25.repeat: | iteration/int |
index := task-index * 53 + iteration
expected := initial[index]
expect-equals #[expected] (device.write-read #[index] 1)
done[task-index].set true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| done := List 4: monitor.Latch | |
| errors := List 4: null | |
| 4.repeat: | task-index/int | | |
| task:: | |
| errors[task-index] = catch --unwind: | |
| 25.repeat: | iteration/int | | |
| index := task-index * 53 + iteration | |
| expected := initial[index] | |
| expect-equals #[expected] (device.write-read #[index] 1) | |
| done[task-index].set true | |
| done.do: it.get | |
| errors.do: | error/any? | | |
| if error: throw error | |
| done := List 4: monitor.Latch | |
| errors := List 4: null | |
| 4.repeat: | task-index/int | | |
| task:: | |
| errors[task-index] = catch: | |
| 25.repeat: | iteration/int | | |
| index := task-index * 53 + iteration | |
| expected := initial[index] | |
| expect-equals #[expected] (device.write-read #[index] 1) | |
| done[task-index].set true | |
| done.do: it.get | |
| errors.do: | error/any? | | |
| if error: throw error |
🤖 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-async-controller-shared.toit` around lines 97 - 109,
Remove the --unwind option from the catch expression inside the task block so
exceptions are stored in errors[task-index], allowing done[task-index].set true
to execute and the later errors.do propagation to report failures without
hanging.
| errors.do: | error/any? | | ||
| if error: throw error | ||
|
|
||
| if system.architecture != system.ARCHITECTURE-ESP32: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Lines 199-210 are unreachable, and the two architecture guards are redundant.
test-board1 returns at line 53 for ESP32. All code after line 54 runs only on non-ESP32 targets. Therefore:
- The guards at line 111 and line 161 are always true.
- The block at line 161 ends with
returnat line 197. TheCLOSEhandshake, the bus reuse check, andport.closeat lines 199-210 never run.
The missing CLOSE handshake also makes the CLOSE branch in test-board2 unreachable for non-ESP32 boards, so board2 never closes its targets or its UART port through the normal path. The bus reuse check at lines 205-209 also duplicates the one at lines 182-187.
Remove the redundant guards, drop the return at line 197, and place the destructive timeout case after the CLOSE handshake, or delete the duplicated block at lines 199-210 and document that board2 relies on the rig reset.
Also applies to: 161-210
🤖 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-async-controller-shared.toit` at line 111, Remove the
redundant architecture guards in test-board1, eliminate the return that exits
before cleanup, and ensure the normal CLOSE handshake, bus reuse check, and
port.close execute before any destructive timeout path. Update test-board2 so
its CLOSE branch remains reachable and closes targets and the UART normally;
remove the duplicated bus reuse logic while preserving the intended cleanup
behavior.
06a1eb7 to
2911d39
Compare
2911d39 to
fe4dcd0
Compare
b576370 to
70dee26
Compare
70dee26 to
f7763a2
Compare
1 participant
Summary
I2C_NACK/I2C_TIMEOUTerrors.ESP_ERR_NO_MEMthrough the existing retryable OOM path after cleaning all partial state; callbacks allocate nothing and cannot report OOM.CONFIG_I2C_ISR_IRAM_SAFEis enabled, allowing the original ESP32 image to fit.Dependencies
Testing
toit analyzefor both two-device hardware test programs.git diff --checkin both repositories.Stack created with GitHub Stacks CLI • Give Feedback 💬