How to simplify this easy function

Hello , can someone help me with my thought ? I have this function which i want to simplify...

void leds_off(String row){

  if (row == "left") { 
  
	  digitalWrite(left_Pin, LOW);

  }
  else if (row == "right") { 
  
	  digitalWrite(right_Pin, LOW);
  }
  else if (row == "center") { 
  
	  digitalWrite(center_Pin, LOW);
  }
}

I thought it would be great to get rid of the "if" statement and change the name o left_Pin,right_Pin,center_Pin dynamically somehow...

I know its wrong but i want to give you an idea of what i want to do !

void leds_off(String row){
   
	  digitalWrite(row + _Pin, LOW);
}

Thanks !

That's not how to compare strings, == doesn't look at the string, just its address.

void leds_off(String row){
  if (strcmp (row, "left") == 0){ 
	  digitalWrite(left_Pin, LOW);
  }
  else if (strcmp (row, "right") == 0) { 
	  digitalWrite(right_Pin, LOW);
  }
  else if (strcmp (row, "center") == 0) { 
	  digitalWrite(center_Pin, LOW);
  }
}

What you want to do is create a variable name from a string at runtime, but strings
are often a poor choice to encode information for a machine (unless that machine
understands English!).

void leds_off(String row){
	  digitalWrite(row + _Pin, LOW);
}

cannot work as the compiler requires a variable to be named at compile time.

The whole thing would be better with compile-time names rather than runtime strings - only
numbers are needed at runtime and arrays can be used to map abstract numbers to actual pin numbers
for instance:

#define LEFT 0   // define a contiguous set of  names that number the possibilities from zero (or use enum type)
#define CENTER 1
#define RIGHT 2

#define LEFT_PIN 7        // or whatever - no need to be in any order
#define CENTER_PIN 12
#define RIGHT_PIN 14

byte ledpin_for_row [] = { LEFT_PIN, CENTER_PIN, RIGHT_PIN } ;  // array of pin numbers

void leds_off (byte rownum)
{
  digitalWrite (ledpin_for_row [rownum], LOW) ;
}

...
  leds_off (LEFT) ;    // no need for any strings at all, all the names are handled at compile time...

Wow, Nice job. Arrays are powerful things that I need to get more familiar with...

And this method doesn't waste any additional memory...

#define leds_off( row ) digitalWrite( Pin_##row, LOW )
..
leds_off( left );

For technical limitations, it is required that you change left_Pin etc, to Pin_left, etc.

But if you really need to use strings, then you have no other choice than using your original code or strcmp's.

Use enumerated types, not strings.

That's not how to compare strings, == doesn't look at the string, just its address.

But the object of the comparison is not a string, it is a String.
I don't like them, but that's the correct way to compare them.

Ah, String type - even worse, use up all the RAM on your microcontroller...

Thank you all it was very helpfull!!! i wil go with MarkT example

i wil go with MarkT example

Until the String class bites you in the ass. Then, maybe you'll get rid of something that you do NOT need. Your choice.

invader7:
Hello , can someone help me with my thought ? I have this function which i want to simplify...

If you use arrays, the code becomes so trivial that it is not worth defining a function. Just switch the pin off directly, like this:

digitalWrite(pins[row], LOW);

The variable pins is an array that you will need to declare containing the pin numbers for each row. You will also find the array enables you to initialise all the pins using a simple 'for' loop instead of copying the code for each pin.

This does require you to know the 'row number' rather than a string, and since you haven't shown us the rest of your code I can't advise you how to do that, but you would have to be working on an extremely unusual problem for string-based pin naming to be a sensible solution and I think it's very likely that numbering your pins (rather than naming them) is what you need.

I replaced strings and now i use an array

void leds_off(int led_row){
   
	for(int i = 0; i <  8; i++){ 
		if (led_row == LEDS_L) { leds_left[i] = LOW; } 
		else if (led_row == LEDS_R) { leds_right[i] = LOW; }
		else if (led_row == LEDS_C) { leds_center[i] = LOW; }
	} 
	toggle_leds(led_row);
  
  
} 

void toggle_leds(int led_row){

	  led_row = pin_of[led_row];
	  
	  digitalWrite(led_row+1, LOW);

	  for(int i = 0; i <  e; i++){
		digitalWrite(led_row+2, LOW);

		int val = leds_left[i];

		digitalWrite(led_row, val);
		digitalWrite(led_row+2, HIGH);

	  }
	  digitalWrite(led_row+1, HIGH);
}

also used...
#Define LEDS_L 4
#Define LEDS_R 5
#Define LEDS_C 6

and pin_of is an array...

EDIT: aaaah just found out that int val = leds_left*; has to be replaced with an if statement , to know "which" of left right or center leds should it use...*

invader7:
also used...
#Define LEDS_L 4
#Define LEDS_R 5
#Define LEDS_C 6

Did it compile OK?

invader7:
I replaced strings and now i use an array

That code does far, far more than your original example and the logic looks very dodgy. Perhaps you ought to stop and explain exactly what behaviour you are trying to produce. How many output pins are you trying to control, and what exactly are you trying to make them do?

Just curious, don't you need to turn off whatever LED was on before you turn on something else?

No... :wink: its define instead of Define !

liudr:
Just curious, don't you need to turn off whatever LED was on before you turn on something else?

Good comment ! Yes i turn off all leds and then i light which i want... I took this example http://bildr.org/2011/02/74hc595/ and tried to work with the code

int SER_Pin = 8;   //pin 14 on the 75HC595
int RCLK_Pin = 9;  //pin 12 on the 75HC595
int SRCLK_Pin = 10; //pin 11 on the 75HC595

//How many of the shift registers - change this
#define number_of_74hc595s 1 

//do not touch
#define numOfRegisterPins number_of_74hc595s * 8

boolean registers[numOfRegisterPins];

void setup(){
  pinMode(SER_Pin, OUTPUT);
  pinMode(RCLK_Pin, OUTPUT);
  pinMode(SRCLK_Pin, OUTPUT);

  //reset all register pins
  clearRegisters();
  writeRegisters();
}               

//set all register pins to LOW
void clearRegisters(){
  for(int i = numOfRegisterPins - 1; i >=  0; i--){
     registers[i] = LOW;
  }
} 

//Set and display registers
//Only call AFTER all values are set how you would like (slow otherwise)
void writeRegisters(){

  digitalWrite(RCLK_Pin, LOW);

  for(int i = numOfRegisterPins - 1; i >=  0; i--){
    digitalWrite(SRCLK_Pin, LOW);

    int val = registers[i];

    digitalWrite(SER_Pin, val);
    digitalWrite(SRCLK_Pin, HIGH);

  }
  digitalWrite(RCLK_Pin, HIGH);

}

//set an individual pin HIGH or LOW
void setRegisterPin(int index, int value){
  registers[index] = value;
}

void loop(){

  setRegisterPin(2, HIGH);
  setRegisterPin(3, HIGH);
  setRegisterPin(4, LOW);
  setRegisterPin(5, HIGH);
  setRegisterPin(7, HIGH);

  writeRegisters();  //MUST BE CALLED TO DISPLAY CHANGES
  //Only call once after the values are set how you need.
}

so my code is....

void leds_off(int led_row){
   
	for(int i = 0; i <  8; i++){ 
		if (led_row == LEDS_L) { leds_left[i] = LOW; } 
		else if (led_row == LEDS_R) { leds_right[i] = LOW; }
	} 
	toggle_leds(led_row);
  
  
} 

void toggle_leds(int led_row){

	  led_row = pin_of[led_row];
	  
	  digitalWrite(led_row+1, LOW);

	  for(int i = 0; i <  8; i++){
		digitalWrite(led_row+2, LOW);

		int val = leds_left[i]; //here i have to put an if staement... I know how to fix it

		digitalWrite(led_row, val);
		digitalWrite(led_row+2, HIGH);

	  }
	  digitalWrite(led_row+1, HIGH);
}

void setRegisterPin(int index, int value, int led_row){

  if (led_row == LEDS_L) { leds_left[index] = value; }
  else if (led_row == LEDS_R) { leds_right[index] = value; }
  
  toggle_leds(led_row);
}

void loop(){

leds_off(LEDS_L);
leds_off(LEDS_R);

setRegisterPin(led_row, HIGH , LEDS_L);
setRegisterPin(led_row, HIGH , LEDS_R);
}
//do not touch
#define numOfRegisterPins number_of_74hc595s * 8

You really should touch it :

//do not touch
#define numOfRegisterPins (number_of_74hc595s * 8)

AWOL:

//do not touch

#define numOfRegisterPins number_of_74hc595s * 8



You really should touch it :


//do not touch
#define numOfRegisterPins (number_of_74hc595s * 8)


8)

I didn't use it :slight_smile: i work with 1 chip :slight_smile: so its 8 from the start for me. thanks anyway !