Hi there All, I have a problem which seemed to be defying explanation, but I have come up with a theory. Could I possibly have some feedback on whether the following is likely, plausible, possible, untrue or downright rubbish? If one reads the contents of a CAN or ADC chip register at a particular address, then the label volatile is placed upon that address to prevent the compiler optimising out repeat readings of the address. If one reads the contents of the address into a variable, then the compiler would automatically treat the contents of this variable with similar care. Is it possible that there has been an oversight with statements where the contents of a variable depend on the contents of a volatile by way of an if statement, ie...
normal_var=volatile_var;
normal_var=voltile_var; if (normal_var=0x00) { another_normal_var+=1; }
This assumption of yours: If one reads the contents of the address into a variable, then the compiler would automatically treat the contents of this variable with similar care. is false. "Volatile" is a property of the variable, not of its current value at any time.
Cheers Hans, a fair point. I guess what I should be saying is, I'm assuming the compiler automatically treat a variable as volatile if it sees that the contents of an address 'labelled' as volatile are being read into the variable. Is this a reasonable assumption? Yours, Richard.
Hi there Mike, Cheers for your patience. Stripping out all the unnecesary stuff, the code which I am working with is as shown below. Note that it is for testing for successful transmission and reception of CAN messages. The aim being to pass one of four suitable values to a set of lights on the CAN card to set them green or red to say if CAN messages are being successfully sent of received...
//define the addresses on the CAN chip containing the send and receive flags #define TransmitA (*((volatile unsigned char far*)0x200008)) #define TransmitB (*((volatile unsigned char far*)0x200009)) #define ReceiveA (*((volatile unsigned char far*)0x200004)) #define ReceiveB (*((volatile unsigned char far*)0x200005)) // define the address which governs the CAN card LED's and the values which could be assigned to it. #define CAN_board_LEDs (*((volatile unsigned char far*)0x20002E)) #define Transmit_OK 0x05 #define Receive_OK 0x0A #define RESET 0x00 //define the variables unsigned char CAN_LED_status; unsigned char temp_a; unsigned char temp_b; unsigned char test; //piece of code located in the main function Start_timing(); TransmitB=0x47; TransmitA=0xff; ReceiveB=0; ReceiveA=0; Timing_pause(); \\waits for end of 100Hz cycle time to be flagged test=0x00; while (test!=0xb8)//test for at least one CAN message being received { Read_signals();//fills CAN message registers with fresh data from ADC's temp_b=TransmitB; temp_a=TransmitA; TransmitB=0x47; TransmitA=0xff; CAN_LED_status=RESET; if ((temp_a==0x00)&&(temp_b==0x00)) { CAN_LED_status+=Transmit_OK; } temp_b=ReceiveB; temp_a=ReceiveA; ReceiveB=0; ReceiveA=0; if ((temp_a==0x00)&&(temp_b==0xb8)) { CAN_LED_status+=Receive_OK; } CAN_board_LEDs=CAN_LED_status; test=temp_b; Watchdog(); Timing_pause(); }
CAN_LED_status+=Transmit_OK; CAN_LED_status+=Receive_OK;
while(1) { Watchdog(); }
PS I ought to add about this piece of code, that if the loop to detect if any CAN messages are received is repeated further down the piece of code. If these further repeats are removed (by for example the use of the lines...
Hi Richard, That's more like it. When discussing anything to do with C, I much prefer C code to abstract words :-) It sounds like you don't have an in-circuit debugger set up... Anyway, there is no need to try and guess how the compiler optimized your code when you can actually see the generated code. Print out the disassembly of your code and the questions will go away. If you are not familiar with the C166 instruction set, it wouldn't be too dificult to learn. After all, C166 is a RISC-like architecture, there are not that many instructions. I would suggest posting compiler listing here if there wasn't that much code. Regards, - mike
Hi Mike, Cheers for this suggestion, it looks like it is bearing fruit. Looking at the Main.LST file, the differences seem to be in the interpretation of the lines...
CAN_LED_status=RESET; if ((temp_a==0x00)&&(temp_b==0x00)) CAN_LED_status+=Transmit_OK; temp_b=ReceiveB; temp_a=ReceiveA; ReceiveB=0; ReceiveA=0; if ((temp_a==0x00)&&(temp_b==0xb8)) CAN_LED_status+=Receive_OK; CAN_board_LEDs=CAN_LED_status;
2A6C E00E MOV R14,#00H 2A6E F480DC00 MOVB RL4,[R0+#0DCH]; temp_a 2A72 3D04 JMPR cc_NZ,?C0236 2A74 F480DE00 MOVB RL4,[R0+#0DEH]; temp_b 2A78 3D01 JMPR cc_NZ,?C0236 2A7A 08E5 ADD R14,#05H 2A7C ?C0236: 2A7C D7408000 EXTP #080H,#01H 2A80 F3F80500 MOVB RL4,05H 2A84 E480DE00 MOVB [R0+#0DEH],RL4; temp_b 2A88 D7408000 EXTP #080H,#01H 2A8C F3F80400 MOVB RL4,04H 2A90 E480DC00 MOVB [R0+#0DCH],RL4; temp_a 2A94 E108 MOVB RL4,#00H 2A96 D7408000 EXTP #080H,#01H 2A9A F7F80500 MOVB 05H,RL4 2A9E E108 MOVB RL4,#00H 2AA0 D7408000 EXTP #080H,#01H 2AA4 F7F80400 MOVB 04H,RL4 2AA8 F480DC00 MOVB RL4,[R0+#0DCH]; temp_a 2AAC 3D07 JMPR cc_NZ,?C0237 2AAE F480DE00 MOVB RL4,[R0+#0DEH]; temp_b 2AB2 47F8B800 CMPB RL4,#0B8H 2AB6 3D02 JMPR cc_NZ,?C0237 2AB8 06FE0A00 ADD R14,#0AH 2ABC ?C0237: 2ABC F04E MOV R4,R14 2ABE D7408000 EXTP #080H,#01H 2AC2 F7F82E00 MOVB 02EH,RL4
2A82 E108 MOVB RL4,#00H 2A84 E480DA00 MOVB [R0+#0DAH],RL4; CAN_LED_status 2A88 F480DC00 MOVB RL4,[R0+#0DCH]; temp_a 2A8C 3D08 JMPR cc_NZ,?C0236 2A8E F480DE00 MOVB RL4,[R0+#0DEH]; temp_b 2A92 3D05 JMPR cc_NZ,?C0236 2A94 F480DA00 MOVB RL4,[R0+#0DAH]; CAN_LED_status 2A98 0985 ADDB RL4,#05H 2A9A E480DA00 MOVB [R0+#0DAH],RL4; CAN_LED_status 2A9E ?C0236: 2A9E D7408000 EXTP #080H,#01H 2AA2 F3F80500 MOVB RL4,05H 2AA6 E480DE00 MOVB [R0+#0DEH],RL4; temp_b 2AAA D7408000 EXTP #080H,#01H 2AAE F3F80400 MOVB RL4,04H 2AB2 E480DC00 MOVB [R0+#0DCH],RL4; temp_a 2AB6 E108 MOVB RL4,#00H 2AB8 D7408000 EXTP #080H,#01H 2ABC F7F80500 MOVB 05H,RL4 2AC0 E108 MOVB RL4,#00H 2AC2 D7408000 EXTP #080H,#01H 2AC6 F7F80400 MOVB 04H,RL4 2ACA F480DC00 MOVB RL4,[R0+#0DCH]; temp_a 2ACE 3D0B JMPR cc_NZ,?C0237 2AD0 F480DE00 MOVB RL4,[R0+#0DEH]; temp_b 2AD4 47F8B800 CMPB RL4,#0B8H 2AD8 3D06 JMPR cc_NZ,?C0237 2ADA F480DA00 MOVB RL4,[R0+#0DAH]; CAN_LED_status 2ADE 07F80A00 ADDB RL4,#0AH 2AE2 E480DA00 MOVB [R0+#0DAH],RL4; CAN_LED_status 2AE6 ?C0237: 2AE6 F480DA00 MOVB RL4,[R0+#0DAH]; CAN_LED_status 2AEA D7408000 EXTP #080H,#01H 2AEE F7F82E00 MOVB 02EH,RL4
2A22 E108 MOVB RL4,#00H 2A24 E480D600 MOVB [R0+#0D6H],RL4; CAN_LED_status 2A28 F480D800 MOVB RL4,[R0+#0D8H]; temp_a 2A2C 3D08 JMPR cc_NZ,?C0236 2A2E F480DA00 MOVB RL4,[R0+#0DAH]; temp_b 2A32 3D05 JMPR cc_NZ,?C0236 2A34 F480D600 MOVB RL4,[R0+#0D6H]; CAN_LED_status 2A38 0985 ADDB RL4,#05H 2A3A E480D600 MOVB [R0+#0D6H],RL4; CAN_LED_status 2A3E ?C0236: 2A3E D7408000 EXTP #080H,#01H 2A42 F3F80500 MOVB RL4,05H 2A46 E480DA00 MOVB [R0+#0DAH],RL4; temp_b 2A4A D7408000 EXTP #080H,#01H 2A4E F3F80400 MOVB RL4,04H 2A52 E480D800 MOVB [R0+#0D8H],RL4; temp_a 2A56 E108 MOVB RL4,#00H 2A58 D7408000 EXTP #080H,#01H 2A5C F7F80500 MOVB 05H,RL4 2A60 E108 MOVB RL4,#00H 2A62 D7408000 EXTP #080H,#01H 2A66 F7F80400 MOVB 04H,RL4 2A6A F480D800 MOVB RL4,[R0+#0D8H]; temp_a 2A6E 3D0B JMPR cc_NZ,?C0237 2A70 F480DA00 MOVB RL4,[R0+#0DAH]; temp_b 2A74 47F8B800 CMPB RL4,#0B8H 2A78 3D06 JMPR cc_NZ,?C0237 2A7A F480D600 MOVB RL4,[R0+#0D6H]; CAN_LED_status 2A7E 07F80A00 ADDB RL4,#0AH 2A82 E480D600 MOVB [R0+#0D6H],RL4; CAN_LED_status 2A86 ?C0237: 2A86 F480D600 MOVB RL4,[R0+#0D6H]; CAN_LED_status 2A8A D7408000 EXTP #080H,#01H 2A8E F7F82E00 MOVB 02EH,RL4
The only difference I see is that in the 'broken' code one of the variables (CAN_LED_status) is placed in the register R14. All the other veriables are placed on the user stack. Of course, this is perfectly legal and the compiler generated correct code. There can be all sorts of reasons why the code fails: changed execution speed, corruption of registers by interrupt service routines we don't know of, plain bug in the code (so it works accidentally sometimes) and so on. Best of luck! - mike
I'll second Mike's observation. There's absolutely no change at all between the three assembly outputs you show, as far as accesses to your CAN registers are concerned. They're those EXTB+MOVB pairs. Which means that whatever the real bug is, it's not in the piece of assembly code you did show. It's elsewhere. One thing I'm worried about is that I don't see any remains of the most significan byte actual addresses of your CAN registers (0x20) in any of the quoted code. Now, I don't do 166's at all, but this does feel fishy. Are you sure this code succeeds in accessing your CAN hardware registers at all, in the first place? You really should trace through this in some simulator or emulator to see what actually happens.
One thing I'm worried about is that I don't see any remains of the most significan byte actual addresses of your CAN registers (0x20) in any of the quoted code. It's those EXTP instructions. EXTP #080H,#01H is pretty much the same as EXTS #020H,#01H, which wouldn't alarm you. For some reason the C166 compiler prefers EXTP to EXTS, but that doesn't do any harm. - mike
Hello, I would like to ask the next question... After I've read this thread I would to ask... Here some code listing... int idata timer_cnt=0; bit global_refresh=0; void timer_isr interrupt 1 { timer_cnt++; if(timer_cnt==4) { global_refresh=1; timer_cnt=0; } } void main() { while() { ....... global_refresh=0; } } and it works without any error though I've declared the variables as global(without "volatile")??? May be here there is nothing to optimize? Thanks for any input.. With best wishes, S.
May be here there is nothing to optimize? That, or your usage pattern happens to be a safe one. global_refresh should be declared volatile in this case, as it is shared between main() and the ISR. Timer_cnt is not shared, and so need not be volatile. Consider that, given this code snippet, it would be legal for a compiler to hoist the assignment out of the while loop. This source
while () { global_refresh = 0; ... }
global_refresh = 0; while () { ... }
while () { ... global_refresh = 0; }
Hi Drew, I try to give it much more precisely... here this code snippet... /////////////////////////////////////////// int idata timer_cnt=0; bit global_refresh=0; void timer_isr interrupt 1 { timer_cnt++; if(timer_cnt==4) { global_refresh=1; timer_cnt=0; if(global_refresh) LED=0;//LED is ON } } void main() { while() { if(global_refresh) { ....... global_refresh=0; if(!global_refresh) LED=1;//LED is OFF } } } and it's blinking. That's why I sad it works! But WHY does it work without any VOLATILE declaration? With best wishes, S.
A couple of things occur to me here: 1) I believe that sbit variables are implicitly volatile, although I've had a look in the manual and cannot seem to find anything to back that up. Maybe plain 'bit' variables are also considered volatile? 2) The compiler would have no difficulty seeing that global_refresh is modified both inside the ISR and in main(). Perhaps it automatically suppresses optimisation in cases like this? Stefan
A couple of things occur to me here: 1) I believe that sbit variables are implicitly volatile, although I've had a look in the manual and cannot seem to find anything to back that up. Maybe plain 'bit' variables are also considered volatile? 2) The compiler would have no difficulty seeing that global_refresh is modified both inside the ISR and in main(). Perhaps it automatically suppresses optimisation in cases like this? Optimisation is entirely optional for the compiler. In practice, writes to global variables tend not to be optimised out because, in general, it is difficult for the compiler to keep track of where else a variable might be read. However, reads of global variables is another matter. In this case SV's code works because it is simply fortunate that the compiler chooses to read the value of global_refresh from memory rather than keeping a copy in a register. With this processor there is probably no benefit in keeping a local copy of a bit, but if global_refresh were to be changed to a char then the code may fail to work as expected. Worse still, the code might work for now, but fail when the compiler is updated!
Hi Stefan, 1. I tried it with char global_refresh and it works too... Regards, S.
I'm using 8051F124 compiler keil v7.08 assembler v7.08a
and exactly to say I tried char xdata global_refresh;
just tried without xdata - it works too...
S V, you're chasing a red herring. What happens in a particular program, using a particular compiler version on a particular platform, if you do not qualify object as volatile which really has to be, misses the point. The point is that by not making this variable volatile, you're inviting the compiler to optimize it in whatever way it thinks. This risks the way actually being chosen to be one that breaks your program. It may not happen today, but that says nothing about what may happen next week, after you added some completely unconspicious pieces of code elsewhere in the program, or upgrade your compiler. Your code as given is buggy, and since you obviously know how to fix that bug, there's no valid excuse for leaving it unfixed. Not making your variable, which is shared between the interrupt handler and the main code, a volatile is a lie by omission. Let's repeat that: Your code lies to the compiler. And as always, this means you will responsible for whatever the consequences of that lie may be. You're causing undefined behaviour --- and trust me, you don't want to do that. Do you really believe it's a good idea to lie to an entity you're supposed to cooperate with to complete the greater project at hand? Lying to your supervisor will get you fired quickly --- lying to the compiler may take a bit longer, but it will ultimately get you fired, too. Don't do it.
just tried without xdata - it works too... And who gives a hoot. Succesful testing does no prove the abscence of bugs, it only proves the abscence of known bugs. Hans-Bernhard states: says nothing about what may happen next week, after you added some completely unconspicious pieces of code elsewhere in the program, or upgrade your compiler He is actually missing the real problem. The interrupt variable may share the location with an extremely rarely accessed variable in the mainstream and when - and only when - the interrupt happens while this little rarely active piece of code is running, you program will go astray. Why discuss "can I get by naking it the wrong way" just make it the right way and go on. Erik
Hello Hans-Bernhard Broeker , thank you very much for you reply and ,believe me, it was a question only for understanding of the things, which have taken place in my project. My project is about 35K of code(I think it's huge enough and it's real complicated) and I've checked this behavior(without volatile) many times in the different stages of the project development and it has never failed. It's clear, that I will use this VOLATILE declaration, but I've yet sad , it's only a question of exact understanding. And I saw such behavior in another projects as for Cx51 as well for Cx166 - it has work faultless! With best regards, S. P.S. I would like to have this snippet of code get failed...
Hi Erik, I think your message hit exact the problem.. "The interrupt variable may share the location with an extremely rarely accessed variable in the mainstream and when - and only when - the interrupt happens while this little rarely active piece of code is running, you program will go astray." Now I try to explain... I use this refresh_global for the display updating with a refresh period of ca.300ms. 1.And change of the display contents is not so fast, may be only in the case if one would rotate the potentiometer. May be here one can not exactly say if one refresh was missed or not(it's just too fast for eye). 2.This variable has only some places in the program where it's changed - it's really rarely accessed variable and I had luck that in that moment this variable was accessed in the mainstream this ISR has not occured. Does it somehow hit the problem? With best wishes, S.
Does it somehow hit the problem? It does hit the reason to apply volatile where needed. Erik
to Erik Thanks, P.S. I've understood you answer that what I've written trying to explain why my code "has not failed" were correct?
I would like to have this snippet of code get failed... Your code as shown may just be too simple to fail. If this has never failed you yet, that only proves you either didn't really check strictly enough, or that you got lucky everytime. Neither of which would justify relying on such behaviour for future projects.
I've understood you answer that what I've written trying to explain why my code "has not failed" were correct? Yes Erik
I would like to have this snippet of code get failed... No guarantee this will work for you. Once when I had to sort out someone elses mess and the person refused to believe me, I managed to show the problrm by setting the timer complete bit while stopped at the mainline routine in the emulator. I doubt a simulator will blow as well as an emulator, but if you do not have an ICE, you can try it. Erik
something more .... dissasembly... some snippets of this code... ////void timer_isr interrupt 1//// 280: global_refresh=1; C:0x66AD 755001 MOV global_refresh(0x50),#refr_en(0x01) 281: if(global_refresh==1)LED=1; C:0x66B0 E550 MOV A,global_refresh(0x50) ////////////////////////////////// ///////////void main////////////// /////while() 56: if(global_refresh==1) C:0x03EB E550 MOV A,global_refresh(0x50) C:0x03ED 6401 XRL A,#refr_en(0x01) .................................... .................some code.......... 317: global_refresh=0; C:0x1367 E4 CLR A C:0x1368 F550 MOV global_refresh(0x50),A 350: if(global_refresh==0) LED=0; C:0x141E E550 MOV A,global_refresh(0x50) //////////////////////////////////// Here one can see - nothing has been put in the register and in all cases it has been read from the same data location memory!(D:0050H PUBLIC global_refresh). Additionaly, what I didn't say before, the timer was not declared with "using x", it means , it uses the same bank as the main(). I think, the main point, why it works that, there is nothing to optimize! with best regards, S.
Why wouldn't one just use a critical section for a variable shared amongst multiple priority ISRS and main run code?... or have ownership flags..... I'm a little confused on how using a volatile type qualifier protects you when using a shared resource?
Why wouldn't one just use a critical section for a variable shared amongst multiple priority ISRS and main run code? Because C doesn't have critical sections. or have ownership flags.. Because the objects in question are not owned by any of the threads of execution alone. They are shared by definition. Volatile is needed because, by default, a C compiler is supposed to know everything that's going on in a C program, so it can optimize things. If that presumption is broken, i.e. during the execution of a given code fragment, the state of any object used by it can change for a reason other than what's written out in that particular code fragment, you have to tell the compiler about that. And the volatile qualifier is the way you do that.
some snippets of this code... Here one can see - nothing has been put in the register Here. In this particular case. So yes, this is an example that omitting volatile where you should have used it doesn't automatically guarantee your code to fail. So what else is new? Examples don't prove anything, period. Let me repeat that: PROOF BY EXAMPLE DOESN'T WORK. [Sorry for the shouting, everybody, but it seems there's no other way we'll get the message into this person's mind.]
Examples don't prove anything, period. Let me repeat that: PROOF BY EXAMPLE DOESN'T WORK. [Sorry for the shouting, everybody, but it seems there's no other way we'll get the message into this person's mind.] Hans-Bernhard, I do not know how many posts we have seen stating "it tested" and I am frustrated, just like you, that so many believe in "testing". Succesful testing does not prove the abscence of bugs, it only proves the abscence of known bugs. The analogy would be that because when you did not stop at a stop sign you did not cause an accident it can be concluded that there is no risk ignoring that stop sign. I wonder how this point can get across and really do appreciate to see you phrasing it in a different form, so maybe one of the many can understand that form. Erik
View all questions in Keil forum