Updates for ensure_safe_temperature

[tcm0116]

This is based on my comments in #6407 and to d878450

This PR supersedes #6958

[thinkyhead]

Is this thoroughly tested? Some of this seems like it simply will not work.

[thinkyhead]

I believe the previous implementation of M600 would start heatup for you and go to the wait-for-heatup screen. Then it would do the same for you if the nozzle timed out and started to cool again. This seems like a step backward to insist that the target temperature already be set at the start, especially as we still have to make sure the nozzle has reached the set temperature anyway and we still have to deal with reheating after a timeout.

So, let's continue trying to restore everything that M600 code used to (or should) do. A lot of good work was done by @petrzjunior and I hate to see it lost.

[tcm0116]

This was the very beginning of gcode_M600 prior to #6407 being merged:

    if (!DEBUGGING(DRYRUN) && thermalManager.tooColdToExtrude(active_extruder)) {
      SERIAL_ERROR_START;
      SERIAL_ERRORLNPGM(MSG_TOO_COLD_FOR_M600);
      return;
    }

It didn't check to see if the target temperature was reasonable and if the nozzle wasn't warm enough, it would just bail. That's exactly how advance pause worked prior to the addition of ensure_safe_temperature. This change adds some additional checks to ensure that the target temperature is above the limit (if there is a limit) and then waits for the nozzle to heat up prior to continuing, which is what you requested in your latest comments in #6407.

[thinkyhead]

Gotcha. I guess when I noticed the temperature suddenly dropping in the middle of M600 I presumed something had been lost. Thanks for making the extra effort!

[thinkyhead]

This can never happen. The while statement will already have caught this condition.

[tcm0116]

I may be mistaken, but I thought the call to idle() was to allow the emergency parser to execute. If that's the case, then if the emergency parser sees a M108, then that M108 will set wait_for_heatup to false, which would immediately be overwritten by the line right after the call to idle() without this change.

[tcm0116]

I added a new commit to resolve this in a different way that will prevent any possible race conditions or logic errors associated with wait_for_heatup being reset by M108 in the emergency parser. This should allow M108 to work as desired with M600.

[thinkyhead]

We don't want to keep calling this function repeatedly. It should only be called the first time that the code detects that heating is needed.

[tcm0116]

We can put that back in, but I took it out because the original gcode_M600 function didn't do that for similar calls to lcd_advanced_pause_show_message:

while (wait_for_user) {

  if (nozzle_timed_out)
    lcd_filament_change_show_message(FILAMENT_CHANGE_MESSAGE_CLICK_TO_HEAT_NOZZLE);

  ...
}

[tcm0116]

Looking at this more closely, lcd_filament_change_show_message calls lcd_goto_screen, which first verifies that currentScreen != screen prior to proceeding. As such, I believe it's relative harmless to call lcd_filament_change_show_message repeatedly.

[thinkyhead]

Where is the logic to wait for the click, turn on the heater, and so on? I can't see how this is supposed to work.

[tcm0116]

All I did in the original #6407 was split gcode_M600 into three functions, with almost no other changes to the function of M600. If you look in the current gcode_M600, you'll see that the three functions are called sequentially:

    if (pause_print(retract, z_lift, x_pos, y_pos, unload_length, beep_count, true)) {
      wait_for_filament_reload(beep_count);
      resume_print(load_length, ADVANCED_PAUSE_EXTRUDE_LENGTH, beep_count);
    }

[thinkyhead]

As long as it works! I'll have a closer look to make sure I understand what's going on. At some point we'll want to clean up and standardize this LCD stuff more, so I should get a handle on it now!

[tcm0116]

I actually want to re-work this section. Currently, after it unloads, it instructs the user to load the new filament. If the heater idle timeout it expires and tells the user to press the button to re-throw. If the user presses the button, it re-heats, but immediately loads the filament when it gets up to temp. This could be confusing if the user wasn't at the printer when the load filament message was displayed, but they get there when the press to re-heat message is being displayed. I went back and I believe this is how M600 worked before #6407, so I don't think I changed anything at this point.

[tcm0116]

@thinkyhead For some reason, I believe you think I made significant changes to how M600 works in #6407, when in reality I didn't. @Roxy-3D and I throughly tested the changes in #6407 and I believe that any issues that have been found since then were actually issues in the original M600 that carried over into the advanced pause feature because I limited the actual changes to the its function. You might consider revisiting #6407 to see what was actually changed so that we can all be on the same page.

[tcm0116]

@thinkyhead Have you had an opportunity to review this again? M600 is currently broken due to the change that was made to wait_for_filament_reload to call ensure_safe_temperature . The current sequence of events when M600 is performed is:

1. If printing from SD, the print is paused
2. LCD displays "Wait for start of the filament change" and the filament is retracted
3. The nozzle moves to the filament change location
4. LCD displays "Wait for filament unload" and the filament is unloaded
5. LCD displays "Insert filament and press button to continue..." and the buzzer beeps
6. The e-stepper is disabled and heaters are configured to go idle after the specified timeout
7. When the heaters timeout and go idle, ensure_safe_temperature is called which displays "Heating Nozzle Please wait..." on the LCD

This is where M600 is broken because ensure_safe_temperature is waiting for the nozzles to heat up, but they won't because their idle timeouts have expired and they're cooling down. As such, the printer will get stuck at this step and will not be able to continue.

This PR fixes this issue by reverting wait_for_filament_reload to display "Press button to heat nozzle." when the heaters timeout instead of calling ensure_safe_temperature, which is instead called in resume_print after the heaters have been reset and start to re-heat.

[Roxy-3D]

This does fix the immediate M600 issue. I think it is worth merging because it helps people (myself included) that use M600. It still does not address Thinky's issue that if the nozzle is too cold to do the M600, it gives up and says:

[ERROR] Error: M600 Hotend too cold to change filament.

The reason I'm going to merge right now is some of us REALLY need M600 to work. This one issue is can be resolved independently of the other issues. In fact... I would put the one or two lines of code in to resolve it if I knew what to do. The problem I have with putting one or two lines in to resolve it is I don't know what temperature is OK. PLA Temps? ABS Temps? The previous hot temp? What makes sense?

In the middle of March the code was doing this:

  inline void gcode_M600() {

    if (!DEBUGGING(DRYRUN) && thermalManager.tooColdToExtrude(active_extruder)) {
      SERIAL_ERROR_START;
      SERIAL_ERRORLNPGM(MSG_TOO_COLD_FOR_M600);
      return;
    }

It was just giving an error and stopping. And with this merge... It does the same thing.

Let's continue the discussions on what makes sense. And when we agree... I'll personally do the coding to get it there. But right now.... We need M600 to work and this does that!

[tcm0116]

@Roxy-3D I tested what M600 would do when first started in different configurations this morning. Here's the rundown (assuming PREVENT_COLD_EXTRUSION is enabled):

• If M600 is called with the active extruder target temperature set to a value lower than the minimum extrusion temperature, M600 aborts giving the "Error: M600 Hotend too cold to change filament." message.
• If the target temperature is above the minimum extrusion temperature when M600 is called, but the nozzle isn't up to temperature, then the "Heating nozzle Please wait..." message is displayed on the LCD until the nozzle is done heading, at which M600 will continue on as normal.
• If the active extruder temperature is above the minimum extrusion temperature, then M600 operates as usual.

I think your concern is with the first point, but I don't think there's a better option. M600 is intended to be used during a print with the nozzle operating at temperature. I wouldn't think we'd want M600 to heat the nozzle up to some arbitrary temperature and then try to unload the filament. If there's a high-temp filament in the extruder, such as nylon, the default minimum extrusion temperature is probably too low, and the filament will likely strip in the feeder. On some printers, that can be a pain to clear.

One thing to note is that the nozzle temperatures are only checked if a non-zero unload_length is passed to pause_print such as during M600 but not for M125. When I was looking at #6979 earlier, I realized that pause_print does still retract the filament a bit during M125, so we might want to apply the above checks when nob-zero values for either retract or unload_length are passed to pause_print.

Finally, the fist commit on this PR won't compile because ensure_safe_temperature was added to pause_print but wasn't defined at that point in the code, but Travis called it good. As such, we need to update Travis to check some advanced pause setups.

[Roxy-3D]

I wouldn't think we'd want M600 to heat the nozzle up to some arbitrary temperature and then try to unload the filament. If there's a high-temp filament in the extruder, such as nylon, the default minimum extrusion temperature is probably too low, and the filament will likely strip in the feeder. On some printers, that can be a pain to clear.

I think I agree... We don't have enough information to do the 'right thing'.

Finally, the fist commit on this PR won't compile because ensure_safe_temperature was added to pause_print but wasn't defined at that point in the code, but Travis called it good. As such, we need to update Travis to check some advanced pause setups.

I believe the entire PR is merged. If it isn't I'll scramble to get things fixed. I did grab the code after the merge and verify it. Can you do a quick double check?

[tcm0116]

I believe the entire PR is merged.

I'm sure it did (I can't check for sure at this moment). My point was that Travis isn't enabling the ADVANCED_PAUSE_FEATURE when it's running its tests.

[Roxy-3D]

Oh! OK. We should probably turn that on in a few of the configurations. The big problem is it already takes Travis 10 minutes to do the checks that it does do. So we can really do a full suite of tests on every option. We kind of have to bundle a number of checks together.

[thinkyhead]

Sorry, guys. It's been that kind of a week, and I haven't been able to follow up on issues as much as I want. (Plus the pile is getting large.)

Anyway, is there anything to follow up on now that this is merged?

[tcm0116]

@thinkyhead gotta love those kind of weeks :-/ I'd still appreciate if you could review my responses to your comments. I went through and tried to test as much as I could to validate what I think you were wanting to see. I think the big things are that the sequencing is back to working how it did before, with the exception of the start of M600 as explained above. I did also validate that M108 aborts the heating process, which M600 had never done previously.

There are some more improvements I'm going to work on, including checking the nozzle temperature for M125 if a retraction is to be performed, as well as looking into removing the requirement for an LCD to use M600. I was thinking of accomplishing this via serial messages to indicate the next step, but I'm not sure if there's a precedent for that. Do you have any thoughts on doing something like that, and would it be a good idea to make it translatable?

[thinkyhead]

appreciate if you could review my responses

I'll read through them shortly…

removing the requirement for an LCD to use M600. I was thinking of accomplishing this via serial messages to indicate the next step, but I'm not sure if there's a precedent for that.

Well, we haven't ever had a process that is guided by serial feedback, and this is generally because hosts aren't very good at doing back-and-forth with users. I've been trying to come up with a good protocol by which the firmware could ask the host to ask the user for a response, then send that response back to the firmware, but it's not developed.

Instead of taking the guided approach, for G29 with PROBE_MANUALLY it uses a persistent state approach, emulating the way that MBL also works. For MBL you send G29 with different S values to do a certain step in the leveling procedure. ABL with PROBE_MANUALLY just takes G29 by itself and automatically goes to the next state. It prints a message to serial on each step, or when queried with G29 Q.

So, for the initial implementation, the filament change procedure should follow the same pattern. The first M600 would pause, move, and eject the filament. The next M600 (or perhaps M600 S1 or M600.1) would load the filament. If you wanted to load more you might use M600 S2 (or M600.2) to do the "extrude more." Then one final M600 (or M601… or M600.3…) would complete the process and resume the print. If you wanted to abort completely in the middle of an M600 then you would click STOP in the host, or use M22 to stop an SD print.

[thinkyhead]

M600 is intended to be used during a print with the nozzle operating at temperature. I wouldn't think we'd want M600 to heat the nozzle up to some arbitrary temperature and then try to unload the filament.

M600 taking a temperature value would be a good option. And from the LCD, when the extruder is cold: "Change PLA Filament", "Change ABS Filament", or an intermediate selection screen where you choose from a stored list of materials. That would be a good augmentation to the "Filament" sub-menu generally. We can alter only two material temperature settings currently. It would be nice for users to have 4 slots where they can put their most used filaments, customize them via LCD, and save them to EEPROM.

[tcm0116]

@thinkyhead I think that's a great idea. I'll look into it.

[petrzjunior]

@thinkyhead About a year ago, we talked about Load and Unload filament, didn't we? I don't seem to have enough free time to get into it, but we should try make M600 procedure more generic as some parts (load, unload) can be reused later on. Passing temperature as parameter seem perfect to me!