Simplify code in this state machine? SOLVED

[johnerrington]

my program is working but I feel the code could be simplified for cases 2 - 8 as its a bit repetitive. The switch loop runs every 100msec and reptarget = 6 so the web events are issued every 6 seconds with a 0.1 sec delay between each.
So - could I use a single buffer instead of differnent ones (eg static char myDateBuf[ 20 ] ; ?
is there a way to avoid repeating the " if (repCount >= repTarget) {" ?

just looking for a more elegant solution.

    switch (state) {

      case 0: {
          if (!getLocalTime(&timeinfo)) {
            Serial.println("Failed to obtain time");
            return;
          }
          state = 1;
        }
        break;

      case 1: {
          checkPeriod(); //if 6 min elapsed, record current, previous, max values of watts

          if (timeinfo.tm_mday != currDay) {
            //start of new day
            newDay(); //save day data to file
            currDay = timeinfo.tm_mday; //update current day
          }

          if (timeinfo.tm_mon != currMonth) {
            //start of new month
            newMonth();
          }

          buildDate(); //only needed for web page date & time display
          state = 2;
        }
        break;

      // send web event updates

      case 2: {
          //Serial.printf("repcount is %i \n", repCount);
          if (repCount >= repTarget) {
            events.send("ping", NULL, millis());
          }
          state = 3;
        }
        break;

      case 3: {
          if (repCount >= repTarget) {
            static char myDateBuf[ 20 ] ; // make it big enough including trailing /0. Must be static if locally defined.
            myDate.toCharArray( myDateBuf, 20 ) ;  // myDate is already a String
            events.send( myDateBuf , "time", millis());
          }
          state = 4;
        }
        break;

      case 4: {
          if (repCount >= repTarget) {
            static char mydTotalkWh[ 12] ;
            snprintf(mydTotalkWh, 8, "%7.2f", dTotalkWh);  //buffer, max bytes, format specifier (%[flags][width][.precision][length]specifier), value
            events.send( mydTotalkWh , "dtotal", millis());
          }
          state = 5;
        }
        break;

      case 5: {
          if (repCount >= repTarget) {
            static char myyTotalkWh[ 12] ;
            snprintf(myyTotalkWh, 8, "%7.2f", yTotalkWh);
            events.send( myyTotalkWh , "ytotal", millis());
          }
          state = 6;
        }
        break;

      case 6: {
          if (repCount >= repTarget) {
            static char mypWatts[ 12] ;
            snprintf(mypWatts, 8, "%i", pWatts);
            events.send( mypWatts , "pwatts", millis());
          }
          state = 7;
        }
        break;

      case 7: {
          if (repCount >= repTarget) {
            static char mymWatts[ 12] ;
            snprintf(mymWatts, 8, "%i", mWatts);
            events.send( mymWatts , "mwatts", millis());
          }
          state = 8;
        }
        break;

      case 8: {
          if (repCount >= repTarget) {
            static char mycWatts[ 12] ;
            snprintf(mycWatts, 8, "%i", cWatts);
            events.send( mycWatts , "cwatts", millis());
            repCount = 0; //reset rep count
          }
          else repCount++;
          state = 9;
        }
        break;

      case 9: {
          state = 0;
        }
        break;
    }  //switch

[countrypaul]

You have only shown part of the code, so that makes it difficult to see if some changes are appropriate or not as we cannot tell where else the variables might be used.

1. Why use "static char" but then always change the contents of the variable as the first operation? Static char means the variable is kept like a global and not allocated off the stack as such it can limit your use of memory more than a simple char array that is lost when the function is exited.

2. If there is no need to use static variables then you could use the same buffer, set to the largest size you require in every case, but called something generic such as myBuffer.

3. You increment the state by one every time around the loop in each case except in case 9. It might make the code look tidier to simply put an "if (state == 9) state = 0 else state++;" at the end of the case section.

4. You could use a boolean variable set to "repCount >= repTarget" rather than repeating the calculation in each case.

5. In case 3 you have a comment that the char arrary must be static if locally defined - why? and do you mean declared?

Just a couple of ideas to get you started.

[gcjr]

the code is more of a sequencer than a state machine because each state unconditionally changes to the next state.

without seeing more of the code, it's unclear when this code is invoked. is it invoked only after some input or unconditionally in loop(), in which case the code for each case could just be combined into a single function

[johnerrington]

Thanks, now using a single buffer and its working fine.
Also understand static a bit better now! not relevant in this case as its, as you say, being set each time, and also its not in a function, just in the loop();

@countrypaul

You increment the state by one every time around the loop in each case except in case 9. It might make the code look tidier to simply put an "if (state == 9) state = 0 else state++;" at the end of the case section.

Understand - but I'm happy with my way as it makes testing easier just by changing one eg "state = 3;" to a different value.