Conversation
The Rasperry Pi Pico, based on the RP2040, has a register that maintains the status of I2C interrupt flags called IC_INTR_STAT. The bits of this register are set by hardware and cleared by software. Before this commit, the I2CTarget library did not clear the restart bit (R_RESTART_DET) in this register after an I2C transaction ended, causing the is_restart field of the i2ctarget_i2c_target_request_obj_t struct to always be true after the first I2C transaction. This commit causes the restart and stop bits to get cleared when the I2C transaction ends. Signed-off-by: Amaar Ebrahim <amaaraebrahim@gmail.com>
…arate issue with read_byte returning early.
tannewt
left a comment
tannewt
left a comment
There was a problem hiding this comment.
Thanks for picking this up! One question before we merge.
| // Wait for data to arrive or a STOP condition indicating the transfer is over. | ||
| // Without this wait, the CPU can drain the FIFO faster than bytes arrive on the | ||
| // I2C bus, causing transfers to be split into multiple partial reads. | ||
| for (int t = 0; t < 100; t++) { |
There was a problem hiding this comment.
Do we want to formalize the timeout here? Maybe based on the baudrate?
There was a problem hiding this comment.
This is my first experience with i2ctarget side, so I'm not sure of the details. But talking it over with claude a bit and seems like the target side code does not have a reliable way to know the baudrate. The raspberrypi port implementation makes reference to setting a value here: https://github.com/FoamyGuy/circuitpython/blob/e376e2416b84d7b06f3f74843682d1d74d13efd4/ports/raspberrypi/common-hal/i2ctarget/I2CTarget.c#L52-L53 but the comment indicates it's not used.
Claude had this to say about the idea of basing it on bus speed:
the current fix is already effectively speed-independent — the loop exits early via the R_STOP_DET check as soon as the master finishes the transfer. The 1ms ceiling (100 × 10µs) is just a safety timeout, not a tuned wait. At the slowest standard I2C speed (100kHz), one byte is 9 clocks = 90µs, so 4 bytes would be ~360µs, well within the 1ms budget. The loop won't actually spin for 1ms in normal operation; it'll break out as soon as either data arrives or STOP is detected.
The values that it chose came from the atmel-samd implementation I think: https://github.com/FoamyGuy/circuitpython/blob/e376e2416b84d7b06f3f74843682d1d74d13efd4/ports/atmel-samd/common-hal/i2ctarget/I2CTarget.c#L148-L150
3 participants
This supercedes #10474. It contains the original commit from 10474 and a new commit that resolves the changes that were requested on that PR.
This also resolves #10423. While looking into this and testing the refactored fix I found this issue and the reproducer code in it and took a quick shot at resolving it with help from claude.
The root cause of 10423 is read_byte was checking the RX FIFO status (RFNE) once and returning 0 immediately if empty. At I2C bus speeds (even 400kHz), the CPU runs much faster than bytes arrive on the wire. So after reading the first 1-2 bytes, the CPU would check the FIFO, find it momentarily empty (the next byte hasn't clocked in yet), return 0, and the shared-bindings read() loop would break — splitting a single 4-byte transfer into two 2-byte reads.
I tested with the reproducer code in the issue and with this fix the pico side is now able to successfully read messages larger than 1 or two bytes. The 4 from the original code work fine and I tested 16 bytes and found it working as well.