- 
                Notifications
    
You must be signed in to change notification settings  - Fork 12
 
Bump 1.4.10 #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump 1.4.10 #68
Conversation
… conversions in Dynamixel class
…te model files for unit information
…e and model file processing
…ges in DynamixelHardware
…guage specification
…iple Dynamixel model files
…tency in Dynamixel hardware interface files
…into CMake configuration for improved build settings
…ce in CMake configuration
…and updating compiler settings
…el hardware interface and related files
…hecks for consistency
Added Dynamixel Y error code handling logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
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!
This pull request upgrades the dynamixel_hardware_interface package to version 1.4.10. The core changes revolve around introducing a more flexible and robust unit conversion system for Dynamixel data, significantly improving error reporting with detailed diagnostics, and enhancing the package's build and installation process for better integration within the ROS 2 ecosystem.
Highlights
- New Unit Conversion System: I've implemented a generic unit conversion system for Dynamixel control items. This replaces previous hardcoded conversions for position, velocity, and current, making the system more flexible and extensible. This is achieved by introducing a new 
[unit info]section in the Dynamixel model files, which defines conversion values and sign types for various data names. - Enhanced Dynamixel Error Handling: I've added comprehensive support for interpreting and reporting Dynamixel Y Error Codes (register 153) and detailed Hardware Error Status bits. This provides more granular and human-readable error diagnostics, including a new service to retrieve error summaries for individual Dynamixels or all connected devices.
 - Improved CMake Configuration: I've updated the 
CMakeLists.txtto properly export library targets and refine installation rules. This aligns the package better with ROS 2 best practices, making it easier for other projects to discover and link against this library. - Refactored Torque Control and Parameter Handling: I've streamlined the internal management of torque-enabled Dynamixels and updated the torque control service to be non-blocking, improving the system's responsiveness. Additionally, I've added more robust parameter validation during initialization and made several parameters optional with default values, enhancing the overall robustness and flexibility of the hardware interface.
 
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request bumps the package version to 1.4.10 and introduces significant improvements, including a new data-driven unit system for model files and enhanced error code handling and reporting. The CMake configuration has also been modernized.
The review has identified a few issues:
- Critical: The logic for determining SYNC vs. BULK operations in 
checkReadTypeandcheckWriteTypeis flawed and could lead to incorrect device communication. - High: There's a potential data overflow when calculating the size for bulk read/write operations, which could cause them to fail or behave unexpectedly.
 - Medium: Several new functions pass 
std::stringby value, which is inefficient and should be changed to pass byconstreference. 
These issues should be addressed to ensure the correctness and performance of the new features.
| if (read_data_list_.at(dxl_index).item_size.size() != | ||
| read_data_list_.at(dxl_index - 1).item_size.size()) | ||
| { | ||
| return BULK; | ||
| } | ||
| for (size_t item_index = 0; item_index < read_data_list_.at(dxl_index).item_name.size(); | ||
| for (size_t item_index = 0; item_index < read_data_list_.at(dxl_index).item_size.size(); | ||
| item_index++) | ||
| { | ||
| if (read_data_list_.at(dxl_index).item_name.at(item_index) != | ||
| read_data_list_.at(dxl_index - 1).item_name.at(item_index) || | ||
| read_data_list_.at(dxl_index).item_addr.at(item_index) != | ||
| read_data_list_.at(dxl_index - 1).item_addr.at(item_index) || | ||
| read_data_list_.at(dxl_index).item_size.at(item_index) != | ||
| if (read_data_list_.at(dxl_index).item_size.at(item_index) != | ||
| read_data_list_.at(dxl_index - 1).item_size.at(item_index)) | ||
| { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to determine whether to use SYNC or BULK read has been simplified to only check for matching item sizes. The previous implementation also checked for matching item_name and item_addr, which is crucial for correctness. Without these checks, devices with different control items but coincidentally matching item sizes could be incorrectly grouped for a SYNC operation, leading to reading from wrong addresses. This is a critical bug. Please restore the checks for item_addr and item_name.
| if (read_data_list_.at(dxl_index).item_size.size() != | |
| read_data_list_.at(dxl_index - 1).item_size.size()) | |
| { | |
| return BULK; | |
| } | |
| for (size_t item_index = 0; item_index < read_data_list_.at(dxl_index).item_name.size(); | |
| for (size_t item_index = 0; item_index < read_data_list_.at(dxl_index).item_size.size(); | |
| item_index++) | |
| { | |
| if (read_data_list_.at(dxl_index).item_name.at(item_index) != | |
| read_data_list_.at(dxl_index - 1).item_name.at(item_index) || | |
| read_data_list_.at(dxl_index).item_addr.at(item_index) != | |
| read_data_list_.at(dxl_index - 1).item_addr.at(item_index) || | |
| read_data_list_.at(dxl_index).item_size.at(item_index) != | |
| if (read_data_list_.at(dxl_index).item_size.at(item_index) != | |
| read_data_list_.at(dxl_index - 1).item_size.at(item_index)) | |
| { | |
| for (size_t item_index = 0; item_index < read_data_list_.at(dxl_index).item_size.size(); | |
| item_index++) | |
| { | |
| if (read_data_list_.at(dxl_index).item_size.at(item_index) != | |
| read_data_list_.at(dxl_index - 1).item_size.at(item_index)) | |
| { | |
| return BULK; | |
| } | |
| } | 
| bool GetDxlUnitValue(uint8_t id, std::string data_name, double & unit_value); | ||
| bool GetDxlSignType(uint8_t id, std::string data_name, bool & is_signed); | ||
| 
               | 
          ||
| // Template-based conversion methods | ||
| template<typename T> | ||
| double ConvertValueToUnit(uint8_t id, std::string data_name, T value); | ||
| 
               | 
          ||
| template<typename T> | ||
| T ConvertUnitToValue(uint8_t id, std::string data_name, double unit_value); | ||
| 
               | 
          ||
| // Helper method for internal use | ||
| double GetUnitMultiplier(uint8_t id, std::string data_name); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data_name parameter in GetDxlUnitValue, GetDxlSignType, ConvertValueToUnit, ConvertUnitToValue, and GetUnitMultiplier is passed by value. This causes an unnecessary copy of the string every time these functions are called, which can be inefficient, especially in performance-sensitive code. Please pass std::string by const reference instead.
| bool GetDxlUnitValue(uint8_t id, std::string data_name, double & unit_value); | |
| bool GetDxlSignType(uint8_t id, std::string data_name, bool & is_signed); | |
| // Template-based conversion methods | |
| template<typename T> | |
| double ConvertValueToUnit(uint8_t id, std::string data_name, T value); | |
| template<typename T> | |
| T ConvertUnitToValue(uint8_t id, std::string data_name, double unit_value); | |
| // Helper method for internal use | |
| double GetUnitMultiplier(uint8_t id, std::string data_name); | |
| bool GetDxlUnitValue(uint8_t id, const std::string &data_name, double & unit_value); | |
| bool GetDxlSignType(uint8_t id, const std::string &data_name, bool & is_signed); | |
| // Template-based conversion methods | |
| template<typename T> | |
| double ConvertValueToUnit(uint8_t id, const std::string &data_name, T value); | |
| template<typename T> | |
| T ConvertUnitToValue(uint8_t id, const std::string &data_name, double unit_value); | |
| // Helper method for internal use | |
| double GetUnitMultiplier(uint8_t id, const std::string &data_name); | 
| DxlError Dynamixel::ProcessDirectReadData( | ||
| uint8_t comm_id, | ||
| const std::vector<uint16_t> & item_addrs, | ||
| const std::vector<std::string> & item_names, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The item_names parameter is passed by value, which creates an unnecessary copy of the string. For performance, it's better to pass it by const reference. This also applies to ConvertUnitValueToRawValue.
| const std::vector<std::string> & item_names, | |
| DxlError Dynamixel::ProcessDirectReadData( | |
| uint8_t comm_id, | |
| const std::vector<uint16_t> & item_addrs, | |
| const std::vector<const std::string &> & item_names, | |
| const std::vector<uint8_t> & item_sizes, | 
No description provided.