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

Independent X & Y Accelerations #4

Merged
merged 8 commits into from
Jan 28, 2024

Conversation

Piezoid
Copy link
Contributor

@Piezoid Piezoid commented Aug 18, 2022

I'll copy the relevant parts from the Discourse thread.

I've been using different accelerations and velocities limits for X and Y on my E3Pro for the past two years. Now that the code has been tested a bit by the community, I decided to share it here.

The syntax for the axis limits are similar to the ones for Z, max_x_accel, max_x_velocity, etc.

To get all the speedup benefits, main max_accel and max_velocity should be set to the hypotenuse: √(x²+y²). There is a new gcode command SET_KINEMATICS_LIMIT which will show these values and at the diagonal angle where they are reached.

If you set accelerations per feature type in the slicer, and have tuned your acceleration to get similar resonance amplitudes and level of smoothing on X and Y, you may want to enable scale_xy_accel: true. This will scale the X and Y accelerations by the ratio M204 acceleration / config.max_accel.
For example, if you have max_accel: 10000 in your config and set in the slicer an acceleration of 5000 mm/s² (M204 S5000), effective max_x_accel and max_y_accel values will be halved compared to their original values set in the [printer] config.
Otherwise, with scale_xy_accel: false (the default) they behave as machine limits, like max_z_accel and max_z_velocity always do. Gcode velocities under the limits are never scaled per axis and stay isotropic.

The corexy version has not been tried and tested enough, but it should work. There is no velocity per axis since the same steppers are used for both X and Y. Whereas cartesian printers may have motors with different characteristics on X and Y (inductance, and therefore torque pull-out velocities). However small CoreXY machines still benefit from lower Y acceleration since it has to move the gantry which is heavier than just the toolhead. One CoreXY, the X and Y accelerations don't immediately translate into a torque load of the A and B motors, so the calculations are a bit more complicated.

klipper_estimator should support the limited_cartesian settings out of the box, with the exception of scale_xy_accel. I'm working on a patch.

Note that there are print quality concerns with anisotropic accelerations and velocities. This is the main reason that previous attempts to upstream something like this failed.
But I guess that's what this fork is about. 🙂

I open to renaming things and spelling fixes are welcome.

theotherjimmy
theotherjimmy previously approved these changes Aug 19, 2022
Copy link

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not analyze in details of the code, and everything else seems good.

Copy link
Contributor

@Fisheiyy Fisheiyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move config to dangerous-configs in a PR

@Piezoid
Copy link
Contributor Author

Piezoid commented Aug 20, 2022

Let me black it.
I think I'll remove the notebook. What do you think?

@Piezoid Piezoid force-pushed the work-peraxis_pr branch 4 times, most recently from 5ec11fc to d50f26d Compare August 20, 2022 07:45
@Piezoid Piezoid requested a review from Fisheiyy August 20, 2022 07:46
Fisheiyy
Fisheiyy previously approved these changes Aug 20, 2022
Copy link
Contributor

@Fisheiyy Fisheiyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I have knowledge of Python, but not enough to say for sure this is good, but everything else is fine.

@Piezoid
Copy link
Contributor Author

Piezoid commented Aug 20, 2022

@chestwood96
Copy link
Contributor

Note that there are print quality concerns with anisotropic accelerations and velocities. This is the main reason that previous attempts to upstream something like this failed. But I guess that's what this fork is about. 🙂

I don't see that mentioned in the docs, adding a "could cause x print quality defect in y cases" would add a lot of value imo.

We may need some consistent formatting to highlight potential problems but having the information itself is the more important part.

@Piezoid
Copy link
Contributor Author

Piezoid commented Aug 22, 2022

I don't see that mentioned in the docs, adding a "could cause x print quality defect in y cases" would add a lot of value imo.

We may need some consistent formatting to highlight potential problems but having the information itself is the more important part.

What about:
A ⚠️ icon in the title to distinguish danger specific feature, and warning admonition for these kind of important remarks.
]
In dark mode, the title icon color matches the logo, while the admonition icon remains orange:
image

This is done by adding this to mkdocs.yml:

markdown_extensions:
  - [...]
  - admonition
  - pymdownx.details
  - pymdownx.superfences
  - pymdownx.emoji:
      emoji_index: !!python/name:materialx.emoji.twemoji
      emoji_generator: !!python/name:materialx.emoji.to_svg

If you are ok with this, I'll do a separate PR with the mkdocs.yml changes and a few admonitions examples.

@chestwood96
Copy link
Contributor

Like the format but "print quality concerns" is about as unspecific as it gets (Though categorizing possible problems does sound like a good idea), would be interesting to know what problems it can cause. Without that it barely provides more information that the warning icon.

@Piezoid
Copy link
Contributor Author

Piezoid commented Aug 22, 2022

