Pages: [1]   Go Down
Author Topic: ternary operator using HIGH and LOW  (Read 790 times)
0 Members and 1 Guest are viewing this topic.
Offline Offline
Full Member
***
Karma: 3
Posts: 188
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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?

Code:
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);
  }
}
« Last Edit: March 03, 2013, 11:03:55 pm by hilukasz » Logged

for(i = 0, i < 820480075, i++){ Design(); Code(); delay(1000); } // hellowoo.com

California
Offline Offline
Faraday Member
**
Karma: 88
Posts: 3380
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
  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.
Logged

Rome, Italy
Offline Offline
Sr. Member
****
Karma: 20
Posts: 442
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
« Last Edit: March 03, 2013, 11:10:20 pm by spatula » Logged

Offline Offline
God Member
*****
Karma: 17
Posts: 765
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Offline Offline
Full Member
***
Karma: 3
Posts: 188
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
  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.
Logged

for(i = 0, i < 820480075, i++){ Design(); Code(); delay(1000); } // hellowoo.com

Offline Offline
Full Member
***
Karma: 3
Posts: 188
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

for(i = 0, i < 820480075, i++){ Design(); Code(); delay(1000); } // hellowoo.com

Offline Offline
Full Member
***
Karma: 3
Posts: 188
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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:

Code:
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 smiley
« Last Edit: March 03, 2013, 11:16:32 pm by hilukasz » Logged

for(i = 0, i < 820480075, i++){ Design(); Code(); delay(1000); } // hellowoo.com

Rome, Italy
Offline Offline
Sr. Member
****
Karma: 20
Posts: 442
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

[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).
« Last Edit: March 03, 2013, 11:27:22 pm by spatula » Logged

Offline Offline
Faraday Member
**
Karma: 62
Posts: 3013
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Code:
  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.

Code:
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 )


Logged

California
Offline Offline
Faraday Member
**
Karma: 88
Posts: 3380
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

East Anglia (UK)
Offline Offline
Faraday Member
**
Karma: 114
Posts: 4255
May all of your blinks be without delay()
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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 smiley
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
Code:
    isHighLow = !isHighLow;
Logged

Please do not send me PMs asking for help.  Post in the forum then everyone will benefit from seeing the questions and answers.

Pages: [1]   Go Up
Jump to: