Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow wait-for-cooling, break at threshold or if cooling stalls #4169

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Jun 29, 2016

Instead of breaking immediately when M109 Rn or M190 Rn specifies a temperature below EXTRUDE_MINTEMP/2 or 30, proceed to wait, but break out when the temperature reaches the low threshold. Also check the temperature every 20 seconds. If it doesn't drop by at least 1°C in 20 seconds, stop waiting.

Reference: #4039 (comment)

next_cool_check_ms = now + 5000;
}
}

Copy link
Contributor

@Blue-Marlin Blue-Marlin Jun 29, 2016

Choose a reason for hiding this comment

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

if ((old_temp - temp) < 1.0) break; we are cooling!
old_temp must be set to temp somewhere. Courrently it's set to 9999 and stays there.
Make clear the temperatures are float. (xxx.0 or xxxf)
5 seconds seems to be a magnitude to small. At the beginning the temperature still could rise. I suggest 60000 - 1° per minute.

@thinkyhead thinkyhead changed the title Allow wait-for-cooling, but break out at low threshold Allow wait-for-cooling, break at threshold or if cooling stalls Jun 29, 2016
@thinkyhead thinkyhead merged commit 725fde8 into MarlinFirmware:RCBugFix Jun 30, 2016
@thinkyhead thinkyhead deleted the rc_wait_but_break branch June 30, 2016 23:13
@thinkyhead
Copy link
Member Author

Let's see how 20 seconds works for the cooling. The whole low temperature threshold could be removed, as long as this can reliably detect that cooling has stalled.

@ghost
Copy link

ghost commented Jul 1, 2016

After this PR was merged, I got compilation warnings.

\AppData\Local\Temp\build6674025c445f92d0b07185e4e4080804.tmp\sketch\Marlin_main.cpp: In function 'void gcode_M109()':

\AppData\Local\Temp\build6674025c445f92d0b07185e4e4080804.tmp\sketch\Marlin_main.cpp:4623:5: warning: 'wants_to_cool' may be used uninitialized in this function [-Wmaybe-uninitialized]

     if (wants_to_cool) {

     ^

\AppData\Local\Temp\build6674025c445f92d0b07185e4e4080804.tmp\sketch\Marlin_main.cpp: In function 'void gcode_M190()':

\AppData\Local\Temp\build6674025c445f92d0b07185e4e4080804.tmp\sketch\Marlin_main.cpp:4717:7: warning: 'wants_to_cool' may be used uninitialized in this function [-Wmaybe-uninitialized]

       if (wants_to_cool) {

       ^

I think that it's better that wants_to_cool is initialized, but I don't understand that which one is better true or false.

@thinkyhead
Copy link
Member Author

thinkyhead commented Jul 2, 2016

@esenapaj That warning is overkill in this case. The compiler can't tell, but the logic is such that it will never permit wants_to_cool to be used uninitialized. Whatever value we initialize it with, it will be overwritten before it is ever used. Here's the key:

float theTarget = -1.0; // theTarget is always initialized to -1
. . .
do {
  // The actual target temperature can never be -1
  if (theTarget != thermalManager.degTargetHotend(target_extruder)) {
    // So this always happens the first time through the loop
    wants_to_cool = thermalManager.isCoolingHotend(target_extruder);
    . . .
  }
. . .
} while (!cancel_heatup && TEMP_CONDITIONS);

@Blue-Marlin
Copy link
Contributor

Blue-Marlin commented Jul 2, 2016

Just a false warning. theTarget is initialized with -1. so theTarget != thermalManager.degTargetHotend(target_extruder) is always true because a temperature is always at least 0. The first time the do-while loop is executed theTarget is initialized.(Too complicated for the compiler)
If you want to avoid the warning you can initialize wants_to_cool with any value.

@thinkyhead
Copy link
Member Author

thinkyhead commented Jul 2, 2016

I also just did a scrub-through of the temperature setters and accessors to make sure there's no place where an int is being compared as equal (or not-equal) to a float. I didn't find any problematic comparisons, but I will continue to keep an eye out for them.

@ghost
Copy link

ghost commented Jul 2, 2016

@Blue-Marlin @thinkyhead
Thanks, I understand those.
So I leave these warnings for the present.

@thinkyhead thinkyhead mentioned this pull request Jul 8, 2016
@Blue-Marlin
Copy link
Contributor

Made some tests today.
Deactivated the lower temperature limit. Environment ~25.3°.
Testscript

M109 S50
M109 R0
M300
M106 S255
M109 R0
M300
;
M107
M190 S50
M190 R0
M300
M106 S255
M190 R0
M300

dropped out of the loop °C.

                0.25/5 1/20  1/30  1/60  1.5/60  °C/s
uncooled hotend noise  44.7  39.7  32.2  35.6
  cooled hotend noise  32.1  30.3  27.3  27.7
uncooled bed    noise  50.2  44.1  35.0  40.0
  cooled bed    noise  36.7  34.1  29.1  30.0

1/20 uncooled bed dropped out during the overshot. Later measured separately as 47.9°C

For longer times the constant must be long better unsigned long.
Numbers are only valid for the tested nozzle, bed, fan.
For the tested combination I'll stay with 1.5°C/minute - a good distance to possible noise, long enough for most over-shots (i hope), end-temperatuer around body temperature on moderate warm days.

@Blue-Marlin
Copy link
Contributor

1.5°C/minute.
wait for cooling

AnHardt added a commit to AnHardt/Marlin that referenced this pull request Jul 11, 2016
Adjust wait_for_cooling slope
and drop mintemp for cooling.

See
MarlinFirmware#4169 (comment)
@jbrazio jbrazio modified the milestone: 1.1.0 Jul 18, 2016
CONSULitAS pushed a commit to CONSULitAS/Marlin-K8200 that referenced this pull request Aug 18, 2016
Adjust wait_for_cooling slope
and drop mintemp for cooling.

See
MarlinFirmware#4169 (comment)
drewmoseley pushed a commit to drewmoseley/Marlin that referenced this pull request Nov 8, 2023
Move nozzle to center during Thermal model cal.
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.

3 participants