Add support for sync and bulk read/write instructions in Slave class #149
Conversation
Woojin-Crive
commented
Jun 4, 2025
Woojin-Crive
commented
- Implemented processInstSyncWrite, processInstBulkWrite, processInstSyncRead, processInstBulkRead, processInstFastSyncRead, and processInstFastBulkRead methods.
- Updated processInst to handle new instruction types.
- Implemented processInstSyncWrite, processInstBulkWrite, processInstSyncRead, processInstBulkRead, processInstFastSyncRead, and processInstFastBulkRead methods. - Updated processInst to handle new instruction types.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request.
This pull request introduces support for several group communication instructions within the Slave class, specifically Sync Write, Bulk Write, Sync Read, Bulk Read, Fast Sync Read, and Fast Bulk Read. These instructions allow a master device to efficiently communicate with multiple control table items across potentially multiple slave devices (though the implementation here focuses on the slave processing its own relevant data within a broadcast packet). The changes involve adding new methods to handle the logic for each instruction type and integrating these handlers into the main instruction processing function.
Highlights
- Sync Write Implementation: Adds the
processInstSyncWritemethod to handle the Sync Write instruction, allowing multiple control table items on the slave to be written in a single packet. It supports both Protocol 1.0 and 2.0. - Bulk Write Implementation: Adds the
processInstBulkWritemethod to handle the Bulk Write instruction, which allows writing to different addresses and lengths for different IDs (though this slave only processes its own ID). This is specific to Protocol 2.0. - Sync Read Implementation: Adds the
processInstSyncReadmethod to handle the Sync Read instruction, allowing the master to request data from multiple control table items on the slave with a single packet. It supports both Protocol 1.0 and 2.0. - Bulk Read Implementation: Adds the
processInstBulkReadmethod to handle the Bulk Read instruction, allowing the master to request data from different addresses and lengths for different IDs (this slave responds for its own ID). This is specific to Protocol 2.0. - Fast Sync Read Implementation: Adds the
processInstFastSyncReadmethod, a Protocol 2.0 specific variant of Sync Read designed for faster response. - Fast Bulk Read Implementation: Adds the
processInstFastBulkReadmethod, a Protocol 2.0 specific variant of Bulk Read designed for faster response. - Instruction Dispatch Update: Updates the
processInstmethod to include cases for the newly supported Sync, Bulk, and Fast Read/Write instructions, directing the processing to the appropriate new handler methods.
Changelog
- src/utility/slave.cpp
- Added
processInstSyncWritemethod to handle Sync Write instructions (lines 634-781). - Added
processInstBulkWritemethod to handle Bulk Write instructions (lines 784-908). - Updated
processInstto dispatch Sync/Bulk/Fast Read/Write instructions to new handlers (lines 942-964). - Added
processInstSyncReadmethod to handle Sync Read instructions (lines 974-1072). - Added
processInstBulkReadmethod to handle Bulk Read instructions (lines 1150-1230). - Added
processInstFastSyncReadmethod to handle Fast Sync Read instructions (lines 1234-1308). - Added
processInstFastBulkReadmethod to handle Fast Bulk Read instructions (lines 1312-1392).
- Added
- src/utility/slave.h
- Added declarations for
processInstSyncWrite,processInstBulkWrite,processInstSyncRead,processInstBulkRead,processInstFastSyncRead, andprocessInstFastBulkReadmethods (lines 128-133).
- Added declarations for
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.
Packet flies so fast,
Sync and Bulk, data flows,
Slave hears, then responds.
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
This pull request introduces support for several important DYNAMIXEL 2.0 instructions (Sync/Bulk Read/Write and their Fast variants) to the Slave class. This is a valuable addition.
The review identified a few key areas for improvement:
- Parameter Length Validation: The checks for
recv_param_lenin several new functions need to be more precise to correctly handle instruction packet parameters. - Redundancy: There's significant code duplication between
processInstSyncReadandprocessInstFastSyncRead, and betweenprocessInstBulkReadandprocessInstFastBulkRead. This should be refactored. - SyncRead Implementation: The current
processInstSyncReadactually implements "Fast Sync Read" logic. A proper "Sync Read" that parses a list of target IDs from the instruction packet seems to be missing. - Minor Issues: Inconsistent protocol version comparison (e.g.,
!= 2.0instead of!= 2) and a persistent typo in callback argument names (user_write_callbakc_arg_,user_read_callbakc_arg_) used in the new code.
Addressing these points will enhance the correctness and maintainability of these new features.
Summary of Findings
- Incorrect Packet Parameter Length Checks: Several new instruction handlers (
processInstSyncWrite,processInstSyncRead,processInstFastSyncRead) use incorrect or overly lenient checks forp_rx_info->recv_param_len. This could lead to misinterpreting packet structures or errors when processing malformed packets. These checks need to be precise based on the DYNAMIXEL protocol specification for each instruction type and protocol version. - Redundant Function Implementations:
processInstFastSyncReadis nearly identical toprocessInstSyncRead. Similarly,processInstFastBulkReadis nearly identical toprocessInstBulkRead. This redundancy increases maintenance overhead and potential for inconsistencies. These should be refactored, possibly by having the 'Fast' versions call the regular versions if the core logic is the same, or by extracting common helper functions. - Misimplementation/Naming of
processInstSyncRead: The currentprocessInstSyncReadfunction implements logic suitable for a "Fast Sync Read" instruction (it parses only a common start address and data length). A standard "Sync Read" instruction requires parsing a list of target DYNAMIXEL IDs from the packet parameters, which is not done. This means a true Sync Read capability is missing, and there's a naming conflict with the actual behavior. - Protocol Version Comparison Style: In
processInstBulkWrite,processInstFastSyncRead, andprocessInstFastBulkRead,p_rx_info->protocol_ver(which is auint8_t) is compared against2.0(a float). While this might work due to implicit conversion, it's clearer and safer to compare with the integer2. - Misplaced Buffer Capacity Checks: In
processInstSyncWriteandprocessInstSyncRead(and its Fast variant), there are checks likedata_length+11 > packet_buf_capacity_. For SyncWrite, which doesn't send a status packet, this check is irrelevant. For Read operations, while related to status packet size, thetxStatusPacketfunction already performs robust buffer checks, making these preliminary checks potentially redundant. - Callback Argument Variable Typo: There's a consistent typo in user callback argument variable names (e.g.,
user_write_callbakc_arg_instead ofuser_write_callback_arg_). This typo is pre-existing inslave.hbut is used by the new functions. It should be corrected throughout the codebase for consistency.
Merge Readiness
The pull request introduces valuable new functionalities. However, due to the identified high severity issues related to packet processing correctness, redundancy, and misimplementation of processInstSyncRead, I recommend that these changes be addressed before merging. Addressing these concerns will significantly improve the reliability and maintainability of the new features. I am unable to approve the pull request directly; please ensure these changes are reviewed and approved by other maintainers after revision.
| Slave::processInstFastSyncRead() | ||
| { | ||
| bool ret = false; | ||
| DXLLibErrorCode_t err = DXL_LIB_OK; | ||
| InfoToParseDXLPacket_t *p_rx_info; | ||
| uint8_t *p_rx_param; | ||
| uint16_t addr, data_length = 0; | ||
| uint8_t packet_err = 0; | ||
|
|
||
| p_rx_info = &info_rx_packet_; | ||
| p_rx_param = p_rx_info->p_param_buf; | ||
|
|
||
| if(p_rx_info->id != DXL_BROADCAST_ID){ | ||
| last_lib_err_ = DXL_LIB_ERROR_NOT_SUPPORT_BROADCAST; | ||
| return false; | ||
| } | ||
|
|
||
| if(p_rx_info->protocol_ver != 2.0){ | ||
| last_lib_err_ = DXL_LIB_ERROR_NOT_SUPPORTED; | ||
| return false; | ||
| } | ||
|
|
||
| // extract start address and length from the instruction packet | ||
| if(p_rx_info->recv_param_len <= 2) { //2 = Address(2)+Length(2) | ||
| err = DXL_LIB_ERROR_WRONG_PACKET; | ||
| } | ||
| else { | ||
| addr = ((uint16_t)p_rx_param[1]<<8) | (uint16_t)p_rx_param[0]; | ||
| data_length = ((uint16_t)p_rx_param[3]<<8) | (uint16_t)p_rx_param[2]; | ||
|
|
||
| if(data_length+11 > packet_buf_capacity_){ | ||
| err = DXL_LIB_ERROR_NOT_ENOUGH_BUFFER_SIZE; | ||
| } | ||
| } | ||
|
|
||
| if(err == DXL_LIB_OK){ | ||
| uint8_t i, j; | ||
| uint16_t item_start_addr, item_addr_length; | ||
| ControlItem_t *p_item; | ||
| uint8_t *p_tx_param; | ||
|
|
||
| // Prepare response data | ||
| p_tx_param = &p_packet_buf_[9 + DXL_BYTE_STUFF_SAFE_CNT]; | ||
| memset(p_packet_buf_, 0, packet_buf_capacity_); | ||
|
|
||
| // Process each registered item | ||
| for(i=0; i < registered_item_cnt_; i++){ | ||
| p_item = &control_table_[i]; | ||
| item_start_addr = p_item->start_addr; | ||
| item_addr_length = p_item->length; | ||
|
|
||
| if(item_addr_length != 0 && p_item->p_data != nullptr | ||
| && isAddrInRange(item_start_addr, item_addr_length, addr, data_length) == true){ | ||
|
|
||
| if(user_read_callback_ != nullptr){ | ||
| user_read_callback_(item_start_addr, packet_err, user_read_callbakc_arg_); | ||
| if(packet_err != 0){ | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| for(j=0; j<item_addr_length; j++){ | ||
| p_tx_param[item_start_addr-addr+j] = p_item->p_data[j]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if(packet_err == 0){ | ||
| ret = txStatusPacket(id_, 0, p_tx_param, data_length); | ||
| } | ||
| } | ||
|
|
||
| last_lib_err_ = err; | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
This function processInstFastSyncRead is almost identical to processInstSyncRead (lines 975-1073). This introduces redundancy.
Given that processInstSyncRead already implements Fast Sync Read logic (it doesn't parse an ID list), one of these functions is likely redundant or misnamed.
Consider removing this function and having DXL_INST_FAST_SYNC_READ in processInst call the (potentially renamed) processInstSyncRead, or refactor common logic into a helper.
| if(p_rx_info->recv_param_len <= 2){ //2 = Address(1)+Length(1) | ||
| err = DXL_LIB_ERROR_WRONG_PACKET; | ||
| } |
There was a problem hiding this comment.
For Sync Read (or Fast Sync Read) Protocol 1, the parameters are Start Address(1) | Data Length(1). Thus, p_rx_info->recv_param_len should be exactly 2.
The current check p_rx_info->recv_param_len <= 2 is incorrect.
Should this be if(p_rx_info->recv_param_len != 2)?
| if(p_rx_info->recv_param_len <= 2){ //2 = Address(1)+Length(1) | |
| err = DXL_LIB_ERROR_WRONG_PACKET; | |
| } | |
| if(p_rx_info->recv_param_len != 2){ //2 = Address(1)+Length(1) |
| if(p_rx_info->recv_param_len <= 2){ //2 = Address(1)+Length(1)+Data(n) | ||
| err = DXL_LIB_ERROR_WRONG_PACKET; | ||
| } |
There was a problem hiding this comment.
Similar to the Protocol 2 check, p_rx_info->recv_param_len <= 2 seems incorrect for Protocol 1 Sync Write. Parameters are Start Address(1) | Data Length(1) | ID1(1) | Data1(L) | ....
p_rx_info->recv_param_len must be at least 2 bytes for Start Address(1) and Data Length(1).
Should this be p_rx_info->recv_param_len < 2?
| if(p_rx_info->recv_param_len <= 2){ //2 = Address(1)+Length(1)+Data(n) | |
| err = DXL_LIB_ERROR_WRONG_PACKET; | |
| } | |
| if(p_rx_info->recv_param_len < 2){ //2 = Address(1)+Length(1)+Data(n) |
| Slave::processInstFastBulkRead() | ||
| { | ||
| bool ret = false; | ||
| DXLLibErrorCode_t err = DXL_LIB_OK; | ||
| InfoToParseDXLPacket_t *p_rx_info; | ||
| uint8_t *p_rx_param; | ||
| uint8_t packet_err = 0; | ||
|
|
||
| p_rx_info = &info_rx_packet_; | ||
| p_rx_param = p_rx_info->p_param_buf; | ||
|
|
||
| if(p_rx_info->id != DXL_BROADCAST_ID){ | ||
| last_lib_err_ = DXL_LIB_ERROR_NOT_SUPPORT_BROADCAST; | ||
| return false; | ||
| } | ||
|
|
||
| if(p_rx_info->protocol_ver != 2.0){ | ||
| last_lib_err_ = DXL_LIB_ERROR_NOT_SUPPORTED; | ||
| return false; | ||
| } | ||
|
|
||
| // Process each ID's data in the fast bulk read packet | ||
| while(p_rx_info->recv_param_len > 0) { | ||
| if(p_rx_info->recv_param_len < 5) { // Need at least ID(1) + Address(2) + Length(2) | ||
| err = DXL_LIB_ERROR_WRONG_PACKET; | ||
| break; | ||
| } | ||
|
|
||
| uint8_t id = p_rx_param[0]; | ||
| uint16_t addr = ((uint16_t)p_rx_param[2]<<8) | (uint16_t)p_rx_param[1]; | ||
| uint16_t data_length = ((uint16_t)p_rx_param[4]<<8) | (uint16_t)p_rx_param[3]; | ||
|
|
||
| if(id == id_) { // Only process if this is our ID | ||
| uint8_t i, j; | ||
| uint16_t item_start_addr, item_addr_length; | ||
| ControlItem_t *p_item; | ||
| uint8_t *p_tx_param; | ||
|
|
||
| // Prepare response data | ||
| p_tx_param = &p_packet_buf_[9 + DXL_BYTE_STUFF_SAFE_CNT]; | ||
| memset(p_packet_buf_, 0, packet_buf_capacity_); | ||
|
|
||
| // Process each registered item | ||
| for(i=0; i < registered_item_cnt_; i++){ | ||
| p_item = &control_table_[i]; | ||
| item_start_addr = p_item->start_addr; | ||
| item_addr_length = p_item->length; | ||
|
|
||
| if(item_addr_length != 0 && p_item->p_data != nullptr | ||
| && isAddrInRange(item_start_addr, item_addr_length, addr, data_length) == true){ | ||
|
|
||
| if(user_read_callback_ != nullptr){ | ||
| user_read_callback_(item_start_addr, packet_err, user_read_callbakc_arg_); | ||
| if(packet_err != 0){ | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| for(j=0; j<item_addr_length; j++){ | ||
| p_tx_param[item_start_addr-addr+j] = p_item->p_data[j]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if(packet_err == 0){ | ||
| ret = txStatusPacket(id_, 0, p_tx_param, data_length); | ||
| } | ||
| } | ||
|
|
||
| // Move to next ID's data | ||
| p_rx_param += 5; // Skip ID(1) + Address(2) + Length(2) | ||
| p_rx_info->recv_param_len -= 5; | ||
| } | ||
|
|
||
| if(err == DXL_LIB_OK) { | ||
| ret = true; | ||
| } | ||
|
|
||
| last_lib_err_ = err; | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
This function processInstFastBulkRead is almost identical to processInstBulkRead (lines 1150-1231). The slave-side logic for processing a Fast Bulk Read request for its own ID is typically the same as for a Bulk Read request.
This introduces redundancy. Consider having DXL_INST_FAST_BULK_READ in processInst call processInstBulkRead, or refactor common logic.
| if(p_rx_info->recv_param_len <= 2) { //2 = Address(2)+Length(2) | ||
| err = DXL_LIB_ERROR_WRONG_PACKET; | ||
| } |
There was a problem hiding this comment.
For Fast Sync Read (Protocol 2), parameters are Start Address(2) | Data Length(2). p_rx_info->recv_param_len should be 4. The check p_rx_info->recv_param_len <= 2 is incorrect.
Should this be if(p_rx_info->recv_param_len != 4)?
| if(p_rx_info->recv_param_len <= 2) { //2 = Address(2)+Length(2) | |
| err = DXL_LIB_ERROR_WRONG_PACKET; | |
| } | |
| if(p_rx_info->recv_param_len != 4) { //2 = Address(2)+Length(2) |
| return false; | ||
| } | ||
|
|
||
| if(p_rx_info->protocol_ver != 2.0){ |
There was a problem hiding this comment.
The p_rx_info->protocol_ver is a uint8_t (1 or 2). Comparing it with a float 2.0 might lead to implicit conversions and is less clear than comparing with an integer 2.
Would p_rx_info->protocol_ver != 2 be more appropriate here?
| if(p_rx_info->protocol_ver != 2.0){ | |
| if(p_rx_info->protocol_ver != 2){ |
| return false; | ||
| } | ||
|
|
||
| if(p_rx_info->protocol_ver != 2.0){ |
There was a problem hiding this comment.
Protocol version comparison: p_rx_info->protocol_ver != 2 would be more appropriate than p_rx_info->protocol_ver != 2.0.
| if(p_rx_info->protocol_ver != 2.0){ | |
| if(p_rx_info->protocol_ver != 2){ |
|
|
||
| // Run user callback for Write instruction | ||
| if (packet_err == 0 && user_write_callback_ != nullptr){ | ||
| user_write_callback_(item_start_addr, packet_err, user_write_callbakc_arg_); |
There was a problem hiding this comment.
There's a consistent typo in the callback argument variable name: user_write_callbakc_arg_ (should be user_write_callback_arg_). This typo originates in slave.h (line 121) and is used here. While the definition is outside this PR's diff for .h, its usage in new code is noted. Consider correcting this across the codebase for consistency if possible, or at least be aware of it.
| if(data_length+6 > packet_buf_capacity_){ | ||
| err = DXL_LIB_ERROR_NOT_ENOUGH_BUFFER_SIZE; | ||
| } |
There was a problem hiding this comment.
Similar to the Protocol 2 case (lines 664-666), this check data_length+6 > packet_buf_capacity_ seems related to status packet buffer requirements. SyncWrite (Protocol 1) also does not send a status packet. Is this check needed here?
| return false; | ||
| } | ||
|
|
||
| if(p_rx_info->protocol_ver != 2.0){ |
There was a problem hiding this comment.
Protocol version comparison: p_rx_info->protocol_ver != 2 would be more appropriate than p_rx_info->protocol_ver != 2.0.
| if(p_rx_info->protocol_ver != 2.0){ | |
| if(p_rx_info->protocol_ver != 2){ |
