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

Ensure TMC + LIN_ADVANCE pulse length #15807

Merged
merged 4 commits into from
Nov 11, 2019

Conversation

ManuelMcLure
Copy link
Contributor

@ManuelMcLure ManuelMcLure commented Nov 5, 2019

Description

Make sure that if any extruder is a TMC driver, the minimum pulse length is honored. If SQUARE_WAVE_STEPPING is not enabled or this is a standalone driver (which don't support SQUARE_WAVE_STEPPING) set MINIMUM_STEPPER_PULSE to 1 instead of the TMC default of 0.

Benefits

This is a FAQ in the support channels - people switch to a TMC on their extruder and get no extrusion with LIN_ADVANCE enabled.

Related Issues

#15761

@ManuelMcLure
Copy link
Contributor Author

There’s another option for non-standalone drivers which is to enable SQUARE_WAVE_STEPPING instead of setting the pulse length, but I took this option as being safer.

Copy link
Contributor

@psavva psavva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix will not work if MINIMUM_STEPPER_PULSE is configured in the config files...

You may need to change the logic to check if not defined or defined as zero...

If it is defined as zero, the new conditional will not be executed, giving the same behavior with the same config...

@ManuelMcLure
Copy link
Contributor Author

I'm assuming that if MINIMUM_STEPPER_PULSE was explicitly set to 0 the user does not want it overridden. This is just a fix to avoid problems with the default configuration files.

@psavva
Copy link
Contributor

psavva commented Nov 5, 2019

The issue is that the recommendation for TMC is to set it to zero. Having also defined LIN_ADVANCE, the extruder don't work.

If a conditional is set like it is here, it simply won't work when users follow the recommendation, and cannot extrude...

I recommend implementing a sanity check in this case, which will resolve the issue under all user cases, not just when minimum pulse is not defined.

@ManuelMcLure
Copy link
Contributor Author

I don't think we should override values set specifically by the user. We can change the comment with the recommendation to say that if LA is turned on the default should be 1, but I disagree that this is a large problem. Most people will just uncomment the default line and get 2. If they specifically go to the effort of changing the value of MINIMUM_STEPPER_PULSE to 0 we shouldn't change it.

@thisiskeithb
Copy link
Member

thisiskeithb commented Nov 5, 2019

There’s another option for non-standalone drivers which is to enable SQUARE_WAVE_STEPPING instead of setting the pulse length, but I took this option as being safer.

I can't say I've heard of a bad result enabling SQUARE_WAVE_STEPPING, but it's also still fairly new.

In my SKR 1.3/TMC5160/Linear Advance build with SQUARE_WAVE_STEPPING enabled, I no longer have to increase (or enable) a custom MINIMUM_STEPPER_PULSE.

@ManuelMcLure
Copy link
Contributor Author

This PR checks whether SQUARE_WAVE_STEPPING is enabled and doesn't override MINIMUM_STEPPER_PULSE if it is (unless the extruder is a standalone TMC since we can't use SQUARE_WAVE_STEPPING on those).

@teemuatlut
Copy link
Member

I don't think we should override values set specifically by the user.

^

@psavva
Copy link
Contributor

psavva commented Nov 6, 2019

Perhaps just a sanity check then.

If Trinamic and Lin Advance and not ( Stepper Pulse > 0 or Square Wave Stepping) then raise a error

@psavva
Copy link
Contributor

psavva commented Nov 7, 2019

@thinkyhead any suggestions on this one?

@thinkyhead
Copy link
Member

Sanity-check is always the most user-aware route, and is fine for a first approach.

@ManuelMcLure
Copy link
Contributor Author

I've added a sanity check in case a bad value for MINIMUM_STEPPER_PULSE was explicitly set.

@thinkyhead thinkyhead merged commit b9703fd into MarlinFirmware:bugfix-2.0.x Nov 11, 2019
@thinkyhead thinkyhead changed the title Ensure pulse length for TMC & LIN_ADVANCE Ensure TMC + LIN_ADVANCE pulse length Nov 11, 2019
@ManuelMcLure ManuelMcLure deleted the TMCStepWarning branch November 11, 2019 02:22
philippniethammer pushed a commit to philippniethammer/Marlin that referenced this pull request Dec 21, 2019
christran206 pushed a commit to christran206/Marlin2.0-SKR-Mini-E3-1.2 that referenced this pull request Dec 30, 2019
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.

5 participants