State Machine in C (ATmega)

Question

I am trying to do a data transfer control. A button with a pull-up resistor is connected to PINA3 of ATmega644P(Slave). I want to send some data when the button is pressed, if only the bus line is free. For testing purposes, I use another ATmega644P(Master) to send data ,which makes the bus busy, to the Slave one.

This is what I am trying to do: When the button is pressed, start the timer and wait for 300ms. During this time, if it receives some data, go to bus_not_free state. If the timer reaches 300ms while the bus is free, send the data. It works great when the bus is free, but sometimes, it sends the data when the bus is busy.

#include <stdlib.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/pgmspace.h>

#include "uart.h"
#include "bus_free_check.h"
#include "specs.h"

#define F_CPU 4000000UL
#define UART_BAUD_RATE      19200

enum states{
    idle_s,
    start_timer_s,
    wait_s,
    bus_check_s,
    send_data_s,
    bus_not_free_s
};

enum states state=idle_s;

int main(void)
{
    DDRA = 0x00;    //  PORTA is input. 
    DDRB = 0xFF;    //  PORTB is output.
    DDRC = 0xFF;    //  PORTC is output.
    DDRD = 0b11111010;  //  PORTD input/output.

    PORTA = 0x00;
    PORTB = 0x00;
    PORTC = 0x00;

    TCCR1B = (1 << WGM12);
    TIMSK1 = (1 << OCIE1A);

    OCR1A = 1171;   //  300ms

    uart_init( UART_BAUD_SELECT(UART_BAUD_RATE,F_CPU) );

    sei();

    while(1)
    {   
        switch(state)
        {
            case idle_s:
                bus_free=0;
                PORTC=0x01;             

                if (bit_is_clear(PINA,3)) // If the button is pressed
                {
                    while(bit_is_clear(PINA,3)); // Wait until it is unpressed
                    {
                        state=start_timer_s; // Start the timer                         
                    }
                } 
                else
                {
                    state=idle_s; // Wait in the idle_s until the button is pressed
                }

                break;

            case start_timer_s:
                TCCR1B |= (1 << CS10) | (1<< CS12); // Start the timer
                state=wait_s; // Go to wait state and wait for 300ms

                break;

            case wait_s:
                if (bit_is_clear(PIND,0))   // During this waiting, if at any time there is a data transfer going on, go to bus_not_free state.
                {
                    TCCR1B &= ~((1 << CS10) | (1 << CS11) | (1 << CS12)); // Stop the timer
                    TCNT1=0; // Reset the counter
                    bus_free=0;
                    state=bus_not_free_s; // Go to bus_not_free state
                }

                break;

            case send_data_s:
                send_target_seri_no(); // Bus is free, you can send the data
                TCCR1B &= ~((1 << CS10) | (1 << CS11) | (1 << CS12)); // After sending the data, stop the timer.
                TCNT1=0; // And reset the counter.
                state=idle_s; // Go to idle_s and get ready

                break;

            case bus_not_free_s:
                uart_puts("Bus is not free \r"); // Bus is not free, can't send the data
                state=idle_s; // Go to idle state, and don't give up hope

                break;

            default:
                uart_puts("Fatal Error \r");
                break;
        }               
    }
}

ISR(TIMER1_COMPA_vect) // If the timer reaches 300ms, that means the bus is free
{
    bus_free=1;
    if(state==wait_s) // When the timer hits 300ms, if it is still in wait_s state, send the data.
    {
        state=send_data_s;
    }
}

Thank you.

Answer 1

You've got several problems here. One of them is that your state machine is a little off.

Starting in the idle state, you only leave it if the button is pressed. Is this a mechanical button? If so, it needs to be debounced. That means, basically, waiting for it's state to become stable, and THEN determining if a button press has occurred. Spurious input from the button could cause you to pop in and out of the idle state, confusing both you and your program.

You are also abusing the timer a bit. You don't need the timer on if you are not trying to send something, so turn it off when you are done with it. You are also setting the timer over and over again while waiting for the timeout. Not necessary. I'm pretty sure the logic in the timer function is off. It's hard to tell what you meant to do.

The bus check state is the biggest problem. It does too much. You only need to start the timer once when moving out of the idle state. So separate that into another state. Then all the check state does is move to the send state when the bus is free. Your "three tries" loop is not doing anything: it doesn't wait for anything to change.

The reason you get stuck in the "bus not free" state is this: Suppose that the bus input is 0 (not busy) and bus_free=0. How will it ever set bus_free to 1? Your check time function will, in that case, turn off the timer, so the ISR never fires to change bus_free.

Answer 2

I did not get intended function but I can see two bugs.

1) useless code

for(int i=0; i>3; i++)  // Try three times before canceling
{
  state=bus_check_s; // Go back and check again
  PORTB=0x00; // Led is off
  bus_free=0; // Bus is not free
}

state=bus_not_free_s; // After 3 attempts, go to bus_not_free_s and warn the user
bus_free=0; // Bus is not free

it can be simplified to

state=bus_not_free_s;
PORTB=0x00;
bus_free=0; // Bus is not free

2) copy&paste error (if you want really stop timer)

TCCR1B |= (0 << CS10) | (0 << CS11) | (0 << CS12); // Stop the timer.

change to

TCCR1B &= ~((1 << CS10) | (1 << CS11) | (1 << CS12))

Answer 3

I have changed the if condition in wait_s as:

case wait_s:
// rec_char=uart_getc();
                if ((unsigned char) rec_char)   // During this waiting, if at any time there is a data transfer going on, go to bus_not_free state.
                {
                    TCCR1B &= ~((1 << CS10) | (1 << CS11) | (1 << CS12)); // Stop the timer
                    TCNT1=0; // Reset the counter
                    state=bus_not_free_s; // Go to bus_not_free state
                }

Now I am not facing the problem I stated in my question. I guess my mistake was to say bus is busy if I receive a zero bit only.