Removing service client nodes from navfn #897

orduno · 2019-06-27T21:16:57Z

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#816
Primary OS tested on	Ubuntu 18.04
Robotic platform tested on	gazebo simulation of turtlebot 3

Description of contribution in a few bullet points

As described in #816, several nodes are created and used solely for interfacing to services. This PR proposes an approach for removing such nodes.

Several improvements to ServiceClient. It will no longer create a node for interfacing with services if one is provided.
Updated GetRobotPoseClient and CostmapServiceClient to use improved ServiceClient.
Updated Navfn to use new clients. For obtaining async service responses, it requires a multi-threaded executor.

Once we agree on the model, which I'm using navfn as an example, I can propagate the changes to the rest of the nodes on a separate PR.

Future work that may be required in bullet points

Remove service nodes from amcl, costmaps, bt navigator and lifecycle.
Replace the multi-threaded executor with a custom async service executor.

crdelsey

Here's some design drawings that I gathered from the code.

Class Diagram of Key Functions and methods

Construction Sequence

OnConfigure service call

I'd like to clean up the main Construction sequence a bit. I'll try to draw something up later. Main shouldn't have to call has_nested_service_requests. Main should be saying something more like - here's the nodes I have, give me an appropriate executor. I think we can achieve that by adding another interface to our nodes that provides the "has_nested_service_requests" method on it. Then main could just pass in the nodes.

Also, I'd like to somehow hide the CallBackGroup from NavFnPlanner if possible.

crdelsey · 2019-07-23T21:20:59Z

nav2_util/src/map/map_cspace.cpp

@@ -67,7 +67,7 @@ class CachedDistanceMap
 bool operator<(const CellData & a, const CellData & b)
 {
  return a.map_->cells[MAP_INDEX(a.map_, a.i_,
-           a.j_)].occ_dist > a.map_->cells[MAP_INDEX(b.map_, b.i_, b.j_)].occ_dist;
+         a.j_)].occ_dist > a.map_->cells[MAP_INDEX(b.map_, b.i_, b.j_)].occ_dist;


This change should not be part of this PR. It might just be that this needs to be rebased.

crdelsey · 2019-07-23T21:28:38Z

nav2_navfn_planner/include/nav2_navfn_planner/navfn_planner.hpp

@@ -43,6 +43,8 @@ class NavfnPlanner : public nav2_util::LifecycleNode
  NavfnPlanner();
  ~NavfnPlanner();

+  bool has_nested_service_requests() {return has_nested_client_;}


I'd ditch the variable and just make this return true since it is always true.

I'd also add a comment explaining that the existence of costmap_client_ and get_robot_pose_client_ make it true. That way, there is a chance that someone changing those members will also find this function and change it appropriately.

Alternatively, I expect we could do something with template metaprogramming to automatically detect the existence of service clients and have this function just do the right thing.

It'd be nice if there was a function you could call on the node to return a count of service clients it has, so that we don't have to define that here.

crdelsey · 2019-07-23T22:59:20Z

nav2_util/include/nav2_util/service_client.hpp

  }

-  ServiceClient(const std::string & service_name, const std::string & parent_name)
+  // TODO(orduno) Remove this constructor which relies on generating an internal node
+  [[deprecated("use alternative constructors")]]


We don't need to do this deprecated stuff. It's just going to cause our build to fail when we re-enable deprecated warnings, which we should do.

crdelsey · 2019-07-23T23:49:10Z

nav2_util/include/nav2_util/service_client.hpp

@@ -118,10 +194,22 @@ class ServiceClient
    }
  }

+  bool nodeIsSpinable() const


This should be snake case. It's more standard to start the function name with is rather than put it in the middle. And it should be spelt spinnable with two n eg
is_node_spinnable()

crdelsey · 2019-07-24T15:28:55Z

@mjeronimo @orduno As I thought about this some more, I don't think we should continue with this approach to removing the service node.

As I understand it, we thought there would be a fairly simple, elegant way of removing them using some of the futures facilities available. This turned out not to be the case. To make it work requires these fairly big, intrusive changes.

I think we should abandon this change and just get things fixed upstream so we can do reentrant service calls.

Thoughts?

mjeronimo · 2019-07-24T15:36:04Z

nav2_navfn_planner/src/navfn_planner.cpp

