feat: support <driver/i2c_master.h> in i2cdev #655
Conversation
There was a problem hiding this comment.
Hello,
Heads up: I am not from esp-idf-lib (and also not a senior developer). This new version looks great and would be very useful. My comments are focused on the integration of the i2c_master_bus into the existing i2c_dev_t structure. (IIRC) This updated I2C driver from espressif allows for multiple components to share the bus, without the need for an extra layer orchestrating the access between threads. The proposed code in this commit takes away the opportunity to use the new features' of the updated I2C driver, potentially forcing developers to use a second I2C bus (again) if they prefer integration between components without needing to dive into the code of esp-idf-lib's i2cdev component.
tl;dr: I don't think taking ownership of the bus (again) is a good idea given the queuing approach from espressif.
But, I could be completely wrong. You probably have researched this a little more than I have, so I am very curious about your thoughts.
| SEMAPHORE_TAKE(dev->port); | ||
|
|
||
| esp_err_t res = i2c_setup_port(dev); | ||
| if (res == ESP_OK) | ||
| { | ||
|
|
||
| if (out_data && out_size) | ||
| { | ||
| res = i2c_master_transmit_receive(dev->device, (void *)out_data, out_size, in_data, in_size, CONFIG_I2CDEV_TIMEOUT); | ||
| } | ||
| else | ||
| { | ||
| res = i2c_master_receive(dev->device, (void *)out_data, out_size, CONFIG_I2CDEV_TIMEOUT); | ||
| } | ||
| if (res != ESP_OK) | ||
| { | ||
| ESP_LOGE(TAG, "Could not read from device [0x%02x at %d]: %d (%s)", dev->addr, dev->port, res, esp_err_to_name(res)); | ||
| } | ||
| } | ||
|
|
||
| SEMAPHORE_GIVE(dev->port); |
There was a problem hiding this comment.
Why would we lock the bus when using the updated driver? It seems that this feature is already present in the bus structure (link)
EDIT: From the docs
The factory function [i2c_new_master_bus()](https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/i2c.html#_CPPv418i2c_new_master_busPK23i2c_master_bus_config_tP23i2c_master_bus_handle_t) and [i2c_new_slave_device()](https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/i2c.html#_CPPv420i2c_new_slave_devicePK18i2c_slave_config_tP22i2c_slave_dev_handle_t) are guaranteed to be thread safe by the driver, which means, user can call them from different RTOS tasks without protection by extra locks. Other public I2C APIs are not thread safe. which means the user should avoid calling them from multiple tasks, if user strongly needs to call them in multiple tasks, please add extra lock.
Nevermind, it is needed.
| dev.master_bus_config.sda_io_num = CONFIG_EXAMPLE_I2C_MASTER_SDA; | ||
| dev.master_bus_config.scl_io_num = CONFIG_EXAMPLE_I2C_MASTER_SCL; | ||
| dev.master_bus_config.i2c_port = -1; | ||
| dev.master_bus_config.clk_source = I2C_CLK_SRC_DEFAULT; | ||
| dev.master_bus_config.intr_priority = 0; | ||
| dev.master_bus_config.glitch_ignore_cnt = 7; | ||
| dev.master_bus_config.trans_queue_depth = 0; | ||
| dev.master_bus_config.flags.enable_internal_pullup = false; |
There was a problem hiding this comment.
Using the i2c_dev component in this way locks the user/programmer into using this component for all I2C actions. I think this doesn't fit with the features' of the updated I2C driver (queuing). Wouldn't it be much more flexible for this component to store a pointer instead of possessing the bus (again)?
There was a problem hiding this comment.
It would be. What I came up is: create an abstraction header, "i2c_bus.h" for writing less code in most of simple cases, and pass a pointer (and whatever it needs to function) to "i2c_master.h". Users are free to choose either "i2c_bus.h" or their own.
|
Also, a lot of components (using i2cdev) are still using an initializer function using the following (which are broken when not defining cfg in the struct (link to what I mean). esp_err_t xxx_init_desc(aht_t *dev, uint8_t addr, i2c_port_t port, gpio_num_t sda_gpio, gpio_num_t scl_gpio)
{
CHECK_ARG(dev);
...
// Using legacy
dev->i2c_dev.addr = addr;
dev->i2c_dev.cfg.sda_io_num = sda_gpio;
dev->i2c_dev.cfg.scl_io_num = scl_gpio;
#if HELPER_TARGET_IS_ESP32
dev->i2c_dev.cfg.master.clk_speed = I2C_FREQ_HZ; |
| #if defined(CONFIG_I2CDEV_USING_LEGACY_I2C) | ||
| i2c_config_t cfg; //!< I2C driver configuration (i2c.h only) | ||
| uint32_t timeout_ticks; /*!< HW I2C bus timeout (stretch time), in ticks. 80MHz APB clock | ||
| ticks for ESP-IDF, CPU ticks for ESP8266. | ||
| When this value is 0, I2CDEV_MAX_STRETCH_TIME will be used */ | ||
| When this value is 0, I2CDEV_MAX_STRETCH_TIME will be used | ||
| (i2c.h only) */ | ||
| #else |
There was a problem hiding this comment.
There are so many files using the initializer code, that it might be easier to keep these parts of the structure and ignore them when using the updated driver.
| if (!dev) return ESP_ERR_INVALID_ARG; | ||
|
|
||
| SEMAPHORE_TAKE(dev->port); | ||
|
|
||
| esp_err_t res = i2c_setup_port(dev); | ||
| if (res == ESP_OK) | ||
| { | ||
| i2c_cmd_handle_t cmd = i2c_cmd_link_create(); | ||
| i2c_master_start(cmd); | ||
| i2c_master_write_byte(cmd, dev->addr << 1 | (operation_type == I2C_DEV_READ ? 1 : 0), true); | ||
| i2c_master_stop(cmd); | ||
|
|
||
| res = i2c_master_cmd_begin(dev->port, cmd, pdMS_TO_TICKS(CONFIG_I2CDEV_TIMEOUT)); | ||
|
|
||
| i2c_cmd_link_delete(cmd); | ||
| } | ||
|
|
||
| SEMAPHORE_GIVE(dev->port); | ||
|
|
||
| return res; | ||
| return _i2c_dev_probe(dev, operation_type); |
There was a problem hiding this comment.
Thinking about the restructuring as a whole, where you currently split the new and legacy functionality into separate files, it might result in repetitions of code. It depends what the author thinks of this, but using existing function implementations (using preprocessor blocks) will result in the implementations for both the legacy version and updated version in the same place (same functions too).
| goto fail; | ||
| } | ||
| fail: | ||
| free(write_buffer); |
There was a problem hiding this comment.
Are you only freeing on fail? Doesn't this leak memory when the transaction succeeds?
There was a problem hiding this comment.
IIRC, there is no other path to return after malloc() even if succeeded or failed.
| SEMAPHORE_TAKE(dev->port); | ||
|
|
||
| write_buffer_size = with_register_address ? out_reg_size + out_size : out_size; | ||
| write_buffer = malloc(write_buffer_size); |
There was a problem hiding this comment.
I am guessing you're using a heap allocated buffer to prevent a task's (limited) stack from being used instead. Is it not possible to use the stack?
There was a problem hiding this comment.
As the part is obtained from the old code, I cannot comment on this.
There was a problem hiding this comment.
Hmmm, then it would depend on the stack size of the task, set up by the user. Most of the tasks have it set to configMINIMAL_STACK_SIZE * 8 or 3, not sure if that would be enough for a temp buffer.
| only) */ | ||
| #endif | ||
| uint8_t addr; //!< Unshifted address | ||
| SemaphoreHandle_t mutex; //!< Device mutex |
There was a problem hiding this comment.
Why not use use the mutex from i2c_port_state_t?
There was a problem hiding this comment.
As the part is obtained from the old code, I cannot comment on this.
There was a problem hiding this comment.
Oh alright, it may be that they use the same logic for controlling the mutex. We'll have to ask the author
| SEMAPHORE_TAKE(dev->port); | ||
|
|
||
| esp_err_t res = i2c_setup_port(dev); | ||
| if (res == ESP_OK) | ||
| { | ||
| res = i2c_master_probe(dev->bus, dev->device_config.device_address, CONFIG_I2CDEV_TIMEOUT); | ||
| } | ||
|
|
||
| SEMAPHORE_GIVE(dev->port); |
There was a problem hiding this comment.
This is what I meant with the repetition, wouldn't the code be much clearly arranged to only interchange the calls to the old and new i2c drivers (using preprocessor statements)?
There was a problem hiding this comment.
I am inclined to avoid macro because of readability when the change is not trivial. The reason of separating old and new is that, you can modify both as long as the interface (and other expected behaviors) are kept. Others might not agree, though.
There was a problem hiding this comment.
That does make sense, but it may introduce two versions of the "same" logic. My opinion is that it may be less readable, but safer to make changes to (because you won't have to make a change twice, removing the possibility of making a mistake)
|
A much more radical idea: What about not implementing the new i2c_master into i2cdev? Instead, device structures might get away with storing For example, INA260 could look like this (untested code, just a proposition): /* ina260.h */
...
typedef struct
{
i2c_device_config_t i2c_dev_config;
i2c_master_dev_handle_t i2c_dev_handle;
uint16_t config; //!< Current config
uint16_t mfr_id; //!< Manufacturer ID
uint16_t die_id; //!< Die ID
} ina260_t;
esp_err_t ina260_init_desc(ina260_t *dev, uint8_t addr, i2c_master_bus_handle_t bus_handle);/* ina260.c */
esp_err_t ina260_init_desc(ina260_t *dev, uint8_t addr, i2c_master_bus_handle_t bus_handle)
{
CHECK_ARG(dev);
...
// Let the device seperate its own device, instead
// of relying on i2cdev
memset(&dev->i2c_dev_config, 0, sizeof(i2c_device_config_t));
dev->i2c_dev_config.dev_addr_length = I2C_ADDR_BIT_LEN_7;
dev->i2c_dev_config.device_address = addr;
dev->i2c_dev_config.scl_speed_hz = I2C_FREQ_HZ;
return i2c_master_bus_add_device(bus_handle, &dev->i2c_dev_config, &dev->i2c_dev_handle);
}
static esp_err_t read_reg_16(ina260_t *dev, uint8_t reg, uint16_t *val)
{
CHECK_ARG(val);
uint8_t write_buffer[] = { reg };
esp_err_t res = i2c_master_transmit_receive(dev->i2c_dev_handle, write_buffer, 1, (uint8_t *)val, 2, 1000 / portTICK_PERIOD_MS);
*val = (*val >> 8) | (*val << 8);
return res;
}
static esp_err_t write_reg_16(ina260_t *dev, uint8_t reg, uint16_t val)
{
uint16_t v = (val >> 8) | (val << 8);
uint8_t out_buf[4] = { reg, v >> 8, v & 0xff };
size_t out_index = 3;
return i2c_master_transmit(dev->i2c_dev_handle, out_buf, out_index, 1000 / portTICK_PERIOD_MS);
}/* main.c */
void read_power_task(void *pvParameters)
{
i2c_master_bus_handle_t bus_handle = (i2c_master_bus_handle_t)pvParameters;
ina260_t dev;
memset(&dev, 0, sizeof(ina260_t));
ESP_ERROR_CHECK(ina260_init_desc(&dev, INA260_ADDR, bus_handle));
// We no longer need the bus handle, our dev struct holds the i2c device handle
ESP_ERROR_CHECK(ina260_init(&dev));
...
// Use ina260 functions like before
}
void app_main(void)
{
i2c_master_bus_config_t i2c_mst_config = {
.clk_source = I2C_CLK_SRC_DEFAULT,
.glitch_ignore_cnt = 7,
.i2c_port = I2C_NUM,
.scl_io_num = CONFIG_SCL_GPIO,
.sda_io_num = CONFIG_SDA_GPIO,
.flags.enable_internal_pullup = true,
};
i2c_master_bus_handle_t bus_handle;
ESP_ERROR_CHECK(i2c_new_master_bus(&i2c_mst_config, &bus_handle));
xTaskCreate(read_power_task, "read_power_task", configMINIMAL_STACK_SIZE * 8, bus_handle, 5, NULL);
...
} |
Your comment is truly valid. The way I implemented it does not utilize the new feature you have mentioned. I was not able to think of a better way without loosing backward compatibility. A very hack. I will reply to your review comments, but here is a general idea: the primary focus is to keep the same interfaces. I don't understand every parts in the original code. The secondary focus is to start discussions, just like you did and which I appreciate. Keeping the i2cdev.h as is is another idea. What the original author said is "keeping backward compatibility". depending o the definition, it might have a gray zone. Only the original author can answer how. |
I agree. |
The author did post this about i2c changes: #626 (comment), and I am not sure what his approach will be. @UncleRus, what are your thoughts?
It might break some user's applications, but might be a better approach on the long run to let a device (on the I2C bus, INA260 for example) store it's i2c_dev config and handle. If the author wants to keep the current setup, the i2c_dev config and handle could also be stored in the i2c_dev_t object, which might be a meeting-in-the-middle solution (and would reduce the amount of code to be changed, both in this library as well as by users using this library). |
|
Just out of curiosity, I tried if the device can access the bus object: // ina260.h
...
#include "driver/i2c_types.h"
#include "driver/i2c_master.h"
...
/**
* Device descriptor
*/
typedef struct
{
i2c_device_config_t i2c_dev_config;
i2c_master_dev_handle_t i2c_dev_handle;
uint16_t config; //!< Current config
uint16_t mfr_id; //!< Manufacturer ID
uint16_t die_id; //!< Die ID
} ina260_t;
// ina260.c
// Using initialized ina26_t dev
ESP_LOGD(TAG, "[0x%02x@%u] Resetting...", dev->i2c_dev_config.device_address, dev->i2c_dev_handle->master_bus->base.port_num);This threw an error during building: Not sure why this happens. |
The biggest issue is ESP8266 support. As ESP8266 RTOS SDK has been in maintenance mode (minimal fixes only), it will not get updated with new |
ESP-IDF doesn't support the ESP8266 right? It will be very difficult to have try to support (IMO) deprecated hardware |
|
No, it doesn't. However, ESP8266 RTOS SDK has |
Alright, well then it might be much harder to maintain this. I've left my comments, good luck! |
|
A maintainer of ESP8266 RTOS SDK ported |
This is a premature PoC, calling for feedback. I did not test it with a real hardware. There should be style violations and other run-time errors. Never ported any dependent I2C components. Successfully compiled
examples/i2cdev/defaultwith and withoutCONFIG_I2CDEV_USING_LEGACY_I2C. I just followed instructions for newi2c_master, kept the original logic as much as possible, and made it compatible with older implementation.Originally posted by @UncleRus in #612 (comment)
Questions: