Planner: Acceleration parity with 1.1.0 and some optimizations #15

thinkyhead · 2016-10-11T08:53:54Z

As I'm working on integrating improvements to the planner Speed and Jerk code in the main Marlin project (See MarlinFirmware/Marlin#4997), I find it useful to cross-pollinate and bring some of the updates over to this branch, which is my reference.

So this PR's main change is: Apply the acceleration technique adopted into Marlin based on Ultimaker's fork… Optimize the maths to remove most division, using multiplication where possible.

// Compute and limit the acceleration rate for the trapezoid generator.  
// block->step_event_count ... event count of the fastest axis
// block->millimeters ... Euclidian length of the XYZ movement or the E length, if no XYZ movement.
float steps_per_mm = block->step_event_count / block->millimeters;
uint32_t accel = block->acceleration_steps_per_s2;
if (block->steps[X_AXIS] == 0 && block->steps[Y_AXIS] == 0 && block->steps[Z_AXIS] == 0) {
  accel = ceil(retract_acceleration * steps_per_mm); // convert to: acceleration steps/sec^2
}
else {
  // Limit acceleration per axis
  //FIXME Vojtech: One shall rather limit a projection of the acceleration vector instead of using the limit.
  #define LIMIT_ACCEL(AXIS) do{ \
    const uint32_t comp = max_acceleration_steps_per_s2[AXIS] * block->step_event_count; \
    if (accel * block->steps[AXIS] > comp) accel = comp / block->steps[AXIS]; \
  }while(0)
  accel = ceil(acceleration * steps_per_mm); // convert to: acceleration steps/sec^2
  LIMIT_ACCEL(X_AXIS);
  LIMIT_ACCEL(Y_AXIS);
  LIMIT_ACCEL(Z_AXIS);
  LIMIT_ACCEL(E_AXIS);
}
block->acceleration_steps_per_s2 = accel;
// Acceleration of the segment, in mm/sec^2
block->acceleration = accel / steps_per_mm;

Additionally:

Make steps_x, y, z, e into an array
axis_steps_per_sqr_second => max_acceleration_steps_per_s2
Add some macros utilized in Marlin 1.1.x
Adjust spacing and code style, esp. in planner.cpp

bubnikv · 2016-10-11T09:25:34Z

Scott,

I appreciate another pair of hands looking into the changes I have made to
the Marlin planner code.

Mainly: Apply the acceleration technique adopted into Marlin based on
Ultimaker's fork… Optimize the maths to remove most division, using
multiplication where possible.

You made a plenty of cosmetic changes to the code, it is quite difficult to
pin point the functional differences. Would you please point me to the
blocks that make the calculation faster? Maybe a git checkin hash to the
ultimaker fork?

Currently we do not see any issues with the speed of our firmware on the
Prusa I3 MK2 printers, so this topic haa a relatively low priority to us,
but it is still certainly something I would like to follow up on.

Thanks, Vojtech

On Tue, Oct 11, 2016 at 10:53 AM, Scott Lahteine [email protected]
wrote:

Mainly: Apply the acceleration technique adopted into Marlin based on
Ultimaker's fork… Optimize the maths to remove most division, using
multiplication where possible.

// Compute and limit the acceleration rate for the trapezoid generator. // block->step_event_count ... event count of the fastest axis// block->millimeters ... Euclidian length of the XYZ movement or the E length, if no XYZ movement.float steps_per_mm = block->step_event_count / block->millimeters;uint32_t accel = block->acceleration_steps_per_s2;if (block->steps[X_AXIS] == 0 && block->steps[Y_AXIS] == 0 && block->steps[Z_AXIS] == 0) {
accel = ceil(retract_acceleration * steps_per_mm); // convert to: acceleration steps/sec^2
}else {
// Limit acceleration per axis
//FIXME Vojtech: One shall rather limit a projection of the acceleration vector instead of using the limit.
#define LIMIT_ACCEL(AXIS) do{
const uint32_t comp = max_acceleration_steps_per_s2[AXIS] * block->step_event_count;
if (accel * block->steps[AXIS] > comp) accel = comp / block->steps[AXIS];
}while(0)
accel = ceil(acceleration * steps_per_mm); // convert to: acceleration steps/sec^2
LIMIT_ACCEL(X_AXIS);
LIMIT_ACCEL(Y_AXIS);
LIMIT_ACCEL(Z_AXIS);
LIMIT_ACCEL(E_AXIS);
}
block->acceleration_steps_per_s2 = accel;// Acceleration of the segment, in mm/sec^2
block->acceleration = accel / steps_per_mm;

Additionally:

Make steps_x, y, z, e into an array

axis_steps_per_sqr_second => max_acceleration_steps_per_s2

Add some macros utilized in Marlin 1.1.x

Adjust spacing and code style, esp. in planner.cpp

You can view, comment on, or merge this pull request online at:

#15
Commit Summary

Optimize planner, utilize updated acceleration limit code

Adjust some spacing & syntax in planner.cpp

File Changes

M Firmware/Marlin.h
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-0 (11)

M Firmware/Marlin_main.cpp
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-1 (2)

M Firmware/planner.cpp
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-2 (696)

M Firmware/planner.h
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-3 (8)

M Firmware/stepper.cpp
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-4 (22)

Patch Links:

https://github.com/prusa3d/Prusa-Firmware/pull/15.patch

https://github.com/prusa3d/Prusa-Firmware/pull/15.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#15, or mute the thread
https://github.com/notifications/unsubscribe-auth/AFj5I-WOYqyPTD6JBwiDt1n1SCGTWPD2ks5qy06jgaJpZM4KTZQw
.

bubnikv · 2016-10-11T09:26:28Z

I appreciate another pair of hands looking into the changes I have made
to the Marlin planner code.

I meant a pair of eyes, naturally :-)

On Tue, Oct 11, 2016 at 11:25 AM, bubnikv . [email protected] wrote:

Scott,

I appreciate another pair of hands looking into the changes I have made to
the Marlin planner code.

Mainly: Apply the acceleration technique adopted into Marlin based on
Ultimaker's fork… Optimize the maths to remove most division, using
multiplication where possible.

You made a plenty of cosmetic changes to the code, it is quite difficult
to pin point the functional differences. Would you please point me to the
blocks that make the calculation faster? Maybe a git checkin hash to the
ultimaker fork?

Currently we do not see any issues with the speed of our firmware on the
Prusa I3 MK2 printers, so this topic haa a relatively low priority to us,
but it is still certainly something I would like to follow up on.

Thanks, Vojtech

On Tue, Oct 11, 2016 at 10:53 AM, Scott Lahteine <[email protected]

wrote:

Mainly: Apply the acceleration technique adopted into Marlin based on
Ultimaker's fork… Optimize the maths to remove most division, using
multiplication where possible.

// Compute and limit the acceleration rate for the trapezoid generator. // block->step_event_count ... event count of the fastest axis// block->millimeters ... Euclidian length of the XYZ movement or the E length, if no XYZ movement.float steps_per_mm = block->step_event_count / block->millimeters;uint32_t accel = block->acceleration_steps_per_s2;if (block->steps[X_AXIS] == 0 && block->steps[Y_AXIS] == 0 && block->steps[Z_AXIS] == 0) {
accel = ceil(retract_acceleration * steps_per_mm); // convert to: acceleration steps/sec^2
}else {
// Limit acceleration per axis
//FIXME Vojtech: One shall rather limit a projection of the acceleration vector instead of using the limit.
#define LIMIT_ACCEL(AXIS) do{
const uint32_t comp = max_acceleration_steps_per_s2[AXIS] * block->step_event_count;
if (accel * block->steps[AXIS] > comp) accel = comp / block->steps[AXIS];
}while(0)
accel = ceil(acceleration * steps_per_mm); // convert to: acceleration steps/sec^2
LIMIT_ACCEL(X_AXIS);
LIMIT_ACCEL(Y_AXIS);
LIMIT_ACCEL(Z_AXIS);
LIMIT_ACCEL(E_AXIS);
}
block->acceleration_steps_per_s2 = accel;// Acceleration of the segment, in mm/sec^2
block->acceleration = accel / steps_per_mm;

