can I simplify this switch case code?

I’m pinging several different URLs and recording responses, using a switch/case to implement a state machine. For now I’ve stripped out the timing code.

However each case has a similar bit of code. Can anyone suggest if it can be simplified?

switch ( target ) {
    case 0:
      timeASM = millis();
      isFinished = false;
      pingOK = pinger.Ping(IPAddress(host1[target].IP), 5, 200);
      target++;
      break;

    case 1:
      if (isFinished == true )
      {
        host1[target - 1].tPing = pingAvg;
        host1[target - 1].nLost = pingsLost;
        isFinished = false;
        pingOK = pinger.Ping(IPAddress(host1[target].IP), 5, 200)'
        target++;
      }
      break;

    case 2:
      if (isFinished == true )
      {
       host1[target - 1].tPing = pingAvg;
        host1[target - 1].nLost = pingsLost;
        isFinished = false;
        pingOK = pinger.Ping(IPAddress(host1[target].IP), 5, 200)'
        target++;
      }
      break;


    case 3: .. case7: are identical to case 1&2
    

    case 8:
      //flash LEDs according to number of lost (or good) pings.
      for (int n = 0; n <= 5; n++) {
        Serial.print("\n channel "); Serial.print(n); Serial.print(" loss value "); Serial.println(host1[n].nLost);
        //the lower limit on analogWrite() is 100Hz, but startWaveform() doesn’t have that limit:
        //startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t runTimeUS);  //using ping times but pingAverages are wrong.
        startWaveform(dPin[n], (host1[n].nLost * 50000) + 10, ((5 - host1[n].nLost ) * 50000) + 10, 0); //using lost pings: 0=LED ON, 1-4 = flashing, 5=LED OFF
      }
      target = 0;
      break;
  }//switch (target)

The obvious solution is to put the common code in a function and call it when needed

UKHeliBob:
The obvious solution is to put the common code in a function and call it when needed

...and if that function were a state machine in its own right, which started the ping (when isFinished == true), waited until it had finished (while isFinished == false), then recorded the result before setting isFinished = true again, and finally return true to indicate it had finished.

That way you’d get around the whole asymmetry which...
a) increments target in state 0, even though the ping is not complete (so really you should remain in state = 0).
b) requires checking global “isFinished == true” in the next state, and recording of results from the previous one (that work should occur in state 0, as it is related to state 0).

Then your code would be...

switch ( target ) {
    case 0:
    case 1:
    ...
    case 7:
       if (PerformPing(target)) target++;
       break;
    case 8:
      //display results
      target = 0;
      timeASM = millis();
      break;
}

In fact hardly any need for a switch/case at all (you could do it with a single if/else).

And your PerformPing “state machine” becomes....

bool PerformPing(int i)
{
   if (isFinished)
   {
       pingOK = pinger.Ping(IPAddress(host1[i].IP), 5, 200);
       isFinished = false;
   }
   else if (ping complete) // however you currently decide to set isFinished = true but haven’t shown us
   {
       host1[i].tPing = pingAvg;
       host1[i].nLost = pingsLost;
       isFinished = true;
   }
   return isFinished;
}

It may require more states (instead of just a Boolean flag) depending on what is required to complete the ping. But you didn’t show us all you code so we can’t say.

Thanks for your help pcbbc; I met so many problems handling the state machine (stack overflows) that I've reverted to the switch-case, just used a range 1..7 to reduce the code. II'm not happy with the assymetry, but it works.

johnerrington:
Thanks for your help pcbbc; I met so many problems handling the state machine (stack overflows) that I've reverted to the switch-case, just used a range 1..7 to reduce the code. II'm not happy with the assymetry, but it works.

I can’t see how the code I presented would cause a stack overflow. That issue must be in some code you haven’t yet presented.
At a complete guess I would say you are recursively calling some function (e.g. loop) from somewhere else in your code.
Present your complete code and we can help.... don’t and your on your own.

Thanks again pcbbc I didnt want to be too demanding!
Here is the whole code

/*****************************************************************************
  Arduino library handling ping messages for the esp8266 platform
  MIT License  Copyright (c) 2018 Alessio Leoncini


*****************************************************************************/

#include <Pinger.h>
#include <ESP8266WiFi.h>
#include <core_esp8266_waveform.h>

extern "C"
{
#include <lwip/icmp.h> // needed for icmp packet definitions
}

// to do the pin mapping for the Lolin NodeMCU pins D0 - D8
const int dPin[] = {16, 5, 4, 0, 2, 14, 12, 13, 15};

//recording millis() times & set to repeat every 10 second
unsigned long previousMillis = 0;
unsigned long currentMillis;
const long interval = 10000;
bool roundComplete, setComplete=0; //flags all 1 round to a single target,  or all 1 set of pings done

int count = 6; //call nntp and save to SD every 60 sec = 1min

//data for ping:
int pingNumber = 5; //do 5 pings per set
int pingTime = 200; //200ms timeout

struct host {
  byte IP[4];
  int tPing;
  int nLost;
};

//create data set of type host, &  initialise our struct for all cases
host host1[7] = {
  {{192, 168, 1, 1}, 0, 0 } ,  //0; router
  {{192, 168, 1, 11}, 0, 0 } , //1: RYZEN5
  {{192, 168, 1, 16}, 0, 0 } , //2: ALIVE
  {{192, 168, 1, 22}, 0, 0 } , //3: LP
  {{192, 168, 1, 14}, 0, 0 } , //4: GeoBRDG
  {{192, 168, 1, 24}, 0, 0 } , //5: NAS
  {{107, 162, 133, 62}, 0, 0 } //6: TalkTalk
};

const char* ssid     = "TALKTALKCC34C4";
const char* password = "MVGGX799";
byte target = 0; //select url to ping

int pingAvg;
int pingsLost;
bool isFinished;

// Set global to avoid object removing after setup() routine
Pinger pinger;

bool PerformPing(int url)
{
  if (roundComplete)  //ready for next set of pings
  {
    Serial.print(" target ");     Serial.print(url);     Serial.println(" sending ping ");
    isFinished = false;
    bool pingOK = pinger.Ping(IPAddress(host1[url].IP), pingNumber, pingTime);
    roundComplete = false;
  }
  else if (isFinished) // one set of pings to a single target complete and results in
  { Serial.print(" target ");     Serial.print(url);     Serial.println(" values in ");
    isFinished = false;
    host1[url].tPing = pingAvg;
    host1[url].nLost = pingsLost;
    roundComplete = true;
    url++;  target=url;
    if (url == 7) {    
      setComplete = true; //done 1 round to all targets
      Serial.print(" target ");     Serial.print(target);     Serial.println(" set complete ");
    } else {
      setComplete = false; 
    }
  }
  return setComplete;
}

int showLEDs()
{
  //flash LEDs according to number of lost (or good) pings.
  for (int n = 0; n <= 6; n++) {
    Serial.print("\n target "); Serial.print(n); Serial.print(" loss value "); Serial.println(host1[n].nLost);
    //the lower limit on analogWrite() is 100Hz, but startWaveform() doesn’t have that limit:
    //startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t runTimeUS);  //using ping times but pingAverages are wrong.
    startWaveform(dPin[n], (host1[n].nLost * 50000) + 10, ((5 - host1[n].nLost ) * 50000) + 10, 0); //using lost pings: 0=LED ON, 1-4 = flashing, 5=LED OFF
  }
  target = 0;
  return target;
}

void setup()
{
  // Begin serial connection at 74880 baud
  Serial.begin(74880);
  delay(1000); // serial comms is disturbed during a restart

  //set up LED pins as outputs & HIGH (LEDs all off)
  for (int i = 0; i <= 8; i++) {
    pinMode(dPin[i], OUTPUT);
    digitalWrite(dPin[i], 1);
  }

  // Connect to WiFi access point
  digitalWrite(dPin[8], 1); //indicate wifi not connected
  bool stationConnected = WiFi.begin(ssid, password);
  delay(200); //allow time to connect

  // Check if connection errors
  if (!stationConnected)
  {
    Serial.println("Error, unable to connect specified WiFi network.");
  }

  // Wait until connection completed
  Serial.print("Connecting to AP...");
  while (WiFi.status() != WL_CONNECTED)
  {
    delay(200);
    Serial.print(".");
  }
  Serial.print("Ok\n");
  digitalWrite(dPin[8], 0); //indicate wifi connected

  roundComplete = true;  //ready to start a new set of pings

  pinger.OnReceive([](const PingerResponse & response)
  {
    if (response.ReceivedResponse)
    {
      Serial.print("ping response time is "); Serial.print(response.ResponseTime); Serial.println(" msec");
    }
    else
    {
      Serial.printf("Request timed out.\n");
    }

    // Return true to continue the ping sequence. If current event returns false, the ping sequence is interrupted.
    return true;
  });

  pinger.OnEnd([](const PingerResponse & response)
  {
    // Evaluate lost packet percentage
    float loss = 100;
    if (response.TotalReceivedResponses > 0)
    {
      loss = (response.TotalSentRequests - response.TotalReceivedResponses) * 100 / response.TotalSentRequests;
    }

    // Print packet trip data
    Serial.printf(
      "Ping statistics for %s:\n",
      response.DestIPAddress.toString().c_str());
    Serial.printf(
      "    Packets: Sent = %lu, Received = %lu, Lost = %lu (%.2f%% loss),\n",
      response.TotalSentRequests,
      response.TotalReceivedResponses,
      response.TotalSentRequests - response.TotalReceivedResponses,
      loss);

    // Print time information
    if (response.TotalReceivedResponses > 0)
    {
      Serial.printf("Approximate round trip times in milli-seconds:\n");
      Serial.printf(
        "    Minimum = %lums, Maximum = %lums, Average = %.2fms\n",
        response.MinResponseTime,
        response.MaxResponseTime,
        response.AvgResponseTime);
    }

    // Print host data
    Serial.printf("Destination host data:\n");
    Serial.printf(
      "    IP address: %s\n",
      response.DestIPAddress.toString().c_str());
    if (response.DestMacAddress != nullptr)
    {
      Serial.printf(
        "    MAC address: " MACSTR "\n",
        MAC2STR(response.DestMacAddress->addr));
    }
    if (response.DestHostname != "")
    {
      Serial.printf(
        "    DNS name: %s\n",
        response.DestHostname.c_str());
    }
    pingAvg = int(response.AvgResponseTime + 0.5); //ensure pingAvg can not be zero as zero will be used to flag no response.
    pingsLost = response.TotalSentRequests - response.TotalReceivedResponses;
    isFinished = true;  //marks all pings to that target completed
    return true;
  });
}

void loop()
{
  if (WiFi.status() == WL_CONNECTED) digitalWrite(dPin[8], 0); else digitalWrite(dPin[8], 1);

  currentMillis = millis();
  if (currentMillis - previousMillis >= interval) {
    Serial.print("now ");
    Serial.print(currentMillis);
    Serial.print(" was ");
    Serial.print(previousMillis);
    previousMillis = currentMillis;
    Serial.println(" Starting 1 set of pings ");
    //ping each target in turn starting at zero
    for(target=0; target<=6; ) PerformPing(target);
    
    if (setComplete = true) target = showLEDs();
  }

}//loop

and the serial output
…Ok
now 10000 was 0 Starting 1 set of pings
target 0 sending ping

Soft WDT reset

stack>>>

ctx: cont
sp: 3ffffdb0 end: 3fffffc0 offset: 01a0
3fffff50: 3ffee650 00000000 00000000 4020131d
3fffff60: 40205488 0101a8c0 3ffe88c2 40202961
3fffff70: 3ffee660 00000019 3ffee69c 40202aec
3fffff80: 3ffee660 3ffee664 3ffee69c 3ffee714
3fffff90: 3ffee660 3ffee664 3ffee65c 4020162b
3fffffa0: 3fffdad0 00000000 3ffee6d4 402035e8
3fffffb0: feefeffe feefeffe 3ffe8548 40100f09
<<<stack<<<

