[1.1.x] Fix M303 thermal protection #8103 #8126

RowanMeara · 2017-10-27T23:29:17Z

The temperature sanity checking logic was not applied during M303
(pid autotuning) because instead of setting a target temperature, PID_autotune
directly manipulated the pwm values. When PIDTEMP/PIDTEMPBED is
enabled, PWM values rather than the target temperature determine whether
the heater is on. I changed the temperature sanity checking logic to look directly at the PWM values
when pid is enabled.

I tested M303 with the changes and they fix #8103 .

Edit: I also added calls to disable the heaters if PID_autotune errors out.

The temperature sanity checking logic was not being applied during M303 (pid autotuning) because instead of setting a target temperature, it directly manipulated the pwm values. When PIDTEMP/PIDTEMPBED is enabled, PWM values rather than the target temperature determine whether the heater is on. I changed this to look directly at the PWM amount when pid is enabled.

AnHardt · 2017-10-28T00:03:31Z

A good solution!

Currently, PID autotuning stops if it overshoots the temperature by 20C or if if the temperature does not change for 20 minutes and it times out. I added calls to disable the heaters in these scenarios.

ghost · 2017-10-28T14:42:34Z

Very Good job .... 👍
Make the same for 2.0 please 😄

GMagician · 2017-10-28T19:16:42Z

Hello, sorry for int(er)mission, I checked the pid code and I think we can save up to 2K changing some things, but not sure if this is correct or not

at the end of function there is
if (!wait_for_heatup) disable_all_heaters();
but above there is a while cycle that exits when variable is 'false' then we can just call function with no if.
Everywhere in function, when exit and also "disable heaters' are required, instead of 'return' set 'wait_for_heatup' to false and (p)ut remaining code to else {...}

What do you think?
Compiling original code differences are:
184582 bytes (70.4% Full) vs 182470 bytes (69.6% Full)
with this fix maybe we can save more bytes (heater off is c(a)lled more times)

Roxy-3D · 2017-10-28T19:34:48Z

Hello, sorry for intermission, I checked the pid code and I think we can save up to 2K changing some things, but not sure if this is correct or not

I wonder if there is some 'dead code' that is being eliminated by the compiler when you make those changes? Can you cut and paste your code here for people to discuss? It would be helpful if you can show in the comments what changed so people can discuss things easier.

GMagician · 2017-10-28T20:36:38Z

Honestly test was made on 2.0.x-bugfix branch with platformio, sadly with arduino there is no such optimization...
only replacing with 'break' gives me some free memory
default configuration gives me
52246 -> 52236 bytes

temperature.zip

GMagician · 2017-10-28T20:52:48Z

This is really fun..I made same test again, on 2.0 with platformio, just to be sure I'm not crazy

I confirm:
Original code:
Program: 184582 bytes (70.4% Full)
Data: 6486 bytes (79.2% Full)
With variables set to false
Program: 182470 bytes (69.6% Full)
Data: 6486 bytes (79.2% Full)
With break (as 1.1 sent before)
Program: 183734 bytes (70.1% Full)
Data: 6486 bytes (79.2% Full)

o_O

fiveangle · 2017-10-28T20:54:00Z

Aside from trimming potentially dead code, I've noticed Arduino and PlatformIO swapping places a handful of bytes +/- with nearly every change —no rhyme or reason. When I'm struggling to fit into a 128k board, I'll try both and use whichever comes out smaller :)

Added changes suggested by GMagician.

RowanMeara · 2017-10-29T05:43:14Z

I implemented the changes that @GMagician suggested which ended up saving me 62 bytes (116,272->116,210) on my arduino mega.

… into bugfix-1.1.x-HD-CR10 * 'bugfix-1.1.x' of https://github.com/MarlinFirmware/Marlin: (94 commits) [1.1.x] Fix M303 thermal protection MarlinFirmware#8103 (MarlinFirmware#8126) Implement support for Dual X and Y endstops Add Dual Steppers / Endstops to configs Cleanup for DIGIPOTS settings (1.1.x) serious bug G33 Add scripts for .travis.yml to append config options Revert default BABYSTEP_MULTIPLICATOR to 1 Added MKS GEN L board (MarlinFirmware#8088) Concise SD_REPRINT_LAST_SELECTED_FILE description Clean up trailing whitespace (1.1.x) auto tune calibration parameters (MarlinFirmware#8031) Tweak neopixel self-test Add some Polish translations Fix FWRETRACT logic, apply common sense give this language an unique name Initial conflict resolution of SD_REPRINT_LAST_SELECTED_FILE (MarlinFirmware#8104) 1.1.x - save 1400 bytes of FLASH by using reduced font for some languages (MarlinFirmware#8095) More specific M100 description Fix spacing, use single instances of similar pins Encourage users to re-examine their configs ... # Conflicts: # Marlin/Configuration.h # Marlin/Configuration_adv.h

…re#8126) * Fixed M303 thermal protection The temperature sanity checking logic was not being applied during M303 (pid autotuning) because instead of setting a target temperature, it directly manipulated the pwm values. When PIDTEMP/PIDTEMPBED is enabled, PWM values rather than the target temperature determine whether the heater is on. I changed this to look directly at the PWM amount when pid is enabled. * Turn off heaters on M303 error Currently, PID autotuning stops if it overshoots the temperature by 20C or if if the temperature does not change for 20 minutes and it times out. I added calls to disable the heaters in these scenarios. * Removed unnecessary if statement. Added changes suggested by GMagician. * Update temperature.cpp * Update temperature.cpp * Update temperature.cpp

Turn off heaters on M303 error

8f9d492

Currently, PID autotuning stops if it overshoots the temperature by 20C or if if the temperature does not change for 20 minutes and it times out. I added calls to disable the heaters in these scenarios.

Removed unnecessary if statement.

9bf9221

Added changes suggested by GMagician.

thinkyhead added the PR: Bug Fix label Oct 29, 2017

thinkyhead added 3 commits October 29, 2017 04:21

Update temperature.cpp

859c882

Update temperature.cpp

1a7e2ba

Update temperature.cpp

3b7ffb6

thinkyhead mentioned this pull request Oct 29, 2017

[2.0.x] Fix M303 thermal protection #8142

Merged

thinkyhead merged commit 9850ba0 into MarlinFirmware:bugfix-1.1.x Oct 29, 2017

RowanMeara deleted the bugfix-1.1.x branch November 2, 2017 01:03

RowanMeara restored the bugfix-1.1.x branch November 2, 2017 01:06

This was referenced Sep 3, 2018

Printer increases temp to 325C when set to 235C on second layer WITH thermal runaway enabled #11643

Closed

[2.0.x] SECURITYFIX missing max temp error when PID is used #11717

Merged

[1.1.x] SECURITYFIX missing max temp error when PID is used #11718

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.1.x] Fix M303 thermal protection #8103 #8126

[1.1.x] Fix M303 thermal protection #8103 #8126

RowanMeara commented Oct 27, 2017 •

edited

AnHardt commented Oct 28, 2017

ghost commented Oct 28, 2017 •

edited by ghost

GMagician commented Oct 28, 2017 •

edited by Roxy-3D

Roxy-3D commented Oct 28, 2017

GMagician commented Oct 28, 2017

GMagician commented Oct 28, 2017

fiveangle commented Oct 28, 2017 •

edited

RowanMeara commented Oct 29, 2017

[1.1.x] Fix M303 thermal protection #8103 #8126

[1.1.x] Fix M303 thermal protection #8103 #8126

Conversation

RowanMeara commented Oct 27, 2017 • edited

AnHardt commented Oct 28, 2017

ghost commented Oct 28, 2017 • edited by ghost

GMagician commented Oct 28, 2017 • edited by Roxy-3D

Roxy-3D commented Oct 28, 2017

GMagician commented Oct 28, 2017

GMagician commented Oct 28, 2017

fiveangle commented Oct 28, 2017 • edited

RowanMeara commented Oct 29, 2017

RowanMeara commented Oct 27, 2017 •

edited

ghost commented Oct 28, 2017 •

edited by ghost

GMagician commented Oct 28, 2017 •

edited by Roxy-3D

fiveangle commented Oct 28, 2017 •

edited