-
Notifications
You must be signed in to change notification settings - Fork 12
Add virtual_dxl feaure #47
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
Conversation
…virtual DXL support - Added `comm_id` to `HandlerVarType` for better communication management. - Updated methods to utilize `comm_id` for reading and writing operations. - Introduced handling for virtual DXL components in GPIO initialization and state management. - Improved error handling and logging for GPIO parameter checks. - Updated model files to include new virtual DXL models.
…and model definitions
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 @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 introduces a 'virtual_dxl' feature to the Dynamixel hardware interface, enabling a proxy communication pattern where a single device, identified by a 'communication ID', handles interactions for multiple component IDs. This is particularly aimed at supporting RCU-like devices. The changes involve updating data structures, modifying core communication logic to use the new communication ID, adding new device model definitions, and updating the hardware interface initialization and handling to accommodate the new device type.
Highlights
- Virtual Dynamixel Feature: Introduces the concept of 'virtual_dxl' devices, allowing a single controller (like an RCU) to act as a proxy for communication with multiple Dynamixels. This is facilitated by a new 'communication ID' (
comm_id) concept. - Communication ID Implementation: Adds a
comm_idfield to track which physical device (the proxy) is used for communication with a specific component ID. TheDynamixelclass now maps component IDs to their communication IDs. - New Device Model Files: Adds model definitions for new devices, including an RCU (
ffw_g10_rcu.model) and associated steer/drive units (ffw_sg2_steer_X.model,ffw_sg2_drive_X.model), enabling their use with the hardware interface. - Write Operation Retry Logic: Implements retry logic within the core
WriteItemmethod in theDynamixelclass to improve robustness against communication failures. - Hardware Interface Configuration Updates: Modifies the hardware interface initialization to recognize the new
virtual_dxltype and handle the associatedcomm_idandmodel_numparameters from the configuration.
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 configureGemini 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 introduces the virtual_dxl feature, enabling a communication ID to act as a proxy for one or more Dynamixel motors, primarily for the AI Worker RCU. The changes involve modifications to data structures to accommodate comm_id and id_arr, updates to read/write item setup and processing logic, and new model files for the RCU components.
Overall, the changes are well-integrated to support the new feature. Key improvements include enhanced error messaging and retry mechanisms for write operations.
There are a few areas that need attention:
- Clarification is needed for why
SetOperatingModeandretryWriteItemfunctions were commented out. - A potential data error in one of the new
.modelfiles (ffw_sg2_steer_3.model). - A review of
constcorrectness for a modified function parameter. - Ensuring actual DXL IDs are consistently used for value conversions, especially in bulk write operations.
Addressing these points will further improve the robustness and clarity of the implementation.
include/dynamixel_hardware_interface/dynamixel_hardware_interface.hpp
Outdated
Show resolved
Hide resolved
…y mechanism - Updated the maximum number of retries for communication operations from 10 to 5. - Added a new method `InitTorqueStates` to manage torque states during initialization. - Enhanced error logging for torque state management and communication issues. - Introduced a parameter to disable torque at initialization for improved safety.
yun-goon
left a comment
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.
Good!
- Changed goal_velocity type from int16_t to int32_t for better precision. - Replaced instances of ID with comm_id for improved readability and consistency across the codebase. - Added a check to return early if the write_data_list_ is empty in SetDxlValueToBulkWrite method.
- Added early return checks in ReadMultiDxlData and WriteMultiDxlData methods to handle empty data lists.
…r better readability and consistency
…ecessary operations
sunghowoo
left a comment
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.
LGTM!
robotpilot
left a comment
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.
LGTM
Adding communication ID concept for the AI Worker RCU.
virtual_dxl type gpio device uses the controller type device to act like a proxy tunnel for dynamixel communication.