Why isn't my switch case working?/ How can I fix these build errors?

Question

I'm a student and I've written out this code (the task involves controlling an automatic door) I thought I had everything sorted but when I try to build it it gives me a few errors. If someone could point me in the right direction to fixing them that would be great!

#include <reg167.h>
#define SensorsActive P2 & 0x0010 == 0x0010 || P2 & 0x0020 == 0x0020

/* Main program - Automatic doors */

int main(void)
{
    DP2 = 0x0003;
    /* sets bits 0,1 as outputs and the rest as inputs */ 
    ODP2 = 0x0000;
    /* selects output push/pull driver */
    enum {closed, opening, open, closing} state;
    /* define FSM states */
    state = 1;
    /* set starting state as door closed */

    for (;;) {
        switch (state) {
        case closed: /* Door closed */
            if (SensorsActive) {
            /* If motion sensors are activated switch to opening state */
                state = opening;                                        
            }
            break;      
        case opening: /* Door opening */
            P2 = P2 & ~0x0001;
            /* Sets direction bit to clockwise (opening) */
            P2 |= 0x0002;    /* Turns on motor control bit */
            if(P2&0x0004==0x0004)//door open limit switches triggered switch to open
                state = open;
            break;
        case open: /* Door open */
            timer();
            while (T3OTL != 1) ;
            if(SensorsActive) {
            /* If sensors are active break so that open case will be repeated and timer reset... */                                                         /* ...Else switch state to closing */
                    break;
            }
            else state = closing;
            break;
        case closing: /* Door closing */
            P2 |= 0x0001;// Sets direction bit to counterclockwise (closing) 
            P2 |= 0x0002;/* Turns on motor control bit */
            if(SensorsActive)/* If sensors are active start opening doors */ 
                state = opening;
            if(P2 & 0x0008 == 0x0008) {
            //If closed limit switches are reached switch state to closed
                    state = closed;
            }
            break;
        }
    }
}

The errors it is giving are:

Build target 'Target 1'

compiling main.c...

main.c(10): error C25: syntax error near 'enum'

main.c(11): error C53: redefinition of 'state'

main.c(13): error C25: syntax error near 'while'

main.c(13): error C25: syntax error near '1'

main.c(16): error C103: 'state': illegal function definition (missing ';' ?)

Target not created.

Use parenthesis in `#define`s. For the errors: They are very clear. Please read a C book; you seem to have basic syntax missunderstanding. And **please** format your code properly. GNU formatting is just ancient. — too honest for this site, Sep 06 '15 at 16:07
Wow that was quick, thanks! Yeah I know my C is terrible, I'm doing mechatronic engineering so I've missed half the programming courses that everyone else has already done so the book is definitely a good idea, any suggestions? — Jtonkin, Sep 06 '15 at 16:11
Note that `void main()` is not really preferred (it's wrong unless you're coding on Windows), but see [What should `main()` return in C and C++?](http://stackoverflow.com/questions/204476/what-should-main-return-in-c-and-c/18721336#18721336) for the full details. — Jonathan Leffler, Sep 06 '15 at 16:46
Thanks, I'll read through that, speaking of the formatting structures before, is there a now standard format style? Just so I can look it up and learn it — Jtonkin, Sep 06 '15 at 16:58

Some programmer dude · Accepted Answer · 2015-09-06T16:25:09.410

0

It seems you are compiling in C89 mode, where you can't have variable declarations anywhere (it was added in the C99 standard). Move the state declaration to the top of the function.

Alternatively, tell the compiler to use the C99 rules. For GCC this can be done by adding the -std=c99 option to the command line.

Right now your code looks like this (comments added by me):

void main(void) {
    DP2 = 0x0003;  /* Assignment, okay */
    ODP2 = 0x0000; /* Another assignment, okay */
    enum {closed, opening, open, closing} state;  /* Variable declaration, NOT okay */
    state = 1; /* Assignment, okay */

    ...
}

You need to put the variable declarations first in the function:

void main(void) {
    enum {closed, opening, open, closing} state;  /* Variable declaration, okay here */

    DP2 = 0x0003;  /* Assignment, okay */
    ODP2 = 0x0000; /* Another assignment, okay */
    state = 1; /* Assignment, okay */

    ...
}

edited Sep 06 '15 at 16:25

answered Sep 06 '15 at 16:06

Some programmer dude

400,186
35
402
621

Oh like the state = 1 line? Yeah sorry the only programming i've done is arduino c++, where it isn't a problem – Jtonkin Sep 06 '15 at 16:12
I don't think I quite understand what you're saying here, which line were you referring to? – Jtonkin Sep 06 '15 at 16:17
@Jtonkin Please see my updated answer. And it's not an issue on the Arduino, because it's using C++ where you can put variable declarations anywhere. – Some programmer dude Sep 06 '15 at 16:25
Actually just a quick question using enum, what is the best way to assign the starting 'state' to it? as in define which of the values state will start off as? – Jtonkin Sep 06 '15 at 16:38
@Jtonkin Your assignment is fine, but I would probably use the proper enumeration symbol instead, like `state = opening;` – Some programmer dude Sep 06 '15 at 16:40
1

Note that GCC 5.1.0 uses C11 by default — at long last! – Jonathan Leffler Sep 06 '15 at 16:45
Thanks! If you have time could I also ask, is my bitwise 'checking' statement right? The one up the top in the #define ActiveSensors – Jtonkin Sep 06 '15 at 16:49
@Jtonkin: No, on multiple levels. I think it should be a function-like macro, and I there should be more parentheses in and around it: `#define ActiveSensors(p) (((p) & 0x0010) == 0x0010 || ((p) & 0x0020) == 0x0020)`. That then begs the question 'is the test as written sensible'. Even when written thus, the code will return true if either or both of the 0x0010 or 0x0020 bits are set, so the code could simply be: `#define ActiveSensors(p) (((p) & 0x0030) != 0)` or `#define ActiveSensors(p) ((p) & 0x0030)` (though this last produces a different numeric result if used in an assignment). – Jonathan Leffler Sep 06 '15 at 20:30

Why isn't my switch case working?/ How can I fix these build errors?

1 Answers1