Additionally:

Make steps_x, y, z, e into an array

axis_steps_per_sqr_second => max_acceleration_steps_per_s2

Add some macros utilized in Marlin 1.1.x

Adjust spacing and code style, esp. in planner.cpp

You can view, comment on, or merge this pull request online at:

#15
Commit Summary

Optimize planner, utilize updated acceleration limit code

Adjust some spacing & syntax in planner.cpp

File Changes

M Firmware/Marlin.h
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-0 (11)

M Firmware/Marlin_main.cpp
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-1 (2)

M Firmware/planner.cpp
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-2
(696)

M Firmware/planner.h
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-3 (8)

M Firmware/stepper.cpp
https://github.com/prusa3d/Prusa-Firmware/pull/15/files#diff-4 (22)

Patch Links:

https://github.com/prusa3d/Prusa-Firmware/pull/15.patch

https://github.com/prusa3d/Prusa-Firmware/pull/15.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#15, or mute the thread
https://github.com/notifications/unsubscribe-auth/AFj5I-WOYqyPTD6JBwiDt1n1SCGTWPD2ks5qy06jgaJpZM4KTZQw
.

thinkyhead · 2016-10-11T10:26:55Z

it is quite difficult to pin point the functional differences

This link will hide all the whitespace edits so changes are easier to see:
https://github.com/prusa3d/Prusa-Firmware/pull/15/files?w=1

Would you please point me to the blocks that make the calculation faster?

The code block pasted in the original post is one example. That's a case where a long integer division on one side of the comparison was able to change to a long multiplication on the other side of the comparison, giving a minor speedup. (if (a > b / c) => if (a * c > b))

Maybe a git checkin hash to the Ultimaker fork?

Here's the Ultimaker issue in which the change was raised… Ultimaker/Marlin#7

Currently we do not see any issues with the speed of our firmware

I'm sure it performs very well! Cartesian is ideal because it puts the least amount of stress on the CPU, so everything can run more smoothly. I mention in passing that there's an optimization or two, but my point is more to pass on any useful changes or fixes I happen to find while I'm in the process of adapting your innovations to the Marlin 1.1 codebase.

This change in the acceleration code was an interesting fix. Actually, while the main Marlin branch has adopted the adjustment, I'm not sure Ultimaker ever committed the change themselves.

thinkyhead · 2016-10-20T01:26:40Z

I actually ended up reverting part of that patch of acceleration code, as it's too likely to overflow. It's still more likely to overflow now, because it's using not using float. I haven't calculated the range of values where it will overflow, but I think they're well outside normal MK2 parameters.

#define LIMIT_ACCEL(AXIS) do{ \
- const uint32_t comp = max_acceleration_steps_per_s2[AXIS] * block->step_event_count; \
- if (accel * block->steps[AXIS] > comp) accel = comp / block->steps[AXIS]; \
+ if (max_acceleration_steps_per_s2[AXIS] < (accel * block->steps[AXIS]) / block->step_event_count) \
+   accel = (max_acceleration_steps_per_s2[AXIS] * block->step_event_count) / block->steps[AXIS]; \
}while(0)

I like the option of moving towards more "fixed point" maths, although as long as the values are expressed in stepper steps, plain old integer maths will cover most cases. As an intermediate option, we might consider doing some 40-bit integer maths using inline assembler – adding in just enough extra bits to catch those overflows, but requiring neither float conversion nor fixed-point shifting.

bubnikv · 2016-10-20T07:26:31Z

I actually ended up reverting part of that patch of acceleration code, as it's too likely to overflow. It's still more likely to overflow now, because it's using not using float.

The stack usage may be a problem indeed. For example, when the menu system is called from the planner when waiting and then when calling something from the menu, which calls the planner. These kinds of recursions are deadly and I have fixed one long standing Marlin crash due to this stack overflow, when moving axis from the menu.

