1
\$\begingroup\$

I've started to program a state machine on a PIC18F2550 microcontroller. In each state of my machine, there is a designated block of code that runs for a specific amount of real time, such as 20 or 30 seconds. To do this, I use a countdown timer that is decremented once every second. Depending on the state the machine is in, the value of countdown is used to control LEDs, beep speakers, put stuff on the LCD or to run some other dependent tasks (i.e. countdown is versatile; lots of different things at different times rely on it) Since these peripherals are far too slow to call in the interrupt, in my states I just turn the timer on and poll the countdown variable, then fire actions every second (or half second) based on that. This has worked almost perfectly so far.

Understandably, by polling the variable and taking chunky amounts of time by writing to things like the LCD or speaker, I open myself up to being late to respond to the countdown variable when it's changed by the interrupt. Case in point: In one state of my machine I've got a flashing light that 'hiccups' every once in a while.

Before I post a wall of code, the basic control flow of every state is summed up by this pseudocode:

  1. Update the LEDs
  2. Print the state to the LCD (which is daisy chained to the LED registers, so printing something to the LCD updates the LEDs too)
  3. Some states may then wait here, should the machine be manually (remotely) controlled. Remote control may cause it to branch out and go to a new state as indicated in a serial packet (changed within a high priority interrupt)
  4. If the machine is running automatically, or switches to automatic mode from manual mode, it loads and starts the countdown timer
  5. Inside the countdown loop, it frequently updates both the RTC and the countdown times on the LCD
  6. Events such as flashing an LED or beeping a speaker then fire every second or half second
  7. The timer runs out and is shut off within the interrupt; the loop exits. Update the screen again and figure out which state to go next.

Here's an exerpt of my code. The hiccups occur in state 5:

#pragma interruptlow low_isr    //Timers
void low_isr(void){

    if(INTCONbits.TMR0IF){          //Countdown timer update
        OvfT0++;
        if(OvfT0<13){Tick = 0;}     //For half second/flashing
        if(OvfT0>13){Tick = 1;}

        if(OvfT0==25){              //1 second
            OvfT0=0;
            Countdown--;
            if(Countdown==0){
                T0CONbits.TMR0ON = 0;

            }
        }
        TMR0H = 0x9F;               
        TMR0L = 0xFF;               //Preload counters with 40959 -> 24576 ticks to overflow every 40ms 
        INTCONbits.TMR0IF = 0;
    }

    if(PIR1bits.TMR1IF){        //RTC Update
        OvfT1++;
        if(OvfT1==25){          //25 overflows -> 1 second
            OvfT1=0;
            RTCSec++;
            if(RTCSec==60){
                RTCSec=0;
                RTCMin++;
                if(RTCMin==60){ 
                    RTCMin=0;
                    RTCHour++;
                    if(RTCHour==24){
                        RTCHour=0;
                    }
                }
            }
        }
        TMR1H = 0x3F;               //Preload counters with 16383 -> 49152 ticks to overflow every 40ms 
        TMR1L = 0xFF;
        PIR1bits.TMR1IF = 0;
    }   
}

void statemachine(void){
    while(1){               
        switch(State){
            case 4:             //NSC -- North/South Green long cycle (crossing)
                LEDReg1 = 0x64;
                LEDReg2 = 0x89;
                cmdlcd(0xC9);
                putslcd(st[State]);
                if(RemoteOp){
                    while((State==4) && RemoteOp){idle();}  
                    if(State!=4){break;}
                }   
                Countdown = LongCycle;
                T0CONbits.TMR0ON = 1;
                while(T0CONbits.TMR0ON){
                        Delay10KTCYx(10);
                        updatetimes();
                        if((Countdown>=15) && Tick){
                            beephigh();
                            while(Tick){}
                        }   
                }
                updatetimes();  
                Delay10KTCYx(123);      
                State = 5;  
            case 5:             //NSF -- North/South Green w/flashing crosswalk lights (falls in from state 4)
                LEDReg1 = 0x64;
                LEDReg2 = 0x89;
                cmdlcd(0xC9);
                putslcd(st[State]);

                Countdown = FlashCycle;
                T0CONbits.TMR0ON = 1;
                while(T0CONbits.TMR0ON){
                        Delay10KTCYx(10);
                        updatetimes();
                        if(Tick){
                            LEDReg1 = 0xA4;
                        }
                        else{
                            LEDReg1 = 0x24;
                        }       
                }
                updatetimes();      
                State = 6;
            case 6:             //NSY -- North/South Yellow
                LEDReg1 = 0x92;
                LEDReg2 = 0x89;
                cmdlcd(0xC9);
                putslcd(st[State]);
                if(RemoteOp){
                    while((State==6) && RemoteOp){idle();}  
                    if(State!=6){break;}
                }   
                Countdown = YellowCycle;
                T0CONbits.TMR0ON = 1;
                while(T0CONbits.TMR0ON){
                        Delay10KTCYx(10);
                        updatetimes();
                }
                updatetimes();  
                Delay10KTCYx(123);      
                State = 7;
            case 7:             //NSR -- North/South Red
                LEDReg1 = 0x89;
                LEDReg2 = 0x89;
                cmdlcd(0xC9);
                putslcd(st[State]);
                if(RemoteOp){
                    while((State==7) && RemoteOp){idle();}  
                    if(State!=7){break;}
                }   
                Countdown = RedCycle;
                T0CONbits.TMR0ON = 1;
                while(T0CONbits.TMR0ON){
                        Delay10KTCYx(10);
                        updatetimes();
                }
                updatetimes();  
                Delay10KTCYx(123);      
                while(1){       //replace with future branching code        
                    idle(); 
                }

                break;
        }
    }
}

I'm guessing there's a "tighter" way to do all this, but right now it all works perfectly save for at least one hiccup when flashing the LED in state 5. There's probably a lot of optimization that could be made here, but my main focus is to just get something working first.

I'd greatly appreciate any sort of help improving this code.

Compiler is MPLAB/C18

\$\endgroup\$
1
\$\begingroup\$

General

Difficult to read, don't you think? Code is normally a 'write-once, read-often' operation. Therefore it makes sense to make it easy to read, even if it takes longer to write.

  • if, while etc should be followed by a space and operators such as = of == should be surrounded by spaces.

  • Functions start with the brace in column 0. Really.

  • Spreading constants (13, 25, 9F/FF, etc) around your code is a bad idea.  It   makes modifying the code much more error-prone.  Even if you think you will   never need to modify them, it is better to define them in #defines at the   top of the relevant file or in a header.

In low_isr

  • You could simplify this

    if (OvfT0 < 13) {Tick = 0;}
    if (OvfT0 > 13) {Tick = 1;}
    

    to

    Tick = (OvfT0 > 13);
    

    assuming that it doesn't matter what happens when OvfT0 == 13 which is not clear from your code.

  • Your variables such as INTCONbits.TMR0IF, OvfT0, TMR0H etc are presumably pointers to registers; they are the sort of mnemonics beloved of hardware makers. However, you don't have to suffer such cryptic names - you can use inline functions to abstract away the interface - inline to avoid a call-overhead.

    inline int timer_overflow(void) { return INTCONbits.TRM01F; }
    inline void timer_overflow_reset(void) { INTCONbits.TRM01F = 0; }
    
    //Preload counters ...
    inline void preload_timer(void) { TMR0H = ...; TMR0L = ...; }
    
  • your handling of the RTC values is unsafe. By writing three separate values from the ISR you guarantee that at some point, application level (ie. non-isr) code is going to read one of the three values and then get interrupted by this ISR; when the ISR returns the time has changed and it reads a corrupted value. To avoid this I suggest you have a single RTCTime value that holds just the seconds - make sure it is small enough to be readable with one machine instruction (normally an int or int32_t should do fine). Then do the conversion to hours /mins/secs at application level - read the value once from RTCTime and convert that value.

  • make sure the values being written by the ISR are declared volatile

  • deeply indented code is normally wrong.

In statemachine

  • state_machine is easier to read

  • switch should have a default case, even if you think you have covered all the 'likely' cases. You probably haven't.

  • this is pretty unreadable. You should probably separate each state into its own function.

  • the cases all fall through without any comments...! This seems unnecessary, as it is all in a while loop anyway.

  • it seems likely that you can factor-out some common code shared by all states, eg:

    LEDReg1 = 0x..;
    LEDReg2 = 0x..;
    cmdlcd(0xC9);
    putslcd(st[State]);
    ...
    updatetimes();
    
  • this code is repeated in three states with different state values.

    if(RemoteOp){
        while((State==6) && RemoteOp){idle();}
        if(State!=6){break;}
    }
    

    presumably another thread or an ISR is changing State or RemoteOp, otherwise this is an infinite loop. A function might be better:

    static inline int remote_state_change(int current_state)
    {
        if (RemoteOp) {
            while ((State == current_state) && RemoteOp) { idle(); }
            return State!=6;
        }
        return 0;
    }
    

    And call with

    if (remote_state_change(State)) {break;}
    

BTW, do you have any OS support (microkernel etc) or is this bare-metal?

\$\endgroup\$
2
  • \$\begingroup\$ Wow, thanks for going through my code so thoroughly. Here's some comments in response: I'll definitely go over the code and improve the whitespace. Nice catch in low_isr re: Ticks, but re: RTC I'm programming on 8-bit metal, so 24 bit ops add a lot of overhead for something that hasn't been a problem (so far). The full state machine is 32 states so I wanted to spare everyone the wall; those states in particular had the simplest branch logic. RemoteOp and State do change by ISR, so I'll add volatile. I pulled out the print statement to above the switch. I'll try the rest of your code tomorrow! \$\endgroup\$ – BB ON Mar 11 '13 at 5:00
  • \$\begingroup\$ I should have considered that the processor might not have 32-bit regs. The time-reading problem will occur - it is not 'if', but 'when'. Whether it matters depends upon what you do with the time. The overhead you have already for setting H:M:S is probably similar to the cost of a 24-bit access, but more importantly, as it is 8-bit a 24-bit access would not be atomic anyway so you gain nothing by changing. Instead, use a simple application-level function that loops reading the H:M:S until it gets the same result twice. \$\endgroup\$ – William Morris Mar 11 '13 at 13:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.