Lowering amount of arguments in a function?

Hey!

First post, so I’m not sure what to expect.

I have been plotting upon controlling an 8X8 RGB common anode LED matrix using 4 74HC595 shift registers (along with appropriate current limiting resistors and transistors). I most likely understood their functioning, but I have some problems coding.

I’ve already had multiple versions, and already a lot of questions popped up, but I would currently want to focus on one detail: how can I ameliorate my code so that I don’t have too many arguments in my function?

For instance, here is my code which can not be compiled due to the big amount of arguments in Display();
(for clarification, it’s a function I made myself).

void setup() 
{
 int Clock = 11;
 int Data = 12;                      //(11, 12, 13) = pins that control four 74HC595 ic's
 int Latch = 13;
 
 pinMode(Clock, OUTPUT);
 pinMode(Data, OUTPUT);
 pinMode(Latch, OUTPUT);  

}


void loop()
{
 int Clock = 11;
 int Data = 12;             
 int Latch = 13;
 
 int a, b;

 int Anode;

//display a self-coded boat picture

 Display(20,                                 //duration, where 20 is a multiplier of ~1 second

//Red
 B11101000, 
 B11101100,
 B11001110,
 B00001100,
 B00001000,
 B11111111,
 B01111110,
 B00111100,

 //Green
 B11101000,
 B11101100,
 B11001110,
 B00001100,
 B00001000,
 B00000000,
 B00000000,
 B00000000,
 
 //Blue
 B00001000,
 B00001100,
 B00001110,
 B00001100,
 B00001000,
 B00000000,
 B00000000,
 B11000011 );

}

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

void Display(int d, 
             byte R1, byte R2, byte R3, byte R4, byte R5, byte R6, byte R7, byte R8,
             byte G1, byte G2, byte G3, byte G4, byte G5, byte G6, byte G7, byte G8, 
             byte B1, byte B2, byte B3, byte B4, byte B5, byte B6, byte B7, byte B8) 
{
  
 int Clock = 11;
 int Data = 12;             
 int Latch = 13;
 
 int a, b;

 int Anode;

 
 byte RedDisplay[8]= {R1, R2, R3, R4, R5, R6, R7, R8};
                
 byte GreenDisplay[8]={G1, G2, G3, G4, G5, G6, G7, G8}; 
  
 byte BlueDisplay[8]= {B1, B2, B3, B4, B5, B6, B7, B8};
                      
                   
  for(b = 0; b < 125*d; b++)
  {
    for(a = 0; a < 7; a++)
    {
      Anode = (2^a);
    
      digitalWrite(Latch, LOW);
      shiftOut(Data, Clock, MSBFIRST, Anode);
      shiftOut(Data, Clock, LSBFIRST, RedDisplay[a]);
      digitalWrite(Latch, HIGH);

      digitalWrite(Latch, LOW);
      shiftOut(Data, Clock, MSBFIRST, Anode);
      shiftOut(Data, Clock, LSBFIRST, GreenDisplay[a]);
      digitalWrite(Latch, HIGH);
  
      digitalWrite(Latch, LOW);
      shiftOut(Data, Clock, MSBFIRST, Anode);
      shiftOut(Data, Clock, LSBFIRST, BlueDisplay[a]);
      digitalWrite(Latch, HIGH);
    }
  }
}

It would be a great help if you could suggest me how to shrink that amount.

And also, please (if you have the will and energy) comment about any other part of my code which could be done wiser; I have started coding in the arduino IDE not that long ago.

Thank you very much,
Mike

clean_version.ino (2.04 KB)

Learn how Arrays work and how to pass them into functions. Your life will be so much easier.

HazardsMind: Learn how Arrays work and how to pass them into functions. Your life will be so much easier.

I have investigated into arrays but couldn't properly incorporate them into my function (obviously self-teaching has its low points). I will learn more about it, thank you for suggesting and commenting.

Crash course.

byte Red[8] =
{
  B11101000,
  B11101100,
  B11001110,
  B00001100,
  B00001000,
  B11111111,
  B01111110,
  B00111100,
};

byte Green[8] =
{
  B11101000,
  B11101100,
  B11001110,
  B00001100,
  B00001000,
  B00000000,
  B00000000,
  B00000000,
};

byte Blue[8] =
{
  B00001000,
  B00001100,
  B00001110,
  B00001100,
  B00001000,
  B00000000,
  B00000000,
  B11000011
};

void setup() {
  // put your setup code here, to run once:
  Display(10, Red, Green, Blue); // 10 * 100 milliseconds = 1000 milliseconds or 1 second
}

void loop() {
  // put your main code here, to run repeatedly:

}

void Display(unsigned long dur, byte *R, byte *G, byte *B) // pass in the arrays using pointers.
{
  static unsigned long prevTime = millis(); // Do not touch this.

  int Clock = 11;
  int Data = 12;
  int Latch = 13;

  dur *= 100UL; // duration time a tenth of a second. make it 1000UL for a 1 second interval

  int a, b;
  int Anode;

  for (b = 0; b < 8; b++) // cycle through the bytes
  {
    if (millis() - prevTime >= dur) // add delay to the code without blocking anything.
    {
      for (a = 0; a < 8; a++) // cycle through the bits
      {
        //Anode = (2 ^ a); // ^ does not mean 2 to the power a, instead it means xor bit logic
        Anode = (1 << a); // this is an easy way to produce 2 to the power of n

        digitalWrite(Latch, LOW);
        shiftOut(Data, Clock, MSBFIRST, Anode);
        shiftOut(Data, Clock, LSBFIRST, bitRead(R[b], a));// read and return the value of each bit in that particular byte
        digitalWrite(Latch, HIGH);

        digitalWrite(Latch, LOW);
        shiftOut(Data, Clock, MSBFIRST, Anode);
        shiftOut(Data, Clock, LSBFIRST, bitRead(G[b], a));
        digitalWrite(Latch, HIGH);

        digitalWrite(Latch, LOW);
        shiftOut(Data, Clock, MSBFIRST, Anode);
        shiftOut(Data, Clock, LSBFIRST, bitRead(B[b], a));
        digitalWrite(Latch, HIGH);
      }

      prevTime += dur; // update previous time with the duration
    }
  }
}

HazardsMind:
Crash course.

byte Red[8] =

{
 B11101000,
 B11101100,
 B11001110,
 B00001100,
 B00001000,
 B11111111,
 B01111110,
 B00111100,
};

byte Green[8] =
{
 B11101000,
 B11101100,
 B11001110,
 B00001100,
 B00001000,
 B00000000,
 B00000000,
 B00000000,
};

byte Blue[8] =
{
 B00001000,
 B00001100,
 B00001110,
 B00001100,
 B00001000,
 B00000000,
 B00000000,
 B11000011
};

void setup() {
 // put your setup code here, to run once:
 Display(10, Red, Green, Blue); // 10 * 100 milliseconds = 1000 milliseconds or 1 second
}

void loop() {
 // put your main code here, to run repeatedly:

}

void Display(unsigned long dur, byte *R, byte *G, byte *B) // pass in the arrays using pointers.
{
 static unsigned long prevTime = millis(); // Do not touch this.

int Clock = 11;
 int Data = 12;
 int Latch = 13;

dur *= 100UL; // duration time a tenth of a second. make it 1000UL for a 1 second interval

int a, b;
 int Anode;

for (b = 0; b < 8; b++) // cycle through the bytes
 {
   if (millis() - prevTime >= dur) // add delay to the code without blocking anything.
   {
     for (a = 0; a < 8; a++) // cycle through the bits
     {
       //Anode = (2 ^ a); // ^ does not mean 2 to the power a, instead it means xor bit logic
       Anode = (1 << a); // this is an easy way to produce 2 to the power of n

digitalWrite(Latch, LOW);
       shiftOut(Data, Clock, MSBFIRST, Anode);
       shiftOut(Data, Clock, LSBFIRST, bitRead(R[b], a));// read and return the value of each bit in that particular byte
       digitalWrite(Latch, HIGH);

digitalWrite(Latch, LOW);
       shiftOut(Data, Clock, MSBFIRST, Anode);
       shiftOut(Data, Clock, LSBFIRST, bitRead(G[b], a));
       digitalWrite(Latch, HIGH);

digitalWrite(Latch, LOW);
       shiftOut(Data, Clock, MSBFIRST, Anode);
       shiftOut(Data, Clock, LSBFIRST, bitRead(B[b], a));
       digitalWrite(Latch, HIGH);
     }

prevTime += dur; // update previous time with the duration
   }
 }
}

First of all, thank you again, I’m incredibly happy to receive those messages.

Now, if you can answer, I have some questions concerning the code you sent me.

The code looks good and understandable, although those 3 details disturb me:

1- Why is the XOR bit logic necessary? It is reading one bit at a time anyways, not two, so how is declaring such logic helping?

2- Speaking of one bit at a time, doesn’t the shiftOut function shift out one byte at a time? And the bitRead function is returning only one bit at the time, am I correct? Therefore, how can it be compatible?

P.S. I need the whole byte to be displayed in one time, which means that I will need to set into memory all 8 bits before setting the “Latch” pin to HIGH. So if I understood correctly, the code you sent me displays one bit at a time?

3- and last detail - omitting the 2 previous points, this code seems to work just fine when all the bytes are defined. But what if I wish to add multiple “pictures” to be displayed? It will mean that I will need to change the values in “byte Red[8]”, “byte Green[8]” and “byte Blue[8]”. Wouldn’t that be a redefining, which leads the code to be not working? If so, do you know what modifications are necessary so that the values within the byte array could be variable, yet not redefining?

Once again, huge thanks. Please excuse me if some questions are purposeless in your opinion.

Mike

P.S. I need the whole byte to be displayed in one time, which means that I will need to set into memory all 8 bits before setting the "Latch" pin to HIGH. So if I understood correctly, the code you sent me displays one bit at a time?

Yea, I made a mistake with the last function.

1- Why is the XOR bit logic necessary? It is reading one bit at a time anyways, not two, so how is declaring such logic helping?

It's commented out, I was just saying that ^ does not do powers, it does xor bit logic.

2- Speaking of one bit at a time, doesn't the shiftOut function shift out one byte at a time? And the bitRead function is returning only one bit at the time, am I correct? Therefore, how can it be compatible?

Fixed that.

3- and last detail - omitting the 2 previous points, this code seems to work just fine when all the bytes are defined. But what if I wish to add multiple "pictures" to be displayed? It will mean that I will need to change the values in "byte Red[8]", "byte Green[8]" and "byte Blue[8]". Wouldn't that be a redefining, which leads the code to be not working? If so, do you know what modifications are necessary so that the values within the byte array could be variable, yet not redefining?

You would need to use 2D arrays instead and pass them in by index. I'll make you an example.

I changed the code a good bit. now it should show an image then show the next one and reset.

#define NumOfImages 2    // how many images do you have?

byte Red[NumOfImages][8] =
{
  { // image 1
    B11101000,
    B11101100,
    B11001110,
    B00001100,
    B00001000,
    B11111111,
    B01111110,
    B00111100
  }
  ,
  { // image 2 I just added random 1s and 0s
    B11101000,
    B11111110,
    B11001110,
    B01111100,
    B01101000,
    B11111111,
    B01100110,
    B00111100
  }
};

byte Green[NumOfImages][8] =
{
  {
    B11101000,
    B11101100,
    B11001110,
    B00001100,
    B00001000,
    B00000000,
    B00000000,
    B00000000
  }
  ,
  {
    B11101000,
    B11101100,
    B11001110,
    B00001100,
    B00001000,
    B01000010,
    B00111100,
    B00011000
  }
};

byte Blue[NumOfImages][8] =
{
  {
    B00001000,
    B00001100,
    B00001110,
    B00001100,
    B00001000,
    B00000000,
    B00000000,
    B11000011
  }
  ,
  {
    B00001000,
    B01001100,
    B01001110,
    B01001100,
    B01001000,
    B01110000,
    B00001110,
    B11000011
  }
};

// make these global to be seen throughout the code.
const byte Clock = 11;  // const means they wont change and byte because they will never go beyond 255
const byte Data = 12;                      //(11, 12, 13) = pins that control four 74HC595 ic's
const byte Latch = 13;

unsigned long durations[NumOfImages] = {10, 20};

void setup()
{
  // put your setup code here, to run once
  pinMode(Clock, OUTPUT);
  pinMode(Data, OUTPUT);
  pinMode(Latch, OUTPUT);
}

void loop()
{
  // put your main code here, to run repeatedly:
  static byte i = 0; // show the first image

  if (Display(durations[i], Red[i], Green[i], Blue[i])) // will return true when the duration set has been reached. 
  {
    i++; // when the duration has been reached, show the next image
    if (i > (NumOfImages - 1)) // Check bounds, you don't want to show what you don't have.  -1 because it needs to start from 0.
      i = 0;
  }
}

byte Display(unsigned long dur, byte *R, byte *G, byte *B) // pass in the arrays using pointers.
{
  static unsigned long prevTime = millis(); // Do not touch this.

  dur *= 100UL; // duration time a tenth of a second. make it 1000UL for a 1 second interval

  int b;
  int Anode;

  for (b = 0; b < 8; b++) // cycle through the bytes
  {
    Anode = (1 << b); // this is an easy way to produce 2 to the power of n

    digitalWrite(Latch, LOW);
    shiftOut(Data, Clock, MSBFIRST, Anode);
    shiftOut(Data, Clock, LSBFIRST, R[b]);// read and return the value of each bit in that particular byte
    digitalWrite(Latch, HIGH);

    digitalWrite(Latch, LOW);
    shiftOut(Data, Clock, MSBFIRST, Anode);
    shiftOut(Data, Clock, LSBFIRST, G[b]);
    digitalWrite(Latch, HIGH);

    digitalWrite(Latch, LOW);
    shiftOut(Data, Clock, MSBFIRST, Anode);
    shiftOut(Data, Clock, LSBFIRST, B[b]);
    digitalWrite(Latch, HIGH);
  }

  if (millis() - prevTime >= dur) // add delay to the code without blocking anything.
  {
    prevTime += dur; // update previous time with the duration
    return true; // this image has shown for your duration and now you can show the next one.
  }
  return false;
}

HazardsMind: It's commented out, I was just saying that ^ does not do powers, it does xor bit logic.

I'm sorry, I managed to not notice the double slash and therefore I misunderstood. Thanks for letting me know about the " ^ 2 "! I was not aware of this.

Fixed that.

Thank you very much. :)

You would need to use 2D arrays instead and pass them in by index. I'll make you an example.

Thank you so much! :D

I changed the code a good bit. now it should show an image then show the next one and reset.

#define NumOfImages 2    // how many images do you have?

byte Red[NumOfImages][8] = {  { // image 1    B11101000,    B11101100,    B11001110,    B00001100,    B00001000,    B11111111,    B01111110,    B00111100  }  ,  { // image 2 I just added random 1s and 0s    B11101000,    B11111110,    B11001110,    B01111100,    B01101000,    B11111111,    B01100110,    B00111100  } };

byte Green[NumOfImages][8] = {  {    B11101000,    B11101100,    B11001110,    B00001100,    B00001000,    B00000000,    B00000000,    B00000000  }  ,  {    B11101000,    B11101100,    B11001110,    B00001100,    B00001000,    B01000010,    B00111100,    B00011000  } };

byte Blue[NumOfImages][8] = {  {    B00001000,    B00001100,    B00001110,    B00001100,    B00001000,    B00000000,    B00000000,    B11000011  }  ,  {    B00001000,    B01001100,    B01001110,    B01001100,    B01001000,    B01110000,    B00001110,    B11000011  } };

// make these global to be seen throughout the code. const byte Clock = 11;  // const means they wont change and byte because they will never go beyond 255 const byte Data = 12;                      //(11, 12, 13) = pins that control four 74HC595 ic's const byte Latch = 13;

unsigned long durations[NumOfImages] = {10, 20};

void setup() {  // put your setup code here, to run once  pinMode(Clock, OUTPUT);  pinMode(Data, OUTPUT);  pinMode(Latch, OUTPUT); }

void loop() {  // put your main code here, to run repeatedly:  static byte i = 0; // show the first image

 if (Display(durations[i], Red[i], Green[i], Blue[i])) // will return true when the duration set has been reached.  {    i++; // when the duration has been reached, show the next image    if (i > (NumOfImages - 1)) // Check bounds, you don't want to show what you don't have.  -1 because it needs to start from 0.      i = 0;  } }

byte Display(unsigned long dur, byte *R, byte *G, byte *B) // pass in the arrays using pointers. {  static unsigned long prevTime = millis(); // Do not touch this.

 dur *= 100UL; // duration time a tenth of a second. make it 1000UL for a 1 second interval

 int b;  int Anode;

 for (b = 0; b < 8; b++) // cycle through the bytes  {    Anode = (1 << b); // this is an easy way to produce 2 to the power of n

   digitalWrite(Latch, LOW);    shiftOut(Data, Clock, MSBFIRST, Anode);    shiftOut(Data, Clock, LSBFIRST, R[b]);// read and return the value of each bit in that particular byte    digitalWrite(Latch, HIGH);

   digitalWrite(Latch, LOW);    shiftOut(Data, Clock, MSBFIRST, Anode);    shiftOut(Data, Clock, LSBFIRST, G[b]);    digitalWrite(Latch, HIGH);

   digitalWrite(Latch, LOW);    shiftOut(Data, Clock, MSBFIRST, Anode);    shiftOut(Data, Clock, LSBFIRST, B[b]);    digitalWrite(Latch, HIGH);  }

 if (millis() - prevTime >= dur) // add delay to the code without blocking anything.  {    prevTime += dur; // update previous time with the duration    return true; // this image has shown for your duration and now you can show the next one.  }  return false; }

Wow, thank you. I will definitely try this code out! I won't even change the random bytes you entered, I'm intrigued by how creative your randomness is!

Honestly, I didn't expect so much help, it's grandly appreciated. A lot of details in your code are components I have never used before and so I will need to go over it a few times before fully comprehending. Yet, your help explained a lot to me, therefore I was wondering if you could explain me 2 last things:

1- Where does "durations*" come from? "durations" wasn't declared anywhere... Or was it?* ``` *  if (Display(durations[i], Red[i], Green[i], Blue[i])) // will return true when the duration set has been reached.* ``` 2- What is the purpose of "return false" at the end? Thank you Mike

1- Where does “durations*” come from? “durations” wasn’t declared anywhere… Or was it? *

It was declared above setup()

2- What is the purpose of “return false” at the end?

That function lets you know when it has finished sending out the bytes for that particular image. It returns true when the whole image was sent and false when it is not done.