[BUG] Arc (G2/G3) segments longer than defined in some cases

[FormerLurker]

Did you test the latest bugfix-2.0.x code?

Yes, and the problem still exists.

Bug Description

I believe Marlin can produce arcs with segment lengths nearly two times greater than the defined arc length (MM_PER_ARC_SEGMENT). The issue is caused by this line of code:

Marlin/Marlin/src/gcode/motion/G2_G3.cpp

Line 161 in 418743c

uint16_t segments = FLOOR(mm_of_travel / seg_length);

Imagine the mm_of_travel = 1.999 here, and that seg_length is 1.0mm. In this case, the number of segments returned from the line above would be 1 from FLOOR(1.999/1).

In this example, when entering the interpolation loop here:

Marlin/Marlin/src/gcode/motion/G2_G3.cpp

Line 221 in 418743c

for (uint16_t i = 1; i < segments; i++) { // Iterate (segments-1) times

The entire loop would be skipped (segments = 1), and the next item inserted into the buffer would have a length of 1.999mm.

I propose replacing the original line above (161) with the following code:

// Divide total travel by nominal segment length
  uint16_t segments = CEIL(mm_of_travel / seg_length);

That code, when taking the previous example, would set segments = 2. The interpolation loop would run once, producing a segment approximately 1mm in length. The remainder of the segment would be added my moving to the endpoint of the arc. This way, there would be one 1mm segment, and one 0.999mm segment, matching the MM_PER_ARC_SEGMENT define.

Here is a visualization in Simplify3D I made of the resulting toolpaths generated by my ArcStraightener app using the Marlin 2.0.9.1 interpolation algorithm:

Here are the toolpaths after applying the update I am proposing:

The number of extra segments is smaller than one would expect, since this only affects the last segment in any given arc, preventing it from being longer than the mm_per_arc_segment setting.

Total arcs before code change in this file: 185450
Total arcs after code change in this file : 203314

That's less than a 10% increase, though this obviously depends on the GCode being used.

Also, I noticed this issue while adding arc interpolation routines (as close as I could make them in a reasonable amount of time anyway) from Marlin V1.1.9.1 and Marlin V2.0.9.1 to my ArcStraightener project (used for debugging ArcWelder), and it is visible in 1.1.9.1 as well. I will be pushing this code to the FormerLurker/ArcWelderLib repository as soon as I've finished testing, so perhaps that will be useful.

Bug Timeline

It appears to be quite old, existing at least since V1.1.9.1, and very likely before that.

Expected behavior

The MM_PER_ARC_SEGMENT define should be respected. No arc segments should be longer than this value.

Actual behavior

Arcs produced are longer than the MM_PER_ARC_SEGMENT setting, sometimes nearly 2x as long.

Steps to Reproduce

Reproduction is a bit tricky. I will provide the gcode files used for the images below.

Please let me know if you need any other information. I'm willing to submit a PR, if that would be helpful, but I have no good way to test the results (remedying that soon too).

Version of Marlin Firmware

NA - Issue found from source code

Printer model

NA - Issue found from source code

Electronics

NA - Issue found from source code

Add-ons

No response

Bed Leveling

No response

Your Slicer

Cura

Host Software

No response

Additional information & file uploads

Here is the source gcode, and the approximate toolpaths generated from ArcStraightener.exe (default arc interpolation parameters: mm_per_arc_segment=1 and min_arc_segments=24, n_arc_correction=25) using floor and ceil:

gcode_examples.zip

Please let me know if I can provide anything else, and thank you for reading this!

[thinkyhead]

Makes sense! That should not have a negative effect on segments under 1mm (for the example), since those will be handled as a single linear move. The largest segment size would always be < 1mm, and the smallest would be ~0.5mm. Another possibility would be to treat a single segment longer than seg_length as a special case by doing one move of seg_length to that proportion of the arc, and then the final move to the end of the arc, as usual.

[FormerLurker]

Another possibility would be to treat a single segment longer than seg_length as a special case by doing one move of seg_length to that proportion of the arc, and then the final move to the end of the arc, as usual.

Seems like a CEIL would be simpler, and would result in the same path if I'm understanding you correctly. I'm not sure what the performance difference is between CEIL and FLOOR though compared to this suggestion.

Thanks for commenting!!

[thinkyhead]

To wit…

  const float proportion = (segments == 1 && mm_of_travel > seg_length)
                           ? 1.0f - (mm_of_travel - seg_length) / mm_of_travel
                           : 1.0f;

  const float theta_per_segment = proportion * angular_travel / segments,
              sq_theta_per_segment = sq(theta_per_segment),
              sin_T = theta_per_segment - sq_theta_per_segment * theta_per_segment / 6,
              cos_T = 1 - 0.5f * sq_theta_per_segment; // Small angle approximation

  #if HAS_Z_AXIS && DISABLED(AUTO_BED_LEVELING_UBL)
    const float linear_per_segment = proportion * linear_travel / segments;
  #endif
  #if HAS_EXTRUDERS
    const float extruder_per_segment = proportion * extruder_travel / segments;
  #endif

  . . .

  if (proportion != 1.0f) segments++;
  for (uint16_t i = 1; i < segments; i++) { // Iterate (segments-1) times
    . . .
  }

[FormerLurker]

If you give me a bit, I'll update my code with your suggestion and post the resulting paths for that file. Sounds fun!

[thinkyhead]

Slightly modified to increase segments at the right point.

[FormerLurker]

Actually,

  float proportion = 1.0f;
  if (segments == 1 && mm_of_travel > seg_length) {
    proportion -= (mm_of_travel - seg_length) / mm_of_travel;
    segments = 2;
  }

Imagine mm_of_travel here is 2.999. Segments would = 2 in this case (using floor), the interpolation loop would execute 1 time, and the final segment will still be 1.999 mm, no?

[thinkyhead]

The code above was already corrected.

[FormerLurker]

Ok, but there is a check for segments ==1 here:

const float proportion = (segments == 1 && mm_of_travel > seg_length)
                           ? 1.0f - (mm_of_travel - seg_length) / mm_of_travel
                           : 1.0f;

This would only fix the issue if 1 < mm_of_travel < 2, right? It's the remainder of mm_of_travel - floor(mm_of_travel) that is causing the issue, I believe.

Sorry if I'm not getting it. I'll just plug in the code and see what it does, been working a long time already :)

