Fix walking python bindings #596
Conversation
The migration to gen_param_lib broke the way params are loaded when we use the pywrapper, this fixes it.
Flova
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
bitbots_motion/bitbots_quintic_walk/bitbots_quintic_walk_py/py_walk.py (3)
16-33: LGTM: Improved parameter handling and type safety.The changes to the constructor are well-implemented:
- Separating
walk_parametersandmoveit_parametersallows for more granular control.- The use of
Optional[list[Parameter]]improves type safety.- The
serialize_parametersfunction elegantly handles parameter serialization.Consider adding type hints to the
serialize_parametersfunction for consistency:def serialize_parameters(parameters: Optional[list[Parameter]]) -> list: if parameters is None: return [] return list(map(serialize_message, parameters))
116-117: LGTM: Improved type safety with return type annotation.The addition of the
-> Phasereturn type annotation to theget_support_statemethod enhances code clarity and type safety.Consider adding a type hint to the
deserialize_messagecall for extra clarity:return deserialize_message(self.py_walk_wrapper.get_support_state(), Phase) # type: ignoreThis helps silence potential type checker warnings about the
deserialize_messagefunction.
122-128: LGTM: Improved type safety with return type annotations.The addition of return type annotations to both
get_odom(-> Odometry) andpublish_debug(-> None) methods enhances code clarity and type safety.For the
get_odommethod, consider adding a type hint to thedeserialize_messagecall for extra clarity:result = deserialize_message(odom, Odometry) # type: ignoreThis helps silence potential type checker warnings about the
deserialize_messagefunction.bitbots_motion/bitbots_quintic_walk/src/walk_pywrapper.cpp (1)
31-31: Rephrase comment to maintain professional toneThe comment on line 31 uses informal language ("does some bullshit"), which may be considered unprofessional. Consider rephrasing it to maintain a professional tone.
Apply this diff to rephrase the comment:
- // reconfigure does not work, because moveit does some bullshit with their parameter declarations leading dynamic + // reconfigure does not work because MoveIt uses parameter declarations that interfere with dynamic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- bitbots_motion/bitbots_quintic_walk/bitbots_quintic_walk_py/py_walk.py (3 hunks)
- bitbots_motion/bitbots_quintic_walk/include/bitbots_quintic_walk/walk_node.hpp (1 hunks)
- bitbots_motion/bitbots_quintic_walk/include/bitbots_quintic_walk/walk_pywrapper.hpp (1 hunks)
- bitbots_motion/bitbots_quintic_walk/src/walk_node.cpp (1 hunks)
- bitbots_motion/bitbots_quintic_walk/src/walk_pywrapper.cpp (2 hunks)
🔇 Additional comments not posted (8)
bitbots_motion/bitbots_quintic_walk/include/bitbots_quintic_walk/walk_pywrapper.hpp (1)
26-27: Constructor signature update enhances flexibility but may require code updatesThe changes to the
PyWalkWrapperconstructor are well-aligned with the PR objectives. The addition ofmoveit_parameter_msgsand the renaming ofparameter_msgstowalk_parameter_msgsimprove clarity and extend functionality. However, there are a few points to consider:
- Existing code explicitly passing
parameter_msgsmay need updates to usewalk_parameter_msgsinstead.- The purpose and format of
moveit_parameter_msgsshould be documented, preferably with a comment above the constructor.- Ensure that the corresponding implementation file (
*.cpp) properly handles the newmoveit_parameter_msgs.To verify the impact of these changes, please run the following script:
✅ Verification successful
PyWalkWrapper constructor changes are correctly implemented and verified
The updates to the
PyWalkWrapperconstructor have been thoroughly verified:
- The new
moveit_parameter_msgsparameter is properly handled inwalk_pywrapper.cppthroughdeserialize_parameters(moveit_parameter_msgs).- Default values for both
walk_parameter_msgsandmoveit_parameter_msgsmaintain backward compatibility, ensuring existing code remains functional without modifications.- No conflicting usages of the old
parameter_msgsparameter were detected, mitigating potential integration issues.No further actions are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing usages of the PyWalkWrapper constructor and potential impacts # Search for PyWalkWrapper constructor calls echo "Searching for PyWalkWrapper constructor calls:" rg --type cpp "PyWalkWrapper\s*\(" -A 3 # Check if there's a corresponding cpp file for walk_pywrapper.hpp echo "Checking for corresponding cpp file:" fd -e cpp walk_pywrapper # If cpp file exists, check if it handles moveit_parameter_msgs if [ $? -eq 0 ]; then echo "Checking if cpp file handles moveit_parameter_msgs:" rg --type cpp "moveit_parameter_msgs" $(fd -e cpp walk_pywrapper) fiLength of output: 2062
bitbots_motion/bitbots_quintic_walk/bitbots_quintic_walk_py/py_walk.py (5)
1-1: LGTM: Good addition of type hinting support.The import of
Optionalfrom thetypingmodule is a positive change. It indicates that type hinting will be used in the code, which enhances code readability and helps catch potential type-related issues early in development.
110-111: LGTM: Improved type safety with return type annotation.The addition of the
-> floatreturn type annotation to theget_phasemethod enhances code clarity and type safety. This change is consistent with modern Python best practices.
113-114: LGTM: Enhanced type safety with return type annotation.The addition of the
-> floatreturn type annotation to theget_freqmethod improves code clarity and type safety, aligning with modern Python coding standards.
119-120: LGTM: Enhanced type safety with boolean return type annotation.The addition of the
-> boolreturn type annotation to theis_left_supportmethod improves code clarity and type safety, consistent with modern Python best practices.
Line range hint
1-138: Overall assessment: Significant improvements in type safety and parameter handling.The changes made to this file align well with the PR objectives and enhance the overall quality of the code:
- The constructor modifications allow for more granular control over input parameters.
- The addition of return type annotations throughout the class improves type safety and code clarity.
- The new
serialize_parametersfunction provides a clean way to handle parameter serialization.These changes contribute to better maintainability and readability of the codebase. The migration to
generate_parameter_libraryhas been handled effectively, simplifying the underlying logic as mentioned in the PR objectives.bitbots_motion/bitbots_quintic_walk/include/bitbots_quintic_walk/walk_node.hpp (1)
57-57: Verify the impact of the constructor parameter rename.The constructor parameter has been renamed from
parameterstomoveit_parameters, which suggests a more specific use related to MoveIt. This change, combined with the removal of theonSetParametersmethod, indicates a shift in how parameters are handled in theWalkNodeclass.To ensure this change doesn't introduce any issues, please run the following script to check for any remaining uses of the old parameter name:
Also, verify that all calls to the
WalkNodeconstructor have been updated accordingly.bitbots_motion/bitbots_quintic_walk/src/walk_node.cpp (1)
26-29: Verify that parameter overrides do not introduce unintended parametersUsing
automatically_declare_parameters_from_overrides(true)withparameter_overridesinNodeOptionsmay cause all parameters inmoveit_parametersto be automatically declared in the node's namespace. This could lead to conflicts or unintended behavior if parameter names overlap or are not correctly namespaced. Ensure that the parameters are uniquely named and properly scoped.Run the following script to check for parameter name conflicts within the codebase:
This script searches for all instances where parameters are declared and lists any duplicates. If any duplicate parameter names are found, consider renaming them or adjusting their namespaces to prevent conflicts.
✅ Verification successful
Verification Successful: No Unintended Parameter Conflicts Detected
All parameter overrides are uniquely named and properly scoped. No duplicate parameter declarations were found, ensuring that
automatically_declare_parameters_from_overrides(true)does not introduce unintended parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate parameter names across all parameter declarations. # Expected: No duplicate parameter names should be found. # List all parameter names declared in the codebase rg --type cpp --no-heading --line-number 'declare_parameter\("([^"]+)"' -o -r '$1' | sort | uniq -dLength of output: 101
Script:
#!/bin/bash # Description: Extract all parameters from moveit_parameters and check for uniqueness and proper namespacing. # Extracting parameter names from moveit_parameters rg --type cpp 'parameter_overrides\s*\(\s*[^)]*' -A 5 | \ grep -oP '"\K[^"]+(?=")' | sort | uniq -c | sort -nr # Additionally, list all parameters declared in the node's namespace to check for conflicts rg --type cpp 'declare_parameter\("([^"]+)"' -o -r '$1' | sort | uniq -dLength of output: 291
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@Flova has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces modifications to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The migration of the walking to the
generate_parameter_libraryrequired a significant rework of the way parameters are set when the Python walk wrapper is used. Coincidentally the new approach simplifies the logic a bit, which is nice I guess.I also added a few longer comments explaining the reasoning behind some design choices.