gpio-motors: deliver sub-tick delays on HZ=100 kernels - #2247
Conversation
PR Summary by Qodogpio-motors: add CLOCK_MONOTONIC busy-wait for sub-tick delays on HZ=100
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
flyrouter
left a comment
flyrouter
left a comment
There was a problem hiding this comment.
Thanks.
It looks like an interesting proposal for improvement.
|
Thanks for this — the diagnosis is correct and the measurements are good. I reproduced your numbers on lab hardware: on a Goke GK7205V200 (4.9.37, HZ=100), But the root cause is The hardware was never the limit
The one-shot machinery is already running. The only thing pinning Measured, one symbol flippedI rebuilt the hi3516ev300 kernel with
Overhead is ~56–85 µs (wakeup latency + the 50 µs default Why I'd rather not merge the spin
The bigger win is orthogonal and free
Heads-up if we go the Kconfig route
Two more things
Nit still open: |
|
Hard to argue with a rebuilt kernel and measurements on both paths — thanks for doing that. The PR is reworked along your lines; two commits pushed. Your fd-caching patch is in (55584c3). Each The spin is now gated on
Both nits fixed in the same commit: On Behaviour change: now called out in the PR body — on coarse-timer kernels, moves tuned against the de-facto 10 ms floor get faster once the requested delay is actually delivered; callers wanting the old pace can pass gpiostep: agreed — |
The delay argument has never actually worked below 10ms. These cameras run HZ=100 kernels without high-resolution timers, so every usleep() rounds up to a 10ms tick: usleep(1500) waits ~10ms, and 8 micro-steps x 10ms puts a hard ~80ms floor under every step regardless of the requested delay. Measured on a Hi3518EV200 (28BYJ-48 steppers): 200 steps took 33s at delay 15 and still 18s at delay 4 - the delay barely mattered, because the tick rounding dominated. With a CLOCK_MONOTONIC spin for delays below one tick the same 200 steps complete in ~4s at 1.5ms per micro-step, and the delay argument finally means what it says. Delays of 10ms and up still use usleep, so slow moves do not spin. Busy-waiting below that is a deliberate trade: moves are short and bounded, and a stepper mid-move needs the CPU for milliseconds, not ticks.
Review follow-up: - compute the elapsed time in long long: on 32-bit targets a long overflows after ~2.1s, which a preemption in the middle of the spin can reach, and signed overflow is undefined behavior - reject a negative delay at the CLI and treat non-positive delays as zero in delay_us, instead of spinning unthrottled - fall back to usleep if clock_gettime fails, so the wait stays bounded
Each gpio_set() did snprintf + fopen + fprintf + fclose, four times per micro-step. On these SoCs a single sysfs open/write/close round trip costs on the order of half a millisecond, so one micro-step spent 2-5ms on file churn alone - more than the step delay it was trying to honour, and pure overhead on every platform. Open each value file once after export and keep the fd for the run; stepping is now an lseek + a 1-byte write per pin. Cleanup still goes through the sysfs paths and closes the fds.
Roughly a third of the board kernels in this tree ship with CONFIG_HIGH_RES_TIMERS=y, and there usleep() already delivers sub-tick delays to within tens of microseconds - spinning on those platforms would trade a working sleep for 100% CPU on a single-core SoC. clock_getres(CLOCK_MONOTONIC) tells the two kernels apart at runtime (1ns with hrtimers, one jiffy without), so consult it once and keep usleep() everywhere except the one case it cannot handle: a sub-tick delay on a coarse-timer kernel. Also route the SELECT_PIN settle through delay_us() - as a plain usleep(100) it silently cost a whole 10ms tick on coarse-timer kernels - and reject delay arguments that would overflow the ms-to-us conversion, since delay_ms * 1000 is signed-overflow UB past INT_MAX/1000.
e072917 to
f74b27d
Compare
3 participants
What
Two changes to how
gpio-motorspaces the stepper sequence:valuefds open for the whole run.gpio_set()used to dosnprintf+fopen+fprintf+fcloseper pin, 4× per micro-step — 2–5 ms of pure file churn per micro-step on these SoCs, more than the delay it was trying to honour. Stepping is now anlseek+ 1-bytewriteon an fd opened once after export.CONFIG_HIGH_RES_TIMERSoff, HZ=100 — the Hi35xx/GK72xx boards here) everyusleeprounds up to a 10 ms tick, sousleep(1500)waits ~10 ms and every micro-step has a hard 10 ms floor.delay_us()consultsclock_getres(CLOCK_MONOTONIC)once (1 ns with hrtimers, one jiffy without) and keepsusleep()everywhere except the one case it cannot handle: a sub-tick delay on a coarse-timer kernel, where it pollsCLOCK_MONOTONICinstead. Platforms with hrtimers keep their working sleep and never spin.Also: the
SELECT_PINsettle goes throughdelay_us()(as a plainusleep(100)it silently cost a whole tick on coarse-timer kernels), and delay arguments that would overflow the ms→µs conversion are rejected.Why
Measured on a Hi3518EV200 (HZ=100, no hrtimers): 200 steps took 33 s at delay 15 and still 18 s at delay 4 — the requested delay barely matters because the tick floor dominates. @widgetii reproduced the same numbers on a GK7205V200. With the fd cache plus a delivered 1.5 ms delay, a micro-step drops from ~12 ms to ~1.6 ms.
The root fix would be
CONFIG_HIGH_RES_TIMERS=yper board, but as the review measurements show it does not fit every kernel partition without per-board trims (hi3516ev300-lite has 203 bytes of headroom), so this stays a userspace accommodation that detects the kernel it got.Note for existing installs on coarse-timer kernels: the 10 ms floor has been the de-facto pace, so moves tuned against it become faster once the requested delay is actually delivered. Callers that want the old pace can pass
delay >= 10.Verification
-Wall -Wextra.