but I’ve been floundering, had issues with the state machine not behaving and letting it progress to printing the output without doing the pings & recording results.

As you will see "IsFinished " is set by the PingerResponse when the data from a set of pings is in.

i don't understand this chunk of code from setup()

it looks like you're defining two functions: pinger.OnReceve() and pinger.OnEnd().

don't understand why you would define functions within a C function and why wouldn't these functions already be defined by what pinger is?

  Serial.print("Ok\n");
  digitalWrite(dPin[8], 0); //indicate wifi connected

  roundComplete = true;  //ready to start a new set of pings

  pinger.OnReceive([](const PingerResponse & response)
  {
    if (response.ReceivedResponse)
    {
      Serial.print("ping response time is "); Serial.print(response.ResponseTime); Serial.println(" msec");
    }
    else
    {
      Serial.printf("Request timed out.\n");
    }

    // Return true to continue the ping sequence. If current event returns false, the ping sequence is interrupted.
    return true;
  });

  pinger.OnEnd([](const PingerResponse & response)
  {
    // Evaluate lost packet percentage
    float loss = 100;
    if (response.TotalReceivedResponses > 0)
    {
      loss = (response.TotalSentRequests - response.TotalReceivedResponses) * 100 / response.TotalSentRequests;
    }

    // Print packet trip data
    Serial.printf(
      "Ping statistics for %s:\n",
      response.DestIPAddress.toString().c_str());
    Serial.printf(
      "    Packets: Sent = %lu, Received = %lu, Lost = %lu (%.2f%% loss),\n",
      response.TotalSentRequests,
      response.TotalReceivedResponses,
      response.TotalSentRequests - response.TotalReceivedResponses,
      loss);
    for(target=0; target<=6; ) PerformPing(target);

the above does not increment target, and therefore is infinite

It also seems you’ve complete removed the outer state machine logic (that you presented back in your original code) and replaced it with the botched for loop…

 for(target=0; target<=6; ) PerformPing(target);

The code is taken (& modified) from this example by the author ESP8266-ping by Alessio Leoncini

target is (I believe) incremented in performPing, so not needed i the for loop?.

isFinished is set in pinger.OnEnd to indicate the set of pings called by this line ...

bool pingOK = pinger.Ping(IPAddress(host1.IP), pingNumber, pingTime);

is complete - so ready to get data and move on to the next set of pings.

Thanks pcbbc, I've tried several approaches but when I put the isFinished in the same routine as the call to ping it seems to screw up the state logic. Works fine in the case structure when its in the next segment. ???

johnerrington:
target is (I believe) incremented in performPing, so not needed i the for loop?

Yes, but ‘target’ is the current state. Therefore the for loop should not needed. Note how your previous supplied switch statement didn’t have a for loop either (unless it was nestled inside one). This is the pitfall of only supplying partial code and asking for advice. People have to make assumptions.

The way this code should work, or at least the way I anticipated it working before you provided the modified version, was that target would stay the same value and you’d repeatedly call PerformPing until that ping operation was complete (presumably when the timeout had expired or isFinished was set true). At which point you’d increment target and loop back around through loop() to do the next ping operation, or display the results when target == 8.

But now you’ve added the for loop into the mix that’s completely broken. It’s also not entirely clear to me if you expect one ping to happen every interval ms and then wait interval ms before doing the next ping, or you expect all ping steps to happen every interval ms one after another?

Really you should break your state machine steps down into as small a units as possible so the main loop() keeps running and doing your other processing. So there’s another reason for not doing the pings one after another with the for loop.

gcjr:
i don’t understand this chunk of code from setup()

it looks like you’re defining two functions: pinger.OnReceve() and pinger.OnEnd().

don’t understand why you would define functions within a C function and why wouldn’t these functions already be defined by what pinger is?

It’s just registering lambda expressions

Thanks pcbbc, the library handles sending a set of pingNumber pings to a single target,
with a timetolive of pingTime msec.

Then "magically " (to me anyway) fires
pinger.OnEnd to produce the results.

The coding of the library is beyond me, I'm not comfortable with OOP.

When all pings to one url are complete I need to progress to the next target.

"Really you should break your state machine steps down into as small a units as possible so the main loop() keeps running and doing your other processing. So there's another reason for not doing the pings one after another with the for loop."

Thanks for your advice, I'll look at it again.

This, incidentally, is not a stack overflow:

Soft WDT reset // from post #6

It is a watch dog timer triggered error. Typically, these occur when the program is in tight loop and other ESP system activities cannot proceed. Using the yield() function in such a loop can help.

Thanks 6v6gt I’ll remember that.

pcbbc: after what you said re the for loop I slept on it and realised I had too many control variables in the machine.
I did what I used to tell my students - simplify the code; and cut down my procedures to mainly just serial prints.
In short its working - and I’m ALMOST back to a switch - case structure. Thanks for your advice.

FYI I’ve put the code below

/

*****************************************************************************
  Arduino library handling ping messages for the esp8266 platform
  MIT License  Copyright (c) 2018 Alessio Leoncini

  Rui Santos: Complete project details at https://randomnerdtutorials.com/esp32-ntp-client-date-time-arduino-ide/
  This requires the  NTP Client library forked by Taranais. https://github.com/taranais/NTPClient

****************************************************************************
*/

#include <Pinger.h>
#include <ESP8266WiFi.h>
#include <core_esp8266_waveform.h>
#include <NTPClient.h>
#include <WiFiUdp.h>

extern "C"
{
#include <lwip/icmp.h> // needed for icmp packet definitions
}

// to do the pin mapping for the Lolin NodeMCU pins D0 - D8
const int dPin[] = {16, 5, 4, 0, 2, 14, 12, 13, 15};

const char* ssid     = "my ssid";
const char* password ="my password";


bool roundComplete, setComplete = 0; //flags all 1 round to a single target,  or all 1 set of pings done

//data for ping:
int pingNumber = 5; //do 5 pings per set
int pingTime = 200; //200ms timeout
byte target = 0; //state machine control and select url to ping

struct host {
  byte IP[4];
  int tPing;
  int nLost;
};

//create data set of type host, &  initialise our struct for all cases
host host1[7] = {
  {{192, 168, 1, 1}, 0, 0 } ,  //0; router
  {{192, 168, 1, 11}, 0, 0 } , //1: RYZEN5
  {{192, 168, 1, 16}, 0, 0 } , //2: ALIVE
  {{192, 168, 1, 22}, 0, 0 } , //3: LP
  {{192, 168, 1, 14}, 0, 0 } , //4: GeoBRDG
  {{192, 168, 1, 24}, 0, 0 } , //5: NAS
  {{107, 162, 133, 62}, 0, 0 } //6: TalkTalk
};

// Define NTP Client to get time
WiFiUDP ntpUDP;
NTPClient timeClient(ntpUDP, "pool.ntp.org");
String formattedDate, myFormattedDate;
String dayStamp;
String timeStamp;
const int nRounds = 43; //number of full rounds to complete before calling ntp
//1 round takes 7 sec. approx; so 1 min = 8.6 rounds; 5 min = 43 rounds
int count = 0; // number of rounds completed:  call ntp and save to SD every 5 min

int pingAvg;
int pingsLost;
bool isFinished;

// Set global to avoid object removing after setup() routine
Pinger pinger;

bool PerformPing()
{
  if (roundComplete)  //ready for next set of pings
  {
    //Serial.print(" target ");     Serial.print(target);     Serial.println(" sending ping ");
    isFinished = false;
    int url = target;
    bool pingOK = pinger.Ping(IPAddress(host1[url].IP), pingNumber, pingTime);
    roundComplete = false;
  }
  else if (isFinished) // one set of pings to a single target complete and results in
  { //Serial.print(" target ");     Serial.print(target);     Serial.println(" values in ");
    isFinished = false;
    host1[target].tPing = pingAvg;
    host1[target].nLost = pingsLost;
    roundComplete = true;
    target++;
    if (target == 7) {
      setComplete = true; //done 1 round to all targets
      //Serial.print(" target ");     Serial.print(target);     Serial.println(" set complete ");
    } else {
      setComplete = false;
    }
  }
  return true;
}

int showLEDs()
{
  //flash LEDs according to number of lost (or good) pings.
  Serial.print("loss values: ");
  for (int n = 0; n <= 6; n++) {
    Serial.print(n); Serial.print(": "); Serial.print(host1[n].nLost); Serial.print(";   ");
    //the lower limit on analogWrite() is 100Hz, but startWaveform() doesn’t have that limit:
    startWaveform(dPin[n], (host1[n].nLost * 50000) + 10, ((5 - host1[n].nLost ) * 50000) + 10, 0); //using lost pings: 0=LED ON, 1-4 = flashing, 5=LED OFF
  }
  Serial.println("");
  target++;
  return target;
}

void getTime()
{
  if (count >= nRounds) { //we have done nRounds of sets of pings to all targets so get date and time
    timeClient.update();
    formattedDate = timeClient.getFormattedDate();
    // Extract date
    int splitT = formattedDate.indexOf("T");
    dayStamp = formattedDate.substring(0, splitT);
    //Serial.println(dayStamp);
    // Extract time
    timeStamp = formattedDate.substring(splitT + 1, formattedDate.length() - 1);
    //Serial.println(timeStamp);
    myFormattedDate = String(dayStamp) + ", " +  String(timeStamp);
    Serial.print("my Formatted Date: ");
    Serial.println(myFormattedDate);
    Serial.println("");
    count = 0;
  } else {
    count++;
  }
  target = 0;
}

void setup()
{
  // Begin serial connection at 74880 baud
  Serial.begin(74880);
  delay(1000); // serial comms is disturbed during a restart

  //set up LED pins as outputs & HIGH (LEDs all off)
  for (int i = 0; i <= 8; i++) {
    pinMode(dPin[i], OUTPUT);
    digitalWrite(dPin[i], 1);
  }

  // Connect to WiFi access point
  digitalWrite(dPin[8], 1); //indicate wifi not connected
  bool stationConnected = WiFi.begin(ssid, password);
  delay(200); //allow time to connect

  // Check if connection errors
  if (!stationConnected)
  {
    Serial.println("Error, unable to connect specified WiFi network.");
  }

  // Wait until connection completed
  Serial.print("Connecting to AP...");
  while (WiFi.status() != WL_CONNECTED)
  {
    delay(200);
    Serial.print(".");
  }
  Serial.print("Ok\n");
  digitalWrite(dPin[8], 0); //indicate wifi connected

  // Initialize a NTPClient to get time
  timeClient.begin();
  // Set offset time in seconds to adjust for your timezone, eg GMT +1 = 3600
  timeClient.setTimeOffset(0);

  roundComplete = true;  //ready to start a new set of pings

  pinger.OnReceive([](const PingerResponse & response)
  {
    return true;  // Return true to continue the ping sequence. If current event returns false, the ping sequence is interrupted.
  });

  pinger.OnEnd([](const PingerResponse & response)
  {
    pingAvg = int(response.AvgResponseTime + 0.5); //ensure pingAvg can not be zero as zero will be used to flag no response.
    pingsLost = response.TotalSentRequests - response.TotalReceivedResponses;
    isFinished = true;  //marks all pings to that target completed
    return true;
  });
}

void loop()
{
  if (WiFi.status() == WL_CONNECTED) digitalWrite(dPin[8], 0); else digitalWrite(dPin[8], 1);

  if (target >= 0 && target <= 6) {
    PerformPing();  //ping each target in turn starting at zero
  }

  if (target == 7) showLEDs();

  if (target == 8) getTime();

}//loop