Bump 1.4.7 #51

robotpilot · 2025-06-19T22:59:43Z

No description provided.

…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

…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.

…larity

- 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.

…lude file path

…r better readability and consistency

…ecessary operations

Add virtual_dxl feaure

gemini-code-assist

Summary of Changes

Hello @robotpilot, I'm Gemini Code Assist¹! 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 enhances the dynamixel_hardware_interface package by introducing support for new 'virtual' Dynamixel devices and improving the robustness of communication with existing hardware. It refines how Dynamixel model information, particularly velocity units, is managed and integrates more resilient retry mechanisms for read/write operations. The changes also include better torque state management during initialization and general code modernization, all contributing to a more reliable and extensible hardware interface.

Highlights

Virtual Dynamixel Support: This pull request introduces the concept of 'virtual Dynamixels' (virtual_dxl), allowing the system to interface with devices that emulate Dynamixel behavior but might have distinct communication IDs or model files. This includes the addition of new model files for the ffw_sg2_drive and ffw_sg2_steer series, expanding the range of supported hardware.
Enhanced Communication Robustness: To improve reliability, retry mechanisms with a configurable MAX_COMM_RETRIES (default 5) have been implemented for individual read and write operations to Dynamixels. This helps in environments with potentially unstable communication. Additionally, a new reboot mechanism is triggered upon detection of hardware errors.
Improved Dynamixel Model Handling: The system now incorporates a velocity_unit parameter in Dynamixel model files, which is utilized by the DynamixelInfo class for more accurate velocity conversions. The code also provides default values and issues warnings if torque_constant or velocity_unit parameters are missing from model files, ensuring consistent behavior.
Refactored Item Initialization and Data Processing: The internal handling of read and write items has been refined. Multiple control items can now be efficiently grouped under a single communication ID (comm_id) for bulk operations. The ProcessReadData function has been updated to correctly process data from multiple Dynamixel IDs associated with a single communication ID.
Torque State Management: A new InitTorqueStates function has been added to check the current torque status of Dynamixels during initialization. This function can optionally disable torque if it's found to be enabled, preventing potential issues and providing more controlled startup behavior.
Code Modernization and Cleanup: The codebase has been updated by replacing usleep with std::this_thread::sleep_for for improved cross-platform compatibility and readability. The retryWriteItem helper function has been removed, with its retry logic now integrated directly into the WriteItem and ReadItem methods for a more streamlined approach.

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.

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. ↩

gemini-code-assist

Code Review

This pull request introduces version 1.4.7, primarily adding support for 'virtual_dxl' and 'rcu', which involves a new comm_id concept to differentiate physical from logical Dynamixel IDs. It also refactors communication retry logic, replaces usleep with std::this_thread::sleep_for, and updates numerous Dynamixel model files. Key areas for review include potential memory leaks, const-correctness, and handling of commented-out code.

gemini-code-assist · 2025-06-19T23:02:14Z

src/dynamixel/dynamixel.cpp

-    uint8_t ID = it_write_data.id;
-    uint8_t * param_write_value = new uint8_t[indirect_info_write_[ID].size];
+    uint8_t comm_id = it_write_data.comm_id;
+    uint8_t * param_write_value = new uint8_t[indirect_info_write_[comm_id].size];


Memory leak: param_write_value is allocated with new uint8_t[] but not deallocated with delete[]. Since group_sync_write_->addParam() likely copies the data, param_write_value should be deleted after its use to prevent a memory leak.

Suggested change

uint8_t * param_write_value = new uint8_t[indirect_info_write_[comm_id].size];

uint8_t * param_write_value = new uint8_t[indirect_info_write_[comm_id].size];

// ... (existing code to populate param_write_value) ...

if (group_sync_write_->addParam(comm_id, param_write_value) != true) {

delete[] param_write_value; // Deallocate before returning on error

fprintf(stderr, "[ID:%03d] groupSyncWrite addparam failed\n", comm_id);

return DxlError::SYNC_WRITE_FAIL;

}

delete[] param_write_value; // Deallocate after successful use

gemini-code-assist · 2025-06-19T23:02:14Z

src/dynamixel/dynamixel.cpp

-    if (direct_info_write_.find(ID) != direct_info_write_.end()) {
-      param_write_value = new uint8_t[direct_info_write_[ID].size];
+    if (direct_info_write_.find(comm_id) != direct_info_write_.end()) {
+      param_write_value = new uint8_t[direct_info_write_[comm_id].size];


Memory leak: param_write_value is allocated with new uint8_t[] for direct writes but not deallocated. It should be delete[]-ed after group_bulk_write_->addParam().

Suggested change

param_write_value = new uint8_t[direct_info_write_[comm_id].size];

param_write_value = new uint8_t[direct_info_write_[comm_id].size];

// ... (existing code to populate param_write_value for direct write) ...

if (group_bulk_write_->addParam(

comm_id,

direct_info_write_[comm_id].indirect_data_addr,

direct_info_write_[comm_id].size,

param_write_value) != true)

{

delete[] param_write_value; // Deallocate before returning on error

fprintf(stderr, "[ID:%03d] groupBulkWrite addparam failed\n", comm_id);

return DxlError::BULK_WRITE_FAIL;

}

delete[] param_write_value; // Deallocate after successful use

gemini-code-assist · 2025-06-19T23:02:14Z

src/dynamixel/dynamixel.cpp

    } else {
      // Handle indirect write
-      param_write_value = new uint8_t[indirect_info_write_[ID].size];
+      param_write_value = new uint8_t[indirect_info_write_[comm_id].size];


Memory leak: param_write_value is allocated with new uint8_t[] for indirect writes but not deallocated. It should be delete[]-ed after group_bulk_write_->addParam().

Suggested change

param_write_value = new uint8_t[indirect_info_write_[comm_id].size];

param_write_value = new uint8_t[indirect_info_write_[comm_id].size];

// ... (existing code to populate param_write_value for indirect write) ...

if (group_bulk_write_->addParam(

comm_id,

indirect_info_write_[comm_id].indirect_data_addr,

indirect_info_write_[comm_id].size,

param_write_value) != true)

{

delete[] param_write_value; // Deallocate before returning on error

fprintf(stderr, "[ID:%03d] groupBulkWrite addparam failed\n", comm_id);

return DxlError::BULK_WRITE_FAIL;

}

delete[] param_write_value; // Deallocate after successful use

gemini-code-assist · 2025-06-19T23:02:14Z

include/dynamixel_hardware_interface/dynamixel/dynamixel.hpp


  // Set Dxl Option
-  DxlError SetOperatingMode(uint8_t id, uint8_t dynamixel_mode);
+  // DxlError SetOperatingMode(uint8_t id, uint8_t dynamixel_mode);


The declaration for SetOperatingMode is commented out. If this function is no longer needed, it's better to remove the declaration entirely to improve code clarity and reduce dead code. If it's temporarily disabled, a // TODO: ... comment explaining why and when it might be reinstated would be helpful.

gemini-code-assist · 2025-06-19T23:02:15Z

include/dynamixel_hardware_interface/dynamixel/dynamixel_info.hpp

+  // bool GetDxlTypeInfo(
+  //   uint8_t id,
+  //   int32_t & value_of_zero_radian_position,
+  //   int32_t & value_of_max_radian_position,
+  //   int32_t & value_of_min_radian_position,
+  //   double & min_radian,
+  //   double & max_radian,
+  //   double & torque_constant,
+  //   double & velocity_unit);


The declaration for GetDxlTypeInfo is commented out. Similar to SetOperatingMode, if this function is deprecated, please remove its declaration. If it's planned for future use or is temporarily disabled, a // TODO: ... comment would clarify its status.

gemini-code-assist · 2025-06-19T23:02:15Z

include/dynamixel_hardware_interface/dynamixel_hardware_interface.hpp

    size_t inner_size,
    std::vector<HandlerVarType> & outer_handlers,
-    const std::vector<HandlerVarType> & inner_handlers,
+    std::vector<HandlerVarType> & inner_handlers,


The inner_handlers parameter in MapInterfaces was changed from const std::vector<HandlerVarType>& to std::vector<HandlerVarType>&. If this function does not modify inner_handlers (it appears to only read from it), it should be passed by const& to maintain const-correctness and prevent accidental modifications.

Suggested change

std::vector<HandlerVarType> & inner_handlers,

const std::vector<HandlerVarType> & inner_handlers,

gemini-code-assist · 2025-06-19T23:02:15Z

src/dynamixel/dynamixel.cpp

+// DxlError Dynamixel::SetOperatingMode(uint8_t dxl_id, uint8_t dynamixel_mode)
+// {
+//   if (WriteItem(dxl_id, "Operating Mode", dynamixel_mode) == false) {
+//     return DxlError::ITEM_WRITE_FAIL;
+//   }
+
+//   fprintf(stderr, "[ID:%03d] Succeeded to set operating mode(", dxl_id);
+//   if (dynamixel_mode == DXL_POSITION_CTRL_MODE) {
+//     fprintf(stderr, "Position Control Mode)\n");
+//   } else if (dynamixel_mode == DXL_CURRENT_CTRL_MODE) {
+//     fprintf(stderr, "Current Control Mode)\n");
+//   } else if (dynamixel_mode == DXL_VELOCITY_CTRL_MODE) {
+//     fprintf(stderr, "Velocity Control Mode)\n");
+//   } else {
+//     fprintf(stderr, "Not Defined Control Mode)\n");
+//   }
+
+//   return DxlError::OK;
+// }


The implementation of SetOperatingMode is commented out. If this functionality is no longer required, consider removing the commented block to keep the codebase clean. If it's temporarily disabled, a // TODO: ... comment explaining the situation would be beneficial.

gemini-code-assist · 2025-06-19T23:02:15Z

src/dynamixel/dynamixel.cpp

+  fprintf(stderr, "MAX_COMM_RETRIES should be set to 1 or more\n");
+  return DxlError::ITEM_WRITE_FAIL;


This fprintf message and the subsequent return DxlError::ITEM_WRITE_FAIL; will only be reached if MAX_COMM_RETRIES is 0 or negative. Since MAX_COMM_RETRIES is defined as 5, this code block appears to be unreachable (dead code). Consider removing it or ensuring MAX_COMM_RETRIES is validated at compile-time (e.g., with static_assert if appropriate for its definition context) or during initialization if it were a configurable variable.

gemini-code-assist · 2025-06-19T23:02:15Z

src/dynamixel/dynamixel.cpp

+  fprintf(stderr, "MAX_COMM_RETRIES should be set to 1 or more\n");
+  return DxlError::ITEM_READ_FAIL;


Similar to the WriteItem function, this fprintf message and return path seem unreachable given MAX_COMM_RETRIES is 5. This appears to be dead code and could be removed.

gemini-code-assist · 2025-06-19T23:02:15Z

src/dynamixel/dynamixel_info.cpp

+// bool DynamixelInfo::GetDxlTypeInfo(
+//   uint8_t id,
+//   int32_t & value_of_zero_radian_position,
+//   int32_t & value_of_max_radian_position,
+//   int32_t & value_of_min_radian_position,
+//   double & min_radian,
+//   double & max_radian,
+//   double & torque_constant,
+//   double & velocity_unit)
+// {
+//   value_of_zero_radian_position = dxl_info_[id].value_of_zero_radian_position;
+//   value_of_max_radian_position = dxl_info_[id].value_of_max_radian_position;
+//   value_of_min_radian_position = dxl_info_[id].value_of_min_radian_position;
+//   min_radian = dxl_info_[id].min_radian;
+//   max_radian = dxl_info_[id].max_radian;
+//   torque_constant = dxl_info_[id].torque_constant;
+//   velocity_unit = dxl_info_[id].velocity_unit;
+//   return true;
+// }


The implementation of GetDxlTypeInfo is commented out. If this function is no longer needed, it's advisable to remove the commented block to maintain code cleanliness. If it's intended for future use or is temporarily disabled, a // TODO: ... comment would be appropriate.

Woojin-Crive and others added 21 commits June 5, 2025 10:22

feat: Add GPIO controller state handling and rcu model for ffw

ba695c9

refactor: Update Dynamixel interface for improved handler management …

28c35b6

…and model definitions

refactor: Improve formatting

3dae77e

refactor: Update position parameter in transmission calculation for c…

bb268c6

…larity

refactor: Remove unused retryWriteItem function from Dynamixel interface

ed1d1ee

refactor: Remove duplicate entries for Indirect Address in model file

6aebda4

refactor: Clarify documentation for Dynamixel motor ID array

d157ee4

feat: Add velocity unit handling to Dynamixel model processing

d88ff3b

feat: Add velocity unit parameter to multiple Dynamixel model files

6f696f4

refactor: Improve warning message formatting

00f7d86

refactor: Optimize early return checks in Dynamixel data handling

4392276

- Added early return checks in ReadMultiDxlData and WriteMultiDxlData methods to handle empty data lists.

fix: Enhance warning messages in Dynamixel model file handling to inc…

bf0452f

…lude file path

refactor: Replace sleep functions with std::this_thread::sleep_for fo…

6e1ed52

…r better readability and consistency

fix: Add thread header for dynamixel.cpp

44d3f8f

fix: Handle empty read data list in Dynamixel read operations

7e20f4e

fix: Add check for empty ID array in Sync Read Handler to prevent unn…

f9d5233

…ecessary operations

chore: Update version to 1.4.7 and add new features

1d790f7

Merge pull request #47 from ROBOTIS-GIT/feature-rcu

4be96ec

Add virtual_dxl feaure

robotpilot self-assigned this Jun 19, 2025

robotpilot added the bump Increase the version number to release label Jun 19, 2025

robotpilot added this to Platform Jun 19, 2025

github-project-automation bot moved this to 🌱 Todo in Platform Jun 19, 2025

robotpilot moved this from 🌱 Todo to 📝 Pull Request in Platform Jun 19, 2025

gemini-code-assist bot reviewed Jun 19, 2025

View reviewed changes

robotpilot merged commit 93a6e98 into jazzy Jun 19, 2025
25 checks passed

github-project-automation bot moved this from 📝 Pull Request to 🚩Done in Platform Jun 19, 2025

-    uint8_t * param_write_value = new uint8_t[indirect_info_write_[comm_id].size];
+    uint8_t * param_write_value = new uint8_t[indirect_info_write_[comm_id].size];
+    // ... (existing code to populate param_write_value) ...
+    if (group_sync_write_->addParam(comm_id, param_write_value) != true) {
+      delete[] param_write_value; // Deallocate before returning on error
+      fprintf(stderr, "[ID:%03d] groupSyncWrite addparam failed\n", comm_id);
+      return DxlError::SYNC_WRITE_FAIL;
+    }
+    delete[] param_write_value; // Deallocate after successful use

	std::vector<HandlerVarType> & inner_handlers,
	const std::vector<HandlerVarType> & inner_handlers,

		fprintf(stderr, "MAX_COMM_RETRIES should be set to 1 or more\n");
		return DxlError::ITEM_WRITE_FAIL;

		fprintf(stderr, "MAX_COMM_RETRIES should be set to 1 or more\n");
		return DxlError::ITEM_READ_FAIL;

Bump 1.4.7 #51

Bump 1.4.7 #51

Uh oh!

Conversation

robotpilot commented Jun 19, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants