Skip to content

kinematics: Add dual_carriage to hybrid-corexyz#4296

Merged
KevinOConnor merged 7 commits into
Klipper3d:masterfrom
Tircown:work-hybrid-corexyz-merge
Jun 27, 2021
Merged

kinematics: Add dual_carriage to hybrid-corexyz#4296
KevinOConnor merged 7 commits into
Klipper3d:masterfrom
Tircown:work-hybrid-corexyz-merge

Conversation

@Tircown

@Tircown Tircown commented May 16, 2021

Copy link
Copy Markdown
Contributor

Add dual_carriage abilities for hybrid-corexy and hybrid-corexz
Introduce the module idex_mode
Fix add_stepper to the correct rail in hybrid-corexy

Signed-off-by: Fabrice GALLET tircown@gmail.com

@Tircown

Tircown commented May 18, 2021

Copy link
Copy Markdown
Contributor Author

Thank you to Ankurv who made a tremendous work to test with his IDEX Switchwire and helped to improve and chase the bugs in this kinematic.

Does I have to do something with this failed check? I am not very in hands with github functionalities.

@KevinOConnor

Copy link
Copy Markdown
Collaborator

Thanks. Not sure why the build originally failed - likely some github hiccup. It's now failing due to whitespace errors.

Alas, I wont be able to look at this in detail until next week.

Cheers,
-Kevin

@KevinOConnor

Copy link
Copy Markdown
Collaborator

Thanks.

I'm struggling a bit to understand how this code works. If I understand correctly, the main idea is that one can "disable" the stepper on a particular rail and "enable" some other stepper on the same rail - so as to control two separate carriages on the same rail. The key difference between this dual carriage system vs cartesian is that the "disabled" stepper still needs to generate steps. Did I understand this correctly?

If so, what is the reason for hooking this code into the homing and limit checks? Also, can you give a high-level description of what each of the new classes is used for.

