[2.0.x] Apply feedrate to nozzle movement for kinematic machines #8778

tcm0116 · 2017-12-14T06:28:39Z

Currently, the actual feedrate achieved by a kinematic machine is not the desired feedrate provided in G0/G1 commands. This is due to the way the Planner class computes the distance of the movement for kinematic machines.

The distance of a movement for a kinematic machine is currently calculated in Planner::_buffer_steps as the square root of the sum of the squares of the distance moved by each axis. For cartesian machines, this makes sense. However, for kinematic machines, the distance moved by each axis is only indirectly related to the actual distance moved by the nozzle. Because of this, the distance computed is not valid for kinematic machines, and because the distance is used to compute the length of time of the movement and subsequently the required velocities of the axes, the final feedrate of the nozzle ends up being not what was desired.

This PR aims to resolve this issue by passing the cartesian length of a movement to Planner::_buffer_steps in order to allow for proper computation of the necessary velocities for the axes. This is achievable on kinematic machines because the movements are already segmented, and the length of each segment is easily computed. This ends up replacing three multiplications and a square root call with a single multiplication.

My original intent with this PR was to limit the maximum size of a segmented movement for a kinematic machine. This is necessary because prepare_kinematic_move_to does not consider any maximum feedrates when computing the number of segments for a given feedrate. For example, running G1 X100 Y100 F99999999 will result on only one segment being generated because the large feedrate is not limited by anything.

I still haven't solved this problem because the maximum feedrates defined in the Planner are for the individual axes and not the overall feedrate of the nozzle. One idea I have to solve this issue is to add a DEFAULT_MAX_XY_FEEDRATE which would apply to the cartesian movement of the nozzle, in a similar manner to DEFAULT_ACCELERATION, and would essentially replace the current PLANNER_XY_FEEDRATE() macro.

In order to alleviate any confusion, the documentation for DEFAULT_MAX_FEEDRATE should be updated to state that the values apply to the individual steppers (i.e. ABCE) and not the cartesian movement of the nozzle. For kinematic machines, I would also like to update the labels in the Control/Motion LCD menus to use ABC instead of XYZ, since "Vmax X" really means "the maximum velocity of the A stepper motor" on a kinematic machine, and "Vmax A" is more correct.

Please let me know if you have any comments or suggestions as I don't want to go too far down this path to only find out that there's a fundamental issue with this approach.

thinkyhead · 2017-12-16T00:31:13Z

I've been down this rabbit-hole, including adding the mm length as another parameter to buffer_line. The only value you really need to know is the move time. The length is fine, but it's one step removed from the value the planner ultimately needs to derive.

Consider the issue for SCARA also. The feedrate must be set dynamically to convert mm/s into degrees/s for two rotary joints. A script from Evezor uses the square root of the sum of the squares of the angular changes. I've also tried using the larger of the two angles as the basis for feedrate. Results are currently inconclusive as to which is better.

In the case of a Delta, we see no problems with the current feedrate being based on the tower carriages. But you may find with slower moves that this gives inconsistent lines that speed up and slow down as they progress. Or, this problem may be obviated in Delta. We know it's an issue for SCARA, and especially troublesome when doing laser engraving.

tcm0116 · 2017-12-16T01:18:25Z

The commit on this PR seems to work well for my Delta machine. Considering that move time = distance / feed rate, the planner can easily compute the move time (as it currently does) given the additional distance parameter that is passed to buffer_line.

I don't see why this change wouldn't work for SCARA since the rate of angular change is a function of move time, which is computed using the provided distance.

This change also works with the segmented leveled moves option that was just added since the distance is known.

thinkyhead · 2017-12-21T01:20:25Z

I don't see why this change wouldn't work for SCARA since the rate of angular change is a function of move time, which is computed using the provided distance.

As it happens I added SCARA_FEEDRATE_SCALING to do just that, and was just working on a patch extending it to G2/G3 as well — now at #8851.

thinkyhead · 2017-12-21T01:27:33Z

Marlin/src/module/planner.cpp

