Skip to content

SCurve: fix calculate_path bugs and add comprehensive tests#32730

Merged
rmackay9 merged 5 commits into
ArduPilot:masterfrom
lthall:20260410_FixSCurveCalc
Apr 14, 2026
Merged

SCurve: fix calculate_path bugs and add comprehensive tests#32730
rmackay9 merged 5 commits into
ArduPilot:masterfrom
lthall:20260410_FixSCurveCalc

Conversation

@lthall

@lthall lthall commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix three bugs in SCurve::calculate_path and set_speed_max, add tests
that cover all code paths, and clean up float literal consistency.

Fixes #32724

Bug fixes:

  • Fix sign error in path length acceleration constraint: (L + 4V0tj)
    should be (L - 4V0tj). The old formula inflated the allowed
    acceleration when V0 > 0, producing invalid motion profiles.
  • Add early return when At <= 0. After the sign fix, At can go negative
    when the path is too short for the initial velocity.
  • Fix integer division in set_speed_max: powf(x, 1/3) evaluated the
    exponent as 0, making powf always return 1.0 instead of the cube
    root. This disabled the velocity-based jerk time limit.
  • Guard against divide-by-zero where dP is divided by
    segment[SEG_CONST].end_vel in four places.

Tests:

  • 32 test cases covering all 8 code paths in calculate_path with 4
    inputs per path near transitions and midpoints.
  • Constraint validation replicating the add_segments path structure to
    verify total distance, final velocity, final acceleration, and that
    velocity, acceleration, and jerk limits are not exceeded.

Cleanup:

  • Replace double literals with float literals throughout to avoid
    implicit double-to-float conversions.
  • Fix misleading comment, whitespace, and operator spacing.
Board,antennatracker,blimp,bootloader,copter,heli,plane,rover,sub
CubeOrange,*,*,*,128,128,136,136,128

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

@lthall lthall force-pushed the 20260410_FixSCurveCalc branch from ae85879 to a8e9db7 Compare April 10, 2026 04:11
@lthall lthall changed the title 20260410 fix s curve calc SCurve: fix calculate_path bugs and add comprehensive tests Apr 10, 2026
@lthall lthall requested review from peterbarker and rmackay9 April 10, 2026 04:13
@rmackay9 rmackay9 mentioned this pull request Apr 10, 2026
89 tasks
@amilcarlucas

Copy link
Copy Markdown
Contributor
Binary Name      Text [B]        Data [B]     BSS (B)        Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  --------------  -----------  -------------  ----------------------------  -------------------------
blimp            0 (0.0000%)     0 (0.0000%)  0 (0.0000%)    0 (0.0000%)                                      780972
arducopter       128 (+0.0081%)  0 (0.0000%)  0 (0.0000%)    128 (+0.0080%)                                   374116
ardusub          124 (+0.0087%)  0 (0.0000%)  -4 (-0.0015%)  124 (+0.0086%)                                   530428
arducopter-heli  128 (+0.0081%)  0 (0.0000%)  0 (0.0000%)    128 (+0.0081%)                                   378508
arduplane        124 (+0.0078%)  0 (0.0000%)  -4 (-0.0016%)  124 (+0.0078%)                                   378184
ardurover        124 (+0.0086%)  0 (0.0000%)  4 (+0.0015%)   124 (+0.0085%)                                   512868
antennatracker   0 (0.0000%)     0 (0.0000%)  0 (0.0000%)    0 (0.0000%)                                      800056

@tpwrules

Copy link
Copy Markdown
Contributor

Is this observable in flight?

@rmackay9

Copy link
Copy Markdown
Contributor

Hi @tpwrules, my understanding from a quick chat with Leonard is that it is observable as an overshoot as the vehicle transitions from one waypoint to the next.

@lthall

lthall commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

Is this observable in flight?

I have tired to excite this but it is really difficult. I believe I have seen it once but then could not replicate it. I have spent some time trying but have not been succsessful.

@lthall lthall force-pushed the 20260410_FixSCurveCalc branch from a8e9db7 to ffc489b Compare April 13, 2026 02:47
// add to constant velocity segment to end at the correct position
const float dP = MAX(0.0f, Pend - segment[SEG_DECEL_END].end_pos);
const float t15 = dP / segment[SEG_CONST].end_vel;
const float t15 = is_positive(segment[SEG_CONST].end_vel) ? dP / segment[SEG_CONST].end_vel : 0.0f;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 124 bytes for doing the same check multiple times. You could probably avoid the impact of this by using a function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Addressed in a follow up PR:

#32787

@rmackay9 rmackay9 merged commit f025da2 into ArduPilot:master Apr 14, 2026
114 of 115 checks passed
@Georacer Georacer moved this from Pending to 4.7.0-beta4 in 4.7 Backports Apr 28, 2026
@Georacer Georacer mentioned this pull request Apr 30, 2026
9 tasks
@Georacer

Copy link
Copy Markdown
Contributor

This has been included in 4.7.0-beta4. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: 4.7.0-beta4

Development

Successfully merging this pull request may close these issues.

Copter/Rover: SCurve calculation formula

8 participants