Refuse a low power I2C port number when the low power support is absent - #237
Merged
Conversation
The port arguments were bounds checked against I2C_NUM_MAX, which counts every port the ESP-IDF headers know of. On an ESP-IDF recent enough to number the low power port but older than the 5.4 that the low power support here requires, that bound accepts the low power port number while getDev() has no low power branch and folds it onto a high power port: the functions silently drive that peripheral instead, alongside its legitimate user and without its lock. Check against the ports this implementation can actually drive instead (the high power ports, plus the low power ones when compiled in), so an unsupported low power port number fails with invalid_arg. On the configurations where the support is present, and on the ESP-IDF versions that predate the low power port entirely, both bounds are equal and nothing changes.
There was a problem hiding this comment.
Pull request overview
This PR hardens ESP32-platform I2C port argument validation so that lgfx::i2c::* APIs reject low-power (LP) I2C port numbers when LP I2C support is not compiled in, preventing accidental access to the wrong high-power controller.
Changes:
- Introduces
LGFX_I2C_PORT_NUMas the effective “supported I2C port count” (HP ports + LP ports only when LP support is enabled). - Replaces
I2C_NUM_MAXbounds checks withLGFX_I2C_PORT_NUMacross thelgfx::i2c::*entry points, returninginvalid_arg(or no-op/false forwait()/busy()).
💡 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.
Hardening of the I2C port bounds, in the same problem family as #236.
The port arguments of the
lgfx::i2c::functions are bounds-checked againstI2C_NUM_MAX, which counts every port the ESP-IDF headers know of — including the low power port, from the first ESP-IDF that numbers it. The low power support here, however, only compiles on ESP-IDF 5.4 or later. In the gap (ESP-IDF 5.1–5.3, i.e. Arduino cores 3.0.x / 3.1.x, on the chips that carry a low power port such as the C6 and P4), a low power port number passes the bounds check whilegetDev()has no low power branch and folds it onto a high power controller (I2C0 on the C6, I2C1 on the P4). The functions then silently drive that peripheral — without its lock, alongside its legitimate user.This change checks the port arguments against the ports this implementation can actually drive (
LGFX_I2C_PORT_NUM= the high power ports, plus the low power ones when compiled in), so an unsupported low power port number now fails withinvalid_arginstead. On configurations where the low power support is present, and on ESP-IDF versions that predate the low power port entirely, the two bounds are equal and nothing changes. The negative (software) port numbers are unaffected: they are dispatched before any of these checks.Build-verified on ESP32 (ESP-IDF 4.4), ESP32-C5 and ESP32-P4 (ESP-IDF 5.x).