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

LCD menu_move: Treat rotational axes in angular degrees #24334

Conversation

DerAndere1
Copy link
Contributor

@DerAndere1 DerAndere1 commented Jun 11, 2022

Description

Without this PR, LCD move menu could not be used for rotational axes, especially in inch mode.
With this PR, the LCD move menu treats rotational axes in degrees

Requirements

ZONESTAR_LCD, I_DRIVER_TYPE ..., AXIS4_ROTATES, INCH_MODE_SUPPORT

Benefits

  • No unexpected move distances when moving rotational axes

Configurations

Marlin_PR24334_config1.zip

Related Issues

@DerAndere1 DerAndere1 changed the title MarlinUI menu_move: Treat rotational axes in angular degrees LCD menu_move: Treat rotational axes in angular degrees Jun 12, 2022
@DerAndere1
Copy link
Contributor Author

DerAndere1 commented Jun 12, 2022

EDIT: The following problem is now also fixed:

With these changes, move distances are as expected. but in inch mode (G20), rotational axes are moved slower than expected from the LCD (no problem when sending G-Code or with linear axes). Any ideas what causes this?

@thinkyhead
Copy link
Member

Note that the manual_feedrate_mm_s for a rotational axis should already actually be in degree-per-time units, not mm-per-second. The name has not been changed, but that would be the idea for a rotational axis. i.e., For a rotational axis, Degrees applies wherever Millimeters is usually assumed. If you know the radius to the end of some rotational tool, you could figure out the mm distance and use that, like we do within SCARA kinematics, but that is not formalized. It could be an addendum to feedrate modes, once those are implemented.

@thinkyhead thinkyhead force-pushed the move_rotary_axes_from_LCD branch 3 times, most recently from 3022e5d to 2e15ca2 Compare June 22, 2022 08:34
@DerAndere1
Copy link
Contributor Author

DerAndere1 commented Jun 22, 2022

With your edits, this does not work as intended: when in inch mode, the positions of linear axes shown in the move axis menu are wrong after moving the linear axis via the LCD. Also, when moving rotational axes via the LCD in inch mode, rotational axes move 25.4 times slower than in metric mode.

@DerAndere1
Copy link
Contributor Author

DerAndere1 commented Jun 22, 2022

I fixed the regression in the displayed position. To deal with feedrate for rotational axes in inch mode, I guess an alternative would be to introduce a second variable fr_deg_s in all motion related code if INCH_MODE_SUPPORT and rotational axes are enabled. IIRC, the current motion system (planner and motion.cpp) uses a single feedrate variable which stores the value that is actually only suitable for linear axes, and later, when it figured out that only rotational axes are involved, corrects for the prematurely applied inch-to-mm conversion factor. See

inverse_secs = inverse_millimeters * (cartesian_move ? fr_mm_s : LINEAR_UNIT(fr_mm_s));

@DerAndere1
Copy link
Contributor Author

DerAndere1 commented Jun 23, 2022

The problem is, that for G1/G2/G3 commands, the motion system does not know if the feedrate has to be interpreted in length units or in degrees until line

TERN_(INCH_MODE_SUPPORT, cartesian_move = false);
. So the parser has to assume the feedrate was given in length units and convert to mm/s, then pass it to the planner as fr_mm_s. If we want that fr_mm_s in planner.cpp holds the value as expressed in °/s for moves that involve only rotational axes, we would have to give the parser the ability to decide whether a given move command involves any linear axes or not.

@thinkyhead
Copy link
Member

We should be expressing feedrates in mm/s for all axes in the G-code. Then we do any required math to produce a limited feedrate for all the axes in the move - whatever makes the most sense for the combined move. That final feedrate needs to be determined at the kinematics level. That is how it applies for something like a SCARA. And in the case of a SCARA the feedrate of the rotary axes actually changes throughout the move, as required to maintain the linear feedrate.

When determining the total distance for a move that includes linear axes, plus one or more rotary axes, my suggestion would be to build an initial feedrate based on the linear motions and then apply their speed limits. Then look at how fast the rotary axes are being asked to move at that point, and further limit speed based on their maximum degrees-per-second limits.

If a rotary axis were involved in determining a final toolhead position, then we would need to build kinematics, but for setups with additional anonymous linear / rotary axes, we can only follow the moves and apply their configured limits in a direct per-axis manner.

@thinkyhead thinkyhead added T: Design Concept Technical ideas about ways and methods. C: Motion labels Jun 23, 2022
@DerAndere1
Copy link
Contributor Author

DerAndere1 commented Jun 23, 2022

If i unterstand you correctly, This PR in its original form was doing exactly what you proposed for the case of missing kinematics. With the Addition that for moves that involved no linear axis, the feedrate was Not the axis Limit feedrate but the specified feedrate, Interpreted in deg/s. That is what NIST RS274NGC/LinuxCNC in its default state (CANON_XYZ feedreference mode, „trivial kinematics“) calls for. I am in favour of any changes that achieve the same with cleaner code. And yes, i would Love to have an Multi-axis kinematics that allow Tool length compensation and an Option to Interpret feedrate as Surface Speed of the Tool Center Point Along the toolpath.

@thinkyhead
Copy link
Member

thinkyhead commented Jun 24, 2022

Yeah, I realized later that in the angular case the prior inches-to-mm conversion on input would definitely mess with things. So, that means the feedrate either needs to be un-done later, or skipped on input from the G-code when the move only involves one axis. If it can be skipped right on input, that would be best. It is handled by one particular GcodeParser function.

The trickier problem is that the G1 (and G0) feedrate is persistent (or it should be), and so it seems sensible to start implementing the different feedrate modes (inverse time, etc.), along with tool modes. We don't have to get that fancy now, but they will likely help out in future.

For now, it makes the most sense for Marlin to provide a persistent rotary feedrate (stored in degrees/s) and parameters to set/use it. At the most simple, something like G1 C F1234 – still using the F parameter, but the empty C parameter tells Marlin to save the feedrate (degrees/minute) for the next time C is used – well, whether C has a value or not, it would be stored.

It's kind of non-standard to the Marlin idiom, but it's not that major compared to how far some firmwares have pushed G-code standards.

@DerAndere1
Copy link
Contributor Author

Neat idea.

@DerAndere1 DerAndere1 changed the title LCD menu_move: Treat rotational axes in angular degrees [WIP] LCD menu_move: Treat rotational axes in angular degrees Jun 25, 2022
@DerAndere1
Copy link
Contributor Author

DerAndere1 commented Jul 2, 2022

This should wait until tombrazier's arc optimizations are merged. Let me know whether I am on the right track with today's commits, or not. So far I made the basics work (two seperate feedrates, ZONESTAR_LCD with MarlinUI, AXIS4_ROTATES, INCH_MODE_SUPPORT)

What works:

  • all manual moves from LCD
  • G1 X1 F500 ; sets linear feedrate to 500 linear units / min
  • G1 U1 F1000 ; for any linear axis U: sets linear feedrate to 1000 linear units / min
  • G1 A1 F180 ; for rotational axis A: sets angular feedrate to 180 ° / min
  • G1 C1 F360 ; for rotational axis C: sets angular feedrate to 360 ° / min
  • G1 X10; uses previous linear feedrate (here: 1000 linear units / min)
  • G1 A10; uses previous angular feedrate (here: 360 ° / min)
  • G1 A F360; for rotational axis A: sets angular feedrate.
  • G1 C F360; for rotational axis C: sets angular feedrate.

Missing:

  • compatibility of rotational axes with z-probe, bed leveling, direct stepping, kinematic printers etc.!!!

@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 2 times, most recently from 97117d0 to 5979aab Compare July 7, 2022 02:15
@thinkyhead thinkyhead force-pushed the move_rotary_axes_from_LCD branch 2 times, most recently from e5c295f to 163ab3e Compare July 10, 2022 18:55
@DerAndere1
Copy link
Contributor Author

If we consider rotational axes and inch mode niche features, I can offer to revert to the original simpler solution (https://github.com/DerAndere1/Marlin/tree/rotational_axis_fix) . It is not as complete as this PR in its current state but it requires much less changes as you can see from this diff:
bugfix-2.1.x...DerAndere1:Marlin:rotational_axis_fix

@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 2 times, most recently from 9c65146 to 4f65466 Compare January 26, 2024 00:13
@DerAndere1
Copy link
Contributor Author

I think we can close this. #26174 was merged and is a much easier solution which also works well with my 5 axis inverse kinematics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Motion S: Superseded T: Design Concept Technical ideas about ways and methods.
Projects
None yet
3 participants