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

hybrid_corexy: Fix for inverted hybrid core-xy setups and extended usability #6351

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

HelgeKeck
Copy link

@HelgeKeck HelgeKeck commented Sep 24, 2023

This is a working fix for the inverted hybrid core-xy issue

The setup of some markforged printers are inverted. If this is the case the user needs to swap the dual_carriage onto the left side instead of the right side, or flipping the gantry arround. This results in several problems like the need to use set kinematic position instead of being able to use set g-code offset for the copy and mirror mode. It results also in wrong bed mesh coordinates for copy and mirror mode.

this PR lets you set a inverted parameter for the dual_carriage object. If set to true it inverts the needed calculations and lets you setup the printer like it is supposed to be.

Signed-off-by: Helge Magnus Keck HelgeKeck@hotmail.com

@KevinOConnor
Copy link
Collaborator

Thanks. I don't know enough about these printers to review this PR. Maybe @dmbutyugin can do a review. It does seem a little odd that an option is needed to reverse the direction as typically a corexy printer can invert the direction by swapping the definition of the corexy motors.

-Kevin

@HelgeKeck
Copy link
Author

yes, in case of the hybrid corexy kinematics as it is atm, if you would just swap the steppers or directions then X and DC carriage move in the opposite direction when moving in Y, its a known issue

@HelgeKeck
Copy link
Author

@HelgeKeck
Copy link
Author

@HelgeKeck
Copy link
Author

HelgeKeck commented Sep 27, 2023

so this pr fixes also the issue with the wrong x coordinates in case you probe or z-home with the dual carriage toolhead

@HelgeKeck
Copy link
Author

jsut to explain what happend here, pos[0] is always the x carriage, if you probed with the dual_carriage it still used this coordinate. Since we dont know how many steppers are involved, we dont know the index of the dual carriage item
image

@HelgeKeck
Copy link
Author

HelgeKeck commented Sep 27, 2023

just fyi, it also fixes this, both are probed with the probe one the dual carriage toolhead, one time with this fix another time without
image
image

@dmbutyugin
Copy link
Collaborator

Thanks, I'll take a closer look a bit later, maybe in a week or so. Just a quick question though, stepper_positions['dual_carriage'] fix, is it also useful on the mainline? Does it fix probing with the dual carriage in a non-inverted kinematics case?

@HelgeKeck
Copy link
Author

its definitely useful on the mainline. atm you can only probe with the x carriage since it always sets the x position to the x carriage x

@HelgeKeck
Copy link
Author

just noticed something, it seems that stepper_positions['dual_carriage'] is always the last entry. i have also seen you are reversinng the order of the rails in one place. Can/should we reuse this maybe for this situation?

@github-actions
Copy link

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

  1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
    If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
  2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
  3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

Copy link

github-actions bot commented Nov 2, 2023

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@github-actions github-actions bot closed this Nov 2, 2023
@Sineos Sineos reopened this Nov 2, 2023
@HelgeKeck
Copy link
Author

HelgeKeck commented May 22, 2024

Then, this PR makes rather specific choices as to which combinations of stepper kinematics to support. I.e. it has to be either stepper_x: x+y, stepper_x1: x-y, dual_carriage: x-y, dual_carriage: x+y or stepper_x: x-y, stepper_x1: x+y, dual_carriage: x+y, dual_carriage: x-y. However, are other combinations theoretically possible (from a printer design perspective)? E.g.

possible configs

Single Toolhead A

stepper_x
stepper_y
stepper_y1

Single Toolhead AB

stepper_x
stepper_x1
stepper_y
stepper_y1

IDEX

stepper_x
dual_carriage
stepper_y
stepper_y1

IDEX AWD

stepper_x
stepper_x1
dual_carriage
dual_carriage1
stepper_y
stepper_y1
stepper_y2
stepper_y3

technically there are even more combinations possible, so single toolhead could be done in AWD mode as well, but im sure you get the idea already

@HelgeKeck
Copy link
Author

HelgeKeck commented May 22, 2024

also, i dont have a strong opinion on how to implement the inverted feature, it just should work. But keep in mind its not only the kinematic calculation that needs to be changed from + to -, there is for example also the calc_position function that needs to be aware off.

@dmbutyugin
Copy link
Collaborator

possible configs

