Skip to content

Conversation

tcm0116
Copy link
Contributor

@tcm0116 tcm0116 commented Jun 21, 2017

This is a rebase and rework of #6493 to work with the new advanced pause capability.

I've extensively tested this on my dual-nozzle Prusa i3 clone with a 20x4 LCD.

@thinkyhead
Copy link
Member

thinkyhead commented Jul 2, 2017

The conflict is with M702. The code was already brought in from Prusa firmware to Unload All extruders (but it's not completed). Perhaps there's a standard that can be applied to both the Prusa usage and this usage. The RepRap wiki is ambiguous about which filament(s) are unloaded.

For the MK2 multiplexer the procedure is interesting, probably runs this way to get the best filament pull…

      // MK2 firmware behavior:
      //  - Make sure temperature is high enough
      //  - Raise Z to at least 15 to make room
      //  - Extrude 1cm of filament in 1 second
      //  - Under 230C quickly purge ~12mm, over 230C purge ~10mm
      //  - Change E max feedrate to 80, eject the filament from the tube. Sync.
      //  - Restore E max feedrate to 50

MENU_ITEM(gcode, MSG_FILAMENTLOAD, PSTR("M701"));
#else
if (!thermalManager.targetTooColdToExtrude(0))
MENU_ITEM(gcode, MSG_FILAMENTLOAD1, PSTR("M701 T0"));
Copy link
Member

Choose a reason for hiding this comment

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

Please use MSG_FILAMENT_LOAD MSG_E0 here. Then there is no need for multiple redundant MSG_FILAMENTLOADn defines.

Copy link
Member

@thinkyhead thinkyhead left a comment

Choose a reason for hiding this comment

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

Some changes are needed, some suggested.

MENU_ITEM(gcode, MSG_FILAMENTUNLOAD, PSTR("M702"));
#else
if (!thermalManager.targetTooColdToExtrude(0))
MENU_ITEM(gcode, MSG_FILAMENTUNLOAD1, PSTR("M702 T0"));
Copy link
Member

Choose a reason for hiding this comment

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

Please use MSG_FILAMENT_UNLOAD MSG_E0 here. Then there is no need for multiple redundant MSG_FILAMENTUNLOADn defines.

UNUSED(e);
#endif
return allow_cold_extrude ? false : degTargetHotend(HOTEND_INDEX) < extrude_min_temp;
}
Copy link
Member

Choose a reason for hiding this comment

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

Might as well make an inline for the formula:

      FORCE_INLINE static bool tooCold(const int16_t temp) { return allow_cold_extrude ? false : temp < extrude_min_temp; }
      static bool tooColdToExtrude(const uint8_t e) {
        #if HOTENDS == 1
          UNUSED(e);
        #endif
        return tooCold(degHotend(HOTEND_INDEX));
      }
      static bool targetTooColdToExtrude(const uint8_t e) {
        #if HOTENDS == 1
          UNUSED(e);
        #endif
        return tooCold(degTargetHotend(HOTEND_INDEX));
      }

@@ -3588,12 +3627,15 @@ void kill_screen(const char* lcd_msg) {
*/
#if ENABLED(ADVANCED_PAUSE_FEATURE)

static AdvancedPauseMode advanced_pause_mode = ADVANCED_PAUSE_MODE_PAUSE_PRINT;
static uint8_t advanced_pause_extruder = active_extruder;
Copy link
Member

Choose a reason for hiding this comment

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

You can't use the value of another global as the initializer of a global.

default:
STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_PAUSE, true, true);
break;
}
Copy link
Member

@thinkyhead thinkyhead Jul 2, 2017

Choose a reason for hiding this comment

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

As this is repeated several times, it should be made into a macro…

#define _ADVANCED_PAUSE_HEADER()                                  \
  do { switch (advanced_pause_mode) {                             \
    case ADVANCED_PAUSE_MODE_LOAD_FILAMENT:                       \
      STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_LOAD, true, true);   \
      break;                                                      \
    case ADVANCED_PAUSE_MODE_UNLOAD_FILAMENT:                     \
      STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_UNLOAD, true, true); \
      break;                                                      \
    default:                                                      \
      STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_PAUSE, true, true);  \
      break;                                                      \
  } } while(0)

…and then they can all be replaced by:

_ADVANCED_PAUSE_HEADER();

default:
STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_PAUSE, true, true);
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

_ADVANCED_PAUSE_HEADER();

default:
STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_PAUSE, true, true);
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

_ADVANCED_PAUSE_HEADER();

default:
STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_PAUSE, true, true);
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

_ADVANCED_PAUSE_HEADER();

default:
STATIC_ITEM(MSG_FILAMENT_CHANGE_HEADER_PAUSE, true, true);
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

_ADVANCED_PAUSE_HEADER();

#endif
#ifndef MSG_FILAMENTLOAD1
#define MSG_FILAMENTLOAD1 _UxGT("Load filament 1")
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Only one MSG_FILAMENTLOAD is needed. Append MSG_E0, MSG_E1, etc., in the code if numbers are needed. For example:

MENU_ITEM(gcode, MSG_FILAMENT_LOAD MSG_E0, PSTR("M701 T0"));

@@ -602,6 +602,45 @@
#ifndef MSG_FILAMENTCHANGE
#define MSG_FILAMENTCHANGE _UxGT("Change filament")
#endif
#ifndef MSG_FILAMENTLOADUNLOAD
#define MSG_FILAMENTLOADUNLOAD _UxGT("Filament Un/Load")
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This menu could also be called Change Filament — or these options could simply be included in the existing Change Filament menu, since load and unload are each just half of a filament change.

@tcm0116
Copy link
Contributor Author

tcm0116 commented Jul 7, 2017

The RepRap wiki is ambiguous about which filament(s) are unloaded.

I could make this a configurable option, such that no parameter either unloads the active extruder, or unloads all extruders that are above the minimum temperature. I could also add an unload all option to the LCD.

For the MK2 multiplexer the procedure is interesting, probably runs this way to get the best filament pull…

How would you like me to proceed? This procedure is very specific to the MK2, and I personally don't like the idea of changing max feedrates since that can cause issues with other printers.

My suggestion would be to first implement a generic process, and then printer-specific processes could be added as desired.

@Roxy-3D
Copy link
Member

Roxy-3D commented Jul 7, 2017

How would you like me to proceed? This procedure is very specific to the MK2, and I personally don't like the idea of changing max feedrates since that can cause issues with other printers.

Agreed... Even though the MK2 is providing the motivation to do this and this code is specifically aimed at that purpose... Probably the existing Configuration.h values can be used to handle both the generic case and the MK2 specific case.

My suggestion would be to first implement a generic process, and then printer-specific processes could be added as desired.

Agreed. It should be possible to do a generic version of the code. And maybe it works especially well on the MK2 ???

@tcm0116
Copy link
Contributor Author

tcm0116 commented Jul 7, 2017 via email

@Roxy-3D
Copy link
Member

Roxy-3D commented Jul 7, 2017

OK... I made a bad assumption! :) But clearly, it should be possible to make a generic version of this work with just about all machines! Thank You for the correction.

@tcm0116
Copy link
Contributor Author

tcm0116 commented Jul 8, 2017

@thinkyhead where did you find that procedure for the MK2? I didn't see anything like that in their fork.

@tcm0116
Copy link
Contributor Author

tcm0116 commented Jul 13, 2017

I've updated this to address @thinkyhead's comments. However, I've got a job going for a customer, so I haven't had an opportunity to do much testing of my changes. If anyone (cough @Roxy-3D cough) has some time to help me out with testing, that would be greatly appreciated.

@tcm0116
Copy link
Contributor Author

tcm0116 commented Jul 14, 2017

I had an opportunity to perform some testing this morning. I fixed an issue with ensure_safe_temperature and also an issue with PREVENT_LENGTHY_EXTRUDE when volumetric extrusion is enabled. I believe this PR is good to go now.

@Eddiiie
Copy link

Eddiiie commented Jul 15, 2017

Cool, this is what I was looking for.

It might be hard to make this a generic enough function for all printers but it seems an owner of a printer could formulate their own gcode commands to accomplish the task to their needs. What I mean to say is to create an item in configuration.h (or configuration_adv.h), similar to enabling case LED.. #define ShowFilamentLoadUnloadMenuItem
#ifdef ShowFilamentLoadUnloadMenuItem
GcodeLoadCommand1=M104 S208
GcodeLoadCommand2=M92 E0 ;zero the extruded length
GcodeLoadCommand3=G1 F200 E2000 ;extrude 2000mm of feed stock
GcodeLoadCommand4=G92 E0 ;zero the extruded length again
GcodeLoadCommand5=M104 S0 ;set temp to 0
...
GcodeUnloadCommand1=M104 S208
GcodeUnloadCommand2=M92 E0 ;zero the extruded length
GcodeUnloadCommand3=G1 F200 E-2000 ;retract 2000mm of feed stock
GcodeUnloadCommand4=G92 E0 ;zero the extruded length again
GcodeUnloadCommand5=M104 S0 ;set temp to 0

IgnoreFilamentRunoutSensor=1

Example..

@tcm0116
Copy link
Contributor Author

tcm0116 commented Jul 15, 2017

@Eddiiie M701 and M702 are pretty configurable in Configuration_adv.h. Is there a configurable option not there that you would like to see?

@Eddiiie
Copy link

Eddiiie commented Jul 15, 2017 via email

@tcm0116
Copy link
Contributor Author

tcm0116 commented Jul 19, 2017

@thinkyhead Have you had an opportunity to re-review this yet?

@thinkyhead thinkyhead changed the title Add M701 and M702 filament load and unload [1.1.x] Add M701 and M702 filament load and unload Nov 13, 2017
@thinkyhead thinkyhead force-pushed the bugfix-1.1.x branch 2 times, most recently from 54081a8 to 4e19c59 Compare November 16, 2017 07:18
@thinkyhead thinkyhead force-pushed the bugfix-1.1.x branch 2 times, most recently from 38898fa to 7c1adff Compare December 11, 2017 09:13
@ghost
Copy link

