-
-
Couldn't load subscription status.
- Fork 19.6k
Add G68/G51 rotate/scale workspace (LinuxCNC) #27945
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
base: bugfix-2.1.x
Are you sure you want to change the base?
Add G68/G51 rotate/scale workspace (LinuxCNC) #27945
Conversation
|
If using the LinuxCNC-compatible G10 L2 P- R- syntax for coordinate rotation is not desired, can Marlin use the Haas/Fanuc/RepRapFirmware-compatible G68 command instead of repurposing G7? |
so G7 doesn't appear to be used in Marlin, neither for G68. I suppose we could rename to G68 then? only reason for G7 was because it was close to G10 it sounds like G10 from LinuxCNC is just like G68, so that might work better |
|
Cool. Thanks. Can you briefly verify -that G68 without x and y parameters rotates around the current Position and -that the direktion if the rotation follows the conventions? I found this example for how it should work: |
|
I have some untested suggestions for improvement in this commit: You can ignore changes to G10 |
|
yes improvements for sure. in my testing with the simulator there seems to be some promises: G28 homes normally at any angle. but I think maybe that rotations should stick to +/- 90|180|270 for cartesian square beds. for Delta i suppose doesn't matter, but for consistency maybe have that can also stick to 90 deg increments? im not sure if it rotates around the current position, but it rotates around the center (BED_SIZE / 2). and that was the issue I ran into, it started to rotate around its relative position. so, say you enter and thanks I will look into your commit and try it out 👍 |
|
Workspace rotation is mainly used to simplify Gcode programming of parts with rotational symmetry. That rotational symmetry can be arbitrary (e.g. turbines with 3, 4, 5, 6,...100 blades) no restriction of angels please, that would marke behaviour once again incompatible with other controllers. Choosing angels wisely is task of the gcode programmer/CAM software/slicer |
|
Maybe the inverse_rotate_gcode_coordinates function has to be implemented as part of the get_destination_from_command function in gcode.cpp, taking into account current_position so that variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked out your code, seems to have some issues/typo
I did work on it a bit more and so far I have something working in my testing.
will need some more work as to make it so degrees are only +/- 90|180|270
I added this for square/non-Delta types
can you test it out using MarlinSimulator? I want to say it looks promising now
|
With your G68 followed by multiple G1 commands I still got erratic movement. That is probably because G1 without X or Y resulted in destination[i} = current_position[i], see function get_destination_from_command in gcode.cpp. With quite a bit of efford I got G68 working as documented in the Haas manual at https://www.haascnc.com/service/codes-settings.type=gcode.machine=mill.value=G68.html.
In addition I have the P parameter working like in LinuxCNC G10.
Have a look at the last few commits of my branch: https://github.com/DerAndere1/Marlin/tree/coordinate_rotation |
|
oh nice. yeah that was something I noticed too - G1 moving commands without X or Y results in weird behavior, I might have forgot to mention it but I did write it in the Description at the top
that is something that can probably be fixed easy |
|
It is already fixed at https://github.com/DerAndere1/Marlin/tree/coordinate_rotation. I tried to integrate individual changes from my branch in your branch. So far, correct motion was only achieved when introducing the additional |
|
Seems like this could supplant #21792 especially with the additional fixes from DerAndere1/Marlin/tree/coordinate_rotation . I will doublecheck and close that PR. |
|
Also consider |
|
G69 is already added to my branch. I removed the P parameter from G68 because it will lead to confusion later if someone wants to add the G68.2 3D rotation command, where P selects between different modes. When I am at my PC I will have a look at PR 21792 for additional goodies and prepare a clean pull request targeting classicrocker's branch |
|
PR with improvements, including G50, G51 scaling is open at classicrocker883#3 |
Marlin/src/gcode/gcode.cpp
Outdated
| if (skip_move) | ||
| if (skip_move) { | ||
| #if ANY(SCALE_WORKSPACE, ROTATE_WORKSPACE) | ||
| raw_destination[i] = current_position[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me like …
The current_position will contain a position that was already scaled and rotated. You must unrotate and unscale the current_position or set some flag that indicates that the raw_destination[i] coordinate is already rotated and scaled, and then don't rotate or scale that axis below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made flags that is set when there is rotation/scaling, not sure if thats the right approach but its there for now.
main concern is getting move commands to take a single one like G1 X100, using X+Y works fine, not sure what logic needs changing.
Marlin/src/gcode/gcode.cpp
Outdated
| #endif | ||
|
|
||
| #if ENABLED(SCALE_WORKSPACE) | ||
| if (!(NEAR(scaling_factor.x, 1.0f) || NEAR(scaling_factor.y, 1.0f) || NEAR(scaling_factor.z, 1.0f))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be: if (!(NEAR(scaling_factor.x, 1.0f) && NEAR(scaling_factor.y, 1.0f) && NEAR(scaling_factor.z, 1.0f)))?
Frankly, there will never be any case where a scaling factor is 0.99999 or 1.000001 when it was explicitly set to 1.0 by any method. So it should be absolutely fine to use if (scaling_factor.x != 1.0f || scaling_factor.y != 1.0f || scaling_factor.z != 1.0f).
| #if HAS_Z_AXIS | ||
| if (use_current_pos && parser.seen('Z')) { | ||
| if (parser.seenval('Z')) { | ||
| SERIAL_ECHO_MSG("G51: Do not use value for Z-axis scaling center with 'C' parameter!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how Marlin does these kinds of messages. Be short, concise, and considerate of tiny boards. See other G-codes for general guidance on how to format messages and keep them short. Marlin is deployed on very small boards with very limited resources, and that remains one of our primary considerations.
| */ | ||
| bool GcodeSuite::select_coordinate_system(const int8_t _new) { | ||
| if (active_coordinate_system == _new) return false; | ||
| if (TERN0(ROTATE_WORKSPACE, !NEAR_ZERO(gcode.rotation_angle))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very unlikely that the rotation_angle will be anything other than 0.0 when set explicitly to 0.0 so this can safely be !gcode.rotation_angle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surely you mean if there is rotation_angle?
if != 0 means there is rotation.
|
Pro Tip: When the rotation angle is set, calculate and cache the I leave this one to you. |
|
made some fixes according to comments, not all but some.
reworked gcode.cpp to make this work with single move coordinates |
f29ad22 to
ded2852
Compare
374a975 to
fa4ffd7
Compare
|
My text editor is trippin' today, dropping two characters here and there…. |
mischievous and deceitful, chicanerous and deplorable |
|
Requiring G51 C to use the current position as the scaling center is not my preferred solution for dealing with the situation when rotation AND scaling are active. Maybe we have to enforce that rotation center and scaling center are identical in that case. But, conventionally, if rotation is cancelled with G69 and then G51 without C is sent, Marlin should use the current position at the time of the G51 command as the default scaling center. |
Description
Supposed to be able to rotate the workspace/bed.
Preliminary code,
needs work+testingso far works well.One stipulation I can make is when using
G68for square beds rotation should be in 90 degs. like +/- 90|180|270Delta beds would not have this issue.
Another thing is when testing, you must use both X and Y in the coordinates - something like
G1 X100 Y90That is for now...
so, TODO: make it so can use either X or Y in Gcode moving commands.
See #27898
Requirements
Benefits
Configurations
Related Issues