-
Notifications
You must be signed in to change notification settings - Fork 176
Fix/mpu6050 missing "esp_driver_gpio" dependency (BSP-735) #672
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
base: master
Are you sure you want to change the base?
Fix/mpu6050 missing "esp_driver_gpio" dependency (BSP-735) #672
Conversation
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
components/mpu6050/CMakeLists.txt
Outdated
| SRCS "mpu6050.c" | ||
| INCLUDE_DIRS "include" | ||
| REQUIRES "driver" | ||
| PRIV_REQUIRES "esp_driver_gpio" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Misplaced dependency breaks transitive public API access
The esp_driver_gpio dependency is added to PRIV_REQUIRES but should be in REQUIRES instead. The public header mpu6050.h (located in the public include directory) includes driver/gpio.h and uses gpio_num_t and gpio_isr_t types in its public API. When a public header includes files from another component, that component must be listed in REQUIRES (not PRIV_REQUIRES) so that users of the mpu6050 component get transitive access to the GPIO driver headers. Using PRIV_REQUIRES will cause compilation errors for projects that include mpu6050.h but don't directly depend on esp_driver_gpio.
espzav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget bump component version for release the component into component registry.
components/mpu6050/CMakeLists.txt
Outdated
| SRCS "mpu6050.c" | ||
| INCLUDE_DIRS "include" | ||
| REQUIRES "driver" | ||
| PRIV_REQUIRES "esp_driver_gpio" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use something like this:
| set(REQ esp_driver_gpio esp_driver_i2c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @espzav, thanks for the suggestion!
Now it uses the set(REQ ...) pattern and the version is bumped
diff --git a/components/mpu6050/CMakeLists.txt b/components/mpu6050/CMakeLists.txt
index 7447101..5654d82 100644
--- a/components/mpu6050/CMakeLists.txt
+++ b/components/mpu6050/CMakeLists.txt
@@ -1,6 +1,7 @@
+set(REQ esp_driver_gpio esp_driver_i2c)
+
idf_component_register(
SRCS "mpu6050.c"
INCLUDE_DIRS "include"
- REQUIRES "driver"
- PRIV_REQUIRES "esp_driver_gpio"
+ REQUIRES driver ${REQ}
)
diff --git a/components/mpu6050/idf_component.yml b/components/mpu6050/idf_component.yml
index 4d544f0..14bcd20 100644
--- a/components/mpu6050/idf_component.yml
+++ b/components/mpu6050/idf_component.yml
@@ -1,4 +1,4 @@
-version: "1.2.0"
+version: "1.2.1"
description: I2C driver for MPU6050 6-axis gyroscope and accelerometer
url: https://github.com/espressif/esp-bsp/tree/master/components/mpu6050
dependencies:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a private dependency on the esp_driver_gpio component to the mpu6050 component's CMakeLists configuration.
- Adds
esp_driver_gpioas a private requirement to the mpu6050 component
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1907d32 to
591c966
Compare
| SRCS "mpu6050.c" | ||
| INCLUDE_DIRS "include" | ||
| REQUIRES "driver" | ||
| REQUIRES driver ${REQ} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Backward Compatibility Bug in ESP-IDF Driver Linkage
Missing backward compatibility check for ESP-IDF versions before 5.3. The code unconditionally uses esp_driver_gpio and esp_driver_i2c components which don't exist in IDF versions < 5.3. The component claims to support "idf: >=4.0" but will fail to build on versions before 5.3. Should use a conditional check like: if("${IDF_VERSION_MAJOR}.${IDF_VERSION_MINOR}" VERSION_GREATER_EQUAL "5.3") to set REQ to the new driver components, else use the legacy "driver" component.
b1f26d6 to
ddb6a86
Compare
components/mpu6050/CMakeLists.txt
Outdated
| @@ -1,5 +1,7 @@ | |||
| set(REQ esp_driver_gpio esp_driver_i2c) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| set(REQ esp_driver_gpio esp_driver_i2c) | |
| if("${IDF_VERSION_MAJOR}.${IDF_VERSION_MINOR}" VERSION_GREATER_EQUAL "5.3") | |
| set(REQ esp_driver_gpio esp_driver_i2c) | |
| else() | |
| set(REQ driver) | |
| endif() |
| SRCS "mpu6050.c" | ||
| INCLUDE_DIRS "include" | ||
| REQUIRES "driver" | ||
| REQUIRES driver ${REQ} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| REQUIRES driver ${REQ} | |
| REQUIRES ${REQ} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just added the driver in the requires and set the conditional requires, thank you for you patience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tommaso-merlini Still missing else in condition and removed driver in REQUIRES. For IDF 6 cannot be driver there. It is divided into esp_driver_gpio esp_driver_i2c
ddb6a86 to
77c58aa
Compare
ESP-BSP Pull Request checklist
Change description
Component
mpu6050Issue
The
mpu6050.hheader includes<driver/gpio.h>, which is provided by theesp_driver_gpiocomponent. However,esp_driver_gpiowas not declared in the component's dependencies.This triggers a build error when inlcuding the library via
idf.py add-dependency "espressif/mpu6050^1.1.1":Note
Adds conditional
esp_driver_gpio/esp_driver_i2cdependencies for IDF>=5.3 and bumps component version to 1.2.1.components/mpu6050/CMakeLists.txt, conditionally addesp_driver_gpioandesp_driver_i2ctoREQUIRESwhenIDF_VERSION >= 5.3; keepdriverrequired.components/mpu6050/idf_component.ymlfrom1.2.0to1.2.1.Written by Cursor Bugbot for commit 77c58aa. This will update automatically on new commits. Configure here.