Skip to content
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

[bugfix 1.1.x] Fix G2/G3 so that slicer rounding error does not require impossible arc #15546

Merged
merged 2 commits into from
Oct 13, 2019

Conversation

edwilliams16
Copy link
Contributor

Requirements

Description

This is a back port of a fix to bugfix-2.0.x
It fixes the problem caused by slicer rounding error creating a G2/G3 request for a geometrically impossible arc whose diameter would be less than the distance to the destination point.

Benefits

Fixes #14745

Related Issues

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 13, 2019

Thank You Ed!

You aren't going to be able to claim you don't know C if you keep using ternary operators....

@edwilliams16
Copy link
Contributor Author

@Roxy-3D
I try not to confuse it with the Fortran arithmetic (3-way) if:
IF (expression) negative,zero,positive from the days when goto's were popular.

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 13, 2019

Personally... I really dislike ternary operators. But they do have their place.

For the Marlin coding standards, what you have coded is good! But for me, personally, I would prefer looking at something more like:

                      h2 = ( r - 0.5f * d) * (r + 0.5f * d),        // factor to reduce rounding error         
                      if (h2 >= 0.0)
                            h = SQRT(h2);
                      else
                            h = 0.0;

@edwilliams16
Copy link
Contributor Author

I'd vote for if then else if the true/false expressions were more complicated than they are here.

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 13, 2019

I'm not criticizing your code!!! It is fully compliant with the Marlin code base! I'm just expressing a personal preference. I find a multiple line if-then-else easier to read.

@edwilliams16
Copy link
Contributor Author

I'm not taking it as criticism! As I said if the expressions were not so simple I'd agree with you.
It's a space/clarity trade-off.

@edwilliams16
Copy link
Contributor Author

@Roxy-3D
I think one of the more obscure uses of the ternary operator occurs just a few lines earlier in the code:
const float e = clockwise ^ (r < 0) ? -1 : 1, // clockwise -1/1, counterclockwise 1/-1
This determines the sign (e) of the arc_offset vector, - if clockwise, + if counter-clockwise BUT with the convention that if the arc-radius r is negative, the direction of rotation is reversed. I guess some slicer must emit only G2's and flip the sign of r rather than using G3's.

clockwise ^ (r < 0) has the correct truth table to achieve this result, as would the equally obscure clockwise != (r < 0)
I was really tempted to change it to something like:

     e = clockwise ? -1 : 1;    // sign of arc
     e *= (r >= 0) ? 1 : -1;     // flip if r < 0

or even
e = (clockwise ? -1 : 1) * ((r >= 0) ? 1 : -1);

but I didn't think it appropriate to change merely on the grounds of clarity (to me). Who knows, the XOR trick could be a common idiom that I was unaware of.

@thinkyhead
Copy link
Member

No worries @edwilliams16 — Whatever idiom works best for you. The important thing is that it changes the right bits in the right way at the right time, and the code is not intentionally obfuscatory.

@thinkyhead thinkyhead merged commit 5754472 into MarlinFirmware:bugfix-1.1.x Oct 13, 2019
@edwilliams16 edwilliams16 deleted the G2_G3_fix branch October 13, 2019 22:54
TiagoJustino added a commit to TiagoJustino/Marlin that referenced this pull request Apr 2, 2020
* MarlinFirmware/1.1.x: (133 commits)
  Fix Z position after ABL bilinear G29 with fade (MarlinFirmware#17174)
  Change DUMMY_PID_VALUE to NAN (MarlinFirmware#17079)
  Disable integration testing
  Enable DUAL_NOZZLE_DUPLICATION_MODE (MarlinFirmware#16436)
  [1.1.x] Polish language UTF8 (MarlinFirmware#16141)
  Links for the "New Issue" page
  Add Korean language (MarlinFirmware#15918)
  [1.1.x] MKS_GEN_L_V2 controller (MarlinFirmware#15805)
  Update bug_report.md
  [1.1.x] Fix autostart w/out SD_DETECT_PIN (MarlinFirmware#15667)
  [1.1.x] RAMPS + Viki1 LCD compatibility (MarlinFirmware#15736)
  [1.1.x] Fix compiler warning (MarlinFirmware#15642)
  [1.1.x] Fix for G2/G3 rounding error (MarlinFirmware#15546)
  Let MINIPANEL use SW SPI if needed (MarlinFirmware#15246)
  Fix BLTouch debugging (MarlinFirmware#15232)
  Fix Creality bed thermistor
  Fix PT-BR strings (MarlinFirmware#15023)
  Fix #else extra tokens (MarlinFirmware#15013)
  [1.1.x] Autobuild formatting (MarlinFirmware#14858)
  [1.1.x] BLTouch 3.0 - 3.1 (MarlinFirmware#14839)
  ...
maz3max pushed a commit to maz3max/Marlin that referenced this pull request Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants