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

Add sync_extruder_steppers #4489

Closed
wants to merge 3 commits into from

Conversation

Tircown
Copy link
Contributor

@Tircown Tircown commented Jul 14, 2021

Add the possibility to synchronize multiple extruder steppers.
Typical usage is to mimic extruder with extruder1 on an IDEX printer printing in duplication or mirrored mode.
Related to PR #4464, replace #4445.

Signed-off-by: Fabrice GALLET [email protected]

@KevinOConnor
Copy link
Collaborator

Thanks. As high-level feedback, though, I think this change should wait until we've reworked the code so that the existing SYNC_STEPPER_TO_EXTRUDER command can be used with extruder stepper motors. That is, I'd prefer to not add a new command that does something very similar to an existing command.

Also, the help text on the new command mentions that the command syncs "heaters and stepper", but that doesn't seem to be the case?

-Kevin

@KevinOConnor KevinOConnor added the other work first Topic waiting for other changes to complete label Jul 26, 2021
@Tircown
Copy link
Contributor Author

Tircown commented Jul 26, 2021

Thank you for the comment.

The mention of heating is an oversight from the original version (PR 4445).

I do understand that it's not right to add and then remove a G-code command. Duplication and mirroring modes require the extruder and extruder1 to make the same movements. Unless to declare dummy pins in extruder1 and an extruder_stepper section with the real pins and use the SYNC_STEPPER_TO_EXTUDER command, I don't know how to perform this task from g-code commands without wasting pins. I assume that dummy pins must exist for the given mcu.

Have you defined a schedule for the extruder.py rework? Can I help on some points?

@KevinOConnor
Copy link
Collaborator

Just to be clear, I'm not suggesting that one define an [extruder_stepper] config section to synchronize multiple extruders - I'm saying I think the existing SYNC_STEPPER_TO_EXTUDER command should work for either extruder or extruder_stepper.

Have you defined a schedule for the extruder.py rework? Can I help on some points?

I haven't looked at it in sufficient detail. I'll try to take a closer look at it.

-Kevin

@Tircown
Copy link
Contributor Author

Tircown commented Jul 27, 2021

Thank you for the comment.
Got it.

@KevinOConnor KevinOConnor removed the other work first Topic waiting for other changes to complete label Jan 8, 2022
@KevinOConnor
Copy link
Collaborator

I do think it would be useful to merge this support into Klipper. If there is still interest in working on this then please update it to the latest code and I will prioritize a review of this work.

Thanks,
-Kevin

@KevinOConnor KevinOConnor added the pending feedback Topic is pending feedback from submitter label Jan 8, 2022
Add ability to synchronize extruder steppers.
Help line correction: no heaters are synchronized in this version.
@Tircown Tircown force-pushed the work-sync-extruder-steppers branch from 2477982 to 1bae660 Compare January 10, 2022 19:56
An oversight from a previous version. The heater is not used anymore.
@Tircown
Copy link
Contributor Author

Tircown commented Jan 10, 2022

Thank you. The PR is ready to be reviewed and hopefully merged.

@KevinOConnor KevinOConnor removed the pending feedback Topic is pending feedback from submitter label Jan 11, 2022
@KevinOConnor KevinOConnor self-assigned this Jan 11, 2022
@KevinOConnor
Copy link
Collaborator

Thanks. At a high-level, I think it makes sense to add this support. However, I think it would be preferable to refactor the existing SYNC_STEPPER_TO_EXTRUDER than to introduce a new SYNC_EXTRUDER_STEPPERS command.

I have attempted to do this in PR #5143 . Unfortunately, I don't have a good way to test that PR. Would you be able to review that PR and see if it meets the requirements here?

-Kevin

@Tircown
Copy link
Contributor Author

Tircown commented Jan 16, 2022

Thanks. I think I can close this PR since #5143 seems to work perfectly now for what we need in idex modes.

@Tircown Tircown closed this Jan 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 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.

2 participants