Confused about if-else and break statements

With the following code, both ‘‘second-layer’’-else statements will get ignored. If xval1 < 500 is TRUE, then regardless of the xval3 value it will always print ‘‘status: ping’’. If xval1 >= 500 is TRUE, it will always print ‘‘status: tilt alert2’’. I suspect it has to do something with the break statements, but changing it up doesn’t seem to help at all.

case DEFINE: 
    if (xval1 < 500) {
        if (xval3 < 50 && xval3 > -50) {
            strncpy(eventStatus, "Ping", sizeof(eventStatus)-1);
            Serial.println("status: ping");
            state = PUBLISH;
            break;               
            }
        else {
            strncpy(eventStatus, "Tilt alert", sizeof(eventStatus)-1);
            Serial.println("status: tilt alert1");
            state = PUBLISH;
            break;
            }
        }
    else {
        if (xval3 < 50 && xval3 > -50) {
            strncpy(eventStatus, "Tilt alert", sizeof(eventStatus)-1);
            Serial.println("status: tilt alert2");
            state = PUBLISH;
            break;
            }
        else {
            strncpy(eventStatus, "Collection", sizeof(eventStatus)-1);
            Serial.println("status: collection");
            state = PUBLISH;
            break;                
            }
        }

It’s hard to tell if you need the case here without knowing the context. Also, if you do your if-elses correctly you don’t need the breaks.

it’s part of a switch(state). Basically I’m gathering some data from a sensor, and then based on these values I want to attach a certain status to it for a client. Code works without issues, but it assigns the wrong status repeatedly.

Tested without any break statements at all. Still doesnt work.

@Vitesze, how is xval3 defined?

In C/C++, the switch case statement has “fall-through” behavior–that is after one case is done, the next case in line will be executed unless you end each case section with a break statement.

I would have written this code slightly differently since every path out of the case needs a break, I would just put a break statement after the if/then/else in each case.

2 Likes

int xval3 = std::abs(xval1-xval2);

xval1 and xval2 are also int-variables. Printing xval3 does give the value i expect, but I see this is just plain wrong now.

@Vitesze using abs(), the absolute value returned will never be negative, by definition. As such, your if (xval3 < 50 && xval3 > -50) may not be doing what you might want it to do.

Have you actually printed out xval3? You would have seen that it never went negative. Have you verified that the values of xval1 and xval2 are valid?

I print out all values and they always seem accurate. Yeah I suppose I didn’t think clearly when I did the -50 part, as I added in abs() just for the sake of not having to deal with negative values. Shouldn’t cause any issues though

When printing just now i got:
xval1 = 38
xval2 = 975
xval3 = 937
Result is status: ping

While it should be tilt alert1. I’m not sure what’s wrong; xval1,2,3 are all defined as integers and printing values goes fine

@Vitesze, change the if statement to `if (xval3 < 50) for both cases and try again.

Tried it, and still doesn’t work.
xval1: 48
xval2: 1027
xval3: 979

if (xval3 <50 {
}
else {
}

it still follows the if-statement as if xval3 was below 50

Are the parenthesis OK in your actual code? They are not OK in your snippet above.

Maybe you should could write out in words what you are trying to do? Sometimes code is not the way to see your logic clearly.

First take a measurement with the accelerometer, which will be xval2. Then calculate the difference between xval1 and 2, and call it xval3 (which is always positive).

After that, depending on xval1 and 3 values, it should receive one of four statusses.

case ANGLE:
    if (xval2 == 0) { 
        xl.readXYZTData(XValue, YValue, ZValue, Temperature);
        xval2 = (XValue);
      Serial.print("xval2=");
        Serial.println(xval2);
        break;
        }
    else {
        int xval3 = std::abs(xval1-xval2);
        Serial.print("xval3=");
        Serial.println(xval3);
        state = DEFINE;
        break;
        }
        
case DEFINE: 
    if (xval1 < 500) {
        if (xval3 < 50) {
            strncpy(eventStatus, "Ping", sizeof(eventStatus)-1);
            Serial.println("status: ping");
            state = PUBLISH;
            break;
            }
        else {
            strncpy(eventStatus, "Tilt alert", sizeof(eventStatus)-1);
            Serial.println("status: tilt alert1");
            state = PUBLISH;
            break;
            }
        }
        
    else {
        if (xval3 < 50) {
            strncpy(eventStatus, "Tilt alert", sizeof(eventStatus)-1);
            Serial.println("status: tilt alert2");
            state = PUBLISH;
            break;
            }
        else {
            strncpy(eventStatus, "Collection", sizeof(eventStatus)-1);
            Serial.println("status: collection");
            state = PUBLISH;
            break;
            }
        }
        break;

Well that looks weird. You are defining xval3 inside the else clause curly braces, so that is not available to the other clauses. Is there another declaration for xval3 somewhere?

The value of xval3 you set up in the ELSE of case ANGLE is not available to the IF statements in case DEFINE.

Maybe you didn’t mean to redefine it and should remove the int part?

1 Like

Really? I didn’t know about that.There is no declaration of xval3 anywhere else, but I figured that defining it in an ELSE clause should still make it available to the other cases? How would I be declaring xval2 and 3 here then? Without any if-else statements?

Just declare it above where xval1 and 2 are declared and make sure it has the value you want (via assignment like xval3 = std::abs(xval1-xval2); or whatever) before you use it.

Thanks, did it as follow and it worked. Although I still don’t fully understand why xval3 wasn’t preserved when in the else-statement.

case ANGLE:
    if (xval2 == 0) { 
        xl.readXYZTData(XValue, YValue, ZValue, Temperature);
        xval2 = (XValue);
      Serial.print("xval2=");
        Serial.println(xval2);
        break;
        }
    else {
        state = DEFINE;
        break;
        }
        
case DEFINE: {
    int xval3 = std::abs(xval1-xval2);
    Serial.print("xval3=");
    Serial.println(xval3);
    if (xval1 < 500) {
        if (xval3 < 50) {
            strncpy(eventStatus, "Ping", sizeof(eventStatus)-1);
            Serial.println("status: ping");
            state = PUBLISH;
            break;
            }
        else {
            strncpy(eventStatus, "Tilt alert", sizeof(eventStatus)-1);
            Serial.println("status: tilt alert1");
            state = PUBLISH;
            break;
            }
        }
        
    else {
        if (xval3 < 50) {
            strncpy(eventStatus, "Tilt alert", sizeof(eventStatus)-1);
            Serial.println("status: tilt alert2");
            state = PUBLISH;
            break;
            }
        else {
            strncpy(eventStatus, "Collection", sizeof(eventStatus)-1);
            Serial.println("status: collection");
            state = PUBLISH;
            break;
            }
        }
    }
    break;

case PUBLISH: {
  if (Particle.connected()) {
      char data[256];
      float stateOfCharge = batteryMonitor.getSoC();
      snprintf(data, sizeof(data), "{ \"did\": \"%s\", \"soc\": \"%.02f\", \"stt\": \"%s\", \"cid\": \"%s\" }", deviceName, stateOfCharge, eventStatus, cid);
      Serial.println(data);
      stateTime = millis();
      state = CONFIG;
      break;
        }
    else {
        if(millis() - stateTime >= MAX_TIME_TO_PUBLISH_MS) {
            state = CONFIG;
            break;
            }
        }
    }
    break;
1 Like

Just on an unrelated topic, is there any harm in adding a lot of cases in a switch(state)? Should they be made as compact as possible or is it totally fine to have as many as you want (provided the code is ok)?

There is no harm in added cases to a switch statement. It gets hard to read and maintain after about 7 or 8 for me. If you are making state machines (which is what I use switch/case for) then you can think about having a function for each case instead of inline code and have those functions return the next state you want to go to and do what ever other work needed to be done:

switch (currentState) {
  case RESET:
      nextState = ResetFunction();
      break;
  case FIRSTTHING:
      nextState = FirstThingFunction();
      break;
  ...
}
currentState = nextState;

Ok got it, Thanks a bunch!

xval3 wasn’t preserved because when you hit your DEFINE case, you were in a new loop, and it didn’t get defined.

When you are in the ANGLE case, once you hit a break statement, you completely exit the switch{}. I think your mistake might have been thinking that since you just reset the state to DEFINE, that it would just hit the case DEFINE immediately after and execute from there. But that’s not how switch/case/break works.