Skip to content

[2.0.x] Refactor, optimization of core planner/stepper/endstops logic#10688

Merged
thinkyhead merged 7 commits intoMarlinFirmware:bugfix-2.0.xfrom
ejtagle:bugfix-2.0.x
May 20, 2018
Merged

[2.0.x] Refactor, optimization of core planner/stepper/endstops logic#10688
thinkyhead merged 7 commits intoMarlinFirmware:bugfix-2.0.xfrom
ejtagle:bugfix-2.0.x

Conversation

@ejtagle
Copy link
Contributor

@ejtagle ejtagle commented May 11, 2018

Jitter reduction, pulse regularity

Marlin, due to the way the Stepper ISR was written, was introducing severe jitter to the created stepper pulses. By "jitter" it is understood, once the Stepper ISR is fired, the amount of time elapsed from the start of the ISR (timer compare match) to the exact place in time where the stepper pulses are created varies wildly depending on several conditions of the preceding code of the ISR.

The other problem that also was causing jitter was that Marlin abused the use of Critical sections to protect against corruption of the variables of the Stepper and Planner. There were sections of the Planner that were running the Planner algorithm with interrupts disabled!! - Having interrupts disabled means the Stepper ISR does NOT execute on time, and the Stepper pulses will be delayed by all the duration of the Critical Section.

That will cause noises, and in extreme cases, could lead to lost steps. On interpolating drivers, such as TMC2xxx, the time interpolation algorithm that those drivers internally have create non uniform accelerations and deaccelerations between steps

The first step to try to reduce and/or eliminate jitter was to determine all the dependencies of the Planner and Stepper. Marlin had crossed references from the Planner to the Stepper and vice-versa, making extremely hard to determine that.

Refactor, division of Planner/Stepper

So, we refactored the Stepper and Planner classes in such a way that the Stepper does not need to know anything about the Planner (except the block_t structure), and Marlin does not have to know anything about the Stepper - All interactions with the Motion system are now handled by the Planner class.

With that refactoring done, now the structure is pretty clear: We first proceeded to reorder the stepper ISR routine in such a way that the first thing that is done was to create the step pulses, then after that time critical part, all the querying for new blocks (if needed) of the Planner is done.

Move endstop check to Temperature ISR

After that, we realized that there was some things that can be offloaded from the Stepper ISR, specifically, the endstop processing can be fully run with no problems at all from the temperature ISR. So we also did that change - The Stepper ISR should be as simple as possible!!

After removing the endstop processing from the Stepper ISR, there was no need for the ISR splitting. We reimplemented the ISR splitting in a much simpler way that works very well

Drop Planner Critical Blocks

The next thing was to remove all critical sections that are not required. The planner does NOT require a critical section to queue blocks, as we are using a circular FIFO queue. We can exploit that and avoid critical sections, thus reducing jitter.

And, if a critical section is needed (to ensure atomic reads or modifications of structures), instead of globally disabling all the interrupts, we selectively disable the ISR that could interfere with the operation for the smallest period of time possible.

Fix Due ISR Test

We found a bug in the Due implementation on the macro used to determine if an ISR was enabled. It was always returning the interrupt as disabled. On Sam3x, there is an specific register to check if interrupts are enabled or not. It was not used, and we were reading a WRITE-ONLY register. That bug was fixed.

Optimize Priority Emulation

Then, on AVR, another cause of ISR latency was the ISR priority emulation done in Software, between the Temperature ISR and the Stepper ISR. We wrote a custom assembly prologue and epilogue for those Stepper and Temperature ISRs, so we can do the emulation as fast as possible, reducing the latency of the interrupt processing from 60 to 12 cycles.

Due Emergency Parser

We added an Emergency parser to the Due CDC.

Optimize Planner maths

Then we started Planner optimizations. Using speed squared saves TONS of SQRTs on the Planner algoritm, easily improving 10x its performance

Complete Junction Deviation

We have added all the Junction deviation features, as discussed in #10341 (comment)

Smarter Delays

Finally, we factored out all delays into proper common routines by architecture (AVR/ARM). No more delays in NOPs. Now you can use DELAY_NS() and DELAY_US() and nops are calculated based on CPU speed (and also, uses loops instead of tons of NOPs). DELAY_NS() and DELAY_US() use busy loops, so are safe to be used even on ISRs

Optimization of Endstop handling, and avoiding polling as much as possible if interrupts on pin change are available.

Proper skipping of SYNC blocks, both in planner and in trapezoidal shape calculations

Alternative joining algorithm (this is under evaluation ... it ´s done and commited, but opinions are welcome)

@ejtagle ejtagle changed the title Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification May 11, 2018
@ejtagle ejtagle changed the title Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification / Junction Deviation (GRBL with hoffbacked fixes and improvements) May 11, 2018
@ejtagle
Copy link
Contributor Author

ejtagle commented May 11, 2018

Of course, i have tested this in a Due based board. I also flashed it into a Arduino Mega, but can't connect the Mega to my printer anymore, so the test was just to check it does not hang and responds to commands.
For anyone reading it, please try it if you can, to make sure it works for you 👍

@ejtagle ejtagle changed the title Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification / Junction Deviation (GRBL with hoffbacked fixes and improvements) Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification / Junction Deviation (GRBL with @hoffbacked fixes and improvements) May 11, 2018
@Zwaubel
Copy link

Zwaubel commented May 11, 2018

Nice work, I'm really excited about this!
Just tried to flash my mega+ramps but got the following compiler error:

Marlin\src\gcode\config\M301.cpp: In static member function 'static void GcodeSuite::M301()':
Marlin\src\gcode\config\M301.cpp:54:29: error: 'lpq_len' was not declared in this scope
if (parser.seen('L')) lpq_len = parser.value_float();
^
In file included from Marlin\src\gcode\config\../../inc/MarlinConfigPre.h:28:0,
from Marlin\src\gcode\config\../../inc/MarlinConfig.h:26,
from Marlin\src\gcode\config\M301.cpp:23:
Marlin\src\gcode\config\M301.cpp:55:14: error: 'lpq_len' was not declared in this scope
NOMORE(lpq_len, LPQ_MAX_LEN);
^
Marlin\src\gcode\config\../../inc/../core/macros.h:92:29: note: in definition of macro 'NOMORE'
#define NOMORE(v,n) do{ if (v > n) v = n; }while(0)
^
*** [.pioenvs\megaatmega2560\src\src\gcode\config\M301.cpp.o] Error 1

This error can be fixed by adding #include "../../Marlin.h" to M301.h but I'm not sure if this is the proper solution since this include isn't actually required in the original bugfix-2.0.x branch

Now I'm getting another compiler error:

Marlin\src\module\planner.cpp: In static member function 'static bool Planner::_fill_block(block_t*, bool, const int32_t (&)[4], const float (&)[4], float, uint8_t, const float&)':
Marlin\src\module\planner.cpp:2152:30: error: 'JUNCTION_ACCELERATION_FACTOR' was not declared in this scope
vmax_junction_sqr = (JUNCTION_ACCELERATION_FACTOR * JUNCTION_DEVIATION_FACTOR * sin_theta_d2) / (1.0 - sin_theta_d2);
^
*** [.pioenvs\megaatmega2560\src\src\module\planner.cpp.o] Error 1

Okay, my fault... caused by simply copying my configuration file without actually taking a look if anything had changed from yours to mine.
Obviously #define JUNCTION_ACCELERATION_FACTOR was missing in my configuration_adv file.

@Zwaubel
Copy link

Zwaubel commented May 11, 2018

Results after intial testing: still buggy!

ABL moves are now weirdly split up into at least two motions per move, meaning while moving from one testing point to another the printer is accelerating and decelerating at least twice per horizontal motion and vertical motion.

Printing (from host or SD) does not work:
After performing ABL the printer is just stuck at the starting point while octoprint is receiving "Recv: echo:busy: processing" (temperature has been reached and is stable) and the lcd is very unresponsive (you can even see how each line gets updated when navigating through the menus)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth making the 1.0 configurable?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Straight line acceleration is taxing on the linear advance, and conserving momentum through the corner helps. Being able to set a high corner acceleration and a low block->acceleration is pretty useful. Conversely, lowering the corner speed could help ringing, even with a high block->acceleration with the bezier jerk control.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, if this was available on the menu, the two numbers could be meaningfully calibrated. Since the cornering code is actually following a corner, and the deviation one is a proxy for the give of the hardware at this point, you can create a rectangle with a tiny chamfer on the corners on only one side. The deviation value can be tuned until the amount of force is similar on all corners, and then the acceration could be tuned for an acceptable level of force. If ringing is unacceptable, the acceleration value can be tuned, but the deviation value is specific to the machine. At some point, we may actually follow the deviated path, in which case it wouldn’t need calibration, but is meaningful to adjust.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 11, 2018

