Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32 I2C HAL does not handle NACKs properly in single-byte mode #61

Closed
multiplemonomials opened this issue Sep 27, 2022 · 4 comments
Closed
Labels
Bug Dis is broken STMicro Only affects STMicro targets

Comments

@multiplemonomials
Copy link
Collaborator

multiplemonomials commented Sep 27, 2022

STMicro made some... interesting choices in their implementation of the single-byte I2C API. These choices cause it to not comply with the expected behavior for the API.

How their code works, in general, is:

  1. User calls i2c_byte_write() with byte N
  2. Byte N is clocked in to the I2C output register (TXDR). The hardware can then begin outputting it.
  3. Return 1 from i2c_byte_write() (no real way to detect an error for byte N, because we didn't wait for it to be written out)
  4. User calls i2c_byte_write() with byte N+1
  5. We wait for the TCR flag, indicating that the previous byte has been written out
    • If the previous byte was NACKed, we never get this flag, so we go to the timeout case and return 2
    • Also: potential race condition here, since we don't wait one full byte time like we need to
  6. Once the TCR flag sets, indicating byte N has been sent, we write out byte N+1 to TXDR

The huge issue here is, if we get a NACK on byte N, this shows up as a timeout error, not a NACK, and it shows up for byte N+1, not byte N. The Mbed devs were apparently aware of this issue, and considered it not serious enough to fix (see ARMmbed#9447). However, this is a very serious issue, because it means that single-byte I2C does not behave in the expected way on STMicro devices.

Consider the following code, where someone tries to write an I2C scanner:

addressToUse = 0;
for(uint8_t address : possibleI2CAddresses)
{
    i2c.start();
    if(i2c.write(address << 1) == 1)
    {
        // device found
        i2c.stop();
        addressToUse = address;
        break;
    }
    i2c.stop();
}

This would work as expected on most devices, but not STM32s! There is no way for the current API to return a NACK on the address byte, so it would look like any address you tried to access was present on the bus. Clearly, this is not OK behavior and needs to be fixed.

@multiplemonomials multiplemonomials added Bug Dis is broken STMicro Only affects STMicro targets labels Sep 27, 2022
@JohnK1987
Copy link
Member

Is this Issue for both I2C IP versions?

@multiplemonomials
Copy link
Collaborator Author

No, this only affects newer devices with IP v2. I caught this while testing with that test shield PCB I made a while back, I'm working on a patch over the next few days

@JohnK1987
Copy link
Member

JohnK1987 commented Sep 27, 2022

Ok, affected are all families exclude L1, F1, F2 and F4.
For IP v2 there were reported also another issue about the multi-byte write metod - ARMmbed#15324

@JohnK1987
Copy link
Member

Solved in PR #78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Dis is broken STMicro Only affects STMicro targets
Projects
None yet
Development

No branches or pull requests

2 participants