[JTC] Configurable joint positon error normalization behavior #491
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
requested review from
anfemosa,
aprotyas,
bmagyar,
destogl,
jackcenter,
mcbed and
peterdavidfagan
destogl
reviewed
Jan 9, 2023
joint_trajectory_controller/src/joint_trajectory_controller_parameters.yaml
Show resolved
Hide resolved
joint_trajectory_controller/test/test_trajectory_controller.cpp
Outdated
Show resolved
Hide resolved
| EXPECT_NEAR(points[0][1], state_msg->desired.positions[1], allowed_delta); | ||
| EXPECT_NEAR(points[0][2], state_msg->desired.positions[2], 3 * allowed_delta); | ||
|
|
||
| // no normalization of position error |
There was a problem hiding this comment.
Shouldn't this here be normalized?
There was a problem hiding this comment.
The comments were misleading, maybe it's clearer now
|
This pull request is in conflict. Could you fix it @christophfroehlich? |
bmagyar
approved these changes
Jan 19, 2023
Codecov Report
@@ Coverage Diff @@
## master #491 +/- ##
==========================================
- Coverage 35.78% 32.48% -3.31%
==========================================
Files 189 7 -182
Lines 17570 665 -16905
Branches 11592 357 -11235
==========================================
- Hits 6287 216 -6071
+ Misses 994 157 -837
+ Partials 10289 292 -9997
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@Mergifyio backport humble |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
|
@Mergifyio backport humble |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Apr 23, 2023
* Use the same variables for state feedback and PID loop * Make joint_error normalization configurable * Activate test for only velocity controller * Allow ff_velocity_scale=0 without deprecated warning * Add test for setpoint due to normalized position error * Update comments in test Co-authored-by: Bence Magyar <[email protected]> (cherry picked from commit 67e5d4d)
bmagyar
pushed a commit
that referenced
this pull request
Apr 23, 2023
…579) * Use the same variables for state feedback and PID loop * Make joint_error normalization configurable * Activate test for only velocity controller * Allow ff_velocity_scale=0 without deprecated warning * Add test for setpoint due to normalized position error * Update comments in test Co-authored-by: Bence Magyar <[email protected]> (cherry picked from commit 67e5d4d) Co-authored-by: Christoph Fröhlich <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses an optional configuration if the position error used as input for the PID controller should be normalized between -pi/pi or not (the latter is the default).
This normalization is useful if endless rotary joints are controlled, where always the shortest relative distance is of interest. But if linear joints or limited rotary joints are used, it might be very dangerous.
I'm open for suggestions to a better naming of the parameter, and if it should be situated below the gain-structure or in a separate one.