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

Closed
wants to merge 1 commit into 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

@espzav espzav requested review from tore-espressif and pborcin July 30, 2024 12:01
* @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

Choose a reason for hiding this comment

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

We agreed that we will make 'big leap' and support only Driver-NG in new major versions

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

tore-espressif added a commit that referenced this pull request Oct 10, 2024
@tore-espressif tore-espressif mentioned this pull request Oct 10, 2024
3 tasks
tore-espressif added a commit that referenced this pull request Oct 11, 2024
tore-espressif added a commit that referenced this pull request Oct 14, 2024
tore-espressif added a commit that referenced this pull request Oct 14, 2024
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