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

icm42670: Support I2C NG driver #353

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

Conversation

espzav
Copy link
Collaborator

@espzav espzav commented Jul 26, 2024

ESP-BSP Pull Request checklist

  • Version of modified component bumped
  • CI passing

Change description

The first PR, how to support I2C NG driver in BSP components.

@espzav espzav force-pushed the feature/support_i2c_ng_icm42670 branch 8 times, most recently from 0bfe0da to a895f73 Compare July 26, 2024 10:03
@espzav espzav force-pushed the feature/support_i2c_ng_icm42670 branch from a895f73 to 27e5698 Compare July 26, 2024 10:17
@espzav espzav mentioned this pull request Jul 26, 2024
3 tasks
@espzav
Copy link
Collaborator Author

espzav commented Jul 30, 2024

@tore-espressif PTAL

* @param dev_addr I2C device address of sensor
*
* @return
* - NULL Fail
* - Others Success
*/
#if (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 3, 0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this rule might be too limiting. The new I2C Driver-NG is available from IDFv5.2 In practice, there can be other scenarios:

  1. IDF v5.2 + Driver-NG
  2. IDF v5.3 (or later) + Legacy driver

The only rule is that the driver-ng and the legacy driver cannot be linked in the same application.

So I can see 2 options:

  1. Maintain two versions of this component 1.x: legacy driver, 2.x: Driver-NG
  2. Make this configurable through Menuconfig

ATM, option 2 seems better to me because it is aligned with linear versioning of our components and option 1 is not scalable to BSP components

Copy link
Collaborator

Choose a reason for hiding this comment

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

@igrr @espzav What do you think?

Copy link
Collaborator

@pborcin pborcin left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants