Add arc interpolation features (G2/G3) and M214 command for controlling settings #2657
Conversation
|
I'll give this PR a try tomorrow if I have time. Can you please post the gcode you mentioned in the initial comment? I'd like one for MK3(S) without MMU if that is possible. I like the idea of this PR quite a lot. It would be very interesting if arc commands were more streamlined into slicers... Oh well, guess we're stuck with the crappy STL format for now. It would be so nice if STEP files could be used in Slicers so that actual arcs in the model files could be used for generating the gcode... |
|
Yes sir, and thank you for your review! I will have all the changes posted to the repository by then. |
|
Related #2606 |
|
Here is a zip file containing gcode you can use to test the PR sliced for the MK3S: Description of Contents:
If you would like to try other models, or if you want me to convert other gocde files, please let me know. The full source for the Arc Welder conversion library, as well as a console application that can be used to produce the G2/G3 code from the command line are available in the ArcWelderLib repository that you can find in my github pages. See the ArcWelderConsole project for the command line version. There is also an InverseProcessor project in there that can be used to output gcode that shows the actual line segments that will be put into the planner (though it now needs some slight modification due to the recent modifications I made for this pull request). The project was created using Visual Studio, and some alterations will need to be made if you want to compile it from source. I could send windows binaries if you want. You might also like to try the ArcWelderPlugin for OctoPrint. It is in pre-release status ATM, but you can install from the OctoPrint plugin manager by clicking Get More and pasting this url into the From url... box: Please let me know if I can do anything else to help you test, and thank you for spending your valuable time on this PR! |
|
First, congratulations on getting the new release out! Second, I wanted to mention that I will be implementing some enhancements from this pull request that may bring the small angle approximation back into the picture. It turns out adding another term to the approximation makes a HUGE difference and has a relatively small cost. |
|
Hi all, do you plan add this feature to firmware, please? |
|
I am also in support of this PR |
|
Please add this feature to the firmware! |
|
Add this please 🙏 👍 |
|
Thumbs up for this! Please bring this to the firmware 👍 |
|
Working on some changes to bring this more in-line with Marlin 2. Re-adding N_ARC_CORRECTION and the small angle approximation with an additional term since it seems to be plenty accurate after that. Should speed up getting the first segment into the planner. Will also update M214 and M500 to reflect this. FYI, I'm also going to try to get M214 into marlin 2 where it could make it's way eventually (hopefully) into the Buddy firmware. |
|
Please add this feature to the firmware! |
|
Fyi, this has been slow going, but it should be ready in the next month. |
|
I hope this goes through in the near future as I have been having issues with my Pi Zero W plugged into the back of my mk3s+ and small circular details in which the nozzle seems to just smudge the entire detail leaving a mess, ex. very small half spheres looking more like smudged cylinders. It only happens when printing from OctoPrint even when the exact same GCode file is used from the SD card. One thing to note, the smudging seems to be a little less noticeable when using OctoPrint safe-mode but still not as good as the local SD card. My best guess now is that there is micro-stuttering happening due to the large quantity of G-codes associated with curves and the less than optimal specs of the Zero W. I was hoping this PR along with your ArcWelder plugin may help to alleviate that additional overhead and this would be a good addition to go along with the official Prusa Zero W guides as a sort of optimization. |
|
I've committed some recent and substantial changes, but have noticed one issue at the last minute. I will fix this, test, and amend this pull request ASAP, so no need to review this yet. Thank you. |
|
So, I added one line variable to the ConfigurationStore, a uint8_t, and now the compilation is failing. I noticed in the build details it says it is using 100% of the available memory, but I'm getting 93% using the .h file indicated in the error log. Any ideas as to what is going on? Edit: The .h file that failed was 1_75mm_MK3-EINSy10a-E3Dv6full.h. |
|
Here is what sketch claims to use for me when building for the MK3.h file:
|
|
You are building without multi language support. In reality, the code size is dangerously close to 100% |
|
@FormerLurker The firmware source code is for MK3, MK3S/+, MK2.5 and MK2.5S/+ (MK2, MK2S), so you have to pay attention to your code/PR that it will compile correctly for all variants. The Travis CI test compiles all variants and checks if they succeed:
So rule of thumb is to compile the firmware for all variants upfront before pushing a PR. The MK3 build contains the PAT9125 filament sensor which is more complex and so needs more resources compared to MK3S/+. Variants by recourse usage (high to low):
|
|
Thank you @leptun and @3d-gussner, I will work to reduce the LOC. It's right on the edge. I think the optimizer was omitting some code earlier due to an unreachable code block (code error due to a last minute change). I can def shave off a bit. Will report back. |
|
Makes sense. Let me think about it a bit more. We do have the advantage of being able to do some more complicated math for the recovery than during a normal command. |
|
Actually storing the radius and extrusion remaining may just do the trick. I don't think we can calculate the radius from the offset since we don't know the starting point. |
| // Clamp to the calculated position. | ||
| clamp_to_software_endstops(position); | ||
| // Insert the segment into the buffer | ||
| plan_buffer_line(position[X_AXIS], position[Y_AXIS], position[Z_AXIS], position[E_AXIS], feed_rate, extruder, position); |
There was a problem hiding this comment.
Depending on what we plan to do during recovery, the last argument to plan_buffer_line here the 4 floats are up for "abuse". In G1 this would be "target", since the final coordinate is the invariant during the move.
What we do for for arcs can be different.
There was a problem hiding this comment.
It can be different, but we still need to store XYZE since all of these could be relative. I just find it annoying that I and J is always relative as well. The only purpose of gcode_target is to fix issues with relative gcode commands
There was a problem hiding this comment.
Depending on how we see this, 4 extra floats will always be required somehow for either the (absolute) start or the end of the move because of this :/
There was a problem hiding this comment.
Can you explain how it's done differently in marlin 2.0 that could be used here (in order to save on resources)?
|
I approved this, I think we can work on it as we go :) |
|
The radius can be calculated only when the first (non interrupted) move is scheduled. We always know when we're working on a recovered move, so we can just re-use the old radius in that case. |
|
Well, considering we have 2 approvals, I'm going to merge this now and work from there. |
We don't store the radius globally. Only the offsets from the start position. Also, these get overridden by the next G2/3 command, so I'd be careful with those. The XYZE cached values also get overridden by any G1 command since the global cache for XYZE is common between G0/1/2/3 |
By this I mean store the radius just for the move. This would need to be stored for each block for this reason. Yes, it's ugly :] |
|
Using just the radius will create inconsistencies in the print. Consider the following scenario: If the move is stopped at the middle of a G2 segment, the radius should be actually smaller than if it was at a segment end (exactly the radius). This is probably good enough for high precision segments (max 1mm), but the lower resolution you go (>1mm), the worse the offset will get after resume. Also, the restarted move will not aligh with the segment split of the previous move. I think this will most likely result in some kind of visual artifact. |
I'm not sure this is true. Typically you would draw an arc on paper by calculating the midpoint of the circle (move from starting point by the relative offset), and drawing from the current position to the target position. However, If you flip your compass around (pointy end on the current position instead of the center of the circle), and draw a circle of a fixed radius, it will always intersect in the same two places as long as the pointy end is on the arc. The correct intersection depends on the direction. Here's a crude sketch I made with a screwdriver, a pen, and a piece of foam, so it's not great, but accurate enough to get the idea:
From that, you can reconstruct the offset by finding the intersections of two circles, selecting the correct intersection (not sure how to do that just yet), and just figuring the X and Y coordinates of the vector from the starting point to the intersection. This assumes we will know the current absolute position, and that it is on the arc somewhere. I'll try to create a program that will, given a G2/G3 command and a point along the arc, will attempt to create a new G2/G3 command to complete the arc with only the radius, the extrusion remaining, and the target coordinates (do we know the final destination of the G2/G3, or just the final destination of the interrupted segment?) |
|
I'm not sure I understand the explanation. I'll try to explain my part a bit more: Consider the following sequence: In the picture above, the resolution of the arc was intentionally dropped to highlight the issue. If we use start position, end position and radius for the second arc, this is what it would look like (blue line): |
|
Also, something I didn't consider at all is the handling of broken arc commands. What I mean by this is when the center offsets create invalid geometry. These arcs can be identified by the fact they draw an arc, and then do a straight last move to the target position. I'm 100% sure we can't replicate their behaviour on resume (not that we would want to do that, but still something to consider). |
|
First, THANK YOU SO MUCH for merging this in! I'm so thankful for all of your time and effort! Next, I'm not sure where to carry on the discussion about pause/resume. Should I create an issue where we can work on a solution? |
|
Yes, please create an issue about this where we can dump all the info we have so far (and ideas) |
leptun
added
the
Gcode arcs
|
@FormerLurker Thanks again for this PR. Finally it got merged. 🥳 |
|
@3d-gussner, thank all of YOU for your time and support! I'm familiar with the wiki (I have used it EXTENSIVELY over the years), but I've never edited anything. I will take a look as soon as I have some time, and will let you know how it goes. |
|
Usage question. PrusaSlicer doesn't do G2/G3 for arcs, right? Based on prusa3d/PrusaSlicer#4352 the only way to play with this is ArcWelder plugin, right? (and no solution to wire that into prusaslicer) Edit: never mind, this explains everything |
|
AFAIK it hasn't been integrated in prusaslicer yet because post-processing the gcode as arc-welder does can result in non-coherent segments across layers. This can result in different printing behavior from multiple fronts that the slicer cannot control, so it makes reported slicing issues much harder to reason about. It's ironic that it was merged into a 32bit FW, where I would consider it's primary feature (the resulting g-code space savings) not so important. |
FYI, I have heard this before from several people, but have never seen an example of this with any reasonable resolution setting. Also, the resolution could be set by the slicer to prevent this. Every time this 'issue' has been reported to me, it's been a visualization error within some slicer or gcode viewer. I can think of many reasons why this feature isn't added to PrusaSlicer, but I don't think the above is a good one.
The MK3 firmware is 32 bit? What about the MK2? I ran this fork on my MK2 a bunch for testing.
For me, the main feature is reduced serial transfer so I can stop hearing about 'zits' being created when using OctoPrint :) It was a rare issue, but annoying, and created a lot of github issues for Octolapse. |
|
@FormerLurker
The MK3 is still the 8 bit firmware. Only Mini and newer are the 32 bit firmware (buddy firmware). The MK2 is a separate fork of the 8 bit firmware, but it is not being actively maintained. |
|
@3d-gussner |
|
@johnnynoone Thanks for the reminder. Done https://reprap.org/wiki/G-code#M214:_Set_Arc_configuration_values @FormerLurker Is that okay so? |
I don't expect this pull request to be immediately accepted, and would greatly appreciate your thoughts and feelings about the changes I have made. This is my first attempt to modify any firmware, so I expect a lot of critical feedback.
I recently created a program to convert G0/G1 commands into G2/G3 commands where possible, and noticed that some arcs were coming out as flat edges. These edges were clearly rounded in the original gcode, and I tracked the issue down to the MM_PER_ARC_SEGMENT setting. I did not have much luck at all decreasing this setting. A value of 0.1 (similar to the minimum distance between segments in my original file) yielded terrible stuttering that degraded the print quality severely. A value of 0.75 yielded flat edges in some places. I couldn't seem to find a good value that produced rounded small curves that didn't result in stuttering that impacted quality, so I started exploring the Marlin 2.0 code, and came up with a few ideas of my own that I've added to this pull request.
I've added the following features from Marlin 2.0 to enhance arc interpolation:
MIN_ARC_SEGMENTS - Minimum number of segments in a full circle of any radius (0 = disabled)
ARC_SEGMENTS_PER_SEC - Number of segments per second to be generated based on current F (0 = disabled)
Note that when the two settings above conflict, the smallest arc length calculated from the above two settings is used. Also MM_PER_ARC_SEGMENT is the max segment length allowed.
I've added a third setting to prevent extremely small segments from being generated:
MIN_MM_PER_ARC_SEGMENT - The minimum length of any segment (0 = disabled)
I've also added a new command, M214 (is that and appropriate gcode?) with the following definition that can be used to control the above settings:
Please let me know if you feel the parameters I used (N, S, R and F) seem appropriate to you.
I also added M214 settings to the M503 configuration display command, which produces output similar to this:
I have tested M500 to ensure it works as expected for storing these settings to EEPROM. You will find these new settings within the ConfigurationStore.h file:
I've added default settings for the arc interpolation to each .h file in the /variants folder and have removed the settings from configuration_adv.h.
I have tested installing the latest firmware release (3.8.1) and then upgrading to my variant, and the default settings seemed to load correctly.
The thing I am NOT sure about is the initialization of these new settings. Though I'm sure you will review this completely, and some changes will be necessary, I am not at all sure that I did the initialization properly within ConfigurationStore.cpp. Any help or advice there would be greatly appreciated.
I also removed the N_ARC_CORRECTION setting and functionality completely. I was seeing bad interpolations using a wide variety of settings (endpoints quite far from the arc itself) due to the small angle approximation of COS and SIN. I removed the small angle approximation completely, so there is one COS and one SIN calculation that happens at the beginning of the interpolation loop as long as there is more than one segment. This change immediately resulted in arcs whose endpoints fit nearly exactly along the arc itself for all sizes I tested. The correction factor seemed completely unnecessary after using accurate SIN/COS values. I realize there is a trade-off here, since using those functions results in the first arc segment being inserted into the planner a bit later, but in my testing not only did I not notice any performance difference, but the resulting arcs fit better, especially in arcs of a small radius or where MM_PER_ARC_SEGMENT is large. I did experiment with using a cutoff angle for deciding when to use true SIN and COS, but I did not notice any performance gains at all, so I dropped that in favor of simpler code. Please let me know what you think of this. I wrote a small program that generates the gcode equivalent of the interpolated arcs the firmware is producing if you are wondering how I discovered this.
Additionally, I think the default settings may need to be tweaked somewhat, though they seem to work well for me. There is still some slowdown (but no stuttering) with the default settings when printing arcs with G2/G3 of a very small radius at high speeds (50+mm/sec), and I think that could be eliminated. Now that I've added M214, testing should be MUCH easier. I was hoping to find a way to print multiple copies of the same object and inject the M214 command between each object, but I have not yet found a way to do that automatically. I'm sure there is a way, I just haven't found it yet.
If you need some gcode that includes a lot of arcs to test these changes, please let me know. I have code for the MK2.5 w/mmu 2.0, and could generate gcode for any model you want. I created a 1 layer file containing 6 circles using archimedean chord fill where each circle is printed at a different speed (from 10-60MM/s) to test stuttering, which was quite useful and illuminating.
Thank you very much for your time and consideration!