Yes, I saw your comment regarding this from before. However, I'm particularly curious how the steppers would work in these cases

Single Toolhead AB

stepper_x
stepper_x1
stepper_y
stepper_y1

Do stepper_x and stepper_x1 have to have different kinematics, or can they both have '+' or '-' kinematics?

IDEX AWD

stepper_x
stepper_x1
dual_carriage
dual_carriage1
stepper_y
stepper_y1
stepper_y2
stepper_y3

Here the same, do stepper_x1 and dual_carriage have to have opposite kinematics to their primary steppers? Or can stepper_x1 have the same kinematics as stepper_x and dual_carriage1 the same as dual_carriage?

@HelgeKeck
Copy link
Author

btw, this is the updated kinematic file that works with the recent klipper changes as well
https://github.com/HelgeKeck/klipper_hybrid_corexy/blob/main/klippy/kinematics/ratos_hybrid_corexy.py

@HelgeKeck
Copy link
Author

see stepper_x and stepper_x1 for example, as one stepper, they use the same kinematic calculation. its jsut a supporting stepper motor. This is like a classic AWD setup

@HelgeKeck
Copy link
Author

its jsut like putting 2 stepper in series for the same belt

@dmbutyugin
Copy link
Collaborator

see stepper_x and stepper_x1 for example, as one stepper, they use the same kinematic calculation. its jsut a supporting stepper motor. This is like a classic AWD setup

The reason I was trying to clarify this is because your code has the following snippet:

            self.rails[0].steppers[0].setup_itersolve('corexy_stepper_alloc', b'-')
            if len(self.rails[0].steppers)==2:
                self.rails[0].steppers[1].setup_itersolve('corexy_stepper_alloc', b'+')

which unconditionally assigns an inverted kinematics for stepper_x1.

@HelgeKeck
Copy link
Author

see stepper_x and stepper_x1 for example, as one stepper, they use the same kinematic calculation. its jsut a supporting stepper motor. This is like a classic AWD setup

The reason I was trying to clarify this is because your code has the following snippet:

            self.rails[0].steppers[0].setup_itersolve('corexy_stepper_alloc', b'-')
            if len(self.rails[0].steppers)==2:
                self.rails[0].steppers[1].setup_itersolve('corexy_stepper_alloc', b'+')

which unconditionally assigns an inverted kinematics for stepper_x1.

im sorry, i misunderstood your question and forgot a important detail
so what i said counts for IDEX AWD

however, the Hybrid CoreXY Single Toolhead configuraiton is different, you are right. In this case Stepper X and X1 needs to work like X and DC. i agree the stepper naming could be changed here

@HelgeKeck
Copy link
Author

so this here is what we call a Hybrid CoreXY for single toolheads, in thsi case X and X1 do work like X and DC for the idex

Single Toolhead AB


stepper_x
stepper_x1
stepper_y
stepper_y1

@HelgeKeck
Copy link
Author

so yes, correct stepper naming is key to avoid any future confusion

@HelgeKeck
Copy link
Author

im very sorry, my apologies. long time since i worked with that code. X and X1 do work exactyl like CoreXY, and Y and Y1 are the addtional y betls. sorry for the confusion

Single Toolhead AB

stepper_x
stepper_x1
stepper_y
stepper_y1

@dmbutyugin
Copy link
Collaborator

OK, thanks. Since it seems that for hybrid_corexy (and z) it is important to support kinematics type ('+' or '-') at stepper level, then you can configure any possible printer. So, if we can agree on the proposal

[stepper_x]
kinematics: '+'
step_pin: ...
... # regular config

[stepper_x1]
kinematics: '-'
.... # regular config

[dual_carriage]
kinematics: '-'
.... # regular config

[dual_carriage1]
kinematics: '-'
....  # regular config

[stepper_y]
# regular config
[stepper_y1]
# regular config

[printer]
kinematics: hybrid_corexy

I can put together a PR in the next few days. What is your opinion on this proposal? And @KevinOConnor, do you have any feedback on this matter? Would you accept a PR with such configuration options for steppers? It is something we never had before, but this could be the right direction of development. Alternatively, [printer] section could have corresponding option(s), but I find this just splits what is actually a stepper-level configuration between several sections.

@HelgeKeck
Copy link
Author

OK, thanks. Since it seems that for hybrid_corexy (and z) it is important to support kinematics type ('+' or '-') at stepper level, then you can configure any possible printer. So, if we can agree on the proposal

i personaly would agree, assuming existing non inverted idex installation do not need to change their configs to fit this pattern.

but im fine with it.

@dmbutyugin
Copy link
Collaborator

OK, thanks. Since it seems that for hybrid_corexy (and z) it is important to support kinematics type ('+' or '-') at stepper level, then you can configure any possible printer. So, if we can agree on the proposal

i personaly would agree, assuming existing non inverted idex installation do not need to change their configs to fit this pattern.

but im fine with it.

Yes, the defaults would be '-' for stepper_x and '+' for dual_carriage.

@KevinOConnor
Copy link
Collaborator

And @KevinOConnor, do you have any feedback on this matter?

It definitely sounds like something we should support. I'm not sure I follow the requirements enough to give advice though.

It sounds like the goal is to add an additional stepper that follows the kinematics? If so, the config syntax looks a little confusing to me - how would one choose between a stepper_x1 with - instead of a stepper_y1 with -?

If it's just an additional stepper could that be more explicit - something like:

# An extra stepper motor that follows the main toolhead
[kinematic_stepper my_extra_kinematic_stepper]
kinematics: 'x+y'
....

I think I'm not understanding something.

-Kevin

@miklschmidt
Copy link
Contributor

miklschmidt commented May 22, 2024

OK, thanks. Since it seems that for hybrid_corexy (and z) it is important to support kinematics type ('+' or '-') at stepper level, then you can configure any possible printer. So, if we can agree on the proposal

[stepper_x]
kinematics: '+'
step_pin: ...
... # regular config

[stepper_x1]
kinematics: '-'
.... # regular config

[dual_carriage]
kinematics: '-'
.... # regular config

[dual_carriage1]
kinematics: '-'
....  # regular config

[stepper_y]
# regular config
[stepper_y1]
# regular config

[printer]
kinematics: hybrid_corexy

I can put together a PR in the next few days. What is your opinion on this proposal? And @KevinOConnor, do you have any feedback on this matter? Would you accept a PR with such configuration options for steppers? It is something we never had before, but this could be the right direction of development. Alternatively, [printer] section could have corresponding option(s), but I find this just splits what is actually a stepper-level configuration between several sections.

I like the explicitness of this option, and i can see it being useful in other kinematics implementation in the future. My worry is it's not particularly user friendly. Not that it's an issue for us as we generate the configs anyway, but it sort of forces regular users to understand kinematics, which it didn't before. If it's an optional feature—meaning its exclusion would retain the old defaults from the mainline hybrid_corexy and z—it seems to be a sound, explicit, and flexible approach.

Alternatively, [printer] section could have corresponding option(s), but I find this just splits what is actually a stepper-level configuration between several sections.

Forced co-location is one of the better properties of the klipper configuration DSL imo, so i would definitely keep it on the [stepper] section.

