Skip to content

esp32: fix I2C timeouts on larger transfers - #5674

Open
pottekkat wants to merge 1 commit into
tinygo-org:devfrom
pottekkat:fix-resuming-segment
Open

pottekkat wants to merge 1 commit into
tinygo-org:devfrom
pottekkat:fix-resuming-segment

Conversation

@pottekkat

Copy link
Copy Markdown
Contributor

Fixes #5673

The I2C controller has 16 command slots and 32 bytes of RAM. So it splits longer transfers into segments. A segment ends with i2cCMD_END, which suspends the transfer and holds the bus. And as mentioned in the TRM section 21.3.5:

Note: When there are more than three segments, the address of an END command in the cmd should not be altered into another command by the next segment.

transmit resets reg to COMD0 after every segment and does not clear the slots. The segment that finishes a read or a write does not end with END, so i2cCMD_STOP is programmed into the next slot instead.

For example:

segment 1         COMD0 RSTART   COMD1 WRITE   COMD2 END
middle segments   COMD0 WRITE    COMD1 END
final segment     COMD0 WRITE    COMD1 STOP

Up to 63 bytes, a write is two segments, so the final STOP at COMD1 overwrites a WRITE. From 64 bytes (three segments or more), the final STOP overwrites the END left by the previous segment in COMD1 (which the note above mentions not to do).

Reads fail at 32 bytes for another reason. A segment that reads 32 bytes NACKs the last with i2cCMD_READLAST and ends with i2cCMD_STOP, with no i2cCMD_END.

The fix is to ensure write segments always end with i2cCMD_END, so i2cCMD_STOP goes out alone at COMD0. Follows the same logic in ESP-IDF i2c_master.c#L202-L204.

Reads put the ACKed bytes and the final NACKed byte in separate segments. The READLAST segment now pulls one byte and ends with END. Also follows the same logic in ESP-IDF i2c_master.c#L395-L396.

An isRead flag replaces readTo because STOP is now in its own segment after readTo has been cleared.

Tested on an ESP32-D0WD-V3 with an SH1106 OLED.

@deadprogram

Copy link
Copy Markdown
Member

Thanks @pottekkat for this fix!

Some notes, lightly edited from an automated review.

  1. The write-to-read transition alters an END slot. In transmit, the read branch starts the repeated start at COMD0:
reg.Set(i2cCMD_RSTART)    // COMD0
reg = nextAddress(reg)
reg.Set(i2cCMD_WRITE | 1) // COMD1

The write segment before it left COMD0 = WRITE and COMD1 = END. So COMD1 changes from END into WRITE. For example, Tx(addr, w=64 bytes, r=4 bytes):

seg1  COMD0 RSTART    COMD1 WRITE|32  COMD2 END
seg2  COMD0 WRITE|32  COMD1 END
seg3  COMD0 WRITE|1   COMD1 END
seg4  COMD0 RSTART    COMD1 WRITE|1   COMD2 READ|3  COMD3 END   <-- COMD1 END becomes WRITE

This is segment 4, so the note in TRM section 21.3.5 applies. A write of 32 bytes or more, followed by a read, goes through this path. Did you test that specific case? If the hardware does object, you can clear the command slots at the start of each segment, or keep END at the same slot.

  1. All writes now use two segments. Each write ends with END and starts a transfer, then STOP starts a second transfer. A 1-byte write and CheckDevice change from one hardware start to two. This is the cost of the fix, and ESP-IDF does the same, but it is good to record it in the PR description.

  2. The comment // A resuming segment must not overwrite the slot with END. reads as if the code must not write END, but the code does write it. Also add the source, for example // TRM 21.3.5. Something like this is more clear:

// Always end a write segment with END, so STOP goes out alone. TRM 21.3.5
  1. isRead stays true through the final STOP segment. A timeout there now gives errI2CReadTimeout and not errI2CWriteTimeout. This is probably better, but it is a change in behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

esp32: I2C transactions time out on writes over 63 bytes and reads of 32 or over 63 bytes

2 participants