Skip to content
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

add spacenav example #591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add spacenav example #591

wants to merge 1 commit into from

Conversation

borongyuan
Copy link

Description

Add a simple spacenav example, only launch file. Need to wait for ros-drivers/joystick_drivers#251 to be merged. There I added an option to make spacenav_node publish TwistStamped directly.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

Signed-off-by: Borong Yuan <[email protected]>
@rhaschke
Copy link
Contributor

Thanks for your effort. According to moveit/moveit_tutorials#754 (comment), spacenav_node and the corresponding spacenavd are not needed anymore. It would be great if you could try the approach suggested in the linked issue (and thus getting rid of the special spacenav stuff).

@AndyZe
Copy link
Member

AndyZe commented Feb 15, 2023

Can you be more specific, @rhaschke? Are you against adding any spacenav functionality completely?

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice and simple.

return LaunchDescription(
[
Node(
package="spacenav",
Copy link
Member

Choose a reason for hiding this comment

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

An <exec_depend> on spacenav should be added to root/package.xml

@rhaschke
Copy link
Contributor

Can you be more specific, @rhaschke? Are you against adding any spacenav functionality completely?

I suggest to drop spacenav_node specific stuff and operate the space mouse directly via joystick software (only one thing to maintain in future).

@borongyuan
Copy link
Author

I suggest to drop spacenav_node specific stuff and operate the space mouse directly via joystick software (only one thing to maintain in future).

Yes, this is what I want. As long as the joystick software can directly publish TwistStamped, other nodes for message conversion can be removed.
Replacing Twist with TwistStamped is useful in future applications. This is also discussed and followed up in other packages.
ros-teleop/teleop_tools#54
ros-navigation/navigation2#1594
So it's best to handle this part in joystick_drivers.

@gavanderhoorn
Copy link
Member

So it's best to handle this part in joystick_drivers.

not really, I believe.

joystick_drivers should care about interfacing with joysticks and gamepads. It should not have anything to do with conversion to arbitrary other messages.

That's what the packages in ros/ros-teleop can be used for.

@borongyuan
Copy link
Author

joystick_drivers should care about interfacing with joysticks and gamepads. It should not have anything to do with conversion to arbitrary other messages.

The Joy message is a generic message for all types of joysticks. ros-teleop can be used to define the function of any joystick.
While the SpaceMouse is a 6DoF Joystick. So it's reasonable to publish Twist directly. This makes it easier to get started and also reduces delay. We can still use ros-teleop to configure the function of its buttons.
For conversion to other messages, cartesian_controllers also provides some references.

@AndyZe
Copy link
Member

AndyZe commented Feb 16, 2023

We used teleop_tools in ROS1. It was pretty easy to configure 👍

https://github.com/ros-planning/moveit/blob/master/moveit_ros/moveit_servo/config/spacenav_via_teleop_tools.yaml

@mergify
Copy link

mergify bot commented Aug 14, 2023

This pull request is in conflict. Could you fix it @borongyuan?

1 similar comment
Copy link

mergify bot commented Dec 15, 2023

This pull request is in conflict. Could you fix it @borongyuan?

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.

4 participants