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.

Sign up for GitHub

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

feat: support <driver/i2c_master.h> in i2cdev #655

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat: support <driver/i2c_master.h> in i2cdev #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion components/i2cdev/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,15 @@ else()
set(req driver freertos esp_idf_lib_helpers)
endif()

set(srcs "i2cdev.c")
if(CONFIG_I2CDEV_USING_LEGACY_I2C)
list(APPEND srcs "i2cdev_legacy.c")
else()
list(APPEND srcs "i2cdev_master.c")
endif()

idf_component_register(
SRCS i2cdev.c
SRCS "${srcs}"
INCLUDE_DIRS .
REQUIRES ${req}
)
16 changes: 11 additions & 5 deletions components/i2cdev/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,20 @@ config I2CDEV_TIMEOUT
int "I2C transaction timeout, milliseconds"
default 1000
range 10 5000

config I2CDEV_NOLOCK
bool "Disable the use of mutexes"
default n
help
Attention! After enabling this option, all I2C device
drivers will become non-thread safe.
drivers will become non-thread safe.
Use this option if you need to access your I2C devices
from interrupt handlers.

endmenu
from interrupt handlers.

config I2CDEV_USING_LEGACY_I2C
bool "Use legacy driver/i2c.h"
default y
help
Use the legacy "driver/i2c.h" instead of "driver/i2c_master.h"
introduced in esp-idf 5.2.
endmenu
216 changes: 8 additions & 208 deletions components/i2cdev/i2cdev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,78 +37,19 @@
#include <esp_log.h>
#include "i2cdev.h"

static const char *TAG = "i2cdev";

typedef struct {
SemaphoreHandle_t lock;
i2c_config_t config;
bool installed;
} i2c_port_state_t;

static i2c_port_state_t states[I2C_NUM_MAX];

#if CONFIG_I2CDEV_NOLOCK
#define SEMAPHORE_TAKE(port)
#else
#define SEMAPHORE_TAKE(port) do { \
if (!xSemaphoreTake(states[port].lock, pdMS_TO_TICKS(CONFIG_I2CDEV_TIMEOUT))) \
{ \
ESP_LOGE(TAG, "Could not take port mutex %d", port); \
return ESP_ERR_TIMEOUT; \
} \
} while (0)
#endif
#include "i2cdev_legacy.h"
// #include "i2cdev_master.h"

#if CONFIG_I2CDEV_NOLOCK
#define SEMAPHORE_GIVE(port)
#else
#define SEMAPHORE_GIVE(port) do { \
if (!xSemaphoreGive(states[port].lock)) \
{ \
ESP_LOGE(TAG, "Could not give port mutex %d", port); \
return ESP_FAIL; \
} \
} while (0)
#endif
static const char *TAG = "i2cdev";

esp_err_t i2cdev_init()
{
memset(states, 0, sizeof(states));

#if !CONFIG_I2CDEV_NOLOCK
for (int i = 0; i < I2C_NUM_MAX; i++)
{
states[i].lock = xSemaphoreCreateMutex();
if (!states[i].lock)
{
ESP_LOGE(TAG, "Could not create port mutex %d", i);
return ESP_FAIL;
}
}
#endif

return ESP_OK;
return _i2cdev_init();
}

esp_err_t i2cdev_done()
{
for (int i = 0; i < I2C_NUM_MAX; i++)
{
if (!states[i].lock) continue;

if (states[i].installed)
{
SEMAPHORE_TAKE(i);
i2c_driver_delete(i);
states[i].installed = false;
SEMAPHORE_GIVE(i);
}
#if !CONFIG_I2CDEV_NOLOCK
vSemaphoreDelete(states[i].lock);
#endif
states[i].lock = NULL;
}
return ESP_OK;
return _i2cdev_done();
}

esp_err_t i2c_dev_create_mutex(i2c_dev_t *dev)
Expand Down Expand Up @@ -173,160 +114,19 @@ esp_err_t i2c_dev_give_mutex(i2c_dev_t *dev)
return ESP_OK;
}

inline static bool cfg_equal(const i2c_config_t *a, const i2c_config_t *b)
{
return a->scl_io_num == b->scl_io_num
&& a->sda_io_num == b->sda_io_num
#if HELPER_TARGET_IS_ESP32
&& a->master.clk_speed == b->master.clk_speed
#elif HELPER_TARGET_IS_ESP8266
&& ((a->clk_stretch_tick && a->clk_stretch_tick == b->clk_stretch_tick)
|| (!a->clk_stretch_tick && b->clk_stretch_tick == I2CDEV_MAX_STRETCH_TIME)
) // see line 232
#endif
&& a->scl_pullup_en == b->scl_pullup_en
&& a->sda_pullup_en == b->sda_pullup_en;
}

static esp_err_t i2c_setup_port(const i2c_dev_t *dev)
{
if (dev->port >= I2C_NUM_MAX) return ESP_ERR_INVALID_ARG;

esp_err_t res;
if (!cfg_equal(&dev->cfg, &states[dev->port].config) || !states[dev->port].installed)
{
ESP_LOGD(TAG, "Reconfiguring I2C driver on port %d", dev->port);
i2c_config_t temp;
memcpy(&temp, &dev->cfg, sizeof(i2c_config_t));
temp.mode = I2C_MODE_MASTER;

// Driver reinstallation
if (states[dev->port].installed)
{
i2c_driver_delete(dev->port);
states[dev->port].installed = false;
}
#if HELPER_TARGET_IS_ESP32
#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 1, 0)
// See https://github.com/espressif/esp-idf/issues/10163
if ((res = i2c_driver_install(dev->port, temp.mode, 0, 0, 0)) != ESP_OK)
return res;
if ((res = i2c_param_config(dev->port, &temp)) != ESP_OK)
return res;
#else
if ((res = i2c_param_config(dev->port, &temp)) != ESP_OK)
return res;
if ((res = i2c_driver_install(dev->port, temp.mode, 0, 0, 0)) != ESP_OK)
return res;
#endif
#endif
#if HELPER_TARGET_IS_ESP8266
// Clock Stretch time, depending on CPU frequency
temp.clk_stretch_tick = dev->timeout_ticks ? dev->timeout_ticks : I2CDEV_MAX_STRETCH_TIME;
if ((res = i2c_driver_install(dev->port, temp.mode)) != ESP_OK)
return res;
if ((res = i2c_param_config(dev->port, &temp)) != ESP_OK)
return res;
#endif
states[dev->port].installed = true;

memcpy(&states[dev->port].config, &temp, sizeof(i2c_config_t));
ESP_LOGD(TAG, "I2C driver successfully reconfigured on port %d", dev->port);
}
#if HELPER_TARGET_IS_ESP32
int t;
if ((res = i2c_get_timeout(dev->port, &t)) != ESP_OK)
return res;
// Timeout cannot be 0
uint32_t ticks = dev->timeout_ticks ? dev->timeout_ticks : I2CDEV_MAX_STRETCH_TIME;
if ((ticks != t) && (res = i2c_set_timeout(dev->port, ticks)) != ESP_OK)
return res;
ESP_LOGD(TAG, "Timeout: ticks = %" PRIu32 " (%" PRIu32 " usec) on port %d", dev->timeout_ticks, dev->timeout_ticks / 80, dev->port);
#endif

return ESP_OK;
}

esp_err_t i2c_dev_probe(const i2c_dev_t *dev, i2c_dev_type_t operation_type)
{
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);
Comment on lines -252 to +119
Copy link

@camielverdult camielverdult Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely right.

}

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)
{
if (!dev || !in_data || !in_size) 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();
if (out_data && out_size)
{
i2c_master_start(cmd);
i2c_master_write_byte(cmd, dev->addr << 1, true);
i2c_master_write(cmd, (void *)out_data, out_size, true);
}
i2c_master_start(cmd);
i2c_master_write_byte(cmd, (dev->addr << 1) | 1, true);
i2c_master_read(cmd, in_data, in_size, I2C_MASTER_LAST_NACK);
i2c_master_stop(cmd);

res = i2c_master_cmd_begin(dev->port, cmd, pdMS_TO_TICKS(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));

i2c_cmd_link_delete(cmd);
}

SEMAPHORE_GIVE(dev->port);
return res;
return _i2c_dev_read(dev, out_data, out_size, in_data, 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)
{
if (!dev || !out_data || !out_size) 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, true);
if (out_reg && out_reg_size)
i2c_master_write(cmd, (void *)out_reg, out_reg_size, true);
i2c_master_write(cmd, (void *)out_data, out_size, true);
i2c_master_stop(cmd);
res = i2c_master_cmd_begin(dev->port, cmd, pdMS_TO_TICKS(CONFIG_I2CDEV_TIMEOUT));
if (res != ESP_OK)
ESP_LOGE(TAG, "Could not write to device [0x%02x at %d]: %d (%s)", dev->addr, dev->port, res, esp_err_to_name(res));
i2c_cmd_link_delete(cmd);
}

SEMAPHORE_GIVE(dev->port);
return res;
return _i2c_dev_write(dev, out_reg, out_reg_size, out_data, out_size);
}

esp_err_t i2c_dev_read_reg(const i2c_dev_t *dev, uint8_t reg, void *in_data, size_t in_size)
Expand Down
27 changes: 22 additions & 5 deletions components/i2cdev/i2cdev.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,17 @@
#ifndef __I2CDEV_H__
#define __I2CDEV_H__

#include <driver/i2c.h>
#include <freertos/FreeRTOS.h>
#include <freertos/semphr.h>
#include <esp_err.h>
#include <esp_idf_lib_helpers.h>

#if CONFIG_I2CDEV_USING_LEGACY_I2C
#include <driver/i2c.h>
#else
#include <driver/i2c_master.h>
#endif

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -68,12 +73,23 @@ extern "C" {
typedef struct
{
i2c_port_t port; //!< I2C port number
i2c_config_t cfg; //!< I2C driver configuration
uint8_t addr; //!< Unshifted address
SemaphoreHandle_t mutex; //!< Device mutex
#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
Comment on lines +76 to +82
Copy link

@camielverdult camielverdult Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

i2c_master_bus_handle_t bus; //!< I2C master bus handle (i2c_master.h only)
i2c_master_dev_handle_t device; //!< I2C master bus device handle
//(i2c_master.h only)
i2c_device_config_t device_config; //!< I2C device configuration (i2c_master.h only)
i2c_master_bus_config_t master_bus_config; /*! < I2C master bus specific
configurations (i2c_master.h
only) */
#endif
uint8_t addr; //!< Unshifted address
SemaphoreHandle_t mutex; //!< Device mutex
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use use the mutex from i2c_port_state_t?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the part is obtained from the old code, I cannot comment on this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh alright, it may be that they use the same logic for controlling the mutex. We'll have to ask the author

} i2c_dev_t;

/**
Expand All @@ -97,6 +113,7 @@ esp_err_t i2cdev_init();
/**
* @brief Finish work with library
*
* Delete Deinitialize the I2C master bus. When the legacy i2c.h is used,
* Uninstall i2c drivers.
*
* @return ESP_OK on success
Expand Down
Loading
Loading