search

CR6Community / Marlin

This Marlin fork has the goal of cleaning-up the source code changes for the CR-6 so it can be merged upstream. We also want to extend the functionality to make it fully functional
GNU General Public License v3.0
467 stars 83 forks source link

[FR] Add control of retraction length for "Print Test Pattern" menu option #300

Open ritchiedc opened 2 years ago

ritchiedc commented 2 years ago

Is your feature request related to a problem? Please describe.

The "Print Test Pattern" menu option via the touch screen assumes a 4mm retraction length. This makes the option unusable for printers with direct drive upgrades and can cause clogging of the hotend.

Are you looking for hardware support?

No response

Describe the feature you want

Allow the user to control retraction length. For example, this could be accomplished by adding it to the screen or by using the firmware retraction setting.

If using the firmware retraction setting, implementation of issue #296 "Make Firmware Retraction Settings Available from the SETUP menu" would be advisable. Otherwise, one would have to start a print to change it and then do a settings change such as z offset that would initiate an M500 to save it to eeprom.

If modifying the screen to add the option, consider adding other G26 options such as bed and nozzle temperature.

Thinkersbluff commented 2 years ago

I have also been experimenting with making simple edits to MeshValidationHandler.cpp, in the 6.1 Final release, to explore the potential value of that feature. To use it safely with my Dragon HF/Orbiter CR6-SE, I made this one edit: @ line 72 Add: sprintf_P(gcodeBuffer, PSTR("G90\nG0 X0\nG26 B%d H%d R Q1 P15 X117 Y117"), bed_temperature, nozzle_temperature); Delete: sprintf_P(gcodeBuffer, PSTR("G90\nG0 X0\nG26 B%d H%d R Q4 P2 X0 Y0"), bed_temperature, nozzle_temperature);

I believe the change from Q4 to Q1 made the pattern "safe" to print, by reducing the retraction value from 4mm to 1mm and reduced the "over-extrusion" effect, by reducing the excess extrusion generated by the retract length x 1.2 "restore" factor x 4.

I have also modified G26 on my system, to reduce the restore factor from 1.2 to 1.0, but that would likely be an upstream change, and the impact of that change when Q=1.0 is difficult to see. I think it is worth making the above change to Q1 for DD users of CF6.1, whether G26 is modified or not.

Editing & compiling the firmware may be a "simple" thing for many folks who are capable of converting their printer to a DD configuration, but it may also be helpful to make the Q factor a programmable Setting of either 1 or 4, by enabling a user to declare whether their system is configured as a Bowden (Q=4) or a Direct Drive (Q=1) machine.

NOTES:

  1. If G26 made proper use of the retraction function, it should also be specifying a retraction speed that is not 15mm/s (If I am understanding the code correctly, G26.cpp seems to use the same feedrate as its programmed print speed, for retract/restore movements, and there is no support for a separate Z-Hop movement programmed into G26..) We may find that Q1 is no less effective on Bowden systems than it is on DD systems, since retraction does not seem to be helping stop the nozzle from pulling the filament up at the end of every arc.
  2. The change of origin from 0,0 to 117,117 fixed the problem that the front left pad always got trashed and knocked off my bed. This way, the pattern starts with one of the full circle pads, and any drool/priming residue carried to the starting point by the nozzle is too little to completely trash the pad it draws.
  3. The change of priming amount from 2mm to 15mm gave me additional time to get my tweezers into place and a larger mass to yank the thread away from the nozzle, as it descended for the starting stroke. The resulting pattern was literally "perfect", for the first time in my experience with this feature on this printer.
ritchiedc commented 2 years ago

I would suggest modifying MeshVallidationHandler.cpp line 72 so that there is still a common build between bowden and direct drive and no special build by the user would be required. This can be accomplished as follows:

sprintf_P(gcodeBuffer, PSTR("G90\nG0 X0\nG26 B%d H%d R Q%.1f P2 X0 Y0"), bed_temperature, 
    nozzle_temperature, fwretract.settings.retract_length);

In addition the following include would be required,

include "../../../../feature/fwretract.h"

This method would use the firmware retraction length which is saved in eeprom for the retraction length making the MeshValidationHandler drive type agnostic.

Thinkersbluff commented 2 years ago

In case it helps others contribute their thoughts, here:

Using any retraction length different from the default length requires using the Q switch in the G26 command string, to override that default.

According to the comment on this #define in the current Community Firmware Configuration.h, a retraction length of 1.0mm is supposed to be the default standard retraction used by G26: "#define G26_RETRACT_MULTIPLIER 1.0 // G26 Q (retraction) used by default between mesh test elements."

(Perhaps to ensure this version of G26.cpp will work even without the above line in Configuration.h?) G26 also defines that same parameter and gives it the same value of 1.0, if the variable has not been defined:

"#ifndef G26_RETRACT_MULTIPLIER

define G26_RETRACT_MULTIPLIER 1.0 // x 1mm

endif"

The G26.cpp file includes this explanation of Q: " * Q # Multiplier Retraction Multiplier. Normally not needed. Retraction defaults to 1.0mm and un-retraction is at 1.2mm These numbers will be scaled by the specified amount"

Internally, G26.cpp defines an internal variable, for use in their retraction calculations: "float g26_retraction_multiplier,"

If G26 finds a Q parameter in the command string, it tests for that value being between 0.05 and 15.0, here:

" if (parser.seen('Q')) { if (parser.has_value()) { g26_retraction_multiplier = parser.value_float(); if (!WITHIN(g26_retraction_multiplier, 0.05, 15.0)) { SERIAL_ECHOLNPGM("?Specified Retraction Multiplier not plausible."); return; }"

The G26.cpp retraction instructions seem to be here: "void retract_filament(const xyz_pos_t &where) { if (!g26_retracted) { // Only retract if we are not already retracted! g26_retracted = true; move_to(where, -1.0f * g26_retraction_multiplier); } }"

The recover (un-retract) instructions seem to be here: "void recover_filament(const xyz_pos_t &where) { if (g26_retracted) { // Only un-retract if we are retracted. move_to(where, 1.2f * g26_retraction_multiplier); g26_retracted = false; } }"

NOTE that G26.cpp does not use a separate variable for the un-retraction (recover) distance multiplier, so it will enforce a ratio of 1:1.2, regardless of which value is used as the g26_retraction_multiplier. When the Community Firmware uses a Q of 4, that results in an over-extrusion of 0.8mm at the begining of each new stroke, on a direct-drive printer. I suspect that accounts for some reports that the current implementation may overextrude.

With my limited "code literacy", I have not figured-out where G26.cpp assigns the value of G26_RETRACT_MULTIPLIER to the internal variable g26_retraction_multiplier.

Thinkersbluff commented 2 years ago

I would suggest modifying MeshVallidationHandler.cpp line 72 so that there is still a common build between bowden and direct drive and no special build by the user would be required. This can be accomplished as follows:

sprintf_P(gcodeBuffer, PSTR("G90\nG0 X0\nG26 B%d H%d R Q%.1f P2 X0 Y0"), bed_temperature, 
    nozzle_temperature, fwretract.settings.retract_length);

Have you tested this? I ran into a compile error when I first tried using G26_RETRACT_MULTIPLIER this way. I had to change that to int(G26_RETRACT_MULTIPLIER), to get past a compiler check of variable type.

ritchiedc commented 2 years ago

Yes I've tested it and it works. On your previous comment variables and defines are case sensitive. Defines are compiler directives which instruct the compiler to replace the define name, in this case G26_RETRACT_MULTIPLIER, with the following code which in your example is 1.0. That way you can use the define name multiple places in your code and only have to change the value in one place, conveniently located in Configuration.h.

Specifically, the variables g26_retraction_multiplier and g26_recovery_multiplier are initialized in G26.h at lines 684 and 685.

