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 drake_ros_examples package #39

Merged
merged 18 commits into from
May 24, 2022
Merged

Add drake_ros_examples package #39

merged 18 commits into from
May 24, 2022

Conversation

aaronchongth
Copy link
Collaborator

@aaronchongth aaronchongth commented Feb 5, 2022

Spin off from #2. Built on top of #24.


This change is Reviewable

@aaronchongth aaronchongth changed the base branch from develop to hidmic/drake_ros_viz_pybind April 4, 2022 05:13
@aaronchongth
Copy link
Collaborator Author

Per VC,

  • The iiwa_manipulator example is currently broken, a working example can be found in a separate branch, the issue could possibly be the changes in the header::frame_id, ns and id of the marker messages,
ros2 run drake_ros_examples iiwa_manipulator
ros2 topic echo /scene_marker/visual
  • frame_id: in order to support similar MbPs, the transform names will need to be namespaced with their relevant frame IDs, while the previous implementation does not include any frame IDs

before:
Screenshot from 2022-04-04 20-44-51

after:
Screenshot from 2022-04-04 20-51-53

@sloretz sloretz force-pushed the hidmic/drake_ros_viz_pybind branch 2 times, most recently from 9f285f6 to 2a23ddb Compare April 21, 2022 23:13
@gbiggs gbiggs changed the base branch from hidmic/drake_ros_viz_pybind to develop April 26, 2022 03:34
@gbiggs gbiggs force-pushed the aaron/drake_ros_examples branch from 28b4920 to 4cae022 Compare May 2, 2022 11:43
@gbiggs gbiggs force-pushed the aaron/drake_ros_examples branch from 4cae022 to 1fb51f4 Compare May 9, 2022 00:57
Copy link
Collaborator

@gbiggs gbiggs left a comment

Choose a reason for hiding this comment

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

I tested the iiwa_manipulator example and it worked for me. I'll keep prodding it in case I've missed something, but for now I think the first item can be checked off.

Reviewable status: 0 of 21 files reviewed, all discussions resolved

Copy link
Collaborator

@gbiggs gbiggs left a comment

Choose a reason for hiding this comment

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

I tried out the multirobot example to see how the frame IDs turned out. Below is the result. It looks like each individual robot is namespaced already to me.

image.png

Reviewable status: 0 of 21 files reviewed, all discussions resolved

Copy link
Collaborator

@gbiggs gbiggs left a comment

Choose a reason for hiding this comment

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

I have confirmed with @aaronchongth that the third point is a general note, not something that requires fixing. This PR is ready for review.

Reviewable status: 0 of 21 files reviewed, all discussions resolved

@gbiggs gbiggs requested a review from sloretz May 11, 2022 13:57
Copy link
Collaborator

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 20 of 20 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @aaronchongth)


drake_ros_core/package.xml line 13 at r4 (raw file):

  <buildtool_depend>ament_cmake_ros</buildtool_depend>

  <depend>pybind11-dev</depend>

I think this can be removed since we use pybind11 from drake.


drake_ros_examples/README.md line 28 at r4 (raw file):

## Running

* If you built with `colcon`, then source your workspace.

Nit, this sentence can be deleted because there's now only instructions on building with Colcon.


drake_ros_examples/examples/CMakeLists.txt line 2 at r4 (raw file):

add_subdirectory(iiwa_manipulator)
add_subdirectory(rs_flip_flop)

Nit, subjectively I'd prefer these add_subdirectories where in the main CMakeListst.txt instead of a separate file. Feel free to ignore if you feel differently.

add_subdirectory(examples/iiwa_manipulator)
add_subdirectory(examples/rs_flip_flop)

drake_ros_examples/examples/iiwa_manipulator/iiwa_manipulator.py line 38 at r4 (raw file):

    manipulation_station.SetupClutterClearingStation()
    manipulation_station.Finalize()
    pdb.set_trace()

This debug statement should be removed.


drake_ros_examples/examples/iiwa_manipulator/README.md line 13 at r4 (raw file):

## How To

