Skip to content

esp32: remove incorrect !readLast check - #5585

Open
pottekkat wants to merge 3 commits into
tinygo-org:devfrom
pottekkat:pottekkat/fix-readLast
Open

esp32: remove incorrect !readLast check#5585
pottekkat wants to merge 3 commits into
tinygo-org:devfrom
pottekkat:pottekkat/fix-readLast

Conversation

@pottekkat

@pottekkat pottekkat commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #5584

As described in the issue, the && !readLast check discarded the address NACK on reads of 32 bytes or fewer. Only i2cCMD_WRITE sets ack_check_en, so the address WRITE is the only command in a read that can set ACK_ERR, so this check is unnecessary.

readLast is also removed as it is not used anywhere else other than this check.

Tested on an ESP32-D0WD-V3:

  • reads from a device that is present still succeed
  • reads from an absent address now return errI2CAckExpected
  • values read back are unchanged

You can use the same script in the issue to run the test with any I2C device(s).

@pottekkat pottekkat changed the title fix: remove incorrect !readLast check fix: remove incorrect !readLast check Aug 13, 2026
@pottekkat
pottekkat force-pushed the pottekkat/fix-readLast branch from 3bfd7bd to f276bec Compare August 13, 2026 11:56
@pottekkat pottekkat changed the title fix: remove incorrect !readLast check esp32: remove incorrect !readLast check Aug 13, 2026
@dgryski

dgryski commented Aug 14, 2026

Copy link
Copy Markdown
Member

@deadprogram

@deadprogram

Copy link
Copy Markdown
Member

Thanks for the fix @pottekkat

The same bug is still in machine_esp32xx_i2c.go for the other esp32 family devices. Perhaps you can fix it there as well?

Also machine_esp32_i2c.go:343 returns without calling resetBus(). That path was effectively unreachable for reads before this PR, so it's newly exercised. Worth a hardware check to read an absent address, and then immediately read a present one.

@pottekkat

pottekkat commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

@deadprogram I added a call to resetBus() but it seems unnecessary. I checked the BUS_BUSY bit after a NACK, and it was 0. I had tested absent/present (as in the code shown in issue thread) before making this PR and did not see any issues and just confirmed it now again.

We can keep it for consistency or we don't need to add it. I think the latter is better and that would also be why it wasn't there to begin with. wdyt?

Signed-off-by: Navendu Pottekkat <navendu@apache.org>
Signed-off-by: Navendu Pottekkat <navendu@apache.org>
Signed-off-by: Navendu Pottekkat <navendu@apache.org>
@pottekkat
pottekkat force-pushed the pottekkat/fix-readLast branch from ef9fc68 to dc9e27d Compare August 16, 2026 17:13
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.Tx discards address NACK on reads

3 participants