Fri 30 Sep 2011 01:42:07 PM UTC, comment #3:
> The main problem with this project is the fact that the
> MSB of temperature is incorrect. GCC uses register r27
> with undefined value instead of r14 containing MSB.
Exactly, this is my analysis, too, and this rather clearly
appears to be a compiler bug to me.
Forcing the compiler to inline the CRC function simply hides
that bug, but it's not a solution. With -Os, the compiler
is really right to not inline it, as this causes a smaller
code size. (Switching to -O2 is already sufficient to
inline it.)
Closing as "invalid" since there is nothing avr-libc can
do about here. Please open a GCC bug report for it, and
don't forget to add the preprocessed sourcecode there.
|
Thu 29 Sep 2011 10:59:25 AM UTC, comment #2:
Thank you for your post. Yes, you're rigth. It should work, but doesn't and I wonder why. (The compiler options are as default in AVRStudio 5). Here's wider description of my problem.
I am writing a simple project with Maxim digital thermometer ds18b20 in Atmel AVRStudio 5 using AVRlibc. The main goal is to understand how gcc works, that's why any suggestions are welcome. Uploaded sources are available at http://students.mimuw.edu.pl/~kg277366/thermometer.zip with default AVRStudio makefile. The built files are included as well.
The main problem with this project is the fact that the MSB of temperature is incorrect. GCC uses register r27 with undefined value instead of r14 containing MSB. The part of thermometer.lss file which shows this code disassembly is attached at the end of ds18b20.h file and below. Runtime tests confirm wrong value of MSB.
_crc_ibutton_update function comes from AVRlibc util/crc16.h file. According to my tests, loss of MSB occurs only when my ds_read_temp_crc function is inlined and AVRlibc _crc_ibutton_update function is called. That's why I guess GCC has problem with _crc_ibutton_update function. Of course I don't exclude error in my source code.
I'll be thankful for any ideas explaining this weird compiler behavior.
static int16_t ds_read_temp_crc()
{
int16_t temp;
uint8_t i, data, crc;
if (!ds_reset())
2e2: 0e 94 82 00 call 0x104 ; 0x104 <ds_reset>
2e6: 88 23 and r24, r24
2e8: 09 f4 brne .+2 ; 0x2ec <main+0x118>
{
return DS_RESET_TEMP;
}
2ea: 7b c0 rjmp .+246 ; 0x3e2 <main+0x20e>
// skip ROM
ds_write_byte(0xCC);
2ec: 8c ec ldi r24, 0xCC ; 204
2ee: 0e 94 9e 00 call 0x13c ; 0x13c <ds_write_byte>
// read scratch pad
ds_write_byte(0xBE);
2f2: 8e eb ldi r24, 0xBE ; 190
2f4: 0e 94 9e 00 call 0x13c ; 0x13c <ds_write_byte>
// LSB of temp
data = ds_read_byte();
temp = (int16_t)data;
2f8: 0e 94 c6 00 call 0x18c ; 0x18c <ds_read_byte>
2fc: 08 2f mov r16, r24 ; LSB stored in r16
2fe: 80 e0 ldi r24, 0x00 ; 0
crc = _crc_ibutton_update(0, data);
300: 60 2f mov r22, r16
302: 0e 94 41 00 call 0x82 ; 0x82 <_crc_ibutton_update>
306: 18 2f mov r17, r24
// MSB of temp
data = ds_read_byte();
temp |= (int16_t)(data) << 8;
308: 0e 94 c6 00 call 0x18c ; 0x18c <ds_read_byte>
30c: e8 2e mov r14, r24 ; MSB stored in r14
30e: 81 2f mov r24, r17
crc = _crc_ibutton_update(crc, data);
310: 6e 2d mov r22, r14
312: 0e 94 41 00 call 0x82 ; 0x82 <_crc_ibutton_update>
316: d8 2e mov r13, r24
for (i = 0; i < 7; i++)
{
data = ds_read_byte();
crc = _crc_ibutton_update(crc, data);
}
318: 17 e0 ldi r17, 0x07 ; 7
31a: 0e 94 c6 00 call 0x18c ; 0x18c <ds_read_byte>
31e: 68 2f mov r22, r24
320: 8d 2d mov r24, r13
322: 0e 94 41 00 call 0x82 ; 0x82 <_crc_ibutton_update>
326: d8 2e mov r13, r24
328: 11 50 subi r17, 0x01 ; 1
32a: b9 f7 brne .-18 ; 0x31a <main+0x146>
if (crc)
{
return DS_CRC_ERROR;
}
32c: 88 23 and r24, r24
32e: 09 f0 breq .+2 ; 0x332 <main+0x15e>
330: 5b c0 rjmp .+182 ; 0x3e8 <main+0x214>
332: a0 e0 ldi r26, 0x00 ; r27:r26 contains: 0x?? 00
334: ed 01 movw r28, r26 ; r29:r28 contains: 0x?? 00
336: 10 e0 ldi r17, 0x00 ; r17:r16 contains: 0x00 LSB
return temp;
338: 0c 2b or r16, r28 ; r17:r16 := r17:r16 OR r29:r28, where r29 is undefined instead of MSB
33a: 1d 2b or r17, r29 // temp in r17:r29
33c: b5 e0 ldi r27, 0x05 ; 5
33e: 00 35 cpi r16, 0x50 ; 80
340: 1b 07 cpc r17, r27
342: 09 f4 brne .+2 ; 0x346 <main+0x172>
344: 53 c0 rjmp .+166 ; 0x3ec <main+0x218>
346: e0 e1 ldi r30, 0x10 ; 16
348: 00 31 cpi r16, 0x10 ; 16
34a: 1e 07 cpc r17, r30
34c: 09 f4 brne .+2 ; 0x350 <main+0x17c>
34e: 4e c0 rjmp .+156 ; 0x3ec <main+0x218>
|
Thu 29 Sep 2011 04:52:23 AM UTC, comment #1:
I don't understand that half-sentence:
``and that's why the MSB of temp is lost''
Even without inlining, the correct functionality should be
achieved.
Btw., drop those pointless -f options. If you want short
enums, use _attribute_((packed)). Structs are packed on
the AVR anyway, as it imposes no alignment requirements.
Nobody should rely on the (un)signedness of `char', and if
you want unsigned bitfields, declare them explicitly that
way.
|
Wed 28 Sep 2011 08:33:29 PM UTC, original submission:
gcc 4.5.1, AVR_8_bit_GNU_Toolchain_3.2.3_315
gcc options: -funsigned-char -funsigned-bitfields -Os -fpack-struct -fshort-enums -Wall -c -std=gnu99 -mmcu=atmega16
function:
static _inline_ uint8_t
_crc_ibutton_update(uint8_t __crc, uint8_t __data)
{
uint8_t __i, __pattern;
_asm_ __volatile__ (
" eor %0, %4" "\n\t"
" ldi %1, 8" "\n\t"
" ldi %2, 0x8C" "\n\t"
"1: lsr %0" "\n\t"
" brcc 2f" "\n\t"
" eor %0, %2" "\n\t"
"2: dec %1" "\n\t"
" brne 1b" "\n\t"
: "=r" (__crc), "=d" (__i), "=d" (__pattern)
: "0" (__crc), "r" (__data));
return __crc;
}
should be always inlined. In my code reading DS18B20 temperature:
// LSB of temp
data = ds_read_byte();
temp = (int16_t)data;
crc = _crc_ibutton_update(0, data);
// MSB of temp
data = ds_read_byte();
temp |= (int16_t)(data) << 8;
crc = _crc_ibutton_update(crc, data);
doesn't and that's why the MSB of temp is lost:
0e 94 44 00 call 0x88 ; 0x88 <_crc_ibutton_update>
Adding fallowing line to header seems to be the simplest solution of this issue:
static _inline_ uint8_t crc_ibutton_update(uint8_t __crc, uint8_t __data) __attribute_((always_inline));
Probably the same problem may occur with others inline Assembly.
|