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

G68-G69 Coordinate transform G-codes #21792

Draft
wants to merge 12 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

deirdreobyrne
Copy link
Contributor

@deirdreobyrne deirdreobyrne commented May 3, 2021

Description

(Forgive me if this is not the right forum. I am aware of the "feature request" facility, but the work done by me on this so far - including quite a bit of code from a previous attempt at this problem - makes it feel like more than just a feature request).

There are a number of g-codes dealing with coordinate transformation which are not implemented (or only partially implemented) in Marlin and which are useful (particularly on milling machines). Implementing these g-codes will, I believe, necessitate a refactoring of the Marlin code which deals with coordinates. I wish to present this proposal to the Marlin community before getting into the weeds in the code.

I would really appreciate feedback before getting stuck in, as this is my second attempt at tackling this problem. Also, I do not wish to start what is likely to be a big project without checking to see if there is any relevant work going on that I'm not aware of.

Update 5 May 2021 - I've added a few header files indicating how the refactoring could be implemented.

Proposed refactoring

I propose creating a new class which will implement three coordinate systems and the transforms between them. These systems are

  • Mechanical coordinate system
    This is the coordinate system defined by the mechanical setup of the machine. For cartesian machines the mechanical coordinate system is defined by the endstops, and by the orientation of the three axes involved in moving the machine head (i.e. the mechanical coordinate system is simply the number of steps the steppers have moved since homing divided by steps-per-millimeter). For SCARA and Delta machines, the mechanical coordinate system is the system defined by the endstops, the number of steps the steppers have moved, and the trigonometrical transforms required to transform to a near-orthogonal cartesian coordinate system.

  • Machine coordinate system
    The machine coordinate system has an origin at the front-left of a rectangular or square bed, or at the centre of a circular bed. It is achieved by applying a linear transformation and an offset to the mechanical coordinate system. This corrects for homing switch positions, and for skew in the mechanical setup of the machine.

  • G-Code coordinate system
    This is the coordinate system defined by the coordinate transformation gcodes which have currently been executed. All these transformations are various combinations of linear transformations and offsets, hence the overall gcode-to-mechanical and gcode-to-machine transforms can be calculated and cached whenever coordinate transformation gcodes are encountered.

Whenever a coordinate transforming gcode is encountered, methods in this class are called to update the various linear transforms and offsets.

Skew corrections

Marlin currently has skew corrections implemented, which correct for the fact that the machine's mechanical axes may not be orthogonal. However the mathematics of the implementation introduces scaling on the axes. And, the current method of determining the skew correction requires measuring the lengths of the edges of a 3D print. What is far easier is to measure the perpendicular distance between faces. A mathematical and procedural analysis is pending.

Finally, whereas a 3D printer can move the head in all three axes to correct for skew, on a milling machine it can be very important to not move in X or Y when moving in Z, as to do so could introduce unacceptable stress on the tool in use. I present a mathematical analysis in the docs/Skew Mathematics.odt file.

GCode

There is an ISO international standard covering GCode, however manufacturers of CNC machines seem to see that standard as a set of suggestions.

This is important because the order in which the gcode transformations are applied makes a big difference. cnccookbook.com says the order is

  • Convert to metric
  • Convert from relative to absolute (G90, G91) and polar to cartesian (G15 and G16)
  • G52, G54 and G92 offsets
  • G51 scaling
  • G68 coordinate rotation

There are a number of coordinate transform gcodes which are not in that list. We can turn to cnccookbook.com for a tabulated list.

G68 rotation

There was a previous attempt to implement G68 rotation by the author, which was abandoned when I got stuck in the weeds of both the code and the meanings of various gcodes. G68 is of particular interest to me as it is needed in double-sided PCB CNC milling.

It would appear that the intent of G68 is to provide absolute rotation in the XY plane only. However, when G18 or G19 are executed, the gcode XY plane becomes the machine XZ or YZ plane, hence a form of rotation in those other planes can be achieved. At least that is what appears to be the intent.

Note that the mathematics of rotation are such that it is quite easy to not only provide relative rotation, but also rotation around all three axes at the same time, though "just because you can do something doesn't mean you should" may apply to the latter.

These are the kinds of questions and issues that need addressing before implementing new gcodes.

Next steps

  • Get feedback on the thinking so far.
  • Implement the class as describe above, and re-implement the coordinate transforming gcodes that are currently implemented.
  • Test.
  • Implement G68.
  • Test.
  • Develop the mathematics of skew correction when measuring the perpendicular distance between faces of a 3D print (as opposed to edge lengths).
  • Go through all the coordinate transforming gcodes and decide where in the coordinate transform pipeline they should appear.
  • Implement those other coordinate transformation gcodes.
  • Test

@fedetony
Copy link
Contributor

fedetony commented May 4, 2021

This sounds great. I've been trying to conceptualize a similar concept. Yet my approach was to use an external program to convert the gcode into modified gcode as for example mirrored, rotated or scaled. At the moment my translation function has only intermachine conversion as grbl format gcode to marlin readable gcode. Is not target to a fixed interface, it is fully programmable to multiple interfaces and you can add more, yet I've got functional in my DB only grbl 0.9, grbl 1.1, Marlin 2.0.x and TinyG v1.0 since are machines I can access to debug the program. My program is still under construction, but all conversions were already planned.
Here a screenshot of a run.
2021-05-04 10_49_04-Windows PowerShell

@thisiskeithb thisiskeithb added Needs: Discussion Discussion is needed T: Design Concept Technical ideas about ways and methods. labels May 4, 2021
@deirdreobyrne
Copy link
Contributor Author

I've added a few header files indicating how the refactoring could be done. The idea is to make it relatively easy to implement new gcodes, and also easy to perform transformations between the various coordinate systems.

@thinkyhead
Copy link
Member

thinkyhead commented May 6, 2021

Rotation and translation can be represented as simple 3x3 matrix, with every coordinate being run through the single transform. Pre-creating matrixes that combine transforms makes it easier to place rotations and translation into arbitrary order, and matrices are well tailored to graphics processors and processors with parallel math units.

We have optimized kinematics for Delta, SCARA, and CoreXY, implemented at the lowest levels possible for each, and taking into account the requirement to break up lines into small segments, and so on. Perhaps some aspects of kinematics can be generalized, but we must take care not to balloon the code size in the process. We have a delta.cpp and a scara.cpp which contain the utilities needed for those kinematics. They have some common elements. CoreXY is not a complex kinematic, so it interfaces at the lowest level where the planner is giving counts to the steppers.

At the planner level we have Skew compensation, Backlash compensation, and Z adjustment by the leveling system. These on-the-fly adjustments are applied in Planner::apply_modifiers. Note that the UBL system is not included in Planner::apply_leveling, but provides higher level motion functions (for more optimal segmentation) instead. UBL's Z adjustment code will probably be moved into apply_leveling pretty soon because it can get out of sync if not 'tickled' after a coordinate change, and this sometimes bites us.

Since we already have CNC_WORKSPACE_PLANES and CNC_COORDINATE_SYSTEMS pretty well covered, we don't need any additional new translation layer. The only thing that might need filling-in is rotation. Assuming we pre-calculate our sine and cosine for a single rotation of the workspace around some axis, rotation of G-code on-the-fly will incur, per-coordinate (not per-segment):

  • two more float subtractions
  • two more float multiplications, and
  • two more float additions.

Faster processors and those with an FPU will have no difficulty to handle the extra maths, but it is very much preferred that the slicer or the controller software should take care of rotating objects ahead of time so that the printer electronics can run a little cooler.

@deirdreobyrne
Copy link
Contributor Author

On a practical level, there are programs out there which perform the necessary rotation for use in PCB milling. Personally I use bCNC, as it is the only one which combines measuring the rotation with correcting for it. However bCNC only works with grbl, and I'd like to have access to the range of features Marlin provides, especially since Marlin seems to work smoother (and quieter!) with my cheap CNC. I've also looked at the grbl skew code (a feature I need) and I'm not convinced by it.

