-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ARC_SEGMENTS_PER_SEC for finer G2/G3 arcs #16510
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,7 +103,15 @@ void plan_arc( | |
mm_of_travel = linear_travel ? HYPOT(flat_mm, linear_travel) : ABS(flat_mm); | ||
if (mm_of_travel < 0.001f) return; | ||
|
||
uint16_t segments = FLOOR(mm_of_travel / (MM_PER_ARC_SEGMENT)); | ||
const feedRate_t scaled_fr_mm_s = MMS_SCALED(feedrate_mm_s); | ||
|
||
#ifdef ARC_SEGMENTS_PER_SEC | ||
float seg_length = scaled_fr_mm_s * _RECIP(ARC_SEGMENTS_PER_SEC); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usage of private-styled macro _RECIP from another module. I have compilation fail here: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to use RECIPROCAL() macro instead? |
||
NOLESS(seg_length, MM_PER_ARC_SEGMENT); | ||
#else | ||
constexpr float seg_length = MM_PER_ARC_SEGMENT; | ||
#endif | ||
uint16_t segments = FLOOR(mm_of_travel / seg_length); | ||
NOLESS(segments, min_segments); | ||
|
||
/** | ||
|
@@ -146,10 +154,9 @@ void plan_arc( | |
// Initialize the extruder axis | ||
raw.e = current_position.e; | ||
|
||
const feedRate_t scaled_fr_mm_s = MMS_SCALED(feedrate_mm_s); | ||
|
||
#if ENABLED(SCARA_FEEDRATE_SCALING) | ||
const float inv_duration = scaled_fr_mm_s / MM_PER_ARC_SEGMENT; | ||
const float inv_duration = scaled_fr_mm_s / seg_length; | ||
#endif | ||
|
||
millis_t next_idle_ms = millis() + 200UL; | ||
|
@@ -206,7 +213,7 @@ void plan_arc( | |
planner.apply_leveling(raw); | ||
#endif | ||
|
||
if (!planner.buffer_line(raw, scaled_fr_mm_s, active_extruder, MM_PER_ARC_SEGMENT | ||
if (!planner.buffer_line(raw, scaled_fr_mm_s, active_extruder, seg_length | ||
#if ENABLED(SCARA_FEEDRATE_SCALING) | ||
, inv_duration | ||
#endif | ||
|
@@ -226,7 +233,7 @@ void plan_arc( | |
planner.apply_leveling(raw); | ||
#endif | ||
|
||
planner.buffer_line(raw, scaled_fr_mm_s, active_extruder, MM_PER_ARC_SEGMENT | ||
planner.buffer_line(raw, scaled_fr_mm_s, active_extruder, seg_length | ||
#if ENABLED(SCARA_FEEDRATE_SCALING) | ||
, inv_duration | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a logic or linguistic error.
#define MM_PER_ARC_SEGMENT 1 // (mm) Length (or minimum length) of each arc segment
-> No segment can be smaller than 1 mm!#define MIN_ARC_SEGMENTS 24 // Minimum number of segments in a complete circle
-> When the circumference of the circle is smaller than 24 mm print a circle with not smaller than that - because the segments cant be smaller then 1mm.One of this defines is unlikely to be a minimum. Else arcs with small diameters are impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIN_ARC_SEGMENTS overrides MM_PER_ARC_SEGMENT to make possibly shorter segments. Personally I don't like MIN_ARC_SEGMENTS because it is trying to enforce smoothness irrespective of radius or feed rate, and it can generate excessive resolution that causes stalls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
MM_PER_ARC_SEGMENT
isn't a minimum but a maximum segment length.And is that is right now, there is a conflict with:
//#define ARC_SEGMENTS_PER_SEC 50 // Use feedrate to choose segment length (with MM_PER_ARC_SEGMENT as the minimum)
If it's a maximum as well as a minimum there is no other possible value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
MIN_STEPS_PER_SEGMENT
we already have a lower limit for the length of one segment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MM_PER_ARC_SEGMENT, when not using ARC_SEGMENTS_PER_SEC, is the 'default' segment length and it can be shorter if MIN_ARC_SEGMENTS implies more smoothness than MM_PER_ARC_SEGMENT provides.
When using ARC_SEGMENTS_PER_SEC, the 'usual' behavior is for segment length to be determined by the feed rate but for very low feed rates the segments can become extremely short. If the feed rate is low enough that the segments are shorter than MM_PER_ARC_SEGMENT then it uses MM_PER_ARC_SEGMENT instead, so in that sense it is a minimum on the speed-derived segment length. But the MIN_ARC_SEGMENTS is still in effect, so a very fast small circle might be a hexagon according to ARC_SEGMENTS_PER_SEC, and MIN_ARC_SEGMENTS of 7 or more would force a smoother curve and the segments would be made shorter (and the segment rate would be higher than ARC_SEGMENTS_PER_SEC).
Not all combinations of these settings make sense, for example a large MM_PER_ARC_SEGMENT and a high rate of segments per second would mean the MM_PER_ARC_SEGMENT would almost always take precedence and the segments per second has no effect. A large value for MIN_ARC_SEGMENTS would mean that small circles will ignore feed rate or (otherwise) minimum resolution and proceed with possibly extremely fine resolution that has no benefit.
Yes MIN_STEPS_PER_SEGMENT causes small segments to collapse, so there is no real benefit resolution-wise for MM_PER_ARC_SEGMENT to be much less than that.
I think a combination of settings that makes sense is for ARC_SEGMENTS_PER_SEC to be a value that the CPU can keep up with comfortably, and for MM_PER_ARC_SEGMENT to be a small value similar to the length that corresponds to MIN_STEPS_PER_SEGMENT. And MIN_ARC_SEGMENTS should not be used because ARC_SEGMENTS_PER_SEC will already provide the best smoothness that's reasonable at a given feed rate.