chore: Update to latest espp, use idf-component-manager #12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the project to use the latest espp implementation managed via idf-component-manager while aligning the TinyS3TestStand component with the motor-go BSP component and updating CI/CMake settings.
- Removed the espp submodule and added idf_component.yml manifest files.
- Updated motor initialization API calls in main.cpp to use configuration objects and pointer-based access.
- Revised the TinyS3TestStand interface to include an explicit constructor and singleton getter, and updated the CI workflows and CMake minimum version.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| main/main.cpp | Updated motor initialization and API usage for hardware configuration changes. |
| main/idf_component.yml | Added manifest file for idf-component-manager integration. |
| components/tinys3_test_stand/src/tinys3_test_stand.cpp | Adjusted encoder/driver initialization for the test stand component. |
| components/tinys3_test_stand/include/tinys3_test_stand.hpp | Reworked TinyS3TestStand interface with updated API types, singleton pattern, and explicit constructor. |
| components/tinys3_test_stand/idf_component.yml | Added manifest file for the TinyS3TestStand component. |
| components/tinys3_test_stand/CMakeLists.txt | Included source directory in component registration. |
| components/espp | Removed the espp submodule reference. |
| CMakeLists.txt | Updated the required minimum CMake version to 3.20. |
| .gitmodules | Removed the espp submodule. |
| .github/workflows/* | Updated CI workflows to target esp-idf 5.4 and adjust packaging configurations. |
Comments suppressed due to low confidence (1)
components/tinys3_test_stand/include/tinys3_test_stand.hpp:147
- The explicit constructor is declared here but its definition is not shown. Ensure it properly initializes members such as 'encoder_' and 'motor_driver_' so that the fields used in default_motor_config are set to valid objects before use.
explicit TinyS3TestStand();
There was a problem hiding this comment.
Pull Request Overview
This PR updates the repository to use the latest ESPP via idf-component-manager instead of an espp submodule and adjusts component configurations, API changes, CI workflows, and CMake requirements.
- Replace espp submodule with idf-component-manager and update corresponding manifest files
- Update main code to use new API patterns for motor initialization and configuration
- Revise CI workflows and minimum CMake version to better align with the updated ESP-IDF version
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdkconfig.test_stand | Adds hardware configuration for the test stand |
| sdkconfig.motorgo | Adds hardware configuration for the MotorGo Mini component |
| main/main.cpp | Updates motor initialization and API usage for both hardware types |
| main/idf_component.yml | Adds an IDF component manifest file for the main component |
| components/tinys3_test_stand/src/tinys3_test_stand.cpp | Implements initialization and configuration for the Test Stand |
| components/tinys3_test_stand/include/tinys3_test_stand.hpp | Refactors API to use singleton pattern, shared pointers, and filters |
| components/tinys3_test_stand/idf_component.yml | Adds an IDF manifest file for the Test Stand component |
| components/tinys3_test_stand/CMakeLists.txt | Updates the component registration with proper source directories |
| components/espp | Removes the espp submodule |
| CMakeLists.txt | Updates CMake minimum requirement version |
| .gitmodules | Removes the espp submodule reference |
| .github/workflows/*.yml | Updates CI workflows to use esp-idf 5.4 and matrix build strategies |
Comment on lines
+168
to
+170
| // NOTE: use explicit type of nullptr to force allocation of control block, so | ||
| // that it can be shared even if it's nullptr; | ||
| std::shared_ptr<Encoder> encoder_{(Encoder *)(nullptr)}; |
There was a problem hiding this comment.
Consider initializing the shared pointer using 'nullptr' directly rather than an explicit type cast (e.g., std::shared_ptr encoder_{nullptr};) for improved code clarity and maintainability.
Suggested change
| // NOTE: use explicit type of nullptr to force allocation of control block, so | |
| // that it can be shared even if it's nullptr; | |
| std::shared_ptr<Encoder> encoder_{(Encoder *)(nullptr)}; | |
| // NOTE: use nullptr directly to force allocation of control block, so | |
| // that it can be shared even if it's nullptr; | |
| std::shared_ptr<Encoder> encoder_{nullptr}; |
Comment on lines
+185
to
+196
| // NOTE: use explicit type of nullptr to force allocation of control block, so | ||
| // that it can be shared even if it's nullptr; | ||
| std::shared_ptr<espp::BldcDriver> motor_driver_{(espp::BldcDriver *)(nullptr)}; | ||
|
|
||
| // Filters | ||
| espp::SimpleLowpassFilter motor_velocity_filter_{{.time_constant = 0.005f}}; | ||
| espp::SimpleLowpassFilter motor_angle_filter_{{.time_constant = 0.001f}}; | ||
| VelocityFilter motor_velocity_filter_{{.time_constant = 0.005f}}; | ||
| AngleFilter motor_angle_filter_{{.time_constant = 0.001f}}; | ||
|
|
||
| // Motor | ||
| BldcMotor motor_{{ | ||
| .num_pole_pairs = 7, | ||
| .phase_resistance = 5.0f, | ||
| .kv_rating = 320, | ||
| .current_limit = 1.0f, | ||
| .foc_type = espp::detail::FocType::SPACE_VECTOR_PWM, | ||
| // create shared_ptr from raw pointer to ensure shared_ptr doesn't delete the object | ||
| .driver = | ||
| std::shared_ptr<espp::BldcDriver>(std::shared_ptr<espp::BldcDriver>{}, &motor_driver_), | ||
| // create shared_ptr from raw pointer to ensure shared_ptr doesn't delete the object | ||
| .sensor = std::shared_ptr<Encoder>(std::shared_ptr<Encoder>{}, &encoder_), | ||
| .velocity_pid_config = | ||
| { | ||
| .kp = 0.010f, | ||
| .ki = 1.000f, | ||
| .kd = 0.000f, | ||
| .integrator_min = -1.0f, // same scale as output_min (so same scale as current) | ||
| .integrator_max = 1.0f, // same scale as output_max (so same scale as current) | ||
| .output_min = -1.0, // velocity pid works on current (if we have phase resistance) | ||
| .output_max = 1.0, // velocity pid works on current (if we have phase resistance) | ||
| }, | ||
| .angle_pid_config = | ||
| { | ||
| .kp = 7.000f, | ||
| .ki = 0.300f, | ||
| .kd = 0.010f, | ||
| .integrator_min = -10.0f, // same scale as output_min (so same scale as velocity) | ||
| .integrator_max = 10.0f, // same scale as output_max (so same scale as velocity) | ||
| .output_min = -20.0, // angle pid works on velocity (rad/s) | ||
| .output_max = 20.0, // angle pid works on velocity (rad/s) | ||
| }, | ||
| .velocity_filter = [this](auto v) { return motor_velocity_filter_(v); }, | ||
| .angle_filter = [this](auto v) { return motor_angle_filter_(v); }, | ||
| .auto_init = false, // we have to initialize the SPI first before we can use the encoder | ||
| .log_level = get_log_level(), | ||
| }}; | ||
| // NOTE: use explicit type of nullptr to force allocation of control block, so | ||
| // that it can be shared even if it's nullptr; | ||
| std::shared_ptr<BldcMotor> motor_{(BldcMotor *)(nullptr)}; |
There was a problem hiding this comment.
It is recommended to replace the explicit cast with a direct 'nullptr' initialization (e.g., std::shared_ptrespp::BldcDriver motor_driver_{nullptr};) to enhance readability.
Comment on lines
+185
to
+196
| // NOTE: use explicit type of nullptr to force allocation of control block, so | ||
| // that it can be shared even if it's nullptr; | ||
| std::shared_ptr<espp::BldcDriver> motor_driver_{(espp::BldcDriver *)(nullptr)}; | ||
|
|
||
| // Filters | ||
| espp::SimpleLowpassFilter motor_velocity_filter_{{.time_constant = 0.005f}}; | ||
| espp::SimpleLowpassFilter motor_angle_filter_{{.time_constant = 0.001f}}; | ||
| VelocityFilter motor_velocity_filter_{{.time_constant = 0.005f}}; | ||
| AngleFilter motor_angle_filter_{{.time_constant = 0.001f}}; | ||
|
|
||
| // Motor | ||
| BldcMotor motor_{{ | ||
| .num_pole_pairs = 7, | ||
| .phase_resistance = 5.0f, | ||
| .kv_rating = 320, | ||
| .current_limit = 1.0f, | ||
| .foc_type = espp::detail::FocType::SPACE_VECTOR_PWM, | ||
| // create shared_ptr from raw pointer to ensure shared_ptr doesn't delete the object | ||
| .driver = | ||
| std::shared_ptr<espp::BldcDriver>(std::shared_ptr<espp::BldcDriver>{}, &motor_driver_), | ||
| // create shared_ptr from raw pointer to ensure shared_ptr doesn't delete the object | ||
| .sensor = std::shared_ptr<Encoder>(std::shared_ptr<Encoder>{}, &encoder_), | ||
| .velocity_pid_config = | ||
| { | ||
| .kp = 0.010f, | ||
| .ki = 1.000f, | ||
| .kd = 0.000f, | ||
| .integrator_min = -1.0f, // same scale as output_min (so same scale as current) | ||
| .integrator_max = 1.0f, // same scale as output_max (so same scale as current) | ||
| .output_min = -1.0, // velocity pid works on current (if we have phase resistance) | ||
| .output_max = 1.0, // velocity pid works on current (if we have phase resistance) | ||
| }, | ||
| .angle_pid_config = | ||
| { | ||
| .kp = 7.000f, | ||
| .ki = 0.300f, | ||
| .kd = 0.010f, | ||
| .integrator_min = -10.0f, // same scale as output_min (so same scale as velocity) | ||
| .integrator_max = 10.0f, // same scale as output_max (so same scale as velocity) | ||
| .output_min = -20.0, // angle pid works on velocity (rad/s) | ||
| .output_max = 20.0, // angle pid works on velocity (rad/s) | ||
| }, | ||
| .velocity_filter = [this](auto v) { return motor_velocity_filter_(v); }, | ||
| .angle_filter = [this](auto v) { return motor_angle_filter_(v); }, | ||
| .auto_init = false, // we have to initialize the SPI first before we can use the encoder | ||
| .log_level = get_log_level(), | ||
| }}; | ||
| // NOTE: use explicit type of nullptr to force allocation of control block, so | ||
| // that it can be shared even if it's nullptr; | ||
| std::shared_ptr<BldcMotor> motor_{(BldcMotor *)(nullptr)}; |
There was a problem hiding this comment.
Using 'nullptr' directly instead of an explicit type cast (e.g., std::shared_ptr motor_{nullptr};) would improve code clarity and adhere to modern C++ best practices.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Keeps the repo up to date wit the latest fixes and features.
How has this been tested?
Building
main.Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action.