Seems to be a bug...

 Seems to be a bug...

Hello all,

while checking the code given here.
http://www.maxim-ic.com/appnotes.cfm/appnote_number/3524
I found that Keil compiler is not generating the correct code required for the following commands in C

spiTmp = spiData[i];
SCK_PRE; MOSI=spiTmp7; SCK_MID; spiTmp7=MISO; SCK_POST; // bit 7
SCK_PRE; MOSI=spiTmp6; SCK_MID; spiTmp6=MISO; SCK_POST; // bit 6
SCK_PRE; MOSI=spiTmp5; SCK_MID; spiTmp5=MISO; SCK_POST; // bit 5
SCK_PRE; MOSI=spiTmp4; SCK_MID; spiTmp4=MISO; SCK_POST; // bit 4
SCK_PRE; MOSI=spiTmp3; SCK_MID; spiTmp3=MISO; SCK_POST; // bit 3
SCK_PRE; MOSI=spiTmp2; SCK_MID; spiTmp2=MISO; SCK_POST; // bit 2
SCK_PRE; MOSI=spiTmp1; SCK_MID; spiTmp1=MISO; SCK_POST; // bit 1
SCK_PRE; MOSI=spiTmp0; SCK_MID; spiTmp0=MISO; SCK_POST; // bit 0
spiData[i] = spiTmp;

NOTE: Here the spiTmp variable is getting modified bitwise. And then the modified byte is again copied back to spiData[i] variable

It generates this assemble code

                                           ; SOURCE LINE # 80
0004 7400        R     MOV     A,#LOW spiData
0006 2F                ADD     A,R7
0007 F8                MOV     R0,A
0008 E6                MOV     A,@R0
0009 F500        R     MOV     spiTmp,A
                                           ; SOURCE LINE # 81
000B C282              CLR     SCK
000D A200        R     MOV     C,spiTmp7
000F 9283              MOV     MOSI,C
0011 D282              SETB    SCK
0013 A284              MOV     C,MISO
0015 9200        R     MOV     spiTmp7,C
                                           ; SOURCE LINE # 82
0017 C282              CLR     SCK
0019 A200        R     MOV     C,spiTmp6
001B 9283              MOV     MOSI,C
001D D282              SETB    SCK
001F A284              MOV     C,MISO
0021 9200        R     MOV     spiTmp6,C
                                           ; SOURCE LINE # 83
0023 C282              CLR     SCK
0025 A200        R     MOV     C,spiTmp5
0027 9283              MOV     MOSI,C
0029 D282              SETB    SCK
002B A284              MOV     C,MISO
002D 9200        R     MOV     spiTmp5,C
                                           ; SOURCE LINE # 84
002F C282              CLR     SCK
0031 A200        R     MOV     C,spiTmp4
0033 9283              MOV     MOSI,C
0035 D282              SETB    SCK
0037 A284              MOV     C,MISO
0039 9200        R     MOV     spiTmp4,C
                                           ; SOURCE LINE # 85
003B C282              CLR     SCK
003D A200        R     MOV     C,spiTmp3
003F 9283              MOV     MOSI,C
0041 D282              SETB    SCK
0043 A284              MOV     C,MISO
0045 9200        R     MOV     spiTmp3,C
                                           ; SOURCE LINE # 86
0047 C282              CLR     SCK
0049 A200        R     MOV     C,spiTmp2
004B 9283              MOV     MOSI,C
004D D282              SETB    SCK
004F A284              MOV     C,MISO
0051 9200        R     MOV     spiTmp2,C
                                           ; SOURCE LINE # 87
0053 C282              CLR     SCK
0055 A200        R     MOV     C,spiTmp1
0057 9283              MOV     MOSI,C
0059 D282              SETB    SCK
005B A284              MOV     C,MISO
005D 9200        R     MOV     spiTmp1,C
                                           ; SOURCE LINE # 88
005F C282              CLR     SCK
0061 A200        R     MOV     C,spiTmp0
0063 9283              MOV     MOSI,C
0065 D282              SETB    SCK
0067 A284              MOV     C,MISO
0069 9200        R     MOV     spiTmp0,C
                                          ; SOURCE LINE # 89
006B F6                MOV     @R0,A

See the code generated for line 89. It should be

MOV     A,spiTmp

MOV     @R0,A

or directly

MOV     @R0,spiTmp

I think its because the byte spiTmp is getting modified bitwise and that by no way the compiler is knowing that the byte is modifed and it has to read back the variable and copy the same to the destination.

I tried almost all the setting options for C51 (Code optimisation, warnings etc.), starting with default setting. And that I am using the latest version of the compiler from Keil.

It will be a great help if experts throw more light on this.

Thank you.

Regards,
Shriram

 RE: Seems to be a bug...

Qualifying spiTmp as "volatile" might help.

 RE: Seems to be a bug...

Hi Dan,

I already tested it, and it works fine. But then why not the way as it is?

Shriram

 RE: Seems to be a bug...

How are spiTmp and spiTmp' 0-7 declared? Is spiTmp is bdata? Can you post the defining declarations?

 It is a "Feature"!

Definition: A "Feature" is a Bug that has been documented!

"I seem to remember having seen this..."

Ah yes, here it is:

http://www.keil.com/support/docs/1863.htm

Note that this is linked direct from the online manual page:

http://www.keil.com/support/man/docs/c51/c51_le_bitaddrobj.htm

But I think it should be properly included in the Notes on the page - not just added as a link.

:-(

 RE: Seems to be a bug...

Shriram Patki: "I think its because the byte spiTmp is getting modified bitwise and that by no way the compiler is knowing that the byte is modifed and it has to read back the variable and copy the same to the destination."

I think you are right!

I seem to remember having seen this in the distant past some time before...
What version are you using?

Dan Henry: "Qualifying spiTmp as 'volatile' might help."

Shriram Patki: "I already tested it, and it works fine. But then why not the way as it is?"

See above - I think you have already answered that!
If your hypothesis was correct (and I think it was), then you would expect 'volatile' to fix it, wouldn't you?

This does sound like a flaw in the bdata implementation - the compiler should know that these are all references to the same bit-addresable object!

 RE: Seems to be a bug...

Andy and Dan, thanks.

Dan, it was declared as

usigned char bdata spiTmp;

Andy Neil: "Definition: A "Feature" is a Bug that has been documented!"

Do you mean to say, A Bug I thought was a "Feature" that has been documented!? :)

Andy Neil: "But I think it should be properly included in the Notes on the page - not just added as a link."

Yes, I agree. It should be included there.

Thanks to both of you.

Shriram