[thinkyhead]

The change as currently existing above is a "late hack" that simply reduces the amount of rotation applied within the single loop to an angle corresponding to seg_length. The second and final segment then goes to the destination, whatever it may be.

It should only be needed for the case where there is a single segment, since two or more segments will divide the total travel distance more finely and the overage will get smaller the more segments you have. But you could alter it to ignore the number of segments and check whether mm_of_travel / segments > seg_length, then calculate the proportion using a slightly different formula.

[FormerLurker]

So, I tried out your changes above, and the resulting paths look similar those generated using the current release:

It should only be needed for the case where there is a single segment

I don't think this is true. I believe this pertains to the last segment of any arc due to the combination of the use of floor(segments) and the fact that the interpolation loop starts with i = 1. Let's say mm_travel = 50.99, and seg_length = 1. This line:

 uint16_t segments = floor(mm_of_travel / seg_length);

sets segments = 50. The interpolation loop will run 49 times: i = 1; i < segments; i++

In this case we've moved 49mm, and final segment (outside the loop) will have to move 1.99mm.

I've also thought about leaving the floor calc in there and just setting i=0 in the for loop. If we allow 'segments' to = 0, that should work. if so, it would require some changes to the min_segments calculations, but would make it so we could skip up to 2 NOLESS calls. I will look into that.

[thinkyhead]

Note that the actual segment size used in the move ends up being the total length divided by the number of segments, and not fixed to exactly seg_length. So the proportional error from 49 versus 50 will be much smaller than the proportional error of 1 versus 2, or of 9 versus 10, etc….

[FormerLurker]

I will go back to the drawing board, taking your notes into account, and will see if I can generate a simple example with a single arc command. I think I know what you are saying, but I'm sure I will better understand you after that.

Thanks again!

[thinkyhead]

