i2c_master migration

[jmpmscorp]

Guidelines to port.

I have been porting i2c lib to i2c master trying to do minimal changes. I get working code (need more exhaustive test) with same write/read api. I have tested ads111x and ds3231 examples with a real device and they work.

However, there is a challenge to solve with i2c_dev_t structure. On every i2c device constructor, there is an init descriptor function like xxxx_init_desc(...). In .c code, descriptor is filled. With i2c drive legacy, i2c_dev_t has a member (i2c_config_t cfg) but in i2c_master that struct doesn't exist.

With i2c_master, i2c_dev_t only need i2c_master_dev_handle_t and optionally, i2c_master_bus_handle. The definition with new driver could be:

typedef struct
{
    uint8_t addr;    //!< Unshifted address
    i2c_port_t port; //!< I2C port number
    uint32_t timeout_ticks;  //!< Threshold value (stretch time)

#if USE_I2C_LEGACY_DRIVER
    i2c_config_t cfg;       //!< I2C driver configuration
#else  // USE I2C MASTER DRIVER
    i2c_master_dev_handle_t dev_handle; //!< I2C driver handle
    i2c_master_bus_handle_t bus_handle; //!< Optional I2C bus handle
#endif
    SemaphoreHandle_t mutex; //!< Device mutex

} i2c_dev_t;

This struct change implies that every i2c component must rewrite its initializer with the new structure, something like:

xxx_init_desc(...)
{
  #if USE_I2C_LEGACY
       initliaze i2c legacy structure
  #else
      initialize i2c master structure
  #endif
}

An alternative could be create new global "init/constructor" in i2cdev.h with a new global config structure. In this way, components only need to call common initializer and descriptor/device initialization will be done in i2c_dev_t component. Something like:

i2c_dev.h

typedef struct
{
    i2c_port_t port;
    uint16_t address;
    i2c_clock_source_t clk_src;
    uint8_t sda_gpio_num;
    uint8_t scl_gpio_num;
    uint32_t scl_speed_hz;
    bool enable_gpio_pullups;
    bool allow_pd;
    uint32_t timeout_ticks;
} i2c_dev_config_t;

esp_err_t i2c_dev_new(i2c_dev_t *dev, const i2c_dev_config_t *config);

i2c_legacy.c

esp_err_t i2c_dev_new(i2c_dev_t *dev, const i2c_dev_config_t *config)
{
    required work with i2c legacy driver
}

esp_err_t i2c_dev_new(i2c_dev_t *dev, const i2c_dev_config_t *config)
{
    required work with i2c master driver
}

xxxx_component.c

esp_err_t xxx_init_desc(....)
{
   // fill config structure
   
   return i2c_dev_new(...)
}

Obviously, every i2c component must rewrite its initializer to accomodate to this change.

I'm open to any suggestion.

Regards.

[hiperiondev]

Why not create a generic i2c HAL?
You would need only 4 functions: i2c_init, i2c_read, i2c_write, i2c_close.
All drivers should reference this abstraction and it should be the only point of modification.

[jmpmscorp]

Hi, @hiperiondev.

Main idea is to keep compatibility as much as possible with every i2c component already written. If we change to a new generic HAL, every component must be rewritten to accomodate to this new HAL.

I think we need to do minimal possible changes that not suppose big breaking change. It's only my opinion, :)

[hiperiondev]

That's true, but I don't think what happened this time is not an exception. It's very likely to happen again.
I understand that a generic HAL is more work today, but it will be much less work tomorrow.

[trombik]

What I would like to expect from the migration is:

