Skip to content

RSDK-12491: Fix threading issues and tight loop after completion#14

Open
Steve Briskin (stevebriskin) wants to merge 3 commits into
mainfrom
RSDK-12491-fixthreading
Open

RSDK-12491: Fix threading issues and tight loop after completion#14
Steve Briskin (stevebriskin) wants to merge 3 commits into
mainfrom
RSDK-12491-fixthreading

Conversation

@stevebriskin

Copy link
Copy Markdown
Contributor

Blast from the past...
The main fix here is fixing the infinite loop in doRun and removing redundant operation controls.
Tested manually.

Asked Claude to doublecheck the fix:

I traced every public API path through the new code and through the pre-change (HEAD) code, and read the actual implementations of the three primitives the behavior hinges on: StoppableWorkers.Stop (cancels, then waits for the goroutine), SingleOperationManager.New (cancels the previous op and blocks until that op's method fully returns, and is nested-op aware so WaitForSuccess's inner New is a no-op), and RunInParallel (just passes ctx to each pin write — it doesn't pre-check cancellation itself).

Path-by-path: return point, pin state, pin-write count, what's left running

GoFor / GoTo (nonzero move), runs to completion. Returns once the poller sees position == target (poll period = stepperDelay), same as before. Pin writes: N steps x 4, plus exactly one final all-low write (4 calls) from the worker. Pre-change this was N x 4 plus an unbounded stream of all-low writes — the idle loop hammered doStop continuously until the next command. Afterward nothing is left running (worker de-energizes and exits). Pre-change a spinning goroutine remained forever. One benign race, present in both versions: GoFor can return a poll-tick before the worker's final all-low write lands, so pins may be energized for a few more microseconds after return.

GoFor with caller's context cancelled mid-move. Returns nil (the code deliberately swallows context.Canceled) while the worker keeps stepping to the target, then de-energizes and exits. Identical to pre-change except the goroutine now terminates instead of spinning at the target.

GoFor preempted by a new GoFor/SetPower. opMgr.New in the second call blocks until the first GoFor has fully returned, then the second call's doRun synchronously stops the old worker. Commands are fully serialized; at most one in-flight step completes on the old worker after preemption (pre-change: same one extra step, but with no wait and no guarantee).

GoTo with zero distance. Returns nil, zero pin writes, does not touch the worker — so a previously started SetPower run keeps going. Identical to pre-change.

SetPower / SetRPM (nonzero). Returns after handing off to a fresh worker. Worker runs indefinitely (target ±Inf) — intentional per the API. Exactly one goroutine exists regardless of how many commands were issued; pre-change each doRun leaked the previous StoppableWorkers object and briefly overlapped two stepping goroutines.

Stop / Close / ResetZeroPosition / zero-RPM GoFor / zero-power SetPower (all route through Stop). Behavioral deviations from pre-change, all intentional but worth your sign-off:

  1. Pins end all-low (4 writes). Pre-change Stop wrote nothing — pins stayed energized in the last step pattern (holding torque, coil heating) and a stopped worker could even issue one more step write after Stop returned. Now the end state is deterministic: worker fully exited, then pins low. Trade-off: holding torque is lost on Stop, and Close on a never-moved motor now writes pins where it previously wrote nothing.
  2. Stop latency. Stop now waits for the in-flight step, and doStep sleeps holding the lock, so Stop can block up to one stepperDelay. At normal speeds that's ≤600ms, but at very low RPM the delay is 60/(rpm·ticks) seconds — a crawl-speed SetRPM makes Stop slow. Pre-change Stop returned instantly (at the cost of indeterminate aftermath).
  3. New failure mode: Stop/Close called with an already-cancelled context can now return an error with pins not de-energized, because the all-low write uses the caller's ctx and real GPIO implementations honor cancellation. Pre-change Stop couldn't fail (it also didn't write anything). ResetZeroPosition inherits this error path. If you want Stop to be infallible, the de-energize write should use a background context — flagging, not changing.

ResetZeroPosition also gained correctness: pre-change it cancelled the worker without waiting, so an in-flight doStep could increment stepPosition after the reset, clobbering it. Now the worker is provably gone before the reset.

Comment thread uln28byj48/28byj-48.go Outdated
m.lock.Lock()
defer m.lock.Unlock()
direction := motor.GetSign(powerPct) // get the direction to set target to -ve/+ve Inf
m.targetStepPosition = int64(math.Inf(int(direction)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a change, but... this just doesn't work:

	fmt.Println("Pos:", int64(math.Inf(+1.0)))
	fmt.Println("Neg:", int64(math.Inf(-1.0)))

->

Pos: -9223372036854775808
Neg: -9223372036854775808

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants