Conversation
…ling - Modified data structures to use pairs of communication ID and device ID for torque states and enabled IDs. - Updated methods in Dynamixel and DynamixelInfo classes to accommodate the new communication ID parameter. - Enhanced model file reading functions to include communication ID for better organization and access. - Added new model files for hand controllers and fingers, including control tables and unit information for pressure sensors.
…ts and pressure sensors
…ronization tables
…ct device associations
… in Dynamixel interface
… and communication setup
…roper device initialization
…ace for faster initialization
… m' for consistency
…smission matrices in Dynamixel hardware interface
….0 for consistency across all models
…plementation files for improved readability
…ced functionality
…generator and XML parser scripts
… and XML parser for clarity and consistency
…del generator and XML parser scripts
…dentation in Dynamixel hardware interface and implementation files
…ation for improved readability
…for consistency and clarity
Major Update for Dynamixel Hardware Interface
Summary of ChangesHello @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 significantly upgrades the 'dynamixel_hardware_interface' by introducing a more sophisticated device identification scheme using 'comm_id' and 'id' pairs, enhancing the flexibility for managing multiple devices on a single bus. It also refactors the unit conversion logic into a unified system with offset support, improving data interpretation. The initialization process has been made more robust with sequential steps and better error handling. Furthermore, memory management for internal matrices has been modernized, and new Python tools have been added to streamline the generation of Dynamixel model files, expanding support for a wider range of devices and configurations. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant and valuable changes, including the comm_id/id concept for virtual devices, a unified unit conversion system, and sequential initialization logic. The refactoring to use std::vector for matrices is a great improvement that fixes a memory leak. The addition of scripts for model file generation is also very helpful. My review focuses on ensuring the new comm_id logic is applied consistently and that all functionalities, like device reset, are fully updated. I've found a few areas that need attention to ensure the robustness and correctness of the new features.
| DxlError WriteItem(uint8_t id, uint16_t addr, uint8_t size, uint32_t data); | ||
| DxlError WriteItem(uint8_t comm_id, uint8_t id, std::string item_name, uint32_t data); | ||
| DxlError WriteItem(uint8_t comm_id, uint8_t id, uint16_t addr, uint8_t size, uint32_t data); | ||
| DxlError InsertWriteItemBuf(uint8_t id, std::string item_name, uint32_t data); |
There was a problem hiding this comment.
The signature for InsertWriteItemBuf should include comm_id to properly support virtual devices, where comm_id can be different from id. The current implementation assumes comm_id == id, which is incorrect for virtual devices accessed via services.
DxlError InsertWriteItemBuf(uint8_t comm_id, uint8_t id, std::string item_name, uint32_t data);| // DXL Item Read | ||
| DxlError ReadItem(uint8_t id, std::string item_name, uint32_t & data); | ||
| DxlError ReadItem(uint8_t comm_id, uint8_t id, std::string item_name, uint32_t & data); | ||
| DxlError InsertReadItemBuf(uint8_t id, std::string item_name); |
There was a problem hiding this comment.
The signature for InsertReadItemBuf should include comm_id to properly support virtual devices. The current implementation assumes comm_id == id, which is incorrect for virtual devices accessed via services.
DxlError InsertReadItemBuf(uint8_t comm_id, uint8_t id, std::string item_name);| DxlError ReadItem(uint8_t comm_id, uint8_t id, std::string item_name, uint32_t & data); | ||
| DxlError InsertReadItemBuf(uint8_t id, std::string item_name); | ||
| DxlError ReadItemBuf(); | ||
| bool CheckReadItemBuf(uint8_t id, std::string item_name); |
There was a problem hiding this comment.
The signature for CheckReadItemBuf should include comm_id to correctly identify the device, especially when multiple devices might share the same id but have different comm_ids (e.g., virtual devices). The current implementation could lead to ambiguity.
bool CheckReadItemBuf(uint8_t comm_id, uint8_t id, std::string item_name);| DxlError InsertReadItemBuf(uint8_t id, std::string item_name); | ||
| DxlError ReadItemBuf(); | ||
| bool CheckReadItemBuf(uint8_t id, std::string item_name); | ||
| uint32_t GetReadItemDataBuf(uint8_t id, std::string item_name); |
There was a problem hiding this comment.
The signature for GetReadItemDataBuf should include comm_id to correctly identify the device and retrieve its data. The current implementation could return data from the wrong device if multiple devices share an id.
uint32_t GetReadItemDataBuf(uint8_t comm_id, uint8_t id, std::string item_name);| bool GetDxlControlItem( | ||
| uint8_t comm_id, uint8_t id, std::string item_name, uint16_t & addr, | ||
| uint8_t & size); | ||
| bool CheckDxlControlItem(uint8_t comm_id, uint8_t id, std::string item_name); |
There was a problem hiding this comment.
For improved performance and to adhere to C++ best practices, std::string parameters should be passed by const std::string& to avoid unnecessary copies. This applies to GetDxlControlItem, CheckDxlControlItem, and other methods in this class like GetDxlUnitValue, ConvertValueToUnit, etc.
bool GetDxlControlItem(
uint8_t comm_id, uint8_t id, const std::string & item_name, uint16_t & addr,
uint8_t & size);
bool CheckDxlControlItem(uint8_t comm_id, uint8_t id, const std::string & item_name);| [unit info] | ||
| Data Name value unit Sign Type | ||
| Data Name value unit Sign Type Offset | ||
| Present Velocity 0.0010471976 rad/s signed |
There was a problem hiding this comment.
This line is missing the Offset value, which is inconsistent with the new format and other entries in this file. It should have an offset of 0.0 to match the other velocity entries.
Present Velocity 0.0010471976 rad/s signed 0.0
2 participants
No description provided.