Fix 32-bit overflow in startBrake() ACCELERATING branch#144
Open
94xhn wants to merge 1 commit into
Open
Conversation
steps_remaining = step_count * profile.accel / profile.decel computes the multiplication in 32-bit signed long before dividing, so it can overflow with legal short accel/decel values (e.g. accel=32767, decel=32000 at step_count=70000 yields steps_remaining=-62539 instead of the correct 71677). A negative steps_remaining is treated as STOPPED by getCurrentState(), so nextAction() stops issuing step pulses immediately instead of ramping down, defeating startBrake()'s purpose of a controlled deceleration. Cast to float before the multiply, matching the same overflow-avoidance idiom already used elsewhere in this file (line 187, and the sibling steps_to_cruise/steps_to_brake computation in startMove()). Verified by compiling the affected source against a minimal Arduino.h host mock and driving the public API (setRPM/setSpeedProfile/startMove/ nextAction/startBrake) to reproduce the overflow with real, legal parameters, then confirming the patched result exactly matches the 64-bit ground truth. Also ran 6 non-overflowing regression scenarios (rpm 60-2000, microsteps 1-64, accel/decel 100-32767) with byte-identical output before/after the change. Agent: claude-sonnet-5 (Claude Code)
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.
Problem
BasicStepperDriver::startBrake(),ACCELERATINGcase, computes:step_countislong,profile.accel/profile.decelareshort, so themultiplication happens in 32-bit signed arithmetic before the division ever
runs. With legal, unclamped
shortvalues this overflowsLONG_MAX.Example:
accel = 32767,decel = 32000, andstartBrake()called afterstep_count = 70000steps into an acceleratingLINEAR_SPEEDmove:70000 * 32767 / 32000 = 71677steps_remaining = -62539A negative
steps_remainingis treated asSTOPPEDbygetCurrentState(),so
nextAction()'selsebranch zeroesnext_action_intervaland stopsissuing step pulses immediately, instead of ramping down through
DECELERATING. That defeats the purpose ofstartBrake()(a controlledearly stop) and replaces it with an abrupt halt — the exact failure mode
startBrake()exists to avoid, risking missed/lost steps from motor inertia.This is the same overflow class already fixed in this file for
steps_to_cruise/steps_to_brakeinstartMove()(see the comment atline 164-166 and the float-cast idiom at line 187) — this call site in
startBrake()was missed.Fix
Cast to
floatbefore the multiply, mirroring the exact idiom already usedelsewhere in this same file:
Testing
Compiled the affected source (
src/BasicStepperDriver.cpp) standaloneagainst a minimal
Arduino.hhost mock (pinMode/digitalWrite/micros/yieldno-ops), confirmedsizeof(long)==4on the test toolchain (matchesthe AVR/Cortex target width), and drove the object purely through its
public API (
setRPM,setSpeedProfile,startMove,nextActionpollingloop,
startBrake) to reproduce the overflow with the parameters above.steps_remaining = -62539(wrong, negative).steps_remaining = 71677(exact match to the 64-bitground truth).
non-overflowing parameter sets (rpm 60-2000, microsteps 1-64, accel/decel
100-32767, motor_steps 200/400, both inside and just-exiting
ACCELERATING) — byte-identical output in every case, confirming nobehavior change in the normal operating range.
-Wall, zero warnings, before and after.Note: if the true mathematically-correct answer itself exceeds
LONG_MAX(an extreme
decelvalue far belowaccel), the finalfloat→longcastcan still saturate — that's an inherent capacity limit of using
longforstep counts, unrelated to this arithmetic-ordering bug, and already the
accepted tradeoff elsewhere in this codebase (
steps_remaining,steps_to_cruise, etc. are alllong). Out of scope for this minimal fix.Agent: claude-sonnet-5 (Claude Code)