ghost commented Dec 17, 2017

I ask all , to make these commands in PERMANENT MODE , not in features , and for 1256 processors ... then to not be forced to enable ' PAUSE STRONG CODE ' just for loading/unloading
And to find an idea about the preload that must long and quick , like a lcd button click to accellerate , or any idea , to make a turbo a few moment , for bowden systems
A configurated script if you want , but also , a total user command for all that don't want to have a script and load only by lcd or gcode with gcode buttons

By LCD and Gcode , because of interface ' octoprint and ..... i'm dreaming , the marlin web interface '

@thinkyhead thinkyhead force-pushed the bugfix-1.1.x branch 3 times, most recently from 7c81e3c to b567790 Compare December 22, 2017 04:39
@thinkyhead
Copy link
Member

thinkyhead commented Dec 25, 2017

@tcm0116 Let's revisit this in light of the latest code changes where M600 now uses Park. In the meantime I'd like to also add Load and Unload items to the LCD menus because sometimes you just want one, the other, or both. Aaaannd… I also want to have unload/change deactivate the E stepper at the point when the filament is unloaded and waiting to be loaded, so the remainder can just be pulled out, if desired.

❄️ ❄️ ❄️ 🎄 ❄️ 🎄 ❄️ 🎄 ❄️ 🎄 🎄 ❄️ ❄️🎄
❄️ ❄️ 🎄 ❄️ Merry Saturnalia! ❄️ ❄️ 🎄 ❄️ ❄️🎄
❄️ 🎄 ❄️ 🎄 ❄️ 🎄 ❄️ ❄️ 🎄 ❄️ 🎄 ❄️ 🎄 ❄️

@thinkyhead thinkyhead force-pushed the load_unload branch 8 times, most recently from 8e6deba to 2fcc15d Compare December 30, 2017 10:36
@thinkyhead
Copy link
Member

Rebased and squashed. Moved general improvement changes out to #8965.

@tcm0116
Copy link
Contributor Author

tcm0116 commented Dec 30, 2017

Anyway, for now, without an LCD controller we'll just loudly document that M108 is a substitute click.

I did add echo messages when user interaction is required:

#define MSG_FILAMENT_CHANGE_HEAT            "Press button or send M108 to heat nozzle"
#define MSG_FILAMENT_CHANGE_INSERT          "Insert filament and press button or send M108"

If you want, we can define them based on if an LCD controller is available, emergency parser is available, or both. That would probably make it less confusing and prevent someone from trying to use M108 when they don't have emergency parser enabled.

@thinkyhead
Copy link
Member

I did add echo messages when user interaction is required:

The message about M108 is helpful for users I suppose, but ideally this code shouldn't need to be known by users. Instead, the host would send M108 in response to the user clicking a "Continue" button. The host would display "Continue when ready," or something generic like that. It's something we can propose to the host developers group in the near future.

@thinkyhead
Copy link
Member

Now to package this up for 2.0.x . . .

@tcm0116
Copy link
Contributor Author

tcm0116 commented Dec 31, 2017

Give me a few minutes. I'm modifying M701 and M702 to use tool_change if the specified tool is different than the active tool. However, my RRDFG LCD is locking up when I power on the printer.

EDIT: Sigh. It helps if I re-enable that option in the configuration...

@thinkyhead
Copy link
Member

Good improvements. Rolling them in…

@thinkyhead thinkyhead merged commit 1e18d71 into MarlinFirmware:bugfix-1.1.x Dec 31, 2017
@thinkyhead
Copy link
Member

This is merged to "finalize" the basic form of everything. I'm putting together a PR for 2.0.x with the same changes, and should have that posted today.

@tcm0116
Copy link
Contributor Author

tcm0116 commented Dec 31, 2017

Good deal. Thanks for helping me take this over the goal line.

@thinkyhead
Copy link
Member

Happy to assist — especially with features that are long overdue.

Much thinking about a proper LCD methodology to simplify some things. I'm considering using internal LCD-only G-codes (e.g., MENU_ITEM(gcode, ..., PSTR("@100"))) for commands that should block the command parser while they execute. The M600 command is a good example of a command that already works this way.

For the most part the LCD code works smoothly now, and the menu system is pretty nifty (if baroque). So the first step toward improving the LCD code is just to (yes) split it up into more manageable parts that compile independently, and then see what independent units stand out. So that's something I will undertake soon.

@tcm0116 tcm0116 deleted the load_unload branch December 31, 2017 19:28
@marcio-ao
Copy link
Contributor

Hi, it appears as if ticket #8993 is related to this PR. I just thought I would reference it here to help getting the right eyeballs to look into it.

@marcio-ao
Copy link
Contributor

It looks like the bug has been largely resolved, but I notice that lcd_temp_menu_e0_filament_change() is being defined in line 4145 of "ultralcd.cpp" while it is first used in line 1393. This will likely cause issues when compiling with "avr-gcc". I suggest moving the declarations of the lcd_temp_menu_eN_filament_change() functions above lcd_tune_menu().

@thinkyhead
Copy link
Member

Thanks! Resolved by #9032.

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.

6 participants