Using math::SpeedLimiter on the diff_drive controller. #833

LolaSegura · 2021-05-26T15:49:05Z

Signed-off-by: LolaSegura lsegura@ekumenlabs.com

Summary

This replace the diff_drive controller speed limiter class for the one implemented in ignition math. Part of #810.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

francocipollone

LGTM! I just left a couple of nits.

I will add @scpeters as a reviewer as well

src/systems/diff_drive/DiffDrive.cc

codecov · 2021-05-26T16:36:46Z

Codecov Report

Merging #833 (034eb01) into ign-gazebo3 (7c6c70a) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

@@               Coverage Diff               @@
##           ign-gazebo3     #833      +/-   ##
===============================================
- Coverage        77.97%   77.94%   -0.03%     
===============================================
  Files              216      215       -1     
  Lines            12130    12077      -53     
===============================================
- Hits              9458     9414      -44     
+ Misses            2672     2663       -9

Impacted Files	Coverage Δ
src/systems/diff_drive/DiffDrive.cc	`91.79% <75.00%> (+0.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c6c70a...034eb01. Read the comment docs.

scpeters · 2021-05-27T07:43:44Z

this is looking good!

it's helpful to cross-reference issues from pull requests, so I would add that this is "Part of #810" in the pull request description

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

scpeters · 2021-05-27T18:11:14Z

I noticed that codecov is complaining about test coverage. We are not strict about requiring code coverage, but I think we can easily increase this for this test. I believe the diff_drive system has the following integration test:

https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo3/test/integration/diff_drive_system.cc

The test uses several different world files (search for test/worlds to find lines like this) such as test/worlds/diff_drive.sdf. This world has a plugin that specifies max_acceleration and max_velocity, but doesn't specify min_acceleration or min_velocity. If we added min_acceleration of -1 and min_velocity of ~~0.5~~ -0.5, that will improve the coverage and add symmetry to the test. To get even more coverage and without changing the dynamics of the system we could add a min_jerk value of -Inf and a max_jerk value of Inf (I think that will work).

Since these are easy changes to improve code coverage, do you mind giving them a try? Thanks!

edited to fix typo noted below

LolaSegura · 2021-06-01T13:46:29Z

If we added min_acceleration of -1 and min_velocity of 0.5, that will improve the coverage and add symmetry to the test.

@scpeters here when you mentioned the min_velocity did you meant to write -0.5? Because otherwise it would take the same value as the max_velocity.

scpeters · 2021-06-01T16:41:40Z

If we added min_acceleration of -1 and min_velocity of 0.5, that will improve the coverage and add symmetry to the test.

@scpeters here when you mentioned the min_velocity did you meant to write -0.5? Because otherwise it would take the same value as the max_velocity.

thanks for catching that; I meant to write -0.5

…elocity limits. Added those limits to the .sdf file. Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

francocipollone

ptal at the comments. Also, run codecheck to match style.

francocipollone · 2021-06-07T11:42:20Z

test/integration/diff_drive_system.cc

+    /// \param[in] forward true for testing the forward movement or
+    /// false for testing the backward movement.


Change it to be:

// If `forward` is true, the `max_acceleration` and `max_velocity` properties are tested as the movement is forward, // otherwise `min_acceleraton` and `min_velocity` properties are tested.

francocipollone · 2021-06-07T11:45:16Z

test/integration/diff_drive_system.cc

+
+      msgs::Twist msg;
+
+      // Avoid wheel slip by limiting acceleration


Almost repeated line

francocipollone · 2021-06-07T11:48:39Z

test/integration/diff_drive_system.cc

+      // See <max_velocity> and <max_aceleration> parameters
+      // in "/test/worlds/diff_drive.sdf".
+      // <min_velocity>, <min_aceleration>, <min_jerk> and 
+      // <max_jerk> parameters were also included.


Use

// See <max_velocity>, <min_velocity>, <max_acceleration> and <min_acceleration> parameters // in "/test/worlds/diff_drive.sdf".

instead of

// See <max_velocity> and <max_aceleration> parameters // in "/test/worlds/diff_drive.sdf". // <min_velocity>, <min_aceleration>, <min_jerk> and // <max_jerk> parameters were also included.

francocipollone · 2021-06-07T11:49:41Z

test/integration/diff_drive_system.cc

+      // <min_velocity>, <min_aceleration>, <min_jerk> and 
+      // <max_jerk> parameters were also included.
+      test::Relay velocityRamp;
+      int kMovementDirection = (forward ? 1 : -1);


it can be const

use movementDirection instead.

francocipollone · 2021-06-07T11:54:53Z

test/integration/diff_drive_system.cc

+      double a = (v - v0) / t;
+
+      // Max velocities/accelerations expectations.
+      // Moving time.


// Moving time

It shouldn't be here.
This was related to double t = 3.0 statement.

Also:
Remove these comments before the if and the else and add one comment before the declaration of the previous variables (t,d,v0,v,a) stating something like:

// Verify velocity and acceleration boundaries.

francocipollone · 2021-06-07T11:59:44Z

test/integration/diff_drive_system.cc

@@ -608,4 +634,4 @@ TEST_P(DiffDriveTest, Pose_VCustomTfTopic)

 // Run multiple times
 INSTANTIATE_TEST_SUITE_P(ServerRepeat, DiffDriveTest,
-    ::testing::Range(1, 2));
+    ::testing::Range(1, 2));


Missing a new line at the end of the file?

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

francocipollone · 2021-06-07T14:06:47Z

@LolaSegura . I've tried it locally.

It seems that just adding the min_jerk and max_jerk properties are causing a miss behavior in the diff drive when moving forward(probably backward too).
Leaving those properties commented out the test moving forward works fine.
It is failing only one test when moving backward:

Expected: (expectedLowerPosition.Y()) < (expectedGreaterPosition.Y()), actual: 0.710173 vs 1.40412e-12

Re-check the desire angle you are publishing.

…y and acceleration limits to the diff_drive_custom_topics.sdf Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

LolaSegura · 2021-06-08T19:54:32Z

If we added min_acceleration of -1 and min_velocity of 0.5 -0.5, that will improve the coverage and add symmetry to the test

I added the <min_velocity> and <min_acceleration> to the diff_drive.sdf and the diff_drive_custom_topics.sdf. With this the test its working fine.

To get even more coverage and without changing the dynamics of the system we could add a min_jerk value of -Inf and a max_jerk value of Inf (I think that will work).

I try adding this limits as well, but I encounter a problem because when being parsed the limits where interpreted as zero, causing the test to fail. An option would be setting the <max_jerk> with a big value and the <min_jerk> with a small one to get more test coverage.

francocipollone · 2021-06-10T14:03:45Z

It is ready for review @scpeters

…iffdrive

scpeters

To get even more coverage and without changing the dynamics of the system we could add a min_jerk value of -Inf and a max_jerk value of Inf (I think that will work).

I try adding this limits as well, but I encounter a problem because when being parsed the limits where interpreted as zero, causing the test to fail. An option would be setting the <max_jerk> with a big value and the <min_jerk> with a small one to get more test coverage.

I'll look into this briefly

test/integration/diff_drive_system.cc

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

…iffdrive

scpeters · 2021-06-17T21:51:33Z

To get even more coverage and without changing the dynamics of the system we could add a min_jerk value of -Inf and a max_jerk value of Inf (I think that will work).

I try adding this limits as well, but I encounter a problem because when being parsed the limits where interpreted as zero, causing the test to fail. An option would be setting the <max_jerk> with a big value and the <min_jerk> with a small one to get more test coverage.

thanks for testing this; I just looked into it a bit deeper and found that it is a parsing issue with libsdformat (gazebosim/sdformat#261). We can leave out the jerk tests for now

…iffdrive

Using math::SpeedLimiter on the diff_drive controller.

ea869a4

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

LolaSegura requested a review from francocipollone May 26, 2021 15:49

LolaSegura requested a review from chapulina as a code owner May 26, 2021 15:49

github-actions bot added the 🏰 citadel Ignition Citadel label May 26, 2021

francocipollone reviewed May 26, 2021

View reviewed changes

francocipollone requested a review from scpeters May 26, 2021 16:05

scpeters mentioned this pull request May 27, 2021

Use math::SpeedLimiter inside controllers #810

Closed

Added const modifiers as suggested.

d50c816

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

Change the diff_drive_system test to cover min_acceleration and min_v…

393396b

…elocity limits. Added those limits to the .sdf file. Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

francocipollone self-requested a review June 4, 2021 20:33

francocipollone suggested changes Jun 7, 2021

View reviewed changes

Corrections to match code style.

d9c2742

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

Deleted jerk limits on the diff_drive.sdf file. Added minimun velocit…

fef7844

…y and acceleration limits to the diff_drive_custom_topics.sdf Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

Merge branch 'ign-gazebo3' into LolaSegura/math_speedlimiter_inside_d…

d40c3f5

…iffdrive

scpeters reviewed Jun 15, 2021

View reviewed changes

test/integration/diff_drive_system.cc Outdated Show resolved Hide resolved

test/integration/diff_drive_system.cc Outdated Show resolved Hide resolved

Deleted if/else of the velocity and acceleration limits expectations.

b3bf320

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

LolaSegura requested a review from scpeters June 17, 2021 12:39

Merge branch 'ign-gazebo3' into LolaSegura/math_speedlimiter_inside_d…

7bed825

…iffdrive

Merge branch 'ign-gazebo3' into LolaSegura/math_speedlimiter_inside_d…

034eb01

…iffdrive

scpeters approved these changes Jun 17, 2021

View reviewed changes

scpeters mentioned this pull request Jun 17, 2021

Supporting parsing "inf", "+inf", "-inf" gazebosim/sdformat#261

Closed

scpeters merged commit 7876ca3 into ign-gazebo3 Jun 18, 2021

scpeters deleted the LolaSegura/math_speedlimiter_inside_diffdrive branch June 18, 2021 04:24

scpeters mentioned this pull request Jun 24, 2021

4 to 5 #877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using math::SpeedLimiter on the diff_drive controller. #833

Using math::SpeedLimiter on the diff_drive controller. #833

LolaSegura commented May 26, 2021 •

edited

Loading

francocipollone left a comment

codecov bot commented May 26, 2021 •

edited

Loading

scpeters commented May 27, 2021

scpeters commented May 27, 2021 •

edited

Loading

LolaSegura commented Jun 1, 2021

scpeters commented Jun 1, 2021

francocipollone left a comment

francocipollone Jun 7, 2021

francocipollone Jun 7, 2021

francocipollone Jun 7, 2021

francocipollone Jun 7, 2021

francocipollone Jun 7, 2021

francocipollone Jun 7, 2021

francocipollone Jun 7, 2021

francocipollone commented Jun 7, 2021

LolaSegura commented Jun 8, 2021

francocipollone commented Jun 10, 2021

scpeters left a comment

scpeters commented Jun 17, 2021

		/// \param[in] forward true for testing the forward movement or
		/// false for testing the backward movement.


		msgs::Twist msg;

		// Avoid wheel slip by limiting acceleration

Using math::SpeedLimiter on the diff_drive controller. #833

Using math::SpeedLimiter on the diff_drive controller. #833

Conversation

LolaSegura commented May 26, 2021 • edited Loading

Summary

Checklist

francocipollone left a comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2021 • edited Loading

Codecov Report

scpeters commented May 27, 2021

scpeters commented May 27, 2021 • edited Loading

LolaSegura commented Jun 1, 2021

scpeters commented Jun 1, 2021

francocipollone left a comment

Choose a reason for hiding this comment

francocipollone Jun 7, 2021

Choose a reason for hiding this comment

francocipollone Jun 7, 2021

Choose a reason for hiding this comment

francocipollone Jun 7, 2021

Choose a reason for hiding this comment

francocipollone Jun 7, 2021

Choose a reason for hiding this comment

francocipollone Jun 7, 2021

Choose a reason for hiding this comment

francocipollone Jun 7, 2021

Choose a reason for hiding this comment

francocipollone Jun 7, 2021

Choose a reason for hiding this comment

francocipollone commented Jun 7, 2021

LolaSegura commented Jun 8, 2021

francocipollone commented Jun 10, 2021

scpeters left a comment

Choose a reason for hiding this comment

scpeters commented Jun 17, 2021

LolaSegura commented May 26, 2021 •

edited

Loading

codecov bot commented May 26, 2021 •

edited

Loading

scpeters commented May 27, 2021 •

edited

Loading