Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded SDMMC configuration and tooling updates: the 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b3db675-3bd0-49a3-bf99-56f9ae0109b8
📒 Files selected for processing (4)
Devices/lilygo-thmi/lilygo,thmi.dtsPlatforms/platform-esp32/bindings/espressif,esp32-sdmmc.yamlPlatforms/platform-esp32/include/tactility/drivers/esp32_sdmmc.hPlatforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cpp
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce0a974f-e435-45d0-8ce1-eb7f04bb3628
📒 Files selected for processing (6)
Buildscripts/TactilitySDK/CMakeLists.txtBuildscripts/release-sdk.pyDevices/generic-esp32/devicetree.yamlDevices/generic-esp32c6/devicetree.yamlDevices/generic-esp32p4/devicetree.yamlDevices/generic-esp32s3/devicetree.yaml
✅ Files skipped from review due to trivial changes (4)
- Devices/generic-esp32s3/devicetree.yaml
- Devices/generic-esp32c6/devicetree.yaml
- Devices/generic-esp32/devicetree.yaml
- Devices/generic-esp32p4/devicetree.yaml
| def create_module_cmakelists(module_name): | ||
| return dedent(f''' | ||
| cmake_minimum_required(VERSION 3.20) | ||
| idf_component_register( | ||
| INCLUDE_DIRS "include" | ||
| ) | ||
| add_prebuilt_library({module_name} "binary/lib{module_name}.a") | ||
| '''.format(module_name=module_name)) |
There was a problem hiding this comment.
Redundant .format() call on f-string; also missing target_link_libraries.
-
Line 85 uses
.format(module_name=module_name)on an f-string, which is redundant since the f-string already interpolates{module_name}. -
The generated CMakeLists.txt registers the prebuilt library but doesn't link it to
${COMPONENT_LIB}, so consumers won't automatically get the library linked.
🐛 Proposed fix
def create_module_cmakelists(module_name):
return dedent(f'''
cmake_minimum_required(VERSION 3.20)
idf_component_register(
INCLUDE_DIRS "include"
)
add_prebuilt_library({module_name} "binary/lib{module_name}.a")
- '''.format(module_name=module_name))
+ target_link_libraries(${{COMPONENT_LIB}} INTERFACE {module_name})
+ ''')Note: ${{COMPONENT_LIB}} uses double braces to escape the CMake variable from Python f-string interpolation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def create_module_cmakelists(module_name): | |
| return dedent(f''' | |
| cmake_minimum_required(VERSION 3.20) | |
| idf_component_register( | |
| INCLUDE_DIRS "include" | |
| ) | |
| add_prebuilt_library({module_name} "binary/lib{module_name}.a") | |
| '''.format(module_name=module_name)) | |
| def create_module_cmakelists(module_name): | |
| return dedent(f''' | |
| cmake_minimum_required(VERSION 3.20) | |
| idf_component_register( | |
| INCLUDE_DIRS "include" | |
| ) | |
| add_prebuilt_library({module_name} "binary/lib{module_name}.a") | |
| target_link_libraries(${{COMPONENT_LIB}} INTERFACE {module_name}) | |
| ''') |
| # Drivers | ||
| add_driver(target_path, "bm8563-module") | ||
| add_driver(target_path, "bm8563-module") | ||
| add_driver(target_path, "bmi270-module") | ||
| add_driver(target_path, "mpu6886-module") | ||
| add_driver(target_path, "pi4ioe5v6408-module") | ||
| add_driver(target_path, "qmi8658-module") | ||
| add_driver(target_path, "rx8130ce-module") |
There was a problem hiding this comment.
Duplicate entry: bm8563-module is added twice.
Lines 150-151 both call add_driver(target_path, "bm8563-module"). This mirrors the same duplicate in CMakeLists.txt and appears to be a copy-paste error.
🐛 Proposed fix
# Drivers
add_driver(target_path, "bm8563-module")
- add_driver(target_path, "bm8563-module")
add_driver(target_path, "bmi270-module")
add_driver(target_path, "mpu6886-module")
add_driver(target_path, "pi4ioe5v6408-module")
add_driver(target_path, "qmi8658-module")
add_driver(target_path, "rx8130ce-module")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Drivers | |
| add_driver(target_path, "bm8563-module") | |
| add_driver(target_path, "bm8563-module") | |
| add_driver(target_path, "bmi270-module") | |
| add_driver(target_path, "mpu6886-module") | |
| add_driver(target_path, "pi4ioe5v6408-module") | |
| add_driver(target_path, "qmi8658-module") | |
| add_driver(target_path, "rx8130ce-module") | |
| # Drivers | |
| add_driver(target_path, "bm8563-module") | |
| add_driver(target_path, "bmi270-module") | |
| add_driver(target_path, "mpu6886-module") | |
| add_driver(target_path, "pi4ioe5v6408-module") | |
| add_driver(target_path, "qmi8658-module") | |
| add_driver(target_path, "rx8130ce-module") |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Buildscripts/TactilitySDK/TactilitySDK.cmake (1)
1-5: Empty function stubs should be documented or removed.The
tactility_project()function and_tactility_project()function are empty stubs. If they serve a purpose (e.g., forward declarations or placeholders for future use), consider adding a comment. Otherwise, they add noise.Tests/SdkIntegration/main/Source/main.c (1)
27-33: Headers included for validation only - consider adding a comment.These driver headers appear to be included solely to validate SDK packaging rather than for actual use in the test. A brief comment would clarify intent.
📝 Suggested documentation
`#include` <tactility/lvgl_module.h> +// Driver headers included to validate SDK packaging `#include` <drivers/bm8563.h> `#include` <drivers/bmi270.h>Buildscripts/release-sdk.py (1)
91-95:add_driverusescreate_module_cmakelists— verify this is intentional.The
add_driverfunction (line 94) callscreate_module_cmakelists(driver_name)to generate the CMakeLists.txt. This creates identical CMakeLists content for both drivers and modules. If drivers require different CMake configuration (e.g., different REQUIRES), consider creating a separatecreate_driver_cmakelistsfunction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69a45870-e720-4b26-ae52-f82851b6030d
📒 Files selected for processing (6)
Buildscripts/TactilitySDK/CMakeLists.txtBuildscripts/TactilitySDK/TactilitySDK.cmakeBuildscripts/release-sdk.pyTests/SdkIntegration/CMakeLists.txtTests/SdkIntegration/main/CMakeLists.txtTests/SdkIntegration/main/Source/main.c
💤 Files with no reviewable changes (1)
- Buildscripts/TactilitySDK/CMakeLists.txt
| set(EXTRA_COMPONENT_DIRS | ||
| "Libraries/TactilityFreeRtos" | ||
| "Modules" | ||
| "Drivers" | ||
| ) |
There was a problem hiding this comment.
Relative paths in EXTRA_COMPONENT_DIRS may not resolve correctly.
The paths set here (Libraries/TactilityFreeRtos, Modules, Drivers) are relative but don't use ${TACTILITY_SDK_PATH} as a base. When the macro is invoked from an SDK consumer project, these paths will be resolved relative to the consumer's project directory rather than the SDK directory.
🐛 Proposed fix
set(EXTRA_COMPONENT_DIRS
- "Libraries/TactilityFreeRtos"
- "Modules"
- "Drivers"
+ "${TACTILITY_SDK_PATH}/Libraries/TactilityFreeRtos"
+ "${TACTILITY_SDK_PATH}/Modules"
+ "${TACTILITY_SDK_PATH}/Drivers"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(EXTRA_COMPONENT_DIRS | |
| "Libraries/TactilityFreeRtos" | |
| "Modules" | |
| "Drivers" | |
| ) | |
| set(EXTRA_COMPONENT_DIRS | |
| "${TACTILITY_SDK_PATH}/Libraries/TactilityFreeRtos" | |
| "${TACTILITY_SDK_PATH}/Modules" | |
| "${TACTILITY_SDK_PATH}/Drivers" | |
| ) |
| bm8563-module | ||
| bm8563-module |
There was a problem hiding this comment.
Duplicate entry: bm8563-module is listed twice.
Lines 26 and 27 both include bm8563-module in the COMPONENTS list. This appears to be a copy-paste error and should be removed to avoid potential issues with ESP-IDF's component resolution.
🐛 Proposed fix
set(COMPONENTS
TactilityFreeRtos
bm8563-module
- bm8563-module
bmi270-module
mpu6886-module
pi4ioe5v6408-module
qmi8658-module
rx8130ce-module
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bm8563-module | |
| bm8563-module | |
| set(COMPONENTS | |
| TactilityFreeRtos | |
| bm8563-module | |
| bmi270-module | |
| mpu6886-module | |
| pi4ioe5v6408-module | |
| qmi8658-module | |
| rx8130ce-module | |
| ) |
| set(EXTRA_COMPONENT_DIRS ${TACTILITY_SDK_PATH} ${TACTILITY_SDK_PATH}/Modules ${TACTILITY_SDK_PATH}/Drivers) | ||
|
|
||
| project(SdkTest) | ||
| tactility_project(SdkTest) |
There was a problem hiding this comment.
EXTRA_COMPONENT_DIRS will be overwritten by tactility_project macro.
Line 13 sets EXTRA_COMPONENT_DIRS, but the tactility_project macro (line 16) also sets this variable unconditionally (see lines 18-22 in TactilitySDK.cmake). The macro's assignment will override the test's paths.
Consider either:
- Having the macro append to
EXTRA_COMPONENT_DIRSinstead of replacing it - Moving this assignment after the macro call
- Setting paths within the macro using
${TACTILITY_SDK_PATH}as suggested in the other review
| #include <drivers/mpu6886.h> | ||
| #include <drivers/mpu6886.h> |
There was a problem hiding this comment.
Duplicate include: mpu6886.h is included twice.
Lines 29 and 30 both include <drivers/mpu6886.h>. Remove the duplicate.
🐛 Proposed fix
`#include` <drivers/bmi270.h>
`#include` <drivers/mpu6886.h>
-#include <drivers/mpu6886.h>
`#include` <drivers/pi4ioe5v6408.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <drivers/mpu6886.h> | |
| #include <drivers/mpu6886.h> | |
| `#include` <drivers/bmi270.h> | |
| `#include` <drivers/mpu6886.h> | |
| `#include` <drivers/pi4ioe5v6408.h> |
1 participant
Summary by CodeRabbit
New Features
Improvements
Build / Packaging
Tests / Integration