Fix TEMP_HYSTERESIS calculation #4338
Conversation
|
Firmware/ultralcd.cpp
Outdated
| @@ -1872,7 +1872,7 @@ void mFilamentItem(uint16_t nTemp, uint16_t nTempBed) | |||
|
|
|||
| // the current temperature is within +-TEMP_HYSTERESIS of the target | |||
| // then continue with the filament action if any is set | |||
| if (bFilamentSkipPreheat || abs((int)current_temperature[0] - nTemp) < TEMP_HYSTERESIS) | |||
| if (bFilamentSkipPreheat || fabs(current_temperature[0] - nTemp) < TEMP_HYSTERESIS) | |||
There was a problem hiding this comment.
nTemp is integer (the target temperature). Using fabs will promote nTemp to float, hence the 14 byte increase in code size.
I don't see how this will fix the issue described where it continues to wait to even 1°C instead of 5°C. Am I missing something? I haven't tested this yet obviously :)
There was a problem hiding this comment.
I can't really explain it. I also think both ways should work, but I tested it and this way it triggers with the defined TEMP_HYSTERESIS value and not at the exact temp value as described in the bug report.
I tested it with the default 5 degrees and also with 10. it triggered correctly in both cases.
There was a problem hiding this comment.
If the increase in code size is a concern, perhaps try casting nTemp to int well with abs? I’m wondering if there is something going on with nTemp as it’s unsigned
There was a problem hiding this comment.
Yes, using (int) also on nTemp works and even saves 2 additional bytes.
There was a problem hiding this comment.
Here's the assembly output from CMake build. I added a few comments but can't test it now. The code is testing if the Carry bit is set (brcs), maybe it is failing somehow. It would be interesting to learn exactly what is going wrong since this issue could be in other places in the firmware.
if (bFilamentSkipPreheat || abs((int)current_temperature[0] - nTemp) < TEMP_HYSTERESIS)
2c71e: 80 91 ef 05 lds r24, 0x05EF ; 0x8005ef <bFilamentSkipPreheat>
2c722: 81 11 cpse r24, r1
2c724: 13 c0 rjmp .+38 ; 0x2c74c <mFilamentItem(unsigned int, unsigned int)+0xa0>
2c726: 60 91 00 0e lds r22, 0x0E00 ; 0x800e00 <current_temperature>
2c72a: 70 91 01 0e lds r23, 0x0E01 ; 0x800e01 <current_temperature+0x1>
2c72e: 80 91 02 0e lds r24, 0x0E02 ; 0x800e02 <current_temperature+0x2>
2c732: 90 91 03 0e lds r25, 0x0E03 ; 0x800e03 <current_temperature+0x3>
2c736: 0f 94 66 dc call 0x3b8cc ; 0x3b8cc <__fixsfsi> Convert current_temperature to int16_t
2c73a: 06 17 cp r16, r22 ; 5, Compare, Compare r16 <> r22, nTemp <> current_temperature
2c73c: 17 07 cpc r17, r23
2c73e: 31 f0 breq .+12 ; (nTemp == current_temperature)? 0x2c74c <mFilamentItem(unsigned int, unsigned int)+0xa0>
2c740: 60 1b sub r22, r16 ; current_temperature - nTemp
2c742: 71 0b sbc r23, r17
2c744: 65 30 cpi r22, 0x05 ; 5, Compare with Immediate, Compare r22 <> 5
2c746: 71 05 cpc r23, r1
2c748: 08 f0 brcs .+2 ; 0x2c74c <mFilamentItem(unsigned int, unsigned int)+0xa0>
2c74a: 5e c0 rjmp .+188 ; 0x2c808 <mFilamentItem(unsigned int, unsigned int)+0x15c>
if (bFilamentSkipPreheat || abs((int)current_temperature[0] - (int)nTemp) < TEMP_HYSTERESIS)
2c71e: 80 91 ef 05 lds r24, 0x05EF ; 0x8005ef <bFilamentSkipPreheat>
2c722: 81 11 cpse r24, r1
2c724: 12 c0 rjmp .+36 ; 0x2c74a <mFilamentItem(unsigned int, unsigned int)+0x9e>
2c726: 60 91 00 0e lds r22, 0x0E00 ; 0x800e00 <current_temperature>
2c72a: 70 91 01 0e lds r23, 0x0E01 ; 0x800e01 <current_temperature+0x1>
2c72e: 80 91 02 0e lds r24, 0x0E02 ; 0x800e02 <current_temperature+0x2>
2c732: 90 91 03 0e lds r25, 0x0E03 ; 0x800e03 <current_temperature+0x3>
2c736: 0f 94 65 dc call 0x3b8ca ; 0x3b8ca <__fixsfsi> Convert current_temperature to int16_t
2c73a: 60 1b sub r22, r16 Compare, Compare r16 <> r22, nTemp <> current_temperature
2c73c: 71 0b sbc r23, r17 ; current_temperature - nTemp
2c73e: 6c 5f subi r22, 0xFC ; 252
2c740: 7f 4f sbci r23, 0xFF ; 255
2c742: 69 30 cpi r22, 0x09 ; 9
2c744: 71 05 cpc r23, r1
2c746: 08 f0 brcs .+2 ; 0x2c74a <mFilamentItem(unsigned int, unsigned int)+0x9e>
2c748: 5e c0 rjmp .+188 ; 0x2c806 <mFilamentItem(unsigned int, unsigned int)+0x15a>There was a problem hiding this comment.
Yep. changing the code to:
if (bFilamentSkipPreheat || ((int)current_temperature[0] - nTemp) < 0)
confirms it. The compiler responds with:
warning: comparison of unsigned expression < 0 is always false
but
if (bFilamentSkipPreheat || ((int)current_temperature[0] - (int)nTemp) < 0)
works fine.
As I look at this. The way it's coded TEMP_HYSTERESIS must be > 0 otherwise even the fixed code will not work.
Is that intended or should we fix it to allow 0? (changing it to <= TEMP_HYSTERESIS)
There was a problem hiding this comment.
Yep. changing the code to:
if (bFilamentSkipPreheat || ((int)current_temperature[0] - nTemp) < 0)
confirms it. The compiler responds with:
warning: comparison of unsigned expression < 0 is always false
Good catch :) Now I wonder if this same error is elsewhere.
TEMP_HYSTERESIS must be > 0
We could add a static assert so that the code fails if TEMP_HYSTERESIS is 0, that way the build will fail to compile. But I don't have strong feelings on it. Does it make sense to allow TEMP_HYSTERESIS == 0? As I understand it, it should never be zero as the temperature sensor always has some error.
There was a problem hiding this comment.
We already filter out the error by type casting current_temperature to int. That should give us almost 2 deg of "buffer". (-0.9 to 0.9). But either way is fine for me 😸
There was a problem hiding this comment.
Looking at the other places where TEMP_HYSTERESIS is used it would make sense to add a static assert and not allow 0.
There was a problem hiding this comment.
The compiler catches this with -Wsign-conversion but not -Wsign-compare.
sarusani
force-pushed
the
fix_TEMP_HYSTERESIS
branch
from
829814a to
8a7b729
Compare
sarusani
force-pushed
the
fix_TEMP_HYSTERESIS
branch
from
50f1fc7 to
5f7e4ac
Compare
This fixes #4333.