Poll the port the transfer is on in Bus_I2C::wait() and busy() - #236
Merged
Conversation
Both functions picked the register block with #if I2C_NUM_MAX > 1, but I2C_NUM_MAX is an enum constant, not a macro: the preprocessor evaluates the condition as 0 > 1, the I2C1 branch never compiled, and every port was polled as I2C0 (verified on ESP-IDF 4.4 and 5.x). On a second hardware port, and on the low power ports now that they are reachable, the functions report an idle bus while a transfer is still on the wire. Resolve the device through the i2c implementation instead, which already knows every port kind, as new i2c::wait() / i2c::busy() entry points; a software port reports not busy there, since its transfers are synchronous. The bus busy bit moves into a getBusBusy() accessor next to the other per-target register accessors, replacing both the duplicated target lists in beginTransaction() and the member detection templates that Bus_I2C.cpp carried for the same purpose.
There was a problem hiding this comment.
Pull request overview
This PR fixes ESP32 I2C bus synchronization logic by ensuring Bus_I2C::wait() / busy() poll the correct underlying controller (including LP I2C ports), instead of implicitly (and incorrectly) reading I2C0 status.
Changes:
- Add
lgfx::i2c::busy(int)/lgfx::i2c::wait(int)APIs and routeBus_I2Cthrough them. - Centralize the per-target “bus busy” register read behind
getBusBusy()to avoid duplicated target-condition logic. - Remove the old
Bus_I2C.cppdevice-selection logic and status-register member-detection templates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/lgfx/v1/platforms/esp32/common.hpp | Declares new lgfx::i2c::busy() / wait() entry points. |
| src/lgfx/v1/platforms/esp32/common.cpp | Implements getBusBusy() plus new i2c::busy() / wait() using getDev() for correct port resolution. |
| src/lgfx/v1/platforms/esp32/Bus_I2C.cpp | Replaces hard-coded controller polling with calls to lgfx::i2c::busy() / wait(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Follow-up to a review note on #235: #235 (comment) pointed out that
Bus_I2C::wait()/busy()assume any non-negative port maps to I2C0/I2C1, which goes wrong for an LP port.Verifying the note turned up something broader:
#if I2C_NUM_MAX > 1never held in the first place.I2C_NUM_MAXis an enum constant, invisible to the preprocessor, so the condition evaluates as0 > 1and the I2C1 branch never compiled (confirmed empirically on ESP-IDF 4.4 and 5.x builds with a#pragma messageprobe). Both functions have therefore always polled I2C0 whatever the port: on a second hardware port — and now on an LP port — they report an idle bus while a transfer is still on the wire. Data transfers themselves were unaffected, since the i2c implementation resolves the device correctly; only the wait/busy synchronization was looking at the wrong port.The fix routes both functions through new
i2c::wait()/i2c::busy()entry points, so the port-to-device resolution lives in one place (getDev(), which already knows the second hardware port and the LP port). A software (negative) port reports not busy there, since its transfers are synchronous. The bus-busy bit read moves into agetBusBusy()accessor beside the other per-target register accessors, replacing the duplicated target lists inbeginTransaction()and the member-detection templates Bus_I2C.cpp carried for the same purpose.Build-verified on ESP32 (ESP-IDF 4.4, two hardware ports), ESP32-C5 (one hardware port + LP) and ESP32-P4 (two hardware ports + LP).