Part of this boils down to - what is Marlin trying to be? Because if it is trying to just be for 3D printers, then it's hard to see how rotation could ever be used. Or does it also want to cover milling machines? Because I've seen feature requests for G50 and G51 scaling, and for G68 and G69 rotation, and no doubt those requests are coming from milling machine users. Whereas all those transformations could be implemented one-at-a-time, as is done currently, it occurs to me that it is more efficient to combine them into a set of aggregated transformations. Doing so reduces a number of mathematical operations to (a maximum of) 9 multiplications and 3 additions.

Having said that, I've previously attempted to implement just G68 and G69 rotation (PR #21199), and I ended up just tripping over myself. With what I've learned since I could probably get it to work correctly. Part of the reason I find myself reluctant to do so is because of a contributing factor in how I tripped over myself - I found the current code quite difficult to navigate.

I am having difficulty understanding your last point. A straight line in a rotated, skewed and offset reference frame is still a straight line - it is only the endpoint(s) that need to be transformed - the points in-between still follow a straight line. Granted my proposal would generate considerably more mathematical operations for planning G2 and G3, but doing circular motion requires quite a bit of math anyway. It would also require more maths to generate the X, Y, Z output for the display, but I imagine the display is run in an asynchronous manner? Am I missing something?

Finally I forgot about bed levelling! Again I imagine that it could be considered "external" to the transforms.

@thinkyhead
Copy link
Member

thinkyhead commented May 9, 2021

what is Marlin trying to be?

Well, on one hand, Marlin is trying to be a stable and optimized motion platform and a decent G-code parser, and to keep up with all the electronics. Over the last couple of years we've been busy covering the explosion of new 3D printer boards, peripherals, and fancy multi-head parking extruder gadgets, and now I really want to work on user experience, configuration tools, and configuration documentation next.

On the other hand, as busy as I am with PRs every day, I have the aspiration to fill in all the missing pieces. Marlin has supported Spindle and Laser tools for a while now, so as a completist I would like to see the full suite of CNC G-codes implemented and call it a day, but these things are never so easy. I've made incomplete starts on Feedrate Modes, Spindle Radius, Inner / Outer Paths, and some other things. And there's also a PR hanging around for Drilling Cycles.

it is only the endpoint(s) that need to be transformed

Yes, for example we do workspace translation on a per-coordinate basis, and then we just segment the resulting line with no extra overhead.

So for the simplest implementation of a workspace rotation we'd start by defining the center-point, sine, and cosine for the rotation, then we'd just do the transform in the same place that we're currently doing the workspace translation, which is in GcodeSuite::get_destination_from_command(). That would cover all G0/G1. Looking at G2 / G3, it seems that for each arc we only need to rotate two points, so the amount of added maths is pretty modest, and that can be done within plan_arc.

That is doable, but it would just be a first prototype to play around with. For more complete workspace rotation many internal motion functions will also need to consider the rotated space. So it would be a good idea as a first step to go through all the motion handling in Marlin and do a thorough audit, making sure it is clear which motion handling would be using pre-rotated coordinates, which code would need to add a coordinate rotation step, and which code could continue to use native coordinates.

The motion functions aren't named terrifically well, nor organized beautifully, and it is long overdue to wrap motion.cpp in a singleton class, so any progress in the direction of a motion refactor and clarification is good, even if it's just meticulously documenting everything in a flow chart….

@fedetony
Copy link
Contributor

Just a note here, there is already implemented a vector rotation in Marlin/Marlin/src/libs/vector_3.h and Marlin/Marlin/src/libs/vector_3.cpp It just needs to be complemented. But at least half of the math is there, since normalize and cross product of 3D vectors are there.
The best way is to use a 4D matrix. This is how 3D games FPS transform the 3D vectors.
I have an implementation I made in matlab for that, needs to be translated to c++. I'll take a look into it...

@fedetony
Copy link
Contributor

fedetony commented May 11, 2021

This should help a lot for G68 commands.

vector3.zip

So here I modified the vector_3.cpp and vector_3.h files in the ...src/libs folder to accommodate general rotation function for any xyz vector around any arbitrary axis. The function returns the rotated coordinate as a xyz_float_t type.

