-
Notifications
You must be signed in to change notification settings - Fork 493
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
Fix trajectory unwind bug #1772
Conversation
7b6971d
to
6bc7087
Compare
Codecov ReportBase: 50.32% // Head: 50.36% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1772 +/- ##
==========================================
+ Coverage 50.32% 50.36% +0.04%
==========================================
Files 374 374
Lines 31329 31283 -46
==========================================
- Hits 15762 15751 -11
+ Misses 15567 15532 -35
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Do you mind adding a test that reproduces the fixed issue? |
I can write a couple tests, as part of reviewing this anyway. |
941ea8f
to
df46fbd
Compare
Thanks Andy! |
0dc17bd
to
f9ab99d
Compare
Started writing a test, got distracted by other bugs :/ Will come back to this. Or maybe we merge it as it is, then fix the other bugs and write tests. |
261b900
to
02e95fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK merging this without tests. unwind()
clearly has some other bugs that need to be fixed, anyway. See #1778
We need to wait for one more review, though.
I'm cool with merging as is as well! |
aeb1793
to
a3e26a2
Compare
Okedoke, I think I fixed up the other unwind bugs and got the unwind test case to be successful |
6cc2e01
to
3890357
Compare
c0bcb27
to
26a383f
Compare
d67d8aa
to
a9bebe1
Compare
a9bebe1
to
94e4b32
Compare
3b71654
to
5d213b6
Compare
@mechwiz Can you please rebase this branch, so we can merge it |
5d213b6
to
6afb74f
Compare
Done |
Description
This PR fixes some unwinding/bounds issues for continuous joints.
running_offset
can be off by a multiple of 2PI leading to 2PI jumps in the command for that continous joint. This occurs because in the current implementation, the running_offset is not only being calculated using the offset between the reference state and the unwound reference state but also the offset between the initial waypoint (which may not be unwound) and the unwound current state which is not valid and is like comparing apples and oranges. To fix this, the initial waypoint of the trajectory also has its bounds enforced so that when it's being compared to the reference value, it's comparing apples to apples. Furthermore, I added logic to ensure that the running_offset takes into account when the unwound reference state and the unwound initial waypoint are not on the same side of PI by adding 2PI or subtracting 2PI accordingly.executeAndMonitor
is called, unwinding is not handled properly if there are multiple trajectories within the plan as would happen when executing a task or a serial container from MTC. The way unwinding should work is that the current plan_component's trajectory (item i) should be unwound based on any group-matching previous plan_component's trajectory (item j). The current implementation for some unknown reason is starting at index zero and working up (which is not valid since there could be a matching group plan-component but it might not be the one that precedes component "i") instead of starting at the previous plan compoment and working down.Fixes #1777
@AndyZe
Checklist