From 870855a29f02d0ee8d141ca619c5a54611abfd0d Mon Sep 17 00:00:00 2001 From: Yash Patel Date: Fri, 11 Dec 2020 07:53:25 -0600 Subject: [PATCH 1/8] Proposed fix for #20382 (Unreliable circle detection) --- Marlin/src/gcode/motion/G2_G3.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index 9c6710a08de4..008cb778e731 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -77,8 +77,9 @@ void plan_arc( rt_Y = cart[q_axis] - center_Q, start_L = current_position[l_axis]; - // Angle of rotation between position and target from the circle center. - float angular_travel = ATAN2(rvec.a * rt_Y - rvec.b * rt_X, rvec.a * rt_X + rvec.b * rt_Y); + // Angle of rotation between position and target from the circle center. Zero if target and position same. + const bool target_position_same = NEAR(current_position[p_axis], cart[p_axis]) && NEAR(current_position[q_axis], cart[q_axis]); + float angular_travel = target_position_same ? 0 : ATAN2(rvec.a * rt_Y - rvec.b * rt_X, rvec.a * rt_X + rvec.b * rt_Y); #ifdef MIN_ARC_SEGMENTS uint16_t min_segments = MIN_ARC_SEGMENTS; @@ -86,8 +87,8 @@ void plan_arc( constexpr uint16_t min_segments = 1; #endif - // Do a full circle if angular rotation is near 0 and the target is current position - if (!angular_travel || (NEAR_ZERO(angular_travel) && NEAR(current_position[p_axis], cart[p_axis]) && NEAR(current_position[q_axis], cart[q_axis]))) { + // Do a full circle if angular rotation is 0 + if (!angular_travel) { // Preserve direction for circles angular_travel = clockwise ? -RADIANS(360) : RADIANS(360); } From b6661f3a9d79e927a58c60135b1198588fe149ed Mon Sep 17 00:00:00 2001 From: Yash Patel Date: Fri, 11 Dec 2020 09:46:59 -0600 Subject: [PATCH 2/8] Altnernate Proposed fix for #20382 (Unreliable circle detection) Warning, this changes logic of what is considered circle. Changes default behavior of zero value to represent zero instead of assumed circle. This, imo, is more consistent, but it's up for debate. If original logic is preffered, use prior commit instead of this one. --- Marlin/src/gcode/motion/G2_G3.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index 008cb778e731..55f60cb21372 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -77,9 +77,8 @@ void plan_arc( rt_Y = cart[q_axis] - center_Q, start_L = current_position[l_axis]; - // Angle of rotation between position and target from the circle center. Zero if target and position same. - const bool target_position_same = NEAR(current_position[p_axis], cart[p_axis]) && NEAR(current_position[q_axis], cart[q_axis]); - float angular_travel = target_position_same ? 0 : ATAN2(rvec.a * rt_Y - rvec.b * rt_X, rvec.a * rt_X + rvec.b * rt_Y); + // Angle of rotation between position and target from the circle center. Will be determined below. + float angular_travel = 0; #ifdef MIN_ARC_SEGMENTS uint16_t min_segments = MIN_ARC_SEGMENTS; @@ -87,17 +86,20 @@ void plan_arc( constexpr uint16_t min_segments = 1; #endif - // Do a full circle if angular rotation is 0 - if (!angular_travel) { + // Do a full circle if starting and ending positions same + if (NEAR(current_position[p_axis], cart[p_axis]) && NEAR(current_position[q_axis], cart[q_axis])) { // Preserve direction for circles angular_travel = clockwise ? -RADIANS(360) : RADIANS(360); } else { - // Make sure angular travel over 180 degrees goes the other way around. + //Calculate angle + angular_travel = ATAN2(rvec.a * rt_Y - rvec.b * rt_X, rvec.a * rt_X + rvec.b * rt_Y); + // Make sure angular travel over 180 degrees goes the other way around, but ensure zero stays zero + if (angular_travel) { //only if != 0 switch (((angular_travel < 0) << 1) | clockwise) { case 1: angular_travel -= RADIANS(360); break; // Positive but CW? Reverse direction. case 2: angular_travel += RADIANS(360); break; // Negative but CCW? Reverse direction. - } + } } #ifdef MIN_ARC_SEGMENTS min_segments = CEIL(min_segments * ABS(angular_travel) / RADIANS(360)); NOLESS(min_segments, 1U); From 987d18a1ce5d21669059f64eeb2faada5f1eefd1 Mon Sep 17 00:00:00 2001 From: Yash Patel Date: Fri, 11 Dec 2020 10:52:04 -0600 Subject: [PATCH 3/8] Alternate proposal addendum Adds portion that bails if angle zero but target != position. --- Marlin/src/gcode/motion/G2_G3.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index 55f60cb21372..6b4d4877e0fc 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -99,7 +99,7 @@ void plan_arc( switch (((angular_travel < 0) << 1) | clockwise) { case 1: angular_travel -= RADIANS(360); break; // Positive but CW? Reverse direction. case 2: angular_travel += RADIANS(360); break; // Negative but CCW? Reverse direction. - } } + } } else return; //Target != position, but angle == 0? Bail. In future, maybe add logic to replace G2/G3 with single G1 command to target position. #ifdef MIN_ARC_SEGMENTS min_segments = CEIL(min_segments * ABS(angular_travel) / RADIANS(360)); NOLESS(min_segments, 1U); From 05ea5e20ff6e8e103c404a000bcb45322af762c1 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Tue, 22 Dec 2020 20:25:56 -0600 Subject: [PATCH 4/8] Nest less --- Marlin/src/gcode/motion/G2_G3.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index 6b4d4877e0fc..f99807b333fd 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -92,14 +92,18 @@ void plan_arc( angular_travel = clockwise ? -RADIANS(360) : RADIANS(360); } else { - //Calculate angle + // Calculate the angle angular_travel = ATAN2(rvec.a * rt_Y - rvec.b * rt_X, rvec.a * rt_X + rvec.b * rt_Y); - // Make sure angular travel over 180 degrees goes the other way around, but ensure zero stays zero - if (angular_travel) { //only if != 0 + + // Angular travel too small to detect? Just return. + if (!angular_travel) return; + + // Make sure angular travel over 180 degrees goes the other way around switch (((angular_travel < 0) << 1) | clockwise) { case 1: angular_travel -= RADIANS(360); break; // Positive but CW? Reverse direction. case 2: angular_travel += RADIANS(360); break; // Negative but CCW? Reverse direction. - } } else return; //Target != position, but angle == 0? Bail. In future, maybe add logic to replace G2/G3 with single G1 command to target position. + } + #ifdef MIN_ARC_SEGMENTS min_segments = CEIL(min_segments * ABS(angular_travel) / RADIANS(360)); NOLESS(min_segments, 1U); From 80441b5ededc009585923a96ea30827e56dbd9e3 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Tue, 22 Dec 2020 20:27:49 -0600 Subject: [PATCH 5/8] Update G2_G3.cpp --- Marlin/src/gcode/motion/G2_G3.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index f99807b333fd..7f7c50383e04 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -86,7 +86,7 @@ void plan_arc( constexpr uint16_t min_segments = 1; #endif - // Do a full circle if starting and ending positions same + // Do a full circle if starting and ending positions are "identical" if (NEAR(current_position[p_axis], cart[p_axis]) && NEAR(current_position[q_axis], cart[q_axis])) { // Preserve direction for circles angular_travel = clockwise ? -RADIANS(360) : RADIANS(360); From 03b582205641eece405f74bc021615625d66dc5c Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Tue, 22 Dec 2020 20:29:32 -0600 Subject: [PATCH 6/8] Update G2_G3.cpp --- Marlin/src/gcode/motion/G2_G3.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index 7f7c50383e04..af85243b8f8a 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -77,15 +77,15 @@ void plan_arc( rt_Y = cart[q_axis] - center_Q, start_L = current_position[l_axis]; - // Angle of rotation between position and target from the circle center. Will be determined below. - float angular_travel = 0; - #ifdef MIN_ARC_SEGMENTS uint16_t min_segments = MIN_ARC_SEGMENTS; #else constexpr uint16_t min_segments = 1; #endif + // Angle of rotation between position and target from the circle center. + float angular_travel; + // Do a full circle if starting and ending positions are "identical" if (NEAR(current_position[p_axis], cart[p_axis]) && NEAR(current_position[q_axis], cart[q_axis])) { // Preserve direction for circles From d74bbba331873d34ad401456db66a943a8547de8 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Tue, 22 Dec 2020 20:30:48 -0600 Subject: [PATCH 7/8] Update G2_G3.cpp --- Marlin/src/gcode/motion/G2_G3.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index af85243b8f8a..7fe9ab6aeb40 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -98,7 +98,7 @@ void plan_arc( // Angular travel too small to detect? Just return. if (!angular_travel) return; - // Make sure angular travel over 180 degrees goes the other way around + // Make sure angular travel over 180 degrees goes the other way around. switch (((angular_travel < 0) << 1) | clockwise) { case 1: angular_travel -= RADIANS(360); break; // Positive but CW? Reverse direction. case 2: angular_travel += RADIANS(360); break; // Negative but CCW? Reverse direction. From 4ff11e8a465674780a0ef22314157a429a86f507 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Tue, 22 Dec 2020 20:49:34 -0600 Subject: [PATCH 8/8] Update plan_arc comment --- Marlin/src/gcode/motion/G2_G3.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Marlin/src/gcode/motion/G2_G3.cpp b/Marlin/src/gcode/motion/G2_G3.cpp index 7fe9ab6aeb40..61e50247f3f7 100644 --- a/Marlin/src/gcode/motion/G2_G3.cpp +++ b/Marlin/src/gcode/motion/G2_G3.cpp @@ -41,13 +41,12 @@ #endif /** - * Plan an arc in 2 dimensions + * Plan an arc in 2 dimensions, with optional linear motion in a 3rd dimension * - * The arc is approximated by generating many small linear segments. - * The length of each segment is configured in MM_PER_ARC_SEGMENT (Default 1mm) - * Arcs should only be made relatively large (over 5mm), as larger arcs with - * larger segments will tend to be more efficient. Your slicer should have - * options for G2/G3 arc generation. In future these options may be GCode tunable. + * The arc is traced by generating many small linear segments, as configured by + * MM_PER_ARC_SEGMENT (Default 1mm). In the future we hope more slicers will include + * an option to generate G2/G3 arcs for curved surfaces, as this will allow faster + * boards to produce much smoother curved surfaces. */ void plan_arc( const xyze_pos_t &cart, // Destination position