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

Admittance with filters #560

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

guihomework
Copy link
Contributor

Re-introduced filters to the admittance_controller as originally done in #382. Improved testing, variable names and comments.

Depends on ros-controls/control_toolbox#152 and ros-controls/control_toolbox#153

Tests currently fail due to compensation already moving the robot after the first update step. This is expected but was not expected in past test. So a discussion should occur to decide how to change the test.

Test currently fails at joint state matching commands, but this
should be expected as there is gravity compensation with a 2.3 kg mass
@guihomework guihomework changed the title Admittance with filters rolling Admittance with filters Apr 3, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2023

Codecov Report

Merging #560 (aefd00d) into master (e7f9962) will decrease coverage by 4.90%.
Report is 352 commits behind head on master.
The diff coverage is 25.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     #560      +/-   ##
==========================================
- Coverage   35.78%   30.88%   -4.90%     
==========================================
  Files         189        7     -182     
  Lines       17570      832   -16738     
  Branches    11592      505   -11087     
==========================================
- Hits         6287      257    -6030     
+ Misses        994      133     -861     
+ Partials    10289      442    -9847     
Flag Coverage Δ
unittests 30.88% <25.91%> (-4.90%) ⬇️

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

Files Changed Coverage Δ
...ontroller/test/test_load_diff_drive_controller.cpp 11.11% <0.00%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 12.66% <7.81%> (ø)
diff_drive_controller/src/odometry.cpp 42.16% <11.11%> (ø)
diff_drive_controller/src/speed_limiter.cpp 46.55% <11.11%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 47.13% <46.94%> (ø)
...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

@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2023

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

@guihomework
Copy link
Contributor Author

@destogl I discover that "allowing edits" means allow others to write to my branch. I did not knot this was possible. Isn't it better to always rebase rather than do a merge from the master (which is then not a straight line merging for the PR anymore) ? I can force-push a rebased PR now if desired to get a cleaner history.

@mergify
Copy link
Contributor

mergify bot commented Jun 4, 2023

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

gwalck added 3 commits August 4, 2023 16:46
New initial pose and matching valid frames to ease checks on GC/wrench
Special parameters for test of first state with no mass
 to avoid deviation form GC
@guihomework
Copy link
Contributor Author

Merging master into this branch introduced a test failure (another one than the known one). This came from some invalid parameters that cannot be tested in on_init because they are only used on_configure

Additionally I should have fixed the other test failing due to gravity compensation. In fact this test (receive_message_and_publish_updated_status) should not pass when there is a mass at eef because gravity compensation (the one of the filter in PR or the hard-coded one in existing code) will necessarily make the posture after one iteration deviate from the start.
Why that particular test passed before is because a smoothing (equivalent to the new LP filter) was hard-coded and applied after gravity compensation, which meant that after one iteration almost no gravity compensation was seen by the eef (0.1 instead 23 N). It makes no sense to filter the gravity compensation, only the noisy input data from the sensor should be filtered and that is what the proposed implementation does (LP then GC filter)

I think CI fails as it relies on 2 other PRs.

@guihomework guihomework marked this pull request as ready for review August 4, 2023 20:48
Copy link
Contributor

mergify bot commented Mar 1, 2024

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

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.

4 participants