One-off error in OperationSequence::process?

[Piezoid]

There is some things that I don't understand in klipper_estimator OperationSequence::process:

As I understand it, on a partial flush, the loop looks in reverse order for a move with an entry velocity that doesn't depend on the following moves. This will act as the starting point for propagating peak velocity to previous moves. When such move can't be found in the unplanned part of the buffer, nothing is done or yielded downstream.

When this search reaches an non-move operation which is also the first unplanned operation, it sets flush_count to its index. This enables the operation to be readily yielded by next_move. At least, I think this is the intent of this code.

Now the issue is that self.flush_count + 1 is actually the index of the second operation in the unplanned part of the buffer.

Another thing that I noticed: when the search can't find starting point for planing, self.flush_count is set to 0, which will plan again the moves at the start of the buffer. Not sure if this intended, but otherwise, the old value should be left alone.
Your implementation also recompute the planning of all moves during a complete flush (!partial), I don't think the original implementation does this.

Maybe I'm missing something, this literal translation from optimized python was quite difficult for me to grasp...

[dalegaard]

Hi Piezoid

I'll answer inline :-)

As I understand it, on a partial flush, the loop looks in reverse order for a move with an entry velocity that doesn't depend on the following moves. This will act as the starting point for propagating peak velocity to previous moves. When such move can't be found in the unplanned part of the buffer, nothing is done or yielded downstream.

We look in reverse order, assuming the machine comes to a complete stop at the end of the currently available command sequence. When we find a move that can no longer be affected by adding more moves, we set the flush_count and keep moving backwards. All moves we examine from this point are "certain" and so will be flushed. This is why we have the update_flush_count to track if we already found a new flush point.

When this search reaches an non-move operation which is also the first unplanned operation, it sets flush_count to its index. This enables the operation to be readily yielded by next_move. At least, I think this is the intent of this code.

Now the issue is that self.flush_count + 1 is actually the index of the second operation in the unplanned part of the buffer.

This was intended as an optimization to ensure forward progress in some specific case that happened while I was refactoring code. I don't actually believe it's required anymore, but it won't really hurt.

Another thing that I noticed: when the search can't find starting point for planing, self.flush_count is set to 0, which will plan again the moves at the start of the buffer. Not sure if this intended, but otherwise, the old value should be left alone. Your implementation also recompute the planning of all moves during a complete flush (!partial), I don't think the original implementation does this.

Setting flush_count to zero when nothing could be planned is indeed intended, during partial flushes, as it means the total duration of the buffered moves wasn't long enough to ensure that no future moves could affect them.

You are right wrt. recomputing the buffer on full flushes, that's simply an oversight on my part.

Mainly these issues probably stem from converting from a push style to pull style API. In Klipper, once moves are flushed they are pushed to the next stage. In klipper_estimator, they stay at the front of the move queue until they are pulled.

I'm not really happy with how this part turned out, as you note it's basically a direct port, warts and all. At some point when I have time I'll go through it and see if I can improve it a bit.

Best regards,
/Lasse

[Piezoid]

Thanks for the detailed reply!

Setting flush_count to zero when nothing could be planned is indeed intended, during partial flushes, as it means the total duration of the buffered moves wasn't long enough to ensure that no future moves could affect them.

I was noting that partial flushes that cannot plan anything shouldn't mark previously planned moves as unplanned. Although, this doesn't matter when pull driven: planned moves are all removed from the queue before the next partial flush.

I'm not really happy with how this part turned out, as you note it's basically a direct port, warts and all. At some point when I have time I'll go through it and see if I can improve it a bit.

I came with these questions while refactoring the planner to avoid mutations and "make the monoids more explicit". My goal was understanding the pipeline better before trying some variations (like tagging values to get the originals limits). If I end up with something simple enough I'll make a PR, but I fear that this project want to keep the code layout close to Klipper's for maintenance reasons.

[dalegaard]

I just pushed a commit that properly splits move sequences. I discovered, based on your input, that my last changes, making G4 pauses instead of indeterminate dwells, resulted in potential issues.

MoveSequences no longer contain timeouts, as those were the potential issue. Instead, there's now an OperationSequence which contains dwells and move sequences. This means MoveSequence::process(formerly OperationSequence::process) no longer handles non-moves at all. I did add a small optimization in the form of a loop to advance over any fill commands in a move sequence.

[Piezoid]

MoveSequences no longer contain timeouts, as those were the potential issue. Instead, there's now an OperationSequence which contains dwells and move sequences. This means MoveSequence::process(formerly OperationSequence::process) no longer handles non-moves at all. I did add a small optimization in the form of a loop to advance over any fill commands in a move sequence.

Nice! I'll close this, as it solves the issue.