Like the format but "print quality concerns" is about as unspecific as it gets (Though categorizing possible problems does sound like a good idea), would be interesting to know what problems it can cause. Without that it barely provides more information that the warning icon.

On the "print quality" issues for this specific mod, I haven't personally observed any. That's why I left the warning vague and ask the user to pay attention. Kevin's concerns were about extrusions unevenness, where having different acceleration for each line orientation might cause the extrusion system and PA to behave differently. I might add that along with a link to some PR/issue where this explanation is given.

@Piezoid Piezoid marked this pull request as draft August 23, 2022 15:01
@Fisheiyy Fisheiyy changed the title Limit accelerations and velocities for X and Y axis on Cartesian and CoreXY printers. Independent Accelerations Aug 29, 2022
@Fisheiyy Fisheiyy changed the base branch from master to independent-accelerations August 29, 2022 23:11
@Fisheiyy Fisheiyy changed the title Independent Accelerations Independent X & Y Accelerations Aug 30, 2022
@Piezoid Piezoid force-pushed the work-peraxis_pr branch 3 times, most recently from 004a105 to de62891 Compare September 13, 2022 16:56
@Piezoid Piezoid changed the base branch from independent-accelerations to master September 13, 2022 17:01
@Fisheiyy Fisheiyy dismissed stale reviews from theotherjimmy and themself via f909ca2 January 21, 2023 07:17
@Nathan22211
Copy link

I've been using the limited coreXY on at least 3 machines. a flying bear reborn 2, a voron trident 350, and a zonestar z9v5. I haven't noticed anything unusual from them that I've narrowed down to software (most of the issues is either elsewhere in my config or hardware related), and that's just from adding the kinematic files to the respective folder.

@rogerlz rogerlz marked this pull request as ready for review October 18, 2023 22:37
@rogerlz
Copy link
Contributor

rogerlz commented Oct 18, 2023

looks good to me and we seem to have good feedback.. I'd love to add regression tests just in case, but I can do that after merging the PR

rogerlz
rogerlz previously approved these changes Oct 18, 2023
@rogerlz rogerlz requested a review from bwnance October 18, 2023 22:45
@Piezoid
Copy link
Contributor Author

Piezoid commented Oct 18, 2023

In it's current state the new limits don't play well with SHAPER_CALIBRATE. The code planning the vibration motion is not expecting the acceleration limits which cap the max frequency.

One approach I tried is using a context manager to disable all limits for the duration of the test.

@rogerlz
Copy link
Contributor

rogerlz commented Oct 18, 2023

@Piezoid should we simply add that to the documentation?

@@ -52,6 +53,73 @@ def _parse_axis(gcmd, raw_axis):
return TestAxis(vib_dir=(dir_x, dir_y))


@contextmanager
def suspend_limits(printer, max_accel, max_velocity, input_shaping):
# Override maximum acceleration and acceleration to
Copy link
Contributor Author

@Piezoid Piezoid Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not found of this code with private member access. Ideally toolhead.py should provide a save/restore API similar to gcode state save/restore.

So yes, a simple warning in the doc can be an alternative until we figure something out.

@Kizime123
Copy link

Kizime123 commented Oct 27, 2023

Has anyone brought this fork of this project up here before? https://blog.kdb424.xyz/peraxis/ I know @kdb424 has run the script a lot and so have I. I am linking it because of the SS/ orca/ prusa plugin. As I've read the above thread and know, IS needs an update.

@ShaneDelmore
Copy link

Another user feedback anecdote, I have been testing limited cartesian on my bedslinger for the past month printing daily and it has been flawless for me. considerably improved printing speed with no quality loss or other strange behavior that I have seen. I do disable it when doing input shaper testing but that is due to a warning that there may be an incompatibility, I have not tried it to verify whether there are issues or not.

Piezoid and others added 5 commits January 28, 2024 14:37
Adds velocity and acceleration limits on X and Y for these kinematics.
With limited_ kinematics the acceleration may change between moves.
In order to preserve behavior junction_deviation should be recomputed
each time. Since the  `jd * accel` term is constant for a given scv, it
can replace junction_deviation when computing cornering
limits.
This term is the squared toolhead velocity during a 60° cornering.
Computes the max XY reachable acceleration for the scale_xy_accel
denominator, instead of relying on config [printer] max_accel.
(`hypot(x, y) ` for Cartesian, `max(X,Y)` for CoreXY)

Removes config limit in SET_KINEMATICS_LIMIT for easier tuning.
rogerlz
rogerlz previously approved these changes Jan 28, 2024
@Piezoid Piezoid merged commit 1d8ae0b into DangerKlippers:master Jan 28, 2024
2 checks passed
rogerlz pushed a commit that referenced this pull request May 28, 2024
* Add files via upload

* Add files via upload

* Update MPC.md

Second updated

* Update MPC.md

Edits and spelling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants