-
Notifications
You must be signed in to change notification settings - Fork 132
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 nav2_gps_waypoint_follower_demo #16
Conversation
...aypoint_follower_demo/include/nav2_gps_waypoint_follower_demo/gps_waypoint_follower_demo.hpp
Show resolved
Hide resolved
nav2_gps_waypoint_follower_demo/params/city_world_gps_waypoints.yaml
Outdated
Show resolved
Hide resolved
nav2_gps_waypoint_follower_demo/params/city_world_gps_waypoints.yaml
Outdated
Show resolved
Hide resolved
nav2_gps_waypoint_follower_demo/src/gps_waypoint_follower_demo.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all honesty after reading this, it probably would have been more simple in python, but I'll never argue with more C++ 😄
This looks good to me. Lets merge it with the nav2 changes for GPS (in case we need to make some update here from that review process).
I know this is just a demo, so there's no expectation that this is production-used code, but it would be nice to figure out a way to have this run multiple times. Right now it issues the waypoint following task from parameters on startup and then exits once done. It would be nice, but not required, if this was able to take multiple requests.
I debate to myself if this is worth it. Someone that wants to do this 'for real' is going to want to add some more features like start/stop/pause/new waypoints/etc (basically becoming itself an action server, which would then need another-another thing to run it...). So is it worth spending time on making this slightly more complete as a demo pkg? Probably not...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, still need those simulation files so that people can use it to do their demo thing.
I think with Python the explicity might have been decreased. But I believe with current explicit form this tutorial will be straightforward to follow and will be convertible to Python if someone needs that.
It is very well worth it, but I believe if we do that we will be aiming for application code rather than demo code. We have showcased core tools in order to use gps waypoint following. It is then up to user whether they just want to try out this or taking it next step as aiming an application following the sample code given here. |
@@ -0,0 +1,29 @@ | |||
material Dumpster/Diffuse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you uploading all of these files? Are these not provided by gazebo that you were just using? They'll be downloaded on use from gazebo index, you don't need to upload these here unless you created them or had to manually download them youself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of them are provided in gazebo_models
but some are not, I think to ensure that gazebo world will properly launch its just better to provide all models used in the world file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ones that are, can we not include them here? I don't think there's a need to duplicate everything, that's alot of "stuff" to clone. I thought by including a model://
it would look in the model store and download new things if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I know if Gazebo could not find a model it will then run to get models from gazebo_models and download them to ~/.gazebo/models
.
Edit; Available models that are already in above repo, has been removed.
Fair enough. Do you think there's an application code to be built from this at a later time? I don't have a clear vision of what that would look like to be sufficiently general that wouldn't largely just turn into another action server relaying points to the WP action server. |
What I meant by application was, |
Got it, but you don't see this project becoming something like that. That's kind of what I was fishing for. If we thought we could make this "follower operations" server could/should be generalized to that level, perhaps its worth considering this for the main stack. But this is good for now with the docs to match. We can always elevate this into Nav2 itself if we like in the future. |
It can be elevated to something like that with some effort. But how to extract GPS waypoints is the question here, if it was to be added to main stack. |
I suppose we don't really have an analog for that for just normal waypoints, other than the rviz plugin which would work with with GPS (since the rviz displays are natively using the navsat transform to assign map goals). We can leave it as demo code. What you have here now seems good. There's a couple of models but that's much more reasonable to keep. We'll revisit this once the main PR is ready to be merged but this looks good to me. |
namespace nav2_gps_waypoint_follower_demo | ||
{ | ||
|
||
GPSWaypointCollector::GPSWaypointCollector() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPS Waypoint Publisher probably seems like a better name. You could also publish the oriented GPS message too rather than only logging it. That might be helpful so that they can be stored to a vector and then dumped to a file for use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Publishing will make it hard for user as he wont have control on when to trigger publisher. The logging is simple, the user can go to desired location then trigger just by hitting a Enter and receive the current waypoint at terminal. But storing at vector after each keyboard hit makes sense
Edit; oh okay you meant publish after the key hit ? , but still I dont see how publishing will benefit this, as we finally need to store waypoints at disk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the name "Collector" isn't really accurate since its not collecting things, its just logging them. So maby GPSWaypointLogger
is a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant publish on a topic as well as log, so that both are around.
Superseded! |
Related original ticket; ros-navigation/navigation2#1631
Add example package
nav2_gps_waypoint_follower_demo
that demonstrates usage ofnav2_gps_waypoint_follower
introduced here.