Add support for G2 & G3 commands that use radius R instead of I & J.

gb0101010101 commented 4 years ago

What does this pull request do?

Adds support for G2 & G3 commands that use radius R instead of I & J. e.g. G02 X2.0 Y6.0 R2.0

The code only executes if R is provided in G2 or G3 command. If R, I & J are provided then I & J will be ignored.

Previously this was not supported and code execution would fail if the command was used.

Calculations are copied from MarlinFirmware https://github.com/MarlinFirmware/Marlin/pull/4840/commits/c2744d8a8bdd35a824a1017434a7b3007debdc1e

Does this firmware change affect kinematics or any part of the calibration process?

No.

How can this pull request be tested?

Turn on verbose debugging to see calculated points in console output. This has not been tested on physical machine. I will not have access to a machine for a while.

Rendered output: http://www.helmancnc.com/cnc-g02-circular-interpolation-clockwise-cnc-milling-sample-program/

Test G2 code

G90 G00 X-2.0 Y-1.0 
G01 X0 Y0 F30.0 ; point A 
Y4.0 ; point B 
G02 X2.0 Y6.0 R2.0 ; point C 
G01 X8.0 ; point D 
G02 X9.0 Y2.268 R2.0 ; point E 
G01 X0 Y0 ; point A 
G00 X-2.0 Y-1.0

Test G3 code

G90 G00 X-2.0 Y-1.0 
G01 X0 Y0 F30.0 ; point A 
Y4.0 ; point B 
G03 X-2.0 Y6.0 R2.0 ; point C 
G01 X-8.0 ; point D 
G03 X-9.0 Y2.268 R2.0 ; point E 
G01 X0 Y0 ; point A 
G00 X-2.0 Y-1.0

GroundControl/WebControl

This PR will need to match changes to GroundControl and WebControl so that they render the commands correctly. WebControl PR https://github.com/madgrizzle/WebControl/pull/137

MaslowCommunityGardenRobot commented 4 years ago

Congratulations on the pull request @gb0101010101

Now we need to decide as a community if we want to integrate these changes. You should vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up (but my vote won't count...)!

MaslowCommunityGardenRobot commented 4 years ago

It looks like adding these changes right now isn't a good idea. Consider any feedback that the community has given about why not and feel free to open a new pull request with the changes

davidelang commented 4 years ago

@gb0101010101

please resubmit these as a new pull request. Then when you get the message from the robot, go to the robot's post (which shows a thumbs up) and click the thumbs up. If there are >1 more thumbs up than there are thumbs down, it will get merged.

unfortunately, once the robot closes it, a new PR is required, it won't process this one again even if we re-open it.

we really need to increase the window to more than 2 days @BarbourSmith I thought you had increased it?

BarbourSmith commented 4 years ago

I thought we decided to go back to 48 hours after some issues with the longer time frame for some reason. I'm open to making it longer again.

These look like great changes, but I also don't have access to a stock machine right now to test so before opening this again let's get someone lined up to do hardware testing?

MaslowCNC / Firmware