Thinkersbluff commented 2 years ago

This method would use the firmware retraction length which is saved in eeprom for the retraction length making the MeshValidationHandler drive type agnostic.

What are the default fwretract settings? The firmware is not aware of the drive configuration, so I am wondering whether their defaults assume Bowden also. Not everyone uses the Slicer Override feature or configures their slicer to use FW retract.

Thinkersbluff commented 2 years ago

Specifically, the variables g26_retraction_multiplier and g26_recovery_multiplier are initialized in G26.h at lines 684 and 685.

Ahh. Thank you! Now I have another bit of "code literacy" to work with.

Where is G26.h located? (I don' see it under Marlin/src/gcode/bedlevel, with the .cpp file, and I don't see it being included at the top of G26.cpp.)

ritchiedc commented 2 years ago

The defaults firmware retraction settings are in Configuration_adv.h (search for FWRETRACT). In my opinion, if anyone can compile their own built they can set the firmware retraction settings, even if it means using M207 followed by M500.

For the Print Test Pattern menu command (which executes MeshValidationHandler.cpp) to run neither Slicer Override nor Slicer Firmware Retraction need be set for the firmware retraction length to be used.

ritchiedc commented 2 years ago

I'm sorry, my mistake not proof reading. It is G26.cpp

Thinkersbluff commented 2 years ago

neither Slicer Override nor Slicer Firmware Retraction need be set for the firmware retraction length to be used.

No, I understand that. I am more concerned that those who are unaware that this function is influenced by the fwretract settings might have problems debugging their nozzle clogs.... That is one reason I personally prefer that the Q factor be the one defined as such in Configuration.h. Getting the Bed Mesh Validation feature to use the slicer override function, when printing the mesh validation pattern could be one benefit of adding access to the Tune menu while that pattern is printing, if only G26 used those settings for travel speed, print speed, retraction & recovery speed, etc., but it doesn't.

Thinkersbluff commented 2 years ago

BTW - have you tested the benefits of adjusting retraction distances, when using the mesh validation pattern? I could see it being very desireable, if tweaking the parameter while the pattern is printing would make a visibly identifiable difference. My pattern only ever shows one string and that is the thread that it pulls as it rises at the end of the whole thing.
Do you think maybe it does not retract on that final stroke, or that retraction may not be helping as currently coded? It might help to be able to quantify and demonstrate a benefit to using the fwretract settings, rather than a fixed value of 1.0 for everybody.

ritchiedc commented 2 years ago

I am more concerned that those who are unaware that this function is influenced by the fwretract settings might have problems debugging their nozzle clogs

On the flip side having to make your own build in order to use the pattern from the UI is not an obvious requirement with changing your drive setup. At least the warning could be changed to set the length using M207 and no build required.

BTW - have you tested the benefits of adjusting retraction distances, when using the mesh validation pattern?

I have not tested it. I quit using it when I realized it was retracting too much for my direct drive setup. Fortunately I didn't clog the heat break.

It might help to be able to quantify and demonstrate a benefit to using the fwretract settings, rather than a fixed value of 1.0 for everybody.

In my opinion while a fixed length of 1 would help you and I, it is not a good option since the majority of the community uses the stock or near stock bowden setup. I am assuming that even 4mm would be more beneficial to the stock setup.

ritchiedc commented 2 years ago

I can't ignore that the current configuration works for the stock setup and Sebazzz added the warning for people who read warnings (unlike me). I don't want to reduce the capability for the stock configuration. Of course I'm assuming that it would not work as well with 1mm for a bowden setup.

Thinkersbluff commented 2 years ago

Of course I'm assuming that it would not work as well with 1mm for a bowden setup.

Yeah, that’s why I asked about testing. The release notes say the feature is experimental and may over extrude. I was never impressed by the quality of the pattern I could generate with it, with any of the extruder/hotend combinations I have tried.

No offence intended, but my feeling is if we just assume that 4mm retraction generates a better quality pattern than 1mm would, we might be missing out on an opportunity to open this feature to everyone without actually downgrading its performance for anybody.

If anyone following this discussion is willing to test the impact of this change on a purely stock machine, please comment below.

Thinkersbluff commented 2 years ago

So this is how FWRETRACT is defined in Configuration_adv.h:

Thinkersbluff commented 2 years ago

So the FWRETRACT RETRACT_LENGTH default is defined as 6.5mm in Configuration_adv.h, but the G26_RETRACT_MULTIPLIER is defined as 1.0 (x1mm) in Configuration.h and the current MeshValidationHandler switches G26 to use a value of 4mm.

If the change is implemented as proposed, then users like I - who ignore the FWRETRACT function - would be at higher risk of a nozzle clog if we pressed the Print Mesh Validation button, and Bowden users would be getting a retraction of 6.5mm and a recovery of 7.8mm, not 4 and 5 as per those words on the screen.

Thinkersbluff commented 2 years ago

I have not tested it. I quit using it when I realized it was retracting too much for my direct drive setup.

I thought you might have incorporated your proposed change and tested it on your own printer, to compare the benefits of having a variable retraction length value, rather than just a fixed value of 1.0. If this pattern can be used to fine-tune and validate the fwretraction settings, that would probably be a desireable improvement.

ritchiedc commented 2 years ago

Sorry for the confusion. Per an earlier comment I have tested my proposed change and it worked. What I was referring to in my last comment was that I have not tried comparing different lengths on bowden since I now have DD. IE, checked to see if there is a difference in quality on the bowden setup between 4mm and 1mm. I assume that there was testing by the originator of MeshValidationHandler to arrive at the 4mm number.

The ideal solution would be to add the retraction length to the screen but I haven't learned how to modify the screen yet.

Thinkersbluff commented 2 years ago

Sorry for the confusion. Per an earlier comment I have tested my proposed change and it worked.

Oh, ok My question then is whether you also experimented with evaluating the benefits of a user being able to dynamically vary that retraction value (e.g. while printing the pattern). I realize you would only be able to simulate that by editing the setup and rerunning the whole pattern, for now, but if you can see a difference, maybe this mod extends the usefulness of the pattern a little more.

I assume that there was testing by the originator of MeshValidationHandler to arrive at the 4mm number.

I don’t know how 4.0 was originally chosen. Maybe @Sebazzz will fill us in on that.

The ideal solution would be to add the retraction length to the screen but I haven't learned how to modify the screen yet.

I feel you on this one. I have made changes to the screens before. Never easy, and now there is an open question about which version of the DGUS tools to use and whether to migrate everything to the new tool before making any more mods, so that option may be out of reach for the moment.

ritchiedc commented 2 years ago

whether you also experimented with evaluating the benefits of a user being able to dynamically vary that retraction value (e.g. while printing the pattern).

Changing the value dynamically is a little more problematic. Currently the only way to change the value of the multiplier is thru the G26 command which as currently implemented can't be executed mid-print (I think, based on my 3 minute analysis). I believe a new command won't be executed until prior queued commands are completed. Right now, due to emergent family medical issues, I don't have a lot of extra time to pursue that.

I do have a version that I have not tested that allows specifying the recovery multiplier independent of the retraction multiplier. This is a change to G26 which would be an upstream change. This change I have to think about some more to make sure both multipliers work together without problems, eg, what do I do with retraction if only recovery is specified, etc. Right now I set recovery (a new 'N' parm) to retraction*1.2 if recovery is not specified. Also, I put out an error if recovery is specified without retraction. I know this isn't part of this issue but wanted to get your thoughts.

Thinkersbluff commented 2 years ago

Changing the value dynamically is a little more problematic. Currently the only way to change the value of the multiplier is thru the G26 command which as currently implemented can't be executed mid-print (I think, based on my 3 minute analysis). I believe a new command won't be executed until prior queued commands are completed.

Agreed. Got ahead of myself, in the language. Adding access to some or all of the "Tune" menu is in the back of my head, as a possible future PR, building on this one.

I thought you might have used the Print->Tune-> Slicer Override, or might have built several versions of firmware.bin, with different retraction length values, to experiment with different retraction lengths on sequential runs of the validation pattern.

I am wondering whether using values between 0.1 (the minimum value for FWRETRACT_LENGTH and 1.0 (notionally the max value for DD machines with all-metal hotends) makes any discernible difference to the quality of the printed test pattern on a Direct-Drive machine, or on a Bowden machine with an all-metal hotend.

If it helped users to "fne-tune" their retraction settings, I would think that a benefit. If users get the same quality of pattern whether they use 0.1 or 6.5, on a stock/Bowden system, then I think it better to "open" this feature up to all of our users, by reducing the default retraction length to 1.0 for this function, which seems to be the current default setting in Configuration.h for all other Marlin users, whether Bowden or Direct Drive, according to the files in Marlin/config/examples. (e.g. CR10S V3)

Thinkersbluff commented 2 years ago

I do have a version that I have not tested that allows specifying the recovery multiplier independent of the retraction multiplier. This is a change to G26 which would be an upstream change. This change I have to think about some more to make sure both multipliers work together without problems, eg, what do I do with retraction if only recovery is specified, etc. Right now I set recovery (a new 'N' parm) to retraction*1.2 if recovery is not specified. Also, I put out an error if recovery is specified without retraction. I know this isn't part of this issue but wanted to get your thoughts.

I have actually modified the G26.cpp file on my machine to change the 1.2f to 1.0f in the recover routine. I was also wondering whether to float a PR upstream to investigate where the 1.2 factor comes from and whether the Marlin team would be willing to make that same change to G26.cpp or whether they would prefer to add a G26_RECOVER_LENGTH parameter to Configuration.h, and a new switch to G26.cpp, to allow that flexibility without impacting existing users.

In my head, I would also expect to want control over the retraction and recovery speeds as well, if I was going to start trying to fine-tune the performance even further. And then there is Linear Advance...

It started "snowballing " in my head, so I tried thinking the opposite way - what if there was no retraction at all? Would the pattern look any different?

Unless/until we got some volunteer testers from our Community, it seemed a bit risky to float that idea yet, so I started thinking more like, "suppose we gave everybody control over the retraction length factor used by this function on their printer?"

Since "everybody" would be able to use the display menus, but not '"everybody" can compile their own firmware, I started by asking whether Q=1.0 works for everybody, while simultaneously investigating how to add retraction length as a third user-selectable variable on the Print Test Pattern screen.

I am quite comfortable with the idea of floating a series of modifications, which gradually increase the sophistication of our users' degree of control over this function.

I was often "chided" by my peers to "keep it simple", when brainstorming this type of change. Doing my best to "comply" with that "directive", even though I am long since retired and free to make things as simple or as complicated as I wish, on my own system.

ritchiedc commented 2 years ago

The more I look at it the more I wonder why G26 retraction was set as a multiplier. I think the reason the default retraction length is commonly set to 1mm is so that the "multiplier" becomes the length. But that is a thought with for which I have no evidence.

Thinkersbluff commented 2 years ago
  1. This issue from 2020 may answer where G26_RETRACT_MULTIPLIER came from, though it is not clear to me whether/how the original request to "Allow to set retraction for UBL mesh test" has been addressed by what was done. Maybe the idea was/is to allow users to modify the value used on their systems, by editing Configuration.h.

https://github.com/MarlinFirmware/Marlin/pull/16511

Maybe more was done & I am just having trouble finding it? e.g. Maybe the UBL Wizard added to the Marlin LCD menu was the more complete response?

  1. I see in the photos that the G26 pattern was a bit more sparse back then, but clear signs of stringing at 1mm instead of the 5mm the author of that issue said he needed. This Issue, https://github.com/MarlinFirmware/Marlin/pull/10695, supporting ARC SUPPORT if available may explain that change.