@Zwaubel : There could be a problem with the Planner algorithm. The fact that moves are being split in half seems to indicate that the planner is unable to merge them.
When the movement queue becomes empty, then the next queued movement will be split in 2 movements (we didn´t change that, that is the way it worked previously).
The idea behind that is that there a movement that can´t be changed, because it is being executed by the Stepper drivers. So, splitting the movement in 2 parts allows at least to begin chaining and planning movements (only the movements that are in the queue of movements to be done and not being executed by the stepper driver can be modified by the planner
Something is taking too long to compute, and that causes the movement queue to completely drain before the next movement is queued.
That is what causes the "jerkiness" and the "2 movement splits". This PR does not touch the ABL code.

The question is ... Is ABL so computationally intensive that the AVR micro is unable to perform it in realtime ? ... If disabling ABL all problems go away, that probably is the answer, and we should be carefully be checking and optimizing the ABL code to make it run faster....

My main concern here is to make sure the problem is just caused by lack of processing power, and not by any other bug. I have already run a full print from the SD with no issues, but i am using an Arduino Due that has much more power than the AVR MCU...

@ejtagle ejtagle changed the title Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification / Junction Deviation (GRBL with @hoffbacked fixes and improvements) [2.0.x] Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification / Junction Deviation (GRBL with @hoffbacked fixes and improvements) May 11, 2018
@jokeman3000
Copy link

jokeman3000 commented May 11, 2018

I tried this on an MKS-SBase and there appears to be something wrong. I tried using the latest in @ejtagle branch. Configs attached,
Archive.zip
Serial log from octoprint:

Send: N27 G92 E0*114
Unexpected error while reading serial port, please consult octoprint.log for details: SerialException: 'device reports readiness to read but returned no data (device disconnected or multiple access on port?)' @ comm.py:_readline:2417
Please see https://faq.octoprint.org/serialerror for possible reasons of this.Changing monitoring state from "Printing" to "Offline (Error: SerialException: 'device reports readiness to read but returned no data (device disconnected or multiple access on port?)' @ comm.py:_readline:2417)"Connection closed, closing down monitor

The SBase appears to be completely locked up not responding to input from the LCD or updating it.

@hoffbaked
Copy link

hoffbaked commented May 11, 2018

My machine croaked as soon as it starts to print from the sd card. I tried turning of mesh leveling and bezier jerk control thinking it was low memory, and the only difference was that it very slowly moved an inch and died. On the serial output, I get interesting stuff like:

echo:Unknown command: "G21			          "
echo:busy: processing
echo:busy: processing
X:0.00 Y:0.00 Z:0.00 E:0.00 Count X:0 Y:0 Z:0
echo:Unknown command: "M420 S1"

I enabled the M100 free memory watcher, and the last output was:

echo: Free Memory: 1825  PlannerBufferBytes: 1408

@jokeman3000
Copy link

Right, for me it locks up just as soon as it is about to start extruding. So I think we're seeing the same thing.

@Zwaubel
Copy link

Zwaubel commented May 11, 2018

@jokeman3000 @hoffbaked
I had a similar problem, but deactivating the linear pressure advance option in the configuration file did make my machine functional again. You may try so aswell just to check if the problem is consistent.

@ejtagle
Thanks for the explanation. Deactivating ABL did not change anything. But disabling linear pressure advance did partially fix it.
The printer is still behaving a bit unexpected from time to time: Sometimes homing X and Y axes will lead to the extruder slamming into the endstops several times until noticing that it is already at the home position (not crashing but just triggering the switches a lot more than once).
And fast travel moves seem to be randomly split up causing "jerkiness". But your explanation might be the reason for this...
What I notice during prints is, that the printer got a whole lot more silent. So the stepper driver control and the motion planning definitely seems to be way improved by your work.

@jokeman3000
Copy link

@Zwaubel

On commenting out linear advance, I'm able to get a little further but the printer stops halfway through the line. It's not hung like before, in other words, I can use the LCD and make changes there and it is still reporting serial output to octoprint but it is not printing or cancelling.

What's interesting is that despite having linear advance enabled before, it was set to 0 which means it was effectively off (not changing that value in gcode).

Regardless, thanks for your help, one step further is always good!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're sure this works. The A() macro prepends a space, moving this directive to the second column. The original code has it in the first column.

Copy link
Contributor Author

@ejtagle ejtagle May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove it altogether. This directive is for Cortex M0 MCUs, and none of the currently supported ones are 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if it has no ill side-effects, we should leave it in so we don't have to remember to put it back. Thinking ahead is good.

@thinkyhead thinkyhead changed the title [2.0.x] Refactoring of Stepper and Planner classes / Jitter reduction of pulses / Planner Speedup / Delay unification / Junction Deviation (GRBL with @hoffbacked fixes and improvements) [2.0.x] Refactor, optimization of core planner/stepper May 11, 2018
@thinkyhead
Copy link
Member

thinkyhead commented May 11, 2018

This is epic. But, maybe too much all at once!
It would be nice to have each of the 10 categories of changes as a separate PR.
If you wish, I can rebase this, squash all the extra "oops" commits, and split these up more neatly.

One change I can say is wrong: Endstop handling needs to be in the stepper ISR. We need to catch endstop hits within no more than a few steps from where they trigger. If it were possible to trust endstop tests from the lower-priority (and potentially blocked) temperature ISR, we'd've done that a long time ago. Endstop interrupts is the only optimization we could muster.

Also, I recommend always making a copy of branches that coincide with upstream rather than modifying them directly. Makes it easier to keep your working copy / origin in sync with upstream.
image

@Roxy-3D
Copy link
Member

Roxy-3D commented May 11, 2018

This is epic. Maybe too much all at once. It would be nice to have each of the 10 categories of changes as a separate PR.

This work is very much appreciated!!!

But it is scary to merge this much low level code all at once. I was going to ask "Can we identify 3 topics and group the changes into different buckets?" That way we can merge one set at a time... And wait a few days and make sure we don't have problems. Or, if we do have problems, at least we know where to look to understand them and fix them.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 12, 2018

@thinkyhead @Roxy-3D ... Yes, i am aware of the extents of the changes. But, the main problem is that the specific Stepper/Planner refactoring only works if done all at the same time.
There are some changes that could be split (for example, the delay refactoring could be applied before the refactoring), and some minor corrections (the Emergency parser for the USB CDC of Due, and the fix to the STEPPER_INTERRUPTS_ENABLED() on Due

Relating to the Endstop on the Stepper vs Endstop on temperature ISR... Not sure @thinkyhead . Let me explain. You are right, we need at least to poll (why in hell aren´t we exploiting the interrupt on change feature of the endstops?? .. The ISR is there, on 32bit CPUs can preempt the stepper ISR, on AVR also can preempt it, and we use it JUST TO SET A FLAG??? )

Well, returning to the topic, we need at least one polling every 1mS so the temperature ISR (that is never shut off) seems to be a very good candidate for this.

And even, as said before, if we are using the interrupt on change, in fact i find the idea of polling very bad. There are easy ways to handle debouncing, and to be honest, we don´t need debouncing at all. The first change is what we want.

If we don´t have interrupt on change, then essentially, doing the polling on the Stepper ISR vs doing it on the Temperature ISR is nearly the same. The only thing we must be sure of is that the maximum ISR rate of the Stepper when homing (because that is the only moment when we need EXACT detection of limit switches) is equal or less to the Temperature ISR.

Paying the price of limit switch detection on the Stepper ISR, when we use it just ONCE every print, makes no sense at all. Any code that we add to the Stepper ISR reduces the maximum step rate, and can introduce pulse jitter.

Regarding the splitting of this PR, i am open to any ideas @thinkyhead . Somehow i feel is a race, because this changes so much that every other PR that is accepted makes this PR harder and harder to integrate ... ;(

@ejtagle
Copy link
Contributor Author

ejtagle commented May 12, 2018

I'll try linear advance. I admit i do not use it, just because Bezier speed control is not properly compensated by LA. But i'll try it nevertheless.
What i am trying to achieve and ensure is that after this refactoring, everything that used to work still does 👍

@Roxy-3D
Copy link
Member

Roxy-3D commented May 12, 2018

You are right, we need at least to poll (why in hell aren´t we exploiting the interrupt on change feature of the endstops?

Because most boards don't have all the endstops on pins that came be declared as interrupts. RAMPS does have all of the end stops on pins that can be declared as interrupts. (Bonus points to the designer of the RAMPS board!!!!)

With that said... How about if we do something like this: How about anything we can separate out easily, we do that that as a first pass? And we get that working with no hiccups everywhere.

And then we circle back and figure out how to get the rest merged next?

@thinkyhead
Copy link
Member

How about anything we can separate out easily, we do that that as a first pass?

My plan exactly. I'm on the case now.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 12, 2018

@Roxy-3D: Well, as @thinkyhead suggested, all changes to the planner/stepper can be merged. I'd split the modifications to HAL.h in a separate patch also.
The rest is very hard to split, would require a lot of work...

@hoffbaked
Copy link

@Zwaubel Yep. Disabled linear advance and it's working great.

@jokeman3000 My screen was never unresponsive, and it was still maintaining temperature.

@jokeman3000
Copy link

@hoffbaked - could just be an issue with my config or the mks-sbase then. I've never seen it lock up like that.

With linear advance disabled, I do get further but it's almost as if the serial port on the board no longer receives commands. It has no issue transmitting though.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 12, 2018

Let me check. I did not change LA code, except the LA scheduler at the Stepper ISR...
I want to be sure if LA is the culprit or not ... ;)
I have 2 small pieces of code that i do suspect ... One are the changes to reduce Stepper latency on HAL.h (Only for AVR), and the other is the Stepper ISR scheduler, that is slightly changed when LA is enabled...

@ejtagle
Copy link
Contributor Author

ejtagle commented May 12, 2018

The good news is that if I enable LA, then i have exactly the same symptoms @Zwaubel and @hoffbaked had. That is a very good thing, being able to replicate the problem. I will try to find out the cause and fix it! (Yes, everything seems to work, but after homing print does not resume. Screen is responsive, SD card is working...

@ejtagle
Copy link
Contributor Author

ejtagle commented May 12, 2018

BTW: I have extra debugging information code that was not posted to this PR. I think there is no interest on that... It just helps debugging this kinds of problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Stepper:acc_step_rate is being changed to always be uint32_t then perhaps this too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for changing everything to uint32_t is that on AVR we emulate a 32bit counter (previously, it was also done by splitting interrupts) and on ARM timers are 32bits at least... ;)

Copy link
Contributor Author

@ejtagle ejtagle May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Stepper.cpp, there are 3 places where that should be changed:
hal_timer_t interval = (HAL_STEPPER_TIMER_RATE / 1000);
hal_timer_t acc_step_rate = ...
hal_timer_t step_rate;
uint32_t Stepper::advance_isr() {
hal_timer_t interval;

@jokeman3000
Copy link

I can try this tomm on the sbase. Linear advance has caused slowdowns for me printing the kfactor code since the refactor occurred.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

@jokeman3000 : There has been a report (#10847 (comment)) on this patch to fix the stuttering. Of course, the more people testing it, the better!! - So we will be sure

@jokeman3000
Copy link

I think I liked it better when we had the thread of a thousand threads. It was easier to keep up :)

Sounds like the issue is fixed but if you want validation I'm happy to test again tomorrow.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

It would be nice, just for confirmation that the fix works 👍

@dot-bob
Copy link
Contributor

dot-bob commented May 26, 2018

I verified the fix works on my AVR 2560. Testing with the same gcode and machine that I was getting stutter before when linear advance is enabled. I verified I get stutter before the fix with linear advance. And I get no stutter with linear advance after applying the fix.

@Sebastianv650
Copy link
Contributor

@ejtagle

Add TCNT1=0; after HAL_timer_set_compare(STEP_TIMER_NUM, ticks); -> no hickups
Will work, but is completely ignoring the time that the stepper ISR took to execute.

Absolutely, it was just meant as a quick & dirty test to see if it's realy an error due to the code trying to set an event "in the past".
You are right, I wasn't getting the true meaning of
HAL_timer_get_count(STEP_TIMER_NUM) + hal_timer_t((HAL_TICKS_PER_US) * 4)
yesterday evening. So it was there, just resulting in a tiny bit too less safety margine.

I read to your patch, nice work! 👍

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

@Sebastianv650 : Let´s hope this fix will completely cure the problem 👍

@thinkyhead
Copy link
Member

The patch is good, and well-documented. I'll merge the pending PRs now and we can get back to being confused about layer shifting.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

@thinkyhead : BTW... This patch could also be fixing layer shifts, so, it should also be tested ;)

@thinkyhead
Copy link
Member

Good point!™

@jokeman3000
Copy link

I think there is still some work to be done here. I do still notice the slow down though it's not as obvious as it was before. This patch definitely helped though!

@leefla
Copy link

leefla commented May 26, 2018

@ejtagle

The first idea i came with was to "synchronize" somehow the extruder steppers with the axis steppers. I don´t know how good or how bad that could be.

That's the exact issue we need to avoid, as I mentioned here: -
#10865

The problem seen already (on all Marlin firmware, just exacerbated by the Prusa Mk3 direct-drive extruder), is that the extruder step is effectively locked in timing with the axis steps, and that results in periodic 10% over-extrusion as it tries to catch-up the extruder, because the step interval of the extruder is not an even multiple of the axis step interval, and thus it steps the extruder at slightly too low a frequency until it's out by the interval of the axis stepper, and then makes one step with a lower interval to catch up.
(e.g.: axis step interval at 0.4ms, extruder step required interval at 4.75ms ... it will do a bunch of steps at 4.8ms, then one at 4.4ms so a sudden momentary extruder increase of 10%). The only way to resolve this is either massively increase the ISR frequency (ideally to the LCM of the step frequency/GCD of interval), but early-out if no steps are required (resource-heavy), or to calculate the desired time of each step for each axis (inc extruder) independently, and then schedule the ISR for the next required step. that way extruder steps would not be bound to axis steps.

@thinkyhead
Copy link
Member

thinkyhead commented May 26, 2018

@leefla — Are you sure it's doing as you describe? The Bresenham algorithm is quite simple, and should result in perfect proportionality.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

@leefla is right @thinkyhead . The bresenham algorithm is just an "error accumulator". Once the error is >= to 1 unit, then it advances the stepper. So, he is right. There is no easy workaround, as we have limited computing resources to be able to manage 4 steppers at the same time with perfect pulse timing. as said in the other thread, just increasing ISR speed at low movement speeds is more than enough to solve the problem

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

Achieving the goal @leefla wants is not complex at all. Maybe 6 lines of code (yes, again ... ;) 👍 )

@leefla
Copy link

leefla commented May 26, 2018

@thinkyhead we ran a logic analyzer on the output, so 100% sure. It's extremely obvious what's going on both from the output, and from the code. The extruder stepper is always the same small interval (code execution duration) past an axis step, and though the amount averages out ok, it's irregular as a result.
Independent ISR intervals would be ideal, but increasing the fixed frequency will reduce the effect.

@ejtagle I'm intrigued whether you mean a little code just to increase the ISR frequency, or a little code to actually have it be dynamic

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

Both are not hard to implement. One of them means to get rid of the bresenham algorithm. The other means to artificially raise the frequency. Both will converge to the same result (an be limited by the lack of processing power). I tend to prefer increasing interrupt speed, as the results are more predictable

@leefla
Copy link

leefla commented May 26, 2018

dynamic ISR interval should actually result in less resources in theory, since you're not actually then going to process any no-step intervals, whereas just increasing the frequency means you'll process no-stop intervals.

e.g.: in my example (based on reality) we'd schedule axis steps at 0.4ms interval (and process the first extruder step in sync with the first axis step most likely) then keep processing axis steps at 0.4ms intervals, then when the next 4.75ms extruder step is due we schedule the ISR to run at 0.35ms later, step the extruder, then check if it's already time to run the axis-step (depending on how long it took us to process the extruder-step), and if not due yet then schedule the next ISR for appr ~0.05ms later at the regular 0.4ms since the last axis step

@ejtagle
Copy link
Contributor Author

ejtagle commented May 26, 2018

It depends. @leefla . Scheduling extruders and steppers at different intervals is a problem because the processor can´t simultaneously attend at the 4.
There is a fixed time (isr execution time) that results in the minimum possible separation between pulses of different steppers. That time seems to be about 8uS on AVR. That causes problems (can be seen as jitter in the pulse timing).
If going "slow", probably the best approach is separate scheduling, if going "fast", probably the best approach is using bresenham and making it 2x... 4x the original rate.
"best approach" => the approach that gives less jitter and error

Bresenham is more "organized", the independant scheduling of events is more "a fight and a race" to get processor time to toggle a pin.

But, probably, the most problematic time is when going fast. Slow is always easy and consumes less resources...

@leefla
Copy link

leefla commented May 26, 2018

understood and agreed.
We need mooore CPU Powah !
I really just want to throw a 'fast' Arm Cortex at the problem
Teensy board plus stepper drivers would be a good thing

@hoffbaked
Copy link

@Etagle @leefla On the extruder, which essentially has a built in low pass filter, how effective would some form of dithering be? It seems like, at the very least, you could make the pattern not as regular and easy to see. But maybe the frequency of the effect is too low.

@hoffbaked
Copy link

There's probably some reason why this is a horrible idea, but is there some amount of injected randomness in the timing of the stepper ISR that would actually help with perceived quality? It seems like this would reduce regular interference patterns.

Logically, this seems like it would be as effective as oversampling, but I'm having a hard time imagining if this would sound quieter, because the frequencies would be slightly spread out, or worse.

@hoffbaked
Copy link

Ah, looking up info on the Bresenham algorithm, I see it doesn't actually accumulate error and relies on constant error per step, so it would be pretty difficult to add any sort jitter to make the timing of the steps more accurate on average.

@ejtagle
Copy link
Contributor Author

ejtagle commented May 30, 2018

@hoffbaked : The bresenham algorithm not only has no accumulated error, but also has the best approximation to the proper interpolation of the axis movements. The only thing that can be improved is that oversampling increases step accuracy.

I have no idea on why there is such a perceptible difference on the extrusion. I assume direct drive with no gear reduction and nearly no demultiplication makes each step feed a considerable amount of filament, so it starts to be critical the exact positioning...

@ejtagle
Copy link
Contributor Author

ejtagle commented May 30, 2018

Yes, it is possible to oversample. I have a patch that implements it, but it is not ready to be merged yet...

@hoffbaked
Copy link

As I thought about it more, I realized that if a single extruder step is visibly noticeable, it might be a resolution problem, more than anything.

@hoffbaked
Copy link

It’s been so many years since I’ve been confronted with obvious aliasing and moiré patterns in graphics and audio software, it’s hard going to a platform that doesn’t have the horsepower to apply any of the normal techniques!

@ejtagle
Copy link
Contributor Author

ejtagle commented May 30, 2018

@hoffbaked We have enough horsepower to apply dithering. The main problem is i do not believe it will fix this issue. Dithering works because the visual system (or the auditive system) is less sensitive to high speed fluctuations.
The main problem here is that the user is able to perfectly "see" each individual "pixel". There is no algorithm that could help with that.

@leefla : There could be a different explanation to the problem: Maybe, being a non geared extruder, and a direct one, requires more torque than available with the stepper motor. So, microsteps accumulate without moving the filament, up to a point where the torque is high enough (in a stepper motor, torque is just proportional to the difference between desired position and actual position of the rotor)
Maybe that is what is causing the pattern!
Friction could be nonlinear: Once it starts moving, it is decreased (for example, due to rotor inertia)

What do you think ? ... Maybe the answer is just increasing current to the Extruder stepper... ;)

@leefla
Copy link

leefla commented May 30, 2018

Just as a follow-up we have some truly great looking prints now from a (very early stages) attempt at getting Klipper running on Prusa Mk3. All of the inconsistent extrusion issues are gone. We have some issues due to not having bed levelling working yet, and still tuning accel/decel, and stepper currents etc (causing some ringing etc), but already they're looking much better.
That's just showing that the increased resolution coming from Klipper (by offloading everything else onto RPi) is making a big difference.
There is definitely noticeable (but not great) improvement from oversampling (while still keeping it below 8KHz) when doing a slow print, since it thus does improve the linearity of the stepper somewhat.
Also changing the extruder drive to be a 3.5:1 Skelestruder instead of 1:1 direct-drive helps also (since any mistimings are less amount of filament) and effectively increases the stepper resolution.

Interesting idea about the extruder torque. In theory we could increase the current to the existing stepper (assuming we can drive enough), and see what difference that makes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.