-
Notifications
You must be signed in to change notification settings - Fork 18
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
Finish rename of nonholonomic to ackermann, make ackermann vehicles standardized with RMF stack #49
Conversation
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
66ba1cb
to
3e638fb
Compare
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.
tested with the new changes on open-rmf/rmf_demos#86, LGTM!
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.
Looks great! Thanks for the changes, I just have a few minor comments.
rmf_robot_sim_common/include/rmf_robot_sim_common/slotcar_common.hpp
Outdated
Show resolved
Hide resolved
rmf_robot_sim_common/include/rmf_robot_sim_common/slotcar_common.hpp
Outdated
Show resolved
Hide resolved
Apart from @aaronchongth 's comments, the changes look okay. I would be cool to have a |
Signed-off-by: Luca Della Vedova <[email protected]>
Addressed all the changes! I also found a few areas for small cleanups.
Agreed! I think it would be a great new feature to have. For our specific scenario we are not too concerned with this issue yet so I think it's fine to take time to do a good implementation on the |
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.
LGTM
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.
Thanks for the changes, LGTM
…tandardized with RMF stack (#49) * Nonholonomic vehicles now report their state Signed-off-by: Luca Della Vedova <[email protected]> * Cleanup and change nonholonomic to ackermann Signed-off-by: Luca Della Vedova <[email protected]> * Minor cleanup, style Signed-off-by: Luca Della Vedova <[email protected]> * Address feedback Signed-off-by: Luca Della Vedova <[email protected]> Co-authored-by: ddengster <[email protected]> Signed-off-by: Luca Della Vedova <[email protected]>
Bug fix
Fixed bug
Closes #47 and closes #48.
Ackermann vehicles were using custom topics for path requests (even though they use the same data format as normal robots), as well as not reporting their state hence making it impossible for downstream users to monitor them. This PR fixes both the issues, as well as finishing the renaming of nonholonomic -> ackermann drive and cleaning up the code a bit.
To test, with the matching PR in rmf_demos:
ros2 launch rmf_demos_ign test_ackman.launch.xml
And
ros2 run rmf_demos_tasks request_ackmann_path -p rmf_demos_maps -n maps/test_ackman/nav_graphs/0.yaml -s a1 -e c1 -i task11 -r ambulance