stepcompress_hp: validation-loop cleanup and interval-overflow fixes#933
Open
Hannott wants to merge 2 commits into
Open
stepcompress_hp: validation-loop cleanup and interval-overflow fixes#933Hannott wants to merge 2 commits into
Hannott wants to merge 2 commits into
Conversation
test_step_move() recorded m->last_step on every iteration of the step validation loop and guarded m->first_step with `if (!m->first_step)`, using the value 0 as a "not yet set" sentinel. A step that legitimately lands at time 0 (possible when the queued step sits right at last_step_clock) would then be skipped, leaving first_step pointing at the second step and feeding a wrong first_clock into the move history. Assign first_step on i == 0 instead, and store last_step once after the loop (and explicitly on the early-return error paths) rather than on every iteration. The compressed output is byte-identical to before across all tested motion profiles; removing the per-step 64-bit store measures ~1.5-3% less host CPU in the validation loop (min-of-15 runs, gcc -O2 x86-64: 1190.6 -> 1155.3 us/rep on a constant-velocity workload). Signed-off-by: Åsmund Collin <aakjaergaard@gmail.com>
Two related issues in the high-precision encoder and validator: - test_step_move() checked s.interval for overflow after inc_interval() even on the final iteration, i.e. against an interval that is never used, and on overflow it discarded the current step even though that step had already passed its own point check. Only reject when a following step would actually consume the overflowed interval, and keep the validated step (count = i + 1). - step_move_encode() bounded only the first decoded interval against MAX_INT32 while scaling up the shift, not the running interval across the whole move, so it could emit moves the validator then had to truncate on overflow. Bound max_end_int (the running-interval upper bound) instead. No steps are lost: total step counts are identical to before on every tested profile. Letting more moves keep their full length reduces the emitted queue_step messages by 4-9% on typical moves (e.g. a 20mm zig-zag at 100mm/s: 540 -> 512 messages; a slow 60mm move: 261 -> 239) with zero change in the steps delivered. Signed-off-by: Åsmund Collin <aakjaergaard@gmail.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.
Summary
Two small, self-contained fixes to the high-precision stepping compressor
(
klippy/chelper/stepcompress_hp.c):a redundant per-step store out of the hot loop.
never-used interval, and bound the running interval (not just the first)
when scaling the encoder shift.
Neither fix changes the wire format or the steps delivered to the MCU. Fix 1
is byte-identical in output; fix 2 emits fewer
queue_stepmessages forthe same motion (4–9% on typical moves) while delivering exactly the same
steps.
Each fix is its own commit so they can be reviewed and reverted independently.
Rationale
Fix 1 —
test_step_move()per-step bookkeepingThe step-validation loop did two things on every iteration:
m->last_step = cur_step;— a 64-bit store overwritten on every pass andonly meaningful on the last one.
if (!m->first_step) m->first_step = cur_step;— using0as a"not-yet-set" sentinel.
The sentinel is the real bug: a step can legitimately land at clock offset
0(when the queued step sits right at
last_step_clock, sopoint.minp <= 0).When that happens the first step is silently skipped and
first_stepends uppointing at the second step, which then feeds a wrong
first_clockinto themove history used by
stepcompress_find_past_position().The fix assigns
first_steponi == 0and storeslast_steponce after theloop (and explicitly on the two early-return error paths, preserving the
previous values). As a side benefit it removes a store from the hottest loop
in the compressor.
Fix 2 — interval-overflow handling and shift bound
Two related problems around interval overflow:
Validator: the
s.interval >= 0x80000000check runs afterinc_interval(), including on the final iteration — i.e. it tests theinterval for a step that is never emitted. On overflow it also dropped the
current step, even though that step had already passed its own point
check. The fix only rejects when a following step would actually consume the
overflowed interval (
i + 1 < m->count) and keeps the validated step.Encoder: while scaling the shift up,
step_move_encode()bounded onlythe first decoded interval against
MAX_INT32. Because the interval growsacross the move, the encoder could pick a shift whose later intervals
overflow int32 — producing a move the validator then had to truncate. The
fix bounds
max_end_int(the running-interval upper bound over the wholemove) instead, so the encoder stops one shift earlier and the move stays
valid at full length.
The net effect is that more moves keep their full length, so the compressor
emits fewer, larger
queue_stepcommands for the same motion.Verification
A standalone C harness feeds trapezoidal and constant-velocity step schedules
through
queue_flush_hp()(steady horizon flushes plus a full drain persegment,
max_error= 25 µs at 180 MHz) and links the realmsgblock.cVLQ encoder, so the message counts and byte sizes are the actualon-wire values. It compares three builds: baseline (this branch's base),
fix 1 only, and fix 1 + fix 2.
Output is never degraded — no steps lost
Total steps emitted match the input exactly for every profile and every build.
Fix 1 is byte-identical to baseline (2426 message records, zero diff).
Fix 1 + fix 2 emits the same steps in equal-or-fewer messages:
Host CPU (fix 1)
Because fix 1's output is byte-identical, any timing delta is purely the
hoisted store.
gcc -O2, x86-64, min of 15 runs:Small but consistent, and never a regression.
Host CPU (fix 2) — an honest note
Fix 2 is roughly CPU-neutral but workload-dependent: −4.3 % on the
constant-velocity workload (fewer, larger moves to test) and +2.0 % on the
realistic trapezoid (the extra per-iteration guard and a few more encoder
shift iterations). Its value is the message reduction and overflow safety, not
host CPU; the reduced message count also lowers downstream
serialqueuework.Caveats: measured on x86-64; Kalico typically runs the host on ARM, so
absolute magnitudes will differ (direction should hold). The harness is a
host-side C benchmark, not a full batch run, so it does not exercise MCU-side
decoding — but the message stream it counts is the real encoded output.
Docs
No user-facing commands, config options, status variables, or webhooks change,
and the MCU message format is unchanged (backwards compatible), so no
docs/updates are required perCONTRIBUTING.md.Testing
gcc -Wall -Wextra -O2clean (no new warnings introduced).Signed-off-by: Åsmund Collin aakjaergaard@gmail.com