Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix attempt for https://github.com/gin66/FastAccelStepper/issues/250 #252

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

SHWotever
Copy link
Contributor

On quick direction changes a direction toggle could be missed. My understanding is that this code branch "restarts" the motion right at a stop event. But the direction change would not be applied either doing a few steps in the wrong direction, or if a large move was initiated at this time a much more impacting movement in the wrong direction.

If it's motion restart it feels correct to toggle direction before that ultimate step, but i'm not 100% sure.
Since the direction step is now occurring in the ISR multiple times I moved it as a macro like the others stepper unitary actions for readability.

On quick direction changes a direction toggle could be missed.
My understanding is that this code branch "restarts" the motion right at a stop event. But the direction change would not be applied either doing a few steps in the wrong direction, or if a large move was initiated at this time a much more impacting movement in the wrong direction.

If it's motion restart it feels correct to toggle direction before that ultimate step, but i'm not 100% sure.
@gin66
Copy link
Owner

gin66 commented Apr 22, 2024

I am still wondering: If this problematic branch is triggered by a pause with direction toggle requested, then this toggle will still be lost. Perhaps the toggle should be before the test: if (e->steps > 0) ?

This would explain, why the direction delay has not solved the problem.

@SHWotever
Copy link
Contributor Author

I feel like since it's a motion "restart" (it was about to stop but it was restarted right away from my understanding). It could be harmless above, if the queue processing had requested a toggle it must be applied. This morning I did further testing, and this solution appeared to be the one keeping both steppers perfectly in sync (meaning that "moves" were consistent on both steppers) the second solution of applying first the final pulse and then switching direction seemed to introduce a slight drift at the end of a long session which makes me think those steps must be applied in the newest direction.
That's not done with exact unit testing unfortunately so I can't tell how really accurate is that observation.

I can try to move it up. And test, looks like my test case is really good to see any bad choices in that matter :D

@gin66 gin66 merged commit da0c2a1 into gin66:master Apr 26, 2024
25 of 26 checks passed
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