You may look into my changes in the menu system, where I have reduced the usage of static variables by using unions. Look into ultralcd.cpp, union MenuData.

thinkyhead · 2016-10-21T00:54:51Z

The stack usage may be a problem indeed.

Of course, I refer to the overflow of a 32-bit integer value, not stack overflow…

These kinds of recursions are deadly

Oh yeah. Those were fixed as soon as I spotted them, so the current Marlin code (1.1.0-RC) is much better. (In fact, literally hundreds of bugs have been addressed since Marlin 1.0.x.)

You may look into my changes in the menu system, where I have reduced the usage of static variables by using unions.

I will, thanks! We're definitely trying to move towards more encapsulation. No doubt, ultralcd.cpp and its companions will be made into singleton classes for 1.2.x.

bubnikv · 2016-10-25T18:27:59Z

Maybe we shall look into the current Marlin code base after all :-)

On Fri, Oct 21, 2016 at 2:54 AM, Scott Lahteine [email protected]
wrote:

These kinds of recursions are deadly

Oh yeah. Those were fixed as soon as I spotted them, so the current Marlin
code is much better.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#15 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFj5I79-xrQk94vJBqgkQ8UZQqjmeIzmks5q2A1cgaJpZM4KTZQw
.

bubnikv · 2017-05-31T06:37:53Z

@thinkyhead FYI Pavel has implemented a stack overflow check into our firmware. This is extremely useful if your stack usage is at the limits and / or if you have some unexpected recursion going on eating up the stack. The stack check sets a guard variable at the end of the static variables block and the guard is tested regularly for a change of its value.

thinkyhead · 2017-06-02T22:05:43Z

stack overflow check into our firmware

A good thing to have! A while ago, @Roxy-3D added an M100 command to thoroughly test memory usage, and it has been very useful for new development. We also purged the firmware of all code that might lead to unpredictable recursion and have done lots of cleanup of the LCD code. I also implemented an M43 command as a companion to M42, and that has been extended a lot since by @Bob-the-Kuhn. M43 reports how all pins are assigned, which ones are free, which ones have interrupts, PWM, etc. It also includes general pin monitoring and endstop testing, both very useful for testing new hardware, finding free pins to use for add-ons, or figuring out which pin is connected to the end of a random wire.

Today I started to incorporate your Multiplexer! If you have a look at MarlinFirmware/Marlin#6938 you'll see that it was really easy to incorporate the basic hardware part under the new codebase compared to the old Marlin code. MK2_MULTIPLEXER is implemented as an extension to the SINGLENOZZLE feature, which I added to Marlin (with invaluable help from @MagoKimbra) a few months ago. I only had to add code so that on tool-change it sets the E_MUX pins, and when setting the stepper direction, the even-numbered stepper motors (0, 2) simply turn in reverse. The M600 feature can be configured to handle individual filament changes. However, I do have some more work to do to get M702 finished and patch up the LCD menu.

The next thing I'm doing to the firmware code —this weekend— is to break it up into a proper file hierarchy and putting every G-code handler in its own file. It will look pretty much like this. That will be released as Marlin 1.2.0 as soon as it's ready. Version 1.2.0 will also incorporate a Hardware Abstraction Layer so that Marlin can run on 32-bit boards like the Due and Re-ARM I have sitting on my desk. There have been a lot of requests for that.

bubnikv · 2017-09-21T12:23:51Z

Scott, do you have any idea what the following is for?

/**
 * '#' stops reading from SD to the buffer prematurely, so procedural
 * macro calls are possible. If it occurs, stop_buffering is triggered
 * and the buffer is run dry; this character _can_ occur in serial com
 * due to checksums, however, no checksums are used in SD printing.
 */

Thanks, Vojtech

Linear Advance

thinkyhead · 2018-02-25T15:49:54Z

'#' stops reading from SD to the buffer prematurely

It's for SD card procedures. You can have one SD file start another one, and when it's done, continue where it left off. There were bugs in that code which I fixed earlier this year. Now it works very well!

