Skip to content

Compute velocity errors when using an effort command interface#679

Closed
tingelst wants to merge 2 commits intoros-controls:jtc-featuresfrom
tingelst:jtc-compute-velocity-error-effort-interface
Closed

Compute velocity errors when using an effort command interface#679
tingelst wants to merge 2 commits intoros-controls:jtc-featuresfrom
tingelst:jtc-compute-velocity-error-effort-interface

Conversation

@tingelst
Copy link
Copy Markdown
Contributor

@tingelst tingelst commented Jun 20, 2023

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
Copy Markdown
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
Copy Markdown

codecov-commenter commented Jun 20, 2023

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

christophfroehlich
christophfroehlich reviewed Jun 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@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
Copy Markdown
Contributor Author

tingelst commented Jun 27, 2023

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

@christophfroehlich
Copy link
Copy Markdown
Member

christophfroehlich commented Jun 27, 2023 •
edited
Loading

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 christophfroehlich mentioned this pull request Jun 29, 2023
Merged
@christophfroehlich
Copy link
Copy Markdown
Member

christophfroehlich commented Jun 29, 2023

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.

This was referenced Jul 14, 2023
Closed
Merged
christophfroehlich pushed a commit to christophfroehlich/ros2_controllers that referenced this pull request Jul 17, 2023
@tingelst @christophfroehlich
Compute velocity errors when using an effort command interface (ros-c…
27641e5 
…ontrols#679)

(cherry picked from commit 2e0da5d)
@destogl destogl changed the base branch from master to jtc-features July 17, 2023 16:27
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jul 17, 2023

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

@christophfroehlich
Copy link
Copy Markdown
Member

christophfroehlich commented Jul 17, 2023

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

@destogl
Copy link
Copy Markdown
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
@tingelst @bmagyar
Compute velocity errors when using an effort command interface (#679)
12d46f2 
(cherry picked from commit 2e0da5d)
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
@tingelst @mergify
Compute velocity errors when using an effort command interface (#679)
194e76c 
(cherry picked from commit 2e0da5d)
(cherry picked from commit 12d46f2)
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
@tingelst @mergify
Compute velocity errors when using an effort command interface (#679)
7ca381b 
(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
@tingelst @JafarAbdi
Compute velocity errors when using an effort command interface (ros-c…
45373af 
…ontrols#679)

(cherry picked from commit 2e0da5d)
(cherry picked from commit 12d46f2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christophfroehlich christophfroehlich christophfroehlich left review comments

@bmagyar bmagyar Awaiting requested review from bmagyar

@destogl destogl Awaiting requested review from destogl

@duringhof duringhof Awaiting requested review from duringhof

@jackcenter jackcenter Awaiting requested review from jackcenter

@livanov93 livanov93 Awaiting requested review from livanov93

@mhubii mhubii Awaiting requested review from mhubii

@VanshGehlot VanshGehlot Awaiting requested review from VanshGehlot

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tingelst @bmagyar @codecov-commenter @christophfroehlich @destogl