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

I²C NACK on write/address byte does not error using i2c_master (IDFGH-12073) #13134

Closed
MatthiasKunnen opened this issue Feb 7, 2024 · 4 comments
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@MatthiasKunnen
Copy link

MatthiasKunnen commented Feb 7, 2024

keywords: I²C NACK no error, i2c_master, i2c master, new driver

IDF version: 5454d37 (current master)

Problem/Current behavior

When using the new I²C driver (i2c_master) functions, a NACK is perceived as an ACK on both slave
addresses and write bytes.

When the address byte results in a NACK (no device with this address is on the bus), the driver
continues as if an ACK was received.
When a write byte results in a NACK, the driver continues as if an ACK was received.

This affects the following functions:

  • i2c_master_receive
    • slave address ACK not verified
  • i2c_master_transmit
    • slave address ACK not verified
    • write byte ACK not verified
  • i2c_master_transmit_receive
    • slave address ACK not verified
    • write byte ACK not verified

A non-exhaustive list of things that could go wrong because of this:

  • i2c_master_transmit on a missing slave appears to succeed.
  • i2c_master_transmit on an existing but busy slave appears to succeed.
  • i2c_master_receive and i2c_master_transmit_receive on a missing slave results in fake FF bytes which is not distinguishable from an existing slave and valid FF bytes.

This is a regression from the old driver which verifies both the slave address ACK and the write byte's ACK, e.g.:

I²C Read Byte example using i2c_master_transmit_receive resulting in ESP_OK and FF output.
i2C-nack-on-write-due-to-missing-slave

Code that generated this signal
i2c_device_config_t dev_cfg = {
    .dev_addr_length = I2C_ADDR_BIT_LEN_7,
    .device_address = 0x2F,
    .scl_speed_hz = 50000,
};

i2c_master_dev_handle_t dev_handle;
ESP_ERROR_CHECK(i2c_master_bus_add_device(bus_handle, &dev_cfg, &dev_handle));

uint8_t readRegister = 0x20;
uint8_t receive[1]; // Will be 0xFF

 // Result will be 0, should be non-zero, e.g. 259
esp_err_t result = i2c_master_transmit_receive(dev_handle, &readRegister, 1, receive, 1, -1);

Expected behavior

To be compliant with I²C and/or SMBus, the controller upon detection of the NACK condition must generate a STOP condition to abort the transfer.

Solution

  • Enable ACK verification on every write byte.
  • Enable ACK verification on the address byte.

I will be making a PR that attempts to address both.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Feb 7, 2024
@github-actions github-actions bot changed the title I²C NACK on write/address byte does not error using i2c_master I²C NACK on write/address byte does not error using i2c_master (IDFGH-12073) Feb 7, 2024
@mythbuster5
Copy link
Collaborator

#13136 (comment)

@jrahlf
Copy link

jrahlf commented Feb 8, 2024

Can you clarify which IDF versions "new I2C driver" refers to?

@KaeLL
Copy link
Contributor

KaeLL commented Feb 8, 2024

v5.2 onwards.

KJ7LNW added a commit to KJ7LNW/esp-idf that referenced this issue Mar 15, 2024
As pointed out in PR espressif#13134 by @MatthiasKunnen, there is a deadlock in
`s_i2c_synchronous_transaction()` if `s_i2c_transaction_start()` should fail
because, on error, s_i2c_synchronous_transaction() returns before releasing the
lock.

This commit fixes the deadlock without any other changes.

Closes: espressif#13387

Signed-off-by: Eric Wheeler <[email protected]>
espressif-bot pushed a commit that referenced this issue Mar 16, 2024
As pointed out in PR #13134 by @MatthiasKunnen, there is a deadlock in
`s_i2c_synchronous_transaction()` if `s_i2c_transaction_start()` should fail
because, on error, s_i2c_synchronous_transaction() returns before releasing the
lock.

This commit fixes the deadlock without any other changes.

Closes: #13387

Signed-off-by: Eric Wheeler <[email protected]>
nebkat pushed a commit to nebkat/esp-idf that referenced this issue Apr 9, 2024
As pointed out in PR espressif#13134 by @MatthiasKunnen, there is a deadlock in
`s_i2c_synchronous_transaction()` if `s_i2c_transaction_start()` should fail
because, on error, s_i2c_synchronous_transaction() returns before releasing the
lock.

This commit fixes the deadlock without any other changes.

Closes: espressif#13387

Signed-off-by: Eric Wheeler <[email protected]>
espressif-bot pushed a commit that referenced this issue Apr 9, 2024
As pointed out in PR #13134 by @MatthiasKunnen, there is a deadlock in
`s_i2c_synchronous_transaction()` if `s_i2c_transaction_start()` should fail
because, on error, s_i2c_synchronous_transaction() returns before releasing the
lock.

This commit fixes the deadlock without any other changes.

Closes: #13387

Signed-off-by: Eric Wheeler <[email protected]>
@mythbuster5
Copy link
Collaborator

Solved

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new labels Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants