ternary operator using HIGH and LOW

Projects Programming
1

trying to learn how to use ternary operators to refactor some code, but having some issues. This code lights up all the LEDs but they wont go low after they go high. It should light them all up one by one, then turn them all off a few times. Any ideas where I am going wrong?

void cycleLEDs(){
  const int setupLedSpeed = 80;
  
  boolean isHighLow = HIGH;
  boolean HighLowToggle = (isHighLow == HIGH)? HIGH : LOW;
 
  for(int i = 0; i < 10; i++){
    digitalWrite(LEDone, HighLowToggle);
    delay(setupLedSpeed);
    digitalWrite(LEDtwo, HighLowToggle);
    delay(setupLedSpeed);
    digitalWrite(LEDthree, HighLowToggle);
    delay(setupLedSpeed);
  }
}
2 
  boolean isHighLow = HIGH;
  boolean HighLowToggle = (isHighLow == HIGH)? HIGH : LOW;

Hopefully the ternery operator is being used as an academic exercise, because in this context it's pointless. In fact, those two lines can be replaced by simply assigning HighLowToggle to HIGH. You need a persistent variable, like a static or global.

3

Just guessing... Is the code that turns the LEDs HIGH somewhere else? Here I see that you assign a value HIGH to isHighLow, and immediately test for that value, which results in setting HighLowToggle to LOW. I see no way you can light them on within this function.

Also note, for precision, that HIGH and LOW are defined an integers, not booleans.

[added] This comment was based on code that perhaps I only saw in my dreams, where the values HIGH and LOW were inverted. That didn't work either.

5

Arrch:

  boolean isHighLow = HIGH;

boolean HighLowToggle = (isHighLow == HIGH)? HIGH : LOW;




Hopefully the ternery operator is being used as an academic exercise, because in this context it's pointless. In fact, those two lines can be replaced by simply assigning HighLowToggle to HIGH. You need a persistent variable, like a static or global.

yes just trying to learn how to use this by refactoring some messy code. I hear to stay away from Globals, so am trying to avoid learning bad habits.

6

Delta_G:
You are setting HighLowToggle to be HIGH in the comparison. But then you never change the variable isHighLow. So every time that comparison gets run isHighLow is still HIGH so the comparison is true and HighLowToggle gets set to HIGH. Make isHighLow a static variable so it doesn't get reinitialized to HIGH every time you call the function and then include a line somewhere that toggles the variable isHighLow and you would be there.

ok I think I see what you are saying. I will try to work this in. thank you.

7

Delta_G, this totally fixed it! silly me, I see what was happening now. I may be using this improperly, but this seems to work:

void cycleLEDs(){
  const int setupLedSpeed = 80;
  
  int isHighLow = LOW;

  for(int i = 0; i < 10; i++){
    digitalWrite(led[0], isHighLow);
    delay(setupLedSpeed);
    digitalWrite(led[1], isHighLow);
    delay(setupLedSpeed);
    digitalWrite(led[2], isHighLow);
    delay(setupLedSpeed);
    isHighLow = (isHighLow == HIGH)? LOW : HIGH;
  }
}

working on refactoring the rest too so I dont have so much redundant digitalWrites/delays :slight_smile:

8

[edit] Yet another comment made obsolete by new code. Please disregard.

As an exercise for the ternary operator the syntax is correct, but the logic is flawed.
If you want to do everything in that function, i.e. switch them ON and then OFF, you don't need any 'if' or 'for', just set them to HIGH, add some delay(), turn them LOW one at a time with some delay() within.
If you want to change the previous state, then you cannot initialize the state isHighLow within the function, because that would repeat the same logic every time the function is called. You need to define the state either as a global variable, defined outside any function, or as a static variable, which maintains its value across different function calls. In both cases you don't need a 'for' loop (once they are LOW, the 'for' loop is not going to change their state).

9

HIGH and LOW are supposed to represent the states of a digital input or output pin, and it is rather
unwise to conflate that with the boolean/bool/logical type TRUE and FALSE.

Even if this coincidentally happens to work on some particular hardware ( because those keywords happen
to be replaced by 0 and 1 , for example ), it's still a bad idea.

So where you say

  boolean isHighLow = HIGH;
  boolean HighLowToggle = (isHighLow == HIGH)? HIGH : LOW;

Your first line is wrong, because "HIGH" isn't really a proper keyword for the state of a boolean variable.
Even if this compiler lets you do it.

Your second line is wrong for the same reason.

And if you have some boolean variable and you want to use the so-called "ternary operator", there is no need
to check the equality of a boolean variable to anything, the boolean variable itself represents the boolean state.

boolean select=TRUE ;

int value = (select)?3:4 ;    

int another_value = ( select == TRUE )?3:4  ;       //  the equality test is unnecessary and pointless here ( except in Ada, perhaps )
10

hilukasz:
I hear to stay away from Globals, so am trying to avoid learning bad habits.

Whoever told you that was wrong; globals have their place.

11

hilukasz:
Delta_G, this totally fixed it! silly me, I see what was happening now. I may be using this improperly, but this seems to work:

working on refactoring the rest too so I dont have so much redundant digitalWrites/delays :slight_smile:

Look at the index numbers for the led array. See a pattern ?
Looks like a case for a for loop to me, and why not just

    isHighLow = !isHighLow;