-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
base: master
Are you sure you want to change the base?
hybrid_corexy: Fix for inverted hybrid core-xy setups and extended usability #6351
Conversation
klipper merge
…/HelgeKeck/klipper into inverted-hybrid_corexy-belt-path
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 |
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 |
docs: Add Peopoly to Sponsors.md
so this pr fixes also the issue with the wrong x coordinates in case you probe or z-home with the dual carriage toolhead |
Thanks, I'll take a closer look a bit later, maybe in a week or so. Just a quick question though, |
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 |
just noticed something, it seems that |
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:
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, PS: I'm just an automated script, not a human being. |
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, PS: I'm just an automated script, not a human being. |
possible configs Single Toolhead A
Single Toolhead AB
IDEX
IDEX AWD
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 |
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. |
Yes, I saw your comment regarding this from before. However, I'm particularly curious how the steppers would work in these cases
Do
Here the same, do |
btw, this is the updated kinematic file that works with the recent klipper changes as well |
see |
its jsut like putting 2 stepper in series for the same belt |
The reason I was trying to clarify this is because your code has the following snippet:
which unconditionally assigns an inverted kinematics for |
im sorry, i misunderstood your question and forgot a important detail 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 |
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
|
so yes, correct stepper naming is key to avoid any future confusion |
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
|
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 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, |
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 |
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 If it's just an additional stepper could that be more explicit - something like:
I think I'm not understanding something. -Kevin |
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.
Forced co-location is one of the better properties of the klipper configuration DSL imo, so i would definitely keep it on the 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
Hybrid corexy single toolhead B (this would effectively be "inverted") if used on the same belt path as the previous
Hybrid corexy single toolhead AB
Hybrid corexy single toolhead AB inverted Hybrid corexy IDEX
Hybrid corexy IDEX inverted
The last case (hybrid corexy IDEX inverted) would need special handling though, as the kinematics for b and Bonus is all of these would allow easy multirail mapping. |
Indeed, this is what i meant by not being easy to reason about.
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 |
@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
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
@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?
and
and
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:
and
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 |
Not really, most people understand the concept of inversion, not many understand what kinematics are and what
Yeah, whether you do
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
Indeed, the first one is invalid, that would define 3 toolheads where
Yep, transition period with support for |
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 |
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 |
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:
And if the user has idex, additional definitions like:
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 Cheers, |
What about leaving the 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. |
@KevinOConnor that doesn't look bad to me, maybe we should consider deprecating @Sineos i don't like the concept of a |
@miklschmidt The idea was rather the opposite of a
or in case an axis inversion is needed:
|
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:
And a few other notes:
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
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
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 |
@dmbutyugin - I agree with all your comments. On one minor point:
Just to clarify, my concern on naming is mainly with the proposed Cheers, |
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 |
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 thedual_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 [email protected]