Error in hotted_heater_ctrl.cfg allows probing while nozzle is too hot with "fix_heaters_temperature_settle" set to true

[austinrdennis]

Klippain branch

• [X] I confirm using the main branch

Version

4.1.1 Release

Describe the bug and expected behavior

Same basic issue as #300, but in a different place. If you want me to start putting instances of this into a single issue, just close it out with a comment to that effect and I'll add this one and all future instances to #300 and I'll update the title to be more appropriate.

I thankfully didn't discover this the hard way, but noticed this while reviewing some of the more important safety macros. I put it to the test and sure enough, if you have a macro waiting for the ACTIVATE_PROBE macro to complete cooling down the nozzle, it doesn't wait. The M109 command in ACTIVATE_PROBE does not stop subsequent macro from being called and the tilting procedure continues without regard for the actual nozzle temperature.

This can lead to a situation where the nozzle is too hot and probes the bed anyway thus burning the PEI sheet or shattering glass beds. This can occur easily if a user sets safe_extruder_temp in _USER_VARIABLES to a value higher than 150C and running the preheat nozzle module in START_PRINT. This behavior would take all but the most experienced Klipper users off guard as they would expect to be protected be the ACTIVATE_PROBE macro.

This is a safety issue and I'll be putting up an PR shortly to at least hot fix the issue until something more elegant can be crafted.

Additional information and klippy.log

Here's the snippet in question. The M109 command does not stop more macros from being called while temp is stabilizing.

  #If a Voron TAP probe is defined, then check the temperature and lower it if needed
    {% elif probe_type_enabled == "vorontap" %}
        SAVE_GCODE_STATE NAME=BEFORE_TAP_ACTION

        {% set ACTUAL_TEMP = printer.extruder.temperature %}
        {% set TARGET_TEMP = printer.extruder.target %}

        SET_GCODE_VARIABLE MACRO=ACTIVATE_PROBE VARIABLE=temperature VALUE={TARGET_TEMP}

        {% if TARGET_TEMP > tap_max_probing_temp %}
            { action_respond_info('Extruder temperature target of %.1fC is too high for TAP probing, lowering to %.1fC' % (TARGET_TEMP, tap_max_probing_temp)) }
            M106 S255 ; 100% the part cooling fan to help the extruder cooling
            M109 S{tap_max_probing_temp}
            M106 S0   ; Stop the part cooling fan
        {% else %}
            # Temperature target is already low enough, but nozzle may still be too hot
            {% if ACTUAL_TEMP > tap_max_probing_temp + 3 %}
                M106 S255 ; 100% the part cooling fan to help the extruder cooling
                TEMPERATURE_WAIT SENSOR=extruder MAXIMUM={tap_max_probing_temp}
                M106 S0   ; Stop the part cooling fan
            {% endif %}
        {% endif %}
    {% endif %}

[Frix-x]

"I'm not sure about your specific situation, as it seems different from our own testings. Indeed, we are using the M109 command of Klipper that should "set temperature and wait for stabilization".

As per Klipper official documentation:

Additionally, during the development of this part of Klippain, we were 4 people working on this and we tested it thoroughly before merging: I don't think someone else had any issue...

There may be something else going on in your case. Mhmm, I just had a thought: are you using the following variable set to true? https://github.com/Frix-x/klippain/blob/0122cd35a9cbb1314a094d80c408ed53898beea1/user_templates/variables.cfg#L220-L222

[austinrdennis]

"I'm not sure about your specific situation, as it seems different from our own testings. Indeed, we are using the M109 command of Klipper that should "set temperature and wait for stabilization".

As per Klipper official documentation:

Additionally, during the development of this part of Klippain, we were 4 people working on this and we tested it thoroughly before merging: I don't think someone else had any issue...

There may be something else going on in your case. Mhmm, I just had a thought: are you using the following variable set to true?

https://github.com/Frix-x/klippain/blob/0122cd35a9cbb1314a094d80c408ed53898beea1/user_templates/variables.cfg#L220-L222

Oh, yes I do have that variables set as the Revo responds really quickly to temp changes. I failed to consider the effect of the M109 redefinition. I bet that's related, let me do some testing and get back to you.

[austinrdennis]

That was it, good thinking! The M109 re-definition in hotend_heater_ctrl.cfg incorrectly (maybe, I may just be using it wrong) uses the MINIMUM parameter instead of the MAXIMUM parameter when calling TEMPERATURE_WAIT macro. This causes the TEMPURATURE_WAIT macro to do effectively nothing in this scenario.

I tested after change and it works as intended no matter which value fix_heaters_temperature_settle is set to.

From Klipper docs:

TEMPERATURE_WAIT

TEMPERATURE_WAIT SENSOR= [MINIMUM=] [MAXIMUM=]: Wait until the given temperature sensor is at or above the supplied MINIMUM and/or at or below the supplied MAXIMUM.

The similar macro for bed heater control (M190) may also result in unexpected behavior due to no MAXIMUM being specified.

I'll rework my PR for this and re-commit with the fix.

[Frix-x]

Mhmm, I was considering that as well. However, there might be a complication. Usually, this macro is utilized to increase the temperature of the nozzle or the bed, and in these cases, it requires the MINIMUM parameter. On the other hand, we should use the MAXIMUM parameter when cooling is needed, although this is less common, except for meeting the TAP requirements.

This will need a little more intelligent and adaptable system. I am thinking about a mechanism where it checks the current temperature of the nozzle at the time the M109 macro is called, then depending on the circumstance, it would select either the MINIMUM or MAXIMUM parameter. We could apply a similar strategy for the bed macro. What are your thoughts on this approach?

[austinrdennis]

That seems like a good solution and easy to implement. Now that I have more sleep, I see that I was confused about the intention on the M109 section and it makes sense to use minimum there in the majority of cases.

I'll experiment with putting some checks into the M109 macro and commit it the PR repo when I think I have it. 😀👍

[austinrdennis]

Ok, the latest commit on PR #302 should provide the correct functionality and works as intended on my tests.

[Frix-x]

It's merged in develop now, ready for the next version. Thanks for your work :)