Subsystem integration by mjeronimo · Pull Request #136 · ros-navigation/navigation2

mjeronimo · 2018-10-03T16:25:23Z

Some changes as a result of integrating the various modules into a working system. Also added a service client template class to common up some duplicate code. Added some initial launch-related files, but this still needs work.

Simple navigator gets the goal pose from gazebo via the move_base_simpl/goal message. The Dijkstra global planner gets the current robot pose from localization.

…ose from rviz

…roller

Simple navigator gets the goal pose from gazebo via the move_base_simpl/goal message. The Dijkstra global planner gets the current robot pose from localization.

…ose from rviz

ghost · 2018-10-03T16:33:26Z

launch/test_map.yaml

@@ -0,0 +1,7 @@
+image: test_map.pgm


test_map.pgm and yaml are already checked in under localization/nav2_amcl/test/maps. I'm not sure the right place to put the file, but having multiple copies of the same binary files seems like a bad idea.

I agree. As we discussed we should probably have a maps/resources package that any of the modules and tests can use. I'll file an issue.

This may not be the test map file, but there is also a testmap.yaml & testmap.png inside mapping/nav2_map_server/test

ghost · 2018-10-03T16:42:36Z

src/libs/nav2_robot/include/nav2_robot/ros_robot.hpp

   * @param[in] filename The filename of the URDF file describing this robot.
   */
-  explicit RosRobot(const std::string & urdf_filename);
+  explicit RosRobot(rclcpp::Node *node/*, const std::string & urdf_filename*/);


We should probably just delete the urdf_filename param and the comment above. I suspect it will be a long time before we are extracting robot params from the URDF directly.

ghost · 2018-10-03T16:45:35Z

src/libs/nav2_tasks/include/nav2_tasks/costmap_service_client.hpp

@@ -0,0 +1,38 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.


This looks like new code. Is this copyright correct?

Oops. Copy and paste from the wrong file. Fixed them both.

ghost · 2018-10-03T16:48:06Z

src/libs/nav2_robot/src/ros_robot.cpp

+RosRobot::getCurrentPose()
+{
+  if (!initial_pose_received_)
+    throw std::runtime_error("RosRobot::getCurrentPose: initial pose not received yet");


This seems more of normal event. Maybe this should just return a nullptr instead of throwing? Just a thought. Depends on how it will be used in practice I think.

I put a note in the code. We'll live with it a bit and see.

ghost · 2018-10-03T16:48:41Z

src/libs/nav2_tasks/include/nav2_tasks/map_service_client.hpp

@@ -0,0 +1,38 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.


Same here on the copyright

ghost · 2018-10-03T16:56:11Z

src/libs/nav2_tasks/include/nav2_tasks/service_client.hpp

+
+    if (rclcpp::spin_until_future_complete(node_, result_future) !=
+      rclcpp::executor::FutureReturnCode::SUCCESS)
+    {


I think this is a linter failure. Opening brace needs to be on the same line as the if typically.

I thought so too, but this is what uncrustify wanted. I think it had to do with the expression broken on the previous line. It passed cpplint and uncrustify.

ghost · 2018-10-03T16:58:18Z

src/libs/nav2_tasks/include/nav2_tasks/service_client.hpp

+      if (!rclcpp::ok()) {
+        throw std::runtime_error("waitForServer: interrupted while waiting for service to appear");
+      }
+      RCLCPP_INFO(rclcpp::get_logger("rclcpp"),


I'd prefer a client object to not spit out console messages that the user has no control over, but if you really need this, the name used in the get_logger call should be something specific. You get a name passed in the constructor that you could use instead.

Yes, I agree; the client can control the timeout and output a message, if desired. Removed.

ghost

When I follow the bringup procedure with the Turtlebot 3 packages, cmd_vel is still called cmd_vel.

I think we shouldn't have changed the default topic names, but rather just remapped them to work with Turtlebot 2

bpwilcox · 2018-10-03T18:02:54Z

launch/test_map.yaml

@@ -0,0 +1,7 @@
+image: test_map.pgm


This may not be the test map file, but there is also a testmap.yaml & testmap.png inside mapping/nav2_map_server/test

bpwilcox · 2018-10-03T18:25:31Z

src/mapping/nav2_map_server/src/map_reps/occ_grid_server.cpp

 {
  (void)request_header;
  (void)req;
+//  occ_resp_.map = map_msg_;


I suppose we could get rid of the map_msg_ and only work with occ_resp_.map throughout the file.

bpwilcox · 2018-10-03T18:40:10Z

src/planning/nav2_planning_tests/planner_tester.cpp

@@ -80,6 +80,9 @@ void PlannerTester::spinThread()
 void PlannerTester::loadMap(const std::string image_file_path, const std::string yaml_file_name)


This function duplicates a lot of code from the nav2_map_server. I think it takes from a utility file (map_loader) in libs that duplicates the loadMapFromFile functionality. Do we want this to make that utility as part of libs or keep it maintained in nav2_map_server? My caution is that if we are loading files other than the types here (in this case Occupancy Grids), it would require extensions in the utility functions in addition to the nav2_map_server.

Co-authored-by: andriimaistruk <andriimaistruk>

* Updated migration guide due to ros-navigation#1855 * Update migration/Foxy.rst Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> * review changes incorporated, added the abbreviation definition of BT Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>

Michael Jeronimo and others added 11 commits October 2, 2018 17:19

Working on integrating multiple modules

7e9ba73

Fix cpplint problem

b131679

Hook up source and destination poses

4f63212

Simple navigator gets the goal pose from gazebo via the move_base_simpl/goal message. The Dijkstra global planner gets the current robot pose from localization.

Send a mission plan from the mission executor upon receiving a goal p…

c179d9b

…ose from rviz

Incorporate Carl's change to make the robot move in the stub dwa cont…

eab2725

…roller

Adding geometry_msg dependency to robot library.

accfecf

Start on a system launch file

013067a

Working on integrating multiple modules

ebe726e

Hook up source and destination poses

ddbee57

Simple navigator gets the goal pose from gazebo via the move_base_simpl/goal message. The Dijkstra global planner gets the current robot pose from localization.

Send a mission plan from the mission executor upon receiving a goal p…

42ec573

…ose from rviz

Some additional merge fixes and removing some temporary stuff

f683cf1

mjeronimo requested review from a user, bpwilcox and mhpanah October 3, 2018 16:29

ghost approved these changes Oct 3, 2018

View reviewed changes

Fix Carl's review feedback

1a87996

ghost approved these changes Oct 3, 2018

View reviewed changes

bpwilcox approved these changes Oct 3, 2018

View reviewed changes

mjeronimo merged commit 350533e into ros-navigation:master Oct 3, 2018

mjeronimo deleted the subsystem-integration branch October 10, 2018 16:19

ghost pushed a commit to logivations/navigation2 that referenced this pull request Mar 7, 2022

fix name (ros-navigation#136)

fabce3c

Co-authored-by: andriimaistruk <andriimaistruk>

		@@ -0,0 +1,38 @@
		// Copyright 2016 Open Source Robotics Foundation, Inc.

		@@ -80,6 +80,9 @@ void PlannerTester::spinThread()
		void PlannerTester::loadMap(const std::string image_file_path, const std::string yaml_file_name)

Conversation

mjeronimo commented Oct 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants