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

Extruder: Add g-codes for get and set extruder step_distance #2598

Merged
merged 6 commits into from
Mar 28, 2020

Conversation

ld3300
Copy link
Contributor

@ld3300 ld3300 commented Mar 7, 2020

Add GET_E_STEP_DISTANCE and SET_E_STEP_DISTANCE to available g-codes. Implemented in response to request for a tunable step_distance to account for difference in hardness levels of filament materials.

Signed off by: David Smith [email protected]

@KevinOConnor
Copy link
Collaborator

Thanks.

What's the high-level "use case" you envision for this?

Be aware that some users may wish to disable a stepper (eg, possibly via distance=0.0) and reverse a stepper (eg, possibly via distance=-.001). Doing so would enable #1631 and possibly #950 via [extruder_stepper] config sections.

Separately, the step_distance activation doesn't seem correct to me - I'm not sure why the set_trapq, set_max_jerk, and register_step_generator calls are being made. I'd recommend moving all the low-level logic into stepper:set_step_dist().

Finally, I'd recommend SET_EXTRUDER_STEP_DISTANCE for the command name.

-Kevin

@ld3300
Copy link
Contributor Author

ld3300 commented Mar 8, 2020

Hi Kevin,
This came out of the conversation in #2445 so that the step distance can be declared from the slicer to compensate for different filament hardness levels that cause the extruder teeth to sink into filaments differently, therefore changing the step distance property. With the other tickets you mention, I'm unsure if you are suggesting that these changes would positively or negatively affect those. If it would enable those features in a positive way I would probably need to remove the "safety" check I added (I may have accidentally realized how big a difference there is between between 0.007 and 0.004, thank goodness I didn't miss a zero).

I put all those calls in extruder.py because that is also where they are initially called in the init section. I can do it in the lower level set_step_dist, but I have to pass all the params. I added the calls with formulas that looked like they were only called once and were affected by the step distance. set_max_jerk has _step_distance in it's formula. set_trapq is setup using _stepper_kinematics which is calculated using _step_dist.

I might have had in my head that register_step_generator needed to be recalled too. It looks like it does not need to be.

The command name change makes sense. Will implement that.

-David

…ommands to GET_EXTRUDER_STEP_DISTANCE and SET_EXTRUDER_STEP_DISTANCE. Update names in G-Codes.md
Copy link
Collaborator

@KevinOConnor KevinOConnor left a comment

Choose a reason for hiding this comment

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

Thanks. See my comments below.

-Kevin

Comment on lines 75 to 77
gcode.register_mux_command("GET_EXTRUDER_STEP_DISTANCE", "EXTRUDER",
self.name, self.cmd_GET_E_STEP_DISTANCE,
desc=self.cmd_GET_E_STEP_DISTANCE_help)
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, I would not add a GET_EXTRUDER_STEP_DISTANCE command. Instead, I'd have SET_EXTRUDER_STEP_DISTANCE report the current step distance if an explicit DISTANCE parameter is not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense I will make that change.

Comment on lines 205 to 209
if abs((dist - old_step_dist)/old_step_dist) > 0.25:
gcode.respond_info("Failed: step change is outside of"
" safe margin. %f -> %f" %
(old_step_dist, dist))
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, I don't think this check is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, when I implemented that it was a knee jerk reaction to making a mistake typing in a value too high, but with some of the other uses this might provide the check would only be a hindrance. I will remove it.

Comment on lines 214 to 216
self.stepper.set_max_jerk(9999999.9, 9999999.9)
self.stepper.set_stepper_kinematics(self.sk_extruder)
self.stepper.set_trapq(self.trapq)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These three lines should be removed.

The set_max_jerk() call configures the micro-controller "min_stop_interval" configuration. That configuration can not be changed at run-time, so it's confusing to alter the setting at run-time. In this particular case, a 9999999.9 disables the min_stop_interval check, so it's redundant to call again anyway.

I understand the set_stepper_kinematics() call is to populate the step_distance to the "itersolve" code. That's fine, but it should be done from stepper:set_step_dist().

The set_trapq() call associates the stepper with a "motion queue". That motion queue isn't changing on a step distance update, so there is no reason to call it here.

Separately, I believe it will be necessary to call toolhead.flush_step_generation() prior to updating the step distance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, okay, on the max_jerk I didn't see that check, so makes sense. I will move the set_stepper_kinematics() as suggested. Thank you for note about toolhead.flush_step_generation(). Thank makes a lot of sense. I will make those changes and test to make sure it performs as expected.

def set_step_dist(self, dist):
self._step_dist = dist
logging.info("%s manually set to =%.6f", (self._name, dist))
return self._step_dist
Copy link
Collaborator

Choose a reason for hiding this comment

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

This return statement looks redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, should have trusted my gut on that one, added it thinking "this isn't useful at all"

@kakou-fr
Copy link
Contributor

could you also patch extruder_stepper to allow set step for mixing ?
i tried, but i always have this error : stepcompress o=0 i=0 c=12 a=0: Invalid sequence

diff --git a/klippy/extras/extruder_stepper.py b/klippy/extras/extruder_stepper.py
index f6d0e9ea..6e780949 100644
--- a/klippy/extras/extruder_stepper.py
+++ b/klippy/extras/extruder_stepper.py
@@ -9,17 +9,39 @@ import stepper
 class ExtruderStepper:
     def __init__(self, config):
         self.printer = config.get_printer()
+        self.name = config.get_name()
         self.extruder_name = config.get('extruder', 'extruder')
         self.stepper = stepper.PrinterStepper(config)
         self.stepper.set_max_jerk(9999999.9, 9999999.9)
         self.stepper.setup_itersolve('extruder_stepper_alloc')
         self.printer.register_event_handler("klippy:connect",
                                             self.handle_connect)
+        # Register commands
+        gcode = self.printer.lookup_object('gcode')
+        gcode.register_mux_command("GET_EXTRUDER_STEP_DISTANCE", "EXTRUDER",
+                                   self.name, self.cmd_GET_E_STEP_DISTANCE,
+                                   desc=self.cmd_GET_E_STEP_DISTANCE_help)
+        gcode.register_mux_command("SET_EXTRUDER_STEP_DISTANCE", "EXTRUDER",
+                                   self.name, self.cmd_SET_E_STEP_DISTANCE,
+                                   desc=self.cmd_SET_E_STEP_DISTANCE_help)
     def handle_connect(self):
         extruder = self.printer.lookup_object(self.extruder_name)
         self.stepper.set_trapq(extruder.get_trapq())
         toolhead = self.printer.lookup_object('toolhead')
         toolhead.register_step_generator(self.stepper.generate_steps)
+    cmd_GET_E_STEP_DISTANCE_help = "Get extruder step distance"
+    def cmd_GET_E_STEP_DISTANCE(self, params):
+        gcode = self.printer.lookup_object('gcode')
+        step_dist = self.stepper.get_step_dist()
+        gcode.respond_info("%s E step distance: %f" % (self.name, step_dist))
+    cmd_SET_E_STEP_DISTANCE_help = "Set extruder step distance"
+    def cmd_SET_E_STEP_DISTANCE(self, params):
+        gcode = self.printer.lookup_object('gcode')
+        dist = gcode.get_float('DISTANCE', params, 0.)
+        self.stepper.set_step_dist(dist)
+        step_dist = self.stepper.get_step_dist()
+        gcode.respond_info("%s E step distance set: %f" %
+                                           (self.name, step_dist))

ld3300 added 2 commits March 22, 2020 18:42
…set version of the command when no 'DISTANCE' provided.
…atics to stepper.py. Remove limitation to value of step distance change
@ld3300
Copy link
Contributor Author

ld3300 commented Mar 22, 2020

@kakou-fr I think that would be a good feature and would probably be useful to a lot of people. It falls outside of this pull request, and will likely be outside my abilities (though I will give it a look). Please start a new issue for the request with a good description of how you would expect it to perform. This feature will allow setting the step distance for any extruder by name, so in that aspect it should be helpful.

@kakou-fr
Copy link
Contributor

@ld3300 I am waiting the final version of your patch, redo test, and after I will post an issue for mixing If the final version doesn't work

@ld3300
Copy link
Contributor Author

ld3300 commented Mar 25, 2020

Okay, I've made those changes, and everything seems to be working for the original intent. I was hoping it would also work for #1631, but doing a negative step distance causes klipper to crash pretty hard when a move is attempted. The log also just cuts off. Included if it gives any clues.
klippy.log
I'm not sure where to look for the source of that issue.

@KevinOConnor KevinOConnor merged commit d4bf612 into Klipper3d:master Mar 28, 2020
@KevinOConnor
Copy link
Collaborator

Thanks. FYI, I also made a couple of minor changes on top (commit 336af2f and 7717758).

-Kevin

@ld3300
Copy link
Contributor Author

ld3300 commented Mar 28, 2020

Thank you.

@ld3300 ld3300 deleted the gcode_e_steps_dist branch March 28, 2020 14:12
Delsian pushed a commit to Delsian/klipper that referenced this pull request May 1, 2020
dmbutyugin pushed a commit to dmbutyugin/klipper that referenced this pull request Sep 12, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2022
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.

3 participants