The function:

rotatate_around_axis_at_axispos(const float &angle_rad,const vector_3 &axis, vector_3 &axispos, const vector_3 &point)

…where…

  • angle_rad is the angle in radians to be rotated, following right-hand rule,where the thumb points in axis direction
  • axis is the directional vector of any axis to be rotated not necessarily unitary
  • axispos is the coordinate x,y,z around which you want to rotate.
  • point is the coordinate x,y,z you want to rotate around axispos

Example:

vector_3 rot=vector_3::rotatate_around_axis_at_axispos(-90/180*3.1415,vector_3(0,0,1), vector_3(5,0,0), vector_3(20,0,0)

should rotate (20,0,0) around a Z axis (0,0,1) at (5,0,0) 90 Degrees clockwise i.e return (5,-15,0)
Enjoy 🗡️

PS: Just noticed I called it rotatate instead of rotate :D 🤣
Let me know your thoughts 👍

@thinkyhead
Copy link
Member

The best way is to use a 4D matrix

The math to rotate a point around a center by some angle is not complicated, and it doesn't matter if the sine, cosine, and center-point coordinates for the rotation are stored in a matrix. A matrix is a nice digestible packet that parallelizes well for pipelines that need to combine matrixes and then transform giant batches of points stored in a RAM buffer, but these embedded devices are particularly weak, our calculations are distributed, and we will not be combining a workspace translate-and-rotate matrix with an object translate-and-rotate matrix, so we don't need to package the data in any special way for pipelining or parallelization. It is also likely that generalization to a 4-matrix is only going to add overhead.

Anyway, I really find long discussions bothersome when I can already see the code in my head, and I know it's going to end up being smaller than this paragraph, and it will probably take less time to write than it took for me to try to make this post tactful. I see it again and again: Discussion goes on endlessly for weeks, no one writes a single line of code, and finally it is me who is forced to actually write the code to put an end to the discussion and reclaim my valuable time.

I would much prefer to see the code written first, and then there will be something in existence that we can workshop.

@deirdreobyrne
Copy link
Contributor Author

I too can see the code in my head, though different code to thinkyhead. I don't see the point in the implementation of each gcode trying very hard to produce a set of machine coordinates for the planner to work with when each gcode implementation can produce a set of gcode coordinates, and those coordinates are then translated by the planner as soon as it receives them.

Also I'd love to examine the evidence that modern microcontrollers are struggling, or would struggle, with any of this.

I've been very enthusiastic about the possibility of Marlin satisfying all my needs, and I've found my enthusiasm deflated by the raising of issues that I don't think are issues. I don't see the need for an audit, for instance, if the planner does the translation rather than each gcode implementation.

As for code - I already put quite a bit of work into PR #21199 and my failure to get it right is undoubtedly contributing to my deflated enthusiasm. On the one hand I can see the code in my head, but on the other it is apparently quite different code to thinkyhead's, and I of course need to recognise his considerably greater experience. If he says an audit is needed then I find myself losing confidence in the ideas I have in my head.

So I'm not sure what is the best way to proceed, as the point that Marlin needs to be stable is well taken.

@fedetony
Copy link
Contributor

@deirdreobyrne and @thinkyhead .. In my last reply there is a zip file with all the code implemented for rotation. Please look at it..

@MFlego
Copy link

MFlego commented May 12, 2021

I have the aspiration to fill in all the missing pieces. Marlin has supported Spindle and Laser tools for a while now, so as a completist I would like to see the full suite of CNC G-codes implemented and call it a day, but these things are never so easy. I've made incomplete starts on Feedrate Modes, Spindle Radius, Inner / Outer Paths, and some other things. And there's also a PR hanging around for Drilling Cycles.

@thinkyhead, Like Deirdreobryne I do a fair amount of PCB milling and I make cnc machines as a hobby / professionally. I do see that Marlin is currently the leading complete firmware solution as it runs on such a variety of micro and is very feature complete. I want to throw my hat in and just say that coordinate setting and rotation via probing is the key to modern precision and automation. The G38 series support in Marlin has been a blessing, but ultimately stops a few feet short of real utility without being able to rotate and also automatically set those coordinates into a user space offset.

Not trying to waste anyone's time, I just want to express support for this feature.

@thinkyhead
Copy link
Member

Not trying to waste anyone's time, I just want to express support for this feature.

Thanks for echoing my wish to have a more complete suite of G-codes.
Together we can end the world's hunger for CNC G-codes in Marlin.

@thinkyhead
Copy link
Member

I've found my enthusiasm deflated by the raising of issues that I don't think are issues. I don't see the need for an audit, for instance, if the planner does the translation rather than each gcode implementation.

I apologize for being terse, but I'm trying to give fair warning.

The motion code is still not as mature as it needs to be. It is brittle and confusing and comes with side-effects and special cases, so my worry is that any new element in the motion system will be more difficult to do right if we don't first segregate the code which needs to operate in the native space from the code that wants to work in the rotated / shifted / leveled / skewed / backlash-corrected workspace before we pile on code to apply rotation at the planner level.

This is especially true of third-party UI code and internal motion routines. There will be code that wants to call "plan_rotated_move" and code that wants to call "plan_regular_move" and we don't know exactly which is which yet. Some code at the UI level, and also sent by LCD controllers and hosts, does all moves by sending G-code. Some of these moves may be inappropriate to rotate.

Also, I have a longstanding hope and plan to fix up UBL motion and to do motion.cpp refactoring, and I worry that introducing more complexity now will make that work harder. I would very much like to see the code improved to a point where it feels more manageable and less spaghetti-like as a first step.

As much as I sense that introducing rotation will make things tricky at various levels, of course I cannot back that up until I sit down and actually write the code and run into issues. The puzzle is intriguing, and I'm sure that I would enjoy attacking the problem. It is just not something I can put at the top of my priority list right now, so I encourage y'all to keep working on it until we have a full working model.

I have a long list of TODOs ahead of this particular new feature, but I will review your ZIP soon, and certainly before I would add any rotation code to Marlin — assuming no one beats me to it with a PR.

@deirdreobyrne
Copy link
Contributor Author

thinkyhead - thanks for the fair warning. I'm particularly struck by your assertion that you can see code in your head which, by the sounds of it, is far smaller than the code I'm thinking of (and therefore more stable) and which by the sounds of it doesn't require a refactor to the extent I'm seeing. Every time I thought about solving this problem I simply couldn't see a way of doing it that didn't either require a major refactor, or end up being spaghetti upon spaghetti - unless restrictions were introduced around which combinations of coordinate transforming gcodes could be active at any one time.

So what I'm taking away is that this problem is either far smaller than I think and can see, or it is maybe a bit bigger, requiring a refactor in other places in the code than what I can see.

I'm also taking away that this problem, whether it be large or small, is not high on your priority list. And I'm taking away that the word refactor is certainly not a dirty word when it comes to solving this problem.

My concern is that whereas I could probably beat you to a PR, whatever idea(s) you have floating around in your head as to the solution is far superior to mine (which sounds like it is the case) and hence my PR would be a disservice to Marlin. The last thing this problem needs is another sub-par solution which ends up being carried forward in Marlin until another programmer sees the need for another, more difficult, refactor.

@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 3 times, most recently from 1eaff6a to aee971b Compare May 22, 2021 22:47
@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 2 times, most recently from 9852ef9 to 0e40d47 Compare June 6, 2021 08:58
@thinkyhead thinkyhead marked this pull request as draft July 21, 2021 05:44
@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 2 times, most recently from abffbbe to 22ae09a Compare August 7, 2021 23:26
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 3 times, most recently from de391db to 0f34163 Compare April 12, 2023 05:14
@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 3 times, most recently from c14588b to a39a5d4 Compare June 2, 2023 20:05
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 2 times, most recently from 9c65146 to 4f65466 Compare January 26, 2024 00:13
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 3 times, most recently from 37d77d6 to aa44542 Compare September 28, 2024 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Discussion Discussion is needed T: Design Concept Technical ideas about ways and methods.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants