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

Refactor extruder steppers and extruder_stepper #5143

Merged
merged 4 commits into from
Jan 17, 2022

Conversation

KevinOConnor
Copy link
Collaborator

@KevinOConnor KevinOConnor commented Jan 11, 2022

This PR reworks the extruder stepper logic to make it more flexible. In particular, it is now possible to use the SYNC_STEPPER_TO_EXTRUDER command on either an extruder object or an extruder_stepper object. It is also now possible to call SET_PRESSURE_ADVANCE and SET_EXTRUDER_STEP_DISTANCE on extruder_stepper objects.

This may facilitate synchronizing of multiple extruders on idex printers (such that one can print multiple objects simultaneously). As in #4489.

This may facilitate support for mixing extruders (as in #3920, #4082, #4566).

This PR also deprecates the shared_heater support in extruder config sections. It is thought that one can now obtain the same support by using extruder_stepper config sections.

I do not have a good way to test this code. Feedback and testers would be appreciated.

-Kevin

@Black6spdZ
Copy link

very interested in this and willing to test

@kodonnell
Copy link

Feedback and testers would be appreciated.

I've got an IDEX and have been following e.g. #4464 . As a newbie to klipper (and 3d printers), this might be a really silly question/suggestion ... but I wonder if a single SYNC_MOTORS command is more generic, more obvious, and more flexible in future. In this case (with IDEX in mind) you could sync the steppers on the extruders so that extruder is the master, and extruder1 is the slave and is just sent the exact same pulses etc. There are possibly other cases where you'd want to synchronise other motors (e.g. the two X motors on duplicate mode for IDEX, or synced-but-reversed for mirror mode). Unless I'm missing something, one then doesn't need to worry about adding support for pressure advance etc. - the slave motor just duplicates whatever the master does, and if the master is doing pressure advance, then so will the slave, etc.

Again, apologies if I've missed something obvious.

@Black6spdZ
Copy link

Feedback and testers would be appreciated.

I've got an IDEX and have been following e.g. #4464 . As a newbie to klipper (and 3d printers), this might be a really silly question/suggestion ... but I wonder if a single SYNC_MOTORS command is more generic, more obvious, and more flexible in future. In this case (with IDEX in mind) you could sync the steppers on the extruders so that extruder is the master, and extruder1 is the slave and is just sent the exact same pulses etc. There are possibly other cases where you'd want to synchronise other motors (e.g. the two X motors on duplicate mode for IDEX, or synced-but-reversed for mirror mode). Unless I'm missing something, one then doesn't need to worry about adding support for pressure advance etc. - the slave motor just duplicates whatever the master does, and if the master is doing pressure advance, then so will the slave, etc.

Again, apologies if I've missed something obvious.

this? https://www.klipper3d.org/Config_Reference.html?h=sync#extruder_stepper

@jalanjarosz
Copy link

jalanjarosz commented Jan 13, 2022

Kevin,
I haven't looked into the code changes of this PR. I'm not adept to Python yet.

Suggestion:
Klipper "Extruder" is a combination of elements: Extruder, Heater, Temp Sensor, and Fan (keeping things simple).

Why not remove the Heater, temp sensor, and Fan sub-elements from the Extruder and reference the generic "stand-alone" modules that are in Klipper? ie: add a "Tool_x" module that calls out a Heater, Temp Sensor, Cooling Fan, Tool Fan, and Extruder modules that reference/call the generic objects or list of generic objects?

You could possibly extend the "Tool_x" module to include things like spindle and coolant pumps expanding the use of Klipper into milling or other maker hobbies.

@theophile
Copy link
Contributor

As currently set up, my printer has six bowden "extruders" that feed into a single direct-drive "extruder_stepper" at the hotend. Below is a quick diagram, where "Ex_" represents an Extruder printer object and "ES" represents an Extruder_Stepper printer object under the current code:

 _____   _____   _____   _____   _____   _____
|     | |     | |     | |     | |     | |     |
| Ex0 | | Ex1 | | Ex2 | | Ex3 | | Ex4 | | Ex5 |
|_____| |_____| |_____| |_____| |_____| |_____|
   |       |       |       |       |       |
   |       |       |       |       |       |
   |       |       |       |       |       |
   |_______|_______|_______|_______|_______|
                       |
                       |
                       |
                     __|__
                    |     |
                    | ES  |
                    |_____|
                       V

Currently, each of my Extruders has a shared heater because I only have a single hotend. Also, the Extruder_Stepper always has to be synced to the currently active Extruder because they work in tandem in a sort of push-pull arrangement. This works fine for my purposes.

If I'm understanding correctly, if this PR is merged, then I would need to reconfigure my setup so that I have only one Extruder, and six Extruder_Steppers, like so:

 _____   _____   _____   _____   _____   _____
|     | |     | |     | |     | |     | |     |
| ES0 | | ES1 | | ES2 | | ES3 | | ES4 | | ES5 |
|_____| |_____| |_____| |_____| |_____| |_____|
   |       |       |       |       |       |
   |       |       |       |       |       |
   |       |       |       |       |       |
   |_______|_______|_______|_______|_______|
                       |
                       |
                       |
                     __|__
                    |     |
                    | Ex0 |
                    |_____|
                       V

Then I would not need a shared heater because I only have one Extruder. And instead of switching between six Extruders and then making sure the single Extruder_Stepper is synced to the appropriate active Extruder, I would only ever have one active Extruder and would simply change which of the six Extruder_Steppers is synced to it.

Is this correct? If so, I can foresee problems arising from the fact that an Extruder_Stepper does not move on its own, but only in sync with an Extruder:

  • In my current setup, during filament changes, I disable the direct-drive Extruder_Stepper at the hotend once the filament is retracted from it, and I don't re-enable it again until the new filament is ready to be loaded into it. This prevents it from grinding the gears needlessly when it's "empty." But under the new system, I would not be able to do this, because that stepper would now be the only "Extruder," and it would need to be moving in order for the upstream Extruder_Steppers to be moving.
  • Similarly, a more common setup with a 2-in-1-out system is to have two extruders that feed independently into a single hotend. I don't think it's suitable for Extruder_Steppers to replace the shared heater, because in this use case, the Extruder_Stepper would not be able to move independently of the Extruder.

@KevinOConnor
Copy link
Collaborator Author

Extruder, I would only ever have one active Extruder and would simply change which of the six Extruder_Steppers is synced to it.
Is this correct?

Yes.

The proposal in this branch is to consider an extruder object as a combination of a temperature controlled hotend heater, a motion queue, and an extruder_stepper.

In my current setup, during filament changes, I disable the direct-drive Extruder_Stepper at the hotend once the filament is retracted from it, and I don't re-enable it again until the new filament is ready to be loaded into it. This prevents it from grinding the gears needlessly when it's "empty." But under the new system, I would not be able to do this, because that stepper would now be the only "Extruder," and it would need to be moving in order for the upstream Extruder_Steppers to be moving.

It should be possible to disassociate the extruder stepper from the extruder motion queue:

SYNC_STEPPER_TO_EXTRUDER STEPPER=extruder EXTRUDER=

Similarly, a more common setup with a 2-in-1-out system is to have two extruders that feed independently into a single hotend. I don't think it's suitable for Extruder_Steppers to replace the shared heater, because in this use case, the Extruder_Stepper would not be able to move independently of the Extruder.

I'm not sure what you are reporting. I don't see why it wouldn't work. I'd envision the user would supply macros like the following:

[gcode_macro T0]
gcode:
    # Deactivate stepper in extruder
    SYNC_STEPPER_TO_EXTRUDER STEPPER=extruder EXTRUDER=
    # Activate stepper in my_extruder_stepper
    SYNC_STEPPER_TO_EXTRUDER STEPPER=my_extruder_stepper EXTRUDER=extruder

[gcode_macro T1]
gcode:
    # Deactivate stepper in my_extruder_stepper
    SYNC_STEPPER_TO_EXTRUDER STEPPER=my_extruder_stepper EXTRUDER=
    # Activate stepper in extruder
    SYNC_STEPPER_TO_EXTRUDER STEPPER=extruder EXTRUDER=extruder

Am I missing something?

@KevinOConnor KevinOConnor force-pushed the work-extruder-20220111 branch from ef8c580 to 7a3b54f Compare January 14, 2022 00:36
@Black6spdZ
Copy link

wouldn't this break the opportunity to run separate extruders in a mixing config? or maybe constantly synced but with the capability to alter the proportion "steps/mm" of the two?

@theophile
Copy link
Contributor

I'm not sure what you are reporting. I don't see why it wouldn't work.

As I understand it, an Extruder_Stepper cannot move alone. It can only move in sync with an Extruder. So in your example macros, if the user issues T1, then only "extruder" will be executing the moves while "my_extruder_stepper" sits idle. If the user issues T0, then both "extruder" and "my_extruder_stepper" will be executing the moves in sync. There is no way to get "my_extruder_stepper" to execute the moves by itself, while "extruder" sits idle.

In a basic 2-in-1-out configuration, the user needs two steppers to be able to move independently of one another. And with a mixing hotend like @Black6spdZ is talking about, the user needs multiple steppers to move independently of one another simultaneously.

@KevinOConnor
Copy link
Collaborator Author

If the user issues T0, then both "extruder" and "my_extruder_stepper" will be executing the moves in sync.

No - this PR explicitly adds the ability to disassociate an extruder stepper motor from the extruder motion queue. The example T0 macro above invokes the appropriate command and thus the extruder stepper motor will no longer move with extruder movement. (Pending test results that everything works as expected, of course.)

-Kevin

@theophile
Copy link
Contributor

Ah, that's helpful. I may have been making incorrect assumptions based on the current implementation. If I'm following, then under this PR, an Extruder will simply be a default association of a heater, a motion queue, and an extruder_stepper. Setting the active "Extruder" simply activates these three associated objects, one of which is a defined Extruder_Stepper.

From there, the user can change these independent objects. With respect to the Extruder_Stepper, the user could "unsync" the current one and "sync" a different one instead (so as to use a 2-in-1-out hotend). Or sync an additional Extruder_Stepper to the already active one (as in my push-pull configuration, or an IDEX printer).

If that's right, I like it! One thought though: Wouldn't it make more sense to sync the Extruder_Stepper to a movement queue object, rather than an Extruder object? I think that's what had me confused. In your example macro, the line SYNC_STEPPER_TO_EXTRUDER STEPPER=my_extruder_stepper EXTRUDER=extruder makes me think that the stepper motor defined as "my_extruder_stepper" will mirror the movements performed by the stepper motor defined in "extruder."

Maybe the command JOIN_STEPPER_TO_QUEUE or something along those lines could make the intent/usage clearer?

@KevinOConnor
Copy link
Collaborator Author

I agree the SYNC_STEPPER_TO_EXTRUDER command is a bit confusing. I reused the existing command so as not to break any existing users.

However, it does seem that it's probably better to remove SYNC_STEPPER_TO_EXTRUDER and introduce a new command (eg, SYNC_EXTRUDER_MOTION EXTRUDER=xxx MOTION_QUEUE=yyy). I'll look to update this PR in the next few days.

-Kevin

@Black6spdZ
Copy link

SYNC_EXTRUDER_MOTION EXTRUDER=xxx MOTION_QUEUE=yyy)

hmm, this looks promising.. but I am still at a loss how to alter the extruder's motion as a set percentage of it's original "rotation_distance" calc. There is legecy set_extruder_step_distance but it's value is inversely proportional and would be exponentially large to attempts say a 1% extrusion rate

@3Dbert
Copy link

3Dbert commented Jan 16, 2022

Hi @KevinOConnor,

I was testing this PR and #4464 together with @Tircown on a Cartesian idex printer. We observed the following unexpected behavior:
If PA is active on one extruder, klipper will go into shutdown, as soon as a extrude command (G1 E…) is issued for the given extruder. In other words:
Everything is working fine as long as PA is deactivated for every extruder, that gets used.
This behavior was also observed by @DrumClock.
Following tests have been made:

  1. Ex0:PA Ex1:No PA
    1.1 Dual Color: Internal error on command:"G1" (shutdown after homing)
    1.2 Duplication/Mirror: Internal error on command:"G1" (shutdown after homing)

  2. Ex0:No PA Ex1:PA
    2.1 Dual Color: Internal error on command:"G1" (shutdown when T1 got activated)
    2.2 Duplication/Mirror: Internal error on command:"G1" (shutdown after homing)

  3. Ex0:PA = Ex1:PA
    3.1 Dual Color: Internal error on command:"M109" (shutdown after homing)
    3.2 Duplication/Mirror: Internal error on command:"G1" (shutdown after homing)

  4. Ex0:PA Ex1:PA
    4.1 Dual Color: Internal error on command:"M109" (shutdown after homing)
    4.2 Duplication/Mirror: Internal error on command:"G1" (shutdown after homing)

