Skip to content
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

Fix velocity control for slotcar in Ignition TPE #40

Closed
wants to merge 3 commits into from

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Aug 30, 2021

Bug fix

Fixed bug

Slotcar in Ignition was relying on an incorrect behavior for velocity control. Specifically it was reading JointVelocityCmd to estimate the current robot's velocity, however that component is meant as a write only and the behavior of it being read back to the last command was actually not intended, and it has been fixed with it now always being read as 0.

Fix applied

Now slotcar reads the velocity from the model's canonical link and uses it for velocity control, unfortunately when using dartsim as a physics engine the robot's friction slows down the robots a lot and they barely move, however this PR fixes behavior under TPE.

To test:
ros2 launch rmf_demos_ign office.launch.xml use_tpe:=1

Robots will move after the PR but will be stuck before. Note that doors in TPE don't work since they require an old PR to be picked up again and merged.

Requires ign-physics #289 so it should be tested with a source build of Ignition with the branch above (until it gets merged)

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #40 (8d336a0) into main (fc7fb26) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main     #40   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         72      72           
  Lines       6490    6510   +20     
=====================================
- Misses      6490    6510   +20     
Flag Coverage Δ
tests 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...tion/rmf_robot_sim_common/src/dispenser_common.cpp
...ation/rmf_robot_sim_gazebo_plugins/src/slotcar.cpp
...ation/rmf_building_sim_gazebo_plugins/src/door.cpp
...building_sim_common/src/crowd_simulator_common.cpp
...on_plugins/src/toggle_charging/toggle_charging.cpp
...bot_sim_ignition_plugins/src/TeleportDispenser.cpp
...on/rmf_robot_sim_ignition_plugins/src/readonly.cpp
...on/rmf_robot_sim_ignition_plugins/src/readonly.cpp
..._building_sim_gazebo_plugins/src/toggle_floors.cpp
..._robot_sim_gazebo_plugins/src/TeleportIngestor.cpp
... and 122 more

Signed-off-by: Luca Della Vedova <[email protected]>
Copy link
Contributor

@ddengster ddengster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked with the ignition-physics PR, our robots* are moving again! Thanks!

@luca-della-vedova
Copy link
Member Author

This fix still doesn't work for dartsim, might have to revert to an "open loop" style of control, closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants