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.