Bump 1.4.5 #42
Bump 1.4.5 #42
Conversation
- Introduced a new map for torque enable states per Dynamixel ID. - Removed global torque enable variable and updated logic to enable torque based on individual settings. - Adjusted parameter handling to include torque enable settings during initialization. - Updated model files to include additional parameters for current readings.
Signed-off-by: Wonho Yun <[email protected]>
…mand name conversions and refactoring state synchronization logic. Removed redundant definitions and improved handling of joint commands and states. Updated model files to reflect changes in command interfaces.
- Changed the initialization process to write Operating Mode parameters in the first pass. - Adjusted the logic to skip already written parameters, including Operating Mode and Limit parameters, in subsequent passes. - Improved clarity and efficiency of parameter handling during initialization.
…and read logic - Added new mapping functions for command and state interfaces using unordered maps. - Refactored read logic to support direct and indirect reading, improving error handling and data processing. - Updated initialization and synchronization methods for better clarity and efficiency. - Removed redundant definitions and streamlined state handling in the interface.
Signed-off-by: Wonho Yun <[email protected]>
Signed-off-by: Wonho Yun <[email protected]>
Signed-off-by: Wonho Yun <[email protected]>
Signed-off-by: Wonho Yun <[email protected]>
Signed-off-by: Wonho Yun <[email protected]>
…e error handling in stop method
Add Support for Various Command Interfaces
…are_interface into feature-cmake-fix
Signed-off-by: Wonho Yun <[email protected]>
Modified the CMakelists for kilted
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @yun-goon, 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!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR primarily focuses on bumping the package version to 1.4.5 and incorporating recent changes, including significant updates to the Dynamixel communication logic, improved error handling, and support for a new sensor model. The changes also involve updates to the CMake configuration to deprecate ament_include_dependency and add rclcpp_lifecycle.
Highlights
- Version Bump: The package version has been updated from 1.4.3 to 1.4.5.
- CMakeLists Updates: Deprecated
ament_include_dependencyusage and addedrclcpp_lifecycleas a required dependency and target link library. Also updated include directories and export dependencies. - Dynamixel Communication Refinement: Introduced support for 'Fast Sync Read' and 'Fast Bulk Read' protocols with a fallback mechanism to standard Sync/Bulk read if fast protocols fail. Added support for 'Direct Read' in addition to 'Indirect Read' for BulkRead operations. Refactored read/write handler initialization and data processing.
- Improved Error Handling: Added more robust error handling, particularly around reading Dynamixel model files and during communication attempts (Tx/Rx failures), with retry logic during the
startphase. - Per-Device Torque Control: Replaced the global torque enable control with per-device control based on parameters defined in the hardware configuration.
- New Sensor Model Support: Added support for the
sensorxel_joymodel by including its definition file and updating the maindynamixel.modellist. - Enhanced Joint State-Command Synchronization: Improved the logic for synchronizing joint commands with current states during the
startphase, making it more robust to different interface configurations. - Memory Management: Added explicit nullptr initialization and deletion checks in the
Dynamixeldestructor to ensure proper cleanup of allocated communication handlers.
Changelog
- CHANGELOG.rst
- Bump version to 1.4.5
- Deprecate ament_include_dependency usage in CMakeLists.txt
- Added proper command interface support with ROS2-Dynamixel interface mapping
- Improved error handling and robustness throughout the codebase
- Implemented per-device torque enable control (replacing global control)
- Added support for new sensor model (sensorxel_joy)
- Enhanced joint state-command synchronization
- Improved parameter initialization organization
- Added robust error handling for model file reading
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A version bumps,
Dependencies jump,
New features arrive,
Keeping code alive,
Ready for the next big lump.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the dynamixel_hardware_interface by introducing several key improvements and features, aligning with the changelog entries for versions 1.4.4 and 1.4.5.
Key improvements include:
- Enhanced Error Handling: Robustness has been improved with more
try-catchblocks, usage ofstd::runtime_errorfor critical file operations, and more descriptive error logging. - Per-Device Torque Control: Replaced global torque enable with a per-device mechanism, offering finer control.
- Fast Read Protocols: Introduced an optional fast read protocol (Fast Sync/Bulk Read) with a fallback mechanism to improve communication performance where supported by devices.
- Direct Read/Write for Bulk Operations: Added support for direct memory access in bulk read/write if indirect addressing is unavailable, increasing compatibility.
- Refactored Interface Mapping: The new
MapInterfacesfunction and standardized mapping tables (ros2_to_dxl_cmd_map, etc.) provide a cleaner way to handle command and state interface conversions. - Improved Parameter Initialization: The
initItemsfunction now initializes parameters in a specific order (Operating Mode, Limits, Others), which can be crucial for correct device setup. - CMake Modernization: Updated CMake to use
target_link_librariesoverament_target_dependencies. - Resource Management: The
Dynamixelclass destructor has been significantly improved to properly release SDK resources.
The changes are extensive and generally well-implemented. The introduction of fast read protocols and the fallback mechanism is a thoughtful approach to balancing performance and reliability. The error handling and resource management improvements are also very welcome.
I've identified one critical issue related to memory management that needs to be addressed. Other comments are of medium severity, focusing on potential improvements or clarifications.
Summary of Findings
- Memory Leak in Write Operations: Critical memory leaks were identified in
SetDxlValueToSyncWriteandSetDxlValueToBulkWritedue to missingdelete[]for dynamically allocatedparam_write_valuebuffers after they are used by the Dynamixel SDK'saddParammethods. - Torque Enable Retry Logic: The
start()method uses a fixed 10 retries for enabling torque. Making this configurable could enhance flexibility. - Fast Read Protocol Complexity: The new fast read protocols with fallback mechanisms are complex. Thorough testing across various conditions is recommended to ensure stability and optimal behavior.
- Joint State-Command Synchronization:
SyncJointCommandWithStatesnow specifically syncsHW_IF_POSITION. Confirmation that this covers all intended use cases for initial command synchronization would be beneficial.
Merge Readiness
This pull request introduces substantial and valuable improvements to the dynamixel_hardware_interface. The enhanced error handling, per-device torque control, and the new fast read/write capabilities are significant.
However, there are critical memory leaks in SetDxlValueToSyncWrite and SetDxlValueToBulkWrite that must be addressed before merging. I've provided specific comments on these.
Additionally, there are a few medium-severity points regarding the retry logic for torque enable and the complexity of the fast read protocols, which might benefit from further consideration or testing.
I am unable to approve pull requests directly. Once the critical memory leaks are fixed, I recommend further review and testing, especially for the new communication protocol logic, before this PR is merged.
No description provided.