Fix I2C error-stop handling: 14ms stall per failed read, noisy NACK log, LP port reset - #240
Merged
Merged
Conversation
The HP helper i2c_ll_reset_register() indexes the PCR I2C register array with the port number, and a low power port number is out of range for that array (it holds only the high power ports), so a forced stop on the low power port was resetting an unrelated register. Route low power ports to lp_i2c_ll_reset_register() instead.
i2c_wait() chose between the normal STOP command and the forced stop by reading int_raw, which was never initialized when the ack-wait stage had been skipped, so the choice was undefined after a failed read. The normal STOP path waits for a completion interrupt that never fires on a peripheral that has already been force-stopped by the error handling, costing the full 14ms timeout on every failed transaction. Initialize int_raw, treat an errored context as needing the forced stop, and skip the redundant hardware stop when the error handler has already issued one.
A read from an absent address ended in the "read timeout" warning, although the NACK is the expected outcome when probing for devices, and printing the warning costs several milliseconds per probe on a 115200bps console, slowing bus scans considerably. Report it at verbose level as a NACK, but only when neither the timeout flag nor the arbitration-lost flag accompanies it.
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.
Fixes three problems in the ESP32 I2C error handling, found while running an I2C bus scanner that probes absent addresses with 1-byte reads.
Symptoms
[W][common.cpp] readBytes(): [LGFX] i2c read error : read timeoutfor every probed address, which itself accounts for several ms per probe at 115200 bps.Fixes (one commit each)
Reset a low power I2C port through its own reset register
i2c_ll_reset_register(port)indexes the PCR I2C array with the port number, but the array only holds the HP ports (PCR.i2c[1]on C5), so an LP port number writes out of bounds. LP ports now uselp_i2c_ll_reset_register().Keep an errored I2C transaction on the forced stop path
In
i2c_wait(),int_rawwas read uninitialized whenever the ack-wait stage had been skipped (e.g.endTransaction()after a failed read), so the STOP-path choice was undefined. When the normal STOP command path was taken on a peripheral that had already been force-stopped by the error handling, the completion interrupt never fired and the loop waited out the full 14 ms bound.int_rawis now initialized, an errored context always takes the forced stop, and the redundant second hardware stop is skipped (the error handler has already issued one).Report a pure address NACK on read as NACK instead of timeout
An address NACK is the expected outcome when probing for devices; it is now logged at verbose level as
nack. The timeout warning remains for genuine timeouts, and the NACK demotion only applies when neither the timeout flag nor the arbitration-lost flag is set.With these, a failed 1-byte read probe drops from ~20 ms to sub-millisecond and the log stays quiet.
Testing