AnHardt · 2018-02-25T16:22:29Z

Additionally:
Checksums are 0-255, but are not transferred as a byte (since a good while). They are transferred as up to 3 ASCCI digits '0'-'9'. A '#' will not occur in the checksums, but can be a part of a string (M117!, filenames?)

thinkyhead · 2018-02-25T17:21:57Z

Another code comment that I wrote after getting intimate with the issue:

 * M32  - Select file and start SD print: "M32 [S<bytepos>] !/path/file.gco#". (Requires SDSUPPORT)
 *        Use P to run other files as sub-programs: "M32 P !filename#"
 *        The '#' is necessary when calling from within sd files, as it stops buffer prereading

jiri-jirus · 2021-12-17T01:00:31Z

shouldn't this be closed already?

PFW-1312 Define layout of LCD for MMU error screens

3d-gussner · 2023-08-02T09:43:52Z

@thinkyhead I gonna close this PR thanks. MK2 branch isn't maintained anymore.

thinkyhead added 2 commits October 19, 2016 20:18

Optimize planner, utilize updated acceleration limit code

1dbf324

Adjust some spacing & syntax in planner.cpp

dde3944

thinkyhead force-pushed the mk2_planner_parity branch from eab67c0 to dde3944 Compare October 20, 2016 01:18

PavelSindler pushed a commit to PavelSindler/Prusa-Firmware that referenced this pull request Dec 2, 2017

Merge pull request prusa3d#15 from prusa3d/MK3-michal

352a729

Linear Advance

leptun pushed a commit to leptun/Prusa-Firmware that referenced this pull request May 14, 2022

Merge pull request prusa3d#15 from gudnimg/PFW-1312

a4e16cf

PFW-1312 Define layout of LCD for MMU error screens

3d-gussner closed this Aug 2, 2023

thinkyhead deleted the mk2_planner_parity branch August 7, 2023 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Planner: Acceleration parity with 1.1.0 and some optimizations #15

Planner: Acceleration parity with 1.1.0 and some optimizations #15

thinkyhead commented Oct 11, 2016 •

edited

Loading

bubnikv commented Oct 11, 2016

bubnikv commented Oct 11, 2016

thinkyhead commented Oct 11, 2016 •

edited

Loading

thinkyhead commented Oct 20, 2016 •

edited

Loading

bubnikv commented Oct 20, 2016

thinkyhead commented Oct 21, 2016 •

edited

Loading

bubnikv commented Oct 25, 2016

bubnikv commented May 31, 2017

thinkyhead commented Jun 2, 2017

bubnikv commented Sep 21, 2017

thinkyhead commented Feb 25, 2018

AnHardt commented Feb 25, 2018

thinkyhead commented Feb 25, 2018

jiri-jirus commented Dec 17, 2021

3d-gussner commented Aug 2, 2023

Planner: Acceleration parity with 1.1.0 and some optimizations #15

Planner: Acceleration parity with 1.1.0 and some optimizations #15

Conversation

thinkyhead commented Oct 11, 2016 • edited Loading

bubnikv commented Oct 11, 2016

bubnikv commented Oct 11, 2016

thinkyhead commented Oct 11, 2016 • edited Loading

thinkyhead commented Oct 20, 2016 • edited Loading

bubnikv commented Oct 20, 2016

thinkyhead commented Oct 21, 2016 • edited Loading

bubnikv commented Oct 25, 2016

bubnikv commented May 31, 2017

thinkyhead commented Jun 2, 2017

bubnikv commented Sep 21, 2017

thinkyhead commented Feb 25, 2018

AnHardt commented Feb 25, 2018

thinkyhead commented Feb 25, 2018

jiri-jirus commented Dec 17, 2021

3d-gussner commented Aug 2, 2023

thinkyhead commented Oct 11, 2016 •

edited

Loading

thinkyhead commented Oct 11, 2016 •

edited

Loading

thinkyhead commented Oct 20, 2016 •

edited

Loading

thinkyhead commented Oct 21, 2016 •

edited

Loading