Hello everyone,
I am using the PGA400 from Texas Instruments (an 8051W processor) with uVision V4.02 and have traced an issue back to using a switch statement in the commBuff ISR. It appears to either have corrupted or overflowed some sort of variable (this might not be the case but that's what it appears to have done). I've replaced it from:
switch (CommandState) { case (0): // random code here break; }
to:
if(CommandState==0) { // random code here }
This seems to have fixed my problem for whatever reason. Is it a common practice to avoid using switch statements in an ISR or is this a Keil related issue? Any help you can give me would be appreciated. Thanks a lot!
Thank you for all your replies. As someone had mentioned, yes, you are correct that I do not understand why this would fix the issue, which is my concern. While I'm happy that this has solved it, not understanding WHY means it's possible I do the same thing later on (assuming it's not directly related to switch statements.
I will try and answer everyone's questions.
Dhaval:
The overall code is actually pretty small (granted there are 16 different switch statements but the code is very basic and, in one particular case, I think it goes through only 4 or 5 lines of code (just read commands) and still shows the same issue. Essentially, I would expect to see the same problem with an if() statement if it was code length related, which is why I was curious as to if it was the way Keil handles ISRs.
Andrew:
To my knowledge, timing isn't an issue as it's a fairly short ISR command. Although, if it does take the compiler quite a while to go through the switch statement, it is possible that this is timing related. I understand what you're saying about the time of the ISR being important over the code length though. Below is one particular branch that shows the same issue:
void commBuff_ISR (void) interrupt 6 using 3 { unsigned char commData; EA = 0; /* Added 04-23-13 */ commData = COMBUF; /* Read Data from COMBUF */ // if((CommandState!=7)&&(CommandState!=8)&&(CommandState!=10)&&(CommandState!=11)&&(commData==0x24)) { CommandState = 0; } switch (CommandState) { case (0): if (commData == 0x24) /* 0x24 = '$' */ { CommandState +=1; } else if (commData == 0x62) /* 0x62 = 'b' */ { COMBUF = DigOut; } break; default: break; } EA = 1; /* Added 04-23-13 */ }
This is not to show the functionality of the code (I left out the other 14 or so switch statements but they're all pretty short like that) as much as to give you an idea of how short the ISR paths are (maybe this isn't short by ISR standards but it's fairly straight forward. I can't imagine going from a switch statement to an if statement should make a huge change in terms of timing though.
To give you a little more info though, the issue lies in the fact that the code is working correctly for half an hour or so (the time varies) with me sending it 8 I2C commands per second, I believe it is. It gives the correct values for a while but eventually, it causes the chip to stop updating the DAC. I'm not sure if it's an overflow issue but somehow, it seems to be causing some data corruption in my ADC ISR or something to that effect. I tried a number of things but used GPIO points (because it's an OTP chip) to narrow it down to the switch statement line. Changing it from a switch statement to an if statement seems to remove the issue. It's odd.
Per: I think you're probably right about the ISR deciding which of the actions to perform. I'm guessing based on what your post says, it's quite possible that this could be related to how the hardware is choosing to handle it (if it's not Keil related). So I guess the only thing I can really do is avoid switch statements in the ISR? Or would you avoid using switch statements in the rest of the code to play it safe?
Erik: As I mentioned to Andrew above, I would be surprised if it was timing related but I won't rule it out (maybe the chip handles switch statements a lot slower than if statements). While the procedure appears short to me, maybe it's not as short as I think it is because I'm viewing it as lines of code rather than timing . . . but each of the commands is pretty short so I would be surprised if this is the case. It's also not timing critical for the rest of my code.
CommandState is local to the ISR, though I tried making it a global volatile as well to see if that changed anything but it didn't seem to have any effect.
Thank you to everyone who replied back. If I missed any questions or I missed something conceptually, please let me know when you get the chance!
"CommandState is local to the ISR"
Not as shown in the code posted.
"I left out the other 14 or so switch statements"
Do you really mean that?
Or do you mean that you left out the other 14 case clauses from that one switch statement?
Sorry for the confusion, Andrew. I had CommandState mixed up with commData, which is local. You are correct, CommandState is a global variable (though it could easily be made local).
And you are correct, I have 14 other case clauses from a single switch statement, not 16 switch statements.
Sorry about that!
if (commData == 0x24) /* 0x24 = '$' */ { CommandState +=1; } else if (commData == 0x62) /* 0x62 = 'b' */ { COMBUF = DigOut; }
What's the point of using "magic numbers", and then having to comment that they represent characters?!
If they are characters, why not just use characters:
if (commData == '$') { CommandState +=1; } else if (commData == 'b') { COMBUF = DigOut; }
Because our scope interprets the commands in terms of hex values. I could go the other way and reference the characters but then I'd just have to comment what the hex values are. Truthfully, I can look at a chart, it's just to save me time when looking at the code and the scope. It's for my own use more than anyone else's.