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

ESP-IDF v5.3 I2C Old driver working, new driver not. (IDFGH-13508) #14401

Closed
3 tasks done
western-hoolock opened this issue Aug 19, 2024 · 6 comments
Closed
3 tasks done
Assignees
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally

Comments

@western-hoolock
Copy link

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

Environment: PlatformIO with v5.3 and an ESP32-WROVER-E on a custom board and a Texas Instruments BQ27441 Fuel Gauge and external 4.7k SCL/SDA pull-up resistors.

I wrote some code that uses the deprecated I2C <i2c.h> driver, which works fine at 400kHz:

#define P_I2C_SDA 22
#define P_I2C_SCL 23
#define BQ27441_ADDRESS 0x55

i2c_config_t i2c_conf = 
{
    .mode             = I2C_MODE_MASTER,
    .sda_io_num       = P_I2C_SDA,
    .sda_pullup_en    = GPIO_PULLUP_DISABLE,
    .scl_io_num       = P_I2C_SCL,
    .scl_pullup_en    = GPIO_PULLUP_DISABLE,
    .master.clk_speed = 400000,
};

i2c_param_config(I2C_NUM_0, &i2c_conf);
i2c_driver_install(I2C_NUM_0, i2c_conf.mode, 0, 0, 0);

i2c_set_timeout(I2C_NUM_0, 400000); // ~5ms

i2c_master_write_to_device(I2C_NUM_0, BQ27441_ADDRESS, (uint8_t []){ 0x00, 0x01 }, 2, portMAX_DELAY);
i2c_master_write_to_device(I2C_NUM_0, BQ27441_ADDRESS, (uint8_t []){ 0x01, 0x00 }, 2, portMAX_DELAY);

i2c_master_write_to_device(I2C_NUM_0, BQ27441_ADDRESS, (uint8_t []){ 0x00 }, 1, portMAX_DELAY);
uint8_t val[2];
i2c_master_read_from_device(I2C_NUM_0, BQ27441_ADDRESS, val, sizeof(val), portMAX_DELAY);

printf("Device Type: %04X\r\n", (val[1] << 8) | val[0]);

Then I decided to rewrite this to use the new I2C driver in <i2c_master.h> like so:

#define P_I2C_SDA 22
#define P_I2C_SCL 23
#define BQ27441_ADDRESS 0x55

i2c_master_bus_config_t master_cfg = {
    .clk_source = I2C_CLK_SRC_DEFAULT,
    .i2c_port = I2C_NUM_0,
    .scl_io_num = P_I2C_SCL,
    .sda_io_num = P_I2C_SDA,
    .glitch_ignore_cnt = 7,
    .flags.enable_internal_pullup = 0,
};

i2c_master_bus_handle_t bus_handle;

i2c_new_master_bus(&master_cfg, &bus_handle);

i2c_device_config_t bq27441_cfg = {
    .dev_addr_length = I2C_ADDR_BIT_LEN_7,
    .device_address = BQ27441_ADDRESS,
    .scl_speed_hz = 400000,
    .scl_wait_us = 5 * 1000, // BQ27441 can stretch up to 4ms, use 5ms to be safe.
};

i2c_master_dev_handle_t device_handle;
i2c_master_bus_add_device(bus_handle, &bq27441_cfg, &device_handle);

// This call returns ESP_ERR_INVALID_STATE.
i2c_master_transmit(device_handle, (uint8_t []){ 0x00, 0x01 }, 2, -1);
esp_rom_delay_us(100); // Per BQ27441 datasheet.

i2c_master_transmit(device_handle, (uint8_t []){ 0x01, 0x00 }, 2, -1);
esp_rom_delay_us(100); // Per BQ27441 datasheet.

uint8_t val[2];

i2c_master_transmit(device_handle, (uint8_t []){ 0x00 }, 1, -1);
esp_rom_delay_us(100); // Per BQ27441 datasheet.
i2c_master_receive(device_handle, val, 1, -1);
i2c_master_receive(device_handle, val + 1, 1, -1);

printf("Device Type: %04X\r\n", (val[1] << 8) | val[0]);

This code however works only at 100kHz, not at 400kHz. At 400kHz the line:
i2c_master_transmit(device_handle, (uint8_t []){ 0x00, 0x01 }, 2, -1); fails with ESP_ERR_INVALID_STATE and the logs show:

E (510) i2c.master: I2C hardware timeout detected
E (520) i2c.master: s_i2c_synchronous_transaction(870): I2C transaction failed
E (550) i2c.master: i2c_master_transmit(1072): I2C transaction failed

Thoughts on why the new i2c driver doesn't work at 400kHz while the old one does?

@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 19, 2024
@github-actions github-actions bot changed the title ESP-IDF v5.3 I2C Old driver working, new driver not. ESP-IDF v5.3 I2C Old driver working, new driver not. (IDFGH-13508) Aug 19, 2024
@mythbuster5
Copy link
Collaborator

mythbuster5 commented Aug 20, 2024

May you check the scl/sda data with logic analyzer to see what exactly happens?And probably compared with old one. From the log and description, I guess there might happen with long stretch.

@AxelLin
Copy link
Contributor

AxelLin commented Aug 20, 2024

@western-hoolock Check if this helps: #14129 (comment)

@western-hoolock
Copy link
Author

May you check the scl/sda data with logic analyzer to see what exactly happens?And probably compared with old one. From the log and description, I guess there might happen with long stretch.

Unfortunately I don't have a logic analyzer or access to one.

The BQ27441 datasheet mentions that it can do clock stretching up to 4ms, in both the new and old driver examples I posted I set it to 5ms to be on the safe side:

Old driver:
i2c_set_timeout(I2C_NUM_0, 400000); // ~5ms

New driver:
.scl_wait_us = 5 * 1000, // BQ27441 can stretch up to 4ms, use 5ms to be safe.

One difference I noticed though is that the old driver takes APB clock cycles as units and the new driver takes the number of microseconds.

But yeah, I wish I had access to a logic analyzer so I could actually confirm that the new driver actually respects the allowed timeout for clock stretching.

@western-hoolock
Copy link
Author

@western-hoolock Check if this helps: #14129 (comment)

The issue seems similar at first but eventually it turns out the root cause of the issue was the ESP-IDF not supporting clock stretching above ~12ms while the I2C slave that person uses clearly states it can stretch up to 20ms. This is not the case in my situation, the BQ27441's datasheet states it can stretch up to 4ms, so to be safe I have set it to 5ms in both the old and new I2C driver examples.

@mythbuster5
Copy link
Collaborator

So... i understand, this is a bug in the driver.
图片

line 589 change the timeout value, and line 604 change it back. Please move line 589 behind line 604 and try again..

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Aug 22, 2024
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: In Progress Work is in progress labels Aug 29, 2024
@AxelLin
Copy link
Contributor

AxelLin commented Sep 4, 2024

@mythbuster5
Would you backport this fix to release branches? I hope v5.2.3 can include this fix.

espressif-bot pushed a commit that referenced this issue Sep 25, 2024
espressif-bot pushed a commit that referenced this issue Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

4 participants