gpio-motors: spin only while the rounding error is material - #2393
gpio-motors: spin only while the rounding error is material#2393phedoreanu wants to merge 1 commit into
Conversation
PR Summary by QodoLimit GPIO motor busy-waiting to material timer rounding
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Motor timing ships without field proof
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both items from the merge comment are here, and the arithmetic checks out: CLOCK_RES_NS / 4000 is 2500 us at HZ=100, so the spin survives exactly where I asked it to and the 14.4 s tail is gone.
Two notes inline: the new threshold quietly costs the two rows my table skipped, and the clock_gettime fallback over-waits by a tick.
Qodo's rule violation is correct, and it is still open - you answered it at 09:18, but answering is not the same as clearing it. This changes motor pacing in three shipped images with no before/after from a board that runs the code. best_practices.md 5.3 says flag and close, and I am not going to pretend otherwise because I asked for the change.
That said, I created this situation - my own scope note on #2247 conceded nobody in that thread has a gk7205v500-family board, and the lab here does not have one either. So let me split the ask rather than block on the impossible half:
- The delay primitive is measurable without a PTZ board. I measured
usleep()quantisation on the lab gk7205v200 in #2247 - same HZ=100, same# CONFIG_HIGH_RES_TIMERS is not set, same# CONFIG_ARM_ARCH_TIMER_VCT_ACCESS is not set. A short harness calling the newdelay_us()at 1, 2, 3, 4 and 9 ms, reporting wall-clock and CPU time per call, would settle the threshold behaviour on the same hardware class. That is a real before/after, not a paste from a board that was not exercising the change. - Motor behaviour needs a v500/v510 owner. Whether the new pacing at delay 3-4 slips or stalls a coil is not something the primitive test can answer.
Your point that the threshold is read off the measured table rather than guessed is fair, and I am not asking you to re-derive it - only to show the primitive behaving as the table predicts on a board of that kernel class.
Please mark this draft until the first half is attached. Separately: CI has not run on this branch at all - no checks reported - and it is behind master. Rebase and let the matrix go green before this is mergeable either way.
The sub-tick spin in delay_us() ran for any delay under a tick, so its cost rose exactly as its benefit fell: at delay 9 on an HZ=100 kernel it pinned the core for ~14s over a 200-step move to avoid an 11% timing error, and on cores that cannot read the arch timer from userspace (CONFIG_ARM_ARCH_TIMER_VCT_ACCESS unset, as on gk7205v500) every clock_gettime in that spin is a real syscall. Sleep from a quarter tick upward and accept the rounding; keep the spin below that, where sleeping would at least quadruple the step period - the 1-2ms range the spin was added for. Also check clock_gettime() inside the spin loop; an unchecked failure would leave `now` stale and the loop would never terminate. It falls back to usleep() like the existing pre-loop failure path.
f7ab7c9 to
664da4a
Compare
|
Measured, as asked — the primitive half, on a board of the same kernel class. Board: Hi3518EV200 (ARM926EJ-S), OpenIPC 4.9.37, Harness: one static binary with the two
Reading it:
Raw outputHarness source (delay-bench.c, built with arm-openipc-linux-musleabi-gcc -O2 -static)/*
* delay-bench: wall-clock and CPU cost of the gpio-motors delay_us()
* primitive, before (#2247: spin below one tick) and after (#2393: spin
* below a quarter tick), plus plain usleep() as the baseline. Both
* delay_us() bodies are copied verbatim from the respective commits.
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>
#include <sys/resource.h>
long CLOCK_RES_NS = 0;
static void spin_to(struct timespec start, long us) {
struct timespec now;
long long target = (long long)us * 1000;
for (;;) {
if (clock_gettime(CLOCK_MONOTONIC, &now) != 0) {
usleep(us);
return;
}
long long elapsed = (long long)(now.tv_sec - start.tv_sec) * 1000000000LL + (now.tv_nsec - start.tv_nsec);
if (elapsed >= target)
return;
}
}
/* #2247 as merged: sleep at or above one tick, spin below */
static void delay_before(long us) {
if (us <= 0) return;
if (CLOCK_RES_NS <= 1000000 || us >= CLOCK_RES_NS / 1000) { usleep(us); return; }
struct timespec start;
if (clock_gettime(CLOCK_MONOTONIC, &start) != 0) { usleep(us); return; }
spin_to(start, us);
}
/* #2393: sleep at or above a quarter tick, spin below */
static void delay_after(long us) {
if (us <= 0) return;
if (CLOCK_RES_NS <= 1000000 || us >= CLOCK_RES_NS / 4000) { usleep(us); return; }
struct timespec start;
if (clock_gettime(CLOCK_MONOTONIC, &start) != 0) { usleep(us); return; }
spin_to(start, us);
}
static void delay_usleep(long us) { usleep(us); }
static double ts_diff(struct timespec a, struct timespec b) {
return (b.tv_sec - a.tv_sec) * 1e3 + (b.tv_nsec - a.tv_nsec) / 1e6;
}
static double ru_ms(struct rusage r) {
return (r.ru_utime.tv_sec + r.ru_stime.tv_sec) * 1e3 + (r.ru_utime.tv_usec + r.ru_stime.tv_usec) / 1e3;
}
static void bench(const char *name, void (*fn)(long), long us, int calls) {
struct timespec t0, t1;
struct rusage r0, r1;
getrusage(RUSAGE_SELF, &r0);
clock_gettime(CLOCK_MONOTONIC, &t0);
for (int i = 0; i < calls; i++)
fn(us);
clock_gettime(CLOCK_MONOTONIC, &t1);
getrusage(RUSAGE_SELF, &r1);
double wall = ts_diff(t0, t1), cpu = ru_ms(r1) - ru_ms(r0);
printf("%-7s %2ld ms x%4d wall %8.1f ms (%6.3f ms/call) cpu %8.1f ms (%5.1f%%)\n",
name, us / 1000, calls, wall, wall / calls, cpu, 100.0 * cpu / wall);
}
int main(int argc, char *argv[]) {
int calls = argc > 1 ? atoi(argv[1]) : 1600;
struct timespec res;
if (clock_getres(CLOCK_MONOTONIC, &res) == 0)
CLOCK_RES_NS = res.tv_sec ? 1000000000L : res.tv_nsec;
printf("clock_getres(CLOCK_MONOTONIC) = %ld ns; %d calls per row (200 steps x 8 micro-steps)\n", CLOCK_RES_NS, calls);
long delays[] = {1, 2, 3, 4, 9};
for (unsigned i = 0; i < sizeof delays / sizeof *delays; i++) {
long us = delays[i] * 1000;
bench("usleep", delay_usleep, us, calls);
bench("before", delay_before, us, calls);
bench("after", delay_after, us, calls);
printf("\n");
}
return 0;
}The motor half stays open for a v500/v510 owner, as agreed. Marking ready for review with the primitive measurement attached; branch is rebased onto current master. On CI: the runs for this head exist — build, lint, shell-tests, gcc-compat and qodo-gate all fired on the rebase — but every one is sitting at |
|
Code review by qodo was updated up to the latest commit 664da4a |
2 participants
Follow-up to #2247, picking up both points from the merge comment there.
Gate the spin on the error being material.
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 everyclock_gettimein the spin a real syscall on gk7205v500 (CONFIG_ARM_ARCH_TIMER_VCT_ACCESSis not set, so no vDSO fast path). The sleep threshold is now a quarter tick instead of a whole one:usleep()and accept the rounding.Against the table in the merge comment, that keeps the 900%/400% rows spinning and puts the 100% and 11% rows to sleep, including the 14.4s worst case.
What the threshold costs. The merge-comment table listed 1, 2, 5 and 9 ms; the two rows it skipped are the ones this change gives back to the tick, so here they are on the record (200 steps = 1600 micro-steps, HZ=100):
Delay 4 is the number #2247 opened with ("still 18s at delay 4" on the Hi3518EV200), so this hands that measurement back in exchange for not pinning the core at 4.8-6.4 s per move. Anyone who wants the faster move at 3-4 ms can ask for 2 and get it at 3.2 s with the spin; that is the trade the threshold makes.
Check
clock_gettime()inside the spin loop. An unchecked failure would leavenowstale and the loop would never terminate. On failure it now falls back tousleep()for the full delay, same as the existing pre-loop failure path — over-waiting is the safe direction for a stepper.Compiles clean with
-Wall -Wextra.Measured on a Hi3518EV200 (same kernel class as gk7205v200/v500: HZ=100, no high-resolution timers, no preempt;
clock_getres= 10 ms), with majestic streaming:usleep()lands on 10 ms at every delay; the merged code spins on every row at 72-76% CPU, with the 9 ms row burning 11.8 s of CPU to finish 0.7 s sooner than sleeping; this PR keeps the 1 and 2 ms rows identical to the merged code and sends 3, 4 and 9 ms to the tick at 1.7% CPU. Full table, raw output and harness source: #2393 (comment)The motor half of the evidence needs a gk7205v500/v510 owner:
BR2_PACKAGE_GPIO_MOTORS=yonly exists in those defconfigs, and nobody in the #2247 thread has one. Whether the new pacing at 3-4 ms slips or stalls a coil is the part the primitive test cannot answer.