Run either the C++ `iiwa_manipulator` executable or the Python `iiwa_manipulator.py` script as explained [here](../../README.md#running).

I'd recommend including the commands to run this example in this file too.

ros2 run drake_ros_examples iiwa_manipulator
ros2 run drake_ros_examples iiwa_manipulator.py

drake_ros_examples/examples/rs_flip_flop/README.md line 18 at r4 (raw file):

## How To

Run either the C++ `rs_flip_flop` executable or the Python `rs_flip_flop.py` script as explained [here](../../README.md#running).

I'd recommend pasting the commands to run the examples here

ros2 run drake_ros_examples rs_flip_flop
ros2 run drake_ros_examples rs_flip_flop.py

drake_ros_examples/examples/rs_flip_flop/README.md line 34 at r4 (raw file):

ros2 topic pub /S std_msgs/msg/Bool "data: false"
ros2 topic pub /S std_msgs/msg/Bool "data: true"

I'd recommend regrouping the commands so that Q actually changes. As it is, if I copy past these commands from top to bottom Q will always be `false. Something like


Make Q true with

ros2 topic pub /S std_msgs/msg/Bool "data: false"
ros2 topic pub /R std_msgs/msg/Bool "data: true"

Make Q false again with

ros2 topic pub /R std_msgs/msg/Bool "data: false"
ros2 topic pub /S std_msgs/msg/Bool "data: true"

drake_ros_examples/examples/rs_flip_flop/rs_flip_flop.py line 1 at r4 (raw file):

# Copyright 2020 Open Source Robotics Foundation, Inc.

I think this needs a #!/usr/bin/env python3

$ ros2 run drake_ros_examples rs_flip_flop.py 
Traceback (most recent call last):
  File "/opt/ros/rolling/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli==0.17.1', 'console_scripts', 'ros2')()
  File "/opt/ros/rolling/lib/python3.8/site-packages/ros2cli/cli.py", line 89, in main
    rc = extension.main(parser=parser, args=args)
  File "/opt/ros/rolling/lib/python3.8/site-packages/ros2run/command/run.py", line 70, in main
    return run_executable(path=path, argv=args.argv, prefix=prefix)
  File "/opt/ros/rolling/lib/python3.8/site-packages/ros2run/api/__init__.py", line 64, in run_executable
    process = subprocess.Popen(cmd)
  File "/usr/lib/python3.8/subprocess.py", line 858, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1704, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 8] Exec format error: '/home/osrf/ws/drake/install/lib/drake_ros_examples/rs_flip_flop.py'

drake_ros_examples/examples/rs_flip_flop/rs_flip_flop.py line 145 at r4 (raw file):

    while True:
        simulator.AdvanceTo(simulator_context.get_time() + 0.1)

Need an

if __name__ == '__main__':
    main()

Copy link
Collaborator

@gbiggs gbiggs left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @sloretz)


drake_ros_core/package.xml line 13 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I think this can be removed since we use pybind11 from drake.

Removed in 7161030.


drake_ros_examples/README.md line 28 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Nit, this sentence can be deleted because there's now only instructions on building with Colcon.

Corrected in c2294f2.


drake_ros_examples/examples/CMakeLists.txt line 2 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Nit, subjectively I'd prefer these add_subdirectories where in the main CMakeListst.txt instead of a separate file. Feel free to ignore if you feel differently.

add_subdirectory(examples/iiwa_manipulator)
add_subdirectory(examples/rs_flip_flop)

Done in 47ff0f8.


drake_ros_examples/examples/iiwa_manipulator/iiwa_manipulator.py line 38 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

This debug statement should be removed.

Removed in 1b7b22c.


drake_ros_examples/examples/iiwa_manipulator/README.md line 13 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I'd recommend including the commands to run this example in this file too.

ros2 run drake_ros_examples iiwa_manipulator
ros2 run drake_ros_examples iiwa_manipulator.py

Added in a0c27c8.


drake_ros_examples/examples/rs_flip_flop/README.md line 18 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I'd recommend pasting the commands to run the examples here

ros2 run drake_ros_examples rs_flip_flop
ros2 run drake_ros_examples rs_flip_flop.py

Clarified in 12ba117.


drake_ros_examples/examples/rs_flip_flop/README.md line 34 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I'd recommend regrouping the commands so that Q actually changes. As it is, if I copy past these commands from top to bottom Q will always be `false. Something like


Make Q true with

ros2 topic pub /S std_msgs/msg/Bool "data: false"
ros2 topic pub /R std_msgs/msg/Bool "data: true"

Make Q false again with

ros2 topic pub /R std_msgs/msg/Bool "data: false"
ros2 topic pub /S std_msgs/msg/Bool "data: true"

Good catch! Clarified in 199bb3e.


drake_ros_examples/examples/rs_flip_flop/rs_flip_flop.py line 1 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I think this needs a #!/usr/bin/env python3

$ ros2 run drake_ros_examples rs_flip_flop.py 
Traceback (most recent call last):
  File "/opt/ros/rolling/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli==0.17.1', 'console_scripts', 'ros2')()
  File "/opt/ros/rolling/lib/python3.8/site-packages/ros2cli/cli.py", line 89, in main
    rc = extension.main(parser=parser, args=args)
  File "/opt/ros/rolling/lib/python3.8/site-packages/ros2run/command/run.py", line 70, in main
    return run_executable(path=path, argv=args.argv, prefix=prefix)
  File "/opt/ros/rolling/lib/python3.8/site-packages/ros2run/api/__init__.py", line 64, in run_executable
    process = subprocess.Popen(cmd)
  File "/usr/lib/python3.8/subprocess.py", line 858, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1704, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 8] Exec format error: '/home/osrf/ws/drake/install/lib/drake_ros_examples/rs_flip_flop.py'

Added in eb33c9c.


drake_ros_examples/examples/rs_flip_flop/rs_flip_flop.py line 145 at r4 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Need an

if __name__ == '__main__':
    main()

Done in 0e45e4e.

@gbiggs
Copy link
Collaborator

gbiggs commented May 17, 2022

Thanks for the review, @sloretz. I've made the appropriate fixes. Unfortunately it turns out that the flip flop Python example doesn't actually run, so I need to fix that before this can be merged.

@gbiggs
Copy link
Collaborator

gbiggs commented May 22, 2022

@sloretz I've corrected the API usage in the Python rs_flip_flop sample so it functions now. If you're happy, I think we're ready to merge this.

Copy link
Collaborator

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

I tested the examples locally. This :lgtm: 🎉

Reviewed 7 of 8 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aaronchongth)

@gbiggs gbiggs merged commit 0cd5c90 into develop May 24, 2022
@gbiggs gbiggs deleted the aaron/drake_ros_examples branch May 24, 2022 02:34
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