MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.24k stars 19.22k forks source link

G26 Validation Tool #6609

Closed Tannoo closed 6 years ago

Tannoo commented 7 years ago

Can this be optimized by utilizing the arc movements instead of the current method?

Roxy-3D commented 7 years ago

@oldmcg was looking at that. He is making UBL work with Delta's and that is one of the ideas he is exploring. I think the answer is "Yes!" But I have not had time to look at this yet.

If you have time to look at it.... PLEASE DO!

Tannoo commented 7 years ago

I was going to try.... but you know that I suck as a programmer. lol

oldmcg commented 7 years ago

@Tannoo, do you mean using G2/G3 arc to draw the circles of the mesh?

Turns out for deltas and cartesians the underlying G2/G3 code is really no different than using multiple polygon line segments to draw circles. G2/G3 just breaks down arc into many straight cartesian segments.

Tannoo commented 7 years ago

True, but the circles don't print smoothly, is there something else in play causing it to struggle?

oldmcg commented 7 years ago

Have you tried slowing down the feedrate for the G26 print? I could also see belt tension or linear slop being a factor in drawing small circles too since x/y motors change direction. Sorry, that's the limit of my noob understanding of printer mechanics.

Roxy-3D commented 7 years ago

True, but the circles don't print smoothly, is there something else in play causing it struggle?

It's not struggling... It is pausing to let the stepper motors synchronize. This is the tail end of the routine in G26 that does the line drawing...

    ubl_line_to_destination(feed_value, 0);
    stepper.synchronize();
    set_destination_to_current();

I believe if we take out the stepper.synchronize() the next move will already be in the queue and you won't see the stuttering. And I'm not positive... But it is possible that set_destination_to_current() would need to be pulled out too.

the underlying G2/G3 code is really no different than using multiple polygon line segments to draw circles. G2/G3 just breaks down arc into many straight cartesian segments.

There is a fair amount of logic to draw the some what circle shapes in G26. If we could have real circles and cut that code out of G26, that would be a good thing.

oldmcg commented 7 years ago

@Roxy-3D, G26 could use G2/G3 to draw the circles in single call, but G2/G3 call buffer_line for segments, not ubl_line_to. The code I sent you with changes to apply_leveling could be enabled for cartesian as well, but ubl_line_to would have to call _buffer_line directly to avoid double-leveling (that's what I do in ubl delta line), but then things like G2/G3 which call outer buffer_line could apply ubl leveling per segment, even for cartesian.

Roxy-3D commented 7 years ago

Wow!!! I'm going to have to re-read that email a few times. But one thing that is easy to absorb is that the floating point add is just as expensive as the floating point multiply. I would of guessed it was way cheaper.

And the other important fact is the sqrt() is just about the same as a floating point divide. I don't see how that can be the case. Did the compiler optimize some stuff in your code because constants were used? Even the multiply and divide are done in software so for sure the sqrt() is done in software. Any sqrt() algorithm I know of has to do multiple divides and compares, etc... So... I'm kind of suggesting we double check the sqrt() timing numbers. Those don't pass the 'smell test'.

Tannoo commented 7 years ago

I believe if we take out the stepper.synchronize() the next move will already be in the queue and you won't see the stuttering. And I'm not positive... But it is possible that set_destination_to_current() would need to be pulled out too.

I commented out:

    ubl_line_to_destination(feed_value, 0);
    //stepper.synchronize();
    //set_destination_to_current();

I didn't notice any change in any behavior.

Roxy-3D commented 7 years ago

Actually... There is a second stepper.synchronize() just below

      feed_value = planner.max_feedrate_mm_s[Z_AXIS]/(3.0);  // Base the feed rate off of the configured Z_AXIS feed rate
      destination[X_AXIS] = current_position[X_AXIS];
      destination[Y_AXIS] = current_position[Y_AXIS];
      destination[Z_AXIS] = z;                          // We know the last_z==z or we wouldn't be in this block of code.
      destination[E_AXIS] = current_position[E_AXIS];
      ubl_line_to_destination(feed_value, 0);
      stepper.synchronize();    // **<----<<<<      here is one of them....**
      set_destination_to_current();
      //if (ubl.g26_debug_flag) debug_current_and_destination(PSTR(" in move_to() done with Z move"));
    }
    // Check if X or Y is involved in the movement.
    // Yes: a 'normal' movement. No: a retract() or un_retract()
    feed_value = has_xy_component ? PLANNER_XY_FEEDRATE() / 10.0 : planner.max_feedrate_mm_s[E_AXIS] / 1.5;
    if (ubl.g26_debug_flag) SERIAL_ECHOLNPAIR("in move_to() feed_value for XY:", feed_value);
    destination[X_AXIS] = x;
    destination[Y_AXIS] = y;
    destination[E_AXIS] += e_delta;
    //if (ubl.g26_debug_flag) debug_current_and_destination(PSTR(" in move_to() doing last move"));
    ubl_line_to_destination(feed_value, 0);
    //if (ubl.g26_debug_flag) debug_current_and_destination(PSTR(" in move_to() after last move"));
    stepper.synchronize();   //  **<----<<<  Here is another one.......**
    set_destination_to_current();
  }

If you get rid of both of them... I think you will see a difference. But I'm not sure if the set_destination_to_current() needs to stay or not. Probably need to try it both ways.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.