@@ -714,11 +714,11 @@ void Planner::calculate_volumetric_multipliers() {
 *  fr_mm_s     - (target) speed of the move
 *  extruder    - target extruder
 */
-void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const uint8_t extruder) {
+void Planner::_buffer_steps(const int32_t (&target)[ABCE], float fr_mm_s, const uint8_t extruder, const float millimeters /*= 0.0*/) {


Using const float &millimeters (const reference) makes argument passing quicker — only 2 bytes instead of 4.

filippor · 2018-01-10T23:23:06Z

I'm testing this on a delta printer and seems to work

tcm0116 · 2018-01-25T04:17:42Z

For kinematic machines, I would also like to update the labels in the Control/Motion LCD menus to use ABC instead of XYZ, since "Vmax X" really means "the maximum velocity of the A stepper motor" on a kinematic machine, and "Vmax A" is more correct.

I just updated this PR to change the axis names in the LCD to ABC instead of XYZ for kinematic machines.

I've noticed that the benefit of this change are very apparent on my delta machine when printing circles. With the current method, it's pretty obvious that the nozzle is changing speed as it goes around the circle. With this change in place, the nozzle appears to maintain a more constant velocity.

thinkyhead · 2018-01-26T20:06:32Z

To make merging 1.1.x fixes with 2.0.x easier let's hold this until 1.1.9 is released and all new development is closed on the 1.1.x branch. This can be merged shortly after that release as it will only be included in 2.0.x going forward.

tcm0116 · 2018-01-26T20:14:55Z

Sounds reasonable.

For machines, what would you think about combining the velocity, acceleration, and jerk settings for all three axes? I can't think of a reason why someone would want different settings for the three towers, and it's a little obnoxious having to adjust all three in the LCD.

Sebastianv650 · 2018-01-31T07:12:10Z

Can we really wait fixing this? Basically this means delta support is broken in all marlin versions. Except we think it's fine to fill speed and acceleration values with "random" data. .

Sebastianv650 · 2018-02-01T16:03:16Z

As we have now got a consensus about the bug in Marlin today and which speed etc is related to what, would you create a PR for 1.1 bugfix also? I think we should get this sorted out as fast as possible as this is a long standing bug since DELTAs were introduced to Marlin.

I think with the knowledge available now, we can convince @thinkyhead to merge them if necessary.

tcm0116 · 2018-02-03T00:51:29Z

would you create a PR for 1.1 bugfix also?

Done - #9448

thinkyhead · 2018-02-04T04:50:50Z

I'm eminently convince-able. Let's get delta unbroken.

thinkyhead · 2018-02-04T04:56:48Z

Marlin/src/module/planner.h

@@ -376,7 +376,7 @@ class Planner {

      FORCE_INLINE static void unskew(float &cx, float &cy, const float &cz) {
        if (WITHIN(cx, X_MIN_POS, X_MAX_POS) && WITHIN(cy, Y_MIN_POS, Y_MAX_POS)) {
-          const float sx = cx + cy * xy_skew_factor + cz * xz_skew_factor,
+          const float sx = cx + cy * xy_skew_factor + cz * (xz_skew_factor - (xy_skew_factor * yz_skew_factor)),


Nope! See #8623 (comment)

This is so weird. I didn't make this change in either PR. Something must have happened with the auto merge to somehow regress the change.

thinkyhead · 2018-02-04T06:25:56Z

Updated the translations, reversed the incorrect skew_factor change, and changed NEAR_ZERO(millimeters) to just !millimeters in _buffer_steps.

This way of e_accel calculation is the most precise one. Finaly this is now also usame on kinematic printers (DELTA, SCARA) but ONLY with PR MarlinFirmware#8778! Without that, Marlins acceleration and speed values are some fantasy numbers.

thinkyhead · 2018-02-07T22:15:01Z

@tcm0116 @Sebastianv650 Error: The last segment in segmented moves is not known until after all other segments are done. And usually it will be very small. So the last segment should not be passing the same length as the rest.

Solution: Do an additional SQRT(sq+sq+sq) to get the last segment length.

tcm0116 · 2018-02-07T22:27:39Z

The length of each segment is computed as:

cartesian_segment_mm = cartesian_mm * inv_segments

Since it's a function of the total cartesian length of the movement, once all of the segments have been buffered, the sum of the lengths of each segment will be equal to the total length of the original movement. The last segment will likely be smaller than the rest due to rounding errors, but the total length provided to the planner across all of the segments will be correct.

And usually it will be very small

Do you have data to back that up? I would think the last segment will be about the same length as the other segments, just not exactly the same.

thinkyhead · 2018-02-07T22:44:58Z

The last segment is not just another segment of the same length. It also acts as a correction to the imprecision of previous segments, which are merely adding a pre-calculated float on each pass.

tcm0116 · 2018-02-07T22:50:02Z

The last segment is not just another segment of the same length. It also acts as a correction to the imprecision of previous segments, which are merely adding a pre-calculated float on each pass.

Hence, I stated:

I would think the last segment will be about the same length as the other segments, just not exactly the same.

Is the length of the last segment so much different that it warrants another call to sqrt? I have no problem doing that, but I'm not really sure it's entirely necessary since the sum of the lengths of the segments passed to the planner is equal to the length of the original movement. If you recompute the length of the last segment, then the total length passed for all of the segments will not equal the length of the original movement.

thinkyhead · 2018-02-07T22:51:28Z

The last segment is a direct move to the originally-given destination. Again, as a corrective to the approximation of the segment loop.

thinkyhead · 2018-02-07T22:52:44Z

If the number of segments comes out to 1, it might be good to just do a direct move and exit. Skip all the other logic.

thinkyhead · 2018-02-07T22:55:02Z

    // Since segment_distance is only approximate,
    // the final move must be to the exact destination.
    const float last_segment_mm2 = (
        sq(destination[X_AXIS] - raw[X_AXIS])
      + sq(destination[Y_AXIS] - raw[Y_AXIS])
      + sq(destination[Z_AXIS] - raw[Z_AXIS])
    );
    if (!UNEAR_ZERO(last_segment_mm2))
      planner.buffer_line_kinematic(destination, fr_mm_s, active_extruder, SQRT(last_segment_mm2));

tcm0116 · 2018-02-07T22:57:41Z

Why is absolute precision required for the length of the last segment and not the others? That length value is only used for computing the time that it should take to execute the segment at the desired feedrate. It does not affect the final position of the nozzle.

thinkyhead · 2018-02-07T22:58:27Z

Float rounding leads to accumulation of error in this loop:

    while (--segments) {
      . . .
      LOOP_XYZE(i) raw[i] += segment_distance[i];
      . . .
    }

The old code used to loop all segments. I did an optimization to have it do the segment loop for one less than the total number, so yes (ideally) the last segment should be close to the calculated or configured segment length.

thinkyhead · 2018-02-07T23:01:22Z

^ plan_arc also does one less segment, so…

Hmm, it's possible the figured cartesian length would be close enough in most cases. If it doesn't matter that it's exact, then perhaps it's ok as it is.

thinkyhead · 2018-02-07T23:03:37Z

One optimization that can be made: In segmented_line_to_destination the passed segment length always applies (unless the whole move is shorter than a segment), so… ~~no need to calculate a new cartesian_segment_mm there~~.

Also, this PR leaves out plan_arc.

tcm0116 · 2018-02-07T23:05:28Z

With your approach, the raw array will end up having error because the sum of all of the segment_lengths won't equal the total length of the original movement. Each segment_cartesian_mm is an approximation that sum up to the total length of the original movement. If you re-measure the length of the last segment, then whatever the delta is between that value and segment_cartesian_mm will become error in raw.

thinkyhead · 2018-02-07T23:06:35Z

Exactly.

With ~~your~~ the existing approach

tcm0116 · 2018-02-07T23:07:32Z

It's significantly better than it was when the RSS was taken of the distance moved by the towers and not the nozzle.

thinkyhead · 2018-02-07T23:09:12Z

It's significantly better

I'm not surprised. See also what's required to work out SCARA angular feedrate.

tcm0116 · 2018-02-07T23:10:49Z

I'd say to either compute the length of each segment or none of them, but not a combination.

thinkyhead · 2018-02-07T23:12:28Z

Not a combination of "each" and "none"?

tcm0116 · 2018-02-07T23:12:29Z

I think SCARA already handles the angular feedrate. It just needs to know how long it should take to perform the movement, which this PR should provide.

thinkyhead · 2018-02-07T23:13:39Z

Yes, that's the idea. So do you think this PR supplants the current…

#if ENABLED(SCARA_FEEDRATE_SCALING)
  // For SCARA scale the feed rate from mm/s to degrees/s
  // i.e., Complete the angular vector in the given time.
  inverse_kinematics(raw);
  ADJUST_DELTA(raw);
  planner.buffer_segment(delta[A_AXIS], delta[B_AXIS], raw[Z_AXIS], raw[E_AXIS], HYPOT(delta[A_AXIS] - oldA, delta[B_AXIS] - oldB) * inverse_secs, active_extruder, linear_per_segment);
  oldA = delta[A_AXIS]; oldB = delta[B_AXIS];
#else
  planner.buffer_line_kinematic(raw, fr_mm_s, active_extruder, linear_per_segment);
#endif

tcm0116 · 2018-02-07T23:14:15Z

In other words, don't just compute the length of the last segment, but I think the current approach is sufficient.

thinkyhead · 2018-02-07T23:14:55Z

Yes, after a reëxamination of the code it looks like the current approach should be fine.

thinkyhead · 2018-02-07T23:16:10Z

So, is the effect of this to always make the passed fr_mm_s correctly apply to mm in all cases?

thinkyhead · 2018-02-07T23:16:36Z

Sounds like two things may come from this:

No need to do SCARA scaling of the passed feedrate.
This can be added to plan_arc too.

This way of e_accel calculation is the most precise one. Finaly this is now also usame on kinematic printers (DELTA, SCARA) but ONLY with PR MarlinFirmware#8778! Without that, Marlins acceleration and speed values are some fantasy numbers.

thinkyhead reviewed Dec 21, 2017

View reviewed changes

thinkyhead added T: Design Concept Technical ideas about ways and methods. T: Development Makefiles, PlatformIO, Python scripts, etc. labels Dec 25, 2017

filippor mentioned this pull request Jan 10, 2018

LIN_ADVANCE does not work with DELTA #7521

Closed

thinkyhead force-pushed the bugfix-2.0.x branch from 5f5ed36 to db031d5 Compare January 22, 2018 16:43

tcm0116 force-pushed the 2.0.x_kinematic branch from cac9cfb to ed580b7 Compare January 25, 2018 04:14

tcm0116 mentioned this pull request Jan 31, 2018

Developers: Help on acceleration meaning in kinematic machines #9403

Closed

Sebastianv650 mentioned this pull request Jan 31, 2018

[Don't merge!] LIN_ADVANCE v1.5 for testers #9379

Closed

7 tasks

tcm0116 mentioned this pull request Feb 3, 2018

[1.1.x] Apply feedrate to nozzle movement for kinematic machines #9448

Merged

tcm0116 and others added 2 commits February 3, 2018 22:54

Apply feedrate to nozzle movement for kinematic machines

de4ddd6

Language updates for parity

7f57cd3

thinkyhead force-pushed the 2.0.x_kinematic branch from 1d90422 to 7f57cd3 Compare February 4, 2018 04:55

thinkyhead reviewed Feb 4, 2018

View reviewed changes

Don't regress me, bro!

224e0a9

thinkyhead changed the title ~~[2.0.x][Concept] Apply feedrate to nozzle movement for kinematic machines~~ [2.0.x] Apply feedrate to nozzle movement for kinematic machines Feb 4, 2018

thinkyhead merged commit 7867464 into MarlinFirmware:bugfix-2.0.x Feb 4, 2018

vilsed mentioned this pull request May 2, 2018

[bugfix-2.0.x] Amplified feedrates on delta kinematics #10579

Closed

[2.0.x] Apply feedrate to nozzle movement for kinematic machines #8778

[2.0.x] Apply feedrate to nozzle movement for kinematic machines #8778

Conversation

tcm0116 commented Dec 14, 2017 • edited Loading

thinkyhead commented Dec 16, 2017 • edited Loading

tcm0116 commented Dec 16, 2017 • edited Loading

thinkyhead commented Dec 21, 2017

thinkyhead Dec 21, 2017

Choose a reason for hiding this comment

filippor commented Jan 10, 2018

tcm0116 commented Jan 25, 2018

thinkyhead commented Jan 26, 2018 • edited Loading

tcm0116 commented Jan 26, 2018

Sebastianv650 commented Jan 31, 2018 • edited Loading

Sebastianv650 commented Feb 1, 2018

tcm0116 commented Feb 3, 2018

thinkyhead commented Feb 4, 2018

thinkyhead Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

tcm0116 Feb 4, 2018

Choose a reason for hiding this comment

thinkyhead commented Feb 4, 2018

thinkyhead commented Feb 7, 2018

tcm0116 commented Feb 7, 2018

thinkyhead commented Feb 7, 2018

tcm0116 commented Feb 7, 2018

thinkyhead commented Feb 7, 2018

thinkyhead commented Feb 7, 2018

thinkyhead commented Feb 7, 2018

tcm0116 commented Feb 7, 2018 via email

thinkyhead commented Feb 7, 2018 • edited Loading

thinkyhead commented Feb 7, 2018 • edited Loading

thinkyhead commented Feb 7, 2018 • edited Loading

tcm0116 commented Feb 7, 2018 via email

thinkyhead commented Feb 7, 2018 • edited Loading

tcm0116 commented Feb 7, 2018 via email

thinkyhead commented Feb 7, 2018

tcm0116 commented Feb 7, 2018 via email

thinkyhead commented Feb 7, 2018

tcm0116 commented Feb 7, 2018 via email

thinkyhead commented Feb 7, 2018

tcm0116 commented Feb 7, 2018 via email

thinkyhead commented Feb 7, 2018

thinkyhead commented Feb 7, 2018

thinkyhead commented Feb 7, 2018

tcm0116 commented Dec 14, 2017 •

edited

Loading

thinkyhead commented Dec 16, 2017 •

edited

Loading

tcm0116 commented Dec 16, 2017 •

edited

Loading

thinkyhead commented Jan 26, 2018 •

edited

Loading

Sebastianv650 commented Jan 31, 2018 •

edited

Loading

thinkyhead Feb 4, 2018 •

edited

Loading

thinkyhead commented Feb 7, 2018 •

edited

Loading

thinkyhead commented Feb 7, 2018 •

edited

Loading

thinkyhead commented Feb 7, 2018 •

edited

Loading

thinkyhead commented Feb 7, 2018 •

edited

Loading