Fix off-by-one in moveTimed() duration chunking at 131072 boundary#367
Merged
Conversation
When this_duration == 131072 exactly, the check `this_duration > 131072` falls through to the `this_duration > 65535` branch, which computes this_duration / 2 == 65536. Since cmd.ticks is a uint16_t, this silently truncates to 0, causing addQueueEntry() to reject the command with AQE_ERROR_TICKS_TOO_LOW and moveTimed() to return an error for an otherwise perfectly ordinary single-step move. Changing the comparison to >= mirrors the already-correct sibling check in the steps==0 loop above (line 768: `duration >= 131072`), which never exhibits this bug.
Owner
|
Good catch. Thanks for the patch. |
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.
Bug
In
FastAccelStepper::moveTimed(), when a single step's rate requires morethan one queue command (
rate > 65535), the per-step duration is chunked ina loop:
When
this_durationis exactly131072, the first condition (>) isfalse, so it falls into the
this_duration / 2branch, computing65536. Sincestepper_command_s::ticksis auint16_t(
src/fas_arch/common.h), this silently truncates to0.addQueueEntry()then rejects the zero-tick command withAQE_ERROR_TICKS_TOO_LOW, andmoveTimed()immediately returns thaterror — so a perfectly ordinary single-step move (duration = 131072
ticks, e.g. ~8.19 ms at 16 MHz) is spuriously rejected, while durations
of 131071 and 131073 both succeed.
The sibling loop just above (for
steps == 0, i.e. thealready-queued-commands path) has the same boundary but already uses
>=:Fix
Change the
steps > 0loop's comparison from>to>=to match itssibling, so
this_duration == 131072takes thecmd.ticks = 65535branch instead of the truncating divide-by-2 branch.
Testing
Built the library's own PC-based host tests (
extras/tests/pc_based/)against both the original and patched source. The existing
moveTimed()test (
test_16.cpp) and all other quick-running tests produce identicaloutput before/after the fix. Additionally verified directly:
moveTimed(1, 131072, &actual)— before: returns an error,actual == 0. After: succeeds,actual == 131072.moveTimed(1, 131071, &actual)andmoveTimed(1, 131073, &actual)— succeed identically before and after (unaffected boundary neighbors).