Nav2 Planner + Navfn plugin

[SteveMacenski]

Major items

• removed nav2_msgs/Plan in favor of nav_msgs/Plan because they're basically the same and nav_msgs/Plan included the PoseStamped which is more general for model based trajectory planners
• Created plugin navfn planner
• created general planner server
• updated relevant tests and launch files
• Refined global planner and recovery abstract headers as needed

[SteveMacenski]

@mkhansen-intel @crdelsey this is analogous to the controller PR but I think with some updates I think are better (but open for discussion) EI

• no configure and initialize, just 'configure'
• the nav2_planner's node is planner_server (whereas I'd like nav2_controller to be controller_server)
• Passing in name for logging to use (just like us mere humans, even plugins want to feel connected to their identity), costmap raw pointer, and tf buffer in case we would like them to use another buffer different than the costmap

Should you also agree, I'd like the controller PR to mirror in those respects

[mkhansenbot]

Overall, this looks really good. I have a few questions / change requests.

One that it wouldn't let me make inline is - can we get rid of the /test/.gitignore file? It should be removed.

[mkhansenbot]

@orduno - Are these TODO's still relevant? It seems like the exception should already provide this information

[SteveMacenski]

Travis failed on nav2_utilS in package xml, not sure how everything else passed... perhaps our rosdep uses the -q flag to silence errors and travis didnt? I feel like that should have been caught by all CI builds. Either way fixed now but mentioning it for folks to cache in their brains

[SteveMacenski]

@mkhansen-intel do you have thoughts on the 3 bullets in my first comment? I don't want to steam roll those changes if there's any other thoughts on the table

[mkhansenbot]

I agree with those bullets. See my comment above about documenting the 'how to' of using this plugin interface. Can you add a few sentences to the nav2_planner README for that?

[SteveMacenski]

@mkhansen-intel will do

@bpwilcox can you re-review so I can get this in and move onto recovery plugin-izing this afternoon?

[ghost]

Travis failed on nav2_utilS in package xml, not sure how everything else passed... perhaps our rosdep uses the -q flag to silence errors and travis didnt?

Looks like there is a bug in the CircleCI config. If I read it right, it isn't actually running rosdep on our workspace - just the underlay. I'll take a stab at fixing it.

[ghost]

Passing in name for logging to use (just like us mere humans, even plugins want to feel connected to their identity),

Wouldn't the plugin already know it's name? It could be compiled into the plugin.

costmap raw pointer,

I'm ok with raw pointers in some cases, but, by and large, ROS2 doesn't use them. I tried once to use raw pointers in some places in the code, but it caused more pain than it was worth. Everytime I wanted to make a call into rclcpp, I had to create a shared ptr out of it. We'd be swimming upstream and I don't think it's worth the effort for this.

and tf buffer in case we would like them to use another buffer different than the costmap

@yathartha3 's not going to like this. Didn't you ask him to remove this from the controller interface? ;-)

[SteveMacenski]

Wouldn't the plugin already know it's name? It could be compiled into the plugin.

Not if you have many of them, you want unique names.

The costmap is still a shared_ptr, but rather than giving the plugins a shared_ptr I just give them the raw poitner from the shared ptr. Since the server owns the costmap and it'll destroy it, there's not a huge reason to give the plugin a smart pointer. It wouldn't hurt either, but it was causing me trouble is it was just the path of least resistance

.... oh sh*t, I did. Uh, um, er, what do you think? TF buffer or no TF buffer

[ghost]

Not if you have many of them, you want unique names.

Oh. You want to be able to load multiple instances of the same plugin within the planner_server? In that case a name would be appropriate. Do you have an idea of why you'd want multiple instances of the same planner? Even if not, it would be possible to do with this architecture, so I agree, a name would still be a good idea.

The costmap is still a shared_ptr, but rather than giving the plugins a shared_ptr I just give them the raw poitner from the shared ptr. Since the server owns the costmap and it'll destroy it, there's not a huge reason to give the plugin a smart pointer. It wouldn't hurt either, but it was causing me trouble is it was just the path of least resistance

Do you recall what the trouble was? It usually seems easier to me to stick with shared_ptrs and not fight the system. In this case, it might not matter though. We don't really expect users to interact with costmap as a node, so they probably don't need to pass it to any rclcpp calls.

I have a slight preference for shared_ptr since everywhere else somebody gets or uses a pointer in ROS2, it's a shared_ptr. Just seems more consistent.

.... oh sh*t, I did. Uh, um, er, what do you think? TF buffer or no TF buffer

I don't have a use case in mind for a separate TF buffer. Did you have something in mind?

[yathartha3]

@SteveMacenski

.... what do you think? TF buffer or no TF buffer

I re-introduced the TF buffer, so no worries.

[SteveMacenski]

I suppose wanting to load multiple of the same -- I took inspiration from that from the costmap layer API, but maybe doesnt make sense in this context. I could change it to have a virtual method for the plugin to implement called getName() that would return the name for logging / debug purposes instead. That would be OK

not fight the system

Viva la revolución. But seriously, I don't remember off hand. If you have a strong preference for giving it the shared_ptr, I can take a look at it again

[ghost]

I suppose wanting to load multiple of the same -- I took inspiration from that from the costmap layer API, but maybe doesnt make sense in this context. I could change it to have a virtual method for the plugin to implement called getName() that would return the name for logging / debug purposes instead. That would be OK

If we're already doing it this way elsewhere, lets leave the name in. I don't have a good reason to be inconsistent.

Viva la revolución. But seriously, I don't remember off hand. If you have a strong preference for giving it the shared_ptr, I can take a look at it again

I'm ok with using bare pointers internally where appropriate, but I think we should follow convention and use the shared_ptr on the public interface if not too inconvenient.

[SteveMacenski]

@crdelsey made the changes requested

I don't have a good reason to be inconsistent.

I'm not sure its inconsistent, but it certainly doesnt hurt if someone wants to do something wacky.

Lets leave it as a shared_ptr for now, and I'll file a ticket to audit the use of raw pointers

[mkhansenbot]

I was going to chime in on the shared_ptr vs raw ptr and agree with @crdelsey that we should use a shared_ptr. I think this is now agreed?

[SteveMacenski]

I filed this ticket to cover that #1114

I'll plan to take another stab at it when I do the recovery server work

[SteveMacenski]

@crdelsey I think a very reasonable next update here would be to move to the shared_ptr & potentially remove the name field. @yathartha3 then wouldnt need to add it to the local planner.

[SteveMacenski]

I now remember why TF buffer: so both that it can have another TF buffer and future proofing for if/when we remove the costmap object from the direct process, it'll require the buffer and that way we can change the API and the plugins wouldnt need to do a whole lot to migrate