Unify M600 and M125 pause features #6407

tcm0116 · 2017-04-21T04:30:27Z

@Roxy-3D requested the ability to prime the nozzle when resuming a print. While looking into this request, it occurred to me that the recently added M125 code was was almost copy-and-pased from M600. As such, I decided it would be helpful to merge them together into a single implementation in order to have the flexibility to provide the benefits of both without a bunch of code duplication.

Renamed the FILAMENT_CHANGE_FEATURE to ADVANCED_PAUSE_FEATURE.
Added U parameter to M600 to allow for specifying the unload length separately from the load length.
Added the B parameter to M600 to allow for specifying how many times to beep when the reload is ready.
Disabled beeping for M600 when called from the LCD.
Moved the heater timeout controls to ThermalManager to provide for a more generic implementation.
Modified the LCD to flash the target temperature if the heater has been put into an idle state.
As part of the unification, M24 will now wait for the heaters to re-heat and present the user with the option for priming the nozzle (if equipped with an LCD).

I tried to give this a fair amount of testing, but I only have one printer with a 20x4 LCD. As such, it would be helpful if this could get some more testing on different configurations, especially the target temperature idle flashing on the DOGM display when the heaters have become idle after pausing.

thinkyhead · 2017-04-21T04:39:52Z

Marlin/Marlin_main.cpp

+    }
+  }
+
+  static bool pause_print(float retract, float z_lift, float x_pos, float y_pos,


For float arguments on AVR, const float & is preferred whenever possible.

thinkyhead · 2017-04-21T04:41:45Z

Marlin/Marlin_main.cpp

-      #if FILAMENT_CHANGE_LOAD_LENGTH > 0
-        + FILAMENT_CHANGE_LOAD_LENGTH
+    const float load_length = code_seen('L') ? code_value_axis_units(E_AXIS) : 0
+      #if defined(FILAMENT_CHANGE_LOAD_LENGTH)


#ifdef should be used instead of #if defined(...) for cases like this.

thinkyhead · 2017-04-21T04:42:04Z

Marlin/Marlin_main.cpp

-    #endif
-    stepper.synchronize();
+    const int beep_count = code_seen('B') ? code_value_int() :
+      #if defined(FILAMENT_CHANGE_NUMBER_OF_ALERT_BEEPS)


#ifdef…

thinkyhead · 2017-04-21T04:44:06Z

This would be a good opportunity also to unify this with NOZZLE_PARK_FEATURE.

tcm0116 · 2017-04-21T04:50:42Z

This would be a good opportunity also to unify this with NOZZLE_PARK_FEATURE

I could see how it could make use of that feature for moving to the park location. I'll look into the idea.

tcm0116 · 2017-04-21T13:09:22Z

@thinkyhead Can you provide any guidance on what you're thinking with the NOZZLE_PARK_FEATURE?

thinkyhead · 2017-04-30T22:45:10Z

We can get into it more after the release.

tcm0116 · 2017-05-02T04:36:13Z

@thinkyhead I understand if you want to push this off to the next release. However, I think there are some fixes in this pull request that would be good to include in this release. For example, this pull request fixes an issue with the Filament Reload screen that was modified to show the extruder temperature, but didn't actually update. Also, I fixed several issues related to cancelling a SD print, such as not shutting off the heaters or fans.

Would you like for me to extract some of the smaller bug fixes and submit them as separate pull requests?

Roxy-3D · 2017-05-02T05:07:32Z

I understand if you want to push this off to the next release. However, I think there are some fixes in this pull request that would be good to include in this release.

@tcm0116 I've been super busy looking for an elusive bug. But I think I have that one figured out and fixed. What can I do to help?

tcm0116 · 2017-05-02T05:46:12Z

@Roxy-3D not sure. If @thinkyhead wants me to extract the few small bug fixes from this pull request, I can get that taken care of no problem.

Maybe it's about time to start having an architecture discussion. I'm thinking that @thinkyhead is wanting to move some of these types of features into the Nozzle class, which makes a lot of sense. However, it might be helpful if we put together a diagram of sorts to help us determine which features and capabilities will go into which classes. With that as an end point, we can start refactoring the code one piece at a time into a fully object-oriented implementation.

Furthermore, I'm stewing on an idea to support different tool heads without having to re-load the firmware. If you consider LulzBot's TAZ-6 design with their various tool heads, it could be a compelling feature. In my case, I'm going with the E3D Chimera & Cyclops Legendary Pack, and I just like the idea of not having to re-flash the firmware when I switch between the two. The Nozzle class seems like a good place to handle this, but it's going to make Configuration.h rather interesting.

Roxy-3D · 2017-05-02T06:01:29Z

Maybe it's about time to start having an architecture discussion. I'm thinking that @thinkyhead is wanting to move some of these types of features into the Nozzle class, which makes a lot of sense. However, it might be helpful if we put together a diagram of sorts to help us determine which features and capabilities will go into which classes. With that as an end point, we can start refactoring the code one piece at a time into a fully object-oriented implementation.

All that is fine and good... But if we can do a couple small tweaks and make this suitable to be included in the 'Golden Master' I think we should do it. And we can have an architectural discussion about how to organize things better for v1.1.1 after this Release Candidate gets promoted.

tcm0116 · 2017-05-08T21:17:30Z

@thinkyhead Are we at a point where we can resume the discussion about this? With the PR for M701 and M702 (#6493) making yet another version of the filament change code, I think we're starting to see a strong case for unifying all of these very related features in order to cut down on the amount of duplicate code floating around. There were also some heater pausing features added in #6622 for BLTouch that overlap some of the work in this PR related to turning off the heaters during an extended pause.

I still don't exactly follow your comment about unifying this with the NOZZLE_PARK_FEATURE, so I'd like to have some discussion about what you're thinking with that.

Roxy-3D · 2017-05-09T17:22:15Z

@tcm0116 Are there any pieces of code that can be combined to shrink redundancy? That is not an architectural discussion... But it probably will kick of some discussion in that direction.

tcm0116 · 2017-05-09T17:41:06Z

@Roxy-3D when M125 was added, the code for M600 was duplicated and massaged a bit. The aim of this PR was to consolidate them back into a single back-end that was shared between the M125 and M600 commands. I believe it wouldn't be much more effort to make use of the same shared functionality for the M701 and M702 filament load and unload commands from #6493.

tcm0116 · 2017-05-10T03:43:12Z

I've updated this PR to advance it to the current bugfix-1.1.x branch. As part of that, I've merged the heater pause capability that @bgort added in #6622 with the heater idle timeout capability I had originally added. I'd still like to merge in M701 and M702 from #6493, but I'd like to get some more feedback on what I've done so far before I proceed.

I still need someone to test the target temperature flashing on the DOGM display when the heaters and/or bed are paused. That can be done by starting a print from SD and then pausing it via the LCD. After the configured timeout, the target temperatures for the extruders should start flashing and the actual temperature will start dropping. Testing the bed temperature flashing should be able to be accomplished by enabling the pause while probing feature and then it should flash during each probe event.

bgort · 2017-05-17T14:13:02Z

Looking good....

floyd871 · 2017-05-27T09:28:42Z

With this Merge you have overwritten two entries in many language files with the english one!!
Please revert that.

#define MSG_FILAMENT_CHANGE_HEADER _UxGT("PRINT PAUSED")
#define MSG_FILAMENT_CHANGE_OPTION_HEADER _UxGT("RESUME OPTIONS:")

tcm0116 · 2017-05-27T14:52:42Z

@floyd871 that was by design. The original text was specific to the M600 filament change, but don't make sense to the M125 pause since it now uses some of those screens when the print is resumed. It's my understanding that there are people that periodically update the translations as necessary. If you want to provide a translation for your language, I'm sure we could get it incorporated.

floyd871 · 2017-05-27T16:14:35Z

I see, that makes sense, but then it would be better if you have renamed the label as well.
"Filament change" doesn't match "print paused".... just my opinion.

tcm0116 · 2017-05-27T16:46:14Z

I thought the same thing when you posted them. I tried to change everything, but I guess I missed those.

Roxy-3D · 2017-05-27T16:51:18Z

Well... Now that the bulk of the changes are merged.... It is easy to followup with a couple of small patches. Somebody is helping! #6871

Roxy-3D · 2017-05-28T21:33:25Z

@tcm0116 I've been using the Advanced Pause and new Filament Change for 2 days now. I have yet to get the print I wanted done. (It is a very difficult print) But the new Pause code seems to do exactly what it is supposed to do. I have paused it several times and changed the filament several times.

Unfortunately... I thought the last print was going to succeed, but I paused it for a couple of hours that I had to be away from the printer. When I cane back, the ABS plastic I was using broke away from the glass and lost adhesion. Soooo... Restarting it with PLA plastic on my FrankenStein printer....

tcm0116 · 2017-05-29T01:06:01Z

@Roxy-3D That's great to hear! Did you happen to remember to check for the flashing target temperature on the filament change LCD screens?

Roxy-3D · 2017-05-29T01:25:25Z

No! I forgot... But I'll try to remember to do that next time I pause something...

Roxy-3D · 2017-05-29T14:15:11Z

@tcm0116

As it turns out... We still have a little bit of work to do! :)

When giving the printer a M600... The second number doesn't blink.

tcm0116 · 2017-05-30T00:52:53Z

@Roxy-3D does the first number update as the temperature is dropping without you having to do anything like move the encoder?

Roxy-3D · 2017-05-30T04:48:23Z

does the first number update as the temperature is dropping without you having to do anything like move the encoder?

Yes! It is slow on my printer... but after 30 or 40 seconds, it is clear that it is dropping....

tcm0116 · 2017-05-31T20:51:04Z

@Roxy-3D I put together a change at tcm0116@f3528b9 if you have a chance to give it a try.

Roxy-3D · 2017-05-31T21:02:14Z

I can grab your pause_flash branch, right? But... should we edit this? We have nested #if ENABLED(ADVANCED_PAUSE_FEATURE)

 #if ENABLED(ADVANCED_PAUSE_FEATURE)

    static void lcd_implementation_hotend_status(const uint8_t row) {
      row_y1 = row * row_height + 1;
      row_y2 = row_y1 + row_height - 1;

      if (!PAGE_CONTAINS(row_y1 + 1, row_y2 + 2)) return;

      u8g.setPrintPos(LCD_PIXEL_WIDTH - 11 * (DOG_CHAR_WIDTH), row_y2);
      lcd_print('E');
      lcd_print((char)('1' + active_extruder));
      lcd_print(' ');
      lcd_print(itostr3(thermalManager.degHotend(active_extruder)));
      lcd_print('/');
      
      #if ENABLED(ADVANCED_PAUSE_FEATURE)
        if (lcd_blink() || !thermalManager.is_heater_idle(active_extruder))
      #endif	
      lcd_print(itostr3(thermalManager.degTargetHotend(active_extruder)));
    }

  #endif // ADVANCED_PAUSE_FEATURE

tcm0116 · 2017-05-31T21:31:48Z

@Roxy-3D Oops. Updated in the branch https://github.com/tcm0116/Marlin/tree/pause_flash

Roxy-3D · 2017-05-31T21:33:42Z

Grabbing it now...

Roxy-3D · 2017-05-31T21:58:22Z

It works on a Graphical Display!!! I don't have a way to test a 20x4 right now.

tcm0116 · 2017-05-31T22:04:26Z

I can test it on my printer.

* Unify M600 and M125 pause features * Cleanup per thinkyhead's comments * Rename filament_change_menu_response to advanced_pause_menu_response * Include HAS_BED_PROBE in QUIET_PROBING * Update gMax example file * is_idle() is out of scope without the braces * Convert FT-i3-2020 to Advance Pause names... * Allow pause even if not printing

thinkyhead · 2017-06-04T20:35:52Z

Seems there is a problem. The filament change feature was broken by this PR, apparently. If the nozzle is cooling down when you start M600, but it is above the extrusion temperature, then M600 is allowed to proceed. However, this PR removes the code from M600 that was put in place to ensure that the temperature won't drop below safe extrusion temperature before first ejection and during loading.

Let's restore that code to M600, because otherwise the temperature just drops and never gets set to the extrusion temperature again.

// Show "wait for heating"
lcd_filament_change_show_message(FILAMENT_CHANGE_MESSAGE_WAIT_FOR_NOZZLES_TO_HEAT);

wait_for_heatup = true;
while (wait_for_heatup) {
  idle();
  wait_for_heatup = false;
  HOTEND_LOOP() {
    if (abs(thermalManager.degHotend(e) - temps[e]) > 3) {
      wait_for_heatup = true;
      break;
    }
  }
}

thinkyhead · 2017-06-04T21:31:17Z

Moreover, the original code I believe took into account that you can break out of a wait for temperature loop prematurely via M108 — so we will need to make sure that after waiting for temperature that the code will abort if the target temperature has still not been reached.

I would advise combing the previous gcode_M600 code and making sure that the current code reproduces beat-for-beat everything that the previous code did. It is not enough, for example, to put up the wait_for_heating screen. The M600 code was responsible for also tracking the temperature and moving forward once the target was reached. The approach with M600 was to make the gcode handler the controller of the LCD, in contrast to procedures which are initiated from the LCD.

Roxy-3D · 2017-06-04T21:34:02Z

Maybe we should revert this? And then move forward with that in mind? The main purpose of the commit was to collapse duplicate code.

thinkyhead · 2017-06-04T21:37:33Z

I think at this point it will be less work to plough forward.

tcm0116 · 2017-06-05T02:40:23Z

However, this PR removes the code from M600 that was put in place to ensure that the temperature won't drop below safe extrusion temperature before first ejection and during loading.

I don't believe this is a true statement. As can be seen in the snippet below from resume_print from the current bugfix-1.1.x branch, the code you've referenced is still present. The difference is that the old code stored the target temperatures prior to turning off the heaters, whereas the new code directly looks at thermalManager.degTargetHotend(e). The only difference this could result in is if the user changes the target temperature while the print is paused for the M600, but I don't think that's possible since the LCD is blocked by the pause screens and the parser is blocked by the call to gcode_M600. The reason for this change was because I added the ability to put the heaters into an idle state directly in the Thermal Manager, so it was no longer necessary for M600 to keep track of the original target temperature.

If the nozzle is cooling down when you start M600, but it is above the extrusion temperature, then M600 is allowed to proceed.

I'm 100% certain that the old M600 code would do the exact same thing.

Moreover, the original code I believe took into account that you can break out of a wait for temperature loop prematurely via M108

The while (wait_for_heatup) line in resume_print maintains that capability in the exact same manner as the original M600 code. However, there appears to logic error in that resume_print (and the old gcode_M600) is spinning on wait_for_heatup, but then manipulates wait_for_heatup within the loop. If resume_print enters the while loop while wait_for_heatup is true, but then the LCD click or M108 resets wait_for_heatup to false during the call to idle() in the loop, then that transition to false will be lost because resume_print overwrites it blindly.

#if ENABLED(ULTIPANEL)
  // Show "wait for heating"
  lcd_advanced_pause_show_message(ADVANCED_PAUSE_MESSAGE_WAIT_FOR_NOZZLES_TO_HEAT);
#endif

wait_for_heatup = true;
while (wait_for_heatup) {
  idle();
  wait_for_heatup = false;
  HOTEND_LOOP() {
    if (abs(thermalManager.degHotend(e) - thermalManager.degTargetHotend(e)) > 3) {
      wait_for_heatup = true;
      break;
    }
  }
}

The solution to the first issue is to ensure that the target temperature for the active extruder is set to a reasonable value when starting M600 in addition to verifying that the nozzle is actually at a reasonable value.

To correct the second issue, we just need to insert if (!wait_for_heatup) break; after the call to idle() in resume_print.

* Unify M600 and M125 pause features * Cleanup per thinkyhead's comments * Rename filament_change_menu_response to advanced_pause_menu_response * Include HAS_BED_PROBE in QUIET_PROBING * Update gMax example file * is_idle() is out of scope without the braces * Convert FT-i3-2020 to Advance Pause names... * Allow pause even if not printing

Unify M600 and M125 pause features

eeb7a76

thinkyhead reviewed Apr 21, 2017

View reviewed changes

Cleanup per thinkyhead's comments

6dc0847

thinkyhead added this to the 1.1.1 milestone Apr 30, 2017

thinkyhead added the S: Don't Merge Work in progress or under discussion. label Apr 30, 2017

tcm0116 mentioned this pull request May 2, 2017

M701 and M702 support from LCD and Gcode #6493

Closed

This was referenced May 2, 2017

Shutdown heaters and fan on Stop SD Print #6553

Closed

Continuously update LCD when hotend status is displayed in Filament Change screens #6554

Closed

Printer killed when homing with BLTouch from high Z #6606

Closed

bgort mentioned this pull request May 7, 2017

Improve BLTOUCH_HEATERS_OFF functionality #6622

Merged

tcm0116 added 2 commits May 9, 2017 19:28

Merge branch 'advanced_pause2' into bugfix-1.1.x

fdfa5e9

Rename filament_change_menu_response to advanced_pause_menu_response

55f3d6e

tcm0116 changed the base branch from RCBugFix to bugfix-1.1.x May 10, 2017 03:37

This was referenced May 17, 2017

[FR] Advanced Stop Feature #5982

Closed

Lift Z when print stopped #6763

Closed

tcm0116 deleted the advanced_pause2 branch May 31, 2017 20:48

tcm0116 referenced this pull request Jun 5, 2017

Ensure safe temperature for M600

d878450

tcm0116 mentioned this pull request Jun 5, 2017

Updates for ensure_safe_temperature #6963

Merged

Roxy-3D mentioned this pull request Jun 7, 2017

M600 "PAUSE_PARK_NO_STEPPER_TIMEOUT" not working #6979

Closed

fiveangle mentioned this pull request Jul 20, 2017

Need a pause button #2003

Closed

Unify M600 and M125 pause features #6407

Unify M600 and M125 pause features #6407

Conversation

tcm0116 commented Apr 21, 2017

thinkyhead Apr 21, 2017

Choose a reason for hiding this comment

thinkyhead Apr 21, 2017

Choose a reason for hiding this comment

thinkyhead Apr 21, 2017

Choose a reason for hiding this comment

thinkyhead commented Apr 21, 2017

tcm0116 commented Apr 21, 2017

tcm0116 commented Apr 21, 2017

thinkyhead commented Apr 30, 2017

tcm0116 commented May 2, 2017

Roxy-3D commented May 2, 2017

tcm0116 commented May 2, 2017

Roxy-3D commented May 2, 2017

tcm0116 commented May 8, 2017

Roxy-3D commented May 9, 2017

tcm0116 commented May 9, 2017

tcm0116 commented May 10, 2017

bgort commented May 17, 2017

floyd871 commented May 27, 2017

tcm0116 commented May 27, 2017

floyd871 commented May 27, 2017

tcm0116 commented May 27, 2017 via email

Roxy-3D commented May 27, 2017 • edited

Roxy-3D commented May 28, 2017

tcm0116 commented May 29, 2017 via email

Roxy-3D commented May 29, 2017

Roxy-3D commented May 29, 2017

tcm0116 commented May 30, 2017

Roxy-3D commented May 30, 2017

tcm0116 commented May 31, 2017

Roxy-3D commented May 31, 2017

tcm0116 commented May 31, 2017

Roxy-3D commented May 31, 2017

Roxy-3D commented May 31, 2017

tcm0116 commented May 31, 2017 via email

thinkyhead commented Jun 4, 2017 • edited

thinkyhead commented Jun 4, 2017 • edited

Roxy-3D commented Jun 4, 2017 • edited

thinkyhead commented Jun 4, 2017

tcm0116 commented Jun 5, 2017 • edited

Roxy-3D commented May 27, 2017 •

edited

thinkyhead commented Jun 4, 2017 •

edited

thinkyhead commented Jun 4, 2017 •

edited

Roxy-3D commented Jun 4, 2017 •

edited

tcm0116 commented Jun 5, 2017 •

edited