Bump 1.4.10 #67
Bump 1.4.10 #67
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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Woojin-Crive, 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 primarily focuses on upgrading the dynamixel_hardware_interface package to version 1.4.10. The core changes involve introducing a flexible, data-driven unit conversion system for Dynamixel control items, which allows for more accurate and extensible handling of various physical quantities. Additionally, it significantly enhances the error reporting capabilities by providing detailed breakdowns of Dynamixel hardware errors and error codes, improving diagnostic insights. The pull request also includes several build system improvements to ensure better integration within the ROS 2 ecosystem.
Highlights
- Generic Unit System: Implemented a new data-driven unit conversion system for Dynamixel control items, replacing hardcoded conversions with definitions loaded from model files. This allows for flexible handling of various physical quantities (e.g., velocity, current).
- Enhanced Error Handling: Added comprehensive support for Dynamixel hardware error status bits and error codes (153), providing detailed logging and a new service to retrieve error summaries for improved diagnostics.
- Build System Improvements: Upgraded CMake configuration to properly export public include directories and library targets, enhancing integration with other ROS 2 packages.
- Model File Updates: Modified all Dynamixel model files to incorporate the new unit system definitions, removing deprecated
velocity_unitandtorque_constantparameters. - Torque Control Refinements: Improved the torque enable/disable logic to check for the existence of the 'Torque Enable' control item and streamlined the process using a dedicated list of torque-enabled IDs.
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.
Code Review
An excellent set of updates that significantly enhance the functionality and robustness of the hardware interface. The introduction of a generic unit system and detailed error handling are great additions. I've identified one critical issue regarding the logic for selecting communication protocols and a few medium-severity issues related to performance and error handling.
| 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)) | ||
| { | ||
| return BULK; |
There was a problem hiding this comment.
The logic to determine if SYNC read can be used has been oversimplified. It now only checks if the item_size is the same across different Dynamixels. This is incorrect because different control items can have the same size (e.g., 'Goal Position' and 'Goal Velocity' are both 4 bytes). For SYNC read to be correct, especially with indirect addressing, the list of items being read must be identical for all devices in the group. The check should include item_name to ensure the same logical items are being read. Without this check, the system might incorrectly use SYNC mode, leading to reading incorrect data. A similar issue exists in checkWriteType.
| for (size_t item_index = 0; item_index < write_data_list_.at(dxl_index).item_size.size(); | ||
| item_index++) | ||
| { | ||
| if (write_data_list_.at(dxl_index).item_name.at(item_index) != | ||
| write_data_list_.at(dxl_index - 1).item_name.at(item_index) || | ||
| write_data_list_.at(dxl_index).item_addr.at(item_index) != | ||
| write_data_list_.at(dxl_index - 1).item_addr.at(item_index) || | ||
| write_data_list_.at(dxl_index).item_size.at(item_index) != | ||
| if (write_data_list_.at(dxl_index).item_size.at(item_index) != | ||
| write_data_list_.at(dxl_index - 1).item_size.at(item_index)) | ||
| { | ||
| return BULK; |
There was a problem hiding this comment.
Similar to checkReadType, the logic to determine if SYNC write can be used has been oversimplified. It now only checks if the item_size is the same across different Dynamixels. This is incorrect because different control items can have the same size. For SYNC write to be correct, the list of items being written must be identical for all devices in the group. The check should include item_name to ensure the same logical items are being written. Without this check, the system might incorrectly use SYNC mode, leading to writing incorrect data to devices.
| double Dynamixel::ConvertValueWithUnitInfo( | ||
| uint8_t id, std::string item_name, uint32_t raw_value, | ||
| uint8_t size, bool is_signed) |
There was a problem hiding this comment.
The item_name parameter is passed by value, which can cause unnecessary string copies and impact performance, especially if this function is called frequently. It's better to pass it by const reference.
| double Dynamixel::ConvertValueWithUnitInfo( | |
| uint8_t id, std::string item_name, uint32_t raw_value, | |
| uint8_t size, bool is_signed) | |
| double Dynamixel::ConvertValueWithUnitInfo( | |
| uint8_t id, const std::string & item_name, uint32_t raw_value, | |
| uint8_t size, bool is_signed) |
| uint32_t Dynamixel::ConvertUnitValueToRawValue( | ||
| uint8_t id, std::string item_name, double unit_value, | ||
| uint8_t size, bool is_signed) |
There was a problem hiding this comment.
The item_name parameter is passed by value, which can cause unnecessary string copies and impact performance. It's better to pass it by const reference.
| uint32_t Dynamixel::ConvertUnitValueToRawValue( | |
| uint8_t id, std::string item_name, double unit_value, | |
| uint8_t size, bool is_signed) | |
| uint32_t Dynamixel::ConvertUnitValueToRawValue( | |
| uint8_t id, const std::string & item_name, double unit_value, | |
| uint8_t size, bool is_signed) |
| void Dynamixel::WriteValueToBuffer(uint8_t * buffer, uint8_t offset, uint32_t value, uint8_t size) | ||
| { | ||
| if (size == 1) { | ||
| buffer[offset] = static_cast<uint8_t>(value); | ||
| } else if (size == 2) { | ||
| uint16_t value_16 = static_cast<uint16_t>(value); | ||
| buffer[offset + 0] = DXL_LOBYTE(value_16); | ||
| buffer[offset + 1] = DXL_HIBYTE(value_16); | ||
| } else if (size == 4) { | ||
| buffer[offset + 0] = DXL_LOBYTE(DXL_LOWORD(value)); | ||
| buffer[offset + 1] = DXL_HIBYTE(DXL_LOWORD(value)); | ||
| buffer[offset + 2] = DXL_LOBYTE(DXL_HIWORD(value)); | ||
| buffer[offset + 3] = DXL_HIBYTE(DXL_HIWORD(value)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function handles data sizes of 1, 2, and 4 bytes. If an unsupported size is passed, the function does nothing and returns silently. This could hide bugs. It would be more robust to throw an exception or log an error for unhandled sizes, similar to how ConvertValueWithUnitInfo and ConvertUnitValueToRawValue handle it.
| void Dynamixel::WriteValueToBuffer(uint8_t * buffer, uint8_t offset, uint32_t value, uint8_t size) | |
| { | |
| if (size == 1) { | |
| buffer[offset] = static_cast<uint8_t>(value); | |
| } else if (size == 2) { | |
| uint16_t value_16 = static_cast<uint16_t>(value); | |
| buffer[offset + 0] = DXL_LOBYTE(value_16); | |
| buffer[offset + 1] = DXL_HIBYTE(value_16); | |
| } else if (size == 4) { | |
| buffer[offset + 0] = DXL_LOBYTE(DXL_LOWORD(value)); | |
| buffer[offset + 1] = DXL_HIBYTE(DXL_LOWORD(value)); | |
| buffer[offset + 2] = DXL_LOBYTE(DXL_HIWORD(value)); | |
| buffer[offset + 3] = DXL_HIBYTE(DXL_HIWORD(value)); | |
| } | |
| } | |
| void Dynamixel::WriteValueToBuffer(uint8_t * buffer, uint8_t offset, uint32_t value, uint8_t size) | |
| { | |
| if (size == 1) { | |
| buffer[offset] = static_cast<uint8_t>(value); | |
| } else if (size == 2) { | |
| uint16_t value_16 = static_cast<uint16_t>(value); | |
| buffer[offset + 0] = DXL_LOBYTE(value_16); | |
| buffer[offset + 1] = DXL_HIBYTE(value_16); | |
| } else if (size == 4) { | |
| buffer[offset + 0] = DXL_LOBYTE(DXL_LOWORD(value)); | |
| buffer[offset + 1] = DXL_HIBYTE(DXL_LOWORD(value)); | |
| buffer[offset + 2] = DXL_LOBYTE(DXL_HIWORD(value)); | |
| buffer[offset + 3] = DXL_HIBYTE(DXL_HIWORD(value)); | |
| } else { | |
| // Throw error for unknown sizes | |
| std::string error_msg = "Unknown data size " + std::to_string(size) + " for WriteValueToBuffer"; | |
| throw std::runtime_error(error_msg); | |
| } | |
| } |
| bool GetDxlUnitValue(uint8_t id, std::string data_name, double & unit_value); | ||
| bool GetDxlSignType(uint8_t id, std::string data_name, bool & is_signed); |
There was a problem hiding this comment.
The data_name parameter is passed by value, which can cause unnecessary string copies and impact performance. It's better to pass it by const reference.
| bool GetDxlUnitValue(uint8_t id, std::string data_name, double & unit_value); | |
| bool GetDxlSignType(uint8_t id, std::string data_name, bool & is_signed); | |
| 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<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); |
There was a problem hiding this comment.
The data_name parameter in these template functions is passed by value, which can cause unnecessary string copies and impact performance. It's better to pass it by const reference.
| 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); | |
| 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); |
| 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.
The data_name parameter is passed by value, which can cause unnecessary string copies and impact performance. It's better to pass it by const reference.
| double GetUnitMultiplier(uint8_t id, std::string data_name); | |
| double GetUnitMultiplier(uint8_t id, const std::string & data_name); |
|
|
||
| // Template implementations | ||
| template<typename T> | ||
| double DynamixelInfo::ConvertValueToUnit(uint8_t id, std::string data_name, T value) |
There was a problem hiding this comment.
The data_name parameter is passed by value, which can cause unnecessary string copies and impact performance. It's better to pass it by const reference.
| double DynamixelInfo::ConvertValueToUnit(uint8_t id, std::string data_name, T value) | |
| double DynamixelInfo::ConvertValueToUnit(uint8_t id, const std::string & data_name, T value) |
| } | ||
|
|
||
| template<typename T> | ||
| T DynamixelInfo::ConvertUnitToValue(uint8_t id, std::string data_name, double unit_value) |
There was a problem hiding this comment.
The data_name parameter is passed by value, which can cause unnecessary string copies and impact performance. It's better to pass it by const reference.
| T DynamixelInfo::ConvertUnitToValue(uint8_t id, std::string data_name, double unit_value) | |
| T DynamixelInfo::ConvertUnitToValue(uint8_t id, const std::string & data_name, double unit_value) |
No description provided.