Skip to content

Moveit2#72

Merged
cmower merged 9 commits intomainfrom
dev-sdas-moveit2
Sep 17, 2025
Merged

Moveit2#72
cmower merged 9 commits intomainfrom
dev-sdas-moveit2

Conversation

@sarthakdas
Copy link
Copy Markdown
Contributor

The base implementation of movie 2 with ark.

@sarthakdas sarthakdas requested a review from cmower September 15, 2025 09:59
Copy link
Copy Markdown
Contributor

@cmower cmower left a comment

Choose a reason for hiding this comment

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

Please change all cases of move_it2 and replace with moveit2.

Comment thread ark/tools/move_it2/move_it2_bridge.py Outdated
Comment thread ark/tools/move_it2/move_it2_bridge.py Outdated

def moveit_translator(self, ros_msg, ros_channel, ros_type, ark_channel, ark_type):
"""Convert joint state positions into Ark command."""
msg = ark_type()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what is ark_type? At this point, these are supposed to be known, so no need to be generic - can just use the name can't we?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unless it conflicts with another variable, then i think should be changed to be more explicit and easier for the reader to understand the code

Comment thread ark/tools/move_it2/move_it2_bridge.py Outdated
Comment thread ark/tools/move_it2/move_it2_bridge.py Outdated
@cmower
Copy link
Copy Markdown
Contributor

cmower commented Sep 16, 2025

@sarthakdas - please make sure to address above comments asap, once the tutorial is done let me know and i will approve/merge

@cmower
Copy link
Copy Markdown
Contributor

cmower commented Sep 16, 2025

This last commit re types is wrong, the type for ark_msg is not joint_group_command_t. And as I mentioned the variable names should align with what they are, and mot be generic. Also ros_msg variable names should be updated and be given the correct type hint

@sarthakdas
Copy link
Copy Markdown
Contributor Author

The callback parameters need to remain generic so they can handle all potential callbacks. Different callbacks may use different message channel names and types, as defined in the ROS2 bridge implementation. This follows the same layout we use for subscriber callbacks in Ark, where each callback receives (t, channel_name, msg). If the parameter names in this function were changed, it would break compatibility with the ROS2 bridge, since we rely on functools.partial to dynamically generate these callbacks. Without this, the feature would no longer function as intended.

The current function signature is:
(ros_msg: JointTrajectoryControllerState, ros_channel: str, ros_type: JointTrajectoryControllerState, ark_channel: str, ark_type: joint_group_command_t).
In this case, the input parameters will always be of these exact types, and the function will consistently return a packed joint_group_command_t message. Similarly, the ROS message will always be of type JointTrajectoryControllerState for this specific callback, as defined in the MoveIt mapping table. I am not sure by what is ment by "ark_msg is not joint_group_command_t" because it always will be as discussed above.

@cmower cmower merged commit fd76793 into main Sep 17, 2025
@cmower cmower deleted the dev-sdas-moveit2 branch September 17, 2025 09:18
Douglas-Tilley pushed a commit that referenced this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants