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

Compute velocity errors when using an effort command interface #679

Conversation

tingelst
Copy link
Contributor

This PR is in reference to the discussion in issue #673 (comment). The changes in 8e6697b introduced a bug in that the velocity error terms was not computed when using an effort command interface, resulting in a wobbly/jerky motion. This PR fixes this issue.

@bmagyar
Copy link
Member

bmagyar commented Jun 20, 2023

Do you have a good idea for adding a test for this? To avoid making this mistake in the future

@codecov-commenter
Copy link

Codecov Report

Merging #679 (2e0da5d) into master (e7f9962) will increase coverage by 0.75%.
The diff coverage is 34.91%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   35.78%   36.53%   +0.75%     
==========================================
  Files         189        7     -182     
  Lines       17570      676   -16894     
  Branches    11592      357   -11235     
==========================================
- Hits         6287      247    -6040     
+ Misses        994      134     -860     
+ Partials    10289      295    -9994     
Flag Coverage Δ
unittests 36.53% <34.91%> (+0.75%) ⬆️

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

Impacted Files Coverage Δ
...ontroller/test/test_load_diff_drive_controller.cpp 11.11% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp 42.16% <11.11%> (ø)
diff_drive_controller/src/speed_limiter.cpp 46.55% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 17.62% <12.08%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 47.09% <46.88%> (ø)
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <100.00%> (ø)
...troller/include/diff_drive_controller/odometry.hpp 100.00% <100.00%> (ø)

... and 189 files with indirect coverage changes

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

I think this is fine. But I'm not sure if #491 really introduced this, the velocity error wasn't calculated before with effort command interface?

@tingelst
Copy link
Contributor Author

The velocity error was calculated before, but it was done directly in the computeCommand as seen here.

@christophfroehlich
Copy link
Contributor

christophfroehlich commented Jun 27, 2023

The velocity error was calculated before, but it was done directly in the computeCommand as seen here.

I see, sorry. I thought I removed the redundant difference calculation only, but hadn't had the effort interface in mind.
Let's add a test for that! Effort controller tests are active again with the jtc-features branch, see #607

@christophfroehlich
Copy link
Contributor

I've written a test and made the PR #690 against jtc-features branch, where the effort tests are active already. It failed before I cherry-picked the commit from this PR. Let the maintainers decide if we merge the test directly to master or wait for jtc-features to be merged.

@mergify
Copy link
Contributor

mergify bot commented Jul 17, 2023

This pull request is in conflict. Could you fix it @tingelst?

@christophfroehlich
Copy link
Contributor

@destogl this is already included in #705 (I squashed the changes, this is why it shows new commits but no changes)

@destogl
Copy link
Member

destogl commented Jul 17, 2023

OK, I'll then close this, OK?

@destogl destogl closed this Jul 17, 2023
bmagyar pushed a commit that referenced this pull request Jul 17, 2023
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
(cherry picked from commit 2e0da5d)
(cherry picked from commit 12d46f2)
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
(cherry picked from commit 2e0da5d)
(cherry picked from commit 12d46f2)
JafarAbdi pushed a commit to JafarAbdi/ros2_controllers that referenced this pull request Jul 20, 2023
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.

5 participants