Update Torque Disable #28
Conversation
- Add omy_hat.model and omy_end.model to supported dynamixel models - Update dynamixel.model to include new OMY models (220 and 230) - Update gitignore to ensure model files are tracked
- Changed value_of_min_radian_position to -740 and min_radian to -1.099 in omy_end.model and rh_p12_rn.model for handling negative values
…erface in one ros2_control_node
…t control - Updated ReadMultiDxlData, GetDxlValueFromSyncRead, and GetDxlValueFromBulkRead to accept a period_ms parameter. - Introduced ProcessReadCommunication and ProcessReadData functions for better data handling and communication management. - Enhanced error handling and logging for read operations.
Signed-off-by: Wonho Yun <ywh@robotis.com>
![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
This pull request focuses on updating the torque disable functionality within the dynamixel hardware interface. It introduces a global torque enable parameter, adds controller type identification, and refactors the read data processing. Additionally, it includes new dynamixel model files and modifies the dynamixel communication process to include a timeout based on the period.
Highlights
- Global Torque Enable: A global torque enable parameter is introduced, allowing users to disable torque for all dynamixels. This is controlled by the
torque_enableparameter in the hardware configuration. - Controller Type Identification: The code now identifies controller type dynamixels, allowing for specific initialization and handling of these devices.
- Read Data Processing Refactor: The read data processing is refactored into separate functions for communication and data conversion, improving code readability and maintainability. The read functions now take a
period_msparameter. - Dynamixel Model Files: New dynamixel model files (
omy_hat.modelandomy_end.model) are added, and the.gitignorefile is updated to include*.modelfiles.
Changelog
Click here to see the changelog
- .gitignore
- Added a rule to ignore Dynamixel model files (
*.model).
- Added a rule to ignore Dynamixel model files (
- include/dynamixel_hardware_interface/dynamixel/dynamixel.hpp
- Added
<functional>include. - Modified
ReadMultiDxlData,GetDxlValueFromSyncRead, andGetDxlValueFromBulkReadfunctions to accept adouble period_msparameter. - Added
ProcessReadDataandProcessReadCommunicationfunctions for handling data processing and communication. - Added
ResetIndirectReadfunction.
- Added
- include/dynamixel_hardware_interface/dynamixel_hardware_interface.hpp
- Added
global_torque_enable_member variable. - Added
controller_id_andcontroller_item_member variables. - Added
initItems,retryWriteItem, andInitControllerItemshelper functions. - Added
is_set_hdl_member variable.
- Added
- param/dxl_model/dynamixel.model
- Added
omy_hat.modelandomy_end.modelto the list of available dynamixel models.
- Added
- param/dxl_model/omy_end.model
- Added a new dynamixel model file for
omy_end.
- Added a new dynamixel model file for
- param/dxl_model/omy_hat.model
- Added a new dynamixel model file for
omy_hat.
- Added a new dynamixel model file for
- param/dxl_model/rh_p12_rn.model
- Updated
value_of_min_radian_positionto -740 andmin_radianto -1.099.
- Updated
- src/dynamixel/dynamixel.cpp
- Added
<functional>include. - Modified
DynamixelDisableto returnDxlError resultinstead of exiting on first error. - Modified
ReadMultiDxlDatato callGetDxlValueFromSyncReadorGetDxlValueFromBulkReadwith theperiod_msparameter. - Modified
GetDxlValueFromSyncReadandGetDxlValueFromBulkReadto useProcessReadDataandProcessReadCommunication. - Modified
SetBulkReadItemAndHandlerto returnDxlError::BULK_READ_FAILinstead ofDxlError::SYNC_READ_FAIL. - Added
ProcessReadCommunicationfunction to handle communication for SyncRead and BulkRead. - Added
ProcessReadDatafunction to handle data processing for SyncRead and BulkRead. - Added
ResetIndirectReadfunction.
- Added
- src/dynamixel_hardware_interface.cpp
- Added parsing of
torque_enableparameter from hardware parameters. - Added handling for controller type in
on_init. - Modified
on_initto initialize controller items before dxl items. - Modified
startto enable torque only ifglobal_torque_enable_is true. - Modified
readto passperiod_mstoReadMultiDxlData. - Modified
CommResetto initialize controller items. - Added
retryWriteItemfunction to retry writing items. - Added
initItemsfunction to initialize items for a specific type. - Added
InitControllerItemsfunction. - Modified
InitDxlItemsto also initialize sensor items.
- Added parsing of
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.
In realms of code, where robots roam,
Torque's control finds a new home.
With bits and bytes, a gentle hand,
To guide the motors 'cross the land.
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
The pull request introduces several updates to the dynamixel_hardware_interface, including adding a global torque enable parameter, controller items, and refactoring read data processing. The changes seem well-structured and aim to improve the flexibility and maintainability of the interface. However, there are some areas that could benefit from further review and refinement.
Summary of Findings
- Error Handling in DynamixelDisable: The DynamixelDisable function in src/dynamixel/dynamixel.cpp accumulates errors but always returns the last error encountered. This could mask earlier errors. Consider returning a combined error status or logging all errors.
- Redundant Initialization of Communication: The on_init function in src/dynamixel_hardware_interface.cpp initializes the communication port separately for controller and DXL/sensor IDs. This could lead to redundant initialization and potential conflicts. Consider unifying the initialization process.
- Missing Error Handling in InitDxlItems: The InitDxlItems function in src/dynamixel_hardware_interface.cpp initializes Dynamixel items but lacks comprehensive error handling for write operations. Consider adding retry mechanisms or logging failures for individual item writes.
- Inconsistent use of RCLCPP_INFO_STREAM: There are several instances where RCLCPP_INFO_STREAM is used for logging. Consider using RCLCPP_DEBUG_STREAM for less critical information to reduce verbosity in production.
Merge Readiness
The pull request introduces significant changes that require careful review. While the changes seem to improve the interface's functionality, the error handling and initialization processes need further attention. I recommend addressing the identified issues before merging. I am unable to directly approve this pull request, and users should have others review and approve this code before merging, particularly after the changes requested are implemented.
| if (WriteItem(it_id, "Torque Enable", TORQUE_OFF) < 0) { | ||
| fprintf(stderr, "[ID:%03d] Cannot write \"Torque Off\" command!\n", it_id); | ||
| return DxlError::ITEM_WRITE_FAIL; | ||
| result = DxlError::ITEM_WRITE_FAIL; |
There was a problem hiding this comment.
The function currently overwrites the result variable in each iteration, potentially masking earlier errors. It might be better to either return immediately on the first error or accumulate all errors and return a combined status. What if the first few motors failed to disable torque, but the last one succeeded? The error would be masked.
| if (WriteItem(it_id, "Torque Enable", TORQUE_OFF) < 0) { | |
| fprintf(stderr, "[ID:%03d] Cannot write \"Torque Off\" command!\n", it_id); | |
| return DxlError::ITEM_WRITE_FAIL; | |
| result = DxlError::ITEM_WRITE_FAIL; | |
| if (WriteItem(it_id, "Torque Enable", TORQUE_OFF) < 0) { | |
| fprintf(stderr, "[ID:%03d] Cannot write \"Torque Off\" command!\n", it_id); | |
| if (result == DxlError::OK) { | |
| result = DxlError::ITEM_WRITE_FAIL; // Only set if no previous error | |
| } | |
| } |
| if (controller_id_.size() > 0) { | ||
| while (trying_connect) { | ||
| std::vector<uint8_t> id_arr; | ||
| for (auto controller : controller_id_) { | ||
| id_arr.push_back(controller); | ||
| } | ||
| if (dxl_comm_->InitDxlComm(id_arr, port_name_, baud_rate_) == DxlError::OK) { | ||
| RCLCPP_INFO_STREAM(logger_, "Trying to connect to the communication port..."); | ||
| trying_connect = false; | ||
| } else { | ||
| sleep(1); | ||
| cnt++; | ||
| if (cnt > trying_cnt) { | ||
| RCLCPP_ERROR_STREAM(logger_, "Cannot connect communication port! :("); | ||
| cnt = 0; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code initializes the communication port for controller IDs. It's duplicated later for DXL/sensor IDs. Consider refactoring this into a separate function to avoid redundancy and potential inconsistencies. Also, the trying_connect variable is not reset to true before the second while loop, which could cause issues if the first connection attempt fails.
| if (!retryWriteItem(id, it.first, static_cast<uint32_t>(stoi(it.second)))) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Consider adding a retry mechanism or logging a warning if the write operation fails. Without this, a single failed write can cause the entire initialization to fail, which is not ideal.
Signed-off-by: Wonho Yun <ywh@robotis.com>
Signed-off-by: Wonho Yun <ywh@robotis.com>
Signed-off-by: Wonho Yun <ywh@robotis.com>
Signed-off-by: Pyo <pyo@robotis.com>
Description
Update Torque Disable