Port to ros2 #92

vatanaksoytezer · 2021-05-22T15:04:50Z

No description provided.

vatanaksoytezer · 2021-05-26T19:28:45Z

This is currently blocked by: PickNikRobotics/rviz_visual_tools#185 . With that merged this should work without any problems.

JafarAbdi · 2021-06-10T16:07:08Z

This is currently blocked by: PickNikRobotics/rviz_visual_tools#185 . With that merged this should work without any problems.

I merged it

vatanaksoytezer · 2021-06-10T20:15:24Z

Sorry, for forgetting this I will also need a ROS2 branch on https://github.com/PickNikRobotics/graph_msgs and send a PR port right there as well

henningkayser

Looks pretty clean so far 👍 Were you able to get all of the demo work as expected? I'll probably test this later today

CMakeLists.txt

henningkayser · 2021-06-11T09:22:19Z

src/moveit_visual_tools.cpp

    {
      counter += CHECK_TIME_INTERVAL;
-      ros::Duration(CHECK_TIME_INTERVAL).sleep();
+      rclcpp::sleep_for(std::chrono::milliseconds(int(CHECK_TIME_INTERVAL * 1000)));


Suggested change

rclcpp::sleep_for(std::chrono::milliseconds(int(CHECK_TIME_INTERVAL * 1000)));

rclcpp::sleep_for(rclcpp::Duration::from_seconds(CHECK_TIME_INTERVAL));

This does not compile, sleep_for cannot be initialized with rclcpp::Duration::from_seconds

std::chrono::seconds?

std::chrono::seconds takes integers only and I have 0.25 seconds to sleep.

src/moveit_visual_tools.cpp

src/moveit_visual_tools_demo.cpp

vatanaksoytezer · 2021-06-11T09:52:47Z

Looks pretty clean so far Were you able to get all of the demo work as expected? I'll probably test this later today.

@henningkayser Yes! Demo should work as expected. Thanks a lot for the detailed review, that is really helpful for me. I will send the fixes in Sunday when I get back home.

CMakeLists.txt

Co-authored-by: AdamPettinger <adam.pettinger@utexas.edu>

AdamPettinger · 2021-07-16T21:38:55Z

I've built and tested this locally. The demo runs well, and I was able to use moveit_visual_tools in another package successfully (but not used as in depth as the demo of course)

henningkayser

I guess this is the last round of comments ;)

README.md

moveit_visual_tools.repos

henningkayser · 2021-07-30T20:40:01Z

src/imarker_robot_state.cpp

+  // ! Add node here
+  // imarker_server_ = std::make_shared<interactive_markers::InteractiveMarkerServer>(imarker_topic, "", false);
+  // TODO: Check topic ns or imarker_topic
+  imarker_server_ = std::make_shared<interactive_markers::InteractiveMarkerServer>("", node);


So the topic namespace should still be used, the constructor accepts that as well. I think it's still pretty important that namespaces are used everywhere, also in line 130 above.

src/moveit_visual_tools.cpp

henningkayser · 2021-07-30T20:45:05Z

