 |
 |
| View previous topic :: View next topic |
| Author |
Message |
Reler
Guest
|
| if/else vs switch/case |
 Posted: Fri Mar 05, 2004 5:36 am |
 |
|
Hi all.
I have a strange problem with my code.
This function is compiled ok but it does not work right.
| Code: |
void UnlockCom(eComDev Device)
{
switch(Device)
{
case COMDEV_BT:
case COMDEV_USB:
case COMDEV_RADIO:
bCom1Lock = false;
break;
case COMDEV_GSM:
case COMDEV_BAL:
bCom2Lock = false;
break;
}
}
|
If I call it with COMDEV_BAL for example, bCom2Lock is not set to false.
Instead, this one runs well:
| Code: |
void UnlockCom(eComDev Device)
{
if(Device==COMDEV_BT || Device==COMDEV_USB || Device==COMDEV_RADIO)
{
bCom1Lock=false;
return;
}
if(Device==COMDEV_GSM || Device==COMDEV_BAL)
bCom2Lock = false;
}
|
What's happening? I don't understand it.
eComDevice is an enumeration:
| Code: |
enum eComDev {COMDEV_NONE,COMDEV_USB,COMDEV_BT,COMDEV_RADIO,COMDEV_GSM,COMDEV_BAL};
|
I am using PCH 3.184
Thanks in advance. |
|
 |
mpfj
Joined: 09 Sep 2003
Posts: 95
Location: UK
|
|
| Posted: Fri Mar 05, 2004 8:04 am |
|
|
Could you post the listing file (or relevant portion) ?
Mark |
|
|
Charlie U
Joined: 09 Sep 2003
Posts: 183
Location: Somewhere under water in the Great Lakes
|
|
| Posted: Fri Mar 05, 2004 8:29 am |
|
|
First, you have not included all of the possible enumeration values and they are not in enumerated order. This may not be a problem, but I try to do it when I have a switch with an enumerated variable.
You also have 2 lines following the COMDEV_RADIO case. I think the compiler is performing the bCom1Lock = false; line as you intended, in response to the switch, then just executing the break and never making it to the final 2 case statements. (See the added braces)
Try this and see if it helps:
| Code: |
void UnlockCom(eComDev Device)
{
switch(Device)
{
case COMDEV_NONE:
break;
case COMDEV_USB:
case COMDEV_BT:
case COMDEV_RADIO:
{
bCom1Lock = false;
break;
}
case COMDEV_GSM:
case COMDEV_BAL:
{
bCom2Lock = false;
break;
}
}
}
|
|
|
|
mpfj
Joined: 09 Sep 2003
Posts: 95
Location: UK
|
|
| Posted: Fri Mar 05, 2004 8:41 am |
|
|
| Argh ... those pesky brackets !! I can't believe I missed that one ... |
|
|
RKnapp
Joined: 23 Feb 2004
Posts: 51
|
|
| Posted: Fri Mar 05, 2004 9:31 am |
|
|
But those brackets shouldn't be necessary. The initial function looked fine! In fact, the inclusion of those brackets would imply that the COMDEV_NONE case would need them, too.
To me, it looks as if something funny is happening with the compiler's interpretation of the enumeration. Do the values of the enum start with 0 and increment? Are they int8s? It seems possible to me that all the values of the enum wind up as zero, so that in the initial form of this function, which looks fine to me, calling it with anything unlocks Com1 but never Com2.
Just a thought,
 Robert |
|
|
mpfj
Joined: 09 Sep 2003
Posts: 95
Location: UK
|
|
| Posted: Fri Mar 05, 2004 9:45 am |
|
|
| Agreed, the brackets *shouldn't* be necessary, but I'd like to see the listing file all the same ... |
|
|
mpfj
Joined: 09 Sep 2003
Posts: 95
Location: UK
|
| Re: if/else vs switch/case |
| Posted: Fri Mar 05, 2004 10:00 am |
|
|
I've compiled a copy of your code and, looking at the listing, the compiler is using a table lookup routine for the first piece of code, and "normal" compare / jump code for the second.
What version of the compiler are you using ? Some versions have problems with table loopkups.
If you add a "default" statement (good programming practice I was always told ??), the compiler switches back to compare / jump code. Try it, and see if it makes a difference ... |
|
|
Reler
Guest
|
|
| Posted: Fri Mar 05, 2004 1:53 pm |
|
|
Hi again and thanks for your help.
Here is the code generated by the compiler for both functions.
| Code: |
.................... void UnlockCom(eComDev Device)
.................... {
.................... if(Device==COMDEV_BT || Device==COMDEV_USB || Device==COMDEV_RADIO)
.................... {
*
046C: MOVLB A
046E: MOVF x42,W
0470: SUBLW 02
0472: BTFSC FD8.2
0474: GOTO 048C
0478: DECFSZ x42,W
047A: GOTO 0482
047E: GOTO 048C
0482: MOVF x42,W
0484: SUBLW 03
0486: BTFSS FD8.2
0488: GOTO 0498
.................... bCom1Lock=false;
048C: MOVLB 0
048E: BCF x77.1
.................... Com1LockedDev=COMDEV_NONE;
0490: CLRF x79
.................... return;
0492: GOTO 04B2
0496: MOVLB A
.................... }
.................... if(Device==COMDEV_GSM || Device==COMDEV_BAL)
.................... {
0498: MOVF x42,W
049A: SUBLW 04
049C: BTFSC FD8.2
049E: GOTO 04AC
04A2: MOVF x42,W
04A4: SUBLW 05
04A6: BTFSS FD8.2
04A8: GOTO 04B4
.................... bCom2Lock = false;
04AC: MOVLB 0
04AE: BCF x77.5
.................... Com1LockedDev=COMDEV_NONE;
04B0: CLRF x79
04B2: MOVLB A
04B4: MOVLB 0
04B6: RETLW 00
|
| Code: |
.................... void UnlockCom(eComDev Device)
.................... {
.................... switch(Device)
.................... {
*
046C: MOVLW 01
046E: MOVLB A
0470: SUBWF x42,W
0472: ADDLW FB
0474: BTFSC FD8.0
0476: GOTO 0494
047A: ADDLW 05
047C: MOVLB 0
047E: GOTO 0498
.................... case COMDEV_BT:
.................... case COMDEV_USB:
.................... case COMDEV_RADIO:
.................... bCom1Lock = false;
0482: BCF x77.1
.................... break;
0484: MOVLB A
0486: GOTO 0494
....................
.................... case COMDEV_GSM:
.................... case COMDEV_BAL:
.................... bCom2Lock = false;
048A: BCF x77.5
.................... break;
048C: MOVLB A
048E: GOTO 0494
0492: MOVLB A
.................... }
.................... }
0494: MOVLB 0
0496: RETLW 00
|
There is something strange (at least for me) in the second one. A jump in 047E to 0498 out of the scope of the function. In any case, I have searched for that address but it's not in the list file... ¿?
As I wrote before, compiler version is PCH 3.184 |
|
|
Guest
|
|
| Posted: Fri Mar 05, 2004 2:42 pm |
|
|
Very interesting... Like mpfj said, adding a default stament code changes completely switching to "compare & jump mode".
This is the new code generated:
| Code: |
.................... void UnlockCom(eComDev Device)
.................... {
.................... /*
.................... if(Device==COMDEV_BT || Device==COMDEV_USB || Device==COMDEV_RADIO)
.................... {
.................... bCom1Lock=false;
.................... Com1LockedDev=COMDEV_NONE;
.................... return;
.................... }
.................... if(Device==COMDEV_GSM || Device==COMDEV_BAL)
.................... {
.................... bCom2Lock = false;
.................... Com1LockedDev=COMDEV_NONE;
.................... }
.................... */
.................... switch(Device)
.................... {
*
046C: MOVLB A
046E: MOVF x42,W
0470: MOVWF 00
0472: MOVLW 02
0474: SUBWF 00,W
0476: MOVLB 0
0478: BTFSC FD8.2
047A: GOTO 04AA
047E: MOVLW 01
0480: SUBWF 00,W
0482: BTFSC FD8.2
0484: GOTO 04AA
0488: MOVLW 03
048A: SUBWF 00,W
048C: BTFSC FD8.2
048E: GOTO 04AA
0492: MOVLW 04
0494: SUBWF 00,W
0496: BTFSC FD8.2
0498: GOTO 04B0
049C: MOVLW 05
049E: SUBWF 00,W
04A0: BTFSC FD8.2
04A2: GOTO 04B0
04A6: GOTO 04B6
.................... case COMDEV_BT:
.................... case COMDEV_USB:
.................... case COMDEV_RADIO:
.................... bCom1Lock = false;
04AA: BCF x77.1
.................... break;
04AC: GOTO 04B6
....................
.................... case COMDEV_GSM:
.................... case COMDEV_BAL:
.................... bCom2Lock = false;
04B0: BCF x77.5
.................... break;
04B2: GOTO 04B6
....................
.................... default:
.................... }
.................... }
04B6: RETLW 00
|
At this moment I don't have the hardware with me to test it, but it looks better.
I know default statment is optional in ANSI C, so I have checked ccs-manual to see if it is a compiler limitation, but I have found this:
| Code: |
STATEMENTS
switch (expr) {
case cexpr: stmt; //one or more case [default:stmt]
... }
Note: Items in [ ] are optional
|
So, must I double check all the list file looking for more thinks like this before releasing my application? |
|
|
Ttelmah
Guest
|
|
| Posted: Fri Mar 05, 2004 4:03 pm |
|
|
The change in compiler 'mode' for switches, is something I have posted about on many occasions. It will normally default to using the 'compare' mode for smaller switches, and the 'boundary' is usually about 5 tests. Over this number of tests, the compiler uses the jump table, unless a 'default' is present. Hence, if you want to use the table for a switch with effectively a default (for speed), you have to code something like:
| Code: |
if (test<1 || test>6) {
//default code
}
else
switch test {
case 1:
case 2:
case 3:
case 4:
case 5:
case 6:
}
|
Several people in the past have suggested, that the behaviour would be much nicer if it was controllable with a compiler 'switch', (something like #use tableswitch), especially since the presence of a jump table, can cause problems on some chips, and increases the number of registers that have to be saved when entering the interrupt routines...
Normally the jump table code is now fine. It does appear that the use of the enumeration, may be causing an unexpected behaviour in this regard, and really should be reported to CCS.
Best Wishes |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
