[2.0.x] Small assorted collection of fixes and improvements

[ejtagle]

• Get rid of most critical sections on the Serial port drivers for AVR and DUE. Proper usage of FIFOs should allow interrupts to stay enabled without harm to queuing and dequeuing.
  Also, with 8-bit indices (for AVR) and up to 32-bit indices (for ARM), there is no need to protect reads and writes to those indices.
• Simplify the XON/XOFF logic quite a bit. Much cleaner now (both for AVR and ARM)
• Prevent a race condition (edge case) that could happen when estimating the proper value for the stepper timer (by reading it) and writing the calculated value for the time to the next ISR by disabling interrupts in those critical and small sections of the code - The problem could lead to lost steps.
• Fix dual endstops not properly homing bug (maybe).

[Roxy-3D]

Very nice! This will save a few clock cycles during the Serial ISR !

Update: This is one of those times where once you see it, it is obvious (and EASY!!!) to do. How is it we didn't notice this earlier?

[ejtagle]

@Roxy-3D : Don't ask. I found it by accident, while trying to figure out edge cases for the stepper ISR. Then I suddenly remember this to be in my mental "todo" list.. ⭕️ --- So.. And, in the case of 32bit ARM, there is even extra margin for improvements, as the UART can be combined with DMA to get reception at nearly 0 cost....

[Roxy-3D]

Obviously that was a rhetorical question. But it does illustrate one of the reasons why Open Source can generate such high quality software....

[ejtagle]

And to be honest is the way I usually do things... I little bit of intuition, a lot of hard work... Something I like is that things are stabilizing slowly but surely after that big PR... And i actually hope for good! ... There is still lots of things to try and improve, but an stable base is needed. That is what i am trying to get, and i think we are pretty close! 👍

[Roxy-3D]

And, in the case of 32bit ARM, there is even extra margin for improvements, as the UART can be combined with DMA to get reception at nearly 0 cost....

Is this something that should be pushed down into the HAL? To do this, the UART's Rx and Tx ready signals would have to be able to be connected to a DMA channel. And in many (most???) cases, that isn't available. Certainly not on an AVR?

But if the HAL can do it... it probably should be done. Cutting out ISR routines on a per character basis has to be a good thing.

[ejtagle]

The AVR UART driver probably is now in the best possible condition. AVR does not support DMA
Something i DID consider was to run the SPI bus in interrupt mode... Why ? ... Because of the st7920 display! - But it is a good question if cycles could be saved or not by doing that instead of polling.

The other thing that could be redone is how we create the pulses for the steppers. Right now we are using about 2uS width pulse, and that is done by using a software delay inside the ISR... On AVR that is about 32 instructions. Then, LA uses an independant extra delay of 2us for the stepper pulses.
If that delay could be used for something inside the ISR, we could save 32 cycles ...

[thinkyhead]

I've done a couple of pretty long print jobs (8-13 hours) using the latest code, and results have been great — not a single glitch or stutter. I look forward to seeing how this (and the 1.1.x counterpart) improve them even more!

[thinkyhead]

Since the planner/stepper refactor we've had an increasing number of reports of "Probing failed", and I've confirmed that this bug exists in some of my testing. When I went to track it down I found that sometimes a probe move would stop early without triggering the probe, even though it should have gone well below the point where it was stopping. This seemed more common when doing double or multiple-probing at each point.

I haven't completely tracked it down, but my hypothesis is that it's related to do_probe_move failing to update the planner with the current position on some previous probe move. This would be consistent with the probe stopping early, since if a previous probe move stopped at Z=0 but the planner thought it was at Z=-10, then the next probe move would stop early.

The set_current_from_steppers_for_axis and SYNC_PLAN_POSITION_KINEMATIC calls at the end of do_probe_move should ensure that the planner and the coordinate system both know the real physical position of the Z axis when the probe triggered. Of course, this depends on the correct behavior of the stepper ISR — first, that it should stop on a probe or endstop trigger, and second, that it should freeze its count value at that point.

I'll continue my testing and try to to track down the exact reason why the probe move is stopping early. So far my best guess is that the planner is not being corrected after the probe move stops, thus leading to these moves stopping short of the probe triggering.

Thinking it might be due to a race condition, I've added a commit to this PR and its companion to set the stepper positions right away on the call to planner._set_position_mm when there are no blocks in the planner queue, rather than using a "sync" block to update the stepper counts "soon." I don't have high confidence that this will actually help with the "Probing failed" bug, since that mainly depends on the planner's positions and not the stepper counts, but it seemed like a good idea nevertheless, to make sure that set_current_from_steppers_for_axis can be used right away after calling planner.set_position_mm without needing to also call stepper.synchronize.

[ejtagle]

Well, there is an easy test for that: Place something above the bed. A cardboard, for example. And you will notice that when the probe triggers, the bed stops. I also think this could be a race condition, but the counts are frozen when a trigger of the probe happens
When the probe triggers, it calls Stepper::endstop_triggered(), and that calls Stepper::quick_stop()
But just before doing that, it does
endstops_trigsteps[AXIS_Z_PROBE_MIN] = count_position[AXIS_Z_PROBE_MIN]
quick_stop() just sets a flag : abort_current_block = true;
That forces the NEXT stepper ISR to abort the executing movement (only the current block. If there are other queued movements, they will execute!)

[ejtagle]

Have you tried to disable ENDSTOP_INTERRUPTS_FEATURE ... That could be making a difference.
I see that once the block is cancelled, it sets axis_did_move to 0.
once that happens, there is no detection of z_probe anymore, as in Endstops::update() the z-probe input is only read if z axis is moving...
That means there is a race condition... Once the z-probe is triggered,... let me think ...

[ejtagle]

No, seems to be fine. Once the z-probe triggers, there is a delay of 1 isr and the motors will stop and hold the position.
But do_blocking_move() has a call to planner.synchronize() that ensures the do_blocking_move() will wait until the movement is actually cancelled.
I see no problems with that. Then you can examine the trigger_state() to find out the cause of the movement ending.
The thing i haven´t figured out yet is how the stop position is determined, but i think you do, so it is just a matter of adding some ECHOs there to see the position ;)

[thinkyhead]

I haven't yet confirmed that the planner position is incorrect on the start of each probe move, but I will add that logging and test shortly.

[thinkyhead]

how the stop position is determined

set_current_from_steppers_for_axis(Z_AXIS)

[ejtagle]

Seems to be fine. The positions are properly pulled from the steppers... 👍

[thinkyhead]

I haven't been able to reproduce the probing issue with my Cartesian (CR-10S) using the latest code but it was definitely happening on a SCARA I was working on a few days ago with recent firmware. I'll get back to testing that machine on Monday or Tuesday and see if I can narrow down the cause on that particular machine.