gpio-motors: deliver sub-tick delays on HZ=100 kernels

[phedoreanu]

The delay argument has never actually worked below 10 ms. These cameras run HZ=100 kernels without high-resolution timers, so every usleep() rounds up to a 10 ms tick: usleep(1500) waits ~10 ms, and 8 micro-steps × 10 ms puts a hard ~80 ms floor under every step regardless of the requested delay.

Measured on a Hi3518EV200 with 28BYJ-48-style steppers:

	200 steps
delay 15, usleep	33 s
delay 4, usleep	18 s
delay 1.5 ms, CLOCK_MONOTONIC spin	~4 s

The requested delay barely mattered before because tick rounding dominated everything. With a CLOCK_MONOTONIC spin for delays below one tick, the delay argument finally means what it says.

Delays of 10 ms and up still go through 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.

If there is interest, a follow-up can add constant-acceleration ramping (ease in/out over the first and last steps) — at the speeds this fix unlocks, a stepper driven at a flat rate from standstill lurches and can slip steps.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

gpio-motors: add CLOCK_MONOTONIC busy-wait for sub-tick delays on HZ=100

🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

AI Description

• Make step delays below 10ms accurate on HZ=100, no-hrtimer kernels.
• Add a monotonic-clock spin delay for sub-tick waits while keeping usleep() for longer delays.
• Route axis micro-step pacing through the new delay helper to honor requested timing.

Diagram

graph TD
  A["axis_run()"] --> B["delay_us()"] --> C{">= 10ms?"}
  C -->|"yes"| D["usleep()"]
  C -->|"no"| E["clock_gettime(MONOTONIC)"] --> F["spin until target"]

  subgraph Legend
    direction LR
    _fn["Function"] ~~~ _dec{"Decision"} ~~~ _sys["Syscall/Clock"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use clock_nanosleep(TIMER_ABSTIME) for sub-tick delays

• ➕ Avoids burning CPU in a tight loop
• ➕ Absolute-time sleeps can reduce drift across repeated delays
• ➖ On no-hrtimer HZ=100 kernels it will still quantize to the tick, so it may not solve the core problem
• ➖ More error handling and portability considerations than the current approach

2. Hybrid spin-with-yield (spin briefly, then sched_yield in loop)

• ➕ Reduces CPU pressure during longer sub-tick waits
• ➕ Still allows finer-than-tick timing if the runqueue is favorable
• ➖ Timing becomes scheduler-dependent and can introduce jitter
• ➖ More complex to reason about than a simple monotonic busy-wait

3. Move motor stepping into kernel/RT context (PWM/PIO/driver)

• ➕ Best timing determinism and lowest userspace jitter
• ➕ Better scalability if moves become longer or concurrent
• ➖ Much higher implementation and maintenance cost
• ➖ Likely out of scope for this project/repo and target devices

Recommendation: Keep the PR’s approach: a CLOCK_MONOTONIC busy-wait for <10ms is the most reliable way to achieve sub-tick delays on HZ=100 kernels without high-resolution timers, while preserving usleep() for longer waits to avoid unnecessary CPU burn. Alternatives that rely on nanosleep/clock_nanosleep are unlikely to improve resolution on these kernels, and scheduler-yield hybrids trade accuracy for reduced CPU usage in a way that can reintroduce jitter.

Files changed (1) +30 / -1

Bug fix (1) +30 / -1

gpio-motors.cAdd sub-tick delay helper and use it for micro-step timing +30/-1

Add sub-tick delay helper and use it for micro-step timing

• Introduces delay_us() that uses usleep() for delays >=10ms and a CLOCK_MONOTONIC busy-wait for smaller delays to avoid HZ=100 tick rounding. Replaces the axis_run() per-micro-step usleep(delay) call with delay_us(delay) and includes <time.h> for clock_gettime().

general/package/gpio-motors/src/gpio-motors.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. 32-bit elapsed overflow ✓ Resolved 🐞 Bug ☼ Reliability

Description

delay_us() computes elapsed nanoseconds in a signed long; on 32-bit platforms a multi-second
pause/preemption during the loop can trigger signed overflow (undefined behavior) and make the
busy-wait run incorrectly. Since axis_run() calls this per microstep, the motor move can be delayed
unpredictably and the process can burn CPU far longer than requested.

Code

general/package/gpio-motors/src/gpio-motors.c[R152-154]

+		clock_gettime(CLOCK_MONOTONIC, &now);
+		long elapsed = (now.tv_sec - start.tv_sec) * 1000000000L + (now.tv_nsec - start.tv_nsec);
+		if (elapsed >= target) {

Evidence

The PR introduces a busy-wait that computes nanoseconds in long and is invoked for every
microstep; on embedded 32-bit systems this arithmetic can overflow/UB if the thread is paused long
enough during the loop, breaking the wait behavior.

general/package/gpio-motors/src/gpio-motors.c[142-158]
general/package/gpio-motors/src/gpio-motors.c[160-183]
general/package/gpio-motors/Config.in[1-6]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`delay_us()` converts a `timespec` delta into nanoseconds using `long` math. On 32-bit targets, `(tv_sec_delta * 1e9)` can overflow after ~2 seconds, which is signed overflow (undefined behavior) and can break the exit condition of the busy-wait.
## Issue Context
This code runs on embedded SoCs where 32-bit `long` is common, and `delay_us()` is called once per microstep in `axis_run()`.
## Fix Focus Areas
- general/package/gpio-motors/src/gpio-motors.c[142-158]
### Suggested approach
- Include `<stdint.h>`.
- Use `int64_t` (or `uint64_t`) for `target_ns` and `elapsed_ns`:
- `int64_t target_ns = (int64_t)us * 1000;`
- `int64_t elapsed_ns = (int64_t)(now.tv_sec - start.tv_sec) * 1000000000LL + (now.tv_nsec - start.tv_nsec);`
- Consider guarding `us <= 0` with an immediate return.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Negative delay unthrottled ✓ Resolved 🐞 Bug ≡ Correctness

Description

delay_us() does not reject negative delays, so a negative CLI delay becomes a negative target and
the function effectively returns immediately, producing zero inter-microstep delay. This can drive
the motor much faster than intended for invalid input.

Code

general/package/gpio-motors/src/gpio-motors.c[R153-156]

+		long elapsed = (now.tv_sec - start.tv_sec) * 1000000000L + (now.tv_nsec - start.tv_nsec);
+		if (elapsed >= target) {
+			return;
+		}

Evidence

The CLI can produce a negative microsecond delay, and the new delay_us() logic will then return
immediately due to comparing elapsed time against a negative target. The kernel driver in this repo
shows a precedent for rejecting negative delay values.

general/package/gpio-motors/src/gpio-motors.c[142-156]
general/package/gpio-motors/src/gpio-motors.c[190-212]
general/package/gpiostep-openipc/src/gpiostep.c[88-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`delay_us()` treats negative delays as “done” (because `elapsed >= target` when `target` is negative), which results in unthrottled stepping if the CLI delay is negative.
## Issue Context
The CLI parses delay as milliseconds and multiplies by 1000 with no validation. The in-kernel equivalent driver explicitly rejects negative delays.
## Fix Focus Areas
- general/package/gpio-motors/src/gpio-motors.c[142-158]
- general/package/gpio-motors/src/gpio-motors.c[190-212]
### Suggested approach
- In `main()`, parse delay into a signed type and validate `delay_ms >= 0` before multiplying.
- In `delay_us()`, add `if (us <= 0) return;` as a defensive guard.
- (Optional) cap maximum delay to a sane bound and error out on overflow.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. clock_gettime errors unchecked 🐞 Bug ☼ Reliability

Description

delay_us() ignores clock_gettime() return values, so if either call fails the start/now timestamps
may be indeterminate and the busy-wait behavior becomes undefined. A failure should fall back to a
safe sleep or exit the delay path.

Code

general/package/gpio-motors/src/gpio-motors.c[R149-152]

+	clock_gettime(CLOCK_MONOTONIC, &start);
+	long target = us * 1000;
+	for (;;) {
+		clock_gettime(CLOCK_MONOTONIC, &now);

Evidence

The newly added delay_us() calls clock_gettime() twice but never checks for errors, leaving an
undefined-behavior path if the time query fails.

general/package/gpio-motors/src/gpio-motors.c[148-153]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`delay_us()` does not check `clock_gettime()` return values. On failure, `start`/`now` may contain indeterminate data and the elapsed-time calculation can misbehave.
## Issue Context
While `CLOCK_MONOTONIC` failures are uncommon on normal Linux systems, this is a new error path introduced by the PR and is easy to harden.
## Fix Focus Areas
- general/package/gpio-motors/src/gpio-motors.c[148-157]
### Suggested approach
- Check both `clock_gettime()` calls:
- If the initial call fails, fall back to `usleep(us)` (or `nanosleep`) and return.
- If the loop call fails, break and fall back to `usleep(0)` / `sched_yield()` / or just return to avoid an unbounded spin.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