[2.0.x] Move firmware retraction to the planner

[tcm0116]

Description

Addresses #11512

The same issue exists in 1.1.x, so it could be applied there as well.

[thinkyhead]

This approach seems fine, though in the bigger picture I wonder how compatible it (and fw-retract in-general) is with the recover from power-loss feature. It currently doesn't save the retract flag, so if power goes out in-between a retract and a recover, the recover won't happen and the Z position might be off by the z-hop amount when the print resumes. Anyway, that feature needs other work too, but it's something to think about going forward.

[thinkyhead]

Every operation in the planner needs to be reversible for the benefit of converting stepper positions back into a cartesian position. So if there's an apply_retract there should also be unapply_retract. Since we already have apply_leveling and unapply_leveling the most consistent way to manage it is to move this code into the apply_leveling method (or call it from that method) and rename that method apply_modifiers. Then rename unapply_leveling to unapply_modifiers and add code there which removes the hop value. So far the (un)apply_leveling methods don't do anything with E, but if generalized to (un)apply_modifiers then it would be ok to have E modification there too.

UBL doesn't use apply_leveling or unapply_leveling at this time (except for delta), because it was written in its own sandbox, so the separate handling would still be needed in ubl_motion.cpp.

[thinkyhead]

Example: If apply_retract were added to the suggested planner.apply_modifiers then this wouldn't be needed here.

[tcm0116]

UBL doesn't use apply_leveling or unapply_leveling at this time

This seems to be the one exception to that rule as apply_leveling is called a few lines further down if HAS_UBL_AND_CURVES is defined.

I agree that combining apply_leveling and apply_retract is the right thing to do, but I was concerned that it might open a can of worms trying to do so. Perhaps it would be best to keep this PR as is to address the issue at hand (i.e. the print becoming skewed when using firmware retraction) and perform the consolidation in a separate PR.

[thinkyhead]

…or here.

[thinkyhead]

…etc…

[thinkyhead]

…etc…

[thinkyhead]

☝️

[AnHardt]

Would make me wonder if we wouldn't have a similar problem like this on Prusa.

[tcm0116]

@AnHardt #9384 and #9390 were actually created to specifically address that issue. I have a nice gouge in my PEI because of it :-(

[tcm0116]

I wonder how compatible it (and fw-retract in-general) is with the recover from power-loss feature.

As the power-loss feature appears to only save current_position, then I would think that your assertion that the Z-hop would be lost is true and the nozzle would end up being offset by the Z-hop value. In addition, the filament would not be restored, so there would be no material extruded for a while after the print is resumed. To make the power-loss feature and firmware retraction compatible, then I think we'd have to save off FWRetract::current_retract and FWRetract::current_hop (FWRetract::retracted can be derived from that information).

Given the above, I actually think it would be easier to make Firmware Retraction compatible with the power-loss feature with this change as the previous method worked by hiding the fact that the retraction and Z-hop had occurred. While FWRetract::retracted could have been saved, saving FWRetract::hop_amount wouldn't have worked as it wasn't actually ever used due to the static hop_amount variable declared in FWRetract::retract(). Furthermore, if the user had changed the retraction and/or swap distances during the print, then just saving FWRetract::retracted would not have been sufficient to know how much filament was actually retracted by each extruder, which could have led to under or over extrusion on the resume.

[thinkyhead]

Although on its face this commit moving feedrate scaling into the planner would seem to be a great idea, since it simplifies and consolidates this logic, it requires fr_mm_s / millimeters to be re-calculated on every segment, whereas previously this division was only done once, at the start of the line segmentation loop. So this updated code will be a bit slower.

[tcm0116]

I hadn't thought about losing the optimization of pre-computing fr_mm_s / millimeters for every segment by moving the feedrate scaling into the planner. If it's a concern, one option could be to pass it as an optional argument to buffer_line in the same way as millimeters. Alternatively, buffer_line could keep track of the previous fr_mm_s, millimeters, and the scaled feedrate, and only re-compute the scaled feedrate if fr_mm_s or millimeters changes.

[thinkyhead]

I thought about that. Passing an extra float parameter on every segment is definitely going to be less costly than having a division operation. Another option which I've been contemplating would be to get #8311 done, and then presumably the high-level feedrate variables would already contain the duration of the move instead of the speed of the move. But, I'm not sure this would ultimately reduce the amount of calculation needed for kinematic segments.

[thinkyhead]

I suggest we have the extra parameter only for IS_KINEMATIC, and do the calculation in the Planner only for IS_CARTESIAN.

[tcm0116]

UBL had its own implementation of feedrate scaling for SCARA in ubl_buffer_segment_raw. I ended up removing and replacing that function with calls to planner.buffer_line as they ended up being pretty much exactly the same. Sounds like I just need to remove those unused variables.

[thinkyhead]

Rebased and squashed.

I'm still waiting for feedback from @JxpA that this is 100% copacetic. But, Thomas, you've got a delta, right? Since you're see nothing weird with this update I'm inclined to merge it sooner rather than later. If there are any oddities we can fix them up in the followup round….

[tcm0116]

I do have a delta machine, which I was using to verify that the fixes worked. However, I'm a bit concerned with the report that @JxpA provided. It's possible that something happened during one of the rebases, so I'll do some more testing tonight.

[thinkyhead]

A minor warning. The current_position does not have to be updated on a per-segment basis, and there may be code that calls this multiple times without ever updating current_position until the full move has been done. So we should check every call to buffer_segment to rule out this possibility.

[tcm0116]

I've been thinking about that. One solution would be to only allow motion commands to call planner.buffer_line and not planner.buffer_segment. The planner could then keep a copy of the current Cartesian position, which is updated at the end of each call to buffer_line.

I believe I've already accomplished this on kinematic configurations, but UBL still calls buffer_segment for Cartesian machines.

[tcm0116]

@thinkyhead - I just amended the commit to add planner_cart for kinematic machines only. Since current_position was only being used by kinematic machines, and they were already only using buffer_line, I think having the Planner track the current cartesian position mitigates your concern and should allow for accurate calculation of the delta between the target and current cartesian positions.

[tcm0116]

@thinkyhead - I've found an issue with this PR related to the original intent of fixing firmware retraction. It currently doesn't make use of recover lengths and only recovers what was initially retracted. I'll have to think about how I'm going to deal with that.

[tcm0116]

@thinkyhead - I've resolved the issue with the retract recover, so I believe this PR is good to go now.

[thinkyhead]

Rebased and squashed. If it passes Travis testing then it will be merged shortly.

[thinkyhead]

@tcm0116 — What is your opinion of #6295 (comment) and should we add the proposed change?

[tcm0116]

@thinkyhead I think the discussion in #6295 makes sense in that retractions can typically be performed at higher rates than normal extrusion. We do currently have a separate max velocity for travel vs non-travel movements, so there is precedence. I think it might also be a good idea for filament loading and unloading to be able to use the faster feedrate when pushing/pulling filament through the Bowden tubes.

[thinkyhead]

Perhaps I misunderstand, but don't we already have separate configurable feedrate settings for filament change?

[tcm0116]

I think they want to have separate configurable limits for the extruder for when it's actually pushing filament through the hot end and when it's not. During normal extrusion, Marlin would enforce the first limit (which is presumably lower), and would enforce the second when doing things like retraction and loading/unloading filament, which can be faster due to the lack of back pressure on the filament.

I currently have my max extruder speed at 50mm/s, but I know there's no way it could achieve that while actually pushing filament though the hot end, but I guess I just assume the slicing process will result in reasonably limited feedrates.

[thinkyhead]

Ah, I see. So, for these moves, a temporary removal of the speed limit in front of planner.buffer_line for the unconstrained E move, then restore the max feedrate immediately after. It should work, since planner does the feedrate constraint, and stepper ISR should obediently execute the given block.