Some things I noticed:

  • It should not be necessary to introduce new C code for this functionality. It looks to me like there are two new kinematics added (-y, and -z), but I think it should be possible to use the existing cartesian step generation functions with the direction pin swapped.
  • The new idex_modes.py code is only imported by code in the klippy/kinematics/ directory. No reason to put that code in klippy/extras/ - as in that case it would be preferable to put it in klippy/kinematics/
  • I saw new code that directly accesses the member variables of classes defined in other python files. The coding convention of Klipper is to avoid that (it's mentioned briefly in docs/Code_Overview.md). The code should use a method to access the data - adding a method if necessary.
  • It looks like the code exports new get_status() information - if so, that should be added to docs/Status_Reference.md and if there are any new config options available then it should be added to docs/Config_Reference.md. This would help the review a bit as it would give some context to some of the changes.

Cheers,
-Kevin

@Tircown

Tircown commented May 31, 2021

Copy link
Copy Markdown
Contributor Author

Thank you for your feedbacks.

It should not be necessary to introduce new C code for this functionality. It looks to me like there are two new kinematics added (-y, and -z), but I think it should be possible to use the existing cartesian step generation functions with the direction pin swapped.

I haven't found how to dynamically reverse a pin. But I also think it is clearer to do it like I did. Do you have a clue for this reversal?

The new idex_modes.py code is only imported by code in the klippy/kinematics/ directory. No reason to put that code in klippy/extras/ - as in that case it would be preferable to put it in klippy/kinematics/

Ok, will do.

I saw new code that directly accesses the member variables of classes defined in other python files. [...]

Yes I was informed of this and I thought I had taken care to avoid it. I don't see where I did this, which class variable did you see to be affected?

It looks like the code exports new get_status() information - if so, that should be added to docs/Status_Reference.md [...]

Ok. will do

[...] if there are any new config options available then it should be added to docs/Config_Reference.md [...]

There is nothing new compared to cartesian dual_carriage at this point. Except that the axis must be x here.

@Tircown

Tircown commented May 31, 2021

Copy link
Copy Markdown
Contributor Author

I'm struggling a bit to understand how this code works. If I understand correctly, the main idea is that one can "disable" the stepper on a particular rail and "enable" some other stepper on the same rail - so as to control two separate carriages on the same rail. The key difference between this dual carriage system vs cartesian is that the "disabled" stepper still needs to generate steps. Did I understand this correctly?

Yes it's exactly that. My goal is to provide a module that can be use for hybrid-corexy/z, cartesian, corexy/z or any style of printer that can handle 2 carriages. The following PR will introduce the duplicate and mirror modes for IDEX. The whole code is already working on ankurv's printer.

If so, what is the reason for hooking this code into the homing and limit checks?

Homing sequence (G28): as cartesian dual_carriage, we need to home both carriage in the homing sequence.

Forced homing during carriage toggle: carriage position is critical for dual-color prints and one have to fine tune the offsets to get nice prints. I have a cartesian IDEX printer and I need to home my axis before using a tool for a more reliable position. Maybe it is caused by disabling the stepper, maybe it is mechanic,.. For hybrid-corexy I presume that passively moving the axis on a not so rigid gantry can cause misalignment in some cases. So why not simply homing both carriages with a G28 X? because it's not required and as the endstops have a certain mechanical durability, this feature divide by 2 the amount of triggering.
Anyway the function force_home_axis will be required for duplicate and mirror modes for the homing sequence.

Limits update: The idea behind this is to set your limits as T0 can never collide T1, i.e. if the carriage is 60mm wide on a 400mm axis, T0 range would be [0, 340] and T1 range would be [60, 400]. This will be enhanced for the duplicate and mirror modes; I will write a documentation for this purpose.

@KevinOConnor

Copy link
Copy Markdown
Collaborator

I haven't found how to dynamically reverse a pin. But I also think it is clearer to do it like I did.

Okay, but in that case, I'd add the new functions to the existing kin_cartesian.c code (as they are fundamentally a cartesian kinematics) - for example, by adding a new cartesian_reverse_stepper_alloc() method.

Yes I was informed of this and I thought I had taken care to avoid it. I don't see where I did this, which class variable did you see to be affected?

I saw dc.rail.get_range(), where rail appears to be a member variable of a class defined in another file. Maybe I'm missing something though.

I have a cartesian IDEX printer and I need to home my axis before using a tool for a more reliable position. Maybe it is caused by disabling the stepper

FWIW, the simple answer would be to not disable the stepper motor.

So why not simply homing both carriages with a G28 X?

Okay, but I think it would help reviewing if this feature could be added after the basic support is added.

Separately, is there a plan to use the new DualCarriageRail stuff on the existing cartesian kinematics?

-Kevin

@Tircown

Tircown commented Jun 10, 2021

Copy link
Copy Markdown
Contributor Author

Thanks!

[...] for example, by adding a new cartesian_reverse_stepper_alloc() method.

edit: For dupe/mirror modes I must reverse the corexyz plus function too i.e. -c.x + cy. Would it be ok if I add a parameter for reversing:
cartesian_stepper_alloc(char axis, char direction)?

edit: edit: how am I supposed to manage the sk->active_flags = AF_X | AF_Z in the cartesian stepper allocation?
edit: Sorry for the multi edits, I got it now!. If it's ok for you, for the dupe mode I need corexy_reverse_stepper_alloc( char p|m ) too. The kinematic formula is -x+y for mirroring the "p" one (x+y) but instead of doing a reverse on the x only I think I will do -(x-y) which is a reverse of the "m" one. Hope I am clear in my explenation.

I saw dc.rail.get_range() [...]

Got it. It refers to the class DualCarriagesRail which is in idex_modes.py too. The rail is created in the kinematic file, then use as argument to create a DualCarriagesRail object. Two DualCarriagesRail objects are then use as arguments for the DualCarriage class. I will add a get_range function to the DualCarriagesRail if it's ok to you.

I will remove the HOMING=0|1 to this PR and I will create a separate PR with it.

Sure, I will add the dupe/mirror modes to cartesian printers. If someone wants to create a corexyu or corexyuv kinematics, my idex_modes module should ease this work. I don't plan to build corexy IDEX so I will not add it myself since I won't be able to test it but adding this should require really little work.

Tircown added 2 commits June 12, 2021 16:39
- Add dual_carriage abilities for hybrid-corexy and hybrid-corexz
- Introduce the module idex_mode
- Fix add_stepper to the correct rail in hybrid-corexy
Fix trailing spaces
@Tircown Tircown force-pushed the work-hybrid-corexyz-merge branch from d40ad3c to 8cf06df Compare June 12, 2021 14:41
Tircown added 2 commits June 12, 2021 17:08
Following PR comments:
- Module idex_mode is now in kinematics/
- Add reverse stepper alloc for cartesian printers
- Fix direct access to rails in DualCarriages
- Add dual_carriages status to the documentation
- Remove the force homing option for carriage toggling and his documentation
Fix line length
@Tircown

Tircown commented Jun 14, 2021

Copy link
Copy Markdown
Contributor Author

I made the required changes accordingly to your comments.

@KevinOConnor

Copy link
Copy Markdown
Collaborator

If it's ok for you, for the dupe mode I need corexy_reverse_stepper_alloc( char p|m ) too

Okay.

It refers to the class DualCarriagesRail which is in idex_modes.py too.

Okay - I missed that. My mistake.

I made the required changes accordingly to your comments.

Thanks. I noticed a couple of minor things in the code:

  1. I think it would be preferable to avoid using hasattr(). I think it would be preferable if the constructor set self.dc_module = None and later had checks for if self.dc_module is not None:.
  2. Adding a new field to toolhead's get_status() results for only one kinematic is problematic because it leads to confusing results when some printers have the field and other printers do not. If you want to export the status I think it would be preferable for DualCarriages to register itself as a printer object (printer.add_object("dual_carriage", self)) and directly implement get_status(). This way users and developers can get at the info with the normal checks for module presence.

Separately, not something that would hold up merging, but I wonder if we could make DualCarriagesRail() behave like a PrinterRail() class - then cartesian.py, corexy.py, corexz.py could unconditionally allocate a DualCarriagesRail and have that class deal with the different cases that occur when a dual_carriage definition is in place or not.

Thanks again,
-Kevin

@Tircown

Tircown commented Jun 15, 2021

Copy link
Copy Markdown
Contributor Author

Thank you.

I wonder if we could make DualCarriagesRail() behave like a PrinterRail() class

I am not sure to understand what you want there. To be honest I would find logical to add all the features of DualCarriageRail into PrinterRail. To match this:
-The parameter axis can be removed to be totally managed in DualCarriage by dealing with the position of the rail only and not the full list.
-The parameter active can be leaved out and by default the rail is active. After set up the second PrinterRail of the dual carriage axis one need to inactivate it expressly in the kinematic _init function.
-The active/inactive/reverse* stepper alloc callbacks can be set in one or few other function(s) after the PrinterRail is created.
-Add a stored position. The stepper position of an inactive rail still evolve as, at least for the hybrid-corexy robots, it moves accordingly to Y. But the carriage does not moves along the X axis. So the rail position is no longer the same as the stepper position. The stepper position is stored in a variable during the inactivation and set back during the activation to get around this problem.
-Add a status for the PrinterRail: ACTIVE, INACTIVE and REVERSED*.
-Add functions to switch between these status and to get the current status and the rail position (not the stepper position)

I did not get into that changes directly as in the PR #3609 adding parameters to PrinterRail were not the good way to do.

*The reverse capability will be introduced in the next PR for the mirror mode and only for the 2nd carriage.
Here is the full module that ankurv uses to print in dupe/mirror mode. Just a preview of what would be the result of this series of PR (minus current changes).

https://github.com/Tircown/klipper/blob/work-hybrid-corexz/klippy/extras/idex_modes.py

@KevinOConnor

Copy link
Copy Markdown
Collaborator

Okay - I think we should change the hasattr and not introduce the get_status() changes now. We can then merge that.

I am not sure to understand what you want there. To be honest I would find logical to add all the features of DualCarriageRail into PrinterRail.

I'm not sure it would be a good idea to add significant complexity to PrinterRail - as that class is used by many other kinematics where DualCarriage isn't used. The DualCarriage is a relatively rare setup, so I think the code would be more manageable in its own class. That said, it should be possible for a DualCarriage class to mimic a PrinterRail class so that its users could largely treat it as a regular rail.

Alas, though, I don't know enough about the upcoming mirror/dup mode changes to know if that actually makes sense. And, unfortunately, I don't think I'll be able to review it in the short term.

Cheers,
-Kevin

Tircown added 2 commits June 23, 2021 00:29
remove all hasattr()
go back over get_status() usage
save_idex_state and restore_idex_state are now independent of get_status()
Fix trailing spaces
@Tircown

Tircown commented Jun 22, 2021

Copy link
Copy Markdown
Contributor Author

I made the required changes.
I revamped the recover_status into save_idex_state and restore_idex_state to not drag axis_position in the status. Restore_idex_state will need these positions to restore the dupe/mirror modes.

Conditional test in calc_position  for carriage_1 to be more tolerent to future modifications. Was very strict on get_status format.
@KevinOConnor KevinOConnor merged commit 4d55963 into Klipper3d:master Jun 27, 2021
@KevinOConnor

Copy link
Copy Markdown
Collaborator

Thanks.

-Kevin

@Tircown Tircown mentioned this pull request Jul 27, 2021
Cirromulus pushed a commit to Cirromulus/klipper that referenced this pull request Aug 4, 2021
- Add dual_carriage abilities for hybrid-corexy and hybrid-corexz
- Introduce the module idex_mode
- Fix add_stepper to the correct rail in hybrid-corexy

Signed-off-by: Fabrice GALLET <tircown@gmail.com>
@liquid-light

Copy link
Copy Markdown

Hej

I would like to "open" this again because of the x+y and x-y topic.
I'm working on my setup since a few weeks now. But one thing I never solved is that this setup always prints mirroed parts.
Every Slicer assumes that xy 0 is front left. But with this setup you have basically no option to get this right (except for designing the whole printer setup according to the code).

With Corexy this is a simple fix. But not with the Hybrid Corexy (aka Markforged) kinematic.
First I thought I could use the mirror option for one axis (which is implemented in the IDEX) mode. But it seems that this is only available if you use a second carriage.

So basically I have two options right now. Either switch to Marlin again or changing the position of my Steppermotor.
Or do I miss something?

By the way, the lack of documentation on this Kinematic system makes it really hard to find a solution.

@Tircown

Tircown commented Jun 7, 2022

Copy link
Copy Markdown
Contributor Author

Hi,

That's true, you can't just invert the direction or swap A/B motor as you would do with a CoreXY. If you have 2 carriages on your hybrid-corexy try to swap physically both carriage. This kinematics depends on which belt is attached on which carriage. So assuming T0 is attached to the bottom belt and T1 is attached to the top belt in a CoreXY belt path configuration, attaching T0 to the top belt and T1 to the bottom belt should solve your issue (you may need to invert some stepper direction too).
I will publish a fix in few weeks in order to solve this problem with a config parameter, at the same time I will rework this kinematics and the dupe/mirror mode quite deeply.

Thanks.

@liquid-light

Copy link
Copy Markdown

Hey @Tircown

That sounds very promising. In fact your implementation of the Markforged Kinematic was the main decision driver for me to switch from Marlin to Klipper!

Unfortunately I have only one carriage.
So basically you're telling me that I have no other option than waiting for your code modification right?

Please let me know if there is any way how I could support you ;)
And thx for your effort!

@liquid-light

Copy link
Copy Markdown

Hi,

That's true, you can't just invert the direction or swap A/B motor as you would do with a CoreXY. If you have 2 carriages on your hybrid-corexy try to swap physically both carriage. This kinematics depends on which belt is attached on which carriage. So assuming T0 is attached to the bottom belt and T1 is attached to the top belt in a CoreXY belt path configuration, attaching T0 to the top belt and T1 to the bottom belt should solve your issue (you may need to invert some stepper direction too). I will publish a fix in few weeks in order to solve this problem with a config parameter, at the same time I will rework this kinematics and the dupe/mirror mode quite deeply.

Thanks.
@Tircown

Any news? I would really like to test the kinematic on my printer. Right now it's just collecting dust. :)

@Tircown

Tircown commented Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

I will have more time to work on the kinematics in few days.

Thanks
Tircown

@liquid-light

Copy link
Copy Markdown

@Tircown

Just curious to hear if something happened in the meantime. I would really like to test it if it's available ;)

Thanks
Liquid-light

@Sineos

Sineos commented Nov 18, 2022

Copy link
Copy Markdown
Collaborator

@github-actions github-actions Bot locked and limited conversation to collaborators Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants