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

Fix reverse motions the have different signs of v0 and v1 #13

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SungChiCHIANG
Copy link

@SungChiCHIANG SungChiCHIANG commented Aug 4, 2023

Currently, the calculation of a reverse motion may get the wrong result when the signs of v0 and v1 are different. For example, consider an input:

    let constraints = SCurveConstraints {
        max_jerk: 3.,
        max_acceleration: 2.0,
        max_velocity: 3.,
    };
    let start_conditions = SCurveStartConditions {
        q0: 0.,
        q1: 10.,
        v0: -1.,
        v1: 2.,
    };
    let input = SCurveInput {
        constraints,
        start_conditions,
    };

And the result profile will be like:
image

But when a reverse version of the above input is calculated:

    let constraints = SCurveConstraints {
        max_jerk: 3.,
        max_acceleration: 2.0,
        max_velocity: 3.,
    };
    let start_conditions = SCurveStartConditions {
        q0: 0.,
        q1: -10.,
        v0: 1.,
        v1: -2.,
    };
    let input = SCurveInput {
        constraints,
        start_conditions,
    };

The result profile will be like:
image

After the modification, the result profile will be like the reverse version of the original profile, as expected:
image

Two test cases are added as well to confirm that two opposite start conditions will get the same time intervals.

Sung-Chi CHIANG added 2 commits August 3, 2023 00:16
velocity of a reverse motion have different signs
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 79.31% and project coverage change: +1.23% 🎉

Comparison is base (0e7dc6f) 79.37% compared to head (43178d8) 80.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
+ Coverage   79.37%   80.60%   +1.23%     
==========================================
  Files           1        1              
  Lines         543      593      +50     
  Branches       95      107      +12     
==========================================
+ Hits          431      478      +47     
+ Misses         63       52      -11     
- Partials       49       63      +14     
Files Changed Coverage Δ
src/lib.rs 80.60% <79.31%> (+1.23%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SungChiCHIANG
Copy link
Author

Hi, should I just add some unit tests to boost up the coverage rate? I can see that SCurveInput::is_trajectory_feasible() is not covered yet.

@marcbone
Copy link
Owner

marcbone commented Aug 8, 2023

First of all, thank you very much for your PR! I am always happy when somebody is opening a PR. I didnt had time yet to look into your PR in detail and you can expect it to take a while. But at a first glimpse it looks like you added a test for the bug you are fixing, so I am more than happy with that. But of course more tests are always appreciated 😅 . Also keep in mind that code coverage for rust based projects is calculated in weird ways. So dont worry about it going down a bit.

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.

2 participants