General Program Architecture Assistance

Hi all. I need opinions, criticism, advice, ideas....anything for the efficiency of my way of thinking. I am trying to write Arduino code for the project shown below in the block diagram that represents a typical application:

It should work like this: (the diagram is self explanatory even for those who are not familiar with state machines) .

I think pictures are much easier and faster to understand than words.

Now, the way I programmed it is as follows:

volatile int A = 0;//choosing pins for the 3 I/Ps and 2 O/Ps
volatile int B = 1;
volatile int C = 2;
volatile int X = 3;
volatile int Y = 4;

void setup(){
pinMode(X, OUTPUT);
pinMode(Y, OUTPUT);
attachInterrupt (A, InterruptServiceRoutine, RISING);//interrupt I/P from pin A (or 0)

digitalWrite(X, LOW);    //Initializing the O/P's
digitalWrite(Y, LOW);
}

void loop()    //nothing happens here. A structural decision that I'm not sure of
{
}

void InterruptServiceRoutine()
{
  
    while(!B)      //state 0 starts here and exits when a pulse from B comes
    {
        digitalWrite(X, HIGH);
        digitalWrite(Y, LOW);
    }
    
    digitalWrite(X, LOW);         //state 1 starts here and lasts for 6 seconds
    digitalWrite(Y, LOW);
    delay (6000);
    
    do             //start of State 2 and wait for a pulse from C
    {
        digitalWrite(X, LOW);
        digitalWrite(Y, HIGH);
    } while (!C);
    
    digitalWrite(X, LOW);
    digitalWrite(Y, LOW);
}

/*Assuming that the length of the pulses that come from mechanical devices 
have a much longer duration than the the time that it takes the microcontroller 
to execute the few lines in the 2 loops. Is that a legitimate assumption?*/

What a mess.

In the first place why use an ISR, particularly one that has a 6 second delay in it and two potentially infinite while loops ?

You have described a state machine so why not use its structure in the program. Something like this pseudo code

state = 0

start of loop
  switch(state)
    case 0
      if input A HIGH
        state = 1
        set X = 1
        set Y = 0
      end of if  
    break
    
    case 1
      if input B HIGH
        state = 2
        X = 0
        Y = 0
        set stateStart = millis()
      end of if
    break
    
    case 2
      if millis() - stateStart greater than interval
        state = 3
        X = 0
        Y = 1
      end of if
      break
      
    case 3
      if input C HIGH
        state = 0
        X = 0
        Y = 0
      end of if
      break
      
  end of switch
  
end of loop

This structure is non blocking, unlike your attempt.

You need to be careful when choosing your input and output pins that they do not clash with other functionaliy of pins such as Serial TX/RX. Which Arduino are you using ?

void InterruptServiceRoutine()
{
...
    delay (6000);

No. Just, no.

http://www.gammon.com.au/interrupts

  while(!B)      //state 0 starts here and exits when a pulse from B comes
    {
        digitalWrite(X, HIGH);
        digitalWrite(Y, LOW);
    }

B doesn't change here, so I don't see the point of the loop.

volatile int A = 0;//choosing pins for the 3 I/Ps and 2 O/Ps
volatile int B = 1;
volatile int C = 2;
volatile int X = 3;
volatile int Y = 4;

I strongly suggest better variable names. A, B, C, X, Y are useful in maths, but they don't serve any useful purpose here.

Very interesting corrections!

first, @UKHeliBob, The reason I used the interrupt (and its ISR was actually the whole sketch) is because clicking the A (START) button is an asynchronous event and it triggers the execution of the whole series of states.

I should've explained that X & Y go to a motor circuit that moves in two directions (say, X is up and Y is down) and when it moves up it always hits a limit switch (input B) - I can put a timed break from the loops if the limit switch(es) stops working.

Now, the main theme is that hitting the start button (A) will always be followed by the limit switch contact (B) at some point, and then after what you called "interval", the motor reverses until another limit switch (C) closes to go back to idle waiting for another start click.

I loved your pseudocode (especially mixing "switch" and "if") and even though it has a potential of endlessly looping through one of the cases if the next event/input does not happen. I will even generalize/simplify it for any beginner with Arduino sketches for any state machine:

state = 0

start of loop
  switch(state)
    case 0
      if input A HIGH
        state = 1
        
         //add statements here

      end of if  
    break
    
    case 1
      if input B HIGH
        state = 2
        
           //add statements here

       set stateStart = millis()
      end of if
    break
    
    case 2
      if millis() - stateStart greater than interval
        state = 3
        
             // add statements here

      end of if
      break
      
    case 3
      if input C HIGH
        state = 0
       
                //statements for the last state

      end of if
      break
      
  end of switch
  
end of loop

I loved your pseudocode (especially mixing "switch" and "if") and even though it has a potential of endlessly looping through one of the cases if the next event/input does not happen.

If the event that triggers a change of state does not occur then of course it will stay in that state. That is the whole point, surely. What it will not do is to block execution of code that should run when it is any particular state (inside the relevant case statement), nor code that should always run no matter what the state (outside of the case statement)

Your reasoning for using an ISR, at least in the way that it was implemented, is flawed. If the click of the A input is asynchronous then by all means trigger an ISR to set a flag indicating that it has happened and deal with it in the loop() function, but we need to know what should happen if A is clicked when the system is in states other than zero.

vincenzo2: The reason I used the interrupt (and its ISR was actually the whole sketch) is because clicking the A (START) button is an asynchronous event and it triggers the execution of the whole series of states.

You should be able to test for the start button in the main loop. Within a few microseconds of it being pressed, you could then call a function to do what you want. Running everything in an ISR is not what they are designed for.

UKHeliBob I shouldn't have thought of an interrupt at all. Thank you so much. I learned so much from you.

Mr. Gammon I'm a big fan of your posts and loved reading your interrupt tutorial. As for using A, B... for variable names, I know that names should be meaningful, but it's just a very small code snippet that was made to make it easier for readers like you to get it faster (I'm very probably wrong) and I always use "real" names and full documentation for useful programs that go to boards and are saved as finals in my code folder for future reference

My original attempt to write code for the state machine was full of flaws and I didn't even upload it to a real board. It was just a thought and I learned a lot from your comments. Thank you so much. This forum is great and there are great members and administrators who really enjoy being creative.

How about using interrupts for reading inputs from a rotary encoder? can that be included in the switch statement and polled frequently enough to not loose a pulse?

I have a page about rotary encoders and interrupts. :)

http://www.gammon.com.au/forum/?id=11130