Marlin: plan_arc() and plan_cubic_move() have a hidden dependency on destination

[ManuelMcLure]

Background
I'm working on code to allow the G26 test pattern to use the ARC_SUPPORT functions if available instead of rolling its own line-drawing functions.
Problem
Although both plan_arc() and plan_cubic_move() take explicit endpoint parameters, they end with a call to set_current_from_destination(). This means that unless destination is set to the same position as the passed-in endpoint current_position will be wrong when these functions return. This is not a problem in current code since the callers set destination and pass it in as the endpoint, but can be confusing for future development (at least, it confused me).
Solutions
I can see several solutions for this:

1. I can explicitly set destination in my updated G26 code. This leaves the possibility of future confusion.
2. plan_arc() and plan_cubic_move() can ignore destination and explicitly set current_position to the passed-in endpoint.
3. plan_arc() and plan_cubic_move() can explicitly set destination to the passed-in endpoint before the set_current_from_destination() call. I don't think this gives us anything that option 2. doesn't.
4. Rename plan_arc() and plan_cubic_move() to plan_arc_to_destination() and plan_cubic_move_to_destination() and remove the endpoint parameter, instead simply using destination as the endpoint. This would explicitly document the requirement for destination to be set correctly.

My preference is for 2. although I could see 4. as an option as well. 1. leaves the confusion in place and I don't see that 3. is any better than 2..

Since I'm already familiar with the code I would be happy to do a PR for whatever option is selected.

[thinkyhead]

This means that unless destination is set to the same position as the passed-in endpoint…

The core motion functions all use destination, which originally exists to serve G0, G1, G2, G3, G5, etc., which all get destination from the G-code via gcode_get_destination. Other functions may use destination if they know they will not interfere with that original usage.

[thinkyhead]

At the end of plan_arc, change:

- set_current_from_destination();
+ COPY(current_position, cart);

[ManuelMcLure]

So option 2. :) I'll generate a PR when I get home this evening. Thanks! This should be the last puzzle piece before I can make a PR for my G26-using-arcs change.

[thinkyhead]

Same deal here…

void plan_cubic_move(const float (&cart)[XYZE], const float (&offset)[4]) {
  cubic_b_spline(current_position, cart, offset, MMS_SCALED(feedrate_mm_s), active_extruder);
  COPY(current_position, cart);
}

    plan_cubic_move(destination, offset);

[Roxy-3D]

This is not a problem in current code since the callers set destination and pass it in as the endpoint, but can be confusing for future development (at least, it confused me).

If you think it is confusing now... Start your reading here: https://github.com/MarlinFirmware/Marlin/issues/8025#issuecomment-338119349 and in particular: https://github.com/MarlinFirmware/Marlin/issues/8025#issuecomment-338240845

But that chuckle aside... I'm looking forward to trying your Arc circles on G26!!!!!

Thank You for the help!

[ManuelMcLure]

I'm looking forward to trying your Arc cicles on G26!!!!!

I almost have them working - in my test the circles looked great (although I may need to tweak the extrusion a bit) but because current_position wasn't what I expected after the circle was printed only about 1/3 of the connecting lines were being drawn correctly. Hopefully this fix will make everything work correctly. I've committed some fixes but I don't want to do a PR until I've had a chance to at least compile them and that will have to wait until I'm back at the printer.

[thinkyhead]

How much extra PROGMEM does this add to G26? It might be useful to have the option to use the original (smaller) code, for boards where the flash budget is limited.

[ManuelMcLure]

I believe the new code is actually smaller than the existing code, since it passes most of the heavy lifting off to plan_arc(). I have it conditionally compile the existing code if ARC_SUPPORT isn't set so that's the fallback. But when I get home I'll compile without ARC_SUPPORT, with ARC_SUPPORT + original code for G26 and with ARC_SUPPORT + my new code and give you the numbers. My gut feel is that if you already have ARC_SUPPORT enabled the new code will actually be a PROGMEM win over the old code. But I could be mistaken.

[thinkyhead]

It will be good to compare. ARC_SUPPORT is one of the features we recommend users disable on 128K boards to recover space when needed.

[Roxy-3D]

Great! Incidentally... It is OK to have lots of commits on the Pull Request. When we (eventually) merge the Pull Request, one of the options is a 'Squash and Merge'. That collapses everything down. What that does is make it easy for others to review, update and improve your Pull Request prior to it being merged.

So... don't worry about having too many commits in it.

[Roxy-3D]

It will be good to compare. ARC_SUPPORT is one of the features we recommend users disable on 128K boards to recover space when needed.

Yes. Depending on the results... It may make sense to leave the original simulated circles in place if ARC_SUPPORT is disabled. But my guess is that code is big enough it won't save anything compared to just turning on ARC_SUPPORT.

[ManuelMcLure]

Yeah - that's why I have it fall back to the original code if ARC_SUPPORT is disabled. I didn't want ARC_SUPPORT to be a requirement for G26_MESH_VALIDATION.

[Roxy-3D]

The G26 simulated circle drawing code isn't very optimized. So... if the program memory comes out close with ARC_SUPPORT and the new code compared to the old code, it might make sense to just turn on ARC_SUPPORT in conditionals.h if G26 is turned on. (And then the code can be simplified by removing the old simulated circles with no increase in program memory).

We can figure out the correct path to go once we have the new code using the G2 Arc's.

[ManuelMcLure]

Incidentally... It is OK to have lots of commits on the Pull Request. When we (eventually) merge the Pull Request, one of the options is a 'Squash and Merge'. That collapses everything down. What that does is make it easy for others to review, update and improve your Pull Request prior to it being merged.

Thanks for the tip. I'm not yet very familiar with Git (I use SVN in my day job) so every bit of info helps!

[ManuelMcLure]

So I tested this and got the following values for PROGMEM (same Configuration.h and Configuration_adv.h except for ARC_SUPPORT) compiling with Arduino 1.8.5:

bugfix-1.1.x without ARC_SUPPORT: 192232 bytes
bugfix-1.1.x with ARC_SUPPORT: 195416 bytes (+3184bytes)
my branch without ARC_SUPPORT: 192236 bytes (+4 bytes)
my branch with ARC_SUPPORT: 195122 bytes (+2890 bytes)

So right now my changes are a small (294 bytes) gain of PROGMEM over the old code when ARC_SUPPORT is already enabled, but that's not enough to compensate for the 3184 bytes that ARC_SUPPORT costs. I honestly don't know where the extra 4 bytes are coming from in the no ARC_SUPPORT case.

Most of my code is handling the edge cases (literally) where we need to draw a partial circle when setting up the parameters to plan_arc()

You can take a look at my changes at https://github.com/ManuelMcLure/Marlin/tree/G26_arc_move.

[ManuelMcLure]

I thought my missing lines in the G26 were due to this issue, but it seems like there's something else going on. The symptom is that the first line printed by each look_for_lines_to_connect() call gets printed in the air and with no extrusion, but the second (if there is one) prints fine. Time to debug further.

[Roxy-3D]

The G26 code does nozzle lifts to prevent scraping lines off the bed when it passes over them.

I bet something to do with that is causing trouble. It may (or may not) be in the G26 code. It might be down lower in the recently modified G2 Arc code.

[ManuelMcLure]

I've figured it out - there are some weird dependencies in the G26 code, specifically print_line_from_here_to_there() depends on destination being set to current_position for the retract/unretract code to work correctly. I solved it by adding a set_destination_from_current() right after drawing the circle and am getting a nice pattern. I've created a pull request for the new G26 code: #10695

I may take a look at cleaning up the G26 code to get rid of some of these hidden dependencies.

[ManuelMcLure]

Fixes checked in and merged. Closing.

[github-actions[bot]]

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.