gin66
commented
2 years ago The forceStopAndNewPosition() is combined with the new position, because it totally stops the stepper interrupt. This causes a little mess in book keeping of the current position and recovering from this mess takes too long for an interrupt service routine.
Alternatives could be:
- Maintaining the position in the interrupt. But for AVR I have squeezed out every us in order to guarantee 25kStep/s (for several steppers). Adding a conditional 32bit increment - only for super fast forceStop - is IMHO not worth it.
- Adding to each command the position reached by this command. The penalty - again: only for super fast forceStop - is 4 Bytes 16 queue entries 3 supported steppers on AVR = 192 Bytes. Again, I don't think, this is worth it.
- Add a compilation flag (preprocessor) to enable the feature, if required. Tradeoff: not available for Arduino IDE users, increased complexity for testing and the compilation flag would still enable a bit complex solution - only for this superfast forceStop.
So in a nutshell, I don't think this makes sense to implement it in FastAccelStepper. In case you have a much better solution to this problem, which I currently cannot come up with, then I will definitely look at it.
But taken up your proposal for a forceStop() with setting new position: In course of the rework in 0.25.x, there is now a super easy solution to stop the stepper quickly (aka within approx. 20ms) without loosing the position. Perhaps this is sufficient.
For the one-liners: Some of the one-liners are accessing the fas_queue, which I actually do not want to leak to the application. Even though, the current file set up may leak it, but this may change in a later, fixed version. And yes, inline would be safe couple of bytes and one call/return-pair. Just those functions are not meant to be called hundred thousand of times per second. So the tradeoff is acceptable.
Yes, writing a good API is difficult, because the one, who designs it, may be blinded by too much insight in the internals. And some of the API implementation is like it is due to some internal implementation details, or for simplicity of a quick and dirty solution. The drawback of changing the API for the better: it may break 3rd party applications or the test code in use by FastAccelStepper. Anyway, good ideas are always welcome.
devrim-oguz
Hello, I didn't want to include this in the previous issue, so I've opened a new one. I don't understand why you make composite functions like
forceStopAndNewPosition()while the user can separately call a forceStop and setCurrentPosition. Making the functions multi-use makes it harder to divide functionality. For example, in my project I run the motor to advance an arm to a position then move it until it touches an item to be grabbed. In this aspect, I don't want to set the new location but want to stop the motor immediately. Also, many functions likeforwardStepandbackwardStepare one-liner and can be marked as inline functions for better compiler optimization ( something like:inline void FastAccelStepper::backwardStep(bool blocking)) I really love your library and use it very often. But your API needs a bit of improvement. I may one day update these and create a pull request when I'm not so busy. Please take this as an advice and not criticism. I really love your library, and thanks for your work. I want to help as much as possible in the future. (my other account is devrim-oguz, I might create a pull request from there.)