I have some alternative ideas. Something that's come up a few times in the past is that using x and y for corexy kinematics is a bit of a misnomer, unfortunately i do not remember the reason for why they're mapped as x and y. If we rename them to A and B, it should be easier to reason about and easier to map programatically (you wouldn't need special casing for X anymore). Then it would go something like this:

Hybrid corexy single toolhead A

[stepper_a]
...
[stepper_y]
...
[stepper_y1]
...

Hybrid corexy single toolhead B (this would effectively be "inverted") if used on the same belt path as the previous

[stepper_b]
...
[stepper_y]
...
[stepper_y1]
...

Hybrid corexy single toolhead AB

[stepper_a]
...
[stepper_b]
...
[stepper_y]
...
[stepper_y1]
...

Hybrid corexy single toolhead AB inverted
Same as above, just swap A / B motor connections.

Hybrid corexy IDEX

[stepper_a]
...
[dual_carriage]
...
[stepper_y]
...
[stepper_y1]
...

Hybrid corexy IDEX inverted

[stepper_b]
...
[dual_carriage]
...
[stepper_y]
...
[stepper_y1]
...

The last case (hybrid corexy IDEX inverted) would need special handling though, as the kinematics for b and dual_carriage would otherwise be the same. But i think this is more down to how IDEX support in general is currently implemented by swapping the "x" or "index 0" rail through use of the DualCarriages class instead of a more generic multi-toolhead strategy.

Bonus is all of these would allow easy multirail mapping.

@miklschmidt
Copy link
Contributor

And @KevinOConnor, do you have any feedback on this matter?

It definitely sounds like something we should support. I'm not sure I follow the requirements enough to give advice though.

It sounds like the goal is to add an additional stepper that follows the kinematics? If so, the config syntax looks a little confusing to me - how would one choose between a stepper_x1 with - instead of a stepper_y1 with -?

Indeed, this is what i meant by not being easy to reason about.

If it's just an additional stepper could that be more explicit - something like:

# An extra stepper motor that follows the main toolhead
[kinematic_stepper my_extra_kinematic_stepper]
kinematics: 'x+y'
....

I think I'm not understanding something.

-Kevin

While this could definitely work and be very flexible, it seems a bit verbose and difficult to understand, it's also not following the usual klipper approach of being able to specify additional steppers by simply incrementing a number suffix (the LookupMultiRail functionality).

@dmbutyugin
Copy link
Collaborator

dmbutyugin commented May 22, 2024

It definitely sounds like something we should support. I'm not sure I follow the requirements enough to give advice though.

It sounds like the goal is to add an additional stepper that follows the kinematics? If so, the config syntax looks a little confusing to me - how would one choose between a stepper_x1 with - instead of a stepper_y1 with -?

If it's just an additional stepper could that be more explicit - something like:

# An extra stepper motor that follows the main toolhead
[kinematic_stepper my_extra_kinematic_stepper]
kinematics: 'x+y'
....

@KevinOConnor it's not just an extra stepper. Basically, for a hybrid_corexy kinematics the steppers on X axis can have different kinematics depending on the belt path. This applies to stepper_x, dual_carriage for IDEX, and any other stepper_xN, dual_carriageN a user may have in a multi-stepper setup on a rail (e.g. if they have some AWD setup). So it cannot be an extra stepper configuration, not unless we want users to have an ability to not define [stepper_x] section at all. My proposal is to add an option to [stepper_??] sections that will be only valid for hybrid_corexy(z) kinematics and only for a specific axis. So, kinematics option will not be valid for axis Y, for instance, since it cannot have different kinematics on hybrid_corexy. It will not be available on other printer kinematics either. And for hybrid_corexy(z) we can simply keep the sign '+' or '-', no need to specify the full x+y or x-z.

I like the explicitness of this option, and i can see it being useful in other kinematics implementation in the future. My worry is it's not particularly user friendly. Not that it's an issue for us as we generate the configs anyway, but it sort of forces regular users to understand kinematics, which it didn't before. If it's an optional feature—meaning its exclusion would retain the old defaults from the mainline hybrid_corexy and z—it seems to be a sound, explicit, and flexible approach.

Not specifying an option would retain the current behavior. And I'm not sure I understand the argument behind user having to understand the stepper kinematics. They have to understand it even with inverted: True option, don't they? Alternatively, a suggestion could be, configure Y axis first that it moves in the right direction, then check if the toolhead moves correctly or in the reversed direction in X axis, invert step_dir, if the X carriage moves diagonally - flip the kinematics sign and repeat. Or something along those lines. So they don't need to truly understand what goes behind kinematics: '+' or kinematics: '-', similarly how the users don't need to understand what the difference between [stepper_x] and [stepper_y] on corexy, just if the toolhead moves incorrectly, swap the steppers.

I have some alternative ideas. Something that's come up a few times in the past is that using x and y for corexy kinematics is a bit of a misnomer, unfortunately i do not remember the reason for why they're mapped as x and y. If we rename them to A and B, it should be easier to reason about and easier to map programatically (you wouldn't need special casing for X anymore). Then it would go something like this:

@miklschmidt, I understand your proposal, but I believe it does not cover all possible kinematics though. For example, how one would define the following configurations and distinguish between them in your proposal?

[stepper_x]
kinematics: '-'

[stepper_x1]
kinematics: '+'

[dual_carriage]
kinematics: '+'

[dual_carriage1]
kinematics: '-'

and

[stepper_x]
kinematics: '-'

[stepper_x1]
kinematics: '-'

[dual_carriage]
kinematics: '-'

[dual_carriage1]
kinematics: '-'

and

[stepper_x]
kinematics: '-'

[stepper_x1]
kinematics: '-'

[dual_carriage]
kinematics: '+'

[dual_carriage1]
kinematics: '+'

Again, not sure if some of these options are valid from a printer design perspective. But at least option 1 and 3 seem to be valid.

Edit: to expand on this point a bit, I think it'll not be possible to configure an IDEX multirail setup reasonably, at least it'll also be difficult to reason about the following configurations:

[stepper_a]

[stepper_b]

[dual_carriage]

[dual_carriage1]

and

[stepper_a]

[stepper_a1]

[dual_carriage]

[dual_carriage1]

however if that's the preference, we can go that way too. Just it'll also not be entirely backwards-compatible (though we could retain the old behavior with [stepper_x].

@miklschmidt
Copy link
Contributor

miklschmidt commented May 23, 2024

Not specifying an option would retain the current behavior. And I'm not sure I understand the argument behind user having to understand the stepper kinematics. They have to understand it even with inverted: True option, don't they?

Not really, most people understand the concept of inversion, not many understand what kinematics are and what + and - means on a per stepper basis in a hybrid_corexy context. I'm not arguing for our current approach over this though, it has it's own problems.

Alternatively, a suggestion could be, configure Y axis first that it moves in the right direction, then check if the toolhead moves correctly or in the reversed direction in X axis, invert step_dir, if the X carriage moves diagonally - flip the kinematics sign and repeat. Or something along those lines. So they don't need to truly understand what goes behind kinematics: '+' or kinematics: '-', similarly how the users don't need to understand what the difference between [stepper_x] and [stepper_y] on corexy, just if the toolhead moves incorrectly, swap the steppers.

Yeah, whether you do kinematics: '-' or inverted: true doesn't really make a difference for how you determine that your steppers run in the wrong direction. It'll just increase the amount of possible "wrong" configurations. You'd never need to only invert the kinematics on one toolhead and not the other for example, at least i have no idea what that would look like in practice.

Again, not sure if some of these options are valid from a printer design perspective. But at least option 1 and 3 seem to be valid.

3 is valid, 1 and 2 are not. I guess you could call 2 valid but it would be highly impractical. You're in IDEX mode (because of dual carriage) so x1 would need to follow the kinematics of x, dual_carriage1 would follow the kinematics of dual_carriage etc, basically IDEX AWD, the most you should have to do is invert the step_pin or invert the kinematics of x and dual_carriage. 2 would only be possible by running both belts along the same path, which would be very problematic as far as racking, gantry twisting and motor positioning goes. I can't think of any practical use case for 1 and 2. Of course that doesn't mean there isn't one, i just can't see how it wouldn't fight itself or not be hybrid_corexy anymore. It would also not be idiomatic klipper if dual_carriage1 defined a third toolhead or something along those lines? I'm not sure what you have in mind exactly.

Edit: to expand on this point a bit, I think it'll not be possible to configure an IDEX multirail setup reasonably, at least it'll also be difficult to reason about the following configurations:

[stepper_a]

[stepper_b]

[dual_carriage]

[dual_carriage1]

and

[stepper_a]

[stepper_a1]

[dual_carriage]

[dual_carriage1]

Indeed, the first one is invalid, that would define 3 toolheads where dual_carriage has 2 motors (AWD) or two toolheads where the first has 2 belts in corexy mode, and the second is AWD. a, b and dual carriage is 3 axes so no longer hybrid corexy? CoreXYUV?. The second one is non-inverted dual markforged AWD, and looks perfectly reasonable and idiomatic to me. But maybe i've just sat with this stuff for too long.

however if that's the preference, we can go that way too. Just it'll also not be entirely backwards-compatible (though we could retain the old behavior with [stepper_x].

Yep, transition period with support for x/y/dual_carriage and a/b/y, a/dual_carriage/y, b/dual_carriage/y should be doable, then issue a deprecation notice for [stepper_x] and note that it should now be called stepper_a (literally the only change needed unless you're using RatOS, in which case we'll handle all this automatically).

@dmbutyugin
Copy link
Collaborator

@miklschmidt

3 is valid, 1 and 2 are not. I guess you could call 2 valid but it would be highly impractical.

I guess my confusion comes from the fact that the implementation from @HelgeKeck in this PR and in RatRig hybrid_corexy kinematics supports option (1) essentially, but does not support (3). Which is the reverse of what you suggest.

@miklschmidt
Copy link
Contributor

@miklschmidt

3 is valid, 1 and 2 are not. I guess you could call 2 valid but it would be highly impractical.

I guess my confusion comes from the fact that the implementation from @HelgeKeck in this PR and in RatRig hybrid_corexy kinematics supports option (1) essentially, but does not support (3). Which is the reverse of what you suggest.

Yep you're right, i'm quite certain that's a bug. Guess it's obvious no one tested AWD IDEX yet rails[3].steppers[0] (dual_carriage) and rails[3].steppers[1] (dual_carriage1) should indeed run the same kinematics.

@HelgeKeck
Copy link
Author

Yep you're right, i'm quite certain that's a bug.

i think this is indeed a bug

@miklschmidt
Copy link
Contributor

miklschmidt commented May 23, 2024

Yep you're right, i'm quite certain that's a bug.

i think this is indeed a bug

A bug that unintentionally supported CoreXYUV + 2 cartesian y's.. 🤔 I guess that would qualify as double hybrid corexy. 6 motors. Potentially AWD on top of that, so 12 motors total. We're getting into some very exotic builds, but your kinematic: '-/+' definitely wins here @dmbutyugin. I haven't seen anyone build those, but they're definitely valid, and there's no good way to define that with only a/b/dual_carriage.

@KevinOConnor
Copy link
Collaborator

@KevinOConnor it's not just an extra stepper. Basically, for a hybrid_corexy kinematics the steppers on X axis can have different kinematics depending on the belt path.

Okay, thanks. I think I understand, but I am finding this topic confusing. (And I'm getting the impression I'm not the only one.)

For what it is worth, if we were to take a "step back", I think in the long-term we'd be better served with more descriptive config sections. Maybe something like:

[printer]
kinematics: new_more_descriptive_cartesian_style_hybrid

[carriage x]
position_min: 0
position_max: 200
position_endstop: 200
homing_speed: 15.0
...

[carriage y]
...

[carriage z]
...

# The steppers controlling the standard carriages...
[stepper my_printers_rear_right]
kinematics: x-y
step_pin: PA2
...

[stepper my_printers_front_right]
kinematics: y
step_pin: PB2
...

[stepper my_printers_lower_center_rear]
kinematics: z
step_pin: PA3
...

And if the user has idex, additional definitions like:

[dual_carriage my_idex_carriage]
axis: x
...

[stepper my_idex_rear_left]
dual_carriage: my_idex_carriage
kinematics: x+y
step_pin: PA4
...

That is the long-term though, and I understand if that doesn't work in the short term. (It's also entirely possible I'm still not understanding the requirements here, and config sections like the above wouldn't work - if so, please let me know.)

Putting the long-term aside, I guess my immediate feedback would be that I don't think we should name the proposed config sections [stepper_x] and [stepper_x1] because they would do something notably different than common cartesian [stepper_x] and [stepper_x1] config sections. I think it would be too confusing to choose the same config names for something that isn't similar.

Cheers,
-Kevin

@Sineos
Copy link
Collaborator

Sineos commented May 23, 2024

What about leaving the [stepper_...] as they are, but to introduce a new config option like [advanced_kinematics] that allows some flexible manipulation? Maybe even the (regularly encountered) stumbling block described in https://docs.vorondesign.com/build/startup/#motor-configuration-guides could be tackled there (instead of physically swapping).

From a user perspective this would be out of the way for most setups, but in case it is needed then there is an option to do it. In any case, if some is building a more complex kinematic and is facing issues, they will depend on a certain understanding or guidance to solve it.

@miklschmidt
Copy link
Contributor

miklschmidt commented May 23, 2024

@KevinOConnor that doesn't look bad to me, maybe we should consider deprecating kinematics under [printer] entirely? seems to me the majority of the work the kinematic classes are doing could be derived from the carriage and stepper sections with this approach. This could also lead to a more flexible handling of endstops, homing and boundaries. Big change though, and definitely not as easy to comprehend for a user who just wants to print things, but it would provide unparalleled flexibility, akin to RRF M669.

@Sineos i don't like the concept of a [god_section] like that, i find it difficult to imagine how it would be done in a nice and declarative way that wouldn't depend on very specific implementations in the "advanced_kinematics" class. Seems significantly less clean. But maybe i'm not understanding the idea correctly?

@Sineos
Copy link
Collaborator

Sineos commented May 23, 2024

@miklschmidt The idea was rather the opposite of a [god_section] but a [advanced_kinematics] that would contain rarely used / advanced settings that might affect certain setups.
For example to carter for the need to correct the movement of a CoreXY and instead of physically swapping the motor connectors, the section could look like:

[advanced_kinematics]
swap_ab: true

or in case an axis inversion is needed:

[advanced_kinematics]
invert: stepper_x

@dmbutyugin
Copy link
Collaborator

In principle, I like long-term proposal from @KevinOConnor and I can also accept the proposal from @miklschmidt (and I think mine is a bit of middle ground between these two). And since this could be once-in-a-long-time opportunity to rework cartesian-like kinematics handling in Klipper, I'd suggest to let these ideas sink in for a bit before making a final decision. A few things to consider:

  • Kevin's proposal would require more time and efforts to implement, but I suppose is the most comprehensive so far.
  • My proposal is the least intrusive for existing installations and has the most of 'backwards compatibility', plus covers a great deal of existing kinematics out there.
  • Mikkel's proposal is the most simple configuration-wise.
  • Neither of the simpler proposals are 'forward-compatible' in a sense that any configuration made will have to be re-done to be aligned with Kevin's proposal.

And a few other notes:

I guess my immediate feedback would be that I don't think we should name the proposed config sections [stepper_x] and [stepper_x1] because they would do something notably different than common cartesian [stepper_x] and [stepper_x1] config sections.

That's true, but the steppers for CoreXY are called that way already, even thought they are not tied to cartesian X and Y axes. For hybrid_corexy using stepper_x arguably makes more sense than for corexy, since the other steppers actually match the cartesian axes (stepper_y, stepper_z), so the remaining stepper complements them as 'X' stepper. And it does not matter whether it uses 'x+y' or 'x-y' kinematics, at the end of the day this is the only stepper that controls X axis (it just needs to account for the motion of other axes). Of course, in Kevin's proposal it does not make sense to name steppers based on cartesian axes directly anymore.

maybe we should consider deprecating kinematics under [printer] entirely? seems to me the majority of the work the kinematic classes are doing could be derived from the carriage and stepper sections with this approach.

FWIW, there are other kinematics classes like Delta, Deltesian, Polar, which would be a lot of bother to describe as configuration options for the steppers. And they don't have [carriage x] and others. So, deprecating this option for all cases does not seem to be practical (and trying to guess the kinematics from the other sections in printer.cfg will likely make error messages more cryptic for the users who made mistakes there, e.g. by trying to adapt someone else's config).

[advanced_kinematics] that would contain rarely used / advanced settings that might affect certain setups.

My opinion is that it would likely be difficult to provide reasonable configuration options and documentation for such section. Many options would depend on the kinematics class and other options in [advanced_kinematics], so it may turn out to be a real mess.

@KevinOConnor
Copy link
Collaborator

@dmbutyugin - I agree with all your comments. On one minor point:

That's true, but the steppers for CoreXY are called that way already, even thought they are not tied to cartesian X and Y axes.

Just to clarify, my concern on naming is mainly with the proposed [stepper_x1]. On cartesian, [stepper_x] describes both the X carriage and the X stepper. On corexy, [stepper_x] describes the X carriage and the X+Y stepper - which I agree is goofy. On all of the existing kinematics though, [stepper_x1] describes an extra stepper that exactly mirrors [stepper_x]. I think the proposal for [stepper_x1] to have its own kinematics separate from [stepper_x] is confusing - better to give it some new name.

Cheers,
-Kevin

@dmbutyugin
Copy link
Collaborator

dmbutyugin commented Jul 6, 2024

OK, since I think we were not fully happy about any of the short-term options, I decided not to pursue them and instead implement a generic_cartesian kinematics following the suggestion from Kevin. I've published the code now and made a thread on the Klipper discourse, and I suggest to continue the conversation there.

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.

None yet

6 participants