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

Change slotcar control to explicitly open loop #43

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Sep 1, 2021

Bug fix

Supersedes #40, closes #37

Fixed bug

Do explicit open loop control instead of relying on undefined behavior (that broke in the ignition-gazebo version 5.1).

Fix applied

We currently rely on reading the Linear / AngularVelocityCmd components to know what is the current robot's speed, this is equivalent to doing open loop control since the components used to have the command sent in the previous iteration.
However, it was actually an incorrect behavior since those components are supposed to be set to zero before the following simulation step, as is done here.
Switching to closed loop simulation as proposed in #40 doesn't work for dartsim because the high friction caused by our way of moving the robots (move it as a box, sliding it on the floor) causes the velocities to be very low and the robot to barely move, as discussed in ign-gazebo #966(comment).

For these reasons, the only way to have the robots to have the same behavior with both physics engines as before is to switch to an explicit open loop control, where we save the velocities of the previous step and use them to compute the updated velocities in the following step.

Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #43 (428946a) into main (fc7fb26) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            main     #43     +/-   ##
=======================================
  Coverage   0.00%   0.00%             
=======================================
  Files         72     138     +66     
  Lines       6490   12857   +6367     
=======================================
- Misses      6490   12857   +6367     
Flag Coverage Δ
tests 0.00% <ø> (ø)

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

Impacted Files Coverage Δ
...ing_sim_gazebo_plugins/src/thumbnail_generator.cpp
...ation/rmf_robot_sim_gazebo_plugins/src/slotcar.cpp
...lation/rmf_building_sim_common/src/lift_common.cpp
...obot_sim_ignition_plugins/src/TeleportIngestor.cpp
...m_ignition_plugins/src/LightTuning/LightTuning.cpp
...f_simulation/rmf_building_sim_common/src/utils.cpp
...ation/rmf_building_sim_gazebo_plugins/src/lift.cpp
...uilding_sim_gazebo_plugins/src/crowd_simulator.hpp
...building_sim_common/src/crowd_simulator_common.cpp
...robot_sim_gazebo_plugins/src/TeleportDispenser.cpp
... and 188 more

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.

tested with use_tpe:=1 and without, in the test_ackman and office environments, it looks fine to me!

@luca-della-vedova luca-della-vedova merged commit 80b22b9 into main Sep 1, 2021
@luca-della-vedova luca-della-vedova deleted the fix/open_loop_slotcar branch September 1, 2021 07:40
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.

Slotcar in Ignition doesn't move
2 participants