gpiostep: busy-wait sub-tick delays; correct the timing claim - #2394
Conversation
PR Summary by QodoHonor sub-tick gpiostep delays and correct timing claims
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. Motor timing lacks real-board proof
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
1816816 to
1a1035d
Compare
There was a problem hiding this comment.
This is the PR I asked for, and the honesty correction is the right one: usleep_range() is hrtimer-backed, those kernels have no hrtimers, so the old "steadier timing, lower CPU" claim was not true on any defconfig that ships the package. IS_ENABLED(CONFIG_HIGH_RES_TIMERS) is the better test here - the module is built against the board kernel, so this resolves at compile time instead of repeating the userspace clock_getres() dance. The 1 ms chunking is right and genuinely needed: ARM's MAX_UDELAY_MS is 2, and the quarter-tick threshold admits 2001-2499 us.
Three things before this lands. Two are inline; the third is in a file this PR does not touch:
gpiostep-ctl still has the overflow nit you fixed on the other side. gpiostep-ctl.c:37 is m.delay_us = atoi(argv[3]) * 1000; with no clamp - the same signed-overflow UB I raised on #2247 and that you fixed in gpio-motors.c:243 with delay_ms > INT_MAX / 1000. This PR is explicitly the gpiostep counterpart of that work; carry the clamp across.
On Qodo - I got this wrong in the first version of this review and have corrected it. I wrote that neither finding had been answered and that finding 2 was a false positive. Both were wrong:
- You answered all three threads at 09:18, within about twelve minutes of the review.
- Finding 2 (README overstates timing) was correct when filed. I only checked
Readme.mdat the current head and missed the force-push at 09:18:46 - the pre-amend commit touched justConfig.inandgpiostep.c, matching Qodo's own "Files changed (2)". You had already fixed it in the amend. The retraction is on theReadme.mdthread; disregard the ask to rebut it. - Finding 1 (hardware evidence) is answered but not resolved, and I am holding it - see #2393 for how I want to split the evidence ask. Short version: the delay primitive is measurable on a gk7205v200-class kernel, the motor behaviour needs a v500/v510 owner, and neither of us has one.
Compile-verifying the module against the goke 4.9.37 tree is the right check for a module and I'll take it, but it is not the evidence this needs. Please mark this draft until the three items above are in and a timing measurement is attached. CI has not run on this branch either.
usleep_range() runs on hrtimers, and without CONFIG_HIGH_RES_TIMERS those expire with jiffy granularity - a sub-tick sleep rounds up to the next tick exactly like a userspace usleep. All three defconfigs that ship this package (gk7205v500_lite, gk7205v500_ultimate, gk7205v510_lite) build HZ=100 kernels without high-resolution timers, so the "steadier timing" this package claimed over the userspace gpio-motors tool did not hold: both were quantised to the same 10ms. Busy-wait with udelay() while the requested delay is under a quarter tick, where the rounding would at least quadruple the step period; from a quarter tick up, keep sleeping and accept the rounding, since the busy-wait cost grows with the delay while its benefit shrinks. The udelay is chunked because ARM bounds a single call at ~2ms, and a cond_resched() per micro-step keeps a move from monopolising the core - these kernels are !SMP and !PREEMPT, so without it nothing else, the encoder included, would run until the whole move finished. Reword Config.in, Readme.md and the file header to claim what the module actually provides: no per-write syscall cost, same tick-bounded granularity.
1a1035d to
0e6424e
Compare
The busy-wait branch admitted delay 0: (unsigned int)0 < 2500 is true, so it ran udelay(0) with only the cond_resched() between micro-steps. Route 0 back to usleep_range() as before this series and drop the unsigned cast, which had turned a negative into a huge value that fell through to usleep_range(negative, ...) - the ioctl already rejects negatives, so the guard is the plain signed comparison. For the record, usleep_range(0, 1) is not a floor either: it is an already-expired hrtimer and returns at once. Measured on a Hi3518EV200 (HZ=100, no hrtimers), 320 micro-steps at delay 0 complete in under 10ms on both the old and the new module. Zero has never paced the coil; this change only keeps that as it was instead of routing it through a path whose guard is the only thing between a negative and udelay(). gpiostep-ctl still multiplied atoi(argv[3]) by 1000 unchecked, the same signed overflow gpio-motors clamps with INT_MAX / 1000. Carry the clamp across, with the same message.
The cond_resched() after each busy-waited micro-step yields to whatever is runnable, and on a !PREEMPT !SMP kernel that can hand the core away for several ticks before the next micro-step. So what the module delivers is a busy-wait between scheduler yields, not sub-tick pacing in general. Say so in Config.in, Readme.md and the file header, and explain the trade in the step_delay() comment.
0e6424e to
d446b61
Compare
|
Measured — and on a real stepper this time, since the board I have drives its pan/tilt coils the same way. Board: Hi3518EV200 (ARM926EJ-S), OpenIPC 4.9.37, Method: both modules built by
Reading it:
Delay 0 — the review's premise doesn't hold, and neither did my first fix's wording. The inline comment said What I could not observe: whether the head actually tracked the coil at 1.4 ms per micro-step. Nobody was at the camera, and it is a different motor from the V510's anyway — so the motor-behaviour half stays with a v500/v510 owner, as agreed. Raw outputTest script (runs on the board)#!/bin/sh
# Runs on the cam. Loads gpiostep.ko (old or new), drives the real pan
# stepper +N then -N at each delay, and prints wall / user / sys per move.
# sys time is the CPU the ioctl burnt in step_delay(); real is the move time.
KO=$1; STEPS=${2:-20}
PINS="pan_gpios=31,32,33,34 tilt_gpios=58,59,60,61"
rmmod gpiostep 2>/dev/null
insmod "$KO" $PINS || exit 1
dmesg | grep gpiostep | tail -1
echo "module=$KO steps=$STEPS (x8 micro-steps per step, +/-)"
for d in 1 2 3 4 9 0; do
echo "--- delay $d ms"
time gpiostep-ctl $STEPS 0 $d
time gpiostep-ctl -$STEPS 0 $d
done 2>&1
rmmod gpiostepAll three items from the review are in: the |
|
Code review by qodo was updated up to the latest commit d446b61 |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Everything from the review is in, and the measurements settle it.
delay 0guard, unsigned cast gone,gpiostep-ctlclamped toINT_MAX / 1000to matchgpio-motors.c:243.- The idle-core caveat is in Config.in, the Readme and the file header, not just the comment.
- The Hi3518EV200 run is the evidence I asked for:
oldpinned at 10.0 ms/micro-step across delays 1-9 ms,newdelivering 1.4 ms and ~3 ms at 1-2 ms and handing 3/4/9 to the tick at 0.05 s sys. Thecond_resched()overrun is visible and bounded, which is exactly the claim the docs now make.
And thank you for measuring the delay 0 premise instead of taking it from me — you were right, usleep_range(0, 1) is an already-expired hrtimer and zero never had a floor. Keeping the guard for the negative-vs-udelay() reason and leaving a real floor at 0 to a separate, explicit change is the right split.
Approving. The V510 motor-behaviour half stays open for an owner of that board, as agreed - it is not a condition on this.
delay_us() spun for any sub-tick delay, so its CPU cost rose exactly as its benefit fell: at delay 9 on an HZ=100 kernel that is ~14s of pinned core per 200-step move to shave an 11% timing error, with every clock_gettime in the spin a real syscall on cores where CONFIG_ARM_ARCH_TIMER_VCT_ACCESS is not set. The sleep threshold is now a quarter tick instead of a whole one. Below a quarter tick (delay 1-2 at HZ=100) sleeping would at least quadruple the step period, so the spin stays - that is the range it was added for in #2247. From a quarter tick up, usleep() and accept the rounding. That gives delay 3 and 4 back to the 10ms tick, turning a 4.8s and 6.4s move into 16s, in exchange for not pinning the core for the duration. clock_gettime() is now checked inside the spin loop. An unchecked failure would leave now stale and the loop would never terminate; on failure it falls back to usleep() for the full delay, matching the pre-loop failure path. Over-waiting is the safe direction for a stepper. Measured on a Hi3518EV200 (HZ=100, no hrtimers, no preempt, no SMP - the same kernel class as the gk7205v500/v510 boards that ship the package), majestic streaming throughout, 1600 calls per row: delay usleep before (#2247) after 1 ms 9.996 ms 1.294 ms, 76% CPU 1.300 ms, 76% CPU 2 ms 9.999 ms 2.504 ms, 76% CPU 2.689 ms, 70% CPU 3 ms 10.013 ms 3.836 ms, 72% CPU 10.013 ms, 1.7% CPU 4 ms 10.006 ms 4.915 ms, 73% CPU 10.010 ms, 1.7% CPU 9 ms 10.400 ms 9.980 ms, 74% CPU 10.426 ms, 1.7% CPU The spin rows overrun the requested delay because a spinning process is still preempted at each tick by the encoder; that is the contrast with the kernel-side busy-wait in #2394, which carries a cond_resched() for the opposite reason. Motor behaviour on a GK7205V500/V510 board itself is still unmeasured; nobody in the thread owns one.
2 participants
The separate gpiostep PR discussed on #2247.
The claim was wrong. Config.in promised "steadier timing, lower CPU than the userspace gpio-motors tool", but
usleep_range()runs on hrtimers, and withoutCONFIG_HIGH_RES_TIMERSthose expire with jiffy granularity — a sub-tick sleep rounds up to the next tick exactly like a userspaceusleep(). All three defconfigs that ship this package (gk7205v500_lite, gk7205v500_ultimate, gk7205v510_lite) build HZ=100 kernels without high-resolution timers, so both drivers were quantised to the same 10ms and the kernel module's only real advantage was skipping syscall traffic.The fix.
step_delay()now busy-waits withudelay()while the requested delay is under a quarter tick — the same materiality threshold as the gpio-motors side (#2393): below it, sleeping would at least quadruple the step period; above it, keep sleeping and accept the rounding, since the busy-wait cost grows with the delay while its benefit shrinks. Details:udelay()is chunked at 1ms because ARM bounds a single call at ~2ms;cond_resched()per micro-step keeps a move from monopolising the core — these kernels are!SMPand!PREEMPT, so without it nothing else (the encoder included) would run until the whole move finished. That was the one hazard the userspace spin never had: the scheduler still preempts userspace at every tick, but kernel code on a!PREEMPTuniprocessor runs until it yields;IS_ENABLED(CONFIG_HIGH_RES_TIMERS)keeps the old behaviour on any future kernel that turns hrtimers on —usleep_range()is strictly better there.Config.in, Readme.md and the file header now claim what the module actually provides: no per-write syscall cost, same tick-bounded granularity, sub-quarter-tick delays busy-waited between scheduler yields — which holds on an idle core, not under load.
Compile-verified against the real target:
make BOARD=gk7205v500_lite br-gpiostep-openipcbuilds the module clean against the goke 4.9.37 tree.Measured on a real stepper, on a Hi3518EV200 (same kernel class: HZ=100, no high-resolution timers, no preempt) with the module driving the camera's own pan coil and majestic streaming: the old module takes 10 ms per micro-step at every delay from 1 to 9 ms (320 micro-steps = 3.2 s regardless of the ask); this branch honours 1 ms at 1.4 ms per micro-step and 2 ms at ~3 ms under that load, at ~70% / ~55%
sys, and hands 3, 4 and 9 ms to the tick at the old cost. Delay 0 has never had a floor —usleep_range(0, 1)returns at once on the old module too — and this PR leaves that as it was. Full table, raw output and test script: #2394 (comment)Whether the V510's motor tracks the coil at 1.4 ms per micro-step is the part this cannot answer; that needs a gk7205v500/v510 owner.