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

Nonholonomic slotcar turning fixes, add turning multiplier offset parameter, rename compute_ds variables #41

Merged
merged 6 commits into from
Sep 1, 2021

Conversation

ddengster
Copy link
Contributor

@ddengster ddengster commented Aug 31, 2021

Bug fix

Fixed bugs

  • The nonholonomic slotcar plugin had a problem with it's turning due to the computation of the turn delta, this PR fixes it via using the arcosine of the dot product.

  • Another problem is the decceleration applied to velocities in compute_ds, making our slotcar enter turns from 0 velocity and in turn making the turn look weird. To fix this we added target_velocity as a targeted final velocity as target_s reaches 0.

  • Added _turning_right_angle_mul_offset as an added multiplier applied to the turning radius for right-angled turns within 20 degrees. Eg. A value of 0.5 for _turning_right_angle_mul_offset makes for a multiplier 1.5 when is applied to a 90 degree turning radius, and linearly downscales to 1.0 for a 70 degree turning radius. This compensates for turning acc/deccelerations making turns 'overshoot'.

  • Corrects up some variables named velocities when they were in fact displacements.

  • Removes unneeded variables in various spots

In conjunction with open-rmf/rmf_demos#82

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
redo min_turning_radius computation and add compute_turning_multiplier
remove unneeded variables, renamed variables in compute_ds function

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
add adjustment for right angled turns
remove unneeded param

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
switch to right angle mul offset variable

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
@ddengster ddengster changed the title Nonholonomic slotcar turning fixes, rename compute_ds variables Nonholonomic slotcar turning fixes, add turning multiplier offset parameter, rename compute_ds variables Aug 31, 2021
@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #41 (d55cfac) into main (ebc4492) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main     #41   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files        138     138           
  Lines      12873   12849   -24     
=====================================
+ Misses     12873   12849   -24     
Flag Coverage Δ
tests 0.00% <ø> (ø)

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

Impacted Files Coverage Δ
...bot_sim_ignition_plugins/src/TeleportDispenser.cpp
..._sim_common/include/rmf_robot_sim_common/utils.hpp
...lding_sim_ignition_plugins/src/crowd_simulator.hpp
...on/rmf_robot_sim_ignition_plugins/src/readonly.cpp
...building_sim_common/src/crowd_simulator_common.cpp
...lding_sim_ignition_plugins/src/crowd_simulator.hpp
...lation/rmf_building_sim_common/src/lift_common.cpp
...ion/rmf_building_sim_ignition_plugins/src/door.cpp
...lding_sim_ignition_plugins/src/crowd_simulator.hpp
..._sim_common/include/rmf_robot_sim_common/utils.hpp
... and 254 more

Signed-off-by: ddengster <ed.fan@osrfoundation.org>
@ddengster ddengster marked this pull request as ready for review August 31, 2021 07:29
@@ -60,7 +60,8 @@ double compute_ds(
const double v_max,
const double accel_nom,
const double accel_max,
const double dt);
const double dt,
const double v_target = 0.0);
Copy link
Member

Choose a reason for hiding this comment

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

Nit, there was some discussion here about the compute_ds and compute_desired_rate_of_change being basically the same function, can we take the chance to remove one of the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a line that differs between functions by double the divisor at https://github.com/open-rmf/rmf_simulation/pull/41/files/d55cfacfe9310deef0b6012e1a81ab2c7bb51394#diff-3bd4615902d61050ebeb9f584b212fc5d5ef07c1eed12cb5fa7814dbed7d59a4R41
but you're right let me remove it and test it with the other robots

Copy link
Contributor Author

@ddengster ddengster Sep 1, 2021

Choose a reason for hiding this comment

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

change delayed, will have to tweak the turning_right_angle_mul_offset values again #42

Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Works great! Just a minor comment.
I guess for the future we might want to have a different parameter for the velocity in bends to be different (smaller) from the velocity in a straight path, but we can address that in the future.

@ddengster ddengster merged commit fc7fb26 into main Sep 1, 2021
@ddengster ddengster deleted the fix/motion branch September 1, 2021 03:56
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.

None yet

2 participants