Alternative impl. of LCD-based manual movement

[thinkyhead]

Recently we found that LCD-based movement can cause a stack overflow if the planner buffer gets full. This is because plan_buffer_line() calls idle() in a loop. Since idle() calls lcd_update() which in turn may call plan_buffer_line() again, if you do a lot of manual moves with a slow axis (like Z), the stack quickly overflows due to nested calls to plan_buffer_line().

The previous workaround was to skip calling plan_buffer_line() if the buffer had some moves already queued up. Unfortunately this may cause current_position to be different from the last place moved, because any moves that can't be added are just dropped!

This workaround changes the way that manual moves are applied, so moves won't be dropped, while at the same ensuring that lcd_update() will never call plan_buffer_line() if it would result in recursion.

The key is to simply set a flag that a manual move has been applied to one of the axes. Whenever lcd_update() is called (for example, from idle()) it will check to see if any manual move was applied but postponed. If so, and there's room in the planner, the move is added to the planner.

A helpful feature of this change is that in the case of a long delay only the most recent destination set will be added to the planner. So if a single Z move takes a long time while there's a full planner buffer, fewer additional Z moves will end up being buffered.

This change also opens up the potential for interruptible manual moves, which could be achieved by setting a separate manual move destination, then adding smaller moves to the planner until the destination is reached. In this way, if the destination ends up being in the opposite direction from a previous destination, the direction change can happen more quickly.

[thinkyhead]

Here current_position has already been set, but if the planner has some moves already the move is just dropped. This will cause the position to be out of sync until the next planner move. The solution is to set a move flag to add the move as soon as possible, then try once to do it immediately.

[Blue-Marlin]

I don't see the difference in your solution. The moves you can't do immediately are accumulated in current_position. We do not leave the move axis menu so the in formation about what axis to move is stored in 'currentMenu'. As long as we turn or klick the encoder, it does not make a difference if we enqueue the next move at the beginning of lcd_update(), or a tiny bit later, when we are back in the move menu.

[thinkyhead]

If the encoder isn't actually clicked, it would seem to never add that last move, because lcd_return_to_status ends up being called after a time-out.

The main thing I wanted to get in place and try was the manage_manual_move function to see if there was some way to make manual moves interruptible. So if you change your mind and spin in the other direction there will only be a small move before it reverses direction – instead of having to wait for a bunch of queued moves to complete. But I have yet to work out the details.

[Blue-Marlin]

!planner_is_full() has much more latency than (movesplanned() <= 3`.

[thinkyhead]

The stepper ISR code needs to be light and fast, but portions of the code that are written to run in the main loop() should just aim for simplicity and saving bytes where possible.

Still, do you think it would help to FORCE_INLINE the planner_is_full function and move its definition to planner.cpp? Apart from not being inline it shouldn't have any overhead compared to movesplanned(). They both return a char.

I see that making it FORCE_INLINE saves 18 bytes.

[Blue-Marlin]

No, sorry i have not been precise in what i said.
With planner_is_full() you have to wait up to 16 moves until the machine stops - with (movesplanned() <= 3) only 3.
3 because 1 or 2 does not give the planer the chance to optimize the accelerations - you'l have stops between the single moves.

Let's begin with turning the knob.
The first enqueued moves are all short - speed is low - until the buffer is full (or we reach an other limit (3)).
From there on the steps size will increase.
Finally the steps are long enough (take enough time) to turn the encoder back and forth without enqueuing new moves - only changing the size of the next move.
Finally you are able to send moves with a complete axis length.
Waiting for 16 full axis length moves, until the tool stops, is a bit long? Isn't it?

[thinkyhead]

I don't see how any 16-move wait could occur. Both movesplanned() < 3 and planner_is_full() have the same "problem." Both of them, if they are failing, must wait until some previously-added move finishes. In the first case, it will be a move that was added a couple moves ago. In the latter case, it will be the move that was added perhaps several moves ago, or perhaps just a couple of moves ago. In both cases, if the check fails, and then a move is added before we come back to our check in the next loop (not likely, since we're probably not actively printing), it will continue to fail until finally there is a single slot (or more) available in the buffer. The only difference I see is that movesplanned() < 3 will begin skipping moves sooner than planner_is_full. The moves it ends up skipping are not guaranteed to be shorter, necessarily, since the encoder values still keep going up.

Anyway, we can probably test this scientifically and see what actually happens. I would be curious to know.

In any case, the current solution seems pretty reasonable. I was thinking of the idea of having the moves be delayed until the encoder stops for 1/4 second. But now I think it's not a good idea. It will tend to encourage much longer single moves, which would especially be a problem with the E axis. I think it's better if either (A) the movement happens quickly as a reaction to turning the knob, or (B) the movement waits until the knob is clicked.

[jbrazio]

The encoder wheel..
This is my humble opinion: By turning the wheel the movement shouldn't be automatically sent to the planner, it should have like a 500ms/1s timeout between I stop at a number and the movement is sent, this will allow the user to fine adjust the position without overloading the planner. The encoder wheel should only allow one movement in the buffer, the active movement, and by turning the wheel while moving this should not send any other movements to the planner until the current movement finishes.. when it finishes the next movement is read from the encoder wheel and so on and so forth.

[thinkyhead]

@jbrazio I like your idea also. As long as the wheel is being moved there should be no movement. Only when it pauses long enough. I think 1/2 second is probably pretty good. This PR lays the groundwork for just that kind of handling.

[jbrazio]

Great ! My experience with the encoder wheel is when moving I tend to over/under shoot positions and this adds to the planner queue, the timeout to "accept" the user input will allow those corrections to happen without filling the planner with unintentional moves.

[thinkyhead]

@jbrazio See how this now works for you. It enforces a 1/2 second delay when using the Move Axis options, so it should be more easy to control.

I was hoping to modify it a bit more, so it splits up long moves into shorter ones (1-2mm for XY and E, 0.2mm for Z). But since the move options directly modify current_position I've decided to wait, try implementing these moves without modifying current_position (until manage_manual_move).

If "interruptible moves" works better than "delayed moves" we might opt for that – or make the behavior configurable to user-preference.

It occurs to me there might be one more option, which is to still use the delayed move technique, but always stop and purge the current move and start a new one. This would allow moves in the opposite direction to be initiated with only the usual 0.5s delay.

Stuff to experiment with later!

[thinkyhead]

Patched for singletons.