Update seires legged branch#3
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the bipedal wheel controller to enhance its robustness and configurability. It centralizes Virtual Model Control logic into a dedicated class, improves the precision and adaptability of leg state detection and recovery behaviors, and introduces new parameters for fine-tuning control. These changes aim to make the controller more resilient to various operational conditions and easier to manage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates to the series legged bipedal controller, including refactoring VMC logic into a dedicated class and adjusting control logic across various modes. While these changes improve parameterization by removing some hardcoded values, several issues need attention. A critical bug was found where hardcoded values are used instead of configured parameters in the new VMC class. Additionally, many new magic numbers have been introduced, and large blocks of commented-out code should be removed to improve maintainability. Adhering to C++ and CMake best practices, such as using enum class and avoiding file(GLOB_RECURSE), would further enhance the code quality.
| const double l1 = 0.218; | ||
| const double l2 = 0.26; |
There was a problem hiding this comment.
This function uses hardcoded values for l1 and l2, but the VMC class stores l1_ and l2_ as member variables initialized from parameters. This is a critical bug, as the calculations will be incorrect if the configured parameters are different from these hardcoded values. You should use the member variables l1_ and l2_ instead.
| const double l1 = 0.218; | |
| const double l2 = 0.26; | |
| const double l1 = l1_; | |
| const double l2 = l2_; |
| ROS_ERROR("Load threshold param fail, check the resist of " | ||
| "under_threshold, front_threshold, behind_threshold"); |
There was a problem hiding this comment.
There's a typo in the error message. "resist of" should probably be "existence of" or "presence of". Clear error messages are important for debugging.
| ROS_ERROR("Load threshold param fail, check the resist of " | |
| "under_threshold, front_threshold, behind_threshold"); | |
| ROS_ERROR("Load threshold param fail, check the existence of " | |
| "under_threshold, front_threshold, behind_threshold"); |
| if (jump_phase_ == JumpPhase::IDLE && linear_acc_base_.z < -7.0 && controller->getCompleteStand() && | ||
| abs(vel_cmd_.x) > 0.1 && abs(x_left(3)) > 0.1 && ((left_pos_[0] + right_pos_[0]) / 2.0f) > 0.30 && | ||
| leg_length_des > 0.30) |
There was a problem hiding this comment.
The values -7.0, 0.1, and 0.30 are magic numbers. They should be defined as named constants with descriptive names to improve readability and make them easier to tune.
| double leg_recovery_velocity_{ 2.0 }, threshold_{ 0.05 }, leg_theta_diff_{ 0.0 }, desired_leg_length_{ 0.36 }; | ||
| const double leg_recovery_velocity_const_{ 2.0 }; | ||
| RecoveryChassisState recovery_chassis_state_{ ForwardSlip }; | ||
| bool detectd_flag{ false }; |
There was a problem hiding this comment.
There is a typo in the variable name detectd_flag. It should be detected_flag for clarity and correctness.
| bool detectd_flag{ false }; | |
| bool detected_flag{ false }; |
| else | ||
| { | ||
| cmd.torque = angle_vel_pid.computeCommand(-4 - leg_spd[1], period); | ||
| cmd.torque = angle_vel_pid.computeCommand(-5 - leg_spd[1], period); |
There was a problem hiding this comment.
The magic number -5 should be replaced with a named constant to improve readability and maintainability. This makes it easier to understand the purpose of the value and to change it in one place if needed.
| theta_des = M_PI / 2 - 0.35; | ||
| length_des = 0.36; | ||
| if (leg_length > 0.35) | ||
| { | ||
| leg_state = LegState::FRONT; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| theta_des = 0.0; | ||
| length_des = 0.18; | ||
| } | ||
| break; | ||
| case LegState::FRONT: | ||
| theta_des = M_PI / 2 - 0.35; | ||
| theta_des = M_PI_2 - 0.35; | ||
| length_des = 0.36; | ||
| if (abs(x[0] - theta_des) < 0.3 && abs(x[4]) < 0.3) | ||
| if ((abs(x[0] - theta_des) < 0.3 && abs(x[4]) < 0.3) || (abs(x[1]) < 0.1 && x[0] > M_PI_2)) |
There was a problem hiding this comment.
This function contains several magic numbers (0.35, 0.36, 0.18, 0.3, 0.1). These should be replaced with named constants to improve readability and make the logic easier to understand and maintain. Consider making them configurable parameters if they need to be tuned.
| } | ||
|
|
||
| double theta_des_l{ M_PI_2 - 0.3 }, theta_des_r{ M_PI_2 - 0.3 }, length_des_l{ 0.18 }, length_des_r{ 0.18 }; | ||
| double theta_des_l{ M_PI_2 - 0.6 }, theta_des_r{ M_PI_2 - 0.6 }, length_des_l{ 0.18 }, length_des_r{ 0.18 }; |
There was a problem hiding this comment.
This line initializes theta_des_l and theta_des_r with magic numbers, but they are immediately overwritten on the next line by a value from parameters. This initialization is redundant and can be misleading. It should be removed.
| double theta_des_l{ M_PI_2 - 0.6 }, theta_des_r{ M_PI_2 - 0.6 }, length_des_l{ 0.18 }, length_des_r{ 0.18 }; | |
| double theta_des_l{}, theta_des_r{}, length_des_l{ 0.18 }, length_des_r{ 0.18 }; |
| thigh_angle = jointThigh_.getPosition() + M_PI_2; | ||
| knee_angle = jointKnee_.getPosition() - M_PI_2; | ||
|
|
||
| // five link vmc | ||
| thigh_angle = jointThigh_.getPosition() + M_PI; | ||
| knee_angle = jointKnee_.getPosition(); | ||
| leg_pos(thigh_angle, knee_angle, position); | ||
| leg_spd(jointThigh_.getVelocity(), jointKnee_.getVelocity(), thigh_angle, knee_angle, speed); | ||
| // thigh_angle = jointThigh_.getPosition() + M_PI; | ||
| // knee_angle = jointKnee_.getPosition(); |
There was a problem hiding this comment.
Toggling between simulation (gazebo) and real hardware (five link vmc) code using comments is error-prone and not ideal for maintainability. Consider using preprocessor directives (#ifdef/#endif) to conditionally compile the correct block of code based on a CMake flag. This makes the configuration explicit and reduces the risk of committing the wrong code.
| // | ||
|
|
||
| #include "bipedal_wheel_controller/vmc/VMC.h" | ||
| #include <math.h> |
There was a problem hiding this comment.
In C++, it is best practice to include C standard library headers using the c prefix and no .h extension (e.g., <cmath>). This places the symbols in the std namespace.
| #include <math.h> | |
| #include <cmath> |
| bool init(hardware_interface::RobotHW* robot_hw, ros::NodeHandle& root_nh, ros::NodeHandle& controller_nh) override; | ||
| void moveJoint(const ros::Time& time, const ros::Duration& period) override; | ||
| void stopping(const ros::Time& time) override; | ||
| // void follow(const ros::Time& time, const ros::Duration& period) override; |
There was a problem hiding this comment.
This commented-out function declaration should be removed to keep the code clean. If the function is no longer needed, it's best to delete it entirely rather than leaving it as dead code.
2 participants
No description provided.