• Keep backward compatibilities as much as possible
• Keep older implementations so that users can opt in them when breaking changes are inevitable (just like what espressif does with i2c.h and i2c_master.h.
• The older implementation eventually will fade out when esp-idf removes what we rely on, i2c.h in this case.

The suggested idea, creating a generic HAL, looks the Right Thing ™ to me. It might take longer to implement, but much time has passed already.

Hope someone leads the migration and others help the development.

[jmpmscorp]

Hi, again.

Basically, the lib is already aprox a "HAL". Mainly, there are four functions that do the work (i2c_dev_read,i2c_dev_write,i2c_dev_read_reg,i2c_dev_write), and last two call i2c_dev_read,i2c_dev_write with 'constant' parameters. So, we could take i2c_dev_read,i2c_dev_write as HAL functions.

The breaking change to resolve would be init/new close/del HAL functions. Actually, i2c_dev_init only allocate mutex to make thread safe I2C port. There isn't a trully "HAL" device structure initializer. Each i2c component is responsible of structure initialization.

I think the better way to resolve it is to define a global opaque device structure handler, a common init config structure and HAL init/new - close/delete functions that receive this config structure.

Something like:

i2cdev.h

...

typedef struct i2c_dev_t;

typedef struct {
   common config init parameters (gpios, port, address,...)
} i2c_dev_config_t;


esp_err_t i2c_dev_new(i2c_dev_t *dev, const i2c_dev_config_t *config);

esp_err_t i2c_dev_read(const i2c_dev_t *dev, const void *out_data, size_t out_size, void *in_data, size_t in_size);

esp_err_t i2c_dev_write(const i2c_dev_t *dev, const void *out_reg, size_t out_reg_size, const void *out_data, size_t out_size);

esp_err_t i2c_dev_read_reg(const i2c_dev_t *dev, uint8_t reg, void *in_data, size_t in_size);

esp_err_t i2c_dev_write_reg(const i2c_dev_t *dev, uint8_t reg, const void *out_data, size_t out_size);

...

i2cdev_legacy.c

...

typedef struct
{
    uint8_t addr;    //!< Unshifted address
    i2c_port_t port; //!< I2C port number
    i2c_config_t cfg;
    uint32_t timeout_ticks;


    i2c_config_t cfg;       //!< I2C driver configuration
    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 */
    SemaphoreHandle_t mutex; //!< Device mutex
} i2c_dev_t;


esp_err_t i2c_dev_new(i2c_dev_t *dev, const i2c_dev_config_t *config)
{
  // legacy implementation
}

esp_err_t i2c_dev_read(const i2c_dev_t *dev, const void *out_data, size_t out_size, void *in_data, size_t in_size)
{
  // legacy implementation
}

esp_err_t i2c_dev_write(const i2c_dev_t *dev, const void *out_reg, size_t out_reg_size, const void *out_data, size_t out_size);

esp_err_t i2c_dev_read_reg(const i2c_dev_t *dev, uint8_t reg, void *in_data, size_t in_size)
{
  // legacy implementation
}

esp_err_t i2c_dev_write_reg(const i2c_dev_t *dev, uint8_t reg, const void *out_data, size_t out_size)
{
  // legacy implementation
}
...

i2cdev_master.c

...

typedef struct
{
    uint8_t addr;    //!< Unshifted address
    i2c_port_t port; //!< I2C port number
    i2c_config_t cfg;
    uint32_t timeout_ticks;

    i2c_master_dev_handle_t dev_handle; //!< I2C driver handle
    i2c_master_bus_handle_t bus_handle; //!< I2C bus handle

    SemaphoreHandle_t mutex; //!< Device mutex
} i2c_dev_t;


esp_err_t i2c_dev_new(i2c_dev_t *dev, const i2c_dev_config_t *config)
{
  // master implementation
}

esp_err_t i2c_dev_read(const i2c_dev_t *dev, const void *out_data, size_t out_size, void *in_data, size_t in_size)
{
  // master implementation
}

esp_err_t i2c_dev_write(const i2c_dev_t *dev, const void *out_reg, size_t out_reg_size, const void *out_data, size_t out_size);

esp_err_t i2c_dev_read_reg(const i2c_dev_t *dev, uint8_t reg, void *in_data, size_t in_size)
{
  // master implementation
}

esp_err_t i2c_dev_write_reg(const i2c_dev_t *dev, uint8_t reg, const void *out_data, size_t out_size)
{
  // master implementation
}
...

With KConfig parameter or in CMakeLists.txt we could select legacy/master/custom/... implementation.

set(req driver freertos esp_idf_lib_helpers)

if(${IDF_TARGET} STREQUAL esp8266)
    append(req esp8266)
else()
    if((${IDF_VERSION_MAJOR} EQUAL 5) AND (${IDF_VERSION_MINOR} GREATER_EQUAL 3) AND (${IDF_VERSION_PATCH} GREATER_EQUAL 0))
        set(srcs i2cdev_master.c)
        message(STATUS "Using i2cdev_master.c")
    else()
        set(srcs i2cdev_legacy.c)
        message(STATUS "Using i2cdev_legacy.c")
    endif()
endif()

idf_component_register(
    SRCS ${srcs}
    INCLUDE_DIRS .
    REQUIRES ${req}
)

[trombik]

I am for it. What I want to avoid is, duplicated efforts. If you have a possible solution for the issue:

• create a branch
• implement it
• create a PR as a draft
• call for the discussion in the PR.

Please take my requirements into account.

[hiperiondev]

Maybe this project can help: https://github.com/lewisxhe/SensorLib

[trombik]

How?

[hiperiondev]

Seems to have a good i2c implementation in https://github.com/lewisxhe/SensorLib/blob/master/src/platform/espidf/

[quinkq]

I've created this i2cdev prototype that's been giving me promising results so far #707. I'd appreciate any feedback.

[trombik]

#707 has been merged.