I2C: share a port the application already opened instead of taking it over - #269
Merged
Merged
Conversation
… over init() assumed the requested HP I2C port was free. When the application had already opened it - through the ESP-IDF i2c_master driver or Arduino Wire - the reset in init() wiped that driver's configuration, the failing i2c_new_master_bus() went unnoticed (and on ESP-IDF 5.5 that failure path unroutes the pins of the existing bus), and release() later tore the driver's bus down. The first transfer after switching sides then failed on every chip. A port that is already open is now recognised beforehand - i2cIsInit() under Arduino, i2c_master_get_bus_handle() on ESP-IDF 5.3.2 and later - and shared: nothing is reset, acquired or re-routed, each transaction applies the library's configuration and endTransaction() restores the driver's register block, with the completion flags cleared first so the driver's own interrupt does not see a stale one. release() on such a port only drops the flag. A port held by a slave driver is refused, and other i2c_new_master_bus() failures now fail init(). Not covered: serialization between the two sides (the driver's lock is not exposed, the application must not run them concurrently), differing pins, the asynchronous ESP-IDF API, low power ports, and a non-default I2C clock source on ESP32-P4 / ESP32-C61.
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
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
lgfx::i2c::init()assumed the requested HP I2C port was free. When the application had already opened it — through the ESP-IDFi2c_masterdriver (IDF >= 5.3) or ArduinoWire—init()reset the controller under that driver, andrelease()later tore the driver's bus (or its pins) down. Reported in #186 asEx_I2Cfailing on ESP32-S3 with IDF 5.5 next to an IDF-driver based sensor library.With this change a port that is already open is recognised beforehand and shared instead of taken over:
i2c_master_get_bus_handle()says whether the port is open (its "not initialized" error log is muted for that one call, since a free port is the normal case). A failedi2c_new_master_bus()is no longer used as the probe — on IDF 5.5 that failure path unroutes the pins of the existing bus, revokes its GPIO reservations and unbalances the RC_FAST clock reference.i2cIsInit()tells whether the sketch has begunWireon that port.endTransaction()restores the driver's register block, so transfers from both sides can alternate.release()only drops the flag.endTransaction()now clears the completion flags before restoring the driver's registers: the driver enables its event interrupts without clearing them first, so a stale flag fired immediately at its next transfer (a real race on dual-core chips when the bus was created on the other core).i2c_new_master_bus()failures now makeinit()fail instead of reporting success with no bus; a port held by a slave driver is refused.Limits, stated in the code: the two sides are not serialized against each other (the driver's lock is not exposed, so the application must not run them concurrently), both must use the same pins, and only the synchronous ESP-IDF API is compatible (its interrupts are off between transfers). A low power port is not shared. On ESP32-P4 / ESP32-C61 the driver must use the default I2C clock source (the library derives its timing from it). Before ESP-IDF 5.3.2 there is no non-destructive way to tell whether a port is open, so behaviour there is unchanged.
Verification
Grove port with an SSD1306 (0x3C) and a Sensirion SGP30 (0x58); each scenario repeated 8-9 times, all transfers checked against the sensor's serial number:
Ex_I2Cfirst / IDF bus first / alternate / hand-over viarelease(): all passWire: all passBefore the change the first transfer after switching sides failed on every chip (the reported
write=FAIL read=FAIL), andrelease()killed the application's bus. Builds also checked on ESP32-C5 (LP port present, IDF and Arduino).Stress on ESP32 (dual core): IDF bus created on core 0 with
enable_internal_pullup = false, a task on core 1 alternating IDF transfer /Ex_I2Ctransfer /Ex_I2Cprobe of an absent address (NACK, forced STOP path) / IDF transfer, 300 rounds: 0 failures, pull-up configuration untouched, IDF bus still working afterEx_I2C.release(). Without the completion-flag clearing the IDF transfer after every NACK path fails (300/300), which is what that part of the change fixes.The one expected failure remains: after the sketch calls
Wire.end(),Ex_I2Con that port stops working too, because in an Arduino build it is the sameWirebus.