Mulling it over, maybe this would be more universal…

  // Divide total travel by segment length
  float segments = FLOOR(mm_of_travel / seg_length);
  NOLESS(segments, min_segments);         // At least some segments

  // Are the segments now too few to reach the destination?
  const float segmented_length = seg_length * segments;
  const bool tooshort = segmented_length < mm_of_travel;
  const float proportion = tooshort ? segmented_length / mm_of_travel : 1.0f;

  . . .

  if (tooshort) segments++; // Run all segments except remainder
  for (uint16_t i = 1; i < segments; i++) { // Iterate (segments-1) times

[thinkyhead]

I've posted #22599 as one potential approach based on this discussion.

[FormerLurker]

Well, I really shouldn't do this kind of stuff after a long day of programming obviously! I had some configuration settings wrong, and now finally understand your point about the distances being spread across the arc segments. As you suspected, the issue IS ONLY happening when segments = 1 and the seg_length < mm_of_travel. Here is a pretty simple code change that leaves floor in place, but solves the issue when segments == 1 (looks familiar, because it's super close to what you had):

Change this

Marlin/Marlin/src/gcode/motion/G2_G3.cpp

Lines 161 to 163 in 418743c

	uint16_t segments = FLOOR(mm_of_travel / seg_length);
	NOLESS(segments, min_segments); // At least some segments
	seg_length = mm_of_travel / segments;

To this:

  uint16_t segments = floor(mm_of_travel / seg_length);
  segments = NOLESS(segments, min_segments);         // At least some segments
  if (segments == 1 && seg_length < mm_of_travel)
    segments++;
  seg_length = mm_of_travel / segments;

The results look almost identical to the CEIL solution, but far fewer extra arc segments generated:

I am looking at the other thread next, I just saw it come it, thanks!

[FormerLurker]

Ok, I finally think I get what you were trying to achieve! I'll update my code and will post the output, and we'll see what the paths look like!

Also, that was one rapid fire fix. You are a machine!

[thinkyhead]

Hopefully that PR makes some sense, as it aims to keep the segment length at the configured size in all cases, whereas the old code would allow the segment length to be more flexible. If it turns out to only be an issue when you have a single segment, that would allow for some optimization, but it seemed to me that the configured segment length should take precedence.

Time to refuel this machine with some caffeine!

[CRCinAU]

Man, I wish I understood half of what you guys were talking about..... My knowledge of that low level is really crappy.

However, I'm a massive fan of @FormerLurker and the work on ArcWelder and recommend it everywhere - and obviously I made extensive use of it myself.

Any improvements to this code to make it better is fantastic news as far as I'm concerned.

Thanks to both of you for digging into the stuff that really does take considerable knowledge to understand entirely.

[thinkyhead]

@FormerLurker — I decided to do more reworking of arc handling in that PR so that it applies some more constraints and works out as much about the segment length and number of segments as possible before starting the move. Please give it a test and see if it produces reasonable results. Note that the CI tests will show as "failing" on that PR, and that's okay. I applied some breaking changes to the arc configuration options, so it may not pass until the example configs are updated to match.

[FormerLurker]

I'm working on testing this out now. The config changes are a bit of a challenge since I'm trying to support many types and versions of various firmware, but this work will eventually need to be done anyway!

As long as we're at it, I'd like to direct you to a new gcode I'm trying to get added to the Prusa fork for setting arc parameters on the fly:

M214 [P] [S] [N] [R] [F]

Here is a definition from the pull request:

https://github.com/prusa3d/Prusa-Firmware/blob/621e15093d7b21828977c0b13e9ed5d939247357/Firmware/Marlin_main.cpp#L7431-L7446

It was marked as a milestone, so it may go into the next release. I've been intending to submit a PR for this, but wanted to mention it now since the settings may be changing a bit.

I'll let you know when I've got enough of your latest commit implemented to show some comparisons, thanks!

[thinkyhead]

Prusa doesn't have to think about fitting G2/G3 on smaller boards, so they can elaborate all day long. I might consider incorporating M214 but it would have to be an optional feature.

[FormerLurker]

Can you submit an issue on the arcwelderlib repo? I'd rather not cloud up this issue too much if possible.

[FormerLurker]

@thinkyhead, absolutely it should be optional to adhere to the marlin philosophy. I have dreamed up many use cases for such a gcode, bit I am guessing it would be better to post this in a separate issue. I will do that as soon as possible.

[thinkyhead]

Thanks! I don't which of the [P] [S] [N] [R] [F] parameters will still apply with the updated implementation, but we can have a closer look at that once the PR is in.

[thisiskeithb]

Fixed in #22599

[FormerLurker]

I see this is closed, but I'll still post the the resulting paths for comparison. The refactor for the new defines took me longer than expected, but I'm getting close. Thanks!

[github-actions]

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.