klippy.log

@KevinOConnor KevinOConnor force-pushed the work-extruder-20220111 branch from 7a3b54f to b304968 Compare January 16, 2022 18:29
@KevinOConnor
Copy link
Collaborator Author

Thanks for testing. Unfortunately there was a typo in the pressure advance code. I'm not sure how it managed to pass the regression test cases. I've corrected it (hopefully) and rebased this branch.

-Kevin

@KevinOConnor
Copy link
Collaborator Author

hmm, this looks promising.. but I am still at a loss how to alter the extruder's motion as a set percentage of it's original "rotation_distance" calc. There is legecy set_extruder_step_distance but it's value is inversely proportional and would be exponentially large to attempts say a 1% extrusion rate

I'm not really sure what you're reporting. If you've got a printer you'd like to test, can you provide a description of it, your current config, and your test attempts with this branch?

It may be possible to configure a mixing extruder with this branch using the SET_EXTRUDER_STEP_DISTANCE command on extruder_stepper and extruder objects. I don't have mixing extruder hardware though, so can't test it.

-Kevin

@DrumClock
Copy link

DrumClock commented Jan 16, 2022

Hi @KevinOConnor
YES it works .....

  • extruder with value pressure_advance: 0.8
  • extruder1with value pressure_advance: 0.0

the "plucking" of the extruder was seen as the X Y changes
video: https://youtu.be/PEEIpGroXLM

Store the pressure_advance value in "struct extruder_stepper" instead
of in the trapq's "struct move".  This makes it possible for multiple
stepper motors to have different pressure advance values while still
using the same trapq.

Signed-off-by: Kevin O'Connor <[email protected]>
…lass

Move the stepper handling (including pressure advance handling) to a
new class.

Signed-off-by: Kevin O'Connor <[email protected]>
Refactor the extruder_stepper support so that it uses the
ExtruderStepper class defined in extruder.py.

Support the SYNC_STEPPER_TO_EXTRUDER command on steppers defined in
either extruder_stepper or extruder config sections.

Signed-off-by: Kevin O'Connor <[email protected]>
Support SYNC_STEPPER_TO_EXTRUDER commands with an EXTRUDER parameter
set to an empty string.

Signed-off-by: Kevin O'Connor <[email protected]>
@KevinOConnor KevinOConnor force-pushed the work-extruder-20220111 branch from b304968 to 02d5f97 Compare January 17, 2022 00:05
@KevinOConnor KevinOConnor merged commit 02d5f97 into master Jan 17, 2022
@KevinOConnor KevinOConnor deleted the work-extruder-20220111 branch January 17, 2022 00:06
@KevinOConnor
Copy link
Collaborator Author

Thanks.

I went ahead and merged this PR, but without the deprecation of the shared_heater. I'll plan to make a separate PR that deprecates shared_heater and SYNC_STEPPER_TO_EXTRUDER.

-Kevin

@KevinOConnor
Copy link
Collaborator Author

FYI, PR #5210 has the updates to deprecate shared_heater, along with other changes.

-Kevin

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.

7 participants