Skip to content

Conversation

@gb0101010101
Copy link

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
MarlinFirmware/Marlin@c2744d8

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link
Member

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?

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.

4 participants