src/moveit_visual_tools.cpp

    {
      counter += CHECK_TIME_INTERVAL;
-      ros::Duration(CHECK_TIME_INTERVAL).sleep();
+      rclcpp::sleep_for(std::chrono::milliseconds(int(CHECK_TIME_INTERVAL * 1000)));


std::chrono::seconds?

src/moveit_visual_tools_demo.cpp

.clang-format

JafarAbdi

There're few bugs related to rclcpp::Duration that need to be fixed before merging this

JafarAbdi · 2021-08-11T19:17:31Z

CMakeLists.txt


 # Install shared resources
-install(DIRECTORY launch    DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
-install(DIRECTORY resources DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
+install(DIRECTORY launch resources DESTINATION share/${PROJECT_NAME})

 # Install executables
 install(TARGETS ${PROJECT_NAME}_demo


+1 to Henning's comment, we specify the ARCHIVE, LIBRARY, and RUNTIME destinations so CMake automatically select the path based on the target's type, I don't see why we're duplicating the command

JafarAbdi · 2021-08-11T19:20:50Z

launch/debug_settings.gdb

@@ -1,2 +0,0 @@
-set breakpoint pending on


Any reason for removing this .? I think we should still keep it in the launch file

I don't think we would need gdb right now, I can add it back though.

moveit_visual_tools.repos

src/imarker_robot_state.cpp

src/moveit_visual_tools.cpp

launch/demo_rviz.launch.py

src/imarker_end_effector.cpp

I didn't look at all changes

vatanaksoytezer · 2021-08-29T21:54:56Z

@henningkayser @JafarAbdi is there anything else you would like me to address?

Vatan Aksoy Tezer added 5 commits May 19, 2021 11:44

Port headers

d86c8dc

Port demo and source files

6064d76

Fix most compile issues

8364a48

Port launch files

f6c31a1

Fix runtime errors, add some verbosity

7b8925d

vatanaksoytezer marked this pull request as draft May 26, 2021 17:42

Vatan Aksoy Tezer added 2 commits May 26, 2021 18:28

Reverting to template pub from rviz visual tools

68dcb4e

Cleanup

9fb8113

vatanaksoytezer marked this pull request as ready for review May 26, 2021 19:28

vatanaksoytezer requested review from henningkayser and JafarAbdi May 26, 2021 19:28

More cleanup on cmake file

0481201

vatanaksoytezer changed the title ~~WIP: Port to ros2~~ Port to ros2 May 27, 2021

Vatan Aksoy Tezer added 7 commits June 10, 2021 21:47

Port ros2 to Github Actions

6085b3f

pre commit fixes

ae128a7

Cleanup

6e1cf94

Add CI

32c70f6

Fix pre commit

480db15

Fix branch name

14bea3e

Fix ci

08dfa96

vatanaksoytezer force-pushed the pr-port_ros2 branch from f453d2b to 08dfa96 Compare June 10, 2021 20:03

Fix image name

b208c6b

Vatan Aksoy Tezer added 3 commits June 10, 2021 23:42

Add graph_msgs repo

c21a6e3

Remove ament tests, fix pre-commit

a4cec6d

Add CI badges

512df23

henningkayser reviewed Jun 11, 2021

View reviewed changes

Add missing dependency, alphabetize

6801d0b

AdamPettinger reviewed Jul 16, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Update CMakeLists.txt

98cb7ba

Co-authored-by: AdamPettinger <adam.pettinger@utexas.edu>

henningkayser reviewed Jul 30, 2021

View reviewed changes

JafarAbdi reviewed Jul 31, 2021

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

Vatan Aksoy Tezer added 11 commits August 3, 2021 11:21

Add clang, adress a few comments

5cf8ff4

clang format and pre-commit fixes

5e4c4e9

Delete unnecesary linking

df11353

Use chrono literals

e5b1f7d

Make shared instead of new

cb7d3aa

animate speed docs

03efd8b

Throw warning if there are no subscribers

b6ccb46

Add manual, fix typo

6658269

Add namespaces

072cacd

pre-commit fixes

9e82477

Use chrono literals

5a3a64c

JafarAbdi requested changes Aug 11, 2021

View reviewed changes

Adress Jafar's review

5c31be9

vatanaksoytezer requested review from henningkayser and JafarAbdi August 18, 2021 16:35

Vatan Aksoy Tezer added 2 commits August 18, 2021 16:43

pre-commit fixes

4ae14ab

Fix ccache

64fff3e

rhaschke previously approved these changes Aug 19, 2021

View reviewed changes

Make libraries shared

e8e4a8d

henningkayser approved these changes Sep 9, 2021

View reviewed changes

JafarAbdi approved these changes Sep 14, 2021

View reviewed changes

JafarAbdi merged commit 715b9d5 into moveit:ros2 Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port to ros2 #92

Port to ros2 #92

vatanaksoytezer commented May 22, 2021

vatanaksoytezer commented May 26, 2021

JafarAbdi commented Jun 10, 2021

vatanaksoytezer commented Jun 10, 2021 •

edited

Loading

henningkayser left a comment

henningkayser Jun 11, 2021

vatanaksoytezer Jun 15, 2021

henningkayser Jul 30, 2021

vatanaksoytezer Aug 3, 2021

vatanaksoytezer commented Jun 11, 2021

AdamPettinger commented Jul 16, 2021

henningkayser left a comment

henningkayser Jul 30, 2021

henningkayser Jul 30, 2021

JafarAbdi left a comment

JafarAbdi Aug 11, 2021

JafarAbdi Aug 11, 2021

vatanaksoytezer Aug 18, 2021

vatanaksoytezer commented Aug 29, 2021

	rclcpp::sleep_for(std::chrono::milliseconds(int(CHECK_TIME_INTERVAL * 1000)));
	rclcpp::sleep_for(rclcpp::Duration::from_seconds(CHECK_TIME_INTERVAL));

Port to ros2 #92

Port to ros2 #92

Conversation

vatanaksoytezer commented May 22, 2021

vatanaksoytezer commented May 26, 2021

JafarAbdi commented Jun 10, 2021

vatanaksoytezer commented Jun 10, 2021 • edited Loading

henningkayser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vatanaksoytezer commented Jun 11, 2021

AdamPettinger commented Jul 16, 2021

henningkayser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JafarAbdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vatanaksoytezer commented Aug 29, 2021

vatanaksoytezer commented Jun 10, 2021 •

edited

Loading