@@ -542,7 +550,7 @@ NavfnPlanner::getCostmap(
  auto request = std::make_shared<nav2_util::CostmapServiceClient::CostmapServiceRequest>();
  request->specs.resolution = 1.0;

-  auto result = costmap_client_.invoke(request, 5s);
+  auto result = costmap_client_->invoke(request, 5s);


Per feedback on another PR and our group discussion, we should parameterize these timeouts. This doesn't have to happen with this PR, but we should open an issue on this. Probably have a service_call_timeout parameter or something like that. We should standardize the name because it will be needed in several modules and should use the same name (not that they are necessarily the same value).

mjeronimo · 2019-07-24T15:40:12Z

nav2_util/include/nav2_util/get_robot_pose_client.hpp

+  explicit GetRobotPoseClient(
+    rclcpp_lifecycle::LifecycleNode::SharedPtr node,
+    rclcpp::callback_group::CallbackGroup::SharedPtr group = nullptr)
+  : ServiceClient<nav2_msgs::srv::GetRobotPose>("GetRobotPose", node, group)
  {


I noticed that there is not a constructor for the node_interfaces version. With node helper classes like this, we should standardize on the constructors we provide. ROS2 seems to provide the rclcpp::Node version and the node_interfaces version. I think there are three options:

All Three - rclcpp::Node, rclcpp_lifecycle::LifecycleNode, and node_interfaces

Node and interfaces (like ROS2)

Node and LifecycleNode

Let's talk as a group to decide.

bpwilcox

@crdelsey I'm in favor of moving this PR forward and working on changes upstream in parallel.

bpwilcox · 2019-07-24T19:39:54Z

nav2_navfn_planner/include/nav2_navfn_planner/navfn_planner.hpp

@@ -43,6 +43,8 @@ class NavfnPlanner : public nav2_util::LifecycleNode
  NavfnPlanner();
  ~NavfnPlanner();

+  bool has_nested_service_requests() {return has_nested_client_;}


It'd be nice if there was a function you could call on the node to return a count of service clients it has, so that we don't have to define that here.

bpwilcox · 2019-07-29T19:50:48Z

I think there could be a test that does a simplified mock of the lifecycle service client use-case on a spinning node as we do in our stack, especially given changes with the use of multithreaded executors and callback groups

SteveMacenski · 2019-07-29T20:30:43Z

nav2_navfn_planner/src/main.cpp


+  auto planner = std::make_shared<nav2_navfn_planner::NavfnPlanner>();
+
+  // The planner needs a special executor


Feedback: I'd just put in the multithreaded executor, making this wrapper makes it less clear to me than just having it all together.

If there's issues with the planner as a service, maybe it should be an action, but in general I dont think a multi-threaded spinner is something to be avoided, I make heavy use of them in autonomy systems in ROS1 to allow for a bunch of services/callbacks to be triggered without waiting.

orduno · 2019-08-09T16:36:05Z

As per our discussion in the ROS2 Navigation WG, I'm closing the issue. For now, we will rely on the current pattern to enable calling a service within a (service) callback. The current approach is to create an additional node during node construction (or configuration).

Instead of following the pattern proposed on this PR, We'll work upstream on the rclcpp package to enable this feature.

Removing service client nodes from navfn

7d624db

orduno added the 2 - Medium Medium Priority label Jun 27, 2019

orduno added this to the E Turtle Release milestone Jun 27, 2019

orduno requested review from mjeronimo and crdelsey June 27, 2019 21:16

orduno self-assigned this Jun 27, 2019

orduno requested a review from SteveMacenski June 27, 2019 21:17

Fix linter fails

01e729b

orduno requested a review from mkhansenbot July 1, 2019 18:03

Refactored executor creation

f2ad376

orduno modified the milestones: E Turtle Release, July 2019 Jul 15, 2019

crdelsey requested review from bpwilcox and removed request for mkhansenbot and SteveMacenski July 22, 2019 21:31

crdelsey reviewed Jul 24, 2019

View reviewed changes

mjeronimo reviewed Jul 24, 2019

View reviewed changes

bpwilcox approved these changes Jul 24, 2019

View reviewed changes

crdelsey modified the milestones: July 2019, E Turtle Release Jul 29, 2019

SteveMacenski reviewed Jul 29, 2019

View reviewed changes

orduno closed this Aug 9, 2019

orduno deleted the 816_remove_service_client_nodes branch August 9, 2019 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing service client nodes from navfn #897

Removing service client nodes from navfn #897

orduno commented Jun 27, 2019 •

edited

Loading

crdelsey left a comment

crdelsey Jul 23, 2019

crdelsey Jul 23, 2019

bpwilcox Jul 24, 2019

crdelsey Jul 23, 2019

crdelsey Jul 23, 2019

crdelsey commented Jul 24, 2019

mjeronimo Jul 24, 2019

mjeronimo Jul 24, 2019

bpwilcox left a comment

bpwilcox Jul 24, 2019

bpwilcox commented Jul 29, 2019

SteveMacenski Jul 29, 2019

orduno commented Aug 9, 2019


		auto planner = std::make_shared<nav2_navfn_planner::NavfnPlanner>();

		// The planner needs a special executor

Removing service client nodes from navfn #897

Removing service client nodes from navfn #897

Conversation

orduno commented Jun 27, 2019 • edited Loading

Basic Info

Description of contribution in a few bullet points

Future work that may be required in bullet points

crdelsey 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

Choose a reason for hiding this comment

crdelsey commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpwilcox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpwilcox commented Jul 29, 2019

Choose a reason for hiding this comment

orduno commented Aug 9, 2019

orduno commented Jun 27, 2019 •

edited

Loading