Avoid 32-bit overflow in speed profile setup; unsigned division in step timing#138
Merged
Merged
Conversation
…n step timing Three small robustness/efficiency changes to the LINEAR_SPEED math: 1. Compute steps_to_brake directly from speed (like steps_to_cruise) instead of steps_to_cruise * accel / decel. The old form overflowed 32-bit long when microsteps*speed^2/2 > 2^31 (e.g. 2400 rpm at 1:128, where it produced a negative value so the motor never braked), and its double truncation understated the braking distance at low speeds - the deceleration-side sibling of #93, where steps_to_cruise truncates to 0 and braking was skipped entirely. 2. Compare move time against steps_remaining * step_pulse in float in the CONSTANT_SPEED path, avoiding overflow for moves longer than ~35 minutes total, and guard the subsequent division against steps_remaining == 0. 3. Rewrite the deceleration recurrence with a positive divisor (c += 2c/(4n-1) instead of c -= 2c/(-4n+1), identical under C truncating division) so both ramps use unsigned division. Verification (all on actual library code): - 3000-case sweep (rpm x accel x decel x microstep x distance) of full pulse sequences vs master: bit-identical in all 2702 cases where the old math did not truncate/overflow; all 298 differing cases are accel != decel configs where the old code understated steps_to_brake (including braking skipped entirely); with accel == decel all cases are bit-identical. - Issue #93 UnitTest scenario still passes (rpm=6: 9,999,400us vs 10,000,000us expected). - ATmega328P cycle benchmark (simavr, 40-step ramps, -Os): accelerating step 749 -> 731 cycles, decelerating step 812 -> 721 cycles, with bit-identical outputs. Flash for UnitTest on uno: +44 bytes (once-per-move float setup in startMove). - UnitTest builds clean for uno, adafruit_feather_m0, nodemcuv2, esp32dev, teensylc. Agent: claude-fable-5 (Claude Code 2.1.153); max context and thinking level not exposed to the agent Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up to #137, addressing the remaining robustness items found while reviewing the LINEAR_SPEED implementation.
Changes
1.
steps_to_brakecomputed directly from speed (wassteps_to_cruise * accel / decel)The old form had two problems:
microsteps·v²/2regardless of accel/decel, which exceeds 2³¹ for extreme configs (e.g. 2400 rpm at 1:128: old result = −1,989,672, sosteps_remaining <= steps_to_brakenever became true and the motor never braked).steps_to_cruisetruncates toward 0 and the error propagates intosteps_to_brake. With rpm=6, accel=32000, decel=100 at 1:128, the old code computedsteps_to_brake = 0— braking skipped entirely despite decel=100 requiring 2 full steps of ramp-down. This is the deceleration-side sibling of Incorrect timing at slow RPMs with acceleration #93.The new form mirrors the
steps_to_cruisecalculation:microsteps * (speed * speed / (2 * decel)).2. CONSTANT_SPEED time check compared in float —
time > steps_remaining * step_pulseoverflowed 32-bitlongfor moves totaling more than ~35 minutes; also guards the division againststeps_remaining == 0.3. Deceleration recurrence rewritten with a positive divisor —
c += (2c + rest)/(4n−1)is bit-for-bit identical to the oldc −= (2c + rest)/(−4n+1)under C truncating division, and lets both ramps use unsigned 32-bit division (__udivmodsi4directly instead of the signed__divmodsi4wrapper on AVR).Test results
All tests drive the actual library code (
startMove()/nextAction()) on a host build with a simulatedmicros()clock.Equivalence sweep — 3,000 configurations (8 rpm × 5 accel × 5 decel × 3 microstep × 5 distances), comparing exact pulse-interval sequences against master:
accel != decelconfigs where the old double truncation understated the braking distance (new sequences brake correctly; e.g. rpm=60, accel=32000, decel=100: old braked 0 steps, new ramps down over 200 steps taking the physically correct 2 s)accel == decel: zero differing cases8-bit performance (per the "must not slow down AVR" requirement) — ATmega328P cycle counts via simavr, 40-step ramps,
-Os, identical inputs and bit-identical outputs:Flash (UnitTest, uno): 12,572 → 12,616 bytes (+44 B, from the once-per-move float setup in
startMove(); the per-step hot path is smaller and faster).Builds — UnitTest compiles clean (no warnings from
src/) for:uno,adafruit_feather_m0,nodemcuv2,esp32dev,teensylc(PlatformIO) andarduino:avr:uno(arduino-cli--warnings all).Performance impact
AVR (and other MCUs without hardware divide): deceleration steps ~11% cheaper, acceleration ~2% cheaper. 32-bit MCUs with hardware divide: no measurable change. No API changes.
Agent: claude-fable-5 (Claude Code 2.1.153); max context and thinking level not exposed to the agent
🤖 Generated with Claude Code