-
Notifications
You must be signed in to change notification settings - Fork 473
Easy sdk python update and improvement of cpp easy sdk #669
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Signed-off-by: Hyungyu Kim <[email protected]>
Summary of ChangesHello @GyuH13, 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 significant updates and improvements to both the Python and C++ Easy SDKs for Dynamixel motors. The Python SDK has been completely refactored into a more modular and Pythonic design, including native exception handling and better management of control table data. Concurrently, the C++ SDK has undergone refactoring to streamline data read/write operations, consolidate data type definitions, and enhance error handling and status checking within group commands, ultimately providing a more robust and user-friendly experience across both platforms. 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
|
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.
Pull Request Overview
This pull request introduces a complete Python Easy SDK implementation and improves the C++ Easy SDK with cleaner abstractions and better error handling.
Key Changes:
- Complete Python Easy SDK with Motor, Connector, and GroupExecutor classes
- Control table files packaged for PyPI distribution
- Pythonic exception-based error handling
- C++ refactoring with internal
readData/writeDatahelper functions - Renamed error enum for clarity:
EASY_SDK_MOTOR_TORQUE_OFF→EASY_SDK_TORQUE_STATUS_MISMATCH - Added support for reading Current and PWM values
Reviewed Changes
Copilot reviewed 86 out of 86 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/src/dynamixel_easy_sdk/motor.py | Core Motor class with read/write operations and staged command support |
| python/src/dynamixel_easy_sdk/group_executor.py | Handles sync/bulk read/write operations with status validation |
| python/src/dynamixel_easy_sdk/dynamixel_error.py | Exception classes and error code definitions |
| python/src/dynamixel_easy_sdk/data_types.py | Enums and dataclasses for operating modes, commands, and requests |
| python/src/dynamixel_easy_sdk/control_table.py | Control table parser with caching |
| python/src/dynamixel_easy_sdk/connector.py | Factory class for creating motors and group executors |
| python/src/dynamixel_easy_sdk/init.py | Package exports |
| python/pyproject.toml | Package configuration for PyPI distribution |
| python/src/dynamixel_easy_sdk/control_table/*.model | Control table definition files for various motor models |
| c++/src/dynamixel_easy_sdk/motor.cpp | Refactored with helper functions and improved mode checking |
| c++/src/dynamixel_easy_sdk/group_executor.cpp | Added status request processing and signed value conversion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 a significant update with the new Easy SDK for Python and substantial improvements to the C++ Easy SDK. The Python SDK is well-structured, following modern Python practices with dataclasses, enums, and proper exception handling. The C++ SDK has been refactored to be more robust and maintainable, with the introduction of helper functions like writeData and readData that reduce code duplication significantly.
I've found a few critical issues that should be addressed. In the C++ code, there's a potential for a dangling pointer in StagedCommand which could lead to crashes, and a minor bug in error reporting. In the Python code, there's a critical issue with the package name in pyproject.toml which will cause conflicts on PyPI, and a bug in the closePort method.
Overall, these are great improvements that make the SDKs more powerful and easier to use. Addressing the identified issues will make them more robust and safer for developers.
…ue status mismatch
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.
Hi, this creates two same control table folders, how about combining them into one?
| if not self._port_handler.openPort(): | ||
| raise DxlRuntimeError('Failed to open the port') | ||
|
|
||
| if not self._port_handler.setBaudRate(baud_rate): | ||
| raise DxlRuntimeError('Failed to set baud rate') |
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.
Why don't you use 'DxlErrorCode' instead of transferring the string directly?
if not self._port_handler.openPort():
raise DxlRuntimeError(DxlErrorCode.SDK_COMM_PORT_BUSY)
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.
| results.append(intvalue) | ||
| return results | ||
|
|
||
| def _processStatusRequests(self, cmd: StagedCommand, data = None) -> None: |
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.
cmd.motor can be 'None', so wouldn't it be better to check it?
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.
At the beginning of the function, filtering is done based on this classification.
if not cmd.status_request:
return
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.
Why are we trying to manage the control table in two different places? Can't we just use the control table already provided here (https://github.com/ROBOTIS-GIT/DynamixelSDK/tree/main/control_table)?
If multiple locations handle the same control table, keeping it updated will become difficult.
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.
I also considered that point.
If we don't include control_table in the Python package, the control table cannot be included when distributing via PyPI.
For now, we'll manage it separately, but I'll look into a way to integrate it later.
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.
I also considered that point.
If we don't include it in the Python package, the control table cannot be included when distributing via PyPI.
For now, we'll manage it separately, but I'll look into a way to integrate it later.
|
|
||
| class Connector: | ||
|
|
||
| PROTOCOL_VERSION = 2.0 |
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.
Is there a reason why PROTOCOL_VERSION is managed inside the Connector class?
Is PROTOCOL_VERSION a variable that is only used within the Connector?
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.
Yes. it is only used in Connector.
| sorted_list.begin(), | ||
| sorted_list.end(), | ||
| [](const StagedCommand & a, const StagedCommand & b) { | ||
| return a.id < b.id; |
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.
Using single-letter variable names like a or b can make the code harder to understand.
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.
In the sort function, the lambda arguments can be named either a, b or cmd1, cmd2. I think there’s not much difference between the two. Do you think using cmd1, cmd2 would be better?
| sorted_list.begin(), | ||
| sorted_list.end(), | ||
| [](const StagedCommand & a, const StagedCommand & b) { | ||
| return a.id == b.id; |
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.
Using single-letter variable names like a or b can make the code harder to understand.
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.
Same answer with above.
…n in Motor and GroupExecutor classes
ola31
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
nhw-robotis
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!
Easy SDK Python is updated.
File added
connector.py:The connector class handles communication and acts as a factory that creates Motor and GroupExecutor objects.
control_table.py:The control table class is responsible for parsing the control table file.
data_types.py:This module includes all data type classes such as enums and data classes.
dynamixel_error.py:Defines custom exception types for Dynamixel (DXL).
group_executor.py:It manages sync and bulk commands and executes them.
motor.py:The Motor class represents a Dynamixel actuator and maps directly to a physical DXL device.
Feature
The control table file is now included in the package so that it is installed in the user’s environment when the package is distributed via PyPI (installed through pip).
All errors are now handled through Python-style exception handling for a more pythonic design.
CPP improvement
writeDataandreadDatadata_types.hppand removedstagedcommand.hppRenamed error enum
EASY_SDK_MOTOR_TORQUE_OFF→EASY_SDK_TORQUE_STATUS_MISMATCHUpdated group control logic to raise errors based on status conditions
getOperatingMode:uint8_t → OperatingMode enum value