RSDK-12491: performance improvements and full step mode support#8
Conversation
| time.Sleep(m.stepperDelay - elapsed) // will return immediately on negative or zero duration | ||
|
|
||
| /* | ||
| m.logger.Infof("stepperDelay: %v, elapsed: %v, totalDurationinSetPins: %v, sleptFor: %v", |
There was a problem hiding this comment.
This will be removed in a followup pr after I'm done profiling.
| return err | ||
| } | ||
| m.workers.Stop() | ||
| if m.workers != nil { |
There was a problem hiding this comment.
this was a bug that caused a panic if the stepper is Closed having never had an operation run on it.
There was a problem hiding this comment.
Fun! You had a TODO about actually using a mutex to protect state variables. Assuming protecting the assignment/access of workers is part of that TODO.
There was a problem hiding this comment.
indeed. when i tried to dumbly add mutexes i deadlocked somewhere so i just did this so i could keep working.
Dan Gottlieb (dgottlieb)
left a comment
Dan Gottlieb (dgottlieb)
left a comment
There was a problem hiding this comment.
I think this PR is achieving what it has scoped out to do. Just highlighted the parts I wanted to make sure were intentionally out of scope.
| err := m.doStep(doRunCtx, m.getStepPosition() < m.getTargetStepPosition()) | ||
| if err != nil { | ||
| m.logger.Errorf("error stepping %v", err) | ||
| return |
There was a problem hiding this comment.
To be clear -- stoppable workers will repeatably call this lambda function. So if we hit an error here, I expect doStep to be retried "immediately".
I think addressing anything of that nature was understandably out of scope for this PR. But wanted to make the API clear. It's not obvious to me this usage was fully aware.
There was a problem hiding this comment.
good point. i'll address it in the next pass.
| return err | ||
| } | ||
| m.workers.Stop() | ||
| if m.workers != nil { |
There was a problem hiding this comment.
Fun! You had a TODO about actually using a mutex to protect state variables. Assuming protecting the assignment/access of workers is part of that TODO.
2 participants
Performance on the rpi zero 2w is notably better up to about 5rpm with full step mode. Testing on an rpi5 pending.
Key changes:
The overhead of module to module communication has a significant impact on our ability to drive these steppers at high speed.
There are still a number of issues with this module (see TODOs) that I will follow up on after this.
This repo hasn't been updated in a while so some changes were needed to appease linter and compiler.