Need Help/Suggestion for my coding program:

Hi guys, yesterday I was scrolling through the forum and found a very nice problem that I could try and solve using Implementation of FSM and also Systemic coding method.

The forum post that I was referring to is this one < Noob Needs Help with Unsigned Long >

I dont know how should I attach the coding as the forum stated that I exceeded the 9000 character limit.

I guess I should break the coding into pieces for post reference and also put the attach file so all of you could read it easily.

Anyway the point of me posting here is so anyone could help me critic or suggestion to improve the code better

/**************************************************************************************************
 * Programmer Name: Ashraf Zolkopli                                                               *
 * Contact email  :                                                      *
 * Code Name      : Sequential Zone Light Up based on SwitchState                                 *
 * Version Number : V00.01                                                                        *
 * Purpose        : When the Switch is being press;                                               *
 *                  Zone1 to Zone 5 will light up in sequence at a 1.2s interval;                 *
 *                  Once all Zone have light up, wait for 2s and turn off all 5 zone;             *
 *                  Sequence will not start again untill the Switch had been repress;             *
 *                  at any moment, if the switch is release, all zone will turn off and reset     *
 *************************************************************************************************/

/********************************** < Library Inclusion > *****************************************
 * Include all the library require for this project                                               *
 * Since this project require a mechanical switch input, using a debounce/change state library    * 
 * would be great. My prefered Debounce Library is Bounce2 created by thomasfredericks            *
 *The links to the library <https://github.com/thomasfredericks/Bounce2>                          *
 *************************************************************************************************/

#include <Bounce2.h>

/***************************** < End of Library Inclusion > ***************************************/



/********************************* < Assign Input Pin > ********************************************       
 *  This Project just use only one input, A switch connected to pin 2 will a pull down resistor    *
 *  the declaration of variable name could be either the following structure:                      *
 *                                                                                                 *
 *  #define switchPin 2                                                                            *
 *                                                                                                 *
 *  or                                                                                             *
 *                                                                                                 * 
 *  const uint8_t switchPin = 2;                                                                   *
 *                                                                                                 *
 *  Ussage for variable is the same for both delaration                                            *
 **************************************************************************************************/

const uint8_t switchPin = 12;

/***************************** < End of Assign Input Pin > ****************************************/



/********************************* < Assign Output Pin > *******************************************    
 *  This project used the built in LED that is connected to pin 13                                 *
 *  This project also make used of 5 relay that is connected to pin 3, 5, 6, 9, and 10 respectively*
 *  These relay will control the zone of which Output will turn on                                 *
 *  Therefore a good varible name for this relay pins would be zone                                *
 *  The repetition of variable name indicated a good candidated to used array[]                    *
 *  One thing I like to have when having an array is to have a the size of the array               *
 **************************************************************************************************/

const uint8_t LEDPin = 13;
const uint8_t zone [] = { 6, 3, 15, 18, 23 };

const uint8_t num_of_zone = sizeof ( zone ) / sizeof ( uint8_t );

/***************************** < End of Assign Output Pin > ***************************************/

Moderator edit: removed email address.

Sequential_Zone_Light_Up_FSM.ino (9.28 KB)

/********************************* < Assign Variable > *********************************************    
    Info will be added later ......
 **************************************************************************************************/
#define outputLogic false
#define pullUp true
#define pin13Logic true
#define debugMode false
#define serialMode false
#define baudrate 250000


#define time_between_zone 1200
#define time_to_off 20000

unsigned long interval = time_between_zone;
unsigned long timePrevious = 0;


/***************************** < End of Assign Variable >    ***************************************/



/********************************* < Assign FSM > **************************************************  
 *  Finite State Machine is a method / technique to manage the flow of system to enable a          *
 *  systematic programming experience. FSM allow the program to be easily program and debug        *
 *                                                                                                 *
 *  For this project, the state is called ZoneState and there are 6 know state:                    * 
 *  1 ) Standby                                                                                    *
 *  2 ) Zone1                                                                                      *
 *  3 ) Zone2                                                                                      *
 *  4 ) Zone3                                                                                      *
 *  5 ) Zone4                                                                                      *
 *  6 ) Zone5                                                                                      *
 *                                                                                                 *
 *  The declaration of each zone is using #define while the zoneState is a uint8_t                 *
 **************************************************************************************************/

#define standby 0 
#define zone1 1
#define zone2 2
#define zone3 3 
#define zone4 4
#define zone5 5

uint8_t ZoneState = standby;


/***************************** < End of FSM >    ***************************************************/
Bounce Sw = Bounce(); 



void setup()
{
  initSerial();
  initInput();
  initOutput(); 
  if (debugMode) Serial.println ( " Void Setup Finish " );
}


void loop()
{
  Sw .update();
  unsigned long timeNow = millis();
  if ( pullUp? Sw. fell() : Sw. rose() )
   {
      ZoneState = zone1;
      timePrevious = timeNow;
      digitalWrite ( LEDPin, pin13Logic );
      if (debugMode) Serial.println ( "ZoneState = zone1" );
   }
  if ( pullUp? Sw. rose() : Sw. fell() )
   {
      ZoneState = standby;
      digitalWrite ( LEDPin, !pin13Logic );
      if (debugMode) Serial.println ( "ZoneState = standby" );
   }

  switch ( ZoneState )
   {
     case standby:
       resetZone();
       interval = time_between_zone;
       //if (debugMode) Serial.println ( "ZoneState = standby" );
       break;
     case zone1:
       digitalWrite ( zone [0], outputLogic);
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = zone2;
          if (debugMode) Serial.println ( "ZoneState = zone2" );
       }
       break;
       case zone2:
       digitalWrite ( zone [1], outputLogic);
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = zone3;
          if (debugMode) Serial.println ( "ZoneState = zone3" );
       }
       break;
       case zone3:
       digitalWrite ( zone [2], outputLogic);
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = zone4;
          if (debugMode) Serial.println ( "ZoneState = zone4" );
       }
       break;
       case zone4:
       digitalWrite ( zone [3], outputLogic);
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = zone5;
          if (debugMode) Serial.println ( "ZoneState = zone5" );
       }
       break;
       case zone5:
       digitalWrite ( zone [4], outputLogic);
       interval = time_to_off;
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = standby;
          if (debugMode) Serial.println ( "ZoneState = standby" );
       }
       break;
   }
}


void initInput()
{
  pinMode ( switchPin, INPUT );
  digitalWrite ( switchPin, pullUp );
  Sw .attach(switchPin);
  Sw .interval(10);
}

void initOutput()
{
 pinMode ( LEDPin, OUTPUT );
 digitalWrite ( LEDPin, !pin13Logic );
 digitalWrite ( LEDPin, !outputLogic );
 for ( uint8_t i = 0; i < num_of_zone; i++ )
 {
    pinMode ( zone [i], OUTPUT);
    digitalWrite ( zone [i], !outputLogic);
 }
}

void initSerial()
{
  if ( debugMode || serialMode ) 
  {
    Serial.begin ( baudrate );
    while ( !Serial ) {}
    if ( debugMode ) Serial.println ( "< debugMode Start >" );
    if ( serialMode ) Serial.println ( "< serialMode Start >" );
  }
}

void resetZone()
{
  for ( uint8_t i = 0; i < num_of_zone; i++ )
 {
    digitalWrite ( zone [i], !outputLogic);
 }
}

End of coding…

Sorry guys half way through commenting my work suddenly my laziness struck me. Anyway please feel free to comment my work

For ZoneState, it would have been better to use an enumerated type:

#define standby 0 
#define zone1 1
#define zone2 2
#define zone3 3 
#define zone4 4
#define zone5 5

uint8_t ZoneState = standby;

http://www.cprogramming.com/tutorial/enum.html

Also you should edit and remove your email address from the code.

if (debugMode) Serial.println (F( " Setup Finish ") );

Don't introduce new bugs with your debug code.

@aarg:

if I understand you correctly, what you are suggesting is that I change this part

#define standby 0 
#define zone1 1
#define zone2 2
#define zone3 3 
#define zone4 4
#define zone5 5

uint8_t ZoneState = standby;

to maybe something along this line

enum ZoneState_t  { standby, zone1, zone2, zone3, zone4, zone5};
ZoneState_t ZoneState = standby;

I do see now how the implementation of enum could help keep the FSM confined to the state stated.

on the second note: Sorry guys, I guess I just copy and paste my template and forget to edit my email

@Awol:

Thanks Awol, I could I forget to use F function

Look at the duplication here. Can't you factor the code?

       case zone2:
       digitalWrite ( zone [1], outputLogic);
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = zone3;
          if (debugMode) Serial.println ( "ZoneState = zone3" );
       }
       break;
       case zone3:
       digitalWrite ( zone [2], outputLogic);
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = zone4;
          if (debugMode) Serial.println ( "ZoneState = zone4" );
       }

If you had 50 zones, you would need 50 of these statements.

@aarg:

Actually you lost me there. I know that its repetitive and in any good code that have many repetitive component could be refactor into function.

for this particular implementation, I could see that I could refactor this portion

digitalWrite ( zone [0], outputLogic);
case zone1:
       digitalWrite ( zone [0], outputLogic);
       if ( timeNow - timePrevious >= interval )
       {
          timePrevious = timeNow;
          ZoneState = zone2;
          if (debugMode) Serial.println ( F("ZoneState = zone2") );
       }

into something like this

case zone1:
    updateState(zone2,timeNow,interval);
    break;

whereby the function call could be

void updateState(int nextState, unsigned long currentTime, unsigned long duration)
{
  digitalWrite ( zone [ ZoneState - 1 ], outputLogic );
  if ( currentTime - timePrevious >= duration)
  {
    timePrevious = currentTime;
    ZoneState = nextState;
    if (debugMode)
    {
       Serial.print ( F("ZoneState = ") );
       Serial.print ( nextState );
       Serial.print ( F("\n") );
    }
  }
}

but I do think this method is not as robust as want it to be.

but I do think this method is not as robust as want it to be.

But it's a whole lot easier to debug, it's less prone to cut and paste errors, and overall, the code is shorter.