Skip to content

Resolved the issue where the build was failing on SBC#391

Merged
robotpilot merged 4 commits intomainfrom
feature-humble-issue-handling
Mar 14, 2025
Merged

Resolved the issue where the build was failing on SBC#391
robotpilot merged 4 commits intomainfrom
feature-humble-issue-handling

Conversation

@yun-goon
Copy link
Member

@yun-goon yun-goon commented Mar 10, 2025

Description

Fixed the issue where the Workbench example was not building on SBC by adding the ARM option.

Testing & Validation

Verified that it builds successfully on the TurtleBot3 SBC without any issues.

Motivation

#301 #294

Copilot AI review requested due to automatic review settings March 10, 2025 08:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @yun-goon, 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

This pull request addresses issue #301, where the Workbench example was failing to build on SBC (Single Board Computers). The fix involves adding an ARM option to the CMakeLists.txt file to link against the correct library for SBC architectures. Additionally, there are minor formatting changes across multiple C++ example files, primarily adjusting spacing and line endings.

Highlights

  • Build Fix: The primary change is to enable building the dynamixel_workbench_toolbox examples on ARM-based SBCs by linking against libdxl_sbc_cpp.so when the system processor is ARM or AArch64.
  • CMakeLists.txt Modification: The CMakeLists.txt file in the dynamixel_workbench_toolbox/examples/ directory is updated to include a conditional library link based on the system processor architecture.
  • Minor Formatting Changes: Several C++ example files in dynamixel_workbench_toolbox/examples/src/ have been modified to adjust spacing and ensure consistent line endings.

Changelog

Click here to see the changelog
  • dynamixel_workbench_toolbox/examples/CMakeLists.txt
    • Added conditional logic to link libdxl_sbc_cpp.so when the system processor is ARM or AArch64.
    • Ensured that the monitor target links against dynamixel_workbench.
  • dynamixel_workbench_toolbox/examples/src/a_Model_Scan.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/b_Ping.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/c_ID_Change.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/d_BPS_Change.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/e_Mode_Change.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
  • dynamixel_workbench_toolbox/examples/src/f_Reboot.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/g_Reset.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/h_Position.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/i_Velocity.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/j_Current_Based_Position.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/k_Read_Write.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/l_Sync_Write.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/m_Sync_Read_Write.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/n_Bulk_Read_Write.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/o_Find_Dynamixel.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/examples/src/p_Monitor.cpp
    • Minor formatting changes to the main function declaration.
    • Adjusted spacing around the printf function call.
    • Ensured consistent line endings.
  • dynamixel_workbench_toolbox/include/dynamixel_workbench_toolbox/dynamixel_driver.h
    • Minor formatting changes to typedef struct and class DynamixelDriver.
  • dynamixel_workbench_toolbox/include/dynamixel_workbench_toolbox/dynamixel_item.h
    • Minor formatting changes to typedef struct.
  • dynamixel_workbench_toolbox/include/dynamixel_workbench_toolbox/dynamixel_tool.h
    • Minor formatting changes to class DynamixelTool.
  • dynamixel_workbench_toolbox/include/dynamixel_workbench_toolbox/dynamixel_workbench.h
    • Minor formatting changes to class DynamixelWorkbench.
  • dynamixel_workbench_toolbox/src/DynamixelWorkbench.h
    • Removed an extra newline at the end of the file.
  • dynamixel_workbench_toolbox/src/dynamixel_workbench_toolbox/dynamixel_driver.cpp
    • Minor formatting changes to class DynamixelDriver.
  • dynamixel_workbench_toolbox/src/dynamixel_workbench_toolbox/dynamixel_item.cpp
    • Minor formatting changes to static const ModelInfo and static const ControlItem.
  • dynamixel_workbench_toolbox/src/dynamixel_workbench_toolbox/dynamixel_tool.cpp
    • Minor formatting changes to typedef struct and class DynamixelTool.
  • dynamixel_workbench_toolbox/src/dynamixel_workbench_toolbox/dynamixel_workbench.cpp
    • Minor formatting changes to class DynamixelWorkbench.
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.


Trivia time!

What is the primary use case for CMake?

Click here for the answer
CMake is primarily used to control the software compilation process using simple platform and compiler-independent configuration files, and generate native makefiles and workspaces that can be used in the compiler environment of your choice.

Footnotes

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

@yun-goon yun-goon requested review from GyuH13 and robotpilot March 10, 2025 08:26
@yun-goon yun-goon self-assigned this Mar 10, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request addresses an issue where the Workbench example was failing to build on SBC by adding the ARM option. The changes look good overall, but I have a few suggestions to improve the code's maintainability and readability.

Summary of Findings

  • Conditional Compilation: The use of CMAKE_SYSTEM_PROCESSOR to detect ARM architectures is a good approach, but it might be beneficial to add a comment explaining why this is necessary and linking to the relevant issue.
  • Code Consistency: There are some inconsistencies in the use of whitespace and indentation across the changed files. While not critical, addressing these would improve the code's overall readability.
  • Missing newline at end of file: Many of the C++ files are missing a newline at the end of the file. This is a common convention, and adding it would improve consistency.

Merge Readiness

The pull request addresses a valid issue and the changes seem to resolve it. However, addressing the comments related to code consistency and adding a comment to explain the conditional compilation would improve the code's quality. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. I recommend that the author address the comments before merging.

Comment on lines +19 to +20
elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "arm" OR CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")
target_link_libraries(dynamixel_workbench LINK_PUBLIC "/usr/local/lib/libdxl_sbc_cpp.so")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a comment here to explain why this conditional compilation is necessary and linking to the issue. This will help future developers understand the context of this change.

elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "arm" OR CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64") # Added for SBC build (Issue #301)
  target_link_libraries(dynamixel_workbench LINK_PUBLIC "/usr/local/lib/libdxl_sbc_cpp.so")

@yun-goon yun-goon added the enhancement New feature or request label Mar 10, 2025
Copy link
Member

@robotpilot robotpilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yun-goon, please add the CI at least. The Lint can be added later.

@robotpilot robotpilot assigned robotpilot and unassigned yun-goon Mar 12, 2025
@github-project-automation github-project-automation bot moved this to 🌱 Todo in Platform Mar 12, 2025
@robotpilot robotpilot moved this from 🌱 Todo to 📝 Pull Request in Platform Mar 12, 2025
Copy link
Member

@GyuH13 GyuH13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. It seems that users' inconvenience will be reduced.

Signed-off-by: Wonho Yun <[email protected]>
@yun-goon
Copy link
Member Author

@robotpilot Added CI for the Humble version

Copy link
Member

@robotpilot robotpilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@robotpilot robotpilot merged commit 1bfd061 into main Mar 14, 2025
3 checks passed
@robotpilot robotpilot deleted the feature-humble-issue-handling branch March 14, 2025 08:19
@github-project-automation github-project-automation bot moved this from 📝